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

[Chad Versace]

On 01/21/2015 03:11 PM, Kenneth Graunke wrote:
> 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?

No, I don't have any workload in mind.

>>> @@ -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.)

Ouch. Chrome OS doesn't.
 
>> 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 :)

So it seems neither have a good way of determining if this patch hurts
general performance, other than running $MY_FAVORITE_APP1 and $MY_FAVORITE_APP2
and looking at the FPS. So I delegate the performance checking to Chrome OS QA.

Honestly, we flush so often on SNB, I don't see how flushing consistently at
the start of each draw/blorp instead of flushing at a random time in
almost every draw/blorp would hurt performance. My intuition would be
surprised if this hurts perf.

Even if this patch does hurt perf some, hangs are worse. And, honestly,
no one will ever spend the time on SNB to discover the exact correct
placement of all flushes. So, this solution looks like the best
attainable solution.

Die, hangs! Die!
Reviewed-by: Chad Versace <chad.versace at intel.com>
(If you add the flush back to brw_upload_invariant_state).


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150126/db62b81c/attachment.sig>