[Mesa-dev] [PATCH] mesa/st: Fix pipe_framebuffer_state::height for PIPE_TEXTURE_1D_ARRAY.

[Roland Scheidegger]

Am 03.04.2014 16:57, schrieb jfonseca at vmware.com:
> From: José Fonseca <jfonseca at vmware.com>
> 
> This prevents buffer overflow w/ llvmpipe when running piglit
> 
>   bin/gl-3.2-layered-rendering-clear-color-all-types 1d_array single_level -fbo -auto
> ---
>  src/mesa/state_tracker/st_atom_framebuffer.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/state_tracker/st_atom_framebuffer.c b/src/mesa/state_tracker/st_atom_framebuffer.c
> index 4c4f839..f8eb1f0 100644
> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
> @@ -52,13 +52,13 @@ update_framebuffer_state( struct st_context *st )
>     struct pipe_framebuffer_state *framebuffer = &st->state.framebuffer;
>     struct gl_framebuffer *fb = st->ctx->DrawBuffer;
>     struct st_renderbuffer *strb;
> +   unsigned width = 0;
> +   unsigned height = 0;
>     GLuint i;
>  
>     st_flush_bitmap_cache(st);
>  
>     st->state.fb_orientation = st_fb_orientation(fb);
> -   framebuffer->width = fb->Width;
> -   framebuffer->height = fb->Height;
>  
>     /*printf("------ fb size %d x %d\n", fb->Width, fb->Height);*/
>  
> @@ -81,6 +81,8 @@ update_framebuffer_state( struct st_context *st )
>  
>           if (strb->surface) {
>              pipe_surface_reference(&framebuffer->cbufs[i], strb->surface);
> +            width  = MAX2(width,  strb->surface->width);
> +            height = MAX2(height, strb->surface->height);
>           }
>           strb->defined = GL_TRUE; /* we'll be drawing something */
>        }
> @@ -100,12 +102,18 @@ update_framebuffer_state( struct st_context *st )
>           st_update_renderbuffer_surface(st, strb);
>        }
>        pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +      if (strb->surface) {
> +         width  = MAX2(width,  strb->surface->width);
> +         height = MAX2(height, strb->surface->height);
> +      }
>     }
>     else {
>        strb = st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
>        if (strb) {
>           assert(strb->surface);
>           pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +         width  = MAX2(width,  strb->surface->width);
> +         height = MAX2(height, strb->surface->height);
>        }
>        else
>           pipe_surface_reference(&framebuffer->zsbuf, NULL);
> @@ -122,6 +130,13 @@ update_framebuffer_state( struct st_context *st )
>     }
>  #endif
>  
> +   /*
> +    * framebuffer->width should match fb->Weight, but for PIPE_TEXTURE_1D_ARRAY
> +    * fb->Height has the number of layers as opposed to height.
> +    */
> +   framebuffer->width  = width;
> +   framebuffer->height = height;
> +
>     cso_set_framebuffer(st->cso_context, framebuffer);
>  }
>  
> 

Do you really need all that additional MAX2 code given that the
calculated fb->Width/Height already did that to determine the values? I
guess though it can't hurt, and otherwise you'd hae to check the
dimension of the furst populated attachment.
So, Reviewed-by: Roland Scheidegger <sroland at vmware.com>

I think though longer term it would a MUCH better idea to not treat 1d
array textures as 2d textures _anywhere_ in core mesa (except when
dictated by the API), as far as I can tell it just requires everyone
(from classic drivers / meta code to st) to do workarounds everywhere
(or forget about it and face some weird bugs as a consequence...). A 1d
array texture is just very different to a 2d texture.

Roland