Re: [RFC 6/9] drm/i915: Add sync framework support to execbuff IOCTL

[John Harrison <John.C.Harrison@Intel.com>]

On 13/01/2016 18:43, Chris Wilson wrote:
> On Wed, Jan 13, 2016 at 05:57:32PM +0000, John.C.Harrison@Intel.com wrote:
>>   static int
>>   i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   		       struct drm_file *file,
>> @@ -1428,6 +1465,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   	u32 dispatch_flags;
>>   	int ret, i;
>>   	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 been
>> +	 * saved away above because it is also an input parameter!
>> +	 */
>> +	if (args->flags & I915_EXEC_CREATE_FENCE)
>> +		args->rsvd2 = (__u64) -1;
> But you are not restoring the user input parameter upon an error path.
>
> Very simple example is the user trying to do a wait on a fence but is
> woken up by a signal and then tries to restart the syscall, the standard
> 	do {
> 		ret = ioctl(fd, request, arg);
> 	} while (ret == -1 && (errno == EINTR || errno == EAGAIN));
> loop errors out with EINVAL on the second pass.
>
>>   	if (!i915_gem_check_execbuffer(args))
>>   		return -EINVAL;
>> @@ -1511,6 +1559,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;
>> +	}
>> +
>>   	ret = i915_mutex_lock_interruptible(dev);
>>   	if (ret)
>>   		goto pre_mutex_err;
>> @@ -1695,13 +1754,41 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   	i915_gem_context_reference(ctx);
>>   	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.
>> +		 */
>> +		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 (ret)
>> -		goto err_batch_unpin;
>> +		goto err_fence;
>>   
>>   	/* the request owns the ref now */
>>   	i915_gem_context_unreference(ctx);
>>   
>> +	if (fd_fence_complete != -1) {
>> +		/*
>> +		 * 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;
> Use the upper s32 for the output, so again you are not overwriting user
> state without good reason.
> -Chris
>
Makes sense. Will do.

Thanks,
John.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx