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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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.

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.

Regards,
Badal
> 
> Regards,
> 
> Tvrtko