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 9:56 PM, Lucas De Marchi wrote:
> On Thu, Sep 11, 2025 at 02:21:28PM -0700, Belgaumkar, Vinay wrote:
>>
>> 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.
>
> problem with "limit" is that it gives no clue if it's limiting down or
> up. wait_for_act_freq_up_to() or s/max_freq/max_limit/ ?

true. Either one should clarify it a little.

Thanks,

Vinay.

>
>>
>> with that,
>>
>> Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>
> thanks
> Lucas De Marchi
>
>
>>
>>>  {
>>> -    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);
>>>