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

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?

> 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 <=?

> +               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);

Thanks,
Stuart

>  
>  out:
>         return ret;