[Mesa-dev] [PATCH 2/2] i965: Fix brw_finish_batch to grow the batchbuffer.

[Chris Wilson]

Quoting Kenneth Graunke (2017-09-18 21:44:57)
> On Monday, September 18, 2017 11:14:35 AM PDT Chris Wilson wrote:
> > Quoting Kenneth Graunke (2017-09-18 18:56:57)
> > > brw_finish_batch emits commands needed at the end of every batch buffer,
> > > including any workarounds.  In the past, we freed up some "reserved"
> > > batch space before calling it, so we would never have to flush during
> > > it.  This was error prone and easy to screw up, so I deleted it a while
> > > back in favor of growing the batch.
> > > 
> > > There were two problems:
> > > 
> > > 1. We're in the middle of flushing, so brw->no_batch_wrap is guaranteed
> > >    not to be set.  Using BEGIN_BATCH() to emit commands would cause a
> > >    recursive flush rather than growing the buffer as intended.
> > 
> > My bad. I've tripped over that same issue before.
> > 
> > > 2. We already recorded the throttling batch before growing, which
> > >    replaces brw->batch.bo with a different (larger) buffer.  So growing
> > >    would break throttling.
> > 
> > Ooh, in most cases that means we stored a later active handle.
> 
> Not exactly...we would set brw->throttle_batch[0] = brw->batch.bo, then
> allocate a new brw->batch.bo, and throw away the old one (putting it back
> in the BO cache).  So, brw->throttle_batch[0] might not ever be execbuf'd
> at all...or it could get reused for a future batchbuffer.
> 
> Seems like nothing good could come of this.

I was trying to think of the worst case; either you get no throttling,
or too much (after growing in the flush). Jitter either way.

> > > @@ -904,13 +914,6 @@ _intel_batchbuffer_flush_fence(struct brw_context *brw,
> > >                brw->batch.state_relocs.reloc_count);
> > >     }
> > >  
> > > -   brw_finish_batch(brw);
> > > -
> > > -   intel_upload_finish(brw);
> > > -
> > > -   /* Check that we didn't just wrap our batchbuffer at a bad time. */
> > > -   assert(!brw->no_batch_wrap);
> > 
> > Keep a sanity check !brw->no_batch_wrap after flushing?
> 
> I moved the assert(!brw->no_batch_wrap) before brw_finish_batch, so we'd
> still guard against flushing-while-flushing.  We have to do that because
> brw_finish_batch() now whacks it.  I could leave one here too, if you
> like, but that would only guard against brw_finish_batch() bugs, rather
> than bugs in callers...so it didn't seem as useful...

I was thinking of a bug here would be carried until the next update.
Something like an infinite stream of glMemoryBarrier wouldn't reset
no_batch_wrap. I was trying to capture the invariant in that every new
batch starts with no_batch_wrap = false, a slightly different statement
that we should never flush a batch with no_batch_wrap = true.
-Chris