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

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?

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