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

[Nanley Chery]

On Thu, Aug 30, 2018 at 02:37:40PM -0700, Kenneth Graunke wrote:
> 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>

Same here. Thanks.