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

Submitted by Jose Fonseca on April 23, 2014, 1:55 p.m.

Details

Message ID 1398261307-27297-2-git-send-email-jfonseca@vmware.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jose Fonseca April 23, 2014, 1:55 p.m.
From: José Fonseca <jfonseca@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

v2: Compute the framebuffer size as the minimum size, as pointed out by
Brian;  compacted code;  ran piglit quick test list (with no
regressions.)
---
 src/mesa/state_tracker/st_atom_framebuffer.c | 33 ++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/state_tracker/st_atom_framebuffer.c b/src/mesa/state_tracker/st_atom_framebuffer.c
index 4c4f839..f395ec7 100644
--- a/src/mesa/state_tracker/st_atom_framebuffer.c
+++ b/src/mesa/state_tracker/st_atom_framebuffer.c
@@ -31,6 +31,8 @@ 
   *   Brian Paul
   */
  
+#include <limits.h>
+
 #include "st_context.h"
 #include "st_atom.h"
 #include "st_cb_bitmap.h"
@@ -44,6 +46,24 @@ 
 
 
 /**
+ * Update framebuffer size.
+ *
+ * framebuffer->width should match fb->Weight, but for PIPE_TEXTURE_1D_ARRAY
+ * textures fb->Height has the number of layers, and not the surface height.
+ */
+static void
+update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
+                        struct pipe_surface *surface)
+{
+   assert(surface);
+   assert(surface->width  < UINT_MAX);
+   assert(surface->height < UINT_MAX);
+   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
+   framebuffer->height = MIN2(framebuffer->height, surface->height);
+}
+
+
+/**
  * Update framebuffer state (color, depth, stencil, etc. buffers)
  */
 static void
@@ -57,11 +77,12 @@  update_framebuffer_state( struct st_context *st )
    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);*/
 
+   framebuffer->width  = UINT_MAX;
+   framebuffer->height = UINT_MAX;
+
    /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
     * to determine which surfaces to draw to
     */
@@ -81,6 +102,7 @@  update_framebuffer_state( struct st_context *st )
 
          if (strb->surface) {
             pipe_surface_reference(&framebuffer->cbufs[i], strb->surface);
+            update_framebuffer_size(framebuffer, strb->surface);
          }
          strb->defined = GL_TRUE; /* we'll be drawing something */
       }
@@ -100,12 +122,14 @@  update_framebuffer_state( struct st_context *st )
          st_update_renderbuffer_surface(st, strb);
       }
       pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
+      update_framebuffer_size(framebuffer, strb->surface);
    }
    else {
       strb = st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
       if (strb) {
          assert(strb->surface);
          pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
+         update_framebuffer_size(framebuffer, strb->surface);
       }
       else
          pipe_surface_reference(&framebuffer->zsbuf, NULL);
@@ -122,6 +146,11 @@  update_framebuffer_state( struct st_context *st )
    }
 #endif
 
+   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
+    * attachments, so this should never happen. */
+   assert(framebuffer->width  != UINT_MAX);
+   assert(framebuffer->height != UINT_MAX);
+
    cso_set_framebuffer(st->cso_context, framebuffer);
 }
 

Comments

On 04/23/2014 07:55 AM, jfonseca@vmware.com wrote:
> From: José Fonseca <jfonseca@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
>
> v2: Compute the framebuffer size as the minimum size, as pointed out by
> Brian;  compacted code;  ran piglit quick test list (with no
> regressions.)
> ---
>   src/mesa/state_tracker/st_atom_framebuffer.c | 33 ++++++++++++++++++++++++++--
>   1 file changed, 31 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..f395ec7 100644
> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
> @@ -31,6 +31,8 @@
>     *   Brian Paul
>     */
>
> +#include <limits.h>
> +
>   #include "st_context.h"
>   #include "st_atom.h"
>   #include "st_cb_bitmap.h"
> @@ -44,6 +46,24 @@
>
>
>   /**
> + * Update framebuffer size.
> + *
> + * framebuffer->width should match fb->Weight, but for PIPE_TEXTURE_1D_ARRAY

"fb->Width"


> + * textures fb->Height has the number of layers, and not the surface height.
> + */

The comment seems a bit disconnected from the code. 
update_framebuffer_size() is used to find the size which is the min of 
the attached surfaces.  The comment about 1D array textures doesn't seem 
to matter in the code.  That just seems a little confusing.


> +static void
> +update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
> +                        struct pipe_surface *surface)
> +{
> +   assert(surface);
> +   assert(surface->width  < UINT_MAX);
> +   assert(surface->height < UINT_MAX);
> +   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
> +   framebuffer->height = MIN2(framebuffer->height, surface->height);
> +}
> +
> +
> +/**
>    * Update framebuffer state (color, depth, stencil, etc. buffers)
>    */
>   static void
> @@ -57,11 +77,12 @@ update_framebuffer_state( struct st_context *st )
>      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);*/
>
> +   framebuffer->width  = UINT_MAX;
> +   framebuffer->height = UINT_MAX;
> +
>      /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
>       * to determine which surfaces to draw to
>       */
> @@ -81,6 +102,7 @@ update_framebuffer_state( struct st_context *st )
>
>            if (strb->surface) {
>               pipe_surface_reference(&framebuffer->cbufs[i], strb->surface);
> +            update_framebuffer_size(framebuffer, strb->surface);
>            }
>            strb->defined = GL_TRUE; /* we'll be drawing something */
>         }
> @@ -100,12 +122,14 @@ update_framebuffer_state( struct st_context *st )
>            st_update_renderbuffer_surface(st, strb);
>         }
>         pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +      update_framebuffer_size(framebuffer, strb->surface);
>      }
>      else {
>         strb = st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
>         if (strb) {
>            assert(strb->surface);
>            pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +         update_framebuffer_size(framebuffer, strb->surface);
>         }
>         else
>            pipe_surface_reference(&framebuffer->zsbuf, NULL);
> @@ -122,6 +146,11 @@ update_framebuffer_state( struct st_context *st )
>      }
>   #endif
>
> +   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
> +    * attachments, so this should never happen. */

Close */ on next line.


> +   assert(framebuffer->width  != UINT_MAX);
> +   assert(framebuffer->height != UINT_MAX);
> +
>      cso_set_framebuffer(st->cso_context, framebuffer);
>   }
>
>

Otherwise, Reviewed-by: Brian Paul <brianp@vmware.com>
Thanks for the review.

----- Original Message -----
> On 04/23/2014 07:55 AM, jfonseca@vmware.com wrote:
> > From: José Fonseca <jfonseca@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
> >
> > v2: Compute the framebuffer size as the minimum size, as pointed out by
> > Brian;  compacted code;  ran piglit quick test list (with no
> > regressions.)
> > ---
> >   src/mesa/state_tracker/st_atom_framebuffer.c | 33
> >   ++++++++++++++++++++++++++--
> >   1 file changed, 31 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..f395ec7 100644
> > --- a/src/mesa/state_tracker/st_atom_framebuffer.c
> > +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
> > @@ -31,6 +31,8 @@
> >     *   Brian Paul
> >     */
> >
> > +#include <limits.h>
> > +
> >   #include "st_context.h"
> >   #include "st_atom.h"
> >   #include "st_cb_bitmap.h"
> > @@ -44,6 +46,24 @@
> >
> >
> >   /**
> > + * Update framebuffer size.
> > + *
> > + * framebuffer->width should match fb->Weight, but for
> > PIPE_TEXTURE_1D_ARRAY
> 
> "fb->Width"
> 
> 
> > + * textures fb->Height has the number of layers, and not the surface
> > height.
> > + */
> 
> The comment seems a bit disconnected from the code.
> update_framebuffer_size() is used to find the size which is the min of
> the attached surfaces.  The comment about 1D array textures doesn't seem
> to matter in the code.  That just seems a little confusing.

Yes, the update_framebuffer_size finds the min size, which I think is obvious.  This comment here was supposed to explain why we do it when gl_framebuffer has similar info, ie., the less obvious bit.

But I agree that the comment could be better phrased.  What about this?

   "We need to derive pipe_framebuffer size from the bound pipe_surfaces here instead of copying gl_framebuffer size because for certain target types (like PIPE_TEXTURE_1D_ARRAY) gl_framebuffer::Height has the number of layers instead of 1."

Jose



> > +static void
> > +update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
> > +                        struct pipe_surface *surface)
> > +{
> > +   assert(surface);
> > +   assert(surface->width  < UINT_MAX);
> > +   assert(surface->height < UINT_MAX);
> > +   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
> > +   framebuffer->height = MIN2(framebuffer->height, surface->height);
> > +}
> > +
> > +
> > +/**
> >    * Update framebuffer state (color, depth, stencil, etc. buffers)
> >    */
> >   static void
> > @@ -57,11 +77,12 @@ update_framebuffer_state( struct st_context *st )
> >      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);*/
> >
> > +   framebuffer->width  = UINT_MAX;
> > +   framebuffer->height = UINT_MAX;
> > +
> >      /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
> >       * to determine which surfaces to draw to
> >       */
> > @@ -81,6 +102,7 @@ update_framebuffer_state( struct st_context *st )
> >
> >            if (strb->surface) {
> >               pipe_surface_reference(&framebuffer->cbufs[i],
> >               strb->surface);
> > +            update_framebuffer_size(framebuffer, strb->surface);
> >            }
> >            strb->defined = GL_TRUE; /* we'll be drawing something */
> >         }
> > @@ -100,12 +122,14 @@ update_framebuffer_state( struct st_context *st )
> >            st_update_renderbuffer_surface(st, strb);
> >         }
> >         pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> > +      update_framebuffer_size(framebuffer, strb->surface);
> >      }
> >      else {
> >         strb =
> >         st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
> >         if (strb) {
> >            assert(strb->surface);
> >            pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> > +         update_framebuffer_size(framebuffer, strb->surface);
> >         }
> >         else
> >            pipe_surface_reference(&framebuffer->zsbuf, NULL);
> > @@ -122,6 +146,11 @@ update_framebuffer_state( struct st_context *st )
> >      }
> >   #endif
> >
> > +   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
> > +    * attachments, so this should never happen. */
> 
> Close */ on next line.
> 
> 
> > +   assert(framebuffer->width  != UINT_MAX);
> > +   assert(framebuffer->height != UINT_MAX);
> > +
> >      cso_set_framebuffer(st->cso_context, framebuffer);
> >   }
> >
> >
> 
> Otherwise, Reviewed-by: Brian Paul <brianp@vmware.com>
>
On 04/23/2014 09:17 AM, Jose Fonseca wrote:
> Thanks for the review.
>
> ----- Original Message -----
>> On 04/23/2014 07:55 AM, jfonseca@vmware.com wrote:
>>> From: José Fonseca <jfonseca@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
>>>
>>> v2: Compute the framebuffer size as the minimum size, as pointed out by
>>> Brian;  compacted code;  ran piglit quick test list (with no
>>> regressions.)
>>> ---
>>>    src/mesa/state_tracker/st_atom_framebuffer.c | 33
>>>    ++++++++++++++++++++++++++--
>>>    1 file changed, 31 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..f395ec7 100644
>>> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
>>> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
>>> @@ -31,6 +31,8 @@
>>>      *   Brian Paul
>>>      */
>>>
>>> +#include <limits.h>
>>> +
>>>    #include "st_context.h"
>>>    #include "st_atom.h"
>>>    #include "st_cb_bitmap.h"
>>> @@ -44,6 +46,24 @@
>>>
>>>
>>>    /**
>>> + * Update framebuffer size.
>>> + *
>>> + * framebuffer->width should match fb->Weight, but for
>>> PIPE_TEXTURE_1D_ARRAY
>>
>> "fb->Width"
>>
>>
>>> + * textures fb->Height has the number of layers, and not the surface
>>> height.
>>> + */
>>
>> The comment seems a bit disconnected from the code.
>> update_framebuffer_size() is used to find the size which is the min of
>> the attached surfaces.  The comment about 1D array textures doesn't seem
>> to matter in the code.  That just seems a little confusing.
>
> Yes, the update_framebuffer_size finds the min size, which I think is obvious.  This comment here was supposed to explain why we do it when gl_framebuffer has similar info, ie., the less obvious bit.
>
> But I agree that the comment could be better phrased.  What about this?
>
>     "We need to derive pipe_framebuffer size from the bound pipe_surfaces here instead of copying gl_framebuffer size because for certain target types (like PIPE_TEXTURE_1D_ARRAY) gl_framebuffer::Height has the number of layers instead of 1."

That sounds great.

-Brian


> Jose
>
>
>
>>> +static void
>>> +update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
>>> +                        struct pipe_surface *surface)
>>> +{
>>> +   assert(surface);
>>> +   assert(surface->width  < UINT_MAX);
>>> +   assert(surface->height < UINT_MAX);
>>> +   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
>>> +   framebuffer->height = MIN2(framebuffer->height, surface->height);
>>> +}
>>> +
>>> +
>>> +/**
>>>     * Update framebuffer state (color, depth, stencil, etc. buffers)
>>>     */
>>>    static void
>>> @@ -57,11 +77,12 @@ update_framebuffer_state( struct st_context *st )
>>>       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);*/
>>>
>>> +   framebuffer->width  = UINT_MAX;
>>> +   framebuffer->height = UINT_MAX;
>>> +
>>>       /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
>>>        * to determine which surfaces to draw to
>>>        */
>>> @@ -81,6 +102,7 @@ update_framebuffer_state( struct st_context *st )
>>>
>>>             if (strb->surface) {
>>>                pipe_surface_reference(&framebuffer->cbufs[i],
>>>                strb->surface);
>>> +            update_framebuffer_size(framebuffer, strb->surface);
>>>             }
>>>             strb->defined = GL_TRUE; /* we'll be drawing something */
>>>          }
>>> @@ -100,12 +122,14 @@ update_framebuffer_state( struct st_context *st )
>>>             st_update_renderbuffer_surface(st, strb);
>>>          }
>>>          pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
>>> +      update_framebuffer_size(framebuffer, strb->surface);
>>>       }
>>>       else {
>>>          strb =
>>>          st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
>>>          if (strb) {
>>>             assert(strb->surface);
>>>             pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
>>> +         update_framebuffer_size(framebuffer, strb->surface);
>>>          }
>>>          else
>>>             pipe_surface_reference(&framebuffer->zsbuf, NULL);
>>> @@ -122,6 +146,11 @@ update_framebuffer_state( struct st_context *st )
>>>       }
>>>    #endif
>>>
>>> +   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
>>> +    * attachments, so this should never happen. */
>>
>> Close */ on next line.
>>
>>
>>> +   assert(framebuffer->width  != UINT_MAX);
>>> +   assert(framebuffer->height != UINT_MAX);
>>> +
>>>       cso_set_framebuffer(st->cso_context, framebuffer);
>>>    }
>>>
>>>
>>
>> Otherwise, Reviewed-by: Brian Paul <brianp@vmware.com>
>>
Am 23.04.2014 17:22, schrieb Brian Paul:
> On 04/23/2014 09:17 AM, Jose Fonseca wrote:
>> Thanks for the review.
>>
>> ----- Original Message -----
>>> On 04/23/2014 07:55 AM, jfonseca@vmware.com wrote:
>>>> From: José Fonseca <jfonseca@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
>>>>
>>>> v2: Compute the framebuffer size as the minimum size, as pointed out by
>>>> Brian;  compacted code;  ran piglit quick test list (with no
>>>> regressions.)
>>>> ---
>>>>    src/mesa/state_tracker/st_atom_framebuffer.c | 33
>>>>    ++++++++++++++++++++++++++--
>>>>    1 file changed, 31 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..f395ec7 100644
>>>> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
>>>> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
>>>> @@ -31,6 +31,8 @@
>>>>      *   Brian Paul
>>>>      */
>>>>
>>>> +#include <limits.h>
>>>> +
>>>>    #include "st_context.h"
>>>>    #include "st_atom.h"
>>>>    #include "st_cb_bitmap.h"
>>>> @@ -44,6 +46,24 @@
>>>>
>>>>
>>>>    /**
>>>> + * Update framebuffer size.
>>>> + *
>>>> + * framebuffer->width should match fb->Weight, but for
>>>> PIPE_TEXTURE_1D_ARRAY
>>>
>>> "fb->Width"
>>>
>>>
>>>> + * textures fb->Height has the number of layers, and not the surface
>>>> height.
>>>> + */
>>>
>>> The comment seems a bit disconnected from the code.
>>> update_framebuffer_size() is used to find the size which is the min of
>>> the attached surfaces.  The comment about 1D array textures doesn't seem
>>> to matter in the code.  That just seems a little confusing.
>>
>> Yes, the update_framebuffer_size finds the min size, which I think is
>> obvious.  This comment here was supposed to explain why we do it when
>> gl_framebuffer has similar info, ie., the less obvious bit.
>>
>> But I agree that the comment could be better phrased.  What about this?
>>
>>     "We need to derive pipe_framebuffer size from the bound
>> pipe_surfaces here instead of copying gl_framebuffer size because for
>> certain target types (like PIPE_TEXTURE_1D_ARRAY)
>> gl_framebuffer::Height has the number of layers instead of 1."
> 
> That sounds great.
> 
> -Brian
> 
> 
>> Jose
>>
>>
>>
>>>> +static void
>>>> +update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
>>>> +                        struct pipe_surface *surface)
>>>> +{
>>>> +   assert(surface);
>>>> +   assert(surface->width  < UINT_MAX);
>>>> +   assert(surface->height < UINT_MAX);
>>>> +   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
>>>> +   framebuffer->height = MIN2(framebuffer->height, surface->height);
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>>     * Update framebuffer state (color, depth, stencil, etc. buffers)
>>>>     */
>>>>    static void
>>>> @@ -57,11 +77,12 @@ update_framebuffer_state( struct st_context *st )
>>>>       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);*/
>>>>
>>>> +   framebuffer->width  = UINT_MAX;
>>>> +   framebuffer->height = UINT_MAX;
>>>> +
>>>>       /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
>>>>        * to determine which surfaces to draw to
>>>>        */
>>>> @@ -81,6 +102,7 @@ update_framebuffer_state( struct st_context *st )
>>>>
>>>>             if (strb->surface) {
>>>>                pipe_surface_reference(&framebuffer->cbufs[i],
>>>>                strb->surface);
>>>> +            update_framebuffer_size(framebuffer, strb->surface);
>>>>             }
>>>>             strb->defined = GL_TRUE; /* we'll be drawing something */
>>>>          }
>>>> @@ -100,12 +122,14 @@ update_framebuffer_state( struct st_context *st )
>>>>             st_update_renderbuffer_surface(st, strb);
>>>>          }
>>>>          pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
>>>> +      update_framebuffer_size(framebuffer, strb->surface);
>>>>       }
>>>>       else {
>>>>          strb =
>>>>          st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
>>>>          if (strb) {
>>>>             assert(strb->surface);
>>>>             pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
>>>> +         update_framebuffer_size(framebuffer, strb->surface);
>>>>          }
>>>>          else
>>>>             pipe_surface_reference(&framebuffer->zsbuf, NULL);
>>>> @@ -122,6 +146,11 @@ update_framebuffer_state( struct st_context *st )
>>>>       }
>>>>    #endif
>>>>
>>>> +   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
>>>> +    * attachments, so this should never happen. */
>>>
>>> Close */ on next line.
>>>
>>>
>>>> +   assert(framebuffer->width  != UINT_MAX);
>>>> +   assert(framebuffer->height != UINT_MAX);
>>>> +
>>>>       cso_set_framebuffer(st->cso_context, framebuffer);
>>>>    }
>>>>
>>>>
>>>
>>> Otherwise, Reviewed-by: Brian Paul <brianp@vmware.com>
>>>

Series looks good to me too. Though I still think one day we don't want
to use framebuffer height in core mesa to mean layers for 1d arrays...

Roland