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

John Harrison
2016-01-14T11:47:19Z

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.