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

["Anirban, Sk" <sk.anirban@intel.com>]

Hi,

On 13-10-2025 11:14, Nilawar, Badal wrote:
>
> 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.
A new series has been sent to address this change,
https://patchwork.freedesktop.org/series/155809/ .

Thanks,
Anirban
>>>>
>>>>>>          return pc_action_set_param(pc,
>>>>>> SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>>>>>> -- 
>>>>>> 2.43.0
>>>>>>