Re: [PATCH 2/4] sync_file: add replace and export some functionality

[Dave Airlie <airlied@gmail.com>]

On 15 March 2017 at 18:55, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Mar 15, 2017 at 02:19:16PM +1000, Dave Airlie wrote:
>> >
>> > uabi semantics question: Should we wake up everyone when the fence gets
>> > replaced? What's the khr semaphore expectation here?
>>
>> There are no real semantics for this case, you either wait the semaphore and
>> replace it with NULL, or signal it where you replace the fence with a new fence.
>>
>> Nobody should be using the sync_fence interfaces to poll on these fence fds.
>>
>> So not crashing for non-vulkan behaviour is what we need.
>>
>> The spec doesn't know anything other than this is an opaque fd,
>> it shouldn't be doing operations on it, execpt importing it.
>>
>> >>  static int sync_file_set_fence(struct sync_file *sync_file,
>> >>                              struct dma_fence **fences, int num_fences)
>> >>  {
>> >> @@ -292,8 +328,10 @@ static void sync_file_free(struct kref *kref)
>> >>       struct sync_file *sync_file = container_of(kref, struct sync_file,
>> >>                                                    kref);
>> >>
>> >> -     if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
>> >> -             dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
>> >> +     if (sync_file->fence) {
>> >> +             if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
>> >> +                     dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
>> >> +     }
>> >>       dma_fence_put(sync_file->fence);
>> >>       kfree(sync_file);
>> >>  }
>> >
>> > I think you've missed _poll, won't that oops now?
>>
>> I don't think it will, all the stuff do inside the poll handler is
>> protected by the mutex
>> or do you think we should hook the new fence if the old fence has poll enabled?
>
> Yeah, or at least wake them up somehow and restart it. Or well at least
> think what would happen when that does, and whether we care. If vk says
> you get undefined behaviour when you replace the fence of a semaphore when
> the old one hasn't signalled yet, then we don't need to do anything.

In VK a semaphore is an object, there is no operations on it to touch the fence,
the sync_file implementation is there to support passing the fd
backing the semaphore
between vulkan processes and maybe later GL processes. Nothing else is defined,
and is left as an implementation detail. We just need to protect
against someone doing
something stupid with the sync_file fd, currently however replacing is
a kernel internal
operation only happens when you signal or wait. So I expect yes you could export
 a sem, import it, poll it, when it has never been signaled, which is undefined,
export it, import, signal it, then poll on it, and if someone waits on
it then the poll
is probably going to have issues, this behaviour is totally outside
the vulkan spec.

Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel