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

[Ilia Mirkin]

On Fri, Aug 24, 2018 at 9:39 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 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.

Got it. You want to bail if only NEW_BATCH is set and it's not GLK
2x6. Makes sense. Sorry for the noise!

  -ilia