Test IMAGE1D_BUFFER in more scenario #1806

rjodinchr · 2023-09-04T14:00:25Z

add IMAGE1D_BUFFER test case:

cl_copy_images:
- IMAGE1D_BUFFER to IMAGE1D_BUFFER
- IMAGE1D_BUFFER to IMAGE1D
- IMAGE1D to IMAGE1D_BUFFER
cl_get_info
cl_fill_image
cl_read_write_image
kernel_image_methods

Most of the tests are strongly inspired from the IMAGE1D tests already present.
Most significant changes are:

Using the correct limit to choose the size (using CL_DEVICE_IMAGE_MAX_BUFFER_SIZE)
Creating the buffer needed for IMAGE1D_BUFFER
Add IMAGE1D_BUFFER in many switch/if statements in common code to support the new tests.

I have made those tests because working on implementing 1dbuffer supports in clvk shows that lots of cases were not covered by the OpenCL-CTS to truly validate the implementation.
Ref kpet/clvk#403

lakshmih · 2023-09-19T16:53:54Z

Reviewing.

test_conformance/images/clFillImage/test_loops.cpp

test_conformance/images/clCopyImage/test_loops.cpp

rjodinchr · 2023-09-26T05:41:07Z

About the code formatting issue. I will leave the PR as it is formatted to ease the review. Once we agree on it, I'll fix those errors.

lakshmih · 2023-10-02T18:53:19Z

Need some more time to review this.

lakshmih · 2023-10-04T15:42:07Z

Still reviewing/testing.

test_conformance/images/clGetInfo/test_1D_buffer.cpp

test_conformance/images/clReadWriteImage/test_read_1D_buffer.cpp

test_common/harness/imageHelpers.cpp

test_conformance/images/clReadWriteImage/test_read_1D_buffer.cpp

bashbaug

LGTM, I've run all of these tests on an Intel GPU and they're passing, and I took a quick look through the code and everything looks in order.

There is a LOT of duplication among these tests, which makes them easier to review, but we might want to refactor to reduce the code duplication at some point (not in this PR!).

bashbaug · 2024-02-13T01:28:20Z

test_conformance/images/clCopyImage/test_copy_1D_buffer.cpp

+ int error = clGetDeviceInfo(device, CL_DEVICE_IMAGE2D_MAX_WIDTH,
+ sizeof(maxWidth), &maxWidth, NULL);


Can you check if this is correct? It's not clear to me if this value is used to create a 1D image, or a 1D image from a buffer, or both. If it's used to create a 1D image from a buffer then should we be querying CL_DEVICE_IMAGE_MAX_BUFFER_SIZE instead? Or, if it's both, should we be querying both and using the smallest value?

It is used to create 1D images. We could query both to get the smallest, but it would mean we expect to have an implementation with 1D image from buffer smaller than standard 1D image. Is that really possible?

test_conformance/images/clFillImage/test_fill_generic.cpp

nikhiljnv · 2024-02-28T09:45:48Z

We are seeing some issues on NVIDIA implementation with the changes proposed in this PR. Will update in a couple of days once we dig into this more.

We have fixed the issue on our side. One small change needed here to add image_1d_buffer case.

OpenCL-CTS/test_conformance/images/clCopyImage/test_copy_generic.cpp

Line 445 in e0a31a0

break;

rjodinchr · 2024-02-28T09:59:49Z

We are seeing some issues on NVIDIA implementation with the changes proposed in this PR. Will update in a couple of days once we dig into this more.

We have fixed the issue on our side. One small change needed here to add image_1d_buffer case.

OpenCL-CTS/test_conformance/images/clCopyImage/test_copy_generic.cpp

Line 445 in e0a31a0

break;

I don't understand, I believe this PR already has the CL_MEM_OBJECT_IMAGE1D_BUFFER case in the switch you mentioned. But the link you gave is not the one from this PR, thus that's why it is missing.

nikhiljnv · 2024-02-28T10:28:32Z

We are seeing some issues on NVIDIA implementation with the changes proposed in this PR. Will update in a couple of days once we dig into this more.

We have fixed the issue on our side. One small change needed here to add image_1d_buffer case.

OpenCL-CTS/test_conformance/images/clCopyImage/test_copy_generic.cpp

Line 445 in e0a31a0

break;

I don't understand, I believe this PR already has the CL_MEM_OBJECT_IMAGE1D_BUFFER case in the switch you mentioned. But the link you gave is not the one from this PR, thus that's why it is missing.

I think you are referring to

OpenCL-CTS/test_conformance/images/clCopyImage/test_copy_generic.cpp

Line 458 in 9c6c166

case CL_MEM_OBJECT_IMAGE1D_BUFFER:

from this PR, whereas the one I was referring to is here in this PR

OpenCL-CTS/test_conformance/images/clCopyImage/test_copy_generic.cpp

Line 597 in 9c6c166

default: {

test_conformance/images/clFillImage/test_fill_generic.cpp

lakshmih

Thank you for addressing comments so far, just a couple more questions.

test_conformance/images/clFillImage/test_fill_generic.cpp

lakshmih

Thanks for the changes, just some more minor comments.

lakshmih · 2024-03-29T21:05:57Z

test_conformance/images/clFillImage/test_fill_generic.cpp

+ struct pitch_buffer_data *data = (struct pitch_buffer_data *)malloc(
+ sizeof(struct pitch_buffer_data));
+ data->buf = host_ptr;
+ data->is_aligned = CL_VERSION_MAJOR(version) == 1


We believe this needs to be:
data->is_aligned = !((CL_VERSION_MAJOR(version) == 1)
|| (imageInfo->type != CL_MEM_OBJECT_IMAGE1D_BUFFER));

Following are incorrect assumption as per commit(9a49a34)

Assumption: if version == 1 and type is 1Dbuffer then buffer is aligned.
Actual: if version ==1 then buffer is always un-aligned

Assumption: if version != 1 then buffer is not aligned always.
Actual: if version != 1 and type is 1Dbuffer then buffer is aligned

data->is_aligned = !((CL_VERSION_MAJOR(version) == 1) || (imageInfo->type != CL_MEM_OBJECT_IMAGE1D_BUFFER));

does not match

Actual: if version != 1 and type is 1Dbuffer then buffer is aligned

I think version != 1 && type is 1DBuffer is correct. I'll push that modification.

lakshmih

Hopefully the last one to fix and we should be good. Thank you for the incremental changes!

bashbaug · 2024-04-16T15:48:02Z

Merging as discussed in the April 16th teleconference.

ahesham-arm · 2024-09-02T12:49:01Z

test_conformance/images/kernel_image_methods/test_1D_buffer.cpp

+ if (gDebugTrace)
+ log_info(" - Creating 1D image %d ...\n", (int)imageInfo->width);
+
+ buffer = clCreateBuffer(


The size passed to clCreateBuffer and the one in the call to log_error are different. The correct value that should be passed to both is imageInfo->rowPitch.

Shouldn't rowPitch always equal width * pixel_size?

But I agree, it would be better to remove the use of imageInfo->width * get_pixel_size(imageInfo->format) in favour of imageInfo->rowPitch.

It should, but before the call to this function there is:
imageInfo.rowPitch += extraWidth;

Then the caller might also need to be fixed either way, right?

Sorry, I didn't dig into what the test is trying to do but my general comment is that whatever we pass in as the image row pitch for creating the image, should be the same value we use for creating the buffer (size of the buffer is required to be >= row pitch in this case), and the same value that we log.

qichaogu · 2024-09-23T06:59:23Z

test_conformance/images/kernel_image_methods/test_1D_buffer.cpp

+ do
+ {
+ imageInfo.width =
+ (size_t)random_log_in_range(16, (int)maxWidth / 32, seed);


Integer overflow here: maxWidth is truncated from size_t to int.
It will cause problems:
(int)maxWidth may be a negative integer. As a result, the function random_log_in_range will have a second parameter which is negative so it will yield a negative result integer. Then this negative result integer will be converted to size_t and imageInfo.width will be a very large integer. Finally, the test will fail when trying to allocating memory of a very big size calculated from imageInfo.width.

@rjodinchr Could you please fix the issue?

random_log_in_range is used more than 200 times in the CTS, with this same pattern many times. It feels like a proper fix would involve changing that function and making sure everything is still working as intended. I'm sorry to say I will not have time for that in the short term.

This was referenced Sep 6, 2023

implement image1d_buffer using texel buffer google/clspv#1226

Merged

add support for image1d_buffer kpet/clvk#609

Merged

kpet added the focused review label Sep 12, 2023

lakshmih reviewed Sep 25, 2023

View reviewed changes

test_conformance/images/clFillImage/test_loops.cpp Outdated Show resolved Hide resolved

test_conformance/images/clCopyImage/test_loops.cpp Outdated Show resolved Hide resolved

rjodinchr force-pushed the pr/1dbuffer branch from ed1cc3d to 4203dcb Compare September 26, 2023 05:39

rjodinchr requested review from lakshmih and bashbaug September 26, 2023 05:39

lakshmih reviewed Oct 14, 2023

View reviewed changes

test_conformance/images/clGetInfo/test_1D_buffer.cpp Outdated Show resolved Hide resolved

rjodinchr requested a review from lakshmih October 16, 2023 07:06

lakshmih reviewed Nov 7, 2023

View reviewed changes

test_conformance/images/clReadWriteImage/test_read_1D_buffer.cpp Outdated Show resolved Hide resolved

lakshmih reviewed Nov 9, 2023

View reviewed changes

test_common/harness/imageHelpers.cpp Show resolved Hide resolved

rjodinchr requested a review from lakshmih November 10, 2023 10:42

lakshmih reviewed Nov 21, 2023

View reviewed changes

test_common/harness/imageHelpers.cpp Outdated Show resolved Hide resolved

lakshmih reviewed Nov 22, 2023

View reviewed changes

test_conformance/images/clReadWriteImage/test_read_1D_buffer.cpp Outdated Show resolved Hide resolved

rjodinchr requested a review from lakshmih January 9, 2024 14:01

bashbaug previously approved these changes Feb 13, 2024

View reviewed changes

rjodinchr added 10 commits February 13, 2024 14:43

cl_copy_images

cfdc23a

cl_get_info

f6bcdb1

cl_fill_image

1992631

cl_read_write_image

0554854

kernel_image_methods

eb452c1

IMAGE1D_BUFFER cannot be created with (USE_|ALLOC_|COPY_)_HOST_PTR

1db645f

do not allow mipmap with 1D buffer

598bb72

adjust M to be within maximum_sizes and max_pixels

d471d47

remove unused variables

7e51654

make sure M will never be 0

4ca814c

lakshmih requested changes Feb 27, 2024

View reviewed changes

test_conformance/images/clFillImage/test_fill_generic.cpp Show resolved Hide resolved

fix image1d_buffer creation with gEnablePitch

9c6c166

add missing case in switch

f101b60

rjodinchr requested a review from lakshmih February 28, 2024 14:53

lakshmih requested changes Mar 4, 2024

View reviewed changes

test_conformance/images/clFillImage/test_fill_generic.cpp Outdated Show resolved Hide resolved

use align_malloc when CL version is at least 2.0

8d5fee9

rjodinchr requested a review from lakshmih March 5, 2024 08:36

lakshmih reviewed Mar 11, 2024

View reviewed changes

test_conformance/images/clFillImage/test_fill_generic.cpp Outdated Show resolved Hide resolved

test_conformance/images/clFillImage/test_fill_generic.cpp Outdated Show resolved Hide resolved

use CL_DEVICE_NUMERIC_VERSION and align_free

84e5fc2

rjodinchr requested a review from lakshmih March 12, 2024 07:00

lakshmih reviewed Mar 26, 2024

View reviewed changes

test_conformance/images/clFillImage/test_fill_generic.cpp Show resolved Hide resolved

lakshmih reviewed Mar 26, 2024

View reviewed changes

test_conformance/images/clFillImage/test_fill_generic.cpp Show resolved Hide resolved

lakshmih reviewed Mar 26, 2024

View reviewed changes

fix free of pitch buffer

9a49a34

rjodinchr requested a review from lakshmih March 27, 2024 08:37

rjodinchr added 2 commits March 27, 2024 10:13

fix formatting

cd53e28

fix formatting

26255dd

lakshmih reviewed Mar 29, 2024

View reviewed changes

fix data->is_aligned

71eca12

rjodinchr requested a review from lakshmih March 30, 2024 08:38

lakshmih approved these changes Apr 9, 2024

View reviewed changes

bashbaug merged commit be8b56d into KhronosGroup:main Apr 16, 2024
7 checks passed

rjodinchr deleted the pr/1dbuffer branch April 16, 2024 15:49

ahesham-arm reviewed Sep 2, 2024

View reviewed changes

qichaogu reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test IMAGE1D_BUFFER in more scenario #1806

Test IMAGE1D_BUFFER in more scenario #1806

rjodinchr commented Sep 4, 2023

lakshmih commented Sep 19, 2023

rjodinchr commented Sep 26, 2023

lakshmih commented Oct 2, 2023

lakshmih commented Oct 4, 2023

bashbaug left a comment

bashbaug Feb 13, 2024

rjodinchr Feb 13, 2024

nikhiljnv commented Feb 28, 2024

rjodinchr commented Feb 28, 2024

nikhiljnv commented Feb 28, 2024

lakshmih left a comment

lakshmih left a comment

lakshmih Mar 29, 2024 •

edited

Loading

rjodinchr Mar 30, 2024

lakshmih left a comment

bashbaug commented Apr 16, 2024

ahesham-arm Sep 2, 2024

rjodinchr Sep 2, 2024

ahesham-arm Sep 2, 2024

rjodinchr Sep 2, 2024

ahesham-arm Sep 2, 2024

qichaogu Sep 23, 2024

rjodinchr Sep 23, 2024

		int error = clGetDeviceInfo(device, CL_DEVICE_IMAGE2D_MAX_WIDTH,
		sizeof(maxWidth), &maxWidth, NULL);

Test IMAGE1D_BUFFER in more scenario #1806

Test IMAGE1D_BUFFER in more scenario #1806

Conversation

rjodinchr commented Sep 4, 2023

lakshmih commented Sep 19, 2023

rjodinchr commented Sep 26, 2023

lakshmih commented Oct 2, 2023

lakshmih commented Oct 4, 2023

bashbaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljnv commented Feb 28, 2024

rjodinchr commented Feb 28, 2024

nikhiljnv commented Feb 28, 2024

lakshmih left a comment

Choose a reason for hiding this comment

lakshmih left a comment

Choose a reason for hiding this comment

lakshmih Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshmih left a comment

Choose a reason for hiding this comment

bashbaug commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshmih Mar 29, 2024 •

edited

Loading