Alejandro Piñeiro <apinhe...@igalia.com> writes:
> On 23/07/16 00:31, Francisco Jerez wrote:
>> Alejandro Piñeiro <apinhe...@igalia.com> writes:
>>
>>> 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.
>>>
>> In that case they must have changed their behavior. I wrote the
>> arb_shader_image_load_store piglit tests against the nVidia driver
>> originally, and they would have failed almost every test if they had
>> implemented that rule to the letter, because I never bothered to set the
>> sampler filtering mode of image textures as you noticed here:
>>
>> https://lists.freedesktop.org/archives/piglit/2016-July/020232.html
>>
>> I guess the fact that nVidia is now enforcing this rule makes me
>> somewhat less nervous to make this change to make the CTS test happy,
>> since that means it's not very likely for applications to still be
>> relying on the less strict interpretation of the spec and break...
>
> Ok.
>
>>>>>> 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.
>>>
>> The spec seems rather sketchy for using the language of "texture
>> completeness" in the list of bullet points starting with "An access is
>> considered invalid if: [..]" in section 8.26 of the GL 4.4 spec, because
>> a texture being complete or not is dependent on sampler state that
>> doesn't have any significance for shader images. If you bind a
>> base-complete but non-mipmap-complete texture to a texture unit along
>> with a sampler object specifying single-mipmap filtering, the texture is
>> considered complete by the GL spec. Should it be considered incomplete
>> when the same base level is bound to an image unit instead of a texture
>> unit, even though no minification filtering will take effect and no
>> mipmapping is required? (C.f. section 8.17 claiming that a texture
>> object should be considered incomplete if "[..] the minification filter
>> requires a mipmap [..] and the texture is not mipmap complete.").
>>
>> Let's assume momentarily that it's only the minification filtering mode
>> baked into the texture object that matters for image units. What should
>> happen in the following scenario?
>>
>> - The texture object has filtering modes set to NEAREST so no
>> mipmapping is required and the above rule would consider the texture
>> to be complete regardless of the non-base mipmap levels.
>>
>> - The texture object has inconsistent mipmap levels attached, e.g. some
>> of the levels have incorrect size or inconsistent internal formats in
>> a way that may make the miptree impossible to represent by the
>> implementation.
>>
>> - Any of the non-base mipmap levels part of the texture object is bound
>> to an image unit.
>>
>> Should the image unit state be considered complete or not? Section 8.26
>> implies that it should because the attached texture object is complete,
>> but that means that the implementation would have to support doing image
>> loads and stores on arbitrary levels of mipmap trees of inconsistent
>> layout (!). OTOH Mesa would currently consider the case above to be
>> incomplete regardless of this patch due to the rather ad-hoc rule I
>> implemented in the same function.
>
> Ok, thanks for the long explanation.
>
>> I think my suggestion would be to change section 8.26 of the spec to
>> specify *explicitly* what kind of texture completeness is required, e.g.
>> the texture object should be mipmap-complete if any non-base level is
>> bound to the image unit, but only base-complete (by some definition of
>> base-completeness) if the level bound to the image unit is the base
>> level
>
> And thanks for the suggestion.
>
>> -- However that spec change would have the side effect of making
>> the CTS test you're trying to fix non-compliant...
>
> This would not be the first case of a CTS test that is not correct due
> the complexities/ambiguities of the OpenGL spec. Or worse, CTS tests
> that are just
>Heh, seems like some words were censored from your last paragraph. :P > So what about this: I open a Khronos bug (public unless someone thinks > that it would be better private), using your explanation as a base to > the description (although tempted to just point this email), and we put > the patch/bugfix on hold until we get an answer? > Sure, feel free to use my last post as starting point, and thanks for filing the bug at Khronos. :) > BR >> >>>>> 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: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
