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

[Chad Versace]

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

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 <stereotype441 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>