| Message ID | 20230304012705.70003-2-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pmu: Freq sampling: Fix requested freq fallback | expand |
On 04/03/2023 01:27, Ashutosh Dixit wrote: > On newer generations, the GEN12_RPSTAT1 register contains more than freq > information, e.g. see GEN12_VOLTAGE_MASK. Therefore use only the freq bits > to decide whether to fall back to requested freq. Could you find an appropriate Fixes: tag please? If it can affects a platform out of force probe then cc: stable to. CI is not catching the problem? > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_pmu.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 52531ab28c5f..f0a1e36915b8 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -393,10 +393,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) > * case we assume the system is running at the intended > * frequency. Fortunately, the read should rarely fail! > */ > - val = intel_rps_read_rpstat_fw(rps); > - if (val) > - val = intel_rps_get_cagf(rps, val); > - else > + val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps)); Will this work with gen5_invert_freq as called by intel_rps_get_cagf? Regards, Tvrtko > + if (!val) > val = rps->cur_freq; > > add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
On Mon, 06 Mar 2023 03:04:40 -0800, Tvrtko Ursulin wrote: > Hi Tvrtko, > On 04/03/2023 01:27, Ashutosh Dixit wrote: > > On newer generations, the GEN12_RPSTAT1 register contains more than freq > > information, e.g. see GEN12_VOLTAGE_MASK. Therefore use only the freq bits > > to decide whether to fall back to requested freq. > > CI is not catching the problem? This is because as we know PMU freq sampling happens only when gt is unparked (actively processing requests) so it is highly unlikely that gt will be in rc6 when it might have to fall back to requested freq (I checked this and it seems it is only at the end of the workload that we see it entering the fallback code path). Deleting the fallback path completely will not make much difference to the output and is an option too. Anyway I have retained it for now. > Could you find an appropriate Fixes: tag please? If it can affects a > platform out of force probe then cc: stable to. Cc stable is anyway not needed because affected platforms (DG1 onwards) are under force probe. Also because the issue does not affect real metrics (as mentioned above) as well as because it is a really a missing patch rather than a broken previous patch I am skipping the Fixes tag. > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_pmu.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > > index 52531ab28c5f..f0a1e36915b8 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -393,10 +393,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) > > * case we assume the system is running at the intended > > * frequency. Fortunately, the read should rarely fail! > > */ > > - val = intel_rps_read_rpstat_fw(rps); > > - if (val) > > - val = intel_rps_get_cagf(rps, val); > > - else > > + val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps)); > > Will this work with gen5_invert_freq as called by intel_rps_get_cagf? PMU has ever only supported Gen6+. See intel_rps_read_rpstat_fw (Gen5 does not have a GEN6_RPSTAT1 register) as well as 01b8c2e60e96. More importantly PMU was missing support for MTL. It is to avoid these kinds of issues I have submitted a new series with a different approach which should now take care of both MTL+ as well as Gen5-: https://patchwork.freedesktop.org/series/114814/ > > + if (!val) > > val = rps->cur_freq; > > add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT], Thanks. -- Ashutosh
On 08/03/2023 05:36, Dixit, Ashutosh wrote: > On Mon, 06 Mar 2023 03:04:40 -0800, Tvrtko Ursulin wrote: >> > > Hi Tvrtko, > >> On 04/03/2023 01:27, Ashutosh Dixit wrote: >>> On newer generations, the GEN12_RPSTAT1 register contains more than freq >>> information, e.g. see GEN12_VOLTAGE_MASK. Therefore use only the freq bits >>> to decide whether to fall back to requested freq. >> > >> CI is not catching the problem? > > This is because as we know PMU freq sampling happens only when gt is > unparked (actively processing requests) so it is highly unlikely that gt > will be in rc6 when it might have to fall back to requested freq (I checked > this and it seems it is only at the end of the workload that we see it > entering the fallback code path). Deleting the fallback path completely > will not make much difference to the output and is an option too. Anyway I > have retained it for now. Ah got it now, it is about false positive and not the garbage bits fed in as I initially misunderstood. >> Could you find an appropriate Fixes: tag please? If it can affects a >> platform out of force probe then cc: stable to. > > Cc stable is anyway not needed because affected platforms (DG1 onwards) are > under force probe. Also because the issue does not affect real metrics (as > mentioned above) as well as because it is a really a missing patch rather > than a broken previous patch I am skipping the Fixes tag. "DG1 onwards" - DG2? Should have at least Fixes: if so. >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_pmu.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >>> index 52531ab28c5f..f0a1e36915b8 100644 >>> --- a/drivers/gpu/drm/i915/i915_pmu.c >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c >>> @@ -393,10 +393,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) >>> * case we assume the system is running at the intended >>> * frequency. Fortunately, the read should rarely fail! >>> */ >>> - val = intel_rps_read_rpstat_fw(rps); >>> - if (val) >>> - val = intel_rps_get_cagf(rps, val); >>> - else >>> + val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps)); >> >> Will this work with gen5_invert_freq as called by intel_rps_get_cagf? > > PMU has ever only supported Gen6+. See intel_rps_read_rpstat_fw (Gen5 does > not have a GEN6_RPSTAT1 register) as well as 01b8c2e60e96. PMU _frequency_ not before Gen6, okay, I forgot about that. Regards, Tvrtko > More importantly PMU was missing support for MTL. It is to avoid these > kinds of issues I have submitted a new series with a different approach > which should now take care of both MTL+ as well as Gen5-: > > https://patchwork.freedesktop.org/series/114814/ > >>> + if (!val) >>> val = rps->cur_freq; >>> add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT], > > Thanks. > -- > Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 52531ab28c5f..f0a1e36915b8 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -393,10 +393,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * case we assume the system is running at the intended * frequency. Fortunately, the read should rarely fail! */ - val = intel_rps_read_rpstat_fw(rps); - if (val) - val = intel_rps_get_cagf(rps, val); - else + val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps)); + if (!val) val = rps->cur_freq; add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
On newer generations, the GEN12_RPSTAT1 register contains more than freq information, e.g. see GEN12_VOLTAGE_MASK. Therefore use only the freq bits to decide whether to fall back to requested freq. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_pmu.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)