[Mesa-dev] [PATCH 17/17] i965: Disentangle batch and state buffer flushing.

[Kenneth Graunke]

On Wednesday, September 6, 2017 8:49:25 AM PDT Chris Wilson wrote:
> Quoting Kenneth Graunke (2017-09-06 16:33:48)
> > On Wednesday, September 6, 2017 5:26:10 AM PDT Chris Wilson wrote:
> > > Quoting Kenneth Graunke (2017-09-06 01:09:50)
> > > > We now flush the batch when either the batchbuffer or statebuffer
> > > > reaches the original intended batch size, instead of when the sum of
> > > > the two reaches a certain size (which makes no sense now that they're
> > > > separate buffers).
> > > > 
> > > > With this change, we also need to update our "are we near the end?"
> > > > estimate to require separate batch and state buffer space.  I obtained
> > > > these estimates by looking at the size of draw calls in the Unreal 4
> > > > Elemental Demo (using INTEL_DEBUG=flush and always_flush_batch=true).
> > > > 
> > > > This will increase the batch size by perhaps 2-4x, which will almost
> > > > certainly have a performance impact, and may impact overall system
> > > > responsiveness.
> > > 
> > > You also need to update DEBUG_FLUSH:
> > > 
> > > @@ -823,8 +826,8 @@ _intel_batchbuffer_flush_fence(struct brw_context *brw,
> > >        int bytes_for_state = brw->batch.state_used;
> > >        fprintf(stderr, "%s:%d: Batchbuffer flush with %4db (%0.1f%%) (pkt) + "
> > >                "%4db (%0.1f%%) (state)\n", file, line,
> > > -              bytes_for_commands, 100.0f * bytes_for_commands / BATCH_SZ,
> > > -              bytes_for_state, 100.0f * bytes_for_state / STATE_SZ);
> > > +              bytes_for_commands, 100.0f * bytes_for_commands / brw->batch.bo->size,
> > > +              bytes_for_state, 100.0f * bytes_for_state / brw->batch.state_bo->size);
> > >     }
> > 
> > Ah...I'd actually meant to leave it this way.  The flushing still happens
> > when we reach the target size (BATCH_SZ or STATE_SZ), even if we grow...
> > I figured we could report the "we grew the batch" cases as "105% of
> > the target size", so you can see that the batch is over-utilized...
> > 
> > Which I guess is a good point...with that model, we won't grow more than
> > once anyway, because after we finish the one draw, we'll be over BATCH_SZ
> > (or STATE_SZ) and flush.  So it might be reasonable to just allocate
> > (BATCH_SZ * 2) and not have the pretense of making it continually grow...
> 
> Ok, that makes sense but I completely missed that was the intended
> design as I read through the series. I was just expecting for everything
> to keep on growing on demand until some heuristic said enough was
> enough.
> 
> Just make sure that design is described in a comment somewhere around
> grow_buffers().
> -Chris

Yeah...I meant to write a comment and forgot.  I've added these locally:

+/**
+ * Target sizes of the batch and state buffers.  We create the initial
+ * buffers at these sizes, and flush when they're nearly full.  If we
+ * underestimate how close we are to the end, and suddenly need more space
+ * in the middle of a draw, we can grow the buffers, and finish the draw.
+ * At that point, we'll be over our target size, so the next operation
+ * should flush.  Each time we flush the batch, we recreate both buffers
+ * at the original target size, so it doesn't grow without bound.
+ */
 #define BATCH_SZ (8192*sizeof(uint32_t))
 #define STATE_SZ (8192*sizeof(uint32_t))
 
@@ -248,6 +257,15 @@ replace_bo_in_reloc_list(struct brw_reloc_list *rlist,
    }
 }
 
+/**
+ * Grow either the batch or state buffer to a new larger size.
+ *
+ * We can't actually grow buffers, so we allocate a new one, copy over
+ * the existing contents, and update our lists to refer to the new one.
+ *
+ * Note that this is only temporary - each new batch recreates the buffers
+ * at their original target size (BATCH_SZ or STATE_SZ).
+ */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170908/bb2699d5/attachment.sig>