Re: [PATCH] drm/xe: Fix a bug in pc_adjust_freq_bounds()

On 8/5/2026 1:23 PM, Summers, Stuart wrote:
> On Mon, 2026-08-03 at 15:21 -0700, Vinay Belgaumkar wrote:
>> In cases where min frequency was actually greater than BMG_MIN_FREQ,
>> we were not using the updated min frequency as there was a missing
>> call to pc_action_query_task_state() between the two settings of
>> min frequency. Use the xe_guc_pc_get_min_freq_locked() variant to
>> obtain the frequency as a good practice, and cache the changed value
>> to fix this issue.
>>
>> Fixes: bdde16c9ac5c ("drm/xe/bmg: Update Wa_14022085890")
>> Signed-off-by: Balasubramani Vivekanandan
>> <[email protected]>
>> Signed-off-by: Vinay Belgaumkar <[email protected]>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pc.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 59f2fa79ad42..8bc2ae15a835 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -911,10 +911,15 @@ static bool pc_needs_min_freq_change(struct
>> xe_guc_pc *pc)
>>   static int pc_adjust_freq_bounds(struct xe_guc_pc *pc)
>>   {
>>          int ret;
>> +       u32 min_freq, max_freq;
>>   
>>          lockdep_assert_held(&pc->freq_lock);
>>   
>> -       ret = pc_action_query_task_state(pc);
>> +       ret = xe_guc_pc_get_min_freq_locked(pc, &min_freq);
>> +       if (ret)
>> +               goto out;
>> +
>> +       ret = xe_guc_pc_get_min_freq_locked(pc, &max_freq);
> Should this be xe_guc_pc_get_max_freq_locked?

yeah, I have sent v2, fixed the bugs there. Essentially, we cannot use 
any of the get_*_freq_loacked() variants yet since the pc-freq_ready is 
not set yet.

Thanks,

Vinay.

>
>>          if (ret)
>>                  goto out;
>>   
>> @@ -923,7 +928,7 @@ static int pc_adjust_freq_bounds(struct xe_guc_pc
>> *pc)
>>           * overclocking. Let's adjust it to the Hardware RP0, which
>> is the
>>           * regular maximum
>>           */
>> -       if (pc_get_max_freq(pc) > pc->rp0_freq) {
>> +       if (max_freq > pc->rp0_freq) {
>>                  ret = pc_set_max_freq(pc, pc->rp0_freq);
>>                  if (ret)
>>                          goto out;
>> @@ -933,8 +938,12 @@ static int pc_adjust_freq_bounds(struct
>> xe_guc_pc *pc)
>>           * Same thing happens for Server platforms where min is
>> listed as
>>           * RPMax
>>           */
>> -       if (pc_get_min_freq(pc) > pc->rp0_freq)
>> +       if (min_freq > pc->rp0_freq) {
>>                  ret = pc_set_min_freq(pc, pc->rp0_freq);
> Should we be using xe_guc_pc_set_min_freq_locked() here to align with
> the reads in pc_needs_min_freq_change?
>
>> +               if (ret)
>> +                       goto out;
> nit: extra line here for readability
>
>> +               min_freq = pc->rp0_freq;
>> +       }
>>   
>>          /*
>>           * Setting GT RP min frequency to 1.2GHz by default for
>> @@ -947,8 +956,8 @@ static int pc_adjust_freq_bounds(struct xe_guc_pc
>> *pc)
>>           * we aren't expecting high power output across board
>>           *
>>           */
>> -       if (pc_needs_min_freq_change(pc))
>> -               ret = pc_set_min_freq(pc, max(BMG_MIN_FREQ,
>> pc_get_min_freq(pc)));
>> +       if (pc_needs_min_freq_change(pc) && min_freq < BMG_MIN_FREQ)
> Should this be <=?
>
> Thanks,
> Stuart
>
>> +               ret = pc_set_min_freq(pc, BMG_MIN_FREQ);
>>   
>>   out:
>>          return ret;