Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Use only freq bits for falling back to requested freq

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

On 08/03/2023 05:36, Dixit, Ashutosh wrote:
> On Mon, 06 Mar 2023 03:04:40 -0800, Tvrtko Ursulin wrote:
>>
> 
> Hi Tvrtko,
> 
>> On 04/03/2023 01:27, Ashutosh Dixit wrote:
>>> On newer generations, the GEN12_RPSTAT1 register contains more than freq
>>> information, e.g. see GEN12_VOLTAGE_MASK. Therefore use only the freq bits
>>> to decide whether to fall back to requested freq.
>>
> 
>> CI is not catching the problem?
> 
> This is because as we know PMU freq sampling happens only when gt is
> unparked (actively processing requests) so it is highly unlikely that gt
> will be in rc6 when it might have to fall back to requested freq (I checked
> this and it seems it is only at the end of the workload that we see it
> entering the fallback code path). Deleting the fallback path completely
> will not make much difference to the output and is an option too. Anyway I
> have retained it for now.

Ah got it now, it is about false positive and not the garbage bits fed 
in as I initially misunderstood.

>> Could you find an appropriate Fixes: tag please? If it can affects a
>> platform out of force probe then cc: stable to.
> 
> Cc stable is anyway not needed because affected platforms (DG1 onwards) are
> under force probe. Also because the issue does not affect real metrics (as
> mentioned above) as well as because it is a really a missing patch rather
> than a broken previous patch I am skipping the Fixes tag.

"DG1 onwards" - DG2? Should have at least Fixes: if so.

>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 6 ++----
>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index 52531ab28c5f..f0a1e36915b8 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -393,10 +393,8 @@ 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_rps_read_rpstat_fw(rps);
>>> -		if (val)
>>> -			val = intel_rps_get_cagf(rps, val);
>>> -		else
>>> +		val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps));
>>
>> Will this work with gen5_invert_freq as called by intel_rps_get_cagf?
> 
> PMU has ever only supported Gen6+. See intel_rps_read_rpstat_fw (Gen5 does
> not have a GEN6_RPSTAT1 register) as well as 01b8c2e60e96.

PMU _frequency_ not before Gen6, okay, I forgot about that.

Regards,

Tvrtko

> More importantly PMU was missing support for MTL. It is to avoid these
> kinds of issues I have submitted a new series with a different approach
> which should now take care of both MTL+ as well as Gen5-:
> 
> https://patchwork.freedesktop.org/series/114814/
> 
>>> +		if (!val)
>>> 			val = rps->cur_freq;
>>> 			add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
> 
> Thanks.
> --
> Ashutosh