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