[Mesa-stable] [Mesa-dev] [PATCH] i965/gen6: Fix HiZ hang in WebGL Google maps

[Kenneth Graunke]

On 12/20/2013 07:56 AM, Paul Berry wrote:
> On 20 December 2013 04:47, Chad Versace <chad.versace at linux.intel.com
> <mailto:chad.versace at 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-stable at lists.freedesktop.org
>     <mailto:mesa-stable at lists.freedesktop.org>
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70740
>     Signed-off-by <https://bugs.freedesktop.org/show_bug.cgi?id=70740
>     Signed-off-by>: Stéphane Marchesin <marcheu at chromium.org
>     <mailto:marcheu at chromium.org>>
>     Signed-off-by: Chad Versace <chad.versace at linux.intel.com
>     <mailto:chad.versace at 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.
>  
> 
> 
>     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?  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.

We already at least attempt to do this: before emitting
3DSTATE_DEPTH_BUFFER, we call intel_emit_depth_stall_flushes(), both in
the main rendering code and from BLORP.  Since all depth clears go
through one of those two methods, the flushes do happen before rendering.

Maybe the timing is slightly off.  Chad's patch does add an additional
set of these flushes.