[Mesa-dev] [PATCH v2] i965/gen7_urb: Re-emit PUSH_CONSTANT_ALLOC on some gen9

[Kenneth Graunke]

On Wednesday, August 29, 2018 1:38:51 PM PDT Nanley Chery wrote:
> According to internal docs, some gen9 platforms have a pixel shader push
> constant synchronization issue. Although not listed among said
> platforms, this issue seems to be present on the GeminiLake 2x6's we've
> tested.
> 
> We consider the available workarounds to be too detrimental on
> performance. Instead, we mitigate the issue by applying part of one of
> the workarounds. Re-emit PUSH_CONSTANT_ALLOC at the top of every batch
> (as suggested by Ken).
> 
> Fixes ext_framebuffer_multisample-accuracy piglit test failures with the
> following options:
> * 6 depth_draw small depthstencil
> * 8 stencil_draw small depthstencil
> * 6 stencil_draw small depthstencil
> * 8 depth_resolve small
> * 6 stencil_resolve small depthstencil
> * 4 stencil_draw small depthstencil
> * 16 stencil_draw small depthstencil
> * 16 depth_draw small depthstencil
> * 2 stencil_resolve small depthstencil
> * 6 stencil_draw small
> * all_samples stencil_draw small
> * 2 depth_draw small depthstencil
> * all_samples depth_draw small depthstencil
> * all_samples stencil_resolve small
> * 4 depth_draw small depthstencil
> * all_samples depth_draw small
> * all_samples stencil_draw small depthstencil
> * 4 stencil_resolve small depthstencil
> * 4 depth_resolve small depthstencil
> * all_samples stencil_resolve small depthstencil
> 
> v2: Include more platforms in WA (Ken).
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106865
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93355
> Cc: <mesa-stable at lists.freedesktop.org>
> Tested-by: Mark Janes <mark.a.janes at intel.com>
> ---
>  src/mesa/drivers/dri/i965/gen7_urb.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> I'm not sure I have enough information about what's happening in the HW
> to create a piglit test for this issue.
> 
> diff --git a/src/mesa/drivers/dri/i965/gen7_urb.c b/src/mesa/drivers/dri/i965/gen7_urb.c
> index 2e5f8e60ba9..e7259fc1b8d 100644
> --- a/src/mesa/drivers/dri/i965/gen7_urb.c
> +++ b/src/mesa/drivers/dri/i965/gen7_urb.c
> @@ -118,6 +118,33 @@ gen7_emit_push_constant_state(struct brw_context *brw, unsigned vs_size,
>     const struct gen_device_info *devinfo = &brw->screen->devinfo;
>     unsigned offset = 0;
>  
> +   /* From the SKL PRM, Workarounds section (#878):
> +    *
> +    *    Push constant buffer corruption possible. WA: Insert 2 zero-length
> +    *    PushConst_PS before every intended PushConst_PS update, issue a
> +    *    NULLPRIM after each of the zero len PC update to make sure CS commits
> +    *    them.
> +    *
> +    * This workaround is attempting to solve a pixel shader push constant
> +    * synchronization issue.
> +    *
> +    * There's an unpublished WA that involves re-emitting
> +    * 3DSTATE_PUSH_CONSTANT_ALLOC_PS for every 500-ish 3DSTATE_CONSTANT_PS
> +    * packets. Since our counting methods may not be reliable due to
> +    * context-switching and pre-emption, we instead choose to approximate this
> +    * behavior by re-emitting the packet at the top of the batch.
> +    */
> +   if (brw->ctx.NewDriverState == BRW_NEW_BATCH) {
> +       /* SKL GT2 and GLK 2x6 have reliably demonstrated this issue thus far.
> +        * We've also seen some intermittent failures from SKL GT4 and BXT in
> +        * the past.
> +        */
> +      if (!devinfo->is_skylake &&
> +          !devinfo->is_broxton &&
> +          !devinfo->is_geminilake)
> +         return;
> +   }
> +
>     BEGIN_BATCH(10);
>     OUT_BATCH(_3DSTATE_PUSH_CONSTANT_ALLOC_VS << 16 | (2 - 2));
>     OUT_BATCH(vs_size | offset << GEN7_PUSH_CONSTANT_BUFFER_OFFSET_SHIFT);
> @@ -154,6 +181,7 @@ const struct brw_tracked_state gen7_push_constant_space = {
>     .dirty = {
>        .mesa = 0,
>        .brw = BRW_NEW_CONTEXT |
> +             BRW_NEW_BATCH | /* Push constant workaround */
>               BRW_NEW_GEOMETRY_PROGRAM |
>               BRW_NEW_TESS_PROGRAMS,
>     },
> 

Not sure we can do much better than this.  Thanks for taking care of
this, Nanley.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- 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/20180830/0a2352b6/attachment.sig>