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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
