Re: [PATCH 4/4] DO-NOT-MERGE: drm/i915: Use poll_timeout_us()

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 03 Jul 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make sure poll_timeout_us() works by using it in i915
> instead of the custom __wait_for().
>
> Remaining difference between two:
>                | poll_timeout_us() | __wait_for()
> ---------------------------------------------------
> backoff        | fixed interval    | exponential
> usleep_range() | N/4+1 to N        | N to N*2
> clock          | MONOTONIC         | MONOTONIC_RAW
>
> Just a test hack for now, proper conversion probably
> needs actual thought.

Agreed.

I feel pretty strongly about converting everything to use
poll_timeout_us() and poll_timeout_us_atomic() directly. I think the
plethora of wait_for variants in i915_utils.h is more confusing than
helpful (even if some of them are supposed to be "simpler"
alternatives). I also think the separate atomic variant is better than
magically deciding that based on delay length.

I'm also not all that convinced about the exponential wait. Not all of
the wait_for versions use it, and then it needs to have a max wait
anyway (we have an issue with xe not having that [1]). I believe callers
can decide on a sleep length that is appropriate for the timeout, case
by case, and gut feeling says it's probably fine. ;)

BR,
Jani.


[1] https://lore.kernel.org/r/fe44d12c701c3d410de6e0ebc1f08bae2eec10a1@intel.com


>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: David Laight <david.laight.linux@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Matt Wagantall <mattw@codeaurora.org>
> Cc: Dejin Zheng <zhengdejin5@gmail.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_utils.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index f7fb40cfdb70..8509d1de1901 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -32,6 +32,7 @@
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  #include <linux/sched/clock.h>
> +#include <linux/iopoll.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/hypervisor.h>
> @@ -238,7 +239,7 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>   * timeout could be due to preemption or similar and we've never had a chance to
>   * check the condition before the timeout.
>   */
> -#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
> +#define __wait_for_old(OP, COND, US, Wmin, Wmax) ({ \
>  	const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
>  	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
>  	int ret__;							\
> @@ -263,6 +264,8 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>  	ret__;								\
>  })
>  
> +#define __wait_for(OP, COND, US, Wmin, Wmax)				\
> +	poll_timeout_us(OP, COND, (Wmin), (US), false)
>  #define _wait_for(COND, US, Wmin, Wmax)	__wait_for(, (COND), (US), (Wmin), \
>  						   (Wmax))
>  #define wait_for(COND, MS)		_wait_for((COND), (MS) * 1000, 10, 1000)

-- 
Jani Nikula, Intel