[PATCH 0/2] drm/i915/pmu: Freq sampling: Fix requested freq fallback

[PATCH 0/2] drm/i915/pmu: Freq sampling: Fix requested freq fallback

[Ashutosh Dixit]

A couple of minor fixes to the PMU requested freq fallback for PMU freq
sampling.

Ashutosh Dixit (2):
  drm/i915/pmu: Use only freq bits for falling back to requested freq
  drm/i915/pmu: Use correct requested freq for SLPC

 drivers/gpu/drm/i915/i915_pmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.38.0

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

[Ashutosh Dixit]

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.

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));
+		if (!val)
 			val = rps->cur_freq;
 
 		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
-- 
2.38.0

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

[Tvrtko Ursulin]

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.

Could you find an appropriate Fixes: tag please? If it can affects a 
platform out of force probe then cc: stable to.

CI is not catching the problem?

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

Regards,

Tvrtko

> +		if (!val)
>   			val = rps->cur_freq;
>   
>   		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],

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

[Dixit, Ashutosh]

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.

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

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

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

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

[Tvrtko Ursulin]

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

[PATCH 2/2] drm/i915/pmu: Use correct requested freq for SLPC

[Ashutosh Dixit]

SLPC does not use 'struct intel_rps'. Use UNSLICE_RATIO bits from
GEN6_RPNSWREQ for SLPC. See intel_rps_get_requested_frequency.

Bspec: 52745

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index f0a1e36915b8..5ee836610801 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -394,8 +394,13 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 		 * frequency. Fortunately, the read should rarely fail!
 		 */
 		val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps));
-		if (!val)
-			val = rps->cur_freq;
+		if (!val) {
+			if (intel_uc_uses_guc_slpc(&gt->uc))
+				val = intel_rps_read_punit_req(rps) >>
+						GEN9_SW_REQ_UNSLICE_RATIO_SHIFT;
+			else
+				val = rps->cur_freq;
+		}
 
 		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
 				intel_gpu_freq(rps, val), period_ns / 1000);
-- 
2.38.0

Re: [PATCH 2/2] drm/i915/pmu: Use correct requested freq for SLPC

[Tvrtko Ursulin]

On 04/03/2023 01:27, Ashutosh Dixit wrote:
> SLPC does not use 'struct intel_rps'. Use UNSLICE_RATIO bits from

Would it be more accurate to say 'SLPC does not use rps->cur_freq' 
rather than it not using struct intel_rps?

Fixes: / stable ? CI chances of catching this?

> GEN6_RPNSWREQ for SLPC. See intel_rps_get_requested_frequency.
> 
> Bspec: 52745
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index f0a1e36915b8..5ee836610801 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -394,8 +394,13 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>   		 * frequency. Fortunately, the read should rarely fail!
>   		 */
>   		val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps));
> -		if (!val)
> -			val = rps->cur_freq;
> +		if (!val) {
> +			if (intel_uc_uses_guc_slpc(&gt->uc))
> +				val = intel_rps_read_punit_req(rps) >>
> +						GEN9_SW_REQ_UNSLICE_RATIO_SHIFT;
> +			else
> +				val = rps->cur_freq;
> +		}

That's a bunch of duplication from intel_rps.c so perhaps the 
appropriate helpers should be exported (some way) from there.

Regards,

Tvrtko

>   
>   		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
>   				intel_gpu_freq(rps, val), period_ns / 1000);

Re: [PATCH 2/2] drm/i915/pmu: Use correct requested freq for SLPC

[Dixit, Ashutosh]

On Mon, 06 Mar 2023 03:10:24 -0800, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 04/03/2023 01:27, Ashutosh Dixit wrote:
> > SLPC does not use 'struct intel_rps'. Use UNSLICE_RATIO bits from
>
> Would it be more accurate to say 'SLPC does not use rps->cur_freq' rather
> than it not using struct intel_rps?

No actually SLPC maintains a separate 'struct intel_guc_slpc' and does not
use 'struct intel_rps' at all so all of 'struct intel_rps' is 0.

> Fixes: / stable ? CI chances of catching this?

Same issue as Patch 1, I have answered this there.

> > GEN6_RPNSWREQ for SLPC. See intel_rps_get_requested_frequency.
> >
> > Bspec: 52745
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index f0a1e36915b8..5ee836610801 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -394,8 +394,13 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >		 * frequency. Fortunately, the read should rarely fail!
> >		 */
> >		val = intel_rps_get_cagf(rps, intel_rps_read_rpstat_fw(rps));
> > -		if (!val)
> > -			val = rps->cur_freq;
> > +		if (!val) {
> > +			if (intel_uc_uses_guc_slpc(&gt->uc))
> > +				val = intel_rps_read_punit_req(rps) >>
> > +						GEN9_SW_REQ_UNSLICE_RATIO_SHIFT;
> > +			else
> > +				val = rps->cur_freq;
> > +		}
>
> That's a bunch of duplication from intel_rps.c so perhaps the appropriate
> helpers should be exported (some way) from there.

This is also addressed in the new series:

https://patchwork.freedesktop.org/series/114814/

> >			add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
> >				intel_gpu_freq(rps, val), period_ns / 1000);

Thanks.
--
Ashutosh

Re: [PATCH 2/2] drm/i915/pmu: Use correct requested freq for SLPC

[Tvrtko Ursulin]

On 08/03/2023 05:36, Dixit, Ashutosh wrote:
> On Mon, 06 Mar 2023 03:10:24 -0800, Tvrtko Ursulin wrote:
>>
> 
> Hi Tvrtko,
> 
>> On 04/03/2023 01:27, Ashutosh Dixit wrote:
>>> SLPC does not use 'struct intel_rps'. Use UNSLICE_RATIO bits from
>>
>> Would it be more accurate to say 'SLPC does not use rps->cur_freq' rather
>> than it not using struct intel_rps?
> 
> No actually SLPC maintains a separate 'struct intel_guc_slpc' and does not
> use 'struct intel_rps' at all so all of 'struct intel_rps' is 0.

I keep forgetting how there is zero code / data sharing with all this.. :(

>> Fixes: / stable ? CI chances of catching this?
> 
> Same issue as Patch 1, I have answered this there.

Okay lets see it clarified there.

Regards,

Tvrtko