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

On 8/5/2026 1:37 PM, Summers, Stuart wrote:
> On Tue, 2026-08-04 at 16:22 -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
> But we aren't doing that any more right?
yes, need to remove this.
>
>> obtain the frequency as a good practice, and cache the changed value
>> to fix this issue.
>>
>> v2: pc->freq_ready is not set until after pc_adjust_freq_bounds().
>> Stay
>> with pc_action_query_task_state() instead.
>>
>> 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 | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 59f2fa79ad42..3106ac50827c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -911,6 +911,7 @@ 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;
>>   
>>          lockdep_assert_held(&pc->freq_lock);
>>   
>> @@ -933,8 +934,13 @@ 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)
>> +       min_freq = pc_get_min_freq(pc);
>> +       if (min_freq > pc->rp0_freq) {
>>                  ret = pc_set_min_freq(pc, pc->rp0_freq);
>> +               if (ret)
>> +                       goto out;
> nit: line break for readability
>
>> +               min_freq = pc->rp0_freq;
>> +       }
>>   
>>          /*
>>           * Setting GT RP min frequency to 1.2GHz by default for
>> @@ -947,8 +953,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 <=?
no need if it is =.
>
>> +               ret = pc_set_min_freq(pc, BMG_MIN_FREQ);
> Ok I didn't quite get this. Why do we want to ignore the min_freq if
> it's above BMG_MIN_FREQ? If we calculated as such earlier, wouldn't we
> want the higher value? so this should just be:
> pc_set_min_freq(pc, max(BMG_MIN_FREQ, min_freq);

We already set min_freq. if min > BMG_MIN, no need to set it again. We 
only do it if BMG_MIN is greater, saves one h2g.

Thanks,

Vinay.

>
> Thanks,
> Stuart
>
>>   
>>   out:
>>          return ret;