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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 11-10-2025 01:36, Rodrigo Vivi wrote:
> 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.

Agree with this, but the e.g. I mentioned below, with that rpe will be 
ignored permanently.
Potentially unintentionally by someone unfamiliar with how rpe works. 
And at some point of time
rpe will be settled near min but will remain ignored.

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

Agree this needed to pass igt cleanly. I am not blocking this.

Thanks,
Badal

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