Re: [PATCH v3 2/5] drm/xe/guc_pc: Use poll_timeout_us() for waiting

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 9/11/2025 10:25 AM, Lucas De Marchi wrote:
> Convert wait_for_pc_state() and wait_for_act_freq_limit() to
> poll_timeout_us(). This brings 2 changes in behavior: Drop the
> exponential wait and fix a potential much longer sleep.
>
> usleep_range() will wait anywhere between `wait` and `wait << 1`, so
> it's not correct to assume `slept += wait`.  This code is not really
> accurate. Pairing this with the exponential wait increase, it could be
> waiting much longer than intended.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> v2: Simplify functions by removing helper variables and changing break
>      condition on poll_timeout_us() call (Maarten)
> v3: dial a little bit back from v2: better to have some helper vars
>      and avoid the weird syntax (Jani)
> ---
>   drivers/gpu/drm/xe/xe_guc_pc.c | 42 ++++++++++++------------------------------
>   1 file changed, 12 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 68a5bf8e39462..ecfe37836692f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -7,6 +7,7 @@
>   
>   #include <linux/cleanup.h>
>   #include <linux/delay.h>
> +#include <linux/iopoll.h>
>   #include <linux/jiffies.h>
>   #include <linux/ktime.h>
>   #include <linux/wait_bit.h>
> @@ -130,26 +131,16 @@ static struct iosys_map *pc_to_maps(struct xe_guc_pc *pc)
>   	 FIELD_PREP(HOST2GUC_PC_SLPC_REQUEST_MSG_1_EVENT_ARGC, count))
>   
>   static int wait_for_pc_state(struct xe_guc_pc *pc,
> -			     enum slpc_global_state state,
> +			     enum slpc_global_state target_state,
>   			     int timeout_ms)
>   {
> -	int timeout_us = 1000 * timeout_ms;
> -	int slept, wait = 10;
> +	enum slpc_global_state state;
>   
>   	xe_device_assert_mem_access(pc_to_xe(pc));
>   
> -	for (slept = 0; slept < timeout_us;) {
> -		if (slpc_shared_data_read(pc, header.global_state) == state)
> -			return 0;
> -
> -		usleep_range(wait, wait << 1);
> -		slept += wait;
> -		wait <<= 1;
> -		if (slept + wait > timeout_us)
> -			wait = timeout_us - slept;
> -	}
> -
> -	return -ETIMEDOUT;
> +	return poll_timeout_us(state = slpc_shared_data_read(pc, header.global_state),
> +			       state == target_state,
> +			       20, timeout_ms * USEC_PER_MSEC, false);
>   }
>   
>   static int wait_for_flush_complete(struct xe_guc_pc *pc)
> @@ -164,24 +155,15 @@ static int wait_for_flush_complete(struct xe_guc_pc *pc)
>   	return 0;
>   }
>   
> -static int wait_for_act_freq_limit(struct xe_guc_pc *pc, u32 freq)
> +static int wait_for_act_freq_limit(struct xe_guc_pc *pc, u32 max_freq)

Nit: rename to limit_freq or something instead of max_freq in order to 
avoid linking this to gt max freq.

with that,

Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

>   {
> -	int timeout_us = SLPC_ACT_FREQ_TIMEOUT_MS * USEC_PER_MSEC;
> -	int slept, wait = 10;
> -
> -	for (slept = 0; slept < timeout_us;) {
> -		if (xe_guc_pc_get_act_freq(pc) <= freq)
> -			return 0;
> -
> -		usleep_range(wait, wait << 1);
> -		slept += wait;
> -		wait <<= 1;
> -		if (slept + wait > timeout_us)
> -			wait = timeout_us - slept;
> -	}
> +	u32 freq;
>   
> -	return -ETIMEDOUT;
> +	return poll_timeout_us(freq = xe_guc_pc_get_act_freq(pc),
> +			       freq <= max_freq,
> +			       20, SLPC_ACT_FREQ_TIMEOUT_MS * USEC_PER_MSEC, false);
>   }
> +
>   static int pc_action_reset(struct xe_guc_pc *pc)
>   {
>   	struct xe_guc_ct *ct = pc_to_ct(pc);
>