[Mesa-dev] [PATCH 2/4] i965: Make depth clear flushing more explicit

[Jason Ekstrand]

On Tue, Jan 17, 2017 at 10:48 AM, Topi Pohjolainen <
topi.pohjolainen at gmail.com> wrote:

> Current blorp logic issues unconditional "flush everything"
> (see brw_emit_mi_flush()) after each render. For example, all
> blits issue this unconditionally which shouldn't be needed if
> they set render cache properly os that subsequent renders do
> necessary flushing before drawing.
>
> In case of piglit:
>
> ext_framebuffer_multisample-accuracy all_samples depth_draw small
>
> intel_hiz_exec() is always preceded by blorb blit and the
> unconditional flush looks to hide the lack of stall and flushes
> in depth clears. By removing the brw_emit_mi_flush() I get gpu
> hangs.
>
> This patch adds the stalls and flushes mandated by the spec
> and gets rid of those hangs.
>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_clear.c        | 38
> ++++++++++++++++++++++------
>  src/mesa/drivers/dri/i965/gen8_depth_state.c | 16 ++++++++++++
>  2 files changed, 46 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> b/src/mesa/drivers/dri/i965/brw_clear.c
> index 488732c..b053a7d 100644
> --- a/src/mesa/drivers/dri/i965/brw_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_clear.c
> @@ -36,6 +36,7 @@
>
>  #include "brw_context.h"
>  #include "brw_blorp.h"
> +#include "brw_defines.h"
>
>  #define FILE_DEBUG_FLAG DEBUG_BLIT
>
> @@ -174,14 +175,35 @@ brw_fast_clear_depth(struct gl_context *ctx)
>        mt->depth_clear_value = depth_clear_value;
>     }
>
> -   /* From the Sandy Bridge PRM, volume 2 part 1, page 313:
> -    *
> -    *     "If other rendering operations have preceded this clear, a
> -    *      PIPE_CONTROL with write cache flush enabled and Z-inhibit
> disabled
> -    *      must be issued before the rectangle primitive used for the
> depth
> -    *      buffer clear operation.
> -    */
> -   brw_emit_mi_flush(brw);
> +   if (brw->gen == 6) {
> +      /* From the Sandy Bridge PRM, volume 2 part 1, page 313:
> +       *
> +       *   "If other rendering operations have preceded this clear, a
> +       *    PIPE_CONTROL with write cache flush enabled and Z-inhibit
> disabled
> +       *    must be issued before the rectangle primitive used for the
> depth
> +       *    buffer clear operation.
> +       */
> +       brw_emit_pipe_control_flush(brw,
> +                                   PIPE_CONTROL_RENDER_TARGET_FLUSH |
> +                                   PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> +                                   PIPE_CONTROL_CS_STALL);
> +   } else if (brw->gen >= 7) {
> +      /*
> +       * From the IvyBridge PRM, volume 2, "Depth Buffer Clear":
> +       *
> +       *   If other rendering operations have preceded this clear, a
> +       *   PIPE_CONTROL with depth cache flush enabled, Depth Stall bit
> +       *   enabled must be issued before the rectangle primitive used for
> the
> +       *   depth buffer clear operation.
> +       *
> +       * Same applies for Gen8 and Gen9.
> +       */
> +       brw_emit_pipe_control_flush(brw,
> +                                   PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> +                                   PIPE_CONTROL_CS_STALL);
>

Why are you doing a CS stall and not a depth stall?


> +
> +       brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL);
> +   }
>
>     if (fb->MaxNumLayers > 0) {
>        for (unsigned layer = 0; layer < depth_irb->layer_count; layer++) {
> diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> index 14689f4..ec29669 100644
> --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> @@ -511,6 +511,22 @@ gen8_hiz_exec(struct brw_context *brw, struct
> intel_mipmap_tree *mt,
>     OUT_BATCH(0);
>     ADVANCE_BATCH();
>
> +   /*
> +    * From the Broadwell PRM, volume 7, "Depth Buffer Clear":
> +    *
> +    *  Depth buffer clear pass using any of the methods (WM_STATE,
> 3DSTATE_WM
> +    *  or 3DSTATE_WM_HZ_OP) must be followed by a PIPE_CONTROL command
> with
> +    *  DEPTH_STALL bit and Depth FLUSH bits "set" before starting to
> render.
> +    *  DepthStall and DepthFlush are not needed between consecutive depth
> +    *  clear passes nor is it required if th e depth clear pass was done
> with
> +    *  "full_surf_clear" bit set in the 3DSTATE_WM_HZ_OP.
> +    *
> +    *  TODO: Such as the spec says, this could be conditional.
> +    */
> +   brw_emit_pipe_control_flush(brw,
> +                               PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> +                               PIPE_CONTROL_DEPTH_STALL);
> +
>     /* Mark this buffer as needing a TC flush, as we've rendered to it. */
>     brw_render_cache_set_add_bo(brw, mt->bo);
>
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170117/994ec6a2/attachment-0001.html>