clarify num_mip_levels

[bashbaug]

Fixes #1241.

Clarifies that the number of mip levels may be nonzero when the cl_khr_mipmap_image extension is supported, rather than that the number of mip levels must be greater than one.

[karolherbst]

Sorry for not seeing this earlier. I was actually implementing the extension a few weeks ago and I was confused by this part as well.

I think the original phrasing was actually more correct than the new one, but it should still be clarified.

So prior the extension, 0 was the only allowed value indicating there is one "level". However, the CTS expects that 0 and 1 behave the same, meaning both result in an image with exactly one level. 2 means "there is the base level + one additional one".

Maybe I misunderstood how this all works, but this understanding was the only one making the CTS pass and not going out of bounds.

I think we might need to check out what other implementations are doing and if the CTS just has a different understanding here, but so far the only reasonable explanation I had for the behavior I was seeing was that 0 and 1 are identical in its meaning and I wouldn't be surprised that's where the "more than 1" wording comes from.

[karolherbst]

E.g. here the CTS substracts 1 from imageInfo->num_mip_levels https://github.com/KhronosGroup/OpenCL-CTS/blob/main/test_conformance/images/clCopyImage/test_copy_1D.cpp#L34C63-L34C76 to get a random value between 0 and the upper bound.

However, if num_mip_levels means additional mipmap levels (1 == 2 levels in total, 2 == 3 levels in total) it would make no sense to do the - 1, because 0..num_mip_levels would be all valid values, not 0..num_mip_levels - 1

Therefore I think the original wording was correct, just super confusing and not clear what's meant here.

[bashbaug]

OK, now I'm confused. :-)

This matches my current understanding:

So prior the extension, 0 was the only allowed value indicating there is one "level". However, the CTS expects that 0 and 1 behave the same, meaning both result in an image with exactly one level. 2 means "there is the base level + one additional one".

Which is also consistent with the spec text:

A mipmapped 1D image, 1D image array, 2D image, 2D image array or 3D image is created by specifying num_mip_levels to be a value greater than one in image_desc.

IMHO this means that the CTS tests are incorrect, or at least aren't testing the full set of mip levels. I also think the "must" to "may" change in this PR is correct, regardless, though we ought to clarify what num_mip_levels really means...

[karolherbst]

Yeah.. I'm not entirely sure what should be correct wording, but it also depends on how it's used.

Intel's stack seems to agree with the original wording and my understanding:

opencl/source/helpers/mipmap.h:

inline bool isMipMapped(const cl_image_desc &imgDesc) {
    return (imgDesc.num_mip_levels > 1);
}

And there is also uint32_t mipCount = this->mipCount > 0 ? this->mipCount - 1 : 0; when setting a kernel image argument inside opencl/source/mem_obj/image.inl

So in my opinion, yes, it would have been better if num_mip_levels would mean "additional levels on top of the base (level 0) one", but it sounds more that it means "all levels including the base (level 0)"

And the CTS seems to be consistent with this. So I don't think we should risk breaking applications by changing the meaning here if applications rely on it. I just don't know what application even uses mipmaps, because my original reason to implement is to just get all the GL image extensions implemented up to cl_khr_gl_msaa_sharing.

[karolherbst]

In either case, I think it makes sense to clarify what this value means in the spec, because at the moment it's vague and confusing