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

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

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?

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.

Thanks.
--
Ashutosh

> >>		if (val)
> >>			val = intel_rps_get_cagf(rps, val);
> >>		else
> >> --
> >> 2.25.1
> >>