Re: [PATCH v2 2/6] drm: Add drm_timeout_rel_to_jiffies()

Hi

Am 17.08.26 um 23:05 schrieb Maíra Canal:
> drm_timeout_abs_to_jiffies() covers the drivers whose wait UAPI takes an
> absolute deadline, but there is no equivalent for the drivers that
> express a wait as a duration. Drivers such as i915 and v3d convert the
> value themselves.
>
> Converting a nanosecond duration to jiffies needs some care.
> nsecs_to_jiffies() returns unsigned long, so on 32-bit a large
> userspace-supplied timeout overflows its range and is silently truncated.
>
> i915 already handles both cases in a local helper, which v3d has a copy
> of. Add the same conversion to the core, so that it is available to any
> driver and both copies can be dropped.
>
> Signed-off-by: Maíra Canal <[email protected]>
> ---
>   drivers/gpu/drm/drm_timeout.c | 34 ++++++++++++++++++++++++++++++++++
>   include/drm/drm_utils.h       |  1 +
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_timeout.c b/drivers/gpu/drm/drm_timeout.c
> index 78e9f65e5477..18c2719d1fe9 100644
> --- a/drivers/gpu/drm/drm_timeout.c
> +++ b/drivers/gpu/drm/drm_timeout.c
> @@ -9,6 +9,7 @@
>   #include <linux/export.h>
>   #include <linux/jiffies.h>
>   #include <linux/ktime.h>
> +#include <linux/math64.h>
>   #include <linux/sched.h>
>   
>   #include <drm/drm_utils.h>
> @@ -45,3 +46,36 @@ signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec)
>   	return timeout_jiffies64 + 1;
>   }
>   EXPORT_SYMBOL(drm_timeout_abs_to_jiffies);
> +
> +/**
> + * drm_timeout_rel_to_jiffies - calculate jiffies timeout from relative value
> + *
> + * @timeout_nsec: relative timeout in ns, 0 for poll
> + *
> + * Calculate the timeout in jiffies from a relative timeout in ns, for drivers
> + * whose UAPI expresses a wait as a duration rather than as a deadline.
> + *
> + * The result is clamped to MAX_JIFFY_OFFSET. That keeps it positive once it is
> + * converted to the signed long taken by dma_fence_wait_timeout() and friends,
> + * which matters on 32-bit, and keeps it distinct from MAX_SCHEDULE_TIMEOUT so
> + * that a finite wait is never understood as an infinite one.
> + *
> + * It's strongly discouraged to use relative timeouts in uAPIs, as they do not
> + * survive a restarted ioctl. A signal-interrupted ioctl is re-entered with the
> + * same arguments, so the duration starts counting from zero again. New uAPIs
> + * should take an absolute deadline and use drm_timeout_abs_to_jiffies().

I would not speak of uAPIs here, as it's unrelated to the function at 
hand. You might want to mention it in the commit description though.

Please also see my comments on the docs of drm_timeout_abs_to_jiffies(). 
Most of it applies here as well.

Best regards
Thomas

> + */
> +unsigned long drm_timeout_rel_to_jiffies(u64 timeout_nsec)
> +{
> +	/* make 0 timeout means poll, as for the absolute variant */
> +	if (timeout_nsec == 0)
> +		return 0;
> +
> +	/* nsecs_to_jiffies64() does not guard against overflow */
> +	if ((NSEC_PER_SEC % HZ) != 0 &&
> +	    div_u64(timeout_nsec, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
> +		return MAX_JIFFY_OFFSET;
> +
> +	return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(timeout_nsec) + 1);
> +}
> +EXPORT_SYMBOL(drm_timeout_rel_to_jiffies);
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> index 6a46f755daba..8c0cc9835413 100644
> --- a/include/drm/drm_utils.h
> +++ b/include/drm/drm_utils.h
> @@ -25,5 +25,6 @@ const struct drm_panel_backlight_quirk *
>   drm_get_panel_backlight_quirk(const struct drm_edid *edid);
>   
>   signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
> +unsigned long drm_timeout_rel_to_jiffies(u64 timeout_nsec);
>   
>   #endif
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)