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 24.05.2023 23:56, 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)
> v3: Remove gpu_active argument of pmu_needs_timer (Andrzej)
>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index a814583e19fd7..09313cf9316b4 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -139,7 +139,7 @@ static u32 frequency_enabled_mask(void)
>   	return mask;
>   }
>   
> -static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> +static bool pmu_needs_timer(struct i915_pmu *pmu)
>   {
>   	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>   	u32 enable;
> @@ -157,17 +157,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);
>   
>   	/*
> @@ -295,7 +289,7 @@ static void park_rc6(struct intel_gt *gt)
>   
>   static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
>   {
> -	if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
> +	if (!pmu->timer_enabled && pmu_needs_timer(pmu)) {
>   		pmu->timer_enabled = true;
>   		pmu->timer_last = ktime_get();
>   		hrtimer_start_range_ns(&pmu->timer,
> @@ -321,7 +315,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>   	 */
>   	pmu->unparked &= ~BIT(gt->info.id);
>   	if (pmu->unparked == 0)
> -		pmu->timer_enabled = pmu_needs_timer(pmu, false);
> +		pmu->timer_enabled = false;
>   
>   	spin_unlock_irq(&pmu->lock);
>   }
> @@ -827,7 +821,7 @@ static void i915_pmu_disable(struct perf_event *event)
>   	 */
>   	if (--pmu->enable_count[bit] == 0) {
>   		pmu->enable &= ~BIT(bit);
> -		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
> +		pmu->timer_enabled &= pmu_needs_timer(pmu);
>   	}
>   
>   	spin_unlock_irqrestore(&pmu->lock, flags);