[Mesa-dev] [PATCH] st: Add cubeMapFace parameter to st_finalize_texture.

[Nicolai Hähnle]

Hi Michal,

thanks for the patch. That piglit test actually fails on radeonsi as well.


On 28.03.2017 22:39, Michal Srb wrote:
> st_finalize_texture always accesses image at face 0, but it may not be set if we are working with cubemap that had other face set.
>
> This fixes crash in piglit same-attachment-glFramebufferTexture2D-GL_DEPTH_STENCIL_ATTACHMENT.

Please make sure commit messages are wrapped to <75 characters.

Also:

Cc: mesa-stable at lists.freedesktop.org


> ---
> Hi, this is my attempt to fix crash in piglit test same-attachment-glFramebufferTexture2D-GL_DEPTH_STENCIL_ATTACHMENT ran with LIBGL_ALWAYS_INDIRECT=1.
> I am not sure if it is the right approach. From what I found online rendering into a face of a cube texture that doesn't have all faces set would be invalid, but the test passes with other drivers, so maybe it's ok.
> This makes it pass with software rendering as well.

I actually don't see anything in the spec that would require texture 
completeness. That makes sense, since rendering into one image of a 
texture doesn't imply using sampler state. So allowing the test to pass 
is good.

The flip-side is that this means calling st_finalize_texture at all may 
not be the right thing to do in the FBO code (except perhaps as an 
opportunistic optimization). After all, we could have a messed up 
situation where there are incompatible mip level in a texture, and we 
render to one of them anyway.

Cleaning that up would be quite involved. I think this fix is fine for 
now, since it does improve the situation:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

Let's see if there are any other opinions...

Cheers,
Nicolai


>
>  src/gallium/state_trackers/dri/dri2.c    | 2 +-
>  src/mesa/state_tracker/st_atom_image.c   | 2 +-
>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>  src/mesa/state_tracker/st_cb_fbo.c       | 2 +-
>  src/mesa/state_tracker/st_cb_texture.c   | 5 +++--
>  src/mesa/state_tracker/st_cb_texture.h   | 3 ++-
>  src/mesa/state_tracker/st_gen_mipmap.c   | 2 +-
>  7 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> index b50e096..ed6004f 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -1808,7 +1808,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>           return MESA_GLINTEROP_INVALID_MIP_LEVEL;
>        }
>
> -      if (!st_finalize_texture(ctx, st->pipe, obj)) {
> +      if (!st_finalize_texture(ctx, st->pipe, obj, 0)) {
>           mtx_unlock(&ctx->Shared->Mutex);
>           return MESA_GLINTEROP_OUT_OF_RESOURCES;
>        }
> diff --git a/src/mesa/state_tracker/st_atom_image.c b/src/mesa/state_tracker/st_atom_image.c
> index 5dd2cd6..4101552 100644
> --- a/src/mesa/state_tracker/st_atom_image.c
> +++ b/src/mesa/state_tracker/st_atom_image.c
> @@ -64,7 +64,7 @@ st_bind_images(struct st_context *st, struct gl_program *prog,
>        struct pipe_image_view *img = &images[i];
>
>        if (!_mesa_is_image_unit_valid(st->ctx, u) ||
> -          !st_finalize_texture(st->ctx, st->pipe, u->TexObj) ||
> +          !st_finalize_texture(st->ctx, st->pipe, u->TexObj, 0) ||
>            !stObj->pt) {
>           memset(img, 0, sizeof(*img));
>           continue;
> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
> index 92023e0..5b481ec 100644
> --- a/src/mesa/state_tracker/st_atom_texture.c
> +++ b/src/mesa/state_tracker/st_atom_texture.c
> @@ -73,7 +73,7 @@ update_single_texture(struct st_context *st,
>     }
>     stObj = st_texture_object(texObj);
>
> -   retval = st_finalize_texture(ctx, st->pipe, texObj);
> +   retval = st_finalize_texture(ctx, st->pipe, texObj, 0);
>     if (!retval) {
>        /* out of mem */
>        return GL_FALSE;
> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
> index 78433bf..dce4239 100644
> --- a/src/mesa/state_tracker/st_cb_fbo.c
> +++ b/src/mesa/state_tracker/st_cb_fbo.c
> @@ -488,7 +488,7 @@ st_render_texture(struct gl_context *ctx,
>     struct st_renderbuffer *strb = st_renderbuffer(rb);
>     struct pipe_resource *pt;
>
> -   if (!st_finalize_texture(ctx, pipe, att->Texture))
> +   if (!st_finalize_texture(ctx, pipe, att->Texture, att->CubeMapFace))
>        return;
>
>     pt = st_get_texobj_resource(att->Texture);
> diff --git a/src/mesa/state_tracker/st_cb_texture.c b/src/mesa/state_tracker/st_cb_texture.c
> index bc6f108..1b486d7 100644
> --- a/src/mesa/state_tracker/st_cb_texture.c
> +++ b/src/mesa/state_tracker/st_cb_texture.c
> @@ -2434,7 +2434,8 @@ copy_image_data_to_texture(struct st_context *st,
>  GLboolean
>  st_finalize_texture(struct gl_context *ctx,
>  		    struct pipe_context *pipe,
> -		    struct gl_texture_object *tObj)
> +		    struct gl_texture_object *tObj,
> +		    GLuint cubeMapFace)
>  {
>     struct st_context *st = st_context(ctx);
>     struct st_texture_object *stObj = st_texture_object(tObj);
> @@ -2478,7 +2479,7 @@ st_finalize_texture(struct gl_context *ctx,
>
>     }
>
> -   firstImage = st_texture_image_const(_mesa_base_tex_image(&stObj->base));
> +   firstImage = st_texture_image_const(stObj->base.Image[cubeMapFace][stObj->base.BaseLevel]);
>     assert(firstImage);
>
>     /* If both firstImage and stObj point to a texture which can contain
> diff --git a/src/mesa/state_tracker/st_cb_texture.h b/src/mesa/state_tracker/st_cb_texture.h
> index 415d59f..f647b16 100644
> --- a/src/mesa/state_tracker/st_cb_texture.h
> +++ b/src/mesa/state_tracker/st_cb_texture.h
> @@ -47,7 +47,8 @@ st_get_blit_mask(GLenum srcFormat, GLenum dstFormat);
>  extern GLboolean
>  st_finalize_texture(struct gl_context *ctx,
>  		    struct pipe_context *pipe,
> -		    struct gl_texture_object *tObj);
> +		    struct gl_texture_object *tObj,
> +		    GLuint cubeMapFace);
>
>
>  extern void
> diff --git a/src/mesa/state_tracker/st_gen_mipmap.c b/src/mesa/state_tracker/st_gen_mipmap.c
> index 10af11e..16b914a 100644
> --- a/src/mesa/state_tracker/st_gen_mipmap.c
> +++ b/src/mesa/state_tracker/st_gen_mipmap.c
> @@ -125,7 +125,7 @@ st_generate_mipmap(struct gl_context *ctx, GLenum target,
>         *
>         * After this, we'll have all mipmap levels in one resource.
>         */
> -      st_finalize_texture(ctx, st->pipe, texObj);
> +      st_finalize_texture(ctx, st->pipe, texObj, 0);
>     }
>
>     pt = stObj->pt;
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.