Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set CL_DEVICE_IMAGE_PITCH_ALIGNMENT to 1 when images are supported. #572

Closed
wants to merge 1 commit into from

Conversation

robquill
Copy link

@robquill robquill commented Jul 7, 2023

The spec says that it should be 0 if images are not supported. Otherwise it should be a power of 2. 0 isn't a power of 2 and causes problems with tests which expect to be able to use CL_DEVICE_IMAGE_PITCH_ALIGNMENT to calculate memory allocation sizes etc.

The spec says that it should be 0 if images are not supported.
Otherwise it should be a power of 2. 0 isn't a power of 2 and
causes problems with tests which expect to be able to use
CL_DEVICE_IMAGE_PITCH_ALIGNMENT to calculate memory allocation
sizes etc.
Copy link
Owner

@kpet kpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Assuming I'm reading the specification correctly, this change looks incorrect. The spec states:

The value returned must be a power of 2.
Support for 2D images created from a buffer is required for an OpenCL 2.0, 2.1, or 2.2 device if CL_DEVICE_IMAGE_SUPPORT is CL_TRUE.
This value must be 0 for devices that do not support 2D images created from a buffer.

clvk is an OpenCL 3.0 implementation and does not currently support creating 2D images from buffers (see #43). As far as I can tell, reporting 0 is the correct behaviour.

Furthermore, this change fails the consistency_2d_image_from_buffer CTS test.

My guess would be that the failing application is making incorrect assumptions and not checking for support before using the value returned by CL_DEVICE_IMAGE_PITCH_ALIGNMENT.

@robquill
Copy link
Author

Thanks for reviewing this. You are right, it does look like an app issue. I'll close this PR.

@robquill robquill closed this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants