Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Remove fallback to requested freq for SLPC

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

On Wed, 15 Mar 2023 02:50:17 -0700, Tvrtko Ursulin wrote:
>
> On 10/03/2023 00:59, Ashutosh Dixit wrote:
> > The fallback to requested freq does not work for SLPC because SLPC does not
> > use 'struct intel_rps'. Also for SLPC requested freq can only be obtained
> > from a hw register after acquiring forcewake which we don't want to do for
> > PMU. Therefore remove fallback to requested freq for SLPC. The actual freq
> > will be 0 when gt is in RC6 which is correct. Also this is rare since PMU
> > freq sampling happens only when gt is unparked.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index 7ece883a7d95..f697fabed64a 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -393,7 +393,14 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >		 * frequency. Fortunately, the read should rarely fail!
> >		 */
> >		val = intel_rps_read_actual_frequency_fw(rps);
> > -		if (!val)
> > +
> > +		/*
> > +		 * SLPC does not use 'struct intel_rps'. Also for SLPC
> > +		 * requested freq can only be obtained after acquiring
> > +		 * forcewake and reading a hw register. For SLPC just
> > +		 * let val be 0
> > +		 */
> > +		if (!val && !intel_uc_uses_guc_slpc(&gt->uc))
> >			val = intel_gpu_freq(rps, rps->cur_freq);
>
> I really dislike sprinkling of "uses slpc" since I think the thing hasn't
> really been integrated nicely. Case in point is probably the flow duality
> in intel_rps_boost. Data structures as well, even though some fields and
> concepts are shared.
>
> For instance why we can't have the notion of software tracked cur_freq in
> rps, and/or have it zero if with SLPC we can't have it otherwise?

For SLPC:

* We can't have the notion of software tracked cur_freq in rps because FW is
  managing the freq.
* rps->cur_freq /is/ actually 0 since SLPC does not use 'struct
  intel_rps'. So this patch doesn't really make any practical difference,
  PMU values will be exactly the same with or without this patch. It was
  just clarifying things.

> I will abstain, sorry.

I will drop this patch, there doesn't seem much point in it.

Thanks.
--
Ashutosh