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

[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.

> +   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?

For the time being though I'm fine with the code as-is though.

Reviewed-by: Brian Paul <brianp at vmware.com>