On Fri, Aug 24, 2018 at 9:39 PM, Nanley Chery <nanleych...@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 <nanleych...@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-sta...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
