Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Use only freq bits for falling back to requested freq

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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