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

[Roland Scheidegger]

Am 03.04.2014 17:19, schrieb 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.
Oh yes I missed that in my review I'm required to retract my R-b :-).
I think starting with fb->Width/Height and then using MIN2 would work
(but retain all the ugliness).


> 
> Maybe the real issue is in _mesa_test_framebuffer_completeness().  Maybe
> we should do something special with 1D texture array heights there?
> 
Might work, but ultimately I think it should really work all just
automatically. Treating 1d and 2d array textures differently is just a
real pain (that is 1d array textures having height of layers, which
leads to rb having that height, which ultimately means the fb has that
height). If anything, I think it should be at least fixed up at rb
level, not just fb.

Roland