Skip to content

Commit

Permalink
fix image1d_buffer refcounting with map/unmap cmds (#693)
Browse files Browse the repository at this point in the history 
Fix #691
  • Loading branch information
@rjodinchr
rjodinchr authored Apr 22, 2024
1 parent 96a9f42 commit cde8f36
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 12 deletions.
9 changes: 5 additions & 4 deletions src/api.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3704,9 +3704,10 @@ void* cvk_enqueue_map_buffer(cvk_command_queue* cq, cvk_buffer* buffer,
cl_map_flags map_flags,
cl_uint num_events_in_wait_list,
const cl_event* event_wait_list, cl_event* event,
cl_int* errcode_ret, cl_command_type type) {
auto cmd =
new cvk_command_map_buffer(cq, buffer, offset, size, map_flags, type);
cl_int* errcode_ret, cl_command_type type,
cvk_image* image = nullptr) {
auto cmd = new cvk_command_map_buffer(cq, buffer, offset, size, map_flags,
type, image);

void* map_ptr;
cl_int err = cmd->build(&map_ptr);
Expand Down Expand Up @@ -5435,7 +5436,7 @@ void* CLVK_API_CALL clEnqueueMapImage(
command_queue, static_cast<cvk_buffer*>(img->buffer()),
blocking_map, origin[0] * img->element_size(),
region[0] * img->element_size(), map_flags, num_events_in_wait_list,
event_wait_list, event, &err, CL_COMMAND_MAP_IMAGE);
event_wait_list, event, &err, CL_COMMAND_MAP_IMAGE, img);
} else {
ret = cvk_enqueue_map_image(command_queue, image, blocking_map,
map_flags, origin, region, image_row_pitch,
Expand Down
16 changes: 12 additions & 4 deletions src/memory.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ static inline cvk_mem* icd_downcast(cl_mem mem) {
}

struct cvk_buffer;
struct cvk_image;

using cvk_image_holder = refcounted_holder<cvk_image>;

struct cvk_memobj_mappping {
cvk_buffer* buffer;
Expand All @@ -280,6 +283,10 @@ struct cvk_memobj_mappping {
struct cvk_buffer_mapping : public cvk_memobj_mappping {
size_t offset;
size_t size;

// Needed for buffer mapped through clEnqueueMapImage with a
// CL_MEM_OBJECT_IMAGE1D_BUFFER.
cvk_image_holder image;
};

struct cvk_buffer : public cvk_mem {
Expand Down Expand Up @@ -354,7 +361,8 @@ struct cvk_buffer : public cvk_mem {
}

bool find_or_create_mapping(cvk_buffer_mapping& mapping, size_t offset,
size_t size, cl_map_flags flags) {
size_t size, cl_map_flags flags,
cvk_image* image) {

if (!map()) {
return false;
Expand All @@ -365,6 +373,7 @@ struct cvk_buffer : public cvk_mem {
mapping.size = size;
mapping.ptr = this->map_ptr(offset);
mapping.flags = flags;
mapping.image.reset(image);

return true;
}
Expand All @@ -377,7 +386,7 @@ struct cvk_buffer : public cvk_mem {
return false;
}

m_mappings[mapping.ptr] = mapping;
m_mappings.insert({mapping.ptr, mapping});

return true;
}
Expand All @@ -388,6 +397,7 @@ struct cvk_buffer : public cvk_mem {
auto mapping = m_mappings.at(ptr);
m_mappings.erase(ptr);
mapping.buffer->unmap();
mapping.image.reset(nullptr);
return mapping;
}

Expand Down Expand Up @@ -795,5 +805,3 @@ struct cvk_image : public cvk_mem {
std::mutex m_mappings_lock;
std::unique_ptr<cvk_buffer> m_init_data;
};

using cvk_image_holder = refcounted_holder<cvk_image>;
4 changes: 2 additions & 2 deletions src/queue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1450,8 +1450,8 @@ cl_int cvk_command_fill_buffer::do_action() {

cl_int cvk_command_map_buffer::build(void** map_ptr) {

if (!m_buffer->find_or_create_mapping(m_mapping, m_offset, m_size,
m_flags)) {
if (!m_buffer->find_or_create_mapping(m_mapping, m_offset, m_size, m_flags,
m_image)) {
return CL_OUT_OF_RESOURCES;
}

Expand Down
6 changes: 4 additions & 2 deletions src/queue.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -896,9 +896,10 @@ struct cvk_command_map_buffer final : public cvk_command_buffer_base_region {

cvk_command_map_buffer(cvk_command_queue* queue, cvk_buffer* buffer,
size_t offset, size_t size, cl_map_flags flags,
cl_command_type type)
cl_command_type type, cvk_image* image = nullptr)
: cvk_command_buffer_base_region(queue, type, buffer, offset, size),
m_flags(flags), m_mapping_needs_releasing_on_destruction(false) {}
m_flags(flags), m_mapping_needs_releasing_on_destruction(false),
m_image(image) {}
~cvk_command_map_buffer() {
if (m_mapping_needs_releasing_on_destruction) {
m_buffer->cleanup_mapping(m_mapping);
Expand All @@ -915,6 +916,7 @@ struct cvk_command_map_buffer final : public cvk_command_buffer_base_region {
cl_map_flags m_flags;
cvk_buffer_mapping m_mapping;
bool m_mapping_needs_releasing_on_destruction;
cvk_image* m_image;
};

struct cvk_command_unmap_buffer final : public cvk_command_buffer_base {
Expand Down
86 changes: 86 additions & 0 deletions tests/api/images.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -703,3 +703,89 @@ kernel void test(image1d_buffer_t write_only image)
output[i].s2 == pattern && output[i].s3 == pattern);
}
}

static void set_boolean_to_true(cl_mem memobj, void* bool_ptr) {
((std::atomic<bool>*)bool_ptr)->store(true);
}

TEST_F(WithCommandQueue, 1DBufferImageUnmapAfterRelease) {
const size_t IMAGE_WIDTH = 128;
auto buffer_size = IMAGE_WIDTH * sizeof(cl_float4);
auto buffer = CreateBuffer(CL_MEM_READ_WRITE, buffer_size, nullptr);

cl_image_format format = {CL_RGBA, CL_FLOAT};
cl_image_desc desc = {
CL_MEM_OBJECT_IMAGE1D_BUFFER, // image_type
IMAGE_WIDTH, // image_width
1, // image_height
1, // image_depth
1, // image_array_size
0, // image_row_pitch
0, // image_slice_pitch
0, // num_mip_levels
0, // num_samples
buffer, // buffer
};

auto image = CreateImage(CL_MEM_READ_WRITE, &format, &desc);

std::atomic<bool> destructor_called = false;
clSetMemObjectDestructorCallback(image, set_boolean_to_true,
(void*)&destructor_called);

const size_t origin[3] = {0, 0, 0};
const size_t region[3] = {IMAGE_WIDTH, 1, 1};
auto map_ptr = EnqueueMapImage<cl_uchar>(image, CL_TRUE, 0, origin, region,
nullptr, nullptr);

cl_mem released_image = image.release();
clReleaseMemObject(released_image);
EXPECT_FALSE(destructor_called);

EnqueueUnmapMemObject(released_image, map_ptr);
EXPECT_FALSE(destructor_called);

Finish();
EXPECT_TRUE(destructor_called);
}

TEST_F(WithCommandQueue, 1DBufferImageReleaseAfterUnmap) {
const size_t IMAGE_WIDTH = 128;
auto buffer_size = IMAGE_WIDTH * sizeof(cl_float4);
auto buffer = CreateBuffer(CL_MEM_READ_WRITE, buffer_size, nullptr);

cl_image_format format = {CL_RGBA, CL_FLOAT};
cl_image_desc desc = {
CL_MEM_OBJECT_IMAGE1D_BUFFER, // image_type
IMAGE_WIDTH, // image_width
1, // image_height
1, // image_depth
1, // image_array_size
0, // image_row_pitch
0, // image_slice_pitch
0, // num_mip_levels
0, // num_samples
buffer, // buffer
};

auto image = CreateImage(CL_MEM_READ_WRITE, &format, &desc);

std::atomic<bool> destructor_called = false;
clSetMemObjectDestructorCallback(image, set_boolean_to_true,
(void*)&destructor_called);

const size_t origin[3] = {0, 0, 0};
const size_t region[3] = {IMAGE_WIDTH, 1, 1};
auto map_ptr = EnqueueMapImage<cl_uchar>(image, CL_TRUE, 0, origin, region,
nullptr, nullptr);

EnqueueUnmapMemObject(image, map_ptr);
EXPECT_FALSE(destructor_called);

cl_mem released_image = image.release();
clReleaseMemObject(released_image);
EXPECT_FALSE(destructor_called);

Finish();
EXPECT_TRUE(destructor_called);
}

0 comments on commit cde8f36

Please sign in to comment.