Skip to content

Conversation

@shajder
Copy link
Contributor

@shajder shajder commented Aug 5, 2025

Related to #2282, according to work plan from here

Comment on lines 736 to 741
if (!is_extension_available(device, "cl_khr_gl_msaa_sharing"))
{
log_info("Test not run because 'cl_khr_gl_msaa_sharing' extension is "
"not supported by the tested device\n");
return TEST_SKIPPED_ITSELF;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!is_extension_available(device, "cl_khr_gl_msaa_sharing"))
{
log_info("Test not run because 'cl_khr_gl_msaa_sharing' extension is "
"not supported by the tested device\n");
return TEST_SKIPPED_ITSELF;
}
REQUIRE_EXTENSION("cl_khr_gl_msaa_sharing");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{ "image2d_msaa_t", "2" },
{ "image2d_array_msaa_t", "4" },
{ "image2d_msaa_depth_t", "4" },
{ "image2d_array_msaa_depth_t", "4" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that all of these cases should return an error.

I think image2d_msaa_t should work. Why isn't the 2D image created below trivially an MSAA image, just one with a single sample?

It's probably reasonable to expect that image2d_array_msaa_t and image2d_array_msaa_depth_t will fail, since the image that is created is a 2D image and not a 2D image array. Do we have similar testing for non-MSAA images?

I think that image2d_msaa_depth_t is an especially grey area, since the image is a 2D image, just not one with a CL_DEPTH image channel order. Do we expect implementations to check the channel order?

Note that the error description in clSetKernelArg was clarified recently in KhronosGroup/OpenCL-Docs#1493. The text in the currently published specs is:

If the argument is a multi-sample 2D depth image, the arg_value entry must be a pointer to a multisample depth image object.

This is somewhat ambiguous though - what is considered to be "a multisample depth image object"? Also, note that the spec does not require the image to be a 2D image, which seems like an oversight.

The updated text in the GitHub spec sources but not yet in a published specification is:

If the kernel argument is a 2D MSAA depth image, then the image memory object must be of image type CL_MEM_OBJECT_IMAGE2D and image channel order CL_DEPTH.

This is more precise, but maybe we got the CL_DEPTH image channel order part wrong.

Copy link
Contributor Author

@shajder shajder Dec 16, 2025

Choose a reason for hiding this comment

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

I think image2d_msaa_t should work. Why isn't the 2D image created below trivially an MSAA image, just one with a single sample?

While mathematically a single-sample image is a subset of MSAA, I believe image2d_msaa_t and image2d_t are treated as mutually exclusive types by the API. Specifically, we cannot pass an image2d_msaa_t to a read_imageX function that uses a sampler, nor can we use an image2d_t with the sampler-less read functions. It’s a similar situation to OpenGL, where you cannot use a sampler2D for a texture created with glTexImage2DMultisample (and vice-versa with sampler2DMS).

If the kernel argument is a 2D MSAA depth image, then the image memory object must be of image type CL_MEM_OBJECT_IMAGE2D and image channel order CL_DEPTH.

My interpretation relies on the "must" keyword here. It implies that if the kernel expects image2d_msaa_depth_t, the implementation is required to verify that the passed object actually has the CL_DEPTH channel order. If a user passes a non-depth image, that fails to meet the CL_DEPTH channel order requirement, which is why we expect clSetKernelArg to return an error. Does that make sense?

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