[PATCH] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[PATCH] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Ashutosh Dixit]

pmu_needs_timer() keeps the timer running even when GT is parked,
ostensibly to sample requested/actual frequencies. However
frequency_sample() has the following:

	/* Report 0/0 (actual/requested) frequency while parked. */
	if (!intel_gt_pm_get_if_awake(gt))
		return;

The above code prevents frequencies to be sampled while the GT is
parked. So we might as well turn off the sampling timer itself in this
case and save CPU cycles/power.

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

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d956..8db1d681cf4ab 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -124,11 +124,14 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
 		  ENGINE_SAMPLE_MASK;
 
 	/*
-	 * When the GPU is idle per-engine counters do not need to be
-	 * running so clear those bits out.
+	 * When GPU is idle, frequency or per-engine counters do not need
+	 * to be running so clear those bits out.
 	 */
-	if (!gpu_active)
-		enable &= ~ENGINE_SAMPLE_MASK;
+	if (!gpu_active) {
+		enable &= ~(config_mask(I915_PMU_ACTUAL_FREQUENCY) |
+			    config_mask(I915_PMU_REQUESTED_FREQUENCY) |
+			    ENGINE_SAMPLE_MASK);
+	}
 	/*
 	 * Also there is software busyness tracking available we do not
 	 * need the timer for I915_SAMPLE_BUSY counter.
-- 
2.38.0

Re: [PATCH] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Tvrtko Ursulin]

On 12/05/2023 02:53, Ashutosh Dixit wrote:
> pmu_needs_timer() keeps the timer running even when GT is parked,
> ostensibly to sample requested/actual frequencies. However
> frequency_sample() has the following:
> 
> 	/* Report 0/0 (actual/requested) frequency while parked. */
> 	if (!intel_gt_pm_get_if_awake(gt))
> 		return;
> 
> The above code prevents frequencies to be sampled while the GT is
> parked. So we might as well turn off the sampling timer itself in this
> case and save CPU cycles/power.

The confusing situation seems to be the consequence of b66ecd0438bf ("drm/i915/pmu: Report frequency as zero while GPU is sleeping").

Before that commit we were deliberately sampling the frequencies as GPU minimum during the parked periods and to do so leaving the timer running.

But then some RPS changes exposed that approach as questionable (AFAIR software tracked state stopped being reset to min freq and so created wild PMU readings) and we went the route of reporting zero when parked.

At which point running the timer stopped making sense, so really that commit should/could have made the change you now propose.

> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 7ece883a7d956..8db1d681cf4ab 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -124,11 +124,14 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   		  ENGINE_SAMPLE_MASK;
>   
>   	/*
> -	 * When the GPU is idle per-engine counters do not need to be
> -	 * running so clear those bits out.
> +	 * When GPU is idle, frequency or per-engine counters do not need
> +	 * to be running so clear those bits out.
>   	 */
> -	if (!gpu_active)
> -		enable &= ~ENGINE_SAMPLE_MASK;
> +	if (!gpu_active) {
> +		enable &= ~(config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> +			    config_mask(I915_PMU_REQUESTED_FREQUENCY) |
> +			    ENGINE_SAMPLE_MASK);
> +	}
>   	/*
>   	 * Also there is software busyness tracking available we do not
>   	 * need the timer for I915_SAMPLE_BUSY counter.

LGTM.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Or maybe it is possible to simplify since it looks there is no way to return true if gt is parked. So that could be:

pmu_needs_timer(..)
{
	...

	if (!gpu_active)
		return false;

	...
	enable = pmu->enable;

	...
	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
		  ENGINE_SAMPLE_MASK;

	...
	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
		enable &= ~BIT(I915_SAMPLE_BUSY);

	return enable;
}

Not sure it is any better, your call.

Regards,

Tvrtko

Re: [PATCH] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Dixit, Ashutosh]

On Fri, 12 May 2023 01:59:08 -0700, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 12/05/2023 02:53, Ashutosh Dixit wrote:
> > pmu_needs_timer() keeps the timer running even when GT is parked,
> > ostensibly to sample requested/actual frequencies. However
> > frequency_sample() has the following:
> >
> >	/* Report 0/0 (actual/requested) frequency while parked. */
> >	if (!intel_gt_pm_get_if_awake(gt))
> >		return;
> >
> > The above code prevents frequencies to be sampled while the GT is
> > parked. So we might as well turn off the sampling timer itself in this
> > case and save CPU cycles/power.
>
> The confusing situation seems to be the consequence of b66ecd0438bf
> ("drm/i915/pmu: Report frequency as zero while GPU is sleeping").
>
> Before that commit we were deliberately sampling the frequencies as GPU
> minimum during the parked periods and to do so leaving the timer running.
>
> But then some RPS changes exposed that approach as questionable (AFAIR
> software tracked state stopped being reset to min freq and so created
> wild PMU readings) and we went the route of reporting zero when parked.
>
> At which point running the timer stopped making sense, so really that
> commit should/could have made the change you now propose.
>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index 7ece883a7d956..8db1d681cf4ab 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -124,11 +124,14 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >		  ENGINE_SAMPLE_MASK;
> >		/*
> > -	 * When the GPU is idle per-engine counters do not need to be
> > -	 * running so clear those bits out.
> > +	 * When GPU is idle, frequency or per-engine counters do not need
> > +	 * to be running so clear those bits out.
> >	 */
> > -	if (!gpu_active)
> > -		enable &= ~ENGINE_SAMPLE_MASK;
> > +	if (!gpu_active) {
> > +		enable &= ~(config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> > +			    config_mask(I915_PMU_REQUESTED_FREQUENCY) |
> > +			    ENGINE_SAMPLE_MASK);
> > +	}
> >	/*
> >	 * Also there is software busyness tracking available we do not
> >	 * need the timer for I915_SAMPLE_BUSY counter.
>
> LGTM.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Or maybe it is possible to simplify since it looks there is no way to return true if gt is parked. So that could be:
>
> pmu_needs_timer(..)
> {
>	...
>
>	if (!gpu_active)
>		return false;
>
>	...
>	enable = pmu->enable;
>
>	...
>	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
>		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
>		  ENGINE_SAMPLE_MASK;
>
>	...
>	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
>		enable &= ~BIT(I915_SAMPLE_BUSY);
>
>	return enable;
> }
>
> Not sure it is any better, your call.

I have made this change in v2 of the patch submitted here:

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

And I have retained your R-b since the patch is essentially a copy of your
code above.

The original version of the patch was here:

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

Thanks.
--
Ashutosh