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

[Kenneth Graunke]

On Wednesday, July 19, 2017 3:09:13 AM PDT 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.
> 
> v2: Also update brw_batch_references()
> v3: Reset bo->index on creation (Daniel)
> 
> 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>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  src/mesa/drivers/dri/i965/brw_bufmgr.c        |  1 +
>  src/mesa/drivers/dri/i965/brw_bufmgr.h        |  7 +++++++
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 18 ++++++++++++++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 46da53d353..9723124e8d 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -388,6 +388,7 @@ retry:
>     p_atomic_set(&bo->refcount, 1);
>     bo->reusable = true;
>     bo->cache_coherent = bufmgr->has_llc;
> +   bo->index = -1;
>  
>     pthread_mutex_unlock(&bufmgr->lock);
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> index 6a6051bb71..5310e32ac3 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> @@ -76,6 +76,13 @@ struct brw_bo {
>     uint64_t offset64;
>  
>     /**
> +    * Index of this buffer inside the batch, -1 when not in a batch. Note
> +    * that a single buffer may be in multiple batches (contexts), the index
> +    * only refers to its last use and should not be trusted!
> +    */

How about:

   /**
    * The validation list index for this buffer, or -1 when not in a batch.
    * Note that a single buffer may be in multiple batches (contexts), and
    * this is a global field, which refers to the last batch using the BO.
    * It should not be considered authoritative, but can be used to avoid a
    * linear walk of the validation list in the common case by guessing that
    * exec_bos[bo->index] == bo and confirming whether that's the case.
    */

With that,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   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 81cd578b28..a358269d9b 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;
>  
>        /* Update brw_bo::offset64 */
>        if (batch->exec_objects[i].offset != bo->offset64) {
> @@ -742,6 +752,10 @@ brw_batch_has_aperture_space(struct brw_context *brw, unsigned extra_space)
>  bool
>  brw_batch_references(struct intel_batchbuffer *batch, struct brw_bo *bo)
>  {
> +   unsigned int index = READ_ONCE(bo->index);
> +   if (index < batch->exec_count && batch->exec_bos[index] == bo)
> +      return true;
> +
>     for (int i = 0; i < batch->exec_count; i++) {
>        if (batch->exec_bos[i] == bo)
>           return true;
> 

-------------- 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/20170719/d886e231/attachment.sig>