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

[Pohjolainen, Topi]

On Mon, Feb 25, 2019 at 02:18:52PM -0800, Kenneth Graunke wrote:
> 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+.

Ah, okay. Well, I think mentioning this in the commit would be sufficient
as well. But it is your call, I'm fine either way.