commit d1befaf9fd97cb5756d697b742eb4ac7735b828d
parent 57e9f9c44675f70d190ea4eeb44bd4de749c5b73
Author: Vincent Forest <vincent.forest@meso-star.com>
Date: Mon, 22 Feb 2021 10:37:36 +0100
Fix MPI execution
Output buffer is not allocated on non master process while the
htrdr_draw_map function queried it to retrieve image layout leading to
assertion/invalid memory read.
This commit adds to the htrdr_draw_map_args data structure the
'buffer_layout' member variable defining the layout of the output image
in all situations (i.e. for master and non master processes). It also
updates the htrdr_buffer_create signature that now takes a
htrdr_buffer_layout input argument rather than a set of separated
variables that actually defines this layout.
Diffstat:
7 files changed, 111 insertions(+), 97 deletions(-)
diff --git a/src/atmosphere/htrdr_atmosphere.c b/src/atmosphere/htrdr_atmosphere.c
@@ -389,21 +389,24 @@ htrdr_atmosphere_create
if(res != RES_OK) goto error;
}
- /* Create the image buffer only on the master process; the image parts
- * rendered by the processes are gathered onto the master process. */
- if(!cmd->dump_volumetric_acceleration_structure
- && htrdr_get_mpi_rank(htrdr) == 0) {
+
+ if(!cmd->dump_volumetric_acceleration_structure) {
struct atmosphere_pixel_format pixfmt = ATMOSPHERE_PIXEL_FORMAT_NULL;
atmosphere_get_pixel_format(cmd, &pixfmt);
- res = htrdr_buffer_create(htrdr,
- args->image.definition[0], /* Width */
- args->image.definition[1], /* Height */
- args->image.definition[0] * pixfmt.size, /* Pitch */
- pixfmt.size, /* Size of a pixel */
- pixfmt.alignment, /* Alignment of a pixel */
- &cmd->buf);
- if(res != RES_OK) goto error;
+ /* Setup the buffer layout */
+ cmd->buf_layout.width = args->image.definition[0];
+ cmd->buf_layout.height = args->image.definition[1];
+ cmd->buf_layout.pitch = args->image.definition[0] * pixfmt.size;
+ cmd->buf_layout.elmt_size = pixfmt.size;
+ cmd->buf_layout.alignment = pixfmt.alignment;
+
+ /* Create the image buffer only on the master process; the image parts
+ * rendered by the others processes are gathered onto the master process */
+ if(htrdr_get_mpi_rank(htrdr) == 0) {
+ res = htrdr_buffer_create(htrdr, &cmd->buf_layout, &cmd->buf);
+ if(res != RES_OK) goto error;
+ }
}
exit:
diff --git a/src/atmosphere/htrdr_atmosphere_c.h b/src/atmosphere/htrdr_atmosphere_c.h
@@ -19,6 +19,7 @@
#define HTRDR_ATMOSPHERE_C_H
#include "core/htrdr_accum.h"
+#include "core/htrdr_buffer.h"
#include "core/htrdr_sensor.h"
#include "core/htrdr_spectral.h"
@@ -99,7 +100,9 @@ struct htrdr_atmosphere {
struct htrdr_ran_wlen* ran_wlen;
struct htrdr_sensor sensor;
- struct htrdr_buffer* buf;
+
+ struct htrdr_buffer_layout buf_layout;
+ struct htrdr_buffer* buf; /* NULL on non master processes */
struct htsky* sky;
const char* sky_mtl_name;
diff --git a/src/atmosphere/htrdr_atmosphere_draw_map.c b/src/atmosphere/htrdr_atmosphere_draw_map.c
@@ -435,6 +435,7 @@ setup_draw_map_args_rectangle
ASSERT(cmd && cmd->sensor.type == HTRDR_SENSOR_RECTANGLE && args);
*args = HTRDR_DRAW_MAP_ARGS_NULL;
args->draw_pixel = draw_pixel_flux;
+ args->buffer_layout = cmd->buf_layout;
args->spp = cmd->spp;
args->context = cmd;
}
@@ -447,6 +448,7 @@ setup_draw_map_args_camera
ASSERT(cmd && cmd->sensor.type == HTRDR_SENSOR_CAMERA && args);
*args = HTRDR_DRAW_MAP_ARGS_NULL;
+ args->buffer_layout = cmd->buf_layout;
args->spp = cmd->spp;
args->context = cmd;
diff --git a/src/core/htrdr_buffer.c b/src/core/htrdr_buffer.c
@@ -24,14 +24,9 @@
#include <rsys/ref_count.h>
struct htrdr_buffer {
+ struct htrdr_buffer_layout layout;
char* mem;
- size_t width;
- size_t height;
- size_t pitch;
- size_t elmtsz;
- size_t align;
-
struct htrdr* htrdr;
ref_T ref;
};
@@ -58,42 +53,16 @@ buffer_release(ref_T* ref)
res_T
htrdr_buffer_create
(struct htrdr* htrdr,
- const size_t width,
- const size_t height,
- const size_t pitch,
- const size_t elmtsz,
- const size_t align,
+ const struct htrdr_buffer_layout* layout,
struct htrdr_buffer** out_buf)
{
struct htrdr_buffer* buf = NULL;
size_t memsz = 0;
res_T res = RES_OK;
- ASSERT(htrdr && out_buf);
+ ASSERT(htrdr && layout && out_buf);
- if(!width || !height) {
- htrdr_log_err(htrdr, "invalid buffer definition %lux%lu.\n",
- (unsigned long)width, (unsigned long)height);
- res = RES_BAD_ARG;
- goto error;
- }
- if(pitch < width*elmtsz) {
- htrdr_log_err(htrdr,
- "invalid buffer pitch `%lu' wrt the buffer width `%lu'. "
- "The buffer pitch cannot be less than the buffer width.\n",
- (unsigned long)pitch, (unsigned long)width);
- res = RES_BAD_ARG;
- goto error;
- }
- if(!elmtsz) {
- htrdr_log_err(htrdr,
- "the size of the buffer's elements cannot be null.\n");
- res = RES_BAD_ARG;
- goto error;
- }
- if(!IS_POW2(align)) {
- htrdr_log_err(htrdr,
- "invalid buffer alignment `%lu'. It must be a power of 2.\n",
- (unsigned long)align);
+ if(!htrdr_buffer_layout_check(layout)) {
+ htrdr_log_err(htrdr, "Invalid buffer memory layout.\n");
res = RES_BAD_ARG;
goto error;
}
@@ -104,16 +73,13 @@ htrdr_buffer_create
goto error;
}
ref_init(&buf->ref);
- buf->width = width;
- buf->height = height;
- buf->pitch = pitch;
- buf->elmtsz = elmtsz;
- buf->align = align;
+ buf->layout = *layout;
htrdr_ref_get(htrdr);
buf->htrdr = htrdr;
- memsz = buf->pitch * buf->height;
- buf->mem = MEM_ALLOC_ALIGNED(htrdr_get_allocator(htrdr), memsz, align);
+ memsz = buf->layout.pitch * buf->layout.height;
+ buf->mem = MEM_ALLOC_ALIGNED
+ (htrdr_get_allocator(htrdr), memsz, buf->layout.alignment);
if(!buf->mem) {
res = RES_MEM_ERR;
goto error;
@@ -150,11 +116,7 @@ htrdr_buffer_get_layout
struct htrdr_buffer_layout* layout)
{
ASSERT(buf && layout);
- layout->width = buf->width;
- layout->height = buf->height;
- layout->pitch = buf->pitch;
- layout->elmt_size = buf->elmtsz;
- layout->alignment = buf->align;
+ *layout = buf->layout;
}
void*
@@ -167,7 +129,7 @@ htrdr_buffer_get_data(struct htrdr_buffer* buf)
void*
htrdr_buffer_at(struct htrdr_buffer* buf, const size_t x, const size_t y)
{
- ASSERT(buf && x < buf->width && y < buf->height);
- return buf->mem + y*buf->pitch + x*buf->elmtsz;
+ ASSERT(buf && x < buf->layout.width && y < buf->layout.height);
+ return buf->mem + y*buf->layout.pitch + x*buf->layout.elmt_size;
}
diff --git a/src/core/htrdr_buffer.h b/src/core/htrdr_buffer.h
@@ -19,6 +19,8 @@
#define HTRDR_BUFFER_H
#include "core/htrdr.h"
+
+#include <rsys/math.h>
#include <rsys/rsys.h>
/*
@@ -36,6 +38,30 @@ struct htrdr_buffer_layout {
static const struct htrdr_buffer_layout HTRDR_BUFFER_LAYOUT_NULL =
HTRDR_BUFFER_LAYOUT_NULL__;
+static INLINE int
+htrdr_buffer_layout_eq
+ (const struct htrdr_buffer_layout* a,
+ const struct htrdr_buffer_layout* b)
+{
+ ASSERT(a && b);
+ return a->width == b->width
+ && a->height == b->height
+ && a->pitch == b->pitch
+ && a->elmt_size == b->elmt_size
+ && a->alignment == b->alignment;
+}
+
+static INLINE int
+htrdr_buffer_layout_check(const struct htrdr_buffer_layout* layout)
+{
+ return layout
+ && layout->width
+ && layout->height
+ && layout->elmt_size
+ && layout->width*layout->elmt_size <= layout->pitch
+ && IS_POW2(layout->alignment);
+}
+
/* Forward declarations */
struct htrdr;
struct htrdr_buffer;
@@ -45,11 +71,7 @@ BEGIN_DECLS
HTRDR_CORE_API res_T
htrdr_buffer_create
(struct htrdr* htrdr,
- const size_t width,
- const size_t height,
- const size_t pitch, /* #Bytes between 2 consecutive line */
- const size_t elmt_size, /* Size of an element in the buffer */
- const size_t alignment, /* Alignement of the buffer */
+ const struct htrdr_buffer_layout* layout,
struct htrdr_buffer** buf);
HTRDR_CORE_API void
diff --git a/src/core/htrdr_draw_map.c b/src/core/htrdr_draw_map.c
@@ -56,7 +56,7 @@ struct tile {
size_t pixal; /* Pixel alignment */
uint16_t x, y; /* 2D coordinates of the tile in tile space */
/* Simulate the flexible array member of the C99 standard */
- char ALIGN(16) pixels[1/*Dummy element*/];
+ char ALIGN(16) pixels[1/*Dummy element*/];
} data;
};
@@ -73,7 +73,10 @@ struct proc_work {
static INLINE int
check_draw_map_args(const struct htrdr_draw_map_args* args)
{
- return args && args->draw_pixel && args->spp;
+ return args
+ && args->draw_pixel
+ && args->spp
+ && htrdr_buffer_layout_check(&args->buffer_layout);
}
static INLINE void
@@ -125,7 +128,7 @@ tile_create
exit:
return tile;
error:
- if(tile) {
+ if(tile) {
tile_ref_put(tile);
tile = NULL;
}
@@ -396,25 +399,24 @@ mpi_steal_work
static res_T
mpi_gather_tiles
(struct htrdr* htrdr,
+ const struct htrdr_buffer_layout* buf_layout,
struct htrdr_buffer* buf,
const size_t ntiles,
struct list_node* tiles)
{
/* Compute the size of the tile_data */
- struct htrdr_buffer_layout layout = HTRDR_BUFFER_LAYOUT_NULL;
size_t msg_sz = 0;
struct list_node* node = NULL;
struct tile* tile = NULL;
res_T res = RES_OK;
- ASSERT(htrdr && tiles);
+ ASSERT(htrdr && tiles && htrdr_buffer_layout_check(buf_layout));
ASSERT(htrdr->mpi_rank != 0 || buf);
(void)ntiles;
/* Compute the size of the tile data */
- htrdr_buffer_get_layout(buf, &layout);
- msg_sz =
- sizeof(struct tile_data)
- + TILE_SIZE*TILE_SIZE*layout.elmt_size
+ msg_sz =
+ sizeof(struct tile_data)
+ + TILE_SIZE*TILE_SIZE*buf_layout->elmt_size
- 1/* Dummy octet of the flexible array */;
ASSERT(msg_sz <= INT_MAX);
@@ -427,6 +429,16 @@ mpi_gather_tiles
}
} else { /* Master process */
size_t itile = 0;
+ ASSERT(buf);
+
+#ifndef NDEBUG
+ {
+ /* Check data consistency */
+ struct htrdr_buffer_layout layout = HTRDR_BUFFER_LAYOUT_NULL;
+ htrdr_buffer_get_layout(buf, &layout);
+ ASSERT(htrdr_buffer_layout_eq(&layout, buf_layout));
+ }
+#endif
LIST_FOR_EACH(node, tiles) {
struct tile* t = CONTAINER_OF(node, struct tile, node);
@@ -439,7 +451,10 @@ mpi_gather_tiles
/* Create a temporary tile to receive the tile data computed by the
* concurrent MPI processes */
- tile = tile_create(htrdr->allocator, layout.elmt_size, layout.alignment);
+ tile = tile_create
+ (htrdr->allocator,
+ buf_layout->elmt_size,
+ buf_layout->alignment);
if(!tile) {
res = RES_MEM_ERR;
htrdr_log_err(htrdr,
@@ -521,7 +536,6 @@ static res_T
draw_map
(struct htrdr* htrdr,
const struct htrdr_draw_map_args* args,
- const struct htrdr_buffer_layout* buf_layout,
const size_t ntiles_x,
const size_t ntiles_y,
const size_t ntiles_adjusted,
@@ -535,7 +549,7 @@ draw_map
size_t proc_ntiles = 0;
ATOMIC nsolved_tiles = 0;
ATOMIC res = RES_OK;
- ASSERT(htrdr && check_draw_map_args(args) && buf_layout && work && tiles);
+ ASSERT(htrdr && check_draw_map_args(args) && work && tiles);
ASSERT(ntiles_x && ntiles_y && ntiles_adjusted >= ntiles_x*ntiles_y);
ASSERT(pix_sz && pix_sz[0] > 0 && pix_sz[1] > 0);
(void)ntiles_x, (void)ntiles_y;
@@ -589,7 +603,9 @@ draw_map
/* Create the tile */
tile = tile_create
- (htrdr->allocator, buf_layout->elmt_size, buf_layout->alignment);
+ (htrdr->allocator,
+ args->buffer_layout.elmt_size,
+ args->buffer_layout.alignment);
if(!tile) {
ATOMIC_SET(&res, RES_MEM_ERR);
htrdr_log_err(htrdr,
@@ -611,8 +627,8 @@ draw_map
tile_org[1] = (uint16_t)(tile_org[1] * TILE_SIZE);
/* Compute the size of the tile clamped by the borders of the buffer */
- tile_sz[0] = MMIN(TILE_SIZE, buf_layout->width - tile_org[0]);
- tile_sz[1] = MMIN(TILE_SIZE, buf_layout->height - tile_org[1]);
+ tile_sz[0] = MMIN(TILE_SIZE, args->buffer_layout.width - tile_org[0]);
+ tile_sz[1] = MMIN(TILE_SIZE, args->buffer_layout.height - tile_org[1]);
/* Create a proxy RNG for the current tile. This proxy is used for the
* current thread only and thus it has to manage only one RNG. This proxy
@@ -691,7 +707,6 @@ htrdr_draw_map
size_t ntiles_x, ntiles_y, ntiles, ntiles_adjusted;
size_t itile;
struct proc_work work;
- struct htrdr_buffer_layout layout = HTRDR_BUFFER_LAYOUT_NULL;
size_t proc_ntiles_adjusted;
double pix_sz[2];
@@ -700,19 +715,26 @@ htrdr_draw_map
ASSERT(htrdr && check_draw_map_args(args));
ASSERT(htrdr->mpi_rank != 0 || buf);
- htrdr_buffer_get_layout(buf, &layout);
+#ifndef NDEBUG
+ if(htrdr_get_mpi_rank(htrdr) == 0) {
+ /* Check data consistency */
+ struct htrdr_buffer_layout layout = HTRDR_BUFFER_LAYOUT_NULL;
+ htrdr_buffer_get_layout(buf, &layout);
+ ASSERT(htrdr_buffer_layout_eq(&layout, &args->buffer_layout));
+ }
+#endif
list_init(&tiles);
proc_work_init(htrdr->allocator, &work);
/* Compute the overall number of tiles */
- ntiles_x = (layout.width + (TILE_SIZE-1)/*ceil*/)/TILE_SIZE;
- ntiles_y = (layout.height+ (TILE_SIZE-1)/*ceil*/)/TILE_SIZE;
+ ntiles_x = (args->buffer_layout.width + (TILE_SIZE-1)/*ceil*/)/TILE_SIZE;
+ ntiles_y = (args->buffer_layout.height+ (TILE_SIZE-1)/*ceil*/)/TILE_SIZE;
ntiles = ntiles_x * ntiles_y;
/* Compute the pixel size in the normalized image plane */
- pix_sz[0] = 1.0 / (double)layout.width;
- pix_sz[1] = 1.0 / (double)layout.height;
+ pix_sz[0] = 1.0 / (double)args->buffer_layout.width;
+ pix_sz[1] = 1.0 / (double)args->buffer_layout.height;
/* Adjust the #tiles for the morton-encoding procedure */
ntiles_adjusted = round_up_pow2(MMAX(ntiles_x, ntiles_y));
@@ -751,7 +773,7 @@ htrdr_draw_map
#pragma omp section
{
- draw_map(htrdr, args, &layout, ntiles_x, ntiles_y, ntiles_adjusted, pix_sz, &work,
+ draw_map(htrdr, args, ntiles_x, ntiles_y, ntiles_adjusted, pix_sz, &work,
&tiles);
/* The processes have no more work to do. Stop probing for thieves */
ATOMIC_SET(&probe_thieves, 0);
@@ -767,9 +789,9 @@ htrdr_draw_map
time_dump(&t0, TIME_ALL, NULL, strbuf, sizeof(strbuf));
htrdr_log(htrdr, "Rendering time: %s\n", strbuf);
- /* Gather accum buffers from the group of processes */
+ /* Gather tiles to master process */
time_current(&t0);
- res = mpi_gather_tiles(htrdr, buf, ntiles, &tiles);
+ res = mpi_gather_tiles(htrdr, &args->buffer_layout, buf, ntiles, &tiles);
if(res != RES_OK) goto error;
time_sub(&t0, time_current(&t1), &t0);
time_dump(&t0, TIME_ALL, NULL, strbuf, sizeof(strbuf));
diff --git a/src/core/htrdr_draw_map.h b/src/core/htrdr_draw_map.h
@@ -19,6 +19,8 @@
#define HTRDR_DRAW_MAP_H
#include "core/htrdr.h"
+#include "core/htrdr_buffer.h"
+
#include <rsys/rsys.h>
struct htrdr_draw_pixel_args {
@@ -49,6 +51,7 @@ typedef void
struct htrdr_draw_map_args {
htrdr_draw_pixel_T draw_pixel;
+ struct htrdr_buffer_layout buffer_layout;
struct ssp_rng* rng;
size_t spp; /* Samples per pixel */
void* context; /* User defined data */
@@ -56,6 +59,7 @@ struct htrdr_draw_map_args {
#define HTRDR_DRAW_MAP_ARGS_NULL__ { \
NULL, /* Draw pixel functor */ \
+ HTRDR_BUFFER_LAYOUT_NULL__, /* Layout of the destination buffer */ \
NULL, /* Random Number Generator */ \
0, /* #Samples per pixel */ \
NULL /* User defined data */ \
@@ -63,10 +67,6 @@ struct htrdr_draw_map_args {
static const struct htrdr_draw_map_args HTRDR_DRAW_MAP_ARGS_NULL =
HTRDR_DRAW_MAP_ARGS_NULL__;
-/* Forward declarations */
-struct htrdr;
-struct htrdr_buffer;
-
static INLINE int
htrdr_draw_pixel_args_check(const struct htrdr_draw_pixel_args* args)
{
@@ -86,7 +86,7 @@ HTRDR_CORE_API res_T
htrdr_draw_map
(struct htrdr* htrdr,
const struct htrdr_draw_map_args* args,
- struct htrdr_buffer* buf);
+ struct htrdr_buffer* buf); /* May be NULL for non master processes */
END_DECLS
