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

[Kenneth Graunke]

On Friday, June 9, 2017 6:01:33 AM PDT 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.

I'm not crazy about exposing __brw_bo_busy and brw_bo_busy, with slightly
different semantics.  Why not just make brw_bo_busy do:

   if (bo->idle && bo->reusable)
      return false;

   /* otherwise query the kernel */

These days, it appears that bo->reusable is false for any buffers that
have been imported/exported via dmabuf or flink, and true otherwise.
(We might want to rename it to bo->foreign or such.)

With that change, brw_bo_busy should bypass the ioctl for most BOs, but
would still work for foreign BOs, without the caller having to worry
about it.

> 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 | 17 +++++------------
>  src/mesa/drivers/dri/i965/brw_bufmgr.h | 33 ++++++++++++++++++++++++++++++---
>  2 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 67c15878d0..01590a0b0a 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -194,21 +194,14 @@ brw_bo_reference(struct brw_bo *bo)
>  }
>  
>  int
> -brw_bo_busy(struct brw_bo *bo)
> +__brw_bo_busy(struct brw_bo *bo)
>  {
> -   struct brw_bufmgr *bufmgr = bo->bufmgr;
> -   struct drm_i915_gem_busy busy;
> -   int ret;
> +   struct drm_i915_gem_busy busy = { bo->gem_handle };
>  
> -   memclear(busy);
> -   busy.handle = bo->gem_handle;
> +   drmIoctl(bo->bufmgr->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
>  
> -   ret = drmIoctl(bufmgr->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> -   if (ret == 0) {
> -      bo->idle = !busy.busy;
> -      return busy.busy;
> -   }
> -   return false;
> +   bo->idle = !busy.busy;
> +   return busy.busy;
>  }
>  
>  int
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> index 70cc2bbc6c..3a397be695 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> @@ -240,11 +240,38 @@ int brw_bo_get_tiling(struct brw_bo *bo, uint32_t *tiling_mode,
>   */
>  int brw_bo_flink(struct brw_bo *bo, uint32_t *name);
>  
> +int __brw_bo_busy(struct brw_bo *bo);
> +
>  /**
> - * Returns 1 if mapping the buffer for write could cause the process
> - * to block, due to the object being active in the GPU.
> + * Returns 0 if mapping the buffer is not in active use by the gpu.
> + * If non-zero, any mapping for for write could cause the process
> + * to block, due to the object being active in the GPU. If the lower
> + * 16 bits are zero, then we can map for read without stalling.
> + *
> + * The last-known busy status of the brw_bo is checked first. This may be
> + * stale if the brw_bo has been exported to a foriegn process. If used on an
> + * exported bo, call __brw_bo_busy() directly to bypass the local check.
>   */
> -int brw_bo_busy(struct brw_bo *bo);
> +static inline int brw_bo_busy(struct brw_bo *bo)
> +{
> +   if (bo->idle) /* Note this may be stale if the bo is exported */
> +      return 0;
> +
> +   return __brw_bo_busy(bo);
> +}

I'd rather keep this as a boolean result, rather than an integer with
certain bits having particular meanings.  Bonus points for changing the
return type to "bool".

> +
> +/**
> + * Returns true if mapping the buffer for read will cause the process to
> + * block (i.e. the buffer is still being writen). Note that when it
> + * returns false, the buffer may still be concurrently read by the GPU.
> + */
> +static inline int brw_bo_write_busy(struct brw_bo *bo)
> +{
> +   if (bo->idle) /* Note this may be stale if the bo is exported */
> +      return 0;
> +
> +   return __brw_bo_busy(bo) & 0xffff;
> +}
>  
>  /**
>   * Specify the volatility of the buffer.

This seems like a nice helper.

--Ken
-------------- 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/20170614/b6c4324c/attachment.sig>