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

[Francisco Jerez]

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.

An alternative would be to do something along the lines of:

| if (!_mesa_is_texture_complete(t, &t->Sampler))
|    return GL_FALSE;

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

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

> +
>     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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160714/6154627f/attachment.sig>