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

[Jose Fonseca]

----- Original Message -----
> 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.

Yep, you're right. 

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

I'm not sure. _mesa_test_framebuffer_completeness() is used by non-gallium drivers too, which may be expect the GL semantics of numLayers in height.

Anyway, piglit with llvmpipe is hosed in many places (lots of tests deadlone). I'll need to force MESA_GL_VERSION_OVERRIDE=3.0 on the automated tests for the time being until we iron them out.

Thanks for the review.

Jose