[Mesa-dev] [PATCH 1/9] i965: Check last known busy status on bo before asking the kernel

[Ian Romanick]

On 10/02/2018 11:06 AM, Chris Wilson wrote:
> If we know the bo is idle (that is we have no submitted a command buffer
> referencing this bo since the last query) we can skip asking the kernel.
> Note this may report a false negative if the target is being shared
> between processes (exported via dmabuf or flink). To allow the caller
> control over using the last known flag, the query is split into two.
> 
> v2: Check against external bo before trusting our own tracking.
> 
> 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>
> ---
>  src/mesa/drivers/dri/i965/brw_bufmgr.c | 40 ++++++++++++++++++++------
>  src/mesa/drivers/dri/i965/brw_bufmgr.h | 11 +++++--
>  2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index f1675b191c1..d9e8453787c 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -444,18 +444,40 @@ vma_free(struct brw_bufmgr *bufmgr,
>     }
>  }
>  
> -int
> +static int
> +__brw_bo_busy(struct brw_bo *bo)
> +{
> +   struct drm_i915_gem_busy busy = { bo->gem_handle };

I think it makes the code more readable to keep ".handle = ..."

> +
> +   if (bo->idle && !bo->external)
> +      return 0;
> +
> +   /* If we hit an error here, it means that bo->gem_handle is invalid.
> +    * Treat it as being idle (busy.busy is left as 0) and move along.
> +    */
> +   drmIoctl(bo->bufmgr->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> +
> +   bo->idle = !busy.busy;
> +   return busy.busy;
> +}
> +
> +bool
>  brw_bo_busy(struct brw_bo *bo)
>  {
> -   struct brw_bufmgr *bufmgr = bo->bufmgr;
> -   struct drm_i915_gem_busy busy = { .handle = bo->gem_handle };
> +   return __brw_bo_busy(bo);
> +}
>  
> -   int ret = drmIoctl(bufmgr->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> -   if (ret == 0) {
> -      bo->idle = !busy.busy;
> -      return busy.busy;
> -   }
> -   return false;
> +bool
> +brw_bo_map_busy(struct brw_bo *bo, unsigned flags)
> +{
> +   unsigned mask;
> +
> +   if (flags & MAP_WRITE)
> +      mask = ~0u;
> +   else
> +      mask = 0xffff;

In other places we would do this as

   const unsigned mask = (flags & MAP_WRITE) ? ~0u : 0xffff;

Unless, of course, a later patch adds more choices.

> +
> +   return __brw_bo_busy(bo) & mask;
>  }
>  
>  int
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> index 32fc7a553c9..e1f46b091ce 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> @@ -323,10 +323,17 @@ int brw_bo_get_tiling(struct brw_bo *bo, uint32_t *tiling_mode,
>  int brw_bo_flink(struct brw_bo *bo, uint32_t *name);
>  
>  /**
> - * Returns 1 if mapping the buffer for write could cause the process
> + * Returns false if mapping the buffer is not in active use by the gpu.
> + * If it returns true, any mapping for for write could cause the process

                                      for for^W

With those nits fixed, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

>   * to block, due to the object being active in the GPU.
>   */
> -int brw_bo_busy(struct brw_bo *bo);
> +bool brw_bo_busy(struct brw_bo *bo);
> +
> +/**
> + * Returns true if mapping the buffer for the set of flags (i.e. MAP_READ or
> + * MAP_WRITE) will cause the process to block.
> + */
> +bool brw_bo_map_busy(struct brw_bo *bo, unsigned flags);
>  
>  /**
>   * Specify the volatility of the buffer.
>