[Intel-gfx] [RFC] drm/i915: Android native sync support
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jan 22 05:41:48 PST 2015
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 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"
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
More information about the Intel-gfx
mailing list