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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/09/2022 17:11, Dixit, Ashutosh wrote:
> 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).

We already report zero when GT is idle (as considered by software 
tracking) so I guess the only part you'd like to change is drop the else 
branch in:

		val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1);
		if (val)
			val = intel_rps_get_cagf(rps, val);
		else
			val = rps->cur_freq;

?

What would be pros and cons?

Regards,

Tvrtko