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

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 23.05.2023 17:19, 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.
> 
> v2: Instead of turning freq bits off, return false, since no counters will
>      run after this change when GT is parked (Tvrtko)
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index a814583e19fd7..b47d890d4ada1 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>   	u32 enable;
>   
> +	/* When GPU is idle, at present no counters need to run */
> +	if (!gpu_active)
> +		return false;
> +

What is then purpose of calling pmu_needs_timer with 2nd arg false?
Why not just replace all occurrences of pmu_needs_timer(.., false) with 
false? And remove the 2nd argument.

Regards
Andrzej



>   	/*
>   	 * Only some counters need the sampling timer.
>   	 *
> @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   	 */
>   	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
>   
> -	/*
> -	 * When the GPU is idle per-engine counters do not need to be
> -	 * running so clear those bits out.
> -	 */
> -	if (!gpu_active)
> -		enable &= ~ENGINE_SAMPLE_MASK;
>   	/*
>   	 * Also there is software busyness tracking available we do not
>   	 * need the timer for I915_SAMPLE_BUSY counter.
>   	 */
> -	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> +	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
>   		enable &= ~BIT(I915_SAMPLE_BUSY);
>   
>   	/*