[PATCH 10/11] drm/syncobj: add timeline signal ioctl for syncobj

[Koenig, Christian]

Am 07.12.18 um 14:09 schrieb Zhou, David(ChunMing):
> 在 2018/12/7 19:31, Christian König 写道:
>> Am 07.12.18 um 10:56 schrieb Chunming Zhou:
>>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_internal.h |  2 +
>>>    drivers/gpu/drm/drm_ioctl.c    |  2 +
>>>    drivers/gpu/drm/drm_syncobj.c  | 70 ++++++++++++++++++++++++++++++++++
>>>    include/uapi/drm/drm.h         |  1 +
>>>    4 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_internal.h
>>> b/drivers/gpu/drm/drm_internal.h
>>> index ecbe3d51a702..149c2f589ec9 100644
>>> --- a/drivers/gpu/drm/drm_internal.h
>>> +++ b/drivers/gpu/drm/drm_internal.h
>>> @@ -188,6 +188,8 @@ int drm_syncobj_reset_ioctl(struct drm_device
>>> *dev, void *data,
>>>                    struct drm_file *file_private);
>>>    int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
>>>                     struct drm_file *file_private);
>>> +int drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void
>>> *data,
>>> +                      struct drm_file *file_private);
>>>    int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>>>                    struct drm_file *file_private);
>>>    diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>> index 6b417e3c3ea5..d05586601eb5 100644
>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>> @@ -687,6 +687,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL,
>>> drm_syncobj_timeline_signal_ioctl,
>>> +              DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>        DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE,
>>> drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>> b/drivers/gpu/drm/drm_syncobj.c
>>> index cf4daa670252..238ed89593a7 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -1165,6 +1165,76 @@ drm_syncobj_signal_ioctl(struct drm_device
>>> *dev, void *data,
>>>        return ret;
>>>    }
>>>    +int
>>> +drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
>>> +                  struct drm_file *file_private)
>>> +{
>>> +    struct drm_syncobj_timeline_array *args = data;
>>> +    struct drm_syncobj **syncobjs;
>>> +    struct dma_fence_chain *chains;
>>> +    uint64_t *points;
>>> +    uint32_t i, j, timeline_count = 0;
>>> +    int ret;
>>> +
>>> +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (args->pad != 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (args->count_handles == 0)
>>> +        return -EINVAL;
>>> +
>>> +    ret = drm_syncobj_array_find(file_private,
>>> +                     u64_to_user_ptr(args->handles),
>>> +                     args->count_handles,
>>> +                     &syncobjs);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    points = kmalloc_array(args->count_handles, sizeof(*points),
>>> +                   GFP_KERNEL);
>>> +    if (!points) {
>>> +        ret = -ENOMEM;
>>> +        goto out;
>>> +    }
>>> +    if (!u64_to_user_ptr(args->points)) {
>>> +        memset(points, 0, args->count_handles * sizeof(uint64_t));
>>> +    } else if (copy_from_user(points, u64_to_user_ptr(args->points),
>>> +                  sizeof(uint64_t) * args->count_handles)) {
>>> +        ret = -EFAULT;
>>> +        goto err_points;
>>> +    }
>>> +
>>> +
>>> +    for (i = 0; i < args->count_handles; i++) {
>>> +        if (points[i])
>>> +            timeline_count++;
>>> +    }
>>> +    chains = kmalloc_array(timeline_count, sizeof(*chains),
>>> GFP_KERNEL);
>> I don't think that this will work. We need individually allocated
>> chain objects.
> Yeah, I shouldn't use &chains[i] to get address, how about using cast
> like "(struct dma_fence_chain *)chains[i]".

No, that still won't work. You need an array of individual objects here, 
not a single array with many entries.

E.g. something like this:

struct dma_fence_chain **chains = kmalloc_array(count, sizeof(void*));

for (i = 0; i < count; ++i)
     chains[i] = kmalloc(....);

Christian.

>
> btw, I will add a test case for signal array in igt.
>
>
> -David
>
>> Apart from that this looks good to me,
>> Christian.
>>
>>> +    if (!chains) {
>>> +        ret = -ENOMEM;
>>> +        goto err_points;
>>> +    }
>>> +
>>> +    for (i = 0, j = 0; i < args->count_handles; i++) {
>>> +        if (points[i]) {
>>> +            struct dma_fence *fence = dma_fence_get_stub();
>>> +
>>> +            drm_syncobj_add_point(syncobjs[i], &chains[j++],
>>> +                          fence, points[i]);
>>> +            dma_fence_put(fence);
>>> +        } else
>>> +            drm_syncobj_assign_null_handle(syncobjs[i]);
>>> +    }
>>> +err_points:
>>> +    kfree(points);
>>> +out:
>>> +    drm_syncobj_array_free(syncobjs, args->count_handles);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>    int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>>>                    struct drm_file *file_private)
>>>    {
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 88d6129d4a18..9a5fa3c26f22 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -943,6 +943,7 @@ extern "C" {
>>>    #define DRM_IOCTL_SYNCOBJ_QUERY        DRM_IOWR(0xCB, struct
>>> drm_syncobj_timeline_array)
>>>    #define DRM_IOCTL_SYNCOBJ_BINARY_TO_TIMELINE    DRM_IOWR(0xCC,
>>> struct drm_syncobj_transfer)
>>>    #define DRM_IOCTL_SYNCOBJ_TIMELINE_TO_BINARY    DRM_IOWR(0xCD,
>>> struct drm_syncobj_transfer)
>>> +#define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL    DRM_IOWR(0xCE, struct
>>> drm_syncobj_timeline_array)
>>>      /**
>>>     * Device specific ioctls should only be in their respective headers