Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Dave Airlie <airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 13 April 2017 at 08:34, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Apr 13, 2017 at 07:41:28AM +1000, Dave Airlie wrote:
>> >
>> > The problem, as I see it, is that you are taking functionality away from
>> > sync_file. If you are wrapping them up inside a sync_file, we have a
>> > fair expectation that our code to handle sync_files will continue to
>> > work.
>>
>> What code? there is no code existing that should be sharing sync objects
>> with semaphores backing them, and trying to use them as sync_files.
>
> By masquerading semaphores as sync_files, you are inviting them to be
> used with the existing code to handle sync_file.

But what existing code is going to get a sync object from another process
and blindly use it without knowing where it got it.

I'm not buying the argument that just because something is possible,
we have to support it.

>
> And quite different from the existing CPU visible sync_files. Why try
> stuffing them through the same interface? At the moment the only thing
> they have in common is the single fence pointer, and really you want
> them just for their handle/fd to a GPU channel.
>
> After you strip out the fops from the semaphore code, doesn't it boil
> down to:
>
> static void semaphore_file_release(struct inode *inode, struct file *file)
> {
>         struct semaphore_file *s = file->private_data;
>
>         dma_fence_put(s->fence);
>         kfree(s);
> }
>
> static const struct file_operations semaphore_file_fops = {
>         .release = semaphore_file_release,
> };
>
> struct semaphore_file *semaphore_file_create(void)
> {
>         struct semaphore_file *s;
>
>         s = kzalloc(sizeof(*s), GFP_KERNEL);
>         if (!s)
>                 return NULL;
>
>         s->file = anon_inode_getfile("semaphore_"file",
>                                      &semaphore_file_fops, s, 0);
>
>         return s;
> }
>
> Plus your new routines to manipulate the semaphore.
>
> The commonality with the current sync_file {
>                 struct file *file;
>                 struct dma_fence *fence;
>         };
> could be extracted and sync_file become fence_file. Would it not help to
> avoid any further confusion by treating them as two very distinct classes
> of fd?
>
> And for me to stop calling the user interface sync_file.

That's a better argument, but the consideration is that there are probably
use-cases for doing something with these in the future, and we'd be limiting
those by doing this. The current code adds the API we need without constraining
future use cases overly much, and avoids any pitfalls.

I'll consider whether we should just split sync_file, but you still have the
same problems you mention, you get an fd you do something you shouldn't with
it, it doesn't fix any of the problems you are raising.

If you have an opaque fd you don't know the providence of, and except some
operations to just work o nit, you are going to be disappointed.

Dave.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx