On Thu, Jan 22, 2015 at 11:15:40AM +0000, Tvrtko Ursulin wrote:
> From: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
