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

[Roland Scheidegger]

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 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
>>>>
>>>> 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 at 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