Re: [PATCH v2] drm/i915/selftests: Correct frequency handling in RPS power measurement

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

On 13-01-2025 12:38, Riana Tauro wrote:
> Hi Anirban
>
> On 1/10/2025 8:42 PM, Anirban, Sk wrote:
>>
>>
>>
>> On 09-01-2025 16:45, Nilawar, Badal wrote:
>>>
>>>
>>> On 09-01-2025 15:50, Nilawar, Badal wrote:
>>>>
>>>>
>>>> On 09-01-2025 15:00, sk.anirban@intel.com wrote:
>>>>> From: Sk Anirban<sk.anirban@intel.com>
>>>>>
>>>>> Fix the frequency calculation by ensuring it is adjusted
>>>>> only once during power measurement. Update live_rps_power test
>>>>> to use the correct frequency values for logging and comparison.
>>>>>
>>>>> v2:
>>>>>    - Improved frequency logging (Riana)
>>>>>
>>>>> Signed-off-by: Sk Anirban<sk.anirban@intel.com>
>>>>> Reviewed-by: Riana Tauro<riana.tauro@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/gt/selftest_rps.c | 11 ++++++-----
>>>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/ 
>>>>> drm/i915/gt/selftest_rps.c
>>>>> index c207a4fb03bf..e515d7eb628a 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_rps.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
>>>>> @@ -1126,6 +1126,7 @@ static u64 measure_power_at(struct intel_rps 
>>>>> *rps, int *freq)
>>>>>   {
>>>>>       *freq = rps_set_check(rps, *freq);
>>>>>       msleep(100);
>>>>> +    *freq = intel_gpu_freq(rps, *freq);
>>>> I am seeingrps_set_check will wait till act freq become desired 
>>>> freq, in case of timeout act freq could be different. I think it 
>>>> would be good to check freq returned by rps_set_check is expected 
>>>> freq if not then read freq again after msleep.
>>>
>>> Please ignore above comments, I got your code. You are applying freq 
>>> multiplier before passing to measure_power. While this approach 
>>> works fine, I recommend fixing measure_power() by using read_cagf() 
>>> instead of intel_rps_read_actual_frequency().
>>> Add Fixes: ac4e8560248f ("drm/i915/selftests: Add helper function 
>>> measure_power") in commit message.
>>>
>>> Regards,
>>> Badal
>>>
>> The measure_power() function is being used by slpc also, as slpc is 
>> not passing the raw frequency it may cause issue. So the plan is to 
>> create independent function to measure power for slpc, and for rps I 
>> will be using read_cagf() to calculate the avg.
>
> Are you planning to re-work this patch? In that case use read_cagf for 
> SLPC as well, wouldn't that work?
Yes, I'm modifying the patch to use read_cagf for rps and for slpc I am 
using an independent function to measure power using 
intel_rps_read_actual_frequency and that will internally call read_cagf.
>>
>> Regards,
>> Anirban
>>>
>>>> Regards, Badal
>>>>
>>>>>       return measure_power(rps, freq);
>>>>>   }
>>>>> @@ -1202,13 +1203,13 @@ int live_rps_power(void *arg)
>>>>>           pr_info("%s: min:%llumW @ %uMHz, max:%llumW @ %uMHz\n",
>>>>>               engine->name,
>>>>> -            min.power, intel_gpu_freq(rps, min.freq),
>>>>> -            max.power, intel_gpu_freq(rps, max.freq));
>>>>> +            min.power, min.freq,
>>>>> +            max.power, max.freq);
>>>>>           if (10 * min.freq >= 9 * max.freq) {
>>>>> -            pr_notice("Could not control frequency, ran at [%d: 
>>>>> %uMHz, %d:%uMhz]\n",
>>>>> -                  min.freq, intel_gpu_freq(rps, min.freq),
>>>>> -                  max.freq, intel_gpu_freq(rps, max.freq));
>>>>> +            pr_notice("Could not control frequency, ran at 
>>>>> [%uMHz, %uMhz]\n",
>>>>> +                  min.freq,
>>>>> +                  max.freq);
>>>>>               continue;
>>>>>           }
>>
>