Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register

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

On Wed, 14 Sep 2022 02:56:26 -0700, Nilawar, Badal wrote:
>
> On 13-09-2022 13:17, Tvrtko Ursulin wrote:
> >
> > On 13/09/2022 01:09, Dixit, Ashutosh wrote:
> >> On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote:
> >>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> index 958b37123bf1..a24704ec2c18 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> @@ -371,7 +371,6 @@ static void
> >>>>>    frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >>>>>    {
> >>>>>     struct drm_i915_private *i915 = gt->i915;
> >>>>> -    struct intel_uncore *uncore = gt->uncore;
> >>>>>     struct i915_pmu *pmu = &i915->pmu;
> >>>>>     struct intel_rps *rps = &gt->rps;
> >>>>>
> >>>>> @@ -394,7 +393,7 @@ 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_uncore_read_fw(uncore, GEN6_RPSTAT1);
> >>>>> +        val = intel_rps_read_rpstat(rps);
> >>>>
> >>>> Hmm, we got rid of _fw which the comment above refers to. Maybe we
> >>>> need a
> >>>> fw flag to intel_rps_read_rpstat?
> >>>
> >>> Above function before reading rpstat it checks if gt is awake.
> >>
> >> Ok, so you are referring to intel_gt_pm_get_if_awake check in
> >> frequency_sample.
> >>
> >>> So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with
> >>> forcewake.In that case we can remove above comment.  Let me know your
> >>> thoughts on this.
> >>
> >> I am not entirely sure about this. For example in c1c82d267ae8
> >> intel_uncore_read_fw was introduced with the same
> >> intel_gt_pm_get_if_awake
> >> check. So this would mean even if gt is awake not taking forcewake makes
> >> a
> >> difference. The same code pattern was retained in b66ecd0438bf. Maybe
> >> it's
> >> because there are no locks?
> >
> > Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the
> > actual frequency to avoid fw") explains the _fw variant is to avoid
> > preventing RC6, and so increased GPU power draw, just because someone has
> > PMU open. (Because of the 200Hz sampling timer that is needed for PMU
> > frequency reporting.)
> >
> >> Under the circumstances I think we could do one of two things:
> >> 1. If we want to drop _fw, we should do it as a separate patch with its
> >> own
> >>     justification so it can be reviewed separately.
> >> 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to
> >>     intel_rps_read_rpstat.
> >
> > Agreed. Or instead of the flag, the usual pattern of having
> > intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the
> > forcewake.
> >
> > Also, may I ask, this patch is in the MTL enablement series but the
> > commit message and patch content seem like it is fixing a wider Gen12
> > issue? What is the extent of incorrect behaviour without it? Should it be
> > tagged for stable for first Tigerlake supporting kernel?
>
> GEN6_RPSTAT1(0xa01c) and GEN12_RPSTAT1(0x1381b4) both are supported by
> gen12 and above. The difference between two is GEN6_RPSTAT1 falls under
> RENDER forcewake domain and GEN12_RPSTAT1 does not require forcewake to
> access. GEN12_RPSTAT1 is punit register and when GT is in RC6 it will give
> frequency as 0.

Correct, so no changes needed for stable kernels. But going forward Badal
is proposing (which I sort of agree with but may need some discussion) that
we change i915 behavior to return 0 freq (instead of cur_freq or RPn) when
GT is idle or in RC6 (so we don't take forcewake to read freq when GT is in
RC6).

> Reason for clubbing this patch with MTL series is due to common function
> intel_rps_read_rpstat. I think I should send this patch in separate series.

Agree!

Thanks.
--
Ashutosh