[RFC,1/3] drm: drm_syncobj: Add note in DOC about absolute timeout values

Commit Message

John Stultz July 12, 2022, 4:22 a.m. UTC

After having to debug down through the kernel to figure out
why my _WAIT calls were always timing out, I realized its
an absolute timeout value instead of the more common relative
timeouts.

This detail should be called out in the documentation, as while
the absolute value makes sense here, its not as common for timeout
values.

Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Jason Ekstrand <[e-mail]>
Cc: Christian König <[e-mail]>
Cc: Lionel Landwerlin <[e-mail]>
Cc: Chunming Zhou <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: [e-mail]
Signed-off-by: John Stultz <[e-mail]>
---
 drivers/gpu/drm/drm_syncobj.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

John Stultz July 12, 2022, 5:29 a.m. UTC | #1

On Mon, Jul 11, 2022 at 9:23 PM John Stultz <[e-mail]> wrote:
>
> After having to debug down through the kernel to figure out
> why my _WAIT calls were always timing out, I realized its
> an absolute timeout value instead of the more common relative
> timeouts.
>
> This detail should be called out in the documentation, as while
> the absolute value makes sense here, its not as common for timeout
> values.
>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> Cc: Chunming Zhou <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: John Stultz <[e-mail]>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 7e48dcd1bee4..b84d842a1c21 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -136,6 +136,10 @@
>   * requirement is inherited from the wait-before-signal behavior required by
>   * the Vulkan timeline semaphore API.
>   *
> + * It should be noted, that both &DRM_IOCTL_SYNCOBJ_WAIT and
> + * &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT takes an *absolute* CLOCK_MONOTONIC

Gah. That should be &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT. I must have
pasted in the wrong symbol.

Fixed in my tree and I'll share v2 in a few days so I can get
additional feedback.

thanks
-john

Christian König July 12, 2022, 7:40 a.m. UTC | #2

Am 12.07.22 um 06:22 schrieb John Stultz:
> After having to debug down through the kernel to figure out
> why my _WAIT calls were always timing out, I realized its
> an absolute timeout value instead of the more common relative
> timeouts.
>
> This detail should be called out in the documentation, as while
> the absolute value makes sense here, its not as common for timeout
> values.

Well absolute timeout values are mandatory for making -ERESTARTSYS work 
without any additional handling.

So using them is recommended for ~20 years now and IIRC even documented 
somewhere.

See here as well https://lwn.net/Articles/17744/ how much trouble system 
calls with relative timeouts are.

Regards,
Christian.

>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> Cc: Chunming Zhou <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: John Stultz <[e-mail]>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 7e48dcd1bee4..b84d842a1c21 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -136,6 +136,10 @@
>    * requirement is inherited from the wait-before-signal behavior required by
>    * the Vulkan timeline semaphore API.
>    *
> + * It should be noted, that both &DRM_IOCTL_SYNCOBJ_WAIT and
> + * &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT takes an *absolute* CLOCK_MONOTONIC
> + * nanosecond value for the timeout value. Accidentally passing relative time
> + * values will likely result in an immediate -ETIME return.
>    *
>    * Import/export of syncobjs
>    * -------------------------

John Stultz July 12, 2022, 3:48 p.m. UTC | #3

On Tue, Jul 12, 2022 at 12:40 AM Christian König
<[e-mail]> wrote:
> Am 12.07.22 um 06:22 schrieb John Stultz:
> > After having to debug down through the kernel to figure out
> > why my _WAIT calls were always timing out, I realized its
> > an absolute timeout value instead of the more common relative
> > timeouts.
> >
> > This detail should be called out in the documentation, as while
> > the absolute value makes sense here, its not as common for timeout
> > values.
>
> Well absolute timeout values are mandatory for making -ERESTARTSYS work
> without any additional handling.

Yes! I'm not saying it's wrong to use absolute values, just that
relative values are common enough to create some confusion here.

> So using them is recommended for ~20 years now and IIRC even documented
> somewhere.

So in addition to "somewhere", why not in the interface documentation as well?

> See here as well https://lwn.net/Articles/17744/ how much trouble system
> calls with relative timeouts are.

Yep. Well aware. :)

thanks
-john

Christian König July 12, 2022, 3:54 p.m. UTC | #4

Am 12.07.22 um 17:48 schrieb John Stultz:
> On Tue, Jul 12, 2022 at 12:40 AM Christian König
> <[e-mail]> wrote:
>> Am 12.07.22 um 06:22 schrieb John Stultz:
>>> After having to debug down through the kernel to figure out
>>> why my _WAIT calls were always timing out, I realized its
>>> an absolute timeout value instead of the more common relative
>>> timeouts.
>>>
>>> This detail should be called out in the documentation, as while
>>> the absolute value makes sense here, its not as common for timeout
>>> values.
>> Well absolute timeout values are mandatory for making -ERESTARTSYS work
>> without any additional handling.
> Yes! I'm not saying it's wrong to use absolute values, just that
> relative values are common enough to create some confusion here.
>
>> So using them is recommended for ~20 years now and IIRC even documented
>> somewhere.
> So in addition to "somewhere", why not in the interface documentation as well?

Because it's the desired default behavior and we shouldn't have to 
document that on every instance it is used.

IIRC it's documented centralized (but I need to dig that up as well).

What we should do instead is to have a warning on every relative timeout 
that this probably shouldn't be used as example.

Regards,+
Christian.

>
>> See here as well https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F17744%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C68a13ac3906d4ac4cc4308da641df25c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637932377042931797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=dM4BkqnO0LrsdKBwKKMvx4zMabWrM%2FY7pPGDsdFO%2BnI%3D&amp;reserved=0 how much trouble system
>> calls with relative timeouts are.
> Yep. Well aware. :)
>
> thanks
> -john

Patch

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 7e48dcd1bee4..b84d842a1c21 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -136,6 +136,10 @@ 
  * requirement is inherited from the wait-before-signal behavior required by
  * the Vulkan timeline semaphore API.
  *
+ * It should be noted, that both &DRM_IOCTL_SYNCOBJ_WAIT and
+ * &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT takes an *absolute* CLOCK_MONOTONIC
+ * nanosecond value for the timeout value. Accidentally passing relative time
+ * values will likely result in an immediate -ETIME return.
  *
  * Import/export of syncobjs
  * -------------------------