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

Tvrtko Ursulin
2015-10-28T14:32:00Z

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