[Mesa-dev] [PATCH 1/4] i965: Track last location of bo used for the batch

[Chris Wilson]

Quoting Daniel Vetter (2017-07-07 10:55:49)
> On Mon, Jun 19, 2017 at 11:06:47AM +0100, Chris Wilson wrote:
> > Borrow a trick from anv, and use the last known index for the bo to skip
> > a search of the batch->exec_bo when adding a new relocation. In defence
> > against the bo being used in multiple batches simultaneously, we check
> > that this slot exists and points back to us.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Matt Turner <mattst88 at gmail.com>
> > Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_bufmgr.h        |  5 +++++
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 14 ++++++++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > index 48488bc33b..dd3a37040a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > @@ -76,6 +76,11 @@ struct brw_bo {
> >     uint64_t offset64;
> >  
> >     /**
> > +    * Index of this buffer inside the batch, -1 when not in a batch.
> > +    */
> > +   unsigned int index;
> > +
> > +   /**
> >      * Boolean of whether the GPU is definitely not accessing the buffer.
> >      *
> >      * This is only valid when reusable, since non-reusable
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 62d2fe8ef3..ca7d6b81b1 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -515,12 +515,20 @@ throttle(struct brw_context *brw)
> >     }
> >  }
> >  
> > +#define READ_ONCE(x) (*(volatile __typeof__(x) *)&(x))
> > +
> >  static void
> >  add_exec_bo(struct intel_batchbuffer *batch, struct brw_bo *bo)
> >  {
> >     if (bo != batch->bo) {
> > -      for (int i = 0; i < batch->exec_count; i++) {
> > -         if (batch->exec_bos[i] == bo)
> > +      unsigned int index = READ_ONCE(bo->index);
> > +
> > +      if (index < batch->exec_count && batch->exec_bos[index] == bo)
> > +         return;
> > +
> > +      /* May have been shared between multiple active batches */
> > +      for (index = 0; index < batch->exec_count; index++) {
> > +         if (batch->exec_bos[index] == bo)
> >              return;
> >        }
> >  
> > @@ -553,6 +561,7 @@ add_exec_bo(struct intel_batchbuffer *batch, struct brw_bo *bo)
> >     validation_entry->rsvd1 = 0;
> >     validation_entry->rsvd2 = 0;
> >  
> > +   bo->index = batch->exec_count;
> >     batch->exec_bos[batch->exec_count] = bo;
> >     batch->exec_count++;
> >     batch->aperture_space += bo->size;
> > @@ -597,6 +606,7 @@ execbuffer(int fd,
> >        struct brw_bo *bo = batch->exec_bos[i];
> >  
> >        bo->idle = false;
> > +      bo->index = -1;
> 
> Since we check for matching backpointer I don't think we even need to
> clear this here, but it's cheap. For consistency I think we should also
> clear when allocating a bo:

At the time I was thinking about trying to make -1 mean something
useful, but was kept being stymied by the lack of per-context tracking.

The advantage is that for a single user, it speeds up the not in this
batch check. Multi-user batches has the disadvantage that the bo->index
will ping-pong and they have to keep rechecking (same as before). So I
think it does come out on top, and adding -1 to init should help even
more.

There was also another spot I noticed that used the same hunt for an
exec_bo, brw_batch_references(), that can use the same quick check.
-Chris