[Intel-gfx] [RFC] drm/i915: Android native sync support

[Chris Wilson]

On Thu, Jan 22, 2015 at 11:15:40AM +0000, Tvrtko Ursulin wrote:
> From: Jesse Barnes <jbarnes at virtuousgeek.org>
> 
> Add Android native sync support with fences exported as file descriptors via
> the execbuf ioctl (rsvd2 field is used).
> 
> This is a continuation of Jesse Barnes's previous work, squashed to arrive at
> the final destination, cleaned up, with some fixes and preliminary light
> testing.
> 
> GEM requests are extended with fence structures which are associated with
> Android sync fences exported to user space via file descriptors. Fences which
> are waited upon, and while exported to userspace, are referenced and added to
> the irq_queue so they are signalled when requests are completed. There is no
> overhead apart from the where fences are not requested.
> 
> Based on patches by Jesse Barnes:
>    drm/i915: Android sync points for i915 v3
>    drm/i915: add fences to the request struct
>    drm/i915: sync fence fixes/updates
> 
> To do:
>    * Extend driver data with context id / ring id.
>    * More testing.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig               |  14 ++
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_drv.h            |  25 +++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  19 +++
>  drivers/gpu/drm/i915/i915_sync.c           | 248 +++++++++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h                |   8 +-
>  6 files changed, 312 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_sync.c
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 4e39ab3..6b23d17 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -43,6 +43,20 @@ config DRM_I915_KMS
>  
>  	  If in doubt, say "Y".
>  
> +config DRM_I915_SYNC
> +	bool "Enable explicit sync support"
> +	depends on DRM_I915
> +	default y
> +	select STAGING
> +	select ANDROID
> +	select SYNC
> +	help
> +	  Choose this option to enable Android native sync support and the
> +	  corresponding i915 driver code to expose it.  Slightly increases
> +	  driver size and pulls in sync support from staging.
> +
> +	  If in doubt, say "Y".

So we should move i915 into staging? I think you mean
"default y if STAGING"

> +	if (args->flags & I915_EXEC_FENCE_OUT) {
> +		ret = i915_create_sync_fence_ring(ring, ctx,
> +						  &sync_fence, &fence_fd);
> +		if (ret)
> +			goto sync_err;
> +	}
> +
>  	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
>  				      &eb->vmas, batch_obj, exec_start, flags);

You emit the fence prior to the execution of the batch? Interesting. Not
exactly where I would expect the fence. Both before/after are
justifiable.
 
> +	if (!ret && sync_fence) {
> +		sync_fence_install(sync_fence, fence_fd);
> +		args->rsvd2 = fence_fd;
> +	} else if (sync_fence) {
> +		fput(sync_fence->file);
> +		put_unused_fd(fence_fd);
> +	}

I think this can be tidied up and made consistent with the existing
style of error handling thusly:

if (ret)
	goto fence_err;

if (sync_fence) {
	/* transferr ownershup to userspace */
	sync_fence_install(sync_fence, fence_fd);
	args->rsvd2 = fence_fd; 
	sync_fence = NULL; 
}

fence_err:
if (sync_fence) {
	fput(sync_fence->file);
	put_unused_fd(fence_fd);
}

> +sync_err:


> +static signed long
> +i915_fence_ring_wait(struct fence *fence, bool intr, signed long timeout_js)
> +{
> +	struct drm_i915_gem_request *req = to_i915_request(fence);
> +	struct drm_i915_private *dev_priv = req->ring->dev->dev_private;
> +	int ret;
> +	s64 timeout;
> +
> +	timeout = jiffies_to_nsecs(timeout_js);
> +
> +	ret = __i915_wait_request(req,
> +				  atomic_read(&dev_priv->gpu_error.reset_counter),
> +				  intr, &timeout, NULL);
> +	if (ret == -ETIME)
> +		return nsecs_to_jiffies(timeout);

This should be equivalent to return 0; intended?
> +
> +	return ret;
> +}

> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2e559f6e..c197770 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -246,7 +246,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_HWS_ADDR		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_HWS_ADDR, struct drm_i915_gem_init)
>  #define DRM_IOCTL_I915_GEM_INIT		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_INIT, struct drm_i915_gem_init)
>  #define DRM_IOCTL_I915_GEM_EXECBUFFER	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_EXECBUFFER, struct drm_i915_gem_execbuffer)
> -#define DRM_IOCTL_I915_GEM_EXECBUFFER2	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_EXECBUFFER2, struct drm_i915_gem_execbuffer2)
> +#define DRM_IOCTL_I915_GEM_EXECBUFFER2	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_EXECBUFFER2, struct drm_i915_gem_execbuffer2)
>  #define DRM_IOCTL_I915_GEM_PIN		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_PIN, struct drm_i915_gem_pin)
>  #define DRM_IOCTL_I915_GEM_UNPIN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_UNPIN, struct drm_i915_gem_unpin)
>  #define DRM_IOCTL_I915_GEM_BUSY		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_BUSY, struct drm_i915_gem_busy)
> @@ -718,7 +718,7 @@ struct drm_i915_gem_execbuffer2 {
>  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
>  	__u64 flags;
>  	__u64 rsvd1; /* now used for context info */
> -	__u64 rsvd2;
> +	__u64 rsvd2; /* now used for fence fd */
If we are going to use this slot for fence fd, may as well make it
supply both before/after.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre