Re: [PATCH] drm/xe/guc: Refresh RPe frequency while setting min frequency

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Oct 10, 2025 at 10:29:10AM +0530, Nilawar, Badal wrote:
> 
> On 10-10-2025 02:17, Belgaumkar, Vinay wrote:
> > 
> > On 10/9/2025 12:56 PM, Rodrigo Vivi wrote:
> > > On Thu, Oct 09, 2025 at 11:38:54PM +0530, Sk Anirban wrote:
> > > > Replace cached pc->rpe_freq with xe_guc_pc_get_rpe_freq() call
> > > > to ensure
> > > > current RPe values are used when setting
> > > > SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY.
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5166
> > > > Signed-off-by: Sk Anirban <sk.anirban@intel.com>
> > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > 
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_guc_pc.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > index 3c0feb50a1e2..ea1ff96bec32 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > @@ -330,7 +330,7 @@ static int pc_set_min_freq(struct xe_guc_pc
> > > > *pc, u32 freq)
> > > >        * Our goal is to have the admin choices respected.
> > > >        */
> > > >       pc_action_set_param(pc, SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > > > -                freq < pc->rpe_freq);
> > > > +                freq < xe_guc_pc_get_rpe_freq(pc));
> > > I believe this is correct... But wondering if we should entirely
> > > kill the
> > > pc->rpe_freq ?!
> > > 
> > > Vinay, thoughts?
> > 
> > Makes sense. No point caching something that can change based on thermal
> > conditions.
> 
> Vinay,
> 
> Consider a scenario where the user increases the minimum frequency,
> prompting RPE to adjust accordingly. If the user later lowers the minimum
> frequency which will be below the RPE value, it will result in
> IGNORE_EFFICIENT_FREQUENCY being set permanently until the minimum frequency
> is raised again above RPE. Is it valid behavior or am I missing something?

This is the expected behavior and the reason for the flag.
Unless I'm misunderstanding your case here.

The thing is that if we don't set this flag, GuC will always oscilate
between the RPe and Max... If user is manually request a min that
is lower then RPe, then it wants the range min-max, not rpe-max.

What I cannot remember by heart is why we cannot just set that as a policy.
Perhaps it is because if we do set that, we lose power savings.

Also I can't remember why we don't set when user select a min > rpe.
Perhaps we do that to have a way to unset this without having to
introduce a separate file?!

But what I can remember is that this patch here needs to pass the
igt/tests/xe_gt_freq cleanly. That is our API contract for the
freq management.

I hope this is the case and that it was checked.

Thanks,
Rodrigo.

> 
> e.g.
> 
>    min  =  100  ign_rpe = OFF
>    min  =  200, rpe adjusted to 200+-y%, ign_rpe=OFF
>    min  = 100  which is < rpe , ign_rpe = ON
> 
> Thanks,
> Badal
> 
> > 
> > Thanks,
> > 
> > Vinay.
> > 
> > > 
> > > >         return pc_action_set_param(pc,
> > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> > > > -- 
> > > > 2.43.0
> > > >