[Mesa-dev] [PATCH 4/4] i965: Reimplement all the PIPE_CONTROL rules.

[Kenneth Graunke]

On Monday, February 25, 2019 11:01:33 AM PST Pohjolainen, Topi wrote:
> On Mon, Feb 25, 2019 at 10:32:27AM -0800, Kenneth Graunke wrote:
> > On Monday, February 25, 2019 6:33:11 AM PST Pohjolainen, Topi wrote:
> > > On Thu, Nov 01, 2018 at 08:04:21PM -0700, Kenneth Graunke wrote:
> > > > -         if (GEN_GEN >= 9) {
> > > > -            /* THE PIPE_CONTROL "VF Cache Invalidation Enable" docs continue:
> > > > -             *
> > > > -             *    "Project: BDW+
> > > > -             *
> > > > -             *     When VF Cache Invalidate is set “Post Sync Operation” must
> > > > -             *     be enabled to “Write Immediate Data” or “Write PS Depth
> > > > -             *     Count” or “Write Timestamp”."
> > > > -             *
> > > > -             * If there's a BO, we're already doing some kind of write.
> > > > -             * If not, add a write to the workaround BO.
> > > > -             *
> > > > -             * XXX: This causes GPU hangs on Broadwell, so restrict it to
> > > > -             *      Gen9+ for now...see this bug for more information:
> > > > -             *      https://bugs.freedesktop.org/show_bug.cgi?id=103787
> > > 
> > > In "Flush Types" workarounds later on you apply this for gen8 as well.
> > 
> > Yes, that's intentional - we're supposed to according to the docs.
> > I re-tested the Piglit test from bug 103787 on my Broadwell, and it
> > works fine - no GPU hangs.  I think we were just doing it wrong before.
> > 
> > Trying to figure out an ordering for the workarounds is awful... :(
> 
> What would you think about another patch just before this to enable that for
> gen8? Just in case it causes problems it would bisect to much smaller patch.

It isn't simply enabling it though...in the old code, we had:

      if (devinfo->gen == 8)
         gen8_add_cs_stall_workaround_bits(&flags);
         
      if (flags & PIPE_CONTROL_VF_CACHE_INVALIDATE) {
         if (devinfo->gen >= 9) {
            ...
            if (!bo) {
               flags |= PIPE_CONTROL_WRITE_IMMEDIATE;
               bo = brw->workaround_bo;
            }
         }
      }

Which adds a WRITE_IMMEDIATE to the current PIPE_CONTROL, but does so
after the call to gen8_add_cs_stall_workaround_bits - and that function
would have added a CS_STALL had it seen the WRITE_IMMEDIATE.  I suspect
this bad ordering is why we saw hangs on Broadwell - we missed a stall.

The new code performs these in the opposite order, correctly adding
the necessary CS_STALL.

I could probably write a patch to swap those and enable it on Gen8+.

--Ken
-------------- 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/20190225/98adb629/attachment.sig>