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

[Brian Paul]

On 04/03/2014 08:57 AM, jfonseca at vmware.com wrote:
> 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);
>   }

I think this is going to change the behavior for a framebuffer 
consisting of ordinary 2D surfaces but of mixed sizes.

In _mesa_test_framebuffer_completeness() we compute the 
gl_framebuffer:width/height as the min of the surfaces' dimensions.  So 
the original update_framebuffer_state() code set 
pipe_framebuffer_state::width/height to the min of the surfaces' dimensions.

Now, pipe_framebuffer_state::width/height is getting the max 
width/height of all attached surfaces.

I'm not sure how/if all the gallium drivers use the 
pipe_framebuffer_state::width/height params but they'll be getting 
different numbers now with mixed-sized framebuffers.

I'm not sure if that's a problem.

Maybe the real issue is in _mesa_test_framebuffer_completeness().  Maybe 
we should do something special with 1D texture array heights there?

-Brian