On 12/20/2013 07:56 AM, Paul Berry wrote:
On 20 December 2013 04:47, Chad Versace <chad.vers...@linux.intel.com>wrote:
We need to emit depth stall flushes before depth and hiz resolves.
Placing them at the top of blorp's state emission fixes the hang.
Fixes HiZ hang in the new WebGL Google maps on Sandybridge Chrome OS.
Tested by zooming in and out continuously for 2 hours.
This patch is based on
https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/8bc07bb70163c3706fb4ba5f980e57dc942f56dd
CC: mesa-sta...@lists.freedesktop.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70740
Signed-off-by: Stéphane Marchesin <marc...@chromium.org>
Signed-off-by: Chad Versace <chad.vers...@linux.intel.com>
---
src/mesa/drivers/dri/i965/gen6_blorp.cpp | 12 ++++++++++++
1 file changed, 12 insertions(+)
Are you aware of any text in the bspec saying that these flushes are
necessary? If so it would be nice to quote it in a comment. I searched
for a while and wasn't able to find anything.
I found nothing in the BSpec stating that flushes were needed here. I began
sprinkling flushes around the code in hope of solving a HiZ hang, and this
location (found by marcheu) gave the needed fix.
I know that's not the answer you wanted.
It can take over an hour to reproduce the hang. So, after finding a fix,
I didn't want to continue to iterate to find the exact location of the
needed flush.
diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
index 6a5841f..3a0e7ec 100644
--- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
+++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
@@ -1012,6 +1012,16 @@ gen6_blorp_emit_primitive(struct brw_context *brw,
ADVANCE_BATCH();
}
+static void
+gen6_emit_hiz_workaround(struct brw_context *brw, enum gen6_hiz_op hiz_op)
+{
+ if (hiz_op == GEN6_HIZ_OP_DEPTH_RESOLVE ||
+ hiz_op == GEN6_HIZ_OP_HIZ_RESOLVE) {
Should we also include GEN6_HIZ_OP_DEPTH_CLEAR?
Perhaps. But I don't want to add that change without re-validating the patch.
Perhaps the extra flush on GEN6_HIZ_OP_DEPTH_CLEAR will introduce some unforseen
problem. It's unlikely, but I don't want to alter a patch for mesa-stable
without
validation.
I prefer to flush on all the hiz ops, if only to prevent future Sandybridge
hangs
from haunting us. On Monday, I'll add the change and re-validate. I assume v2 of
the patch automatically gets your r-b.
I found this text in the
bspec that suggests maybe we should (Graphics BSpec: 3D-Media-GPGPU Engine
3D Pipeline Stages > Pixel > Depth and Stencil > Hierarchical Depth
Buffer > Depth Buffer Clear):
The following is required when performing a depth buffer clear with using
the WM_STATE or 3DSTATE_WM:
- 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.
And later on the same page:
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 the depth-clear pass was done with
“full_surf_clear” bit set in the 3DSTATE_WM_HZ_OP.
(Note, however that these depth clear flushes apply to all versions of the
hardware, so perhaps we should handle them in a different patch and a
different place in the code).
I think we should handle these workarounds with a follow-up patch. We have
a patch that's tested and proven to solve a hang, so let's proceed with it
mostly-as-is for now so users can get the fix asap.
Regardless of whether you make any changes due to my comments above, the
patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
+ brw->batch.need_workaround_flush = true;
+ intel_emit_post_sync_nonzero_flush(brw);
+ intel_emit_depth_stall_flushes(brw);
+ }
+}
/**
* \brief Execute a blit or render pass operation.
@@ -1034,6 +1044,8 @@ gen6_blorp_exec(struct brw_context *brw,
uint32_t wm_bind_bo_offset = 0;
uint32_t prog_offset = params->get_wm_prog(brw, &prog_data);
+
+ gen6_emit_hiz_workaround(brw, params->hiz_op);
gen6_emit_3dstate_multisample(brw, params->dst.num_samples);
gen6_emit_3dstate_sample_mask(brw,
params->dst.num_samples > 1 ?
--
1.8.4
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
