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

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

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?

Regards,

Tvrtko