[Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level

[Alejandro Piñeiro]

On 23/07/16 00:31, Francisco Jerez wrote:
> Alejandro Piñeiro <apinheiro at igalia.com> writes:
>
>> Hi,
>>
>> On 15/07/16 22:46, Francisco Jerez wrote:
>>> Alejandro Piñeiro <apinheiro at igalia.com> writes:
>>>
>>>> On 14/07/16 21:24, Francisco Jerez wrote:
>>>>> Alejandro Piñeiro <apinheiro at 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...
>
>>>>> 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.
>
> 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 -- However that spec change would have the side effect of making
> the CTS test you're trying to fix non-compliant...

And as I moved to the next CTS test of the list, just a comment that
seems to suggest that this contradiction exists.

The CTS test GL44-CTS.shader_image_load_store.multiple-uniforms fails on
Broadwell/Skylake, but it was working on Haswell. But when I tested with
the patch for incomplete_textures, it started to fail unless I modify
the CTS to explicitly set the min/mag filtering. A change equivalent to
the one I sent to the piglit test, and you pointed before.

>
>>>> 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160726/13d39609/attachment-0001.sig>