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

[Jason Ekstrand]

On Jan 18, 2017 2:22 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.

v2 (Jason, Ken): Document the rational for separating
                 depth cache flush and stall on Gen7.

Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
---
 src/mesa/drivers/dri/i965/brw_clear.c        | 49
+++++++++++++++++++++++-----
 src/mesa/drivers/dri/i965/gen8_depth_state.c | 16 +++++++++
 2 files changed, 57 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..7fcde6c 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,46 @@ 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.
+       *
+       * In addition, from the Ivybridge PRM, volume 2, 1.10.4.1
PIPE_CONTROL,
+       * Depth Cache Flush Enable:
+       *
+       *   This bit must not be set when Depth Stall Enable bit is set in
+       *   this packet.
+       *
+       * This is confirmed to hold for real, HSW gets immediate gpu hangs.
+       *
+       * Therefore issue two pipe control flushes, one for cache flush and
+       * another for depth stall.
+       */


Makes more sense, thanks!  All 4 are

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

That said... My intuition about flushing indicates that you may be able to
get away with just doing a depth stall and then a flush without the CS
stall.  That would potentially let is avoid the CS stall.  I could be
completely wrong though and I think what you did is probably the safest way
to go.

+       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);
+   }

    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/20170118/72757c3f/attachment-0001.html>