On Wed, Jan 13, 2016 at 05:57:32PM +0000, John.C.Harrison@xxxxxxxxx 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
