| Message ID | 20250109093010.3879245-1-sk.anirban@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/selftests: Correct frequency handling in RPS power measurement | expand |
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. 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; > } >
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 > 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; >> } >>
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. 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; >>> } >>>
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? > > 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; >>>> } >
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; >>>>> } >> >
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); 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; }