diff mbox series

drm/i915/pmu: Don't grab wakeref when enabling events

Message ID 20210118100724.465555-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series drm/i915/pmu: Don't grab wakeref when enabling events | expand

Commit Message

Chris Wilson Jan. 18, 2021, 10:07 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Fixes: f4e9894b6952 ("drm/i915/pmu: Correct the rc6 offset upon enabling")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@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(-)

Comments

Tvrtko Ursulin Jan. 18, 2021, 10:14 a.m. UTC | #1 
On 18/01/2021 10:07, Chris Wilson wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Fixes: f4e9894b6952 ("drm/i915/pmu: Correct the rc6 offset upon enabling")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@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
diff mbox series

Patch

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,