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

[Kenneth Graunke]

On Tuesday, January 17, 2017 8:48:22 PM PST Topi Pohjolainen 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);
> +
> +       brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL);

It sounds like the cited text is suggesting we do:

       brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL |
                                        PIPE_CONTROL_DEPTH_CACHE_FLUSH);

which is illegal according to the PIPE_CONTROL docs in the IVB PRM, but
that text is gone from the latest internal docs.  That said, the newer
internal docs provide /other/ contradictory data...so...who knows?

Assuming we resolve that somehow, the series is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Mark says he's seeing far fewer hangs with this series, so I think it's
a lot better.  But we still got a hang, on a PIPE_CONTROL after a
RECTLIST used for a fast color clear.  So it may not be quite right
yet...

Curro, do you have time to take a look at these?  See if you spot
anything?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170117/8c5329c7/attachment-0001.sig>