[Intel-gfx] [PATCH] drm/i915/pmu: Don't grab wakeref when enabling events

[Tvrtko Ursulin]

On 18/01/2021 10:07, Chris Wilson wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Chris found a CI report which points out calling intel_runtime_pm_get from
> inside i915_pmu_enable hook is not allowed since it can be invoked from
> hard irq context. This is something we knew but forgot, so lets fix it
> once again.
> 
> We do this by syncing the internal book keeping with hardware rc6 counter
> on driver load.
> 
> v2:
>   * Always sync on parking and fully sync on init.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Fixes: f4e9894b6952 ("drm/i915/pmu: Correct the rc6 offset upon enabling")
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> Link: https://patchwork.freedesktop.org/patch/msgid/20201214094349.3563876-1-tvrtko.ursulin@linux.intel.com
> (cherry picked from commit dbe13ae1d6abaab417edf3c37601c6a56594a4cd)
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 30 ++++++++++++++++--------------
>   1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index d76685ce0399..9856479b56d8 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -184,13 +184,24 @@ static u64 get_rc6(struct intel_gt *gt)
>   	return val;
>   }
>   
> -static void park_rc6(struct drm_i915_private *i915)
> +static void init_rc6(struct i915_pmu *pmu)
>   {
> -	struct i915_pmu *pmu = &i915->pmu;
> +	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> +	intel_wakeref_t wakeref;
>   
> -	if (pmu->enable & config_enabled_mask(I915_PMU_RC6_RESIDENCY))
> +	with_intel_runtime_pm(i915->gt.uncore->rpm, wakeref) {
>   		pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(&i915->gt);
> +		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur =
> +					pmu->sample[__I915_SAMPLE_RC6].cur;
> +		pmu->sleep_last = ktime_get();
> +	}
> +}
>   
> +static void park_rc6(struct drm_i915_private *i915)
> +{
> +	struct i915_pmu *pmu = &i915->pmu;
> +
> +	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(&i915->gt);
>   	pmu->sleep_last = ktime_get();
>   }
>   
> @@ -201,6 +212,7 @@ static u64 get_rc6(struct intel_gt *gt)
>   	return __get_rc6(gt);
>   }
>   
> +static void init_rc6(struct i915_pmu *pmu) { }
>   static void park_rc6(struct drm_i915_private *i915) {}
>   
>   #endif
> @@ -612,10 +624,8 @@ static void i915_pmu_enable(struct perf_event *event)
>   		container_of(event->pmu, typeof(*i915), pmu.base);
>   	unsigned int bit = event_enabled_bit(event);
>   	struct i915_pmu *pmu = &i915->pmu;
> -	intel_wakeref_t wakeref;
>   	unsigned long flags;
>   
> -	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>   	spin_lock_irqsave(&pmu->lock, flags);
>   
>   	/*
> @@ -626,13 +636,6 @@ static void i915_pmu_enable(struct perf_event *event)
>   	GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>   	GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>   
> -	if (pmu->enable_count[bit] == 0 &&
> -	    config_enabled_mask(I915_PMU_RC6_RESIDENCY) & BIT_ULL(bit)) {
> -		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur = 0;
> -		pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(&i915->gt);
> -		pmu->sleep_last = ktime_get();
> -	}
> -
>   	pmu->enable |= BIT_ULL(bit);
>   	pmu->enable_count[bit]++;
>   
> @@ -673,8 +676,6 @@ static void i915_pmu_enable(struct perf_event *event)
>   	 * an existing non-zero value.
>   	 */
>   	local64_set(&event->hw.prev_count, __i915_pmu_event_read(event));
> -
> -	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>   }
>   
>   static void i915_pmu_disable(struct perf_event *event)
> @@ -1130,6 +1131,7 @@ void i915_pmu_register(struct drm_i915_private *i915)
>   	hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>   	pmu->timer.function = i915_sample;
>   	pmu->cpuhp.cpu = -1;
> +	init_rc6(pmu);
>   
>   	if (!is_igp(i915)) {
>   		pmu->name = kasprintf(GFP_KERNEL,
> 

Looks 100% as the conflict resolution I was about to send out so thanks 
for doing the work.

Regards,

Tvrtko