[Mesa-dev] [PATCH] i965/gen7_urb: Re-emit PUSH_CONSTANT_ALLOC on GLK

[Nanley Chery]

On Fri, Aug 24, 2018 at 09:17:03PM -0400, Ilia Mirkin wrote:
> On Fri, Aug 24, 2018 at 8:46 PM, Nanley Chery <nanleychery at gmail.com> 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
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106865
> > Cc: <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/mesa/drivers/dri/i965/gen7_urb.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen7_urb.c b/src/mesa/drivers/dri/i965/gen7_urb.c
> > index 2e5f8e60ba9..cb045251236 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_urb.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_urb.c
> > @@ -118,6 +118,28 @@ 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) {
> 
> Did you want & here?
> 

Using & would prevent push constant allocation on non-GLK 2x6 devices
if we had a NEW_BATCH and NEW_GEOMETRY_PROGRAM, which I think we don't
want.

If the equality fails, we'll emit push constant allocation packets,
which is what we want. This block basically filters out the cases in
which we're emitting this packet unnecessarily due to adding the
BRW_NEW_BATCH dirty flag below.

-Nanley

> > +       /* Only GLK 2x6 has demonstrated this issue thus far. */
> > +      if (!devinfo->is_geminilake || devinfo->num_subslices[0] != 2)
> > +         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 +176,7 @@ const struct brw_tracked_state gen7_push_constant_space = {
> >     .dirty = {
> >        .mesa = 0,
> >        .brw = BRW_NEW_CONTEXT |
> > +             BRW_NEW_BATCH | /* GLK workaround */
> >               BRW_NEW_GEOMETRY_PROGRAM |
> >               BRW_NEW_TESS_PROGRAMS,
> >     },
> > --
> > 2.18.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev