Skip to content

Commit 8f6eb2c

Browse files
kpetkpet
authored
Clean up image mappings when image mapping commands fail (#776)
Fixes #553 Change-Id: Idc4c9c3a224f63a733fb07dd5917bf90819b957b
1 parent 7bdb906 commit 8f6eb2c

File tree

4 files changed

+130
-2
lines changed

4 files changed

+130
-2
lines changed

‎src/memory.hpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -761,6 +761,15 @@ struct cvk_image : public cvk_mem {
761761
return mapping;
762762
}
763763

764+
void cleanup_mapping(cvk_image_mapping& mapping) {
765+
std::lock_guard<std::mutex> lock(m_mappings_lock);
766+
if (m_mappings.count(mapping.ptr)) {
767+
m_mappings.erase(mapping.ptr);
768+
}
769+
mapping.buffer->unmap();
770+
mapping.buffer->release();
771+
}
772+
764773
cvk_image_mapping mapping_for(void* ptr) {
765774
std::lock_guard<std::mutex> lock(m_mappings_lock);
766775
CVK_ASSERT(m_mappings.count(ptr) > 0);

‎src/queue.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,6 +1655,7 @@ cl_int cvk_command_map_image::build(void** map_ptr) {
16551655
cvk_error("cannot find or create a mapping");
16561656
return CL_OUT_OF_RESOURCES;
16571657
}
1658+
m_mapping_needs_releasing_on_destruction = true;
16581659

16591660
*map_ptr = m_mapping.ptr;
16601661

@@ -1699,6 +1700,8 @@ cl_int cvk_command_map_image::do_action() {
16991700
}
17001701
}
17011702

1703+
m_mapping_needs_releasing_on_destruction = false;
1704+
17021705
// TODO invalidate buffer if the memory isn't coherent
17031706

17041707
return CL_COMPLETE;

‎src/queue.hpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,11 +1040,16 @@ struct cvk_command_map_image final : public cvk_command {
10401040
const std::array<size_t, 3>& origin,
10411041
const std::array<size_t, 3>& region,
10421042
cl_map_flags flags, bool update_host_ptr = false)
1043-
: cvk_command(CL_COMMAND_MAP_IMAGE, q), m_image(img), m_origin(origin),
1043+
: cvk_command(CL_COMMAND_MAP_IMAGE, q), m_image(img),
1044+
m_mapping_needs_releasing_on_destruction(false), m_origin(origin),
10441045
m_region(region), m_flags(flags),
10451046
m_update_host_ptr(update_host_ptr &&
10461047
m_image->has_flags(CL_MEM_USE_HOST_PTR)) {}
1047-
1048+
~cvk_command_map_image() {
1049+
if (m_mapping_needs_releasing_on_destruction) {
1050+
m_image->cleanup_mapping(m_mapping);
1051+
}
1052+
}
10481053
CHECK_RETURN cl_int build(void** map_ptr);
10491054
CHECK_RETURN cl_int do_action() override final;
10501055
cvk_buffer* map_buffer() { return m_mapping.buffer; }
@@ -1074,6 +1079,7 @@ struct cvk_command_map_image final : public cvk_command {
10741079

10751080
cvk_image_holder m_image;
10761081
cvk_image_mapping m_mapping;
1082+
bool m_mapping_needs_releasing_on_destruction;
10771083
std::array<size_t, 3> m_origin;
10781084
std::array<size_t, 3> m_region;
10791085
cl_map_flags m_flags;

‎tests/api/dependencies.cpp‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,116 @@ TEST_F(WithCommandQueue, FailedAndCompleteDependencies) {
4848
ASSERT_EQ(err, CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
4949
}
5050

51+
TEST_F(WithCommandQueue, CheckMapBufferRemovesMappingIfDependenciesFailed) {
52+
// Create buffer
53+
auto buffer = CreateBuffer(CL_MEM_WRITE_ONLY | CL_MEM_ALLOC_HOST_PTR,
54+
BUFFER_SIZE, nullptr);
55+
56+
// Create user events
57+
auto event = CreateUserEvent();
58+
59+
// Make its status a failure
60+
SetUserEventStatus(event, CL_INVALID_OPERATION);
61+
62+
cl_int err;
63+
64+
// Check that a map buffer that depends on it fails (and rely on tooling to
65+
// check that the mapping is cleaned up)
66+
cl_event dep = event;
67+
clEnqueueMapBuffer(m_queue, buffer, CL_TRUE, CL_MAP_WRITE_INVALIDATE_REGION,
68+
0, BUFFER_SIZE, 1, &dep, nullptr, &err);
69+
70+
ASSERT_EQ(err, CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
71+
}
72+
73+
TEST_F(WithCommandQueue, CheckMapImageRemovesMappingIfDependenciesFailed) {
74+
// Create image
75+
const size_t IMAGE_WIDTH = 97;
76+
const size_t IMAGE_HEIGHT = 13;
77+
78+
cl_image_format format = {CL_R, CL_UNSIGNED_INT8};
79+
cl_image_desc desc = {
80+
CL_MEM_OBJECT_IMAGE2D, // image_type
81+
IMAGE_WIDTH, // image_width
82+
IMAGE_HEIGHT, // image_height
83+
1, // image_depth
84+
1, // image_array_size
85+
0, // image_row_pitch
86+
0, // image_slice_pitch
87+
0, // num_mip_levels
88+
0, // num_samples
89+
nullptr, // buffer
90+
};
91+
92+
auto image = CreateImage(CL_MEM_READ_WRITE, &format, &desc);
93+
94+
// Create user event
95+
auto event = CreateUserEvent();
96+
97+
// Make its status a failure
98+
SetUserEventStatus(event, CL_INVALID_OPERATION);
99+
100+
cl_int err;
101+
size_t origin[3] = {0, 0, 0};
102+
size_t region[3] = {IMAGE_WIDTH, IMAGE_HEIGHT, 1};
103+
size_t row_pitch;
104+
105+
// Check that a map image that depends on it fails (and rely on tooling to
106+
// check that the mapping is cleaned up)
107+
cl_event dep = event;
108+
clEnqueueMapImage(m_queue, image, CL_TRUE, CL_MAP_WRITE_INVALIDATE_REGION,
109+
origin, region, &row_pitch, nullptr, 1, &dep, nullptr,
110+
&err);
111+
112+
ASSERT_EQ(err, CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
113+
}
114+
115+
TEST_F(WithCommandQueue,
116+
CheckMapImage1DBufferRemovesMappingIfDependenciesFailed) {
117+
118+
// Create 1D Buffer image
119+
const size_t IMAGE_WIDTH = 128;
120+
121+
auto buffer_size = IMAGE_WIDTH * sizeof(cl_float4);
122+
auto buffer = CreateBuffer(CL_MEM_READ_WRITE, buffer_size, nullptr);
123+
124+
cl_image_format format = {CL_RGBA, CL_FLOAT};
125+
cl_image_desc desc = {
126+
CL_MEM_OBJECT_IMAGE1D_BUFFER, // image_type
127+
IMAGE_WIDTH, // image_width
128+
1, // image_height
129+
1, // image_depth
130+
1, // image_array_size
131+
0, // image_row_pitch
132+
0, // image_slice_pitch
133+
0, // num_mip_levels
134+
0, // num_samples
135+
buffer, // buffer
136+
};
137+
138+
auto image = CreateImage(CL_MEM_READ_WRITE, &format, &desc);
139+
140+
// Create user event
141+
auto event = CreateUserEvent();
142+
143+
// Make its status a failure
144+
SetUserEventStatus(event, CL_INVALID_OPERATION);
145+
146+
cl_int err;
147+
size_t origin[3] = {0, 0, 0};
148+
size_t region[3] = {IMAGE_WIDTH, 1, 1};
149+
size_t row_pitch;
150+
151+
// Check that a map image that depends on it fails (and rely on tooling to
152+
// check that the mapping is cleaned up)
153+
cl_event dep = event;
154+
clEnqueueMapImage(m_queue, image, CL_TRUE, CL_MAP_WRITE_INVALIDATE_REGION,
155+
origin, region, &row_pitch, nullptr, 1, &dep, nullptr,
156+
&err);
157+
158+
ASSERT_EQ(err, CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
159+
}
160+
51161
TEST_F(WithCommandQueue, InOrderQueueStopsExecutionAfterFailedCommand) {
52162
auto buffer = CreateBuffer(CL_MEM_WRITE_ONLY | CL_MEM_ALLOC_HOST_PTR,
53163
BUFFER_SIZE, nullptr);

0 commit comments

Comments
 (0)