[Mesa-dev] [PATCH] i965: Do Sandybridge workaround flushes before each primitive.

[Kenneth Graunke]

On Wednesday, January 21, 2015 11:59:39 AM Chad Versace wrote:
> On 01/09/2015 11:07 PM, Kenneth Graunke wrote:
> > Sandybridge requires the post-sync non-zero workaround in a ton of
> > places, and if you ever miss one, the GPU usually hangs.
> > 
> > Currently, we try to track exactly when a workaround flush is
> > necessary (via the brw->batch.need_workaround_flush flag).  This is
> > tricky to get right, and we've botched it several times in the past.
> > 
> > This patch unconditionally performs the post-sync non-zero flush at the
> > start of each primitive's state upload (including BLORP).  We drop the
> > needs_workaround_flush flag, and drop all the other callers, as the
> > flush has already been performed.
> > 
> > We have no data to indicate that simply flushing all the time will
> > hurt performance, and it has the potential to help stability.
> > 
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Ken, what prompted you to write this patch? Did a SNB hang bite you
> recently?

I actually wrote a similar patch back in January 2014, which dropped the flag,
but continued doing the flush all through the code.  During review, Eric
suggested doing it once at the top instead.

https://freedesktop.org/patch/19180/

I keep seeing reports of SNB GPU hangs go by occasionally, so I figured I'd
throw the patch out there and see if it helped anybody.  I'm not aware of it
helping anything, but there are some mystery hangs left.

> I know how painful it is to solve i-forgot-fush-hangs on SNB, so
> I'm in favor of this patch if it doesn't hurt performainvariant_statence?
> 
> Before I give it a rb, I'd like to know how you confirmed that it
> didn't hurt perf.

I don't remember.  Is there anything you'd like me to check?

> > @@ -883,10 +863,6 @@ brw_upload_invariant_state(struct brw_context *brw)
> >  {
> >     const bool is_965 = brw->gen == 4 && !brw->is_g4x;
> >  
> > -   /* 3DSTATE_SIP, 3DSTATE_MULTISAMPLE, etc. are nonpipelined. */
> > -   if (brw->gen == 6)
> > -      intel_emit_post_sync_nonzero_flush(brw);
> > -
> 
> With this hunk, no workaround flush happens before uploading the invariant
> state if hw ctx is enabled (which it isn't on Chrome OS btw).

Which reminds me - does ChromeOS add brw_invariant_state back to gen6_atoms?
(It definitely should.)

> Pre-patch, the flush did happen because intel_batchbuffer_init()
> set need_workaround_flush = true. This worries me. Should I be worried?
>
> Since this patch's goal is to be overly cautious, I would like to be
> overlay cautious here too and continue to do the flush.

I agree - we should keep that flush, if only to be cautious.
Apparently I was a bit overzealous with the delete key :)

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150121/3e294a22/attachment-0001.sig>