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 = >->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
> >>
