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

[Tvrtko Ursulin]

On 28/10/15 13:01, John Harrison wrote:
> On 27/07/2015 14:00, Tvrtko Ursulin wrote:

[snip]

>>> +    if (!sync_fence) {
>>> +        put_unused_fd(fd);
>>> +        *fence_fd = -1;
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    sync_fence_install(sync_fence, fd);
>>> +    *fence_fd = fd;
>>> +
>>> +    // Necessary??? Who does the put???
>>> +    fence_get(&req->fence);
>>
>> sync_fence_release?
> Yes but where? Does the driver need to call this? Is it userland's
> responsibility? Does it happen automatically when the fd is closed? Do
> we even need to do the _get() in the first place? It seems to be working
> in that I don't get any unexpected free of the fence and I don't get
> huge numbers of leaked fences. However, it would be nice to know how it
> is working!

When the fd is closed, implicitly or explicitly (close(2)/exit(2)/any 
process termination), kernel will automatically call sync_fence_release 
via the file_operations set in the inode->f_ops at sync_fence creation time

(Actually not when the fd is closed but when the last instance of struct 
file * in question goes away.)

>>> +        return 1;
>>> +    }
>>> +
>>> +    if (atomic_read(&fence->status) == 0) {
>>> +        if (!i915_safe_to_ignore_fence(ring, fence))
>>> +            ret = sync_fence_wait(fence, 1000);
>>
>> I expect you have to wait indefinitely here, not just for one second.
> Causing the driver to wait indefinitely under userland control is surely
> a Bad Thing(tm)? Okay, this is done before acquiring the mutex lock and
> presumably the wait can be interrupted, e.g. if the user land process
> gets a KILL signal. It still seems a bad idea to wait forever. Various
> bits of Android generally use a timeout of either 1s or 3s.
>
> Daniel or anyone else, any views of driver time outs?

It is slightly irrelevant since this is a temporary code path before the 
scheduler lands.

But I don't see it makes sense to have made up timeouts. If userspace 
gave us something to wait on, then I think we should wait until it is 
done or it fails. It is not blocking the driver but is running on the 
behalf of the same process which passed in the fd to wait on. So the 
process can only block itself.

Regards,

Tvrtko