On 01/22/2015 11:42 AM, Chris Wilson wrote:
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"
Did not pay attention to this part, will change.
+ 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.
What do yo consider emitting? To me that is fd_install and that happens
after request was successfully submitted. I thought it is tidier to set
up required objects before and then install the fence, or discard it,
depending on the outcome. You think differently?
+ 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);
}
Yeah makes sense, will change.
+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?
I can't see that it was intended so I'll simplify it unless Jesse know
better.
+
+ 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.
Not sure what you mean by before/after?
In the future it will take in the input fence fd and return the output
fence if that's what you mean.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
