On 11/12/2015 15:29, Tvrtko Ursulin wrote:
Oops. That's from a patch not being upstreamed. It is required for OCL2.0 which is something we are using to test the scheduler.On 11/12/15 13:12, John.C.Harrison@xxxxxxxxx wrote:From: John Harrison <John.C.Harrison@xxxxxxxxx> Various projects desire a mechanism for managing dependencies between work items asynchronously. This can also include work items across complete different and independent systems. For example, an application wants to retreive a frame from a video in device, using it for rendering on a GPU then send it to the video out device for display all without having to stall waiting for completion along the way. The sync framework allows this. It encapsulates synchronisation events in file descriptors. The application can request a sync point for the completion of each piece of work. Drivers should also take sync points in with each new work request and not schedule the work to start until the sync has been signalled. This patch adds sync framework support to the exec buffer IOCTL. A sync point can be passed in to stall execution of the batch buffer until signalled. And a sync point can be returned after each batch buffer submission which will be signalled upon that batch buffer's completion. At present, the input sync point is simply waited on synchronously inside the exec buffer IOCTL call. Once the GPU scheduler arrives, this will be handled asynchronously inside the scheduler and the IOCTL can return without having to wait. Note also that the scheduler will re-order the execution of batch buffers, e.g. because a batch buffer is stalled on a sync point and cannot be submitted yet but other, independent, batch buffers are being presented to the driver. This means that the timeline within the sync points returned cannot be global to the engine. Instead they must be kept per context per engine (the scheduler may not re-order batches within a context). Hence the timeline cannot be based on the existing seqno values but must be a new implementation. This patch is a port of work by several people that has been pulled across from Android. It has been updated several times across several patches. Rather than attempt to port each individual patch, this version is the finished product as a single patch. The various contributors/authors along the way (in addition to myself) were: Satyanantha RamaGopal M <rama.gopal.m.satyanantha@xxxxxxxxx> Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Michel Thierry <michel.thierry@xxxxxxxxx> Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> v2: New patch in series. v3: Updated to use the new 'sync_fence_is_signaled' API rather than having to know about the internal meaning of the 'fence::status' field (which recently got inverted!) [work by Peter Lawthers]. Updated after review comments by Daniel Vetter. Removed '#ifdef CONFIG_SYNC' and add 'select SYNC' to the Kconfig instead. Moved the fd installation of fences to the end of the execbuff call to in order to remove the need to use 'sys_close' to clean up on failure. Updated after review comments by Tvrtko Ursulin. Remvoed the 'fence_external' flag as redundant. Covnerted DRM_ERRORs to DRM_DEBUGs. Changed one second wait to a wait forever when waiting on incoming fences. v4: Re-instated missing return of fd to user land that somehow got lost in the anti-sys_close() re-factor. For: VIZ-5190 Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx> Signed-off-by: Peter Lawthers <peter.lawthers@xxxxxxxxx> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Cc: Daniel Vetter <daniel@xxxxxxxx> --- drivers/gpu/drm/i915/Kconfig | 3 + drivers/gpu/drm/i915/i915_drv.h | 6 ++drivers/gpu/drm/i915/i915_gem.c | 89 +++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 95 ++++++++++++++++++++++++++++--include/uapi/drm/i915_drm.h | 16 ++++- 5 files changed, 200 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 1d96fe1..cb5d5b2 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -22,6 +22,9 @@ config DRM_I915 select ACPI_VIDEO if ACPI select ACPI_BUTTON if ACPI select MMU_NOTIFIERselect MMU_NOTIFIER is not upstream! :)
What will blow up? The ring_name local is a stack array not a pointer to the data segment. Did you miss the '[]'?+ # ANDROID is required for SYNC + select ANDROID + select SYNC help Choose this option if you have a system that has "Intel Graphics Media Accelerator" or "HD Graphics" integrated graphics,diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.hindex d013c6d..194bca0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h@@ -2278,6 +2278,12 @@ void i915_gem_request_notify(struct intel_engine_cs *ring, bool fence_locked);int i915_create_fence_timeline(struct drm_device *dev, struct intel_context *ctx, struct intel_engine_cs *ring); +struct sync_fence; +int i915_create_sync_fence(struct drm_i915_gem_request *req, + struct sync_fence **sync_fence, int *fence_fd); +void i915_install_sync_fence_fd(struct drm_i915_gem_request *req, + struct sync_fence *sync_fence, int fence_fd);+bool i915_safe_to_ignore_fence(struct intel_engine_cs *ring, struct sync_fence *fence);static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req){diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.cindex 4817015..279d79f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -37,6 +37,7 @@ #include <linux/swap.h> #include <linux/pci.h> #include <linux/dma-buf.h> +#include <../drivers/android/sync.h> #define RQ_BUG_ON(expr)@@ -2560,7 +2561,13 @@ void __i915_add_request(struct drm_i915_gem_request *request,/** Add the fence to the pending list before emitting the commands to- * generate a seqno notification interrupt. + * generate a seqno notification interrupt. This will also enable + * interrupts if 'signal_requested' has been set. + *+ * For example, if an exported sync point has been requested for this+ * request then it can be waited on without the driver's knowledge, + * i.e. without calling __i915_wait_request(). Thus interrupts must + * be enabled from the start rather than only on demand. */ i915_gem_request_submit(request);@@ -2901,6 +2908,86 @@ static unsigned i915_fence_timeline_get_next_seqno(struct i915_fence_timeline *treturn seqno; } +int i915_create_sync_fence(struct drm_i915_gem_request *req, + struct sync_fence **sync_fence, int *fence_fd) +{ + char ring_name[] = "i915_ring0"; + int fd; + + fd = get_unused_fd_flags(O_CLOEXEC); + if (fd < 0) { + DRM_DEBUG("No available file descriptors!\n"); + *fence_fd = -1; + return fd; + } + + ring_name[9] += req->ring->id;I think this will possibly blow up if CONFIG_DEBUG_RODATA is set, which is the case on most kernels.So I think you need to make a local copy with kstrdup and free it after calling sync_fence_create_dma.
The test above filters out anything that is signalled (or errored). Stating that again on each subsequent test seems unnecessarily verbose.+ *sync_fence = sync_fence_create_dma(ring_name, &req->fence); + if (!*sync_fence) { + put_unused_fd(fd); + *fence_fd = -1; + return -ENOMEM; + } + + *fence_fd = fd; + + return 0; +} + +void i915_install_sync_fence_fd(struct drm_i915_gem_request *req, + struct sync_fence *sync_fence, int fence_fd) +{ + sync_fence_install(sync_fence, fence_fd); + + /* + * NB: The corresponding put happens automatically on file close + * from sync_fence_release() via the fops callback. + */ + fence_get(&req->fence); + + /* + * The sync framework adds a callback to the fence. The fence + * framework calls 'enable_signalling' when a callback is added. + * Thus this flag should have been set by now. If not then + * 'enable_signalling' must be called explicitly because exporting+ * a fence to user land means it can be waited on asynchronously and+ * thus must be signalled asynchronously. + */ + WARN_ON(!req->signal_requested); +} ++bool i915_safe_to_ignore_fence(struct intel_engine_cs *ring, struct sync_fence *sync_fence)+{ + struct fence *dma_fence; + struct drm_i915_gem_request *req; + int i; + + if (sync_fence_is_signaled(sync_fence)) + return true; + + for(i = 0; i < sync_fence->num_fences; i++) { + dma_fence = sync_fence->cbs[i].sync_pt; + + /* No need to worry about dead points: */ + if (fence_is_signaled(dma_fence)) + continue; + + /* Can't ignore other people's points: */Maybe add "unsignaled" to qualify.
To be honest, I think the one is left over from an earlier iteration of the code which did things slightly differently. It probably could just return zero or -error.+ if(dma_fence->ops != &i915_gem_request_fops) + return false; + + req = container_of(dma_fence, typeof(*req), fence); + + /* Can't ignore points on other rings: */ + if (req->ring != ring) + return false; + + /* Same ring means guaranteed to be in order so ignore it. */ + } + + return true; +} + int i915_gem_request_alloc(struct intel_engine_cs *ring, struct intel_context *ctx, struct drm_i915_gem_request **req_out)diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.cindex bfc4c17..5f629f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -26,6 +26,7 @@ * */ +#include <linux/syscalls.h> #include <drm/drmP.h> #include <drm/i915_drm.h> #include "i915_drv.h" @@ -33,6 +34,7 @@ #include "intel_drv.h" #include <linux/dma_remapping.h> #include <linux/uaccess.h> +#include <../drivers/android/sync.h> #define __EXEC_OBJECT_HAS_PIN (1<<31) #define __EXEC_OBJECT_HAS_FENCE (1<<30) @@ -1322,6 +1324,38 @@ eb_get_batch(struct eb_vmas *eb) return vma->obj; }+static int i915_early_fence_wait(struct intel_engine_cs *ring, int fence_fd)+{ + struct sync_fence *fence; + int ret = 0; + + if (fence_fd < 0) { + DRM_DEBUG("Invalid wait fence fd %d on ring %d\n", fence_fd, + (int) ring->id); + return 1;Suggest adding kerneldoc describing return values from this function. It wasn't immediately clear to me what one means.
But I am also not sure that invalid fd shouldn't be an outright error instead of allowing execbuf to contiue.Wasn't sure if it was possible for a fence to be invalidated behind the back of an application. And you don't want to reject rendering from one app just because another app went splat. I guess even if the underlying fence has been destroyed, the fd will still be private to the current app and hence valid. So maybe it should just return -EINVAL or some such of the fd itself is toast.
Will update the comment for those who can't remember what they read two lines earlier...+ } + + fence = sync_fence_fdget(fence_fd); + if (fence == NULL) { + DRM_DEBUG("Invalid wait fence %d on ring %d\n", fence_fd, + (int) ring->id); + return 1; + } + + if (!sync_fence_is_signaled(fence)) {Minor comment, but i915_safe_to_ignore_fence checks this as well so you could remove it here.+ /* + * Wait forever for the fence to be signalled. This is safe + * because the the mutex lock has not yet been acquired and + * the wait is interruptible. + */ + if (!i915_safe_to_ignore_fence(ring, fence)) + ret = sync_fence_wait(fence, -1); + } + + sync_fence_put(fence); + return ret; +} + static int i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_file *file,@@ -1341,6 +1375,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,u32 dispatch_flags; int ret; bool need_relocs; + int fd_fence_complete = -1; + int fd_fence_wait = lower_32_bits(args->rsvd2); + struct sync_fence *sync_fence; + + /* + * Make sure an broken fence handle is not returned no matter + * how early an error might be hit. Note that rsvd2 has to be + * saved away first because it is also an input parameter! + */Instead of the 2nd sentence maybe say something like "Note that we have saved rsvd2 already for later use since it is also in input parameter!". Like written I was expecting the code following the comment to do that, and then was confused when it didn't. Or maybe my attention span is too short.
It depends which side of the Atlantic you are on. English has a double l, American just a single. So largely it depends who wrote the code/comment and who (if anyone!) has reviewed it.+ if (args->flags & I915_EXEC_CREATE_FENCE) + args->rsvd2 = (__u64) -1; if (!i915_gem_check_execbuffer(args)) return -EINVAL;@@ -1424,6 +1469,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,dispatch_flags |= I915_DISPATCH_RS; } + /* + * Without a GPU scheduler, any fence waits must be done up front. + */ + if (args->flags & I915_EXEC_WAIT_FENCE) { + ret = i915_early_fence_wait(ring, fd_fence_wait); + if (ret < 0) + return ret; + + args->flags &= ~I915_EXEC_WAIT_FENCE; + } + intel_runtime_pm_get(dev_priv); ret = i915_mutex_lock_interruptible(dev);@@ -1571,8 +1627,41 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,params->batch_obj = batch_obj; params->ctx = ctx; + if (args->flags & I915_EXEC_CREATE_FENCE) { + /* + * Caller has requested a sync fence. + * User interrupts will be enabled to make sure that + * the timeline is signalled on completion. + */Is it signaled or signalled? There is a lot of usage of both throughout the patches and I as a non-native speaker am amu^H^H^Hconfused. ;)
+ ret = i915_create_sync_fence(params->request, &sync_fence, + &fd_fence_complete); + if (ret) { + DRM_ERROR("Fence creation failed for ring %d, ctx %p\n", + ring->id, ctx); + goto err_batch_unpin; + } + } + ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas); + if (fd_fence_complete != -1) { + if (ret) { + sync_fence_put(sync_fence); + put_unused_fd(fd_fence_complete); + } else { + /* + * Install the fence into the pre-allocated file + * descriptor to the fence object so that user land + * can wait on it... + */ + i915_install_sync_fence_fd(params->request, + sync_fence, fd_fence_complete); + + /* Return the fence through the rsvd2 field */ + args->rsvd2 = (__u64) fd_fence_complete; + } + } + err_batch_unpin: /** FIXME: We crucially rely upon the active tracking for the (ppgtt)@@ -1602,6 +1691,7 @@ pre_mutex_err:/* intel_gpu_busy should also get a ref, so it will free when the device* is really idle. */ intel_runtime_pm_put(dev_priv); + return ret; }@@ -1707,11 +1797,6 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,return -EINVAL; } - if (args->rsvd2 != 0) { - DRM_DEBUG("dirty rvsd2 field\n"); - return -EINVAL; - } - exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); if (exec2_list == NULL) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 67cebe6..86f7921 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -250,7 +250,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)@@ -695,7 +695,7 @@ struct drm_i915_gem_exec_object2 { __u64 flags; __u64 rsvd1; - __u64 rsvd2; + __u64 rsvd2; /* Used for fence fd */ }; struct drm_i915_gem_execbuffer2 { @@ -776,7 +776,17 @@ struct drm_i915_gem_execbuffer2 { */ #define I915_EXEC_RESOURCE_STREAMER (1<<15) -#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER<<1) +/** Caller supplies a sync fence fd in the rsvd2 field. + * Wait for it to be signalled before starting the work + */ +#define I915_EXEC_WAIT_FENCE (1<<16) + +/** Caller wants a sync fence fd for this execbuffer. + * It will be returned in rsvd2 + */ +#define I915_EXEC_CREATE_FENCE (1<<17) + +#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_CREATE_FENCE<<1) #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff) #define i915_execbuffer2_set_context_id(eb2, context) \Regards, Tvrtko
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
