Hi, On 15/07/16 22:46, Francisco Jerez wrote: > Alejandro Piñeiro <apinhe...@igalia.com> writes: > >> On 14/07/16 21:24, Francisco Jerez wrote: >>> Alejandro Piñeiro <apinhe...@igalia.com> writes: >>> >>>> Without this commit, a image is considered valid if the level of the >>>> texture bound to the image is complete, something we can check as mesa >>>> save independently if it is "base incomplete" of "mipmap incomplete". >>>> >>>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture >>>> Image Loads and Stores"): >>>> >>>> "An access is considered invalid if: >>>> the texture bound to the selected image unit is incomplete;" >>>> >>>> This implies that the access to the image unit is invalid if the >>>> texture is incomplete, no mattering details about the specific texture >>>> level bound to the image. >>>> >>>> This fixes: >>>> GL44-CTS.shader_image_load_store.incomplete_textures >>>> --- >>>> >>>> Current piglit test is not testing what this commit tries to fix. I >>>> will send a patch to piglit in short. >>>> >>>> src/mesa/main/shaderimage.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c >>>> index 90643c4..d20cd90 100644 >>>> --- a/src/mesa/main/shaderimage.c >>>> +++ b/src/mesa/main/shaderimage.c >>>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx, >>>> struct gl_image_unit *u) >>>> if (!t->_BaseComplete && !t->_MipmapComplete) >>>> _mesa_test_texobj_completeness(ctx, t); >>>> >>>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image >>>> + * Loads and Stores: >>>> + * >>>> + * "An access is considered invalid if: >>>> + * the texture bound to the selected image unit is incomplete;" >>>> + */ >>>> + if (!t->_BaseComplete || >>>> + !t->_MipmapComplete) >>>> + return GL_FALSE; >>> I don't think this is correct, AFAIUI a texture having _MipmapComplete >>> equal to false doesn't imply that the texture as a whole would be >>> considered incomplete according to the GL's definition of completeness. >>> Whether or not it's considered complete usually depends on the sampler >>> state while you're doing regular texture sampling: If the sampler a >>> texture object is used with has any of the mipmap filtering modes >>> enabled you need to check _MipmapComplete, otherwise you need to check >>> _BaseComplete. The problem when you attempt to carry over this >>> definition to shader images (as the spec implies) is that image units >>> have no sampler state as such, and that they can only ever access one >>> specified level of the texture at a time (potentially a texture level >>> other than the base). This patch makes image units behave like a >>> sampler unit with mipmap filtering enabled for the purpose of texture >>> completeness validation, which is almost definitely too strong. >> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not >> checking the state at all. Thanks for pointing it. >> >>> An alternative would be to do something along the lines of: >>> >>> | if (!_mesa_is_texture_complete(t, &t->Sampler)) >>> | return GL_FALSE; >> Yes, that is what I wanted, to return false if the texture is incomplete. >> >>> The problem is that you would then run into problems when some of the >>> non-base mipmap levels are missing but the sampler state baked into the >>> gl_texture_object says that you aren't mipmapping, so the GL spec would >>> normally consider the texture to be complete and >>> _mesa_is_texture_complete would return true accordingly, but still you >>> wouldn't be able to use any of the missing texture levels as shader >>> image if the application tried to bind them to an image unit (that's the >>> reason for the u->Level vs t->BaseLevel checks below you're removing). >> Ok, then if I understand correctly, the solution is not about replacing >> the level checks for _mesa_is_texture_complete, but keeping current >> checks, and add a _mesa_is_texture_complete check. Just checked and >> everything seems to work fine (except that now the behaviour is more >> strict, see below). I will send a patch in short. >> > Yeah, that would likely work and get the CTS test to pass, but it would > still be more strict than the spec says and consider cases that are OK > according to the spec to be incomplete, so I was reluctant to call it a > solution. > > I think the ideal solution would be for the state of an image unit to be > independent from the filtering and sampling state, and depend on the > completeness of the bound level *only*. Any idea if this CTS (or your > equivalent piglit test) passes on other GL implementations that support > image load/store (e.g. nVidia's -- I would be surprised if it does).
Just checked today with NVIDIA 352.30 and 352.30. I was not able to directly test this issue with the CTS test, as it fails before with a different error (the shader doesn't compile, and the shader info log is garbage). But I tested it with the equivalent piglit subtest that I sent to the piglit list. It passes on both cases. >>> Honestly I think the current definition of image unit completeness is >>> broken, because it considers the image unit state to be complete in >>> cases where the image unit state is unusable (because the target image >>> level may be missing even though the texture object itself is considered >>> complete), and because it considers it to be incomplete in cases where >>> you really have no reason to care (e.g. why should you care what the >>> filtering mode from the sampler state says if you aren't doing any >>> filtering at all, as long as the one level you've bound to the image >>> unit is present and complete). >> Well, yes in general the spec is being perhaps too strict. But I assume >> that if we want to follow the spec, we don't have any other option. >> > I think the rule is not only unnecessarily strict... If my reading of > the spec is correct it's also impossible to implement to the letter, > because the spec expects you to consider the state of an image unit to > be image-complete just because the bound texture object is > sampler-complete even though the bound texture level may be missing. Sorry but I was not able to find any reference to that expectation on the spec. Although it is true that Im using as reference only this spec: https://www.opengl.org/registry/specs/ARB/shader_image_load_store.txt Did I skip that expectation on the spec, or it is included on other spec ? BTW, when re-reading the spec, I found an issue related to texture completeness: "(7) How do shader image loads and stores interact with texture completeness? What happens if you bind a texture with inconsistent mipmaps? RESOLVED: The image unit is treated as if nothing were bound, where all accesses are treated as invalid." So I guess that it was discussed. > It > might be worth filing a GL bug at Khronos. In order to do that, I would need to clearly point the contradiction. Sorry if I was not able to find it. > >> FWIW, when testing the patch I mentioned before, some of the piglit >> tests start to fail. The reason is that with that patch everything is >> more strict, so the piglit tests need to set the filtering to NEAREST in >> order to not fail. I will send a piglit patch soon too. >>>> + >>>> if (u->Level < t->BaseLevel || >>>> - u->Level > t->_MaxLevel || >>>> - (u->Level == t->BaseLevel && !t->_BaseComplete) || >>>> - (u->Level != t->BaseLevel && !t->_MipmapComplete)) >>>> + u->Level > t->_MaxLevel) >>>> return GL_FALSE; >>>> >>>> if (_mesa_tex_target_is_layered(t->Target) && >>>> -- >>>> 2.7.4
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev