[Mesa-dev] [PATCH] llvmpipe: fix bogus layer clamping in setup

Roland Scheidegger sroland at vmware.com
Fri Oct 25 23:03:33 CEST 2013


Am 25.10.2013 22:33, schrieb Brian Paul:
> On 10/25/2013 10:14 AM, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> The layer coming from GS needs to be clamped (not sure if that's actually
>> the correct error behavior but we need something) as the number can be
>> higher
>> than the amount of layers in the fb. However, this code was using the
>> layer
>> calculation from the scene, and this was actually calculated in
>> lp_scene_begin_rasterization() hence too late (so setup was using the
>> value
>> from the _previous_ scene or just zero if it was the first scene).
>> Since the value is used in both rasterization and setup, move
>> calculation up
>> to lp_scene_begin_binning() though it's a bit more inconvenient to
>> calculate
>> there. (Theoretically could move _all_ code which was in
>> lp_scene_begin_rasterization() to there, because ever since we got rid of
>> swizzled render/depth buffers our "map" functions preparing the fb
>> data for
>> render don't actually change the data in there at all, but it feels like
>> it would be a hack.)
>> ---
>>   src/gallium/drivers/llvmpipe/lp_scene.c |   25
>> ++++++++++++++++++-------
>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/drivers/llvmpipe/lp_scene.c
>> b/src/gallium/drivers/llvmpipe/lp_scene.c
>> index 2abbd25..483bfa5 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_scene.c
>> +++ b/src/gallium/drivers/llvmpipe/lp_scene.c
>> @@ -151,7 +151,6 @@ lp_scene_begin_rasterization(struct lp_scene *scene)
>>   {
>>      const struct pipe_framebuffer_state *fb = &scene->fb;
>>      int i;
>> -   unsigned max_layer = ~0;
>>
>>      //LP_DBG(DEBUG_RAST, "%s\n", __FUNCTION__);
>>
>> @@ -162,7 +161,6 @@ lp_scene_begin_rasterization(struct lp_scene *scene)
>>                                                             
>> cbuf->u.tex.level);
>>            scene->cbufs[i].layer_stride =
>> llvmpipe_layer_stride(cbuf->texture,
>>                                                                
>> cbuf->u.tex.level);
>> -         max_layer = MIN2(max_layer, cbuf->u.tex.last_layer -
>> cbuf->u.tex.first_layer);
>>
>>            scene->cbufs[i].map = llvmpipe_resource_map(cbuf->texture,
>>                                                        cbuf->u.tex.level,
>> @@ -173,7 +171,6 @@ lp_scene_begin_rasterization(struct lp_scene *scene)
>>            struct llvmpipe_resource *lpr =
>> llvmpipe_resource(cbuf->texture);
>>            unsigned pixstride = util_format_get_blocksize(cbuf->format);
>>            scene->cbufs[i].stride = cbuf->texture->width0;
>> -         max_layer = 0;
>>
>>            scene->cbufs[i].map = lpr->data;
>>            scene->cbufs[i].map += cbuf->u.buf.first_element * pixstride;
>> @@ -184,15 +181,12 @@ lp_scene_begin_rasterization(struct lp_scene
>> *scene)
>>         struct pipe_surface *zsbuf = scene->fb.zsbuf;
>>         scene->zsbuf.stride = llvmpipe_resource_stride(zsbuf->texture,
>> zsbuf->u.tex.level);
>>         scene->zsbuf.layer_stride =
>> llvmpipe_layer_stride(zsbuf->texture, zsbuf->u.tex.level);
>> -      max_layer = MIN2(max_layer, zsbuf->u.tex.last_layer -
>> zsbuf->u.tex.first_layer);
>>
>>         scene->zsbuf.map = llvmpipe_resource_map(zsbuf->texture,
>>                                                  zsbuf->u.tex.level,
>>                                                 
>> zsbuf->u.tex.first_layer,
>>                                                 
>> LP_TEX_USAGE_READ_WRITE);
>>      }
>> -
>> -   scene->fb_max_layer = max_layer;
>>   }
>>
>>
>> @@ -506,6 +500,9 @@ end:
>>   void lp_scene_begin_binning( struct lp_scene *scene,
>>                                struct pipe_framebuffer_state *fb,
>> boolean discard )
>>   {
>> +   int i;
>> +   unsigned max_layer = ~0;
>> +
>>      assert(lp_scene_is_empty(scene));
>>
>>      scene->discard = discard;
>> @@ -513,9 +510,23 @@ void lp_scene_begin_binning( struct lp_scene *scene,
>>
>>      scene->tiles_x = align(fb->width, TILE_SIZE) / TILE_SIZE;
>>      scene->tiles_y = align(fb->height, TILE_SIZE) / TILE_SIZE;
>> -
>>      assert(scene->tiles_x <= TILES_X);
>>      assert(scene->tiles_y <= TILES_Y);
>> +
> 
> Maybe add a comment here indicating what we're doing.
Ok.

> 
>> +   for (i = 0; i < scene->fb.nr_cbufs; i++) {
>> +      struct pipe_surface *cbuf = scene->fb.cbufs[i];
>> +      if (llvmpipe_resource_is_texture(cbuf->texture)) {
>> +         max_layer = MIN2(max_layer, cbuf->u.tex.last_layer -
>> cbuf->u.tex.first_layer);
>> +      }
>> +      else {
>> +         max_layer = 0;
>> +      }
>> +   }
>> +   if (fb->zsbuf) {
>> +      struct pipe_surface *zsbuf = scene->fb.zsbuf;
>> +      max_layer = MIN2(max_layer, zsbuf->u.tex.last_layer -
>> zsbuf->u.tex.first_layer);
>> +   }
>> +   scene->fb_max_layer = max_layer;
> 
> Suppose we have a layered color buffer and layered Z/S buffer, but the
> number of layers differs.  Are you sure we shouldn't be using separate
> max_layers for color vs. Z/S?
I believe this should be fine (such a setup is illegal in d3d10 fwiw).
I've put
a comment already at some point to the fb_max_layer variable in
lp_scene.h which reads:
"/* OpenGL permits different amount of layers per rt, but rendering
limited to minimum */"
It is legal in OpenGL not only to have different layers for color and
z/s but also for the different color attachments, so we'd need different
max_layer for each color attachment too. However, the GL wording of this
is (from 4.4 core, 9.8 Layered Framebuffers):
"If the fragment’s layer number is negative, or greater than or equal to
the minimum number of layers of any attachment, the effects of the
fragment on the framebuffer contents are undefined."
So simply clamping to the minimum of any attachment should be enough. I
guess though that comment for the fb_max_layer variable could need some
improvement (as GL should be fine with but does not actually require
this specific behavior). Interestingly enough ARB_geometry_shader4
actually also required that layers count match just like d3d10 (it has a
FRAMEBUFFER_INCOMPLETE_LAYER_COUNT_ARB enum for exactly that purpose).

Roland


> For the time being though I'm fine with the code as-is though.
> 
> Reviewed-by: Brian Paul <brianp at vmware.com>


More information about the mesa-dev mailing list