<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Yeah, the idea is indeed rather clever. I'm just wondering if we don't need to optimize it even more.<br>
<br>
In other words could some IOCTL overhead be saved here if we directly take the value for binary semaphores waits and signals?<br>
<br>
Christian.<br>
<br>
Am 08.08.19 um 17:23 schrieb Zhou, David(ChunMing):<br>
</div>
<blockquote type="cite" cite="mid:jpj2hq-gc4rtpn2vzyj-4d9mg2-idv0dc-g9aoyh-tnn7jq-3ji9nb-u7bc16lcz1gx-m949odea8admiqio0k-dawo4sbud2lr-yyfxz9-we1fbj-nve980-vo8wox-dpxeok-1g0ghu-27hadecortp7-dx288y.1565277745885@email.android.com">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from text --><style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 2px solid; } --></style>
<div>Thank you, I got your mean.<br>
when you have sideband payload, you will go into timeline path. Clever!<br>
<br>
-David<br>
<br>
-------- 原始邮件 --------<br>
主题:Re: [PATCH v3 1/1] drm/syncobj: add sideband payload<br>
发件人:Lionel Landwerlin <br>
收件人:"Zhou, David(ChunMing)" ,<a class="moz-txt-link-abbreviated" href="mailto:dri-devel@freedesktop.org">dri-devel@freedesktop.org</a><br>
抄送:"Koenig, Christian" ,Jason Ekstrand ,"Zhou, David(ChunMing)" <br>
<br>
</div>
<font size="2"><span style="font-size:11pt;">
<div class="PlainText">On 08/08/2019 17:48, Chunming Zhou wrote:<br>
> 在 2019/8/8 22:34, Lionel Landwerlin 写道:<br>
>> On 08/08/2019 17:16, Chunming Zhou wrote:<br>
>>> 在 2019/8/8 22:01, Lionel Landwerlin 写道:<br>
>>>> On 08/08/2019 16:42, Chunming Zhou wrote:<br>
>>>>> No, I just see your comment "The next vkQueueSubmit()<br>
>>>>> waiting on a the syncobj will read the sideband payload and wait for a<br>
>>>>> fence chain element with a seqno superior or equal to the sideband<br>
>>>>> payload value to be added into the fence chain and use that fence to<br>
>>>>> trigger the submission on the kernel driver. ".<br>
>>>> That was meant to say wait on the chain fence to reach the sideband<br>
>>>> payload value.<br>
>>>><br>
>>>> It a bit confusing but I can't see any other way to word it.<br>
>>>><br>
>>>><br>
>>>>> In that, you mentioned wait for sideband.<br>
>>>>> So I want to know we how to use that, maybe I miss something in<br>
>>>>> previous<br>
>>>>> discussing thread.<br>
>>>> In QueueSubmit(), we start by reading the sideband payloads :<br>
>>>> <a href="https://gitlab.freedesktop.org/llandwerlin/mesa/blob/review/anv-timeline_semaphore_prep/src/intel/vulkan/anv_queue.c#L655" moz-do-not-send="true">
https://gitlab.freedesktop.org/llandwerlin/mesa/blob/review/anv-timeline_semaphore_prep/src/intel/vulkan/anv_queue.c#L655</a><br>
>>>><br>
>>>><br>
>>>> Then build everything for the submission and hand it over to the<br>
>>>> submission thread.<br>
>>>><br>
>>>> Instead of the just waiting on the timeline semaphore values, we also<br>
>>>> wait on the binary semaphore sideband payload values.<br>
>>> Waiting on timeline values is when finding fence in kernel.<br>
>><br>
>> Hmm aren't you waiting in a thread in userspace?<br>
> Yes, For timeline case, we can use waitForSyncobj()..., At begin of<br>
> QueueThread, I let it wait in cs_ioctl when drm_syncobj_find_fence.<br>
><br>
> But I still didn't get how to wait on sideband for binary Syncobj.<br>
><br>
> Ah, I see, you will compare it in your QueueThread, if sideband value is<br>
>   >= expected, then do submission, otherwise, loop QueueThread, right?<br>
<br>
<br>
Just treat binary semaphores as timelines and waitForSyncobj on the <br>
sideband payload value.<br>
<br>
It should make the submission thread any busier than currently.<br>
<br>
<br>
-Lionel<br>
<br>
<br>
><br>
> That sounds the QueueThread will be always busy.<br>
><br>
> -David<br>
><br>
><br>
>><br>
>>> But I don't see how to wait/block in kernel when finding fence for<br>
>>> binary sideband payload  values.<br>
>><br>
>> Essentially our driver now handles timelines & binary semaphore using<br>
>> dma-fence-chain in both cases.<br>
><br>
><br>
>> Only with timelines we take the values submitted by the vulkan<br>
>> application.<br>
><br>
>> The binary semaphore auto increment on vkQueueSubmit() and that is<br>
>> managed by the userspace driver.<br>
>><br>
>><br>
>> -Lionel<br>
>><br>
>><br>
>>><br>
>>> -David<br>
>>><br>
>>>> Finally before exiting the QueueSubmit() call, we bump the sideband<br>
>>>> payloads of all the binary semaphores have have been signaled :<br>
>>>> <a href="https://gitlab.freedesktop.org/llandwerlin/mesa/blob/review/anv-timeline_semaphore_prep/src/intel/vulkan/anv_queue.c#L806" moz-do-not-send="true">
https://gitlab.freedesktop.org/llandwerlin/mesa/blob/review/anv-timeline_semaphore_prep/src/intel/vulkan/anv_queue.c#L806</a><br>
>>>><br>
>>>><br>
>>>><br>
>>>> Whoever calls QueueSubmit() after that will pickup the new sideband<br>
>>>> payload values to wait on.<br>
>>>><br>
>>>><br>
>>>> -Lionel<br>
>>>><br>
>>>><br>
>>>><br>
>>>>> -DAvid<br>
>>>>><br>
>>>>><br>
>>>>> 在 2019/8/8 21:38, Lionel Landwerlin 写道:<br>
>>>>>> Interesting question :)<br>
>>>>>><br>
>>>>>> I didn't see any usecase for that.<br>
>>>>>> This sideband payload value is used for a wait so waiting on the wait<br>
>>>>>> value for another wait is bit meta :)<br>
>>>>>><br>
>>>>>> Do you have a use case for that?<br>
>>>>>><br>
>>>>>> -Lionel<br>
>>>>>><br>
>>>>>> On 08/08/2019 16:23, Chunming Zhou wrote:<br>
>>>>>>> The propursal is fine with me.<br>
>>>>>>><br>
>>>>>>> one question:<br>
>>>>>>><br>
>>>>>>> how to wait sideband payload? Following patch will show that?<br>
>>>>>>><br>
>>>>>>> -David<br>
>>>>>>><br>
>>>>>>> 在 2019/8/8 21:14, Lionel Landwerlin 写道:<br>
>>>>>>>> The Vulkan timeline semaphores allow signaling to happen on the<br>
>>>>>>>> point<br>
>>>>>>>> of the timeline without all of the its dependencies to be created.<br>
>>>>>>>><br>
>>>>>>>> The current 2 implementations (AMD/Intel) of the Vulkan spec on<br>
>>>>>>>> top of<br>
>>>>>>>> the Linux kernel are using a thread to wait on the dependencies<br>
>>>>>>>> of a<br>
>>>>>>>> given point to materialize and delay actual submission to the<br>
>>>>>>>> kernel<br>
>>>>>>>> driver until the wait completes.<br>
>>>>>>>><br>
>>>>>>>> If a binary semaphore is submitted for signaling along the side<br>
>>>>>>>> of a<br>
>>>>>>>> timeline semaphore waiting for completion that means that the drm<br>
>>>>>>>> syncobj associated with that binary semaphore will not have a DMA<br>
>>>>>>>> fence associated with it by the time vkQueueSubmit() returns. This<br>
>>>>>>>> and<br>
>>>>>>>> the fact that a binary semaphore can be signaled and unsignaled as<br>
>>>>>>>> before its DMA fences materialize mean that we cannot just rely on<br>
>>>>>>>> the<br>
>>>>>>>> fence within the syncobj but we also need a sideband payload<br>
>>>>>>>> verifying<br>
>>>>>>>> that the fence in the syncobj matches the last submission from the<br>
>>>>>>>> Vulkan API point of view.<br>
>>>>>>>><br>
>>>>>>>> This change adds a sideband payload that is incremented with<br>
>>>>>>>> signaled<br>
>>>>>>>> syncobj when vkQueueSubmit() is called. The next vkQueueSubmit()<br>
>>>>>>>> waiting on a the syncobj will read the sideband payload and wait<br>
>>>>>>>> for a<br>
>>>>>>>> fence chain element with a seqno superior or equal to the sideband<br>
>>>>>>>> payload value to be added into the fence chain and use that<br>
>>>>>>>> fence to<br>
>>>>>>>> trigger the submission on the kernel driver.<br>
>>>>>>>><br>
>>>>>>>> v2: Use a separate ioctl to get/set the sideband value (Christian)<br>
>>>>>>>><br>
>>>>>>>> v3: Use 2 ioctls for get/set (Christian)<br>
>>>>>>>><br>
>>>>>>>> Signed-off-by: Lionel Landwerlin <a class="moz-txt-link-rfc2396E" href="mailto:lionel.g.landwerlin@intel.com">
<lionel.g.landwerlin@intel.com></a><br>
>>>>>>>> Cc: Christian Koenig <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com">
<Christian.Koenig@amd.com></a><br>
>>>>>>>> Cc: Jason Ekstrand <a class="moz-txt-link-rfc2396E" href="mailto:jason@jlekstrand.net">
<jason@jlekstrand.net></a><br>
>>>>>>>> Cc: David(ChunMing) Zhou <a class="moz-txt-link-rfc2396E" href="mailto:David1.Zhou@amd.com">
<David1.Zhou@amd.com></a><br>
>>>>>>>> ---<br>
>>>>>>>>       drivers/gpu/drm/drm_internal.h |  4 ++<br>
>>>>>>>>       drivers/gpu/drm/drm_ioctl.c    |  5 +++<br>
>>>>>>>>       drivers/gpu/drm/drm_syncobj.c  | 79<br>
>>>>>>>> +++++++++++++++++++++++++++++++++-<br>
>>>>>>>>       include/drm/drm_syncobj.h      |  9 ++++<br>
>>>>>>>>       include/uapi/drm/drm.h         |  2 +<br>
>>>>>>>>       5 files changed, 98 insertions(+), 1 deletion(-)<br>
>>>>>>>><br>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_internal.h<br>
>>>>>>>> b/drivers/gpu/drm/drm_internal.h<br>
>>>>>>>> index 51a2055c8f18..0c9736199df0 100644<br>
>>>>>>>> --- a/drivers/gpu/drm/drm_internal.h<br>
>>>>>>>> +++ b/drivers/gpu/drm/drm_internal.h<br>
>>>>>>>> @@ -208,6 +208,10 @@ int drm_syncobj_timeline_signal_ioctl(struct<br>
>>>>>>>> drm_device *dev, void *data,<br>
>>>>>>>>                             struct drm_file *file_private);<br>
>>>>>>>>       int drm_syncobj_query_ioctl(struct drm_device *dev, void<br>
>>>>>>>> *data,<br>
>>>>>>>>                       struct drm_file *file_private);<br>
>>>>>>>> +int drm_syncobj_get_sideband_ioctl(struct drm_device *dev, void<br>
>>>>>>>> *data,<br>
>>>>>>>> +                   struct drm_file *file_private);<br>
>>>>>>>> +int drm_syncobj_set_sideband_ioctl(struct drm_device *dev, void<br>
>>>>>>>> *data,<br>
>>>>>>>> +                   struct drm_file *file_private);<br>
>>>>>>>>          /* drm_framebuffer.c */<br>
>>>>>>>>       void drm_framebuffer_print_info(struct drm_printer *p,<br>
>>>>>>>> unsigned<br>
>>>>>>>> int indent,<br>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c<br>
>>>>>>>> b/drivers/gpu/drm/drm_ioctl.c<br>
>>>>>>>> index f675a3bb2c88..48500bf62801 100644<br>
>>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c<br>
>>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c<br>
>>>>>>>> @@ -703,6 +703,11 @@ static const struct drm_ioctl_desc<br>
>>>>>>>> drm_ioctls[]<br>
>>>>>>>> = {<br>
>>>>>>>>                     DRM_RENDER_ALLOW),<br>
>>>>>>>>           DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY,<br>
>>>>>>>> drm_syncobj_query_ioctl,<br>
>>>>>>>>                     DRM_RENDER_ALLOW),<br>
>>>>>>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_GET_SIDEBAND,<br>
>>>>>>>> drm_syncobj_get_sideband_ioctl,<br>
>>>>>>>> +              DRM_RENDER_ALLOW),<br>
>>>>>>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SET_SIDEBAND,<br>
>>>>>>>> drm_syncobj_set_sideband_ioctl,<br>
>>>>>>>> +              DRM_RENDER_ALLOW),<br>
>>>>>>>> +<br>
>>>>>>>>           DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE,<br>
>>>>>>>> drm_crtc_get_sequence_ioctl, 0),<br>
>>>>>>>>           DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,<br>
>>>>>>>> drm_crtc_queue_sequence_ioctl, 0),<br>
>>>>>>>>           DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE,<br>
>>>>>>>> drm_mode_create_lease_ioctl, DRM_MASTER),<br>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c<br>
>>>>>>>> b/drivers/gpu/drm/drm_syncobj.c<br>
>>>>>>>> index b927e482e554..c90ef20b9242 100644<br>
>>>>>>>> --- a/drivers/gpu/drm/drm_syncobj.c<br>
>>>>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c<br>
>>>>>>>> @@ -1150,8 +1150,10 @@ drm_syncobj_reset_ioctl(struct drm_device<br>
>>>>>>>> *dev, void *data,<br>
>>>>>>>>           if (ret < 0)<br>
>>>>>>>>               return ret;<br>
>>>>>>>>       -    for (i = 0; i < args->count_handles; i++)<br>
>>>>>>>> +    for (i = 0; i < args->count_handles; i++) {<br>
>>>>>>>>               drm_syncobj_replace_fence(syncobjs[i], NULL);<br>
>>>>>>>> +        syncobjs[i]->sideband_payload = 0;<br>
>>>>>>>> +    }<br>
>>>>>>>>              drm_syncobj_array_free(syncobjs, args->count_handles);<br>
>>>>>>>>       @@ -1321,6 +1323,81 @@ int drm_syncobj_query_ioctl(struct<br>
>>>>>>>> drm_device *dev, void *data,<br>
>>>>>>>>               if (ret)<br>
>>>>>>>>                   break;<br>
>>>>>>>>           }<br>
>>>>>>>> +<br>
>>>>>>>> +    drm_syncobj_array_free(syncobjs, args->count_handles);<br>
>>>>>>>> +<br>
>>>>>>>> +    return ret;<br>
>>>>>>>> +}<br>
>>>>>>>> +<br>
>>>>>>>> +int drm_syncobj_get_sideband_ioctl(struct drm_device *dev, void<br>
>>>>>>>> *data,<br>
>>>>>>>> +                   struct drm_file *file_private)<br>
>>>>>>>> +{<br>
>>>>>>>> +    struct drm_syncobj_timeline_array *args = data;<br>
>>>>>>>> +    struct drm_syncobj **syncobjs;<br>
>>>>>>>> +    uint64_t __user *points = u64_to_user_ptr(args->points);<br>
>>>>>>>> +    uint32_t i;<br>
>>>>>>>> +    int ret;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))<br>
>>>>>>>> +        return -EOPNOTSUPP;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (args->pad != 0)<br>
>>>>>>>> +        return -EINVAL;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (args->count_handles == 0)<br>
>>>>>>>> +        return -EINVAL;<br>
>>>>>>>> +<br>
>>>>>>>> +    ret = drm_syncobj_array_find(file_private,<br>
>>>>>>>> + u64_to_user_ptr(args->handles),<br>
>>>>>>>> +                     args->count_handles,<br>
>>>>>>>> +                     &syncobjs);<br>
>>>>>>>> +    if (ret < 0)<br>
>>>>>>>> +        return ret;<br>
>>>>>>>> +<br>
>>>>>>>> +    for (i = 0; i < args->count_handles; i++) {<br>
>>>>>>>> +        copy_to_user(&points[i], &syncobjs[i]->sideband_payload,<br>
>>>>>>>> sizeof(uint64_t));<br>
>>>>>>>> +        ret = ret ? -EFAULT : 0;<br>
>>>>>>>> +        if (ret)<br>
>>>>>>>> +            break;<br>
>>>>>>>> +    }<br>
>>>>>>>> +<br>
>>>>>>>> +    drm_syncobj_array_free(syncobjs, args->count_handles);<br>
>>>>>>>> +<br>
>>>>>>>> +    return ret;<br>
>>>>>>>> +}<br>
>>>>>>>> +<br>
>>>>>>>> +int drm_syncobj_set_sideband_ioctl(struct drm_device *dev, void<br>
>>>>>>>> *data,<br>
>>>>>>>> +                   struct drm_file *file_private)<br>
>>>>>>>> +{<br>
>>>>>>>> +    struct drm_syncobj_timeline_array *args = data;<br>
>>>>>>>> +    struct drm_syncobj **syncobjs;<br>
>>>>>>>> +    uint64_t __user *points = u64_to_user_ptr(args->points);<br>
>>>>>>>> +    uint32_t i;<br>
>>>>>>>> +    int ret;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))<br>
>>>>>>>> +        return -EOPNOTSUPP;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (args->pad != 0)<br>
>>>>>>>> +        return -EINVAL;<br>
>>>>>>>> +<br>
>>>>>>>> +    if (args->count_handles == 0)<br>
>>>>>>>> +        return -EINVAL;<br>
>>>>>>>> +<br>
>>>>>>>> +    ret = drm_syncobj_array_find(file_private,<br>
>>>>>>>> + u64_to_user_ptr(args->handles),<br>
>>>>>>>> +                     args->count_handles,<br>
>>>>>>>> +                     &syncobjs);<br>
>>>>>>>> +    if (ret < 0)<br>
>>>>>>>> +        return ret;<br>
>>>>>>>> +<br>
>>>>>>>> +    for (i = 0; i < args->count_handles; i++) {<br>
>>>>>>>> + copy_from_user(&syncobjs[i]->sideband_payload, &points[i],<br>
>>>>>>>> sizeof(uint64_t));<br>
>>>>>>>> +        ret = ret ? -EFAULT : 0;<br>
>>>>>>>> +        if (ret)<br>
>>>>>>>> +            break;<br>
>>>>>>>> +    }<br>
>>>>>>>> +<br>
>>>>>>>>           drm_syncobj_array_free(syncobjs, args->count_handles);<br>
>>>>>>>>              return ret;<br>
>>>>>>>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h<br>
>>>>>>>> index 6cf7243a1dc5..b587b8e07547 100644<br>
>>>>>>>> --- a/include/drm/drm_syncobj.h<br>
>>>>>>>> +++ b/include/drm/drm_syncobj.h<br>
>>>>>>>> @@ -61,6 +61,15 @@ struct drm_syncobj {<br>
>>>>>>>>            * @file: A file backing for this syncobj.<br>
>>>>>>>>            */<br>
>>>>>>>>           struct file *file;<br>
>>>>>>>> +    /**<br>
>>>>>>>> +     * @sideband_payload: A 64bit side band payload.<br>
>>>>>>>> +     *<br>
>>>>>>>> +     * We use the sideband payload value to wait on binary syncobj<br>
>>>>>>>> fences<br>
>>>>>>>> +     * to materialize. It is a reservation mechanism for the<br>
>>>>>>>> signaler to<br>
>>>>>>>> +     * express that at some point in the future a dma fence with<br>
>>>>>>>> the same<br>
>>>>>>>> +     * seqno will be put into the syncobj.<br>
>>>>>>>> +     */<br>
>>>>>>>> +    u64 sideband_payload;<br>
>>>>>>>>       };<br>
>>>>>>>>          void drm_syncobj_free(struct kref *kref);<br>
>>>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h<br>
>>>>>>>> index 8a5b2f8f8eb9..cffdc6c9772c 100644<br>
>>>>>>>> --- a/include/uapi/drm/drm.h<br>
>>>>>>>> +++ b/include/uapi/drm/drm.h<br>
>>>>>>>> @@ -946,6 +946,8 @@ extern "C" {<br>
>>>>>>>>       #define DRM_IOCTL_SYNCOBJ_QUERY DRM_IOWR(0xCB, struct<br>
>>>>>>>> drm_syncobj_timeline_array)<br>
>>>>>>>>       #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct<br>
>>>>>>>> drm_syncobj_transfer)<br>
>>>>>>>>       #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD,<br>
>>>>>>>> struct drm_syncobj_timeline_array)<br>
>>>>>>>> +#define DRM_IOCTL_SYNCOBJ_GET_SIDEBAND DRM_IOR(0xCE, struct<br>
>>>>>>>> drm_syncobj_timeline_array)<br>
>>>>>>>> +#define DRM_IOCTL_SYNCOBJ_SET_SIDEBAND DRM_IOWR(0xCF, struct<br>
>>>>>>>> drm_syncobj_timeline_array)<br>
>>>>>>>>          /**<br>
>>>>>>>>        * Device specific ioctls should only be in their respective<br>
>>>>>>>> headers<br>
>><br>
<br>
</div>
</span></font></blockquote>
<br>
</body>
</html>