[PATCH 3/8] drm: introduce sync objects as sync file objects with no fd

[Daniel Vetter]

re-adding dri-devel, somehow got lost.

On Tue, Apr 11, 2017 at 8:00 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Apr 11, 2017 at 5:06 AM, Dave Airlie <airlied at gmail.com> wrote:
>>> You can drop the DRM_UNLOCKED, it's the enforced standard for non-legacy
>>> drivers since:
>>>
>>> commit ea487835e8876abf7ad909636e308c801a2bcda6
>>> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Date:   Mon Sep 28 21:42:40 2015 +0200
>>>
>>>     drm: Enforce unlocked ioctl operation for kms driver ioctls
>>
>> This is a core ioctl. not a driver one, so I think I still need UNLOCKED.
>
> Oh right, we need that still for nouveau's legacy ctx usage.
>
>>>> +     /* Check if we currently have a reference on the object */
>>>> +     sync_file = idr_find(&file_private->syncobj_idr, handle);
>>>
>>> You need to get a reference for the sync_file (hm, should we have
>>> sync_file_get/put helpers?), since otherwise someone might sneak in a
>>> destroy ioctl call right after you drop the lock here and boom.
>>
>> Indeed, fixed locally now.
>>
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     sync_file = sync_file_alloc(type, flags);
>>>> +     if (!sync_file)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     idr_preload(GFP_KERNEL);
>>>> +     spin_lock(&file_private->syncobj_table_lock);
>>>> +     ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
>>>> +     spin_unlock(&file_private->syncobj_table_lock);
>>>> +
>>>> +     idr_preload_end();
>>>> +
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     *handle = ret;
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static void drm_syncobj_destroy(struct drm_file *file_private,
>>>> +                             u32 handle)
>>>> +{
>>>> +     struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> +     if (!sync_file)
>>>> +             return;
>>>> +
>>>> +     spin_lock(&file_private->syncobj_table_lock);
>>>> +     idr_remove(&file_private->syncobj_idr, handle);
>>>> +     spin_unlock(&file_private->syncobj_table_lock);
>>>> +
>>>> +     fput(sync_file->file);
>>>> +}
>>>> +
>>>> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
>>>> +                                 u32 handle, int *fd)
>>>> +{
>>>> +     struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> +     int ret;
>>>> +     if (!sync_file)
>>>> +             return -EINVAL;
>>>> +
>>>> +     ret = get_unused_fd_flags(O_CLOEXEC);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +     fd_install(ret, sync_file->file);
>>>> +     *fd = ret;
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>>>> +                                 int fd, u32 *handle)
>>>> +{
>>>> +     struct sync_file *sync_file = sync_file_fdget(fd);
>>>> +     int ret;
>>>> +     if (!sync_file)
>>>> +             return -EINVAL;
>>>> +     idr_preload(GFP_KERNEL);
>>>> +     spin_lock(&file_private->syncobj_table_lock);
>>>> +     ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
>>>> +     spin_unlock(&file_private->syncobj_table_lock);
>>>> +     idr_preload_end();
>>>> +
>>>> +     if (ret < 0) {
>>>> +             fput(sync_file->file);
>>>> +             return ret;
>>>> +     }
>>>> +     *handle = ret;
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int drm_syncobj_info(struct drm_file *file_private,
>>>> +                         u32 handle, u32 *type, u32 *flags)
>>>> +{
>>>> +     struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> +
>>>> +     if (!sync_file)
>>>> +             return -EINVAL;
>>>> +     *type = sync_file->type;
>>>> +     *flags = sync_file->flags;
>>>> +     return 0;
>>>> +}
>>>
>>> Do we really need this? I'd have assumed that all the drm_syncobj are
>>> guaranteed to be of type sema. Why else would you want to store them in an
>>> idr for future reuse?
>>
>> I'm allowing for future changes to the API, for now we could probably
>> limit syncobj
>> to SEMA, but there is nothing stopping you from importing a sync_file fence
>> into a syncobj and exporting it again, we should consider if we should restrict
>> the drm sync obj ioctls to just sema types for now. Hopefully someone has some
>> opinions here, I'm leaning towards just limiting the initial API to syncobj.
>
> Yeah, makes sense.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch