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

[Chris Wilson]

On Wed, Jan 13, 2016 at 05:57:32PM +0000, John.C.Harrison at 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

-- 
Chris Wilson, Intel Open Source Technology Centre