Re: [RFC] drm/i915: Android native sync support

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: John Harrison <John.C.Harrison@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx