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

[Roland Scheidegger]

Am 03.04.2014 17:35, schrieb 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.
> 

To elaborate a bit why this easily leading to bugs, here's another
oddity: the MaxNumLayers field (which we don't use in gallium, maybe we
should) also does its calculation based on the amount of layers from
each rb attachment. It will set this to 6 for cube maps, and
att->Renderbuffer->Depth otherwise. Guess what Depth is for 1d array
textures... Now this would only affect intel drivers and it's possible
there's some more workarounds for that too somewhere, but it's just
insanity.

Roland