[Mesa-dev] [PATCH 2/6] i965/gen4: Remove redundant check for depth when rebasing stencil

[Pohjolainen, Topi]

On Sat, May 20, 2017 at 10:29:53PM +0300, Topi Pohjolainen wrote:
> In case of gen < 6 stencil (if present) is always combined with
> depth. Both stencil and depth attachments point to the same
> physical surface.
> Alignment workaround starts by considering depth and updates
> stencil accordingly. Current logic continues with stencil and
> in vain considers the case where depth would refer to different
> surface than stencil.
> 
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_misc_state.c | 63 ++++++------------------------
>  1 file changed, 11 insertions(+), 52 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index 68a1076..ceefc83 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -295,23 +295,9 @@ brw_workaround_depthstencil_alignment(struct brw_context *brw,
>        }
>  
>        if (stencil_irb) {
> -         stencil_mt = get_stencil_miptree(stencil_irb);
> -         intel_miptree_get_image_offset(stencil_mt,
> -                                        stencil_irb->mt_level,
> -                                        stencil_irb->mt_layer,
> -                                        &stencil_draw_x, &stencil_draw_y);
> -         int stencil_tile_x = stencil_draw_x & tile_mask_x;
> -         int stencil_tile_y = stencil_draw_y & tile_mask_y;
> -
> -         /* If stencil doesn't match depth, then we'll need to rebase stencil
> -          * as well.  (if we hadn't decided to rebase stencil before, the
> -          * post-stencil depth test will also rebase depth to try to match it
> -          * up).
> -          */
> -         if (tile_x != stencil_tile_x ||
> -             tile_y != stencil_tile_y) {
> -            rebase_stencil = true;
> -         }
> +         assert(stencil_irb->mt == depth_irb->mt);
> +         assert(stencil_irb->mt_level == depth_irb->mt_level);
> +         assert(stencil_irb->mt_layer == depth_irb->mt_layer);
>        }
>     }
>  
> @@ -334,6 +320,14 @@ brw_workaround_depthstencil_alignment(struct brw_context *brw,
>     }
>  
>     if (rebase_stencil) {
> +      /* If stencil needs rebase, there isn't a depth attachment and the
> +       * combined depth-stencil is used for stencil only. Otherwise in case
> +       * depth attachment is present both stencil and depth point to the same
> +       * miptree. Rebase of depth is considered first updating stencil
> +       * attachment accordingly - hence stencil is rebased only if there is no
> +       * depth attachment.
> +       */
> +      assert(!depth_irb);
>        perf_debug("HW workaround: blitting stencil level %d to a temporary "
>                   "to fix alignment (stencil tile offset %d,%d)\n",
>                   stencil_irb->mt_level, stencil_tile_x, stencil_tile_y);
> @@ -348,41 +342,6 @@ brw_workaround_depthstencil_alignment(struct brw_context *brw,
>        stencil_tile_x = stencil_draw_x & tile_mask_x;
>        stencil_tile_y = stencil_draw_y & tile_mask_y;
>  
> -      if (depth_irb && depth_irb->mt == stencil_irb->mt) {
> -         intel_miptree_reference(&depth_irb->mt, stencil_irb->mt);
> -         intel_renderbuffer_set_draw_offset(depth_irb);
> -      } else if (depth_irb && !rebase_depth) {
> -         if (tile_x != stencil_tile_x ||
> -             tile_y != stencil_tile_y) {
> -            perf_debug("HW workaround: blitting depth level %d to a temporary "
> -                       "to match stencil level %d alignment (depth tile offset "
> -                       "%d,%d, stencil offset %d,%d)\n",
> -                       depth_irb->mt_level,
> -                       stencil_irb->mt_level,
> -                       tile_x, tile_y,
> -                       stencil_tile_x, stencil_tile_y);
> -
> -            intel_renderbuffer_move_to_temp(brw, depth_irb, invalidate_depth);
> -
> -            tile_x = depth_irb->draw_x & tile_mask_x;
> -            tile_y = depth_irb->draw_y & tile_mask_y;
> -
> -            if (stencil_irb && stencil_irb->mt == depth_mt) {
> -               intel_miptree_reference(&stencil_irb->mt, depth_irb->mt);
> -               intel_renderbuffer_set_draw_offset(stencil_irb);
> -            }
> -
> -            WARN_ONCE(stencil_tile_x != tile_x ||
> -                      stencil_tile_y != tile_y,
> -                      "Rebased stencil tile offset (%d,%d) doesn't match depth "
> -                      "tile offset (%d,%d).\n",
> -                      stencil_tile_x, stencil_tile_y,
> -                      tile_x, tile_y);
> -         }
> -      }
> -   }
> -
> -   if (!depth_irb) {

This line needs to be kept here actually although in practise it doesn't
make any difference. We now fail to set the intra tile offsets in case of
stencil-attachment-only. This doesn't actually break anything that isn't
broken already. As depthstencil.depth_offset isn't set in case of stencil-
attachment-only, failing to set the intra tile offsets doesn't actually
make any difference either.
We should keep this line though just for sake of not making it any more
incorrect than it is.

>        tile_x = stencil_tile_x;
>        tile_y = stencil_tile_y;
>     }
> -- 
> 2.9.3
>