[Intel-gfx] [PATCH 0/6] Add MTL PMU support for multi-gt

[Intel-gfx] [PATCH 0/6] Add MTL PMU support for multi-gt

[Umesh Nerlige Ramappa]

With MTL, frequency and rc6 counters are specific to a gt. Export these
counters via gt-specific events to the user space.

v2: Review comments (Ashutosh, Tvrtko)

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Tvrtko Ursulin (6):
  drm/i915/pmu: Support PMU for all engines
  drm/i915/pmu: Skip sampling engines with no enabled counters
  drm/i915/pmu: Transform PMU parking code to be GT based
  drm/i915/pmu: Add reference counting to the sampling timer
  drm/i915/pmu: Prepare for multi-tile non-engine counters
  drm/i915/pmu: Export counters from all tiles

 drivers/gpu/drm/i915/gt/intel_gt_pm.c |   4 +-
 drivers/gpu/drm/i915/i915_pmu.c       | 263 ++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_pmu.h       |  24 ++-
 include/uapi/drm/i915_drm.h           |  17 +-
 4 files changed, 219 insertions(+), 89 deletions(-)

-- 
2.36.1

[Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
 #include "gt/intel_engine_pm.h"
 #include "gt/intel_engine_regs.h"
 #include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 	struct drm_i915_private *i915 =
 		container_of(hrtimer, struct drm_i915_private, pmu.timer);
 	struct i915_pmu *pmu = &i915->pmu;
-	struct intel_gt *gt = to_gt(i915);
 	unsigned int period_ns;
+	struct intel_gt *gt;
+	unsigned int i;
 	ktime_t now;
 
 	if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 	 * grabbing the forcewake. However the potential error from timer call-
 	 * back delay greatly dominates this so we keep it simple.
 	 */
-	engines_sample(gt, period_ns);
-	frequency_sample(gt, period_ns);
+
+	for_each_gt(gt, i915, i) {
+		engines_sample(gt, period_ns);
+
+		if (i == 0) /* FIXME */
+			frequency_sample(gt, period_ns);
+	}
 
 	hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
 
-- 
2.36.1

[Intel-gfx] [PATCH 2/6] drm/i915/pmu: Skip sampling engines with no enabled counters

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

As we have more and more engines do not waste time sampling the ones no-
one is monitoring.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 67fa6cd77529..ba769f7fc385 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -339,6 +339,9 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
 		return;
 
 	for_each_engine(engine, gt, id) {
+		if (!engine->pmu.enable)
+			continue;
+
 		if (!intel_engine_pm_get_if_awake(engine))
 			continue;
 
-- 
2.36.1

[Intel-gfx] [PATCH 3/6] drm/i915/pmu: Transform PMU parking code to be GT based

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Trivial prep work for full multi-tile enablement later.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.c |  4 ++--
 drivers/gpu/drm/i915/i915_pmu.c       | 16 ++++++++--------
 drivers/gpu/drm/i915/i915_pmu.h       |  9 +++++----
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index e02cb90723ae..c2e69bafd02b 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -87,7 +87,7 @@ static int __gt_unpark(struct intel_wakeref *wf)
 
 	intel_rc6_unpark(&gt->rc6);
 	intel_rps_unpark(&gt->rps);
-	i915_pmu_gt_unparked(i915);
+	i915_pmu_gt_unparked(gt);
 	intel_guc_busyness_unpark(gt);
 
 	intel_gt_unpark_requests(gt);
@@ -109,7 +109,7 @@ static int __gt_park(struct intel_wakeref *wf)
 
 	intel_guc_busyness_park(gt);
 	i915_vma_parked(gt);
-	i915_pmu_gt_parked(i915);
+	i915_pmu_gt_parked(gt);
 	intel_rps_park(&gt->rps);
 	intel_rc6_park(&gt->rc6);
 
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index ba769f7fc385..2b63ee31e1b3 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -217,11 +217,11 @@ static void init_rc6(struct i915_pmu *pmu)
 	}
 }
 
-static void park_rc6(struct drm_i915_private *i915)
+static void park_rc6(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu = &gt->i915->pmu;
 
-	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(to_gt(i915));
+	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(gt);
 	pmu->sleep_last = ktime_get_raw();
 }
 
@@ -236,16 +236,16 @@ static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
 	}
 }
 
-void i915_pmu_gt_parked(struct drm_i915_private *i915)
+void i915_pmu_gt_parked(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu = &gt->i915->pmu;
 
 	if (!pmu->base.event_init)
 		return;
 
 	spin_lock_irq(&pmu->lock);
 
-	park_rc6(i915);
+	park_rc6(gt);
 
 	/*
 	 * Signal sampling timer to stop if only engine events are enabled and
@@ -256,9 +256,9 @@ void i915_pmu_gt_parked(struct drm_i915_private *i915)
 	spin_unlock_irq(&pmu->lock);
 }
 
-void i915_pmu_gt_unparked(struct drm_i915_private *i915)
+void i915_pmu_gt_unparked(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu = &gt->i915->pmu;
 
 	if (!pmu->base.event_init)
 		return;
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index c30f43319a78..a686fd7ccedf 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -13,6 +13,7 @@
 #include <uapi/drm/i915_drm.h>
 
 struct drm_i915_private;
+struct intel_gt;
 
 /*
  * Non-engine events that we need to track enabled-disabled transition and
@@ -151,15 +152,15 @@ int i915_pmu_init(void);
 void i915_pmu_exit(void);
 void i915_pmu_register(struct drm_i915_private *i915);
 void i915_pmu_unregister(struct drm_i915_private *i915);
-void i915_pmu_gt_parked(struct drm_i915_private *i915);
-void i915_pmu_gt_unparked(struct drm_i915_private *i915);
+void i915_pmu_gt_parked(struct intel_gt *gt);
+void i915_pmu_gt_unparked(struct intel_gt *gt);
 #else
 static inline int i915_pmu_init(void) { return 0; }
 static inline void i915_pmu_exit(void) {}
 static inline void i915_pmu_register(struct drm_i915_private *i915) {}
 static inline void i915_pmu_unregister(struct drm_i915_private *i915) {}
-static inline void i915_pmu_gt_parked(struct drm_i915_private *i915) {}
-static inline void i915_pmu_gt_unparked(struct drm_i915_private *i915) {}
+static inline void i915_pmu_gt_parked(struct intel_gt *gt) {}
+static inline void i915_pmu_gt_unparked(struct intel_gt *gt) {}
 #endif
 
 #endif
-- 
2.36.1

[Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We do not want to have timers per tile and waste CPU cycles and energy via
multiple wake-up sources, for a relatively un-important task of PMU
sampling, so keeping a single timer works well. But we also do not want
the first GT which goes idle to turn off the timer.

Add some reference counting, via a mask of unparked GTs, to solve this.

v2: Drop the check for unparked in i915_sample (Ashutosh)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
 drivers/gpu/drm/i915/i915_pmu.h | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 2b63ee31e1b3..725b01b00775 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
 	 * Signal sampling timer to stop if only engine events are enabled and
 	 * GPU went idle.
 	 */
-	pmu->timer_enabled = pmu_needs_timer(pmu, false);
+	pmu->unparked &= ~BIT(gt->info.id);
+	if (pmu->unparked == 0)
+		pmu->timer_enabled = pmu_needs_timer(pmu, false);
 
 	spin_unlock_irq(&pmu->lock);
 }
@@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
 	/*
 	 * Re-enable sampling timer when GPU goes active.
 	 */
-	__i915_pmu_maybe_start_timer(pmu);
+	if (pmu->unparked == 0)
+		__i915_pmu_maybe_start_timer(pmu);
+
+	pmu->unparked |= BIT(gt->info.id);
 
 	spin_unlock_irq(&pmu->lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index a686fd7ccedf..3a811266ac6a 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -76,6 +76,10 @@ struct i915_pmu {
 	 * @lock: Lock protecting enable mask and ref count handling.
 	 */
 	spinlock_t lock;
+	/**
+	 * @unparked: GT unparked mask.
+	 */
+	unsigned int unparked;
 	/**
 	 * @timer: Timer for internal i915 PMU sampling.
 	 */
-- 
2.36.1

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Fri, 12 May 2023 18:55:43 -0700, Umesh Nerlige Ramappa wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We do not want to have timers per tile and waste CPU cycles and energy via
> multiple wake-up sources, for a relatively un-important task of PMU
> sampling, so keeping a single timer works well. But we also do not want
> the first GT which goes idle to turn off the timer.
>
> Add some reference counting, via a mask of unparked GTs, to solve this.
>
> v2: Drop the check for unparked in i915_sample (Ashutosh)

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
>  drivers/gpu/drm/i915/i915_pmu.h | 4 ++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 2b63ee31e1b3..725b01b00775 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>	 * Signal sampling timer to stop if only engine events are enabled and
>	 * GPU went idle.
>	 */
> -	pmu->timer_enabled = pmu_needs_timer(pmu, false);
> +	pmu->unparked &= ~BIT(gt->info.id);
> +	if (pmu->unparked == 0)
> +		pmu->timer_enabled = pmu_needs_timer(pmu, false);
>
>	spin_unlock_irq(&pmu->lock);
>  }
> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>	/*
>	 * Re-enable sampling timer when GPU goes active.
>	 */
> -	__i915_pmu_maybe_start_timer(pmu);
> +	if (pmu->unparked == 0)
> +		__i915_pmu_maybe_start_timer(pmu);
> +
> +	pmu->unparked |= BIT(gt->info.id);
>
>	spin_unlock_irq(&pmu->lock);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index a686fd7ccedf..3a811266ac6a 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -76,6 +76,10 @@ struct i915_pmu {
>	 * @lock: Lock protecting enable mask and ref count handling.
>	 */
>	spinlock_t lock;
> +	/**
> +	 * @unparked: GT unparked mask.
> +	 */
> +	unsigned int unparked;
>	/**
>	 * @timer: Timer for internal i915 PMU sampling.
>	 */
> --
> 2.36.1
>

[Intel-gfx] [PATCH 5/6] drm/i915/pmu: Prepare for multi-tile non-engine counters

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reserve some bits in the counter config namespace which will carry the
tile id and prepare the code to handle this.

No per tile counters have been added yet.

v2:
- Fix checkpatch issues
- Use 4 bits for gt id in non-engine counters. Drop FIXME.
- Set MAX GTs to 4. Drop FIXME.

v3: (Ashutosh, Tvrtko)
- Drop BUG_ON that would never fire
- Make enable u64
- Pull in some code from next patch

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 148 +++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_pmu.h |  11 ++-
 include/uapi/drm/i915_drm.h     |  17 +++-
 3 files changed, 129 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 725b01b00775..b3dd9e51c5cc 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -56,11 +56,21 @@ static bool is_engine_config(u64 config)
 	return config < __I915_PMU_OTHER(0);
 }
 
+static unsigned int config_gt_id(const u64 config)
+{
+	return config >> __I915_PMU_GT_SHIFT;
+}
+
+static u64 config_counter(const u64 config)
+{
+	return config & ~(~0ULL << __I915_PMU_GT_SHIFT);
+}
+
 static unsigned int other_bit(const u64 config)
 {
 	unsigned int val;
 
-	switch (config) {
+	switch (config_counter(config)) {
 	case I915_PMU_ACTUAL_FREQUENCY:
 		val =  __I915_PMU_ACTUAL_FREQUENCY_ENABLED;
 		break;
@@ -78,7 +88,9 @@ static unsigned int other_bit(const u64 config)
 		return -1;
 	}
 
-	return I915_ENGINE_SAMPLE_COUNT + val;
+	return I915_ENGINE_SAMPLE_COUNT +
+	       config_gt_id(config) * __I915_PMU_TRACKED_EVENT_COUNT +
+	       val;
 }
 
 static unsigned int config_bit(const u64 config)
@@ -104,10 +116,22 @@ static unsigned int event_bit(struct perf_event *event)
 	return config_bit(event->attr.config);
 }
 
+static u64 frequency_enabled_mask(void)
+{
+	unsigned int i;
+	u64 mask = 0;
+
+	for (i = 0; i < I915_PMU_MAX_GTS; i++)
+		mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) |
+			config_mask(__I915_PMU_REQUESTED_FREQUENCY(i));
+
+	return mask;
+}
+
 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;
+	u64 enable;
 
 	/*
 	 * Only some counters need the sampling timer.
@@ -120,9 +144,7 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
 	 * Mask out all the ones which do not need the timer, or in
 	 * other words keep all the ones that could need the timer.
 	 */
-	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
-		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
-		  ENGINE_SAMPLE_MASK;
+	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
 
 	/*
 	 * When the GPU is idle per-engine counters do not need to be
@@ -164,9 +186,37 @@ static inline s64 ktime_since_raw(const ktime_t kt)
 	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
 }
 
+static unsigned int
+__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
+{
+	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
+
+	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
+
+	return idx;
+}
+
+static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
+{
+	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
+}
+
+static void
+store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
+{
+	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
+}
+
+static void
+add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
+{
+	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
+}
+
 static u64 get_rc6(struct intel_gt *gt)
 {
 	struct drm_i915_private *i915 = gt->i915;
+	const unsigned int gt_id = gt->info.id;
 	struct i915_pmu *pmu = &i915->pmu;
 	unsigned long flags;
 	bool awake = false;
@@ -181,7 +231,7 @@ static u64 get_rc6(struct intel_gt *gt)
 	spin_lock_irqsave(&pmu->lock, flags);
 
 	if (awake) {
-		pmu->sample[__I915_SAMPLE_RC6].cur = val;
+		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
 	} else {
 		/*
 		 * We think we are runtime suspended.
@@ -190,14 +240,14 @@ static u64 get_rc6(struct intel_gt *gt)
 		 * on top of the last known real value, as the approximated RC6
 		 * counter value.
 		 */
-		val = ktime_since_raw(pmu->sleep_last);
-		val += pmu->sample[__I915_SAMPLE_RC6].cur;
+		val = ktime_since_raw(pmu->sleep_last[gt_id]);
+		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
 	}
 
-	if (val < pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur)
-		val = pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur;
+	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
+		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
 	else
-		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
+		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
 
 	spin_unlock_irqrestore(&pmu->lock, flags);
 
@@ -207,13 +257,20 @@ static u64 get_rc6(struct intel_gt *gt)
 static void init_rc6(struct i915_pmu *pmu)
 {
 	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
-	intel_wakeref_t wakeref;
+	struct intel_gt *gt;
+	unsigned int i;
+
+	for_each_gt(gt, i915, i) {
+		intel_wakeref_t wakeref;
+
+		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
+			u64 val = __get_rc6(gt);
 
-	with_intel_runtime_pm(to_gt(i915)->uncore->rpm, wakeref) {
-		pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(to_gt(i915));
-		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur =
-					pmu->sample[__I915_SAMPLE_RC6].cur;
-		pmu->sleep_last = ktime_get_raw();
+			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
+			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
+				     val);
+			pmu->sleep_last[i] = ktime_get_raw();
+		}
 	}
 }
 
@@ -221,8 +278,8 @@ static void park_rc6(struct intel_gt *gt)
 {
 	struct i915_pmu *pmu = &gt->i915->pmu;
 
-	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(gt);
-	pmu->sleep_last = ktime_get_raw();
+	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
+	pmu->sleep_last[gt->info.id] = ktime_get_raw();
 }
 
 static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
@@ -362,34 +419,30 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
 	}
 }
 
-static void
-add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
-{
-	sample->cur += mul_u32_u32(val, mul);
-}
-
-static bool frequency_sampling_enabled(struct i915_pmu *pmu)
+static bool
+frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
 {
 	return pmu->enable &
-	       (config_mask(I915_PMU_ACTUAL_FREQUENCY) |
-		config_mask(I915_PMU_REQUESTED_FREQUENCY));
+	       (config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt)) |
+		config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt)));
 }
 
 static void
 frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 {
 	struct drm_i915_private *i915 = gt->i915;
+	const unsigned int gt_id = gt->info.id;
 	struct i915_pmu *pmu = &i915->pmu;
 	struct intel_rps *rps = &gt->rps;
 
-	if (!frequency_sampling_enabled(pmu))
+	if (!frequency_sampling_enabled(pmu, gt_id))
 		return;
 
 	/* Report 0/0 (actual/requested) frequency while parked. */
 	if (!intel_gt_pm_get_if_awake(gt))
 		return;
 
-	if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
+	if (pmu->enable & config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt_id))) {
 		u32 val;
 
 		/*
@@ -405,12 +458,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 		if (!val)
 			val = intel_gpu_freq(rps, rps->cur_freq);
 
-		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
+		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
 				val, period_ns / 1000);
 	}
 
-	if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
-		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_REQ],
+	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
+		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
 				intel_rps_get_requested_frequency(rps),
 				period_ns / 1000);
 	}
@@ -444,9 +497,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 
 	for_each_gt(gt, i915, i) {
 		engines_sample(gt, period_ns);
-
-		if (i == 0) /* FIXME */
-			frequency_sample(gt, period_ns);
+		frequency_sample(gt, period_ns);
 	}
 
 	hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
@@ -488,7 +539,13 @@ config_status(struct drm_i915_private *i915, u64 config)
 {
 	struct intel_gt *gt = to_gt(i915);
 
-	switch (config) {
+	unsigned int gt_id = config_gt_id(config);
+	unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
+
+	if (gt_id > max_gt_id)
+		return -ENOENT;
+
+	switch (config_counter(config)) {
 	case I915_PMU_ACTUAL_FREQUENCY:
 		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
 			/* Requires a mutex for sampling! */
@@ -499,6 +556,8 @@ config_status(struct drm_i915_private *i915, u64 config)
 			return -ENODEV;
 		break;
 	case I915_PMU_INTERRUPTS:
+		if (gt_id)
+			return -ENOENT;
 		break;
 	case I915_PMU_RC6_RESIDENCY:
 		if (!gt->rc6.supported)
@@ -596,22 +655,27 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
 			val = engine->pmu.sample[sample].cur;
 		}
 	} else {
-		switch (event->attr.config) {
+		const unsigned int gt_id = config_gt_id(event->attr.config);
+		const u64 config = config_counter(event->attr.config);
+
+		switch (config) {
 		case I915_PMU_ACTUAL_FREQUENCY:
 			val =
-			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_ACT].cur,
+			   div_u64(read_sample(pmu, gt_id,
+					       __I915_SAMPLE_FREQ_ACT),
 				   USEC_PER_SEC /* to MHz */);
 			break;
 		case I915_PMU_REQUESTED_FREQUENCY:
 			val =
-			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_REQ].cur,
+			   div_u64(read_sample(pmu, gt_id,
+					       __I915_SAMPLE_FREQ_REQ),
 				   USEC_PER_SEC /* to MHz */);
 			break;
 		case I915_PMU_INTERRUPTS:
 			val = READ_ONCE(pmu->irq_count);
 			break;
 		case I915_PMU_RC6_RESIDENCY:
-			val = get_rc6(to_gt(i915));
+			val = get_rc6(i915->gt[gt_id]);
 			break;
 		case I915_PMU_SOFTWARE_GT_AWAKE_TIME:
 			val = ktime_to_ns(intel_gt_get_awake_time(to_gt(i915)));
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 3a811266ac6a..ea2d24ef5664 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -38,13 +38,16 @@ enum {
 	__I915_NUM_PMU_SAMPLERS
 };
 
+#define I915_PMU_MAX_GTS 4
+
 /*
  * How many different events we track in the global PMU mask.
  *
  * It is also used to know to needed number of event reference counters.
  */
 #define I915_PMU_MASK_BITS \
-	(I915_ENGINE_SAMPLE_COUNT + __I915_PMU_TRACKED_EVENT_COUNT)
+	(I915_ENGINE_SAMPLE_COUNT + \
+	 I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT)
 
 #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
 
@@ -95,7 +98,7 @@ struct i915_pmu {
 	 *
 	 * Low bits are engine samplers and other events continue from there.
 	 */
-	u32 enable;
+	u64 enable;
 
 	/**
 	 * @timer_last:
@@ -124,11 +127,11 @@ struct i915_pmu {
 	 * Only global counters are held here, while the per-engine ones are in
 	 * struct intel_engine_cs.
 	 */
-	struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
+	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
 	/**
 	 * @sleep_last: Last time GT parked for RC6 estimation.
 	 */
-	ktime_t sleep_last;
+	ktime_t sleep_last[I915_PMU_MAX_GTS];
 	/**
 	 * @irq_count: Number of interrupts
 	 *
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index dba7c5a5b25e..d5ac1fdeb2b1 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -280,7 +280,16 @@ enum drm_i915_pmu_engine_sample {
 #define I915_PMU_ENGINE_SEMA(class, instance) \
 	__I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
 
-#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
+/*
+ * Top 4 bits of every non-engine counter are GT id.
+ */
+#define __I915_PMU_GT_SHIFT (60)
+
+#define ___I915_PMU_OTHER(gt, x) \
+	(((__u64)__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x)) | \
+	((__u64)(gt) << __I915_PMU_GT_SHIFT))
+
+#define __I915_PMU_OTHER(x) ___I915_PMU_OTHER(0, x)
 
 #define I915_PMU_ACTUAL_FREQUENCY	__I915_PMU_OTHER(0)
 #define I915_PMU_REQUESTED_FREQUENCY	__I915_PMU_OTHER(1)
@@ -290,6 +299,12 @@ enum drm_i915_pmu_engine_sample {
 
 #define I915_PMU_LAST /* Deprecated - do not use */ I915_PMU_RC6_RESIDENCY
 
+#define __I915_PMU_ACTUAL_FREQUENCY(gt)		___I915_PMU_OTHER(gt, 0)
+#define __I915_PMU_REQUESTED_FREQUENCY(gt)	___I915_PMU_OTHER(gt, 1)
+#define __I915_PMU_INTERRUPTS(gt)		___I915_PMU_OTHER(gt, 2)
+#define __I915_PMU_RC6_RESIDENCY(gt)		___I915_PMU_OTHER(gt, 3)
+#define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt)	___I915_PMU_OTHER(gt, 4)
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
2.36.1

Re: [Intel-gfx] [PATCH 5/6] drm/i915/pmu: Prepare for multi-tile non-engine counters

[Dixit, Ashutosh]

On Fri, 12 May 2023 18:55:44 -0700, Umesh Nerlige Ramappa wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Reserve some bits in the counter config namespace which will carry the
> tile id and prepare the code to handle this.
>
> No per tile counters have been added yet.
>
> v2:
> - Fix checkpatch issues
> - Use 4 bits for gt id in non-engine counters. Drop FIXME.
> - Set MAX GTs to 4. Drop FIXME.
>
> v3: (Ashutosh, Tvrtko)
> - Drop BUG_ON that would never fire
> - Make enable u64
> - Pull in some code from next patch

Just a reminder in case you want to do something like:

#define I915_PMU_MAX_GTS I915_MAX_GT

Or replace I915_PMU_MAX_GTS by I915_MAX_GT.

But otherwise v3 LGTM:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 148 +++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_pmu.h |  11 ++-
>  include/uapi/drm/i915_drm.h     |  17 +++-
>  3 files changed, 129 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 725b01b00775..b3dd9e51c5cc 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -56,11 +56,21 @@ static bool is_engine_config(u64 config)
>	return config < __I915_PMU_OTHER(0);
>  }
>
> +static unsigned int config_gt_id(const u64 config)
> +{
> +	return config >> __I915_PMU_GT_SHIFT;
> +}
> +
> +static u64 config_counter(const u64 config)
> +{
> +	return config & ~(~0ULL << __I915_PMU_GT_SHIFT);
> +}
> +
>  static unsigned int other_bit(const u64 config)
>  {
>	unsigned int val;
>
> -	switch (config) {
> +	switch (config_counter(config)) {
>	case I915_PMU_ACTUAL_FREQUENCY:
>		val =  __I915_PMU_ACTUAL_FREQUENCY_ENABLED;
>		break;
> @@ -78,7 +88,9 @@ static unsigned int other_bit(const u64 config)
>		return -1;
>	}
>
> -	return I915_ENGINE_SAMPLE_COUNT + val;
> +	return I915_ENGINE_SAMPLE_COUNT +
> +	       config_gt_id(config) * __I915_PMU_TRACKED_EVENT_COUNT +
> +	       val;
>  }
>
>  static unsigned int config_bit(const u64 config)
> @@ -104,10 +116,22 @@ static unsigned int event_bit(struct perf_event *event)
>	return config_bit(event->attr.config);
>  }
>
> +static u64 frequency_enabled_mask(void)
> +{
> +	unsigned int i;
> +	u64 mask = 0;
> +
> +	for (i = 0; i < I915_PMU_MAX_GTS; i++)
> +		mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) |
> +			config_mask(__I915_PMU_REQUESTED_FREQUENCY(i));
> +
> +	return mask;
> +}
> +
>  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;
> +	u64 enable;
>
>	/*
>	 * Only some counters need the sampling timer.
> @@ -120,9 +144,7 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>	 * Mask out all the ones which do not need the timer, or in
>	 * other words keep all the ones that could need the timer.
>	 */
> -	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> -		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
> -		  ENGINE_SAMPLE_MASK;
> +	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
>
>	/*
>	 * When the GPU is idle per-engine counters do not need to be
> @@ -164,9 +186,37 @@ static inline s64 ktime_since_raw(const ktime_t kt)
>	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
>  }
>
> +static unsigned int
> +__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> +{
> +	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> +
> +	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> +
> +	return idx;
> +}
> +
> +static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> +{
> +	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> +}
> +
> +static void
> +store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> +{
> +	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> +}
> +
> +static void
> +add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> +{
> +	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> +}
> +
>  static u64 get_rc6(struct intel_gt *gt)
>  {
>	struct drm_i915_private *i915 = gt->i915;
> +	const unsigned int gt_id = gt->info.id;
>	struct i915_pmu *pmu = &i915->pmu;
>	unsigned long flags;
>	bool awake = false;
> @@ -181,7 +231,7 @@ static u64 get_rc6(struct intel_gt *gt)
>	spin_lock_irqsave(&pmu->lock, flags);
>
>	if (awake) {
> -		pmu->sample[__I915_SAMPLE_RC6].cur = val;
> +		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
>	} else {
>		/*
>		 * We think we are runtime suspended.
> @@ -190,14 +240,14 @@ static u64 get_rc6(struct intel_gt *gt)
>		 * on top of the last known real value, as the approximated RC6
>		 * counter value.
>		 */
> -		val = ktime_since_raw(pmu->sleep_last);
> -		val += pmu->sample[__I915_SAMPLE_RC6].cur;
> +		val = ktime_since_raw(pmu->sleep_last[gt_id]);
> +		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
>	}
>
> -	if (val < pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> -		val = pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur;
> +	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> +		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
>	else
> -		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> +		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
>
>	spin_unlock_irqrestore(&pmu->lock, flags);
>
> @@ -207,13 +257,20 @@ static u64 get_rc6(struct intel_gt *gt)
>  static void init_rc6(struct i915_pmu *pmu)
>  {
>	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> -	intel_wakeref_t wakeref;
> +	struct intel_gt *gt;
> +	unsigned int i;
> +
> +	for_each_gt(gt, i915, i) {
> +		intel_wakeref_t wakeref;
> +
> +		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
> +			u64 val = __get_rc6(gt);
>
> -	with_intel_runtime_pm(to_gt(i915)->uncore->rpm, wakeref) {
> -		pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(to_gt(i915));
> -		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur =
> -					pmu->sample[__I915_SAMPLE_RC6].cur;
> -		pmu->sleep_last = ktime_get_raw();
> +			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> +			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> +				     val);
> +			pmu->sleep_last[i] = ktime_get_raw();
> +		}
>	}
>  }
>
> @@ -221,8 +278,8 @@ static void park_rc6(struct intel_gt *gt)
>  {
>	struct i915_pmu *pmu = &gt->i915->pmu;
>
> -	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(gt);
> -	pmu->sleep_last = ktime_get_raw();
> +	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> +	pmu->sleep_last[gt->info.id] = ktime_get_raw();
>  }
>
>  static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
> @@ -362,34 +419,30 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
>	}
>  }
>
> -static void
> -add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> -{
> -	sample->cur += mul_u32_u32(val, mul);
> -}
> -
> -static bool frequency_sampling_enabled(struct i915_pmu *pmu)
> +static bool
> +frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
>  {
>	return pmu->enable &
> -	       (config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> -		config_mask(I915_PMU_REQUESTED_FREQUENCY));
> +	       (config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt)) |
> +		config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt)));
>  }
>
>  static void
>  frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>  {
>	struct drm_i915_private *i915 = gt->i915;
> +	const unsigned int gt_id = gt->info.id;
>	struct i915_pmu *pmu = &i915->pmu;
>	struct intel_rps *rps = &gt->rps;
>
> -	if (!frequency_sampling_enabled(pmu))
> +	if (!frequency_sampling_enabled(pmu, gt_id))
>		return;
>
>	/* Report 0/0 (actual/requested) frequency while parked. */
>	if (!intel_gt_pm_get_if_awake(gt))
>		return;
>
> -	if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
> +	if (pmu->enable & config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt_id))) {
>		u32 val;
>
>		/*
> @@ -405,12 +458,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>		if (!val)
>			val = intel_gpu_freq(rps, rps->cur_freq);
>
> -		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
> +		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
>				val, period_ns / 1000);
>	}
>
> -	if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
> -		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_REQ],
> +	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> +		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
>				intel_rps_get_requested_frequency(rps),
>				period_ns / 1000);
>	}
> @@ -444,9 +497,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>
>	for_each_gt(gt, i915, i) {
>		engines_sample(gt, period_ns);
> -
> -		if (i == 0) /* FIXME */
> -			frequency_sample(gt, period_ns);
> +		frequency_sample(gt, period_ns);
>	}
>
>	hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
> @@ -488,7 +539,13 @@ config_status(struct drm_i915_private *i915, u64 config)
>  {
>	struct intel_gt *gt = to_gt(i915);
>
> -	switch (config) {
> +	unsigned int gt_id = config_gt_id(config);
> +	unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
> +
> +	if (gt_id > max_gt_id)
> +		return -ENOENT;
> +
> +	switch (config_counter(config)) {
>	case I915_PMU_ACTUAL_FREQUENCY:
>		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>			/* Requires a mutex for sampling! */
> @@ -499,6 +556,8 @@ config_status(struct drm_i915_private *i915, u64 config)
>			return -ENODEV;
>		break;
>	case I915_PMU_INTERRUPTS:
> +		if (gt_id)
> +			return -ENOENT;
>		break;
>	case I915_PMU_RC6_RESIDENCY:
>		if (!gt->rc6.supported)
> @@ -596,22 +655,27 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
>			val = engine->pmu.sample[sample].cur;
>		}
>	} else {
> -		switch (event->attr.config) {
> +		const unsigned int gt_id = config_gt_id(event->attr.config);
> +		const u64 config = config_counter(event->attr.config);
> +
> +		switch (config) {
>		case I915_PMU_ACTUAL_FREQUENCY:
>			val =
> -			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_ACT].cur,
> +			   div_u64(read_sample(pmu, gt_id,
> +					       __I915_SAMPLE_FREQ_ACT),
>				   USEC_PER_SEC /* to MHz */);
>			break;
>		case I915_PMU_REQUESTED_FREQUENCY:
>			val =
> -			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_REQ].cur,
> +			   div_u64(read_sample(pmu, gt_id,
> +					       __I915_SAMPLE_FREQ_REQ),
>				   USEC_PER_SEC /* to MHz */);
>			break;
>		case I915_PMU_INTERRUPTS:
>			val = READ_ONCE(pmu->irq_count);
>			break;
>		case I915_PMU_RC6_RESIDENCY:
> -			val = get_rc6(to_gt(i915));
> +			val = get_rc6(i915->gt[gt_id]);
>			break;
>		case I915_PMU_SOFTWARE_GT_AWAKE_TIME:
>			val = ktime_to_ns(intel_gt_get_awake_time(to_gt(i915)));
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 3a811266ac6a..ea2d24ef5664 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -38,13 +38,16 @@ enum {
>	__I915_NUM_PMU_SAMPLERS
>  };
>
> +#define I915_PMU_MAX_GTS 4
> +
>  /*
>   * How many different events we track in the global PMU mask.
>   *
>   * It is also used to know to needed number of event reference counters.
>   */
>  #define I915_PMU_MASK_BITS \
> -	(I915_ENGINE_SAMPLE_COUNT + __I915_PMU_TRACKED_EVENT_COUNT)
> +	(I915_ENGINE_SAMPLE_COUNT + \
> +	 I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT)
>
>  #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
>
> @@ -95,7 +98,7 @@ struct i915_pmu {
>	 *
>	 * Low bits are engine samplers and other events continue from there.
>	 */
> -	u32 enable;
> +	u64 enable;
>
>	/**
>	 * @timer_last:
> @@ -124,11 +127,11 @@ struct i915_pmu {
>	 * Only global counters are held here, while the per-engine ones are in
>	 * struct intel_engine_cs.
>	 */
> -	struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
>	/**
>	 * @sleep_last: Last time GT parked for RC6 estimation.
>	 */
> -	ktime_t sleep_last;
> +	ktime_t sleep_last[I915_PMU_MAX_GTS];
>	/**
>	 * @irq_count: Number of interrupts
>	 *
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index dba7c5a5b25e..d5ac1fdeb2b1 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -280,7 +280,16 @@ enum drm_i915_pmu_engine_sample {
>  #define I915_PMU_ENGINE_SEMA(class, instance) \
>	__I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
>
> -#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
> +/*
> + * Top 4 bits of every non-engine counter are GT id.
> + */
> +#define __I915_PMU_GT_SHIFT (60)
> +
> +#define ___I915_PMU_OTHER(gt, x) \
> +	(((__u64)__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x)) | \
> +	((__u64)(gt) << __I915_PMU_GT_SHIFT))
> +
> +#define __I915_PMU_OTHER(x) ___I915_PMU_OTHER(0, x)
>
>  #define I915_PMU_ACTUAL_FREQUENCY	__I915_PMU_OTHER(0)
>  #define I915_PMU_REQUESTED_FREQUENCY	__I915_PMU_OTHER(1)
> @@ -290,6 +299,12 @@ enum drm_i915_pmu_engine_sample {
>
>  #define I915_PMU_LAST /* Deprecated - do not use */ I915_PMU_RC6_RESIDENCY
>
> +#define __I915_PMU_ACTUAL_FREQUENCY(gt)		___I915_PMU_OTHER(gt, 0)
> +#define __I915_PMU_REQUESTED_FREQUENCY(gt)	___I915_PMU_OTHER(gt, 1)
> +#define __I915_PMU_INTERRUPTS(gt)		___I915_PMU_OTHER(gt, 2)
> +#define __I915_PMU_RC6_RESIDENCY(gt)		___I915_PMU_OTHER(gt, 3)
> +#define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt)	___I915_PMU_OTHER(gt, 4)
> +
>  /* Each region is a minimum of 16k, and there are at most 255 of them.
>   */
>  #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
> --
> 2.36.1
>

Re: [Intel-gfx] [PATCH 5/6] drm/i915/pmu: Prepare for multi-tile non-engine counters

[Umesh Nerlige Ramappa]

On Fri, May 12, 2023 at 09:41:56PM -0700, Dixit, Ashutosh wrote:
>On Fri, 12 May 2023 18:55:44 -0700, Umesh Nerlige Ramappa wrote:
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Reserve some bits in the counter config namespace which will carry the
>> tile id and prepare the code to handle this.
>>
>> No per tile counters have been added yet.
>>
>> v2:
>> - Fix checkpatch issues
>> - Use 4 bits for gt id in non-engine counters. Drop FIXME.
>> - Set MAX GTs to 4. Drop FIXME.
>>
>> v3: (Ashutosh, Tvrtko)
>> - Drop BUG_ON that would never fire
>> - Make enable u64
>> - Pull in some code from next patch
>
>Just a reminder in case you want to do something like:
>
>#define I915_PMU_MAX_GTS I915_MAX_GT
>
>Or replace I915_PMU_MAX_GTS by I915_MAX_GT.

Hmmm, I thought I sent out a response separately for that in the 
previous series, but I am not able to locate it, strange. Anyways, I did 
try that and ran into issues that Tvrtko was mentioning w.r.t. header 
dependencies. I think i915_drv.h includes intel_engine.h and that 
includes i915_pmu.h. So including i915_drv.h in i915_pmu.h for the 
definition of I915_MAX_GT is just wreaking havoc during compile.

Hence, gave up on that and using whatever existed before in Tvrtko's 
patch.

Thanks,
Umesh

>
>But otherwise v3 LGTM:
>
>Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_pmu.c | 148 +++++++++++++++++++++++---------
>>  drivers/gpu/drm/i915/i915_pmu.h |  11 ++-
>>  include/uapi/drm/i915_drm.h     |  17 +++-
>>  3 files changed, 129 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index 725b01b00775..b3dd9e51c5cc 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -56,11 +56,21 @@ static bool is_engine_config(u64 config)
>>	return config < __I915_PMU_OTHER(0);
>>  }
>>
>> +static unsigned int config_gt_id(const u64 config)
>> +{
>> +	return config >> __I915_PMU_GT_SHIFT;
>> +}
>> +
>> +static u64 config_counter(const u64 config)
>> +{
>> +	return config & ~(~0ULL << __I915_PMU_GT_SHIFT);
>> +}
>> +
>>  static unsigned int other_bit(const u64 config)
>>  {
>>	unsigned int val;
>>
>> -	switch (config) {
>> +	switch (config_counter(config)) {
>>	case I915_PMU_ACTUAL_FREQUENCY:
>>		val =  __I915_PMU_ACTUAL_FREQUENCY_ENABLED;
>>		break;
>> @@ -78,7 +88,9 @@ static unsigned int other_bit(const u64 config)
>>		return -1;
>>	}
>>
>> -	return I915_ENGINE_SAMPLE_COUNT + val;
>> +	return I915_ENGINE_SAMPLE_COUNT +
>> +	       config_gt_id(config) * __I915_PMU_TRACKED_EVENT_COUNT +
>> +	       val;
>>  }
>>
>>  static unsigned int config_bit(const u64 config)
>> @@ -104,10 +116,22 @@ static unsigned int event_bit(struct perf_event *event)
>>	return config_bit(event->attr.config);
>>  }
>>
>> +static u64 frequency_enabled_mask(void)
>> +{
>> +	unsigned int i;
>> +	u64 mask = 0;
>> +
>> +	for (i = 0; i < I915_PMU_MAX_GTS; i++)
>> +		mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) |
>> +			config_mask(__I915_PMU_REQUESTED_FREQUENCY(i));
>> +
>> +	return mask;
>> +}
>> +
>>  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;
>> +	u64 enable;
>>
>>	/*
>>	 * Only some counters need the sampling timer.
>> @@ -120,9 +144,7 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>>	 * Mask out all the ones which do not need the timer, or in
>>	 * other words keep all the ones that could need the timer.
>>	 */
>> -	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
>> -		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
>> -		  ENGINE_SAMPLE_MASK;
>> +	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
>>
>>	/*
>>	 * When the GPU is idle per-engine counters do not need to be
>> @@ -164,9 +186,37 @@ static inline s64 ktime_since_raw(const ktime_t kt)
>>	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
>>  }
>>
>> +static unsigned int
>> +__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
>> +{
>> +	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
>> +
>> +	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
>> +
>> +	return idx;
>> +}
>> +
>> +static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
>> +{
>> +	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
>> +}
>> +
>> +static void
>> +store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
>> +{
>> +	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
>> +}
>> +
>> +static void
>> +add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
>> +{
>> +	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
>> +}
>> +
>>  static u64 get_rc6(struct intel_gt *gt)
>>  {
>>	struct drm_i915_private *i915 = gt->i915;
>> +	const unsigned int gt_id = gt->info.id;
>>	struct i915_pmu *pmu = &i915->pmu;
>>	unsigned long flags;
>>	bool awake = false;
>> @@ -181,7 +231,7 @@ static u64 get_rc6(struct intel_gt *gt)
>>	spin_lock_irqsave(&pmu->lock, flags);
>>
>>	if (awake) {
>> -		pmu->sample[__I915_SAMPLE_RC6].cur = val;
>> +		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
>>	} else {
>>		/*
>>		 * We think we are runtime suspended.
>> @@ -190,14 +240,14 @@ static u64 get_rc6(struct intel_gt *gt)
>>		 * on top of the last known real value, as the approximated RC6
>>		 * counter value.
>>		 */
>> -		val = ktime_since_raw(pmu->sleep_last);
>> -		val += pmu->sample[__I915_SAMPLE_RC6].cur;
>> +		val = ktime_since_raw(pmu->sleep_last[gt_id]);
>> +		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
>>	}
>>
>> -	if (val < pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur)
>> -		val = pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur;
>> +	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
>> +		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
>>	else
>> -		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>> +		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
>>
>>	spin_unlock_irqrestore(&pmu->lock, flags);
>>
>> @@ -207,13 +257,20 @@ static u64 get_rc6(struct intel_gt *gt)
>>  static void init_rc6(struct i915_pmu *pmu)
>>  {
>>	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>> -	intel_wakeref_t wakeref;
>> +	struct intel_gt *gt;
>> +	unsigned int i;
>> +
>> +	for_each_gt(gt, i915, i) {
>> +		intel_wakeref_t wakeref;
>> +
>> +		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
>> +			u64 val = __get_rc6(gt);
>>
>> -	with_intel_runtime_pm(to_gt(i915)->uncore->rpm, wakeref) {
>> -		pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(to_gt(i915));
>> -		pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur =
>> -					pmu->sample[__I915_SAMPLE_RC6].cur;
>> -		pmu->sleep_last = ktime_get_raw();
>> +			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
>> +			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
>> +				     val);
>> +			pmu->sleep_last[i] = ktime_get_raw();
>> +		}
>>	}
>>  }
>>
>> @@ -221,8 +278,8 @@ static void park_rc6(struct intel_gt *gt)
>>  {
>>	struct i915_pmu *pmu = &gt->i915->pmu;
>>
>> -	pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(gt);
>> -	pmu->sleep_last = ktime_get_raw();
>> +	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
>> +	pmu->sleep_last[gt->info.id] = ktime_get_raw();
>>  }
>>
>>  static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
>> @@ -362,34 +419,30 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
>>	}
>>  }
>>
>> -static void
>> -add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
>> -{
>> -	sample->cur += mul_u32_u32(val, mul);
>> -}
>> -
>> -static bool frequency_sampling_enabled(struct i915_pmu *pmu)
>> +static bool
>> +frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
>>  {
>>	return pmu->enable &
>> -	       (config_mask(I915_PMU_ACTUAL_FREQUENCY) |
>> -		config_mask(I915_PMU_REQUESTED_FREQUENCY));
>> +	       (config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt)) |
>> +		config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt)));
>>  }
>>
>>  static void
>>  frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>>  {
>>	struct drm_i915_private *i915 = gt->i915;
>> +	const unsigned int gt_id = gt->info.id;
>>	struct i915_pmu *pmu = &i915->pmu;
>>	struct intel_rps *rps = &gt->rps;
>>
>> -	if (!frequency_sampling_enabled(pmu))
>> +	if (!frequency_sampling_enabled(pmu, gt_id))
>>		return;
>>
>>	/* Report 0/0 (actual/requested) frequency while parked. */
>>	if (!intel_gt_pm_get_if_awake(gt))
>>		return;
>>
>> -	if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
>> +	if (pmu->enable & config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt_id))) {
>>		u32 val;
>>
>>		/*
>> @@ -405,12 +458,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>>		if (!val)
>>			val = intel_gpu_freq(rps, rps->cur_freq);
>>
>> -		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
>> +		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
>>				val, period_ns / 1000);
>>	}
>>
>> -	if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
>> -		add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_REQ],
>> +	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
>> +		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
>>				intel_rps_get_requested_frequency(rps),
>>				period_ns / 1000);
>>	}
>> @@ -444,9 +497,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>>
>>	for_each_gt(gt, i915, i) {
>>		engines_sample(gt, period_ns);
>> -
>> -		if (i == 0) /* FIXME */
>> -			frequency_sample(gt, period_ns);
>> +		frequency_sample(gt, period_ns);
>>	}
>>
>>	hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
>> @@ -488,7 +539,13 @@ config_status(struct drm_i915_private *i915, u64 config)
>>  {
>>	struct intel_gt *gt = to_gt(i915);
>>
>> -	switch (config) {
>> +	unsigned int gt_id = config_gt_id(config);
>> +	unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
>> +
>> +	if (gt_id > max_gt_id)
>> +		return -ENOENT;
>> +
>> +	switch (config_counter(config)) {
>>	case I915_PMU_ACTUAL_FREQUENCY:
>>		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>>			/* Requires a mutex for sampling! */
>> @@ -499,6 +556,8 @@ config_status(struct drm_i915_private *i915, u64 config)
>>			return -ENODEV;
>>		break;
>>	case I915_PMU_INTERRUPTS:
>> +		if (gt_id)
>> +			return -ENOENT;
>>		break;
>>	case I915_PMU_RC6_RESIDENCY:
>>		if (!gt->rc6.supported)
>> @@ -596,22 +655,27 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
>>			val = engine->pmu.sample[sample].cur;
>>		}
>>	} else {
>> -		switch (event->attr.config) {
>> +		const unsigned int gt_id = config_gt_id(event->attr.config);
>> +		const u64 config = config_counter(event->attr.config);
>> +
>> +		switch (config) {
>>		case I915_PMU_ACTUAL_FREQUENCY:
>>			val =
>> -			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_ACT].cur,
>> +			   div_u64(read_sample(pmu, gt_id,
>> +					       __I915_SAMPLE_FREQ_ACT),
>>				   USEC_PER_SEC /* to MHz */);
>>			break;
>>		case I915_PMU_REQUESTED_FREQUENCY:
>>			val =
>> -			   div_u64(pmu->sample[__I915_SAMPLE_FREQ_REQ].cur,
>> +			   div_u64(read_sample(pmu, gt_id,
>> +					       __I915_SAMPLE_FREQ_REQ),
>>				   USEC_PER_SEC /* to MHz */);
>>			break;
>>		case I915_PMU_INTERRUPTS:
>>			val = READ_ONCE(pmu->irq_count);
>>			break;
>>		case I915_PMU_RC6_RESIDENCY:
>> -			val = get_rc6(to_gt(i915));
>> +			val = get_rc6(i915->gt[gt_id]);
>>			break;
>>		case I915_PMU_SOFTWARE_GT_AWAKE_TIME:
>>			val = ktime_to_ns(intel_gt_get_awake_time(to_gt(i915)));
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>> index 3a811266ac6a..ea2d24ef5664 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>> @@ -38,13 +38,16 @@ enum {
>>	__I915_NUM_PMU_SAMPLERS
>>  };
>>
>> +#define I915_PMU_MAX_GTS 4
>> +
>>  /*
>>   * How many different events we track in the global PMU mask.
>>   *
>>   * It is also used to know to needed number of event reference counters.
>>   */
>>  #define I915_PMU_MASK_BITS \
>> -	(I915_ENGINE_SAMPLE_COUNT + __I915_PMU_TRACKED_EVENT_COUNT)
>> +	(I915_ENGINE_SAMPLE_COUNT + \
>> +	 I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT)
>>
>>  #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
>>
>> @@ -95,7 +98,7 @@ struct i915_pmu {
>>	 *
>>	 * Low bits are engine samplers and other events continue from there.
>>	 */
>> -	u32 enable;
>> +	u64 enable;
>>
>>	/**
>>	 * @timer_last:
>> @@ -124,11 +127,11 @@ struct i915_pmu {
>>	 * Only global counters are held here, while the per-engine ones are in
>>	 * struct intel_engine_cs.
>>	 */
>> -	struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
>> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
>>	/**
>>	 * @sleep_last: Last time GT parked for RC6 estimation.
>>	 */
>> -	ktime_t sleep_last;
>> +	ktime_t sleep_last[I915_PMU_MAX_GTS];
>>	/**
>>	 * @irq_count: Number of interrupts
>>	 *
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index dba7c5a5b25e..d5ac1fdeb2b1 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -280,7 +280,16 @@ enum drm_i915_pmu_engine_sample {
>>  #define I915_PMU_ENGINE_SEMA(class, instance) \
>>	__I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
>>
>> -#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
>> +/*
>> + * Top 4 bits of every non-engine counter are GT id.
>> + */
>> +#define __I915_PMU_GT_SHIFT (60)
>> +
>> +#define ___I915_PMU_OTHER(gt, x) \
>> +	(((__u64)__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x)) | \
>> +	((__u64)(gt) << __I915_PMU_GT_SHIFT))
>> +
>> +#define __I915_PMU_OTHER(x) ___I915_PMU_OTHER(0, x)
>>
>>  #define I915_PMU_ACTUAL_FREQUENCY	__I915_PMU_OTHER(0)
>>  #define I915_PMU_REQUESTED_FREQUENCY	__I915_PMU_OTHER(1)
>> @@ -290,6 +299,12 @@ enum drm_i915_pmu_engine_sample {
>>
>>  #define I915_PMU_LAST /* Deprecated - do not use */ I915_PMU_RC6_RESIDENCY
>>
>> +#define __I915_PMU_ACTUAL_FREQUENCY(gt)		___I915_PMU_OTHER(gt, 0)
>> +#define __I915_PMU_REQUESTED_FREQUENCY(gt)	___I915_PMU_OTHER(gt, 1)
>> +#define __I915_PMU_INTERRUPTS(gt)		___I915_PMU_OTHER(gt, 2)
>> +#define __I915_PMU_RC6_RESIDENCY(gt)		___I915_PMU_OTHER(gt, 3)
>> +#define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt)	___I915_PMU_OTHER(gt, 4)
>> +
>>  /* Each region is a minimum of 16k, and there are at most 255 of them.
>>   */
>>  #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
>> --
>> 2.36.1
>>

[Intel-gfx] [PATCH 6/6] drm/i915/pmu: Export counters from all tiles

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Start exporting frequency and RC6 counters from all tiles.

Existing counters keep their names and config values and new one use the
namespace added in the previous patch, with the "-gtN" added to their
names.

Interrupts counter is an odd one off. Because it is the global device
counters (not only GT) we choose not to add per tile versions for now.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 82 ++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index b3dd9e51c5cc..12345fd0b2cd 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -927,11 +927,20 @@ static const struct attribute_group i915_pmu_cpumask_attr_group = {
 	.attrs = i915_cpumask_attrs,
 };
 
-#define __event(__config, __name, __unit) \
+#define __event(__counter, __name, __unit) \
 { \
-	.config = (__config), \
+	.counter = (__counter), \
 	.name = (__name), \
 	.unit = (__unit), \
+	.global = false, \
+}
+
+#define __global_event(__counter, __name, __unit) \
+{ \
+	.counter = (__counter), \
+	.name = (__name), \
+	.unit = (__unit), \
+	.global = true, \
 }
 
 #define __engine_event(__sample, __name) \
@@ -970,15 +979,16 @@ create_event_attributes(struct i915_pmu *pmu)
 {
 	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
 	static const struct {
-		u64 config;
+		unsigned int counter;
 		const char *name;
 		const char *unit;
+		bool global;
 	} events[] = {
-		__event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
-		__event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency", "M"),
-		__event(I915_PMU_INTERRUPTS, "interrupts", NULL),
-		__event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
-		__event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time", "ns"),
+		__event(0, "actual-frequency", "M"),
+		__event(1, "requested-frequency", "M"),
+		__global_event(2, "interrupts", NULL),
+		__event(3, "rc6-residency", "ns"),
+		__event(4, "software-gt-awake-time", "ns"),
 	};
 	static const struct {
 		enum drm_i915_pmu_engine_sample sample;
@@ -993,12 +1003,17 @@ create_event_attributes(struct i915_pmu *pmu)
 	struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
 	struct attribute **attr = NULL, **attr_iter;
 	struct intel_engine_cs *engine;
-	unsigned int i;
+	struct intel_gt *gt;
+	unsigned int i, j;
 
 	/* Count how many counters we will be exposing. */
-	for (i = 0; i < ARRAY_SIZE(events); i++) {
-		if (!config_status(i915, events[i].config))
-			count++;
+	for_each_gt(gt, i915, j) {
+		for (i = 0; i < ARRAY_SIZE(events); i++) {
+			u64 config = ___I915_PMU_OTHER(j, events[i].counter);
+
+			if (!config_status(i915, config))
+				count++;
+		}
 	}
 
 	for_each_uabi_engine(engine, i915) {
@@ -1028,26 +1043,39 @@ create_event_attributes(struct i915_pmu *pmu)
 	attr_iter = attr;
 
 	/* Initialize supported non-engine counters. */
-	for (i = 0; i < ARRAY_SIZE(events); i++) {
-		char *str;
-
-		if (config_status(i915, events[i].config))
-			continue;
-
-		str = kstrdup(events[i].name, GFP_KERNEL);
-		if (!str)
-			goto err;
+	for_each_gt(gt, i915, j) {
+		for (i = 0; i < ARRAY_SIZE(events); i++) {
+			u64 config = ___I915_PMU_OTHER(j, events[i].counter);
+			char *str;
 
-		*attr_iter++ = &i915_iter->attr.attr;
-		i915_iter = add_i915_attr(i915_iter, str, events[i].config);
+			if (config_status(i915, config))
+				continue;
 
-		if (events[i].unit) {
-			str = kasprintf(GFP_KERNEL, "%s.unit", events[i].name);
+			if (events[i].global || !HAS_EXTRA_GT_LIST(i915))
+				str = kstrdup(events[i].name, GFP_KERNEL);
+			else
+				str = kasprintf(GFP_KERNEL, "%s-gt%u",
+						events[i].name, j);
 			if (!str)
 				goto err;
 
-			*attr_iter++ = &pmu_iter->attr.attr;
-			pmu_iter = add_pmu_attr(pmu_iter, str, events[i].unit);
+			*attr_iter++ = &i915_iter->attr.attr;
+			i915_iter = add_i915_attr(i915_iter, str, config);
+
+			if (events[i].unit) {
+				if (events[i].global || !HAS_EXTRA_GT_LIST(i915))
+					str = kasprintf(GFP_KERNEL, "%s.unit",
+							events[i].name);
+				else
+					str = kasprintf(GFP_KERNEL, "%s-gt%u.unit",
+							events[i].name, j);
+				if (!str)
+					goto err;
+
+				*attr_iter++ = &pmu_iter->attr.attr;
+				pmu_iter = add_pmu_attr(pmu_iter, str,
+							events[i].unit);
+			}
 		}
 	}
 
-- 
2.36.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Add MTL PMU support for multi-gt (rev3)

[Patchwork]

== Series Details ==

Series: Add MTL PMU support for multi-gt (rev3)
URL   : https://patchwork.freedesktop.org/series/115836/
State : warning

== Summary ==

Error: dim checkpatch failed
676522fb00c9 drm/i915/pmu: Support PMU for all engines
61ea4272bfb6 drm/i915/pmu: Skip sampling engines with no enabled counters
9fe606b911b9 drm/i915/pmu: Transform PMU parking code to be GT based
43f184cc4b6d drm/i915/pmu: Add reference counting to the sampling timer
073311739b04 drm/i915/pmu: Prepare for multi-tile non-engine counters
-:106: WARNING:AVOID_BUG: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants
#106: FILE: drivers/gpu/drm/i915/i915_pmu.c:194:
+	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));

total: 0 errors, 1 warnings, 0 checks, 351 lines checked
a68ee0bfca8f drm/i915/pmu: Export counters from all tiles

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Add MTL PMU support for multi-gt (rev3)

[Patchwork]

== Series Details ==

Series: Add MTL PMU support for multi-gt (rev3)
URL   : https://patchwork.freedesktop.org/series/115836/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for Add MTL PMU support for multi-gt (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7476 bytes --]

== Series Details ==

Series: Add MTL PMU support for multi-gt (rev3)
URL   : https://patchwork.freedesktop.org/series/115836/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13143 -> Patchwork_115836v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_115836v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_115836v3, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/index.html

Participating hosts (38 -> 38)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_115836v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-adls-5:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/bat-adls-5/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-adls-5/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_115836v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#4613]) +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_pm_backlight@basic-brightness@edp-1:
    - bat-rplp-1:         NOTRUN -> [ABORT][5] ([i915#7077])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-rplp-1/igt@i915_pm_backlight@basic-brightness@edp-1.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][6] ([i915#1886] / [i915#7913])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-rpls-2:         NOTRUN -> [ABORT][7] ([i915#6687])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-rpls-2/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][8] ([fdo#109271]) +15 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-1:
    - fi-rkl-11600:       [PASS][9] -> [FAIL][10] ([fdo#103375])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/fi-rkl-11600/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-rkl-11600/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#4579])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-kbl-soraka/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][12] ([i915#5334]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@requests:
    - {bat-mtlp-8}:       [ABORT][14] ([i915#4983] / [i915#7920] / [i915#7953]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/bat-mtlp-8/igt@i915_selftest@live@requests.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-mtlp-8/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         [ABORT][16] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/bat-rpls-2/igt@i915_selftest@live@reset.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-rpls-2/igt@i915_selftest@live@reset.html

  
#### Warnings ####

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-rplp-1:         [ABORT][18] ([i915#4579] / [i915#8260]) -> [SKIP][19] ([i915#3555] / [i915#4579])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13143/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#7953]: https://gitlab.freedesktop.org/drm/intel/issues/7953
  [i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347


Build changes
-------------

  * Linux: CI_DRM_13143 -> Patchwork_115836v3

  CI-20190529: 20190529
  CI_DRM_13143: 222ff19f23b0bd6aca0b52001d69699f78f5a206 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7286: a482779488f11c432d7ddcb1980691ab1603f356 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115836v3: 222ff19f23b0bd6aca0b52001d69699f78f5a206 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

d575c316e782 drm/i915/pmu: Export counters from all tiles
e2bc9d7b3f35 drm/i915/pmu: Prepare for multi-tile non-engine counters
7b85e1a6a2e8 drm/i915/pmu: Add reference counting to the sampling timer
e37b64b29b01 drm/i915/pmu: Transform PMU parking code to be GT based
aeffb17b6995 drm/i915/pmu: Skip sampling engines with no enabled counters
38cf78edfe9d drm/i915/pmu: Support PMU for all engines

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115836v3/index.html

[-- Attachment #2: Type: text/html, Size: 8761 bytes --]

[Intel-gfx] [PATCH v4 0/6] Add MTL PMU support for multi-gt

[Umesh Nerlige Ramappa]

With MTL, frequency and rc6 counters are specific to a gt. Export these
counters via gt-specific events to the user space.

v2: Remove aggregation support from kernel
v3: Review comments (Ashutosh, Tvrtko)
v4:
- Include R-b for 6/6
- Add Test-with
- Fix versioning info in cover letter

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Test-with: 20230513022234.2832233-1-umesh.nerlige.ramappa@intel.com

Tvrtko Ursulin (6):
  drm/i915/pmu: Support PMU for all engines
  drm/i915/pmu: Skip sampling engines with no enabled counters
  drm/i915/pmu: Transform PMU parking code to be GT based
  drm/i915/pmu: Add reference counting to the sampling timer
  drm/i915/pmu: Prepare for multi-tile non-engine counters
  drm/i915/pmu: Export counters from all tiles

 drivers/gpu/drm/i915/gt/intel_gt_pm.c |   4 +-
 drivers/gpu/drm/i915/i915_pmu.c       | 263 ++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_pmu.h       |  24 ++-
 include/uapi/drm/i915_drm.h           |  17 +-
 4 files changed, 219 insertions(+), 89 deletions(-)

-- 
2.36.1

[Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We do not want to have timers per tile and waste CPU cycles and energy via
multiple wake-up sources, for a relatively un-important task of PMU
sampling, so keeping a single timer works well. But we also do not want
the first GT which goes idle to turn off the timer.

Add some reference counting, via a mask of unparked GTs, to solve this.

v2: Drop the check for unparked in i915_sample (Ashutosh)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++--
 drivers/gpu/drm/i915/i915_pmu.h | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 2b63ee31e1b3..725b01b00775 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
 	 * Signal sampling timer to stop if only engine events are enabled and
 	 * GPU went idle.
 	 */
-	pmu->timer_enabled = pmu_needs_timer(pmu, false);
+	pmu->unparked &= ~BIT(gt->info.id);
+	if (pmu->unparked == 0)
+		pmu->timer_enabled = pmu_needs_timer(pmu, false);
 
 	spin_unlock_irq(&pmu->lock);
 }
@@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
 	/*
 	 * Re-enable sampling timer when GPU goes active.
 	 */
-	__i915_pmu_maybe_start_timer(pmu);
+	if (pmu->unparked == 0)
+		__i915_pmu_maybe_start_timer(pmu);
+
+	pmu->unparked |= BIT(gt->info.id);
 
 	spin_unlock_irq(&pmu->lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index a686fd7ccedf..3a811266ac6a 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -76,6 +76,10 @@ struct i915_pmu {
 	 * @lock: Lock protecting enable mask and ref count handling.
 	 */
 	spinlock_t lock;
+	/**
+	 * @unparked: GT unparked mask.
+	 */
+	unsigned int unparked;
 	/**
 	 * @timer: Timer for internal i915 PMU sampling.
 	 */
-- 
2.36.1

[Intel-gfx] [PATCH 0/6] Add MTL PMU support for multi-gt

[Umesh Nerlige Ramappa]

With MTL, frequency and rc6 counters are specific to a gt. Export these
counters via gt-specific events to the user space.

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Test-with: 20230506005528.1890922-1-umesh.nerlige.ramappa@intel.com 

Tvrtko Ursulin (6):
  drm/i915/pmu: Support PMU for all engines
  drm/i915/pmu: Skip sampling engines with no enabled counters
  drm/i915/pmu: Transform PMU parking code to be GT based
  drm/i915/pmu: Add reference counting to the sampling timer
  drm/i915/pmu: Prepare for multi-tile non-engine counters
  drm/i915/pmu: Export counters from all tiles

 drivers/gpu/drm/i915/gt/intel_gt_pm.c |   4 +-
 drivers/gpu/drm/i915/i915_pmu.c       | 271 ++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_pmu.h       |  22 ++-
 include/uapi/drm/i915_drm.h           |  17 +-
 4 files changed, 225 insertions(+), 89 deletions(-)

-- 
2.36.1

[Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We do not want to have timers per tile and waste CPU cycles and energy via
multiple wake-up sources, for a relatively un-important task of PMU
sampling, so keeping a single timer works well. But we also do not want
the first GT which goes idle to turn off the timer.

Add some reference counting, via a mask of unparked GTs, to solve this.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 2b63ee31e1b3..669a42e44082 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
 	 * Signal sampling timer to stop if only engine events are enabled and
 	 * GPU went idle.
 	 */
-	pmu->timer_enabled = pmu_needs_timer(pmu, false);
+	pmu->unparked &= ~BIT(gt->info.id);
+	if (pmu->unparked == 0)
+		pmu->timer_enabled = pmu_needs_timer(pmu, false);
 
 	spin_unlock_irq(&pmu->lock);
 }
@@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
 	/*
 	 * Re-enable sampling timer when GPU goes active.
 	 */
-	__i915_pmu_maybe_start_timer(pmu);
+	if (pmu->unparked == 0)
+		__i915_pmu_maybe_start_timer(pmu);
+
+	pmu->unparked |= BIT(gt->info.id);
 
 	spin_unlock_irq(&pmu->lock);
 }
@@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 	 */
 
 	for_each_gt(gt, i915, i) {
+		if (!(pmu->unparked & BIT(i)))
+			continue;
+
 		engines_sample(gt, period_ns);
 
 		if (i == 0) /* FIXME */
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index a686fd7ccedf..3a811266ac6a 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -76,6 +76,10 @@ struct i915_pmu {
 	 * @lock: Lock protecting enable mask and ref count handling.
 	 */
 	spinlock_t lock;
+	/**
+	 * @unparked: GT unparked mask.
+	 */
+	unsigned int unparked;
 	/**
 	 * @timer: Timer for internal i915 PMU sampling.
 	 */
-- 
2.36.1

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

On Fri, May 05, 2023 at 05:58:14PM -0700, Umesh Nerlige Ramappa wrote:
>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
>We do not want to have timers per tile and waste CPU cycles and energy via
>multiple wake-up sources, for a relatively un-important task of PMU
>sampling, so keeping a single timer works well. But we also do not want
>the first GT which goes idle to turn off the timer.
>
>Add some reference counting, via a mask of unparked GTs, to solve this.

Looks like the previous patch is a prep work for this one. I would 
mention something about this patch in the previous patch, but then I am 
not sure what's the norm in these scenarios. Recently I created some IGT 
patches that are prep work and refer to future patches in the series.

As is, this patch is 

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks,
Umesh

>
>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>---
> drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
> drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>index 2b63ee31e1b3..669a42e44082 100644
>--- a/drivers/gpu/drm/i915/i915_pmu.c
>+++ b/drivers/gpu/drm/i915/i915_pmu.c
>@@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
> 	 * Signal sampling timer to stop if only engine events are enabled and
> 	 * GPU went idle.
> 	 */
>-	pmu->timer_enabled = pmu_needs_timer(pmu, false);
>+	pmu->unparked &= ~BIT(gt->info.id);
>+	if (pmu->unparked == 0)
>+		pmu->timer_enabled = pmu_needs_timer(pmu, false);
>
> 	spin_unlock_irq(&pmu->lock);
> }
>@@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
> 	/*
> 	 * Re-enable sampling timer when GPU goes active.
> 	 */
>-	__i915_pmu_maybe_start_timer(pmu);
>+	if (pmu->unparked == 0)
>+		__i915_pmu_maybe_start_timer(pmu);
>+
>+	pmu->unparked |= BIT(gt->info.id);
>
> 	spin_unlock_irq(&pmu->lock);
> }
>@@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
> 	 */
>
> 	for_each_gt(gt, i915, i) {
>+		if (!(pmu->unparked & BIT(i)))
>+			continue;
>+
> 		engines_sample(gt, period_ns);
>
> 		if (i == 0) /* FIXME */
>diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>index a686fd7ccedf..3a811266ac6a 100644
>--- a/drivers/gpu/drm/i915/i915_pmu.h
>+++ b/drivers/gpu/drm/i915/i915_pmu.h
>@@ -76,6 +76,10 @@ struct i915_pmu {
> 	 * @lock: Lock protecting enable mask and ref count handling.
> 	 */
> 	spinlock_t lock;
>+	/**
>+	 * @unparked: GT unparked mask.
>+	 */
>+	unsigned int unparked;
> 	/**
> 	 * @timer: Timer for internal i915 PMU sampling.
> 	 */
>-- 
>2.36.1
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We do not want to have timers per tile and waste CPU cycles and energy via
> multiple wake-up sources, for a relatively un-important task of PMU
> sampling, so keeping a single timer works well. But we also do not want
> the first GT which goes idle to turn off the timer.

Apart from this efficiency, what is the reason for having a device level
PMU (which monitors gt level events), rather than independent gt level
PMU's (each of which monitor events from that gt)?

Wouldn't independent gt level PMU's be simpler? And user space tools (say
intel-gpu-top) would hook into events from a gt and treat each gt
independently?

So my question really is what is the reason for keeping the PMU device
level rather than per gt?

Thanks.
--
Ashutosh

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Tue, 09 May 2023 10:25:16 -0700, Dixit, Ashutosh wrote:
>
> On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
> >
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > We do not want to have timers per tile and waste CPU cycles and energy via
> > multiple wake-up sources, for a relatively un-important task of PMU
> > sampling, so keeping a single timer works well. But we also do not want
> > the first GT which goes idle to turn off the timer.
>
> Apart from this efficiency, what is the reason for having a device level
> PMU (which monitors gt level events), rather than independent gt level
> PMU's (each of which monitor events from that gt)?
>
> Wouldn't independent gt level PMU's be simpler? And user space tools (say
> intel-gpu-top) would hook into events from a gt and treat each gt
> independently?
>
> So my question really is what is the reason for keeping the PMU device
> level rather than per gt?

Maybe ignore this for now, the way it is expressed it is too open
ended. Let me get a better handle on the code and the patches and I'll see
if I have anything to say.

Thanks.
--
Ashutosh

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh/Tvrtko,

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We do not want to have timers per tile and waste CPU cycles and energy via
> multiple wake-up sources, for a relatively un-important task of PMU
> sampling, so keeping a single timer works well. But we also do not want
> the first GT which goes idle to turn off the timer.
>
> Add some reference counting, via a mask of unparked GTs, to solve this.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 2b63ee31e1b3..669a42e44082 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>	 * Signal sampling timer to stop if only engine events are enabled and
>	 * GPU went idle.
>	 */
> -	pmu->timer_enabled = pmu_needs_timer(pmu, false);
> +	pmu->unparked &= ~BIT(gt->info.id);
> +	if (pmu->unparked == 0)
> +		pmu->timer_enabled = pmu_needs_timer(pmu, false);
>
>	spin_unlock_irq(&pmu->lock);
>  }
> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>	/*
>	 * Re-enable sampling timer when GPU goes active.
>	 */
> -	__i915_pmu_maybe_start_timer(pmu);
> +	if (pmu->unparked == 0)
> +		__i915_pmu_maybe_start_timer(pmu);
> +
> +	pmu->unparked |= BIT(gt->info.id);
>
>	spin_unlock_irq(&pmu->lock);
>  }
> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>	 */
>
>	for_each_gt(gt, i915, i) {
> +		if (!(pmu->unparked & BIT(i)))
> +			continue;
> +

This is not correct. In this series we are at least sampling frequencies
(calling frequency_sample) even when GT is parked. So these 3 lines should be
deleted. engines_sample will get called and will return without doing
anything if engine events are disabled.

Thanks.
--
Ashutosh


>		engines_sample(gt, period_ns);
>
>		if (i == 0) /* FIXME */
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index a686fd7ccedf..3a811266ac6a 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -76,6 +76,10 @@ struct i915_pmu {
>	 * @lock: Lock protecting enable mask and ref count handling.
>	 */
>	spinlock_t lock;
> +	/**
> +	 * @unparked: GT unparked mask.
> +	 */
> +	unsigned int unparked;
>	/**
>	 * @timer: Timer for internal i915 PMU sampling.
>	 */
> --
> 2.36.1
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
>On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh/Tvrtko,
>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> We do not want to have timers per tile and waste CPU cycles and energy via
>> multiple wake-up sources, for a relatively un-important task of PMU
>> sampling, so keeping a single timer works well. But we also do not want
>> the first GT which goes idle to turn off the timer.
>>
>> Add some reference counting, via a mask of unparked GTs, to solve this.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
>>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index 2b63ee31e1b3..669a42e44082 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>>	 * Signal sampling timer to stop if only engine events are enabled and
>>	 * GPU went idle.
>>	 */
>> -	pmu->timer_enabled = pmu_needs_timer(pmu, false);
>> +	pmu->unparked &= ~BIT(gt->info.id);
>> +	if (pmu->unparked == 0)
>> +		pmu->timer_enabled = pmu_needs_timer(pmu, false);
>>
>>	spin_unlock_irq(&pmu->lock);
>>  }
>> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>>	/*
>>	 * Re-enable sampling timer when GPU goes active.
>>	 */
>> -	__i915_pmu_maybe_start_timer(pmu);
>> +	if (pmu->unparked == 0)
>> +		__i915_pmu_maybe_start_timer(pmu);
>> +
>> +	pmu->unparked |= BIT(gt->info.id);
>>
>>	spin_unlock_irq(&pmu->lock);
>>  }
>> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>>	 */
>>
>>	for_each_gt(gt, i915, i) {
>> +		if (!(pmu->unparked & BIT(i)))
>> +			continue;
>> +
>
>This is not correct. In this series we are at least sampling frequencies
>(calling frequency_sample) even when GT is parked. So these 3 lines should be
>deleted. engines_sample will get called and will return without doing
>anything if engine events are disabled.

Not sure I understand. This is checking pmu->'un'parked bits.

Thanks,
Umesh
>
>Thanks.
>--
>Ashutosh
>
>
>>		engines_sample(gt, period_ns);
>>
>>		if (i == 0) /* FIXME */
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>> index a686fd7ccedf..3a811266ac6a 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>> @@ -76,6 +76,10 @@ struct i915_pmu {
>>	 * @lock: Lock protecting enable mask and ref count handling.
>>	 */
>>	spinlock_t lock;
>> +	/**
>> +	 * @unparked: GT unparked mask.
>> +	 */
>> +	unsigned int unparked;
>>	/**
>>	 * @timer: Timer for internal i915 PMU sampling.
>>	 */
>> --
>> 2.36.1
>>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
>
> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
> > On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >
> > Hi Umesh/Tvrtko,
> >
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> We do not want to have timers per tile and waste CPU cycles and energy via
> >> multiple wake-up sources, for a relatively un-important task of PMU
> >> sampling, so keeping a single timer works well. But we also do not want
> >> the first GT which goes idle to turn off the timer.
> >>
> >> Add some reference counting, via a mask of unparked GTs, to solve this.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
> >>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >> index 2b63ee31e1b3..669a42e44082 100644
> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
> >>	 * Signal sampling timer to stop if only engine events are enabled and
> >>	 * GPU went idle.
> >>	 */
> >> -	pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >> +	pmu->unparked &= ~BIT(gt->info.id);
> >> +	if (pmu->unparked == 0)
> >> +		pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >>
> >>	spin_unlock_irq(&pmu->lock);
> >>  }
> >> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
> >>	/*
> >>	 * Re-enable sampling timer when GPU goes active.
> >>	 */
> >> -	__i915_pmu_maybe_start_timer(pmu);
> >> +	if (pmu->unparked == 0)
> >> +		__i915_pmu_maybe_start_timer(pmu);
> >> +
> >> +	pmu->unparked |= BIT(gt->info.id);
> >>
> >>	spin_unlock_irq(&pmu->lock);
> >>  }
> >> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
> >>	 */
> >>
> >>	for_each_gt(gt, i915, i) {
> >> +		if (!(pmu->unparked & BIT(i)))
> >> +			continue;
> >> +
> >
> > This is not correct. In this series we are at least sampling frequencies
> > (calling frequency_sample) even when GT is parked. So these 3 lines should be
> > deleted. engines_sample will get called and will return without doing
> > anything if engine events are disabled.
>
> Not sure I understand. This is checking pmu->'un'parked bits.

Sorry, my bad. Not "engines_sample will get called and will return without
doing anything if engine events are disabled" but "engines_sample will get
called and will return without doing anything if GT is not awake". This is
the same as the previous behavior before this series.

Umesh and I discussed this but writing this out in case Tvrtko takes a
look.

Thanks.
--
Ashutosh



> >
> >
> >>		engines_sample(gt, period_ns);
> >>
> >>		if (i == 0) /* FIXME */
> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> >> index a686fd7ccedf..3a811266ac6a 100644
> >> --- a/drivers/gpu/drm/i915/i915_pmu.h
> >> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> >> @@ -76,6 +76,10 @@ struct i915_pmu {
> >>	 * @lock: Lock protecting enable mask and ref count handling.
> >>	 */
> >>	spinlock_t lock;
> >> +	/**
> >> +	 * @unparked: GT unparked mask.
> >> +	 */
> >> +	unsigned int unparked;
> >>	/**
> >>	 * @timer: Timer for internal i915 PMU sampling.
> >>	 */
> >> --
> >> 2.36.1
> >>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Umesh Nerlige Ramappa]

On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote:
>On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
>>
>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
>> > On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>> >>
>> >
>> > Hi Umesh/Tvrtko,
>> >
>> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >>
>> >> We do not want to have timers per tile and waste CPU cycles and energy via
>> >> multiple wake-up sources, for a relatively un-important task of PMU
>> >> sampling, so keeping a single timer works well. But we also do not want
>> >> the first GT which goes idle to turn off the timer.
>> >>
>> >> Add some reference counting, via a mask of unparked GTs, to solve this.
>> >>
>> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
>> >>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>> >>  2 files changed, 14 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> >> index 2b63ee31e1b3..669a42e44082 100644
>> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> >> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>> >>	 * Signal sampling timer to stop if only engine events are enabled and
>> >>	 * GPU went idle.
>> >>	 */
>> >> -	pmu->timer_enabled = pmu_needs_timer(pmu, false);
>> >> +	pmu->unparked &= ~BIT(gt->info.id);
>> >> +	if (pmu->unparked == 0)
>> >> +		pmu->timer_enabled = pmu_needs_timer(pmu, false);
>> >>
>> >>	spin_unlock_irq(&pmu->lock);
>> >>  }
>> >> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>> >>	/*
>> >>	 * Re-enable sampling timer when GPU goes active.
>> >>	 */
>> >> -	__i915_pmu_maybe_start_timer(pmu);
>> >> +	if (pmu->unparked == 0)
>> >> +		__i915_pmu_maybe_start_timer(pmu);
>> >> +
>> >> +	pmu->unparked |= BIT(gt->info.id);
>> >>
>> >>	spin_unlock_irq(&pmu->lock);
>> >>  }
>> >> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>> >>	 */
>> >>
>> >>	for_each_gt(gt, i915, i) {
>> >> +		if (!(pmu->unparked & BIT(i)))
>> >> +			continue;
>> >> +
>> >
>> > This is not correct. In this series we are at least sampling frequencies
>> > (calling frequency_sample) even when GT is parked. So these 3 lines should be
>> > deleted. engines_sample will get called and will return without doing
>> > anything if engine events are disabled.
>>
>> Not sure I understand. This is checking pmu->'un'parked bits.
>
>Sorry, my bad. Not "engines_sample will get called and will return without
>doing anything if engine events are disabled" but "engines_sample will get
>called and will return without doing anything if GT is not awake". This is
>the same as the previous behavior before this series.
>
>Umesh and I discussed this but writing this out in case Tvrtko takes a
>look.

Sounds good, Dropping the check here in the new revision.

Thanks,
Umesh
>
>Thanks.
>--
>Ashutosh
>
>
>
>> >
>> >
>> >>		engines_sample(gt, period_ns);
>> >>
>> >>		if (i == 0) /* FIXME */
>> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>> >> index a686fd7ccedf..3a811266ac6a 100644
>> >> --- a/drivers/gpu/drm/i915/i915_pmu.h
>> >> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>> >> @@ -76,6 +76,10 @@ struct i915_pmu {
>> >>	 * @lock: Lock protecting enable mask and ref count handling.
>> >>	 */
>> >>	spinlock_t lock;
>> >> +	/**
>> >> +	 * @unparked: GT unparked mask.
>> >> +	 */
>> >> +	unsigned int unparked;
>> >>	/**
>> >>	 * @timer: Timer for internal i915 PMU sampling.
>> >>	 */
>> >> --
>> >> 2.36.1
>> >>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Tvrtko Ursulin]

On 13/05/2023 00:44, Umesh Nerlige Ramappa wrote:
> On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote:
>> On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
>>>
>>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
>>> > On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>>> >>
>>> >
>>> > Hi Umesh/Tvrtko,
>>> >
>>> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> >>
>>> >> We do not want to have timers per tile and waste CPU cycles and 
>>> energy via
>>> >> multiple wake-up sources, for a relatively un-important task of PMU
>>> >> sampling, so keeping a single timer works well. But we also do not 
>>> want
>>> >> the first GT which goes idle to turn off the timer.
>>> >>
>>> >> Add some reference counting, via a mask of unparked GTs, to solve 
>>> this.
>>> >>
>>> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> >> Signed-off-by: Umesh Nerlige Ramappa 
>>> <umesh.nerlige.ramappa@intel.com>
>>> >> ---
>>> >>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
>>> >>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>> >>  2 files changed, 14 insertions(+), 2 deletions(-)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c 
>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>> >> index 2b63ee31e1b3..669a42e44082 100644
>>> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> >> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>>> >>     * Signal sampling timer to stop if only engine events are 
>>> enabled and
>>> >>     * GPU went idle.
>>> >>     */
>>> >> -    pmu->timer_enabled = pmu_needs_timer(pmu, false);
>>> >> +    pmu->unparked &= ~BIT(gt->info.id);
>>> >> +    if (pmu->unparked == 0)
>>> >> +        pmu->timer_enabled = pmu_needs_timer(pmu, false);
>>> >>
>>> >>    spin_unlock_irq(&pmu->lock);
>>> >>  }
>>> >> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>>> >>    /*
>>> >>     * Re-enable sampling timer when GPU goes active.
>>> >>     */
>>> >> -    __i915_pmu_maybe_start_timer(pmu);
>>> >> +    if (pmu->unparked == 0)
>>> >> +        __i915_pmu_maybe_start_timer(pmu);
>>> >> +
>>> >> +    pmu->unparked |= BIT(gt->info.id);
>>> >>
>>> >>    spin_unlock_irq(&pmu->lock);
>>> >>  }
>>> >> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct 
>>> hrtimer *hrtimer)
>>> >>     */
>>> >>
>>> >>    for_each_gt(gt, i915, i) {
>>> >> +        if (!(pmu->unparked & BIT(i)))
>>> >> +            continue;
>>> >> +
>>> >
>>> > This is not correct. In this series we are at least sampling 
>>> frequencies
>>> > (calling frequency_sample) even when GT is parked. So these 3 lines 
>>> should be
>>> > deleted. engines_sample will get called and will return without doing
>>> > anything if engine events are disabled.
>>>
>>> Not sure I understand. This is checking pmu->'un'parked bits.
>>
>> Sorry, my bad. Not "engines_sample will get called and will return 
>> without
>> doing anything if engine events are disabled" but "engines_sample will 
>> get
>> called and will return without doing anything if GT is not awake". 
>> This is
>> the same as the previous behavior before this series.
>>
>> Umesh and I discussed this but writing this out in case Tvrtko takes a
>> look.
> 
> Sounds good, Dropping the check here in the new revision.

I think it is safe to not have the check, but I didn't quite understand 
the "this is not correct" part. I can only see the argument that it 
could be redundant, not that it is incorrect.

In which case I think it should better stay since it is way more 
efficient, given this gets called at 200Hz, than the *atomic* 
atomic_inc_not_zero (from intel_wakeref_get_if_active).

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Mon, 15 May 2023 02:52:35 -0700, Tvrtko Ursulin wrote:
>
> On 13/05/2023 00:44, Umesh Nerlige Ramappa wrote:
> > On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote:
> >> On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
> >>>
> >>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
> >>> > On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
> >>> >>
> >>> >
> >>> > Hi Umesh/Tvrtko,
> >>> >
> >>> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> >>
> >>> >> We do not want to have timers per tile and waste CPU cycles and
> >>> energy via
> >>> >> multiple wake-up sources, for a relatively un-important task of PMU
> >>> >> sampling, so keeping a single timer works well. But we also do not
> >>> want
> >>> >> the first GT which goes idle to turn off the timer.
> >>> >>
> >>> >> Add some reference counting, via a mask of unparked GTs, to solve
> >>> this.
> >>> >>
> >>> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> >> Signed-off-by: Umesh Nerlige Ramappa
> >>> <umesh.nerlige.ramappa@intel.com>
> >>> >> ---
> >>> >>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
> >>> >>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
> >>> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>> >>
> >>> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>> >> index 2b63ee31e1b3..669a42e44082 100644
> >>> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>> >> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
> >>> >>     * Signal sampling timer to stop if only engine events are
> >>> enabled and
> >>> >>     * GPU went idle.
> >>> >>     */
> >>> >> -    pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >>> >> +    pmu->unparked &= ~BIT(gt->info.id);
> >>> >> +    if (pmu->unparked == 0)
> >>> >> +        pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >>> >>
> >>> >>    spin_unlock_irq(&pmu->lock);
> >>> >>  }
> >>> >> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
> >>> >>    /*
> >>> >>     * Re-enable sampling timer when GPU goes active.
> >>> >>     */
> >>> >> -    __i915_pmu_maybe_start_timer(pmu);
> >>> >> +    if (pmu->unparked == 0)
> >>> >> +        __i915_pmu_maybe_start_timer(pmu);
> >>> >> +
> >>> >> +    pmu->unparked |= BIT(gt->info.id);
> >>> >>
> >>> >>    spin_unlock_irq(&pmu->lock);
> >>> >>  }
> >>> >> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct
> >>> hrtimer *hrtimer)
> >>> >>     */
> >>> >>
> >>> >>    for_each_gt(gt, i915, i) {
> >>> >> +        if (!(pmu->unparked & BIT(i)))
> >>> >> +            continue;
> >>> >> +
> >>> >
> >>> > This is not correct. In this series we are at least sampling
> >>> frequencies
> >>> > (calling frequency_sample) even when GT is parked. So these 3 lines
> >>> should be
> >>> > deleted. engines_sample will get called and will return without doing
> >>> > anything if engine events are disabled.
> >>>
> >>> Not sure I understand. This is checking pmu->'un'parked bits.
> >>
> >> Sorry, my bad. Not "engines_sample will get called and will return
> >> without
> >> doing anything if engine events are disabled" but "engines_sample will
> >> get
> >> called and will return without doing anything if GT is not awake". This
> >> is
> >> the same as the previous behavior before this series.
> >>
> >> Umesh and I discussed this but writing this out in case Tvrtko takes a
> >> look.
> >
> > Sounds good, Dropping the check here in the new revision.

Hi Tvrtko,

> I think it is safe to not have the check, but I didn't quite understand the
> "this is not correct" part. I can only see the argument that it could be
> redundant, not that it is incorrect.

I said that it looks incorrect to me because in this series we are still
sampling freq when gt is parked and we would be skipping that if we
included:
		if (!(pmu->unparked & BIT(i)))
			continue;

> In which case I think it should better stay since it is way more efficient,
> given this gets called at 200Hz, than the *atomic* atomic_inc_not_zero
> (from intel_wakeref_get_if_active).

Where efficiency goes, when we merge the patch below (I have a v2 based on
your suggestion but I am waiting till Umesh's series gets merged):

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

this will turn off the timer itself which will be even more
efficient. Rather than use the above code where the timer is running and
then we skip. So after the link above is merged the above code will be
truly redundant. That was a second reason why I said delete it.

Thanks.
--
Ashutosh

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Tvrtko Ursulin]

On 15/05/2023 22:24, Dixit, Ashutosh wrote:
> On Mon, 15 May 2023 02:52:35 -0700, Tvrtko Ursulin wrote:
>>
>> On 13/05/2023 00:44, Umesh Nerlige Ramappa wrote:
>>> On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote:
>>>> On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
>>>>>
>>>>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
>>>>>> On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
>>>>>>>
>>>>>>
>>>>>> Hi Umesh/Tvrtko,
>>>>>>
>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>
>>>>>>> We do not want to have timers per tile and waste CPU cycles and
>>>>> energy via
>>>>>>> multiple wake-up sources, for a relatively un-important task of PMU
>>>>>>> sampling, so keeping a single timer works well. But we also do not
>>>>> want
>>>>>>> the first GT which goes idle to turn off the timer.
>>>>>>>
>>>>>>> Add some reference counting, via a mask of unparked GTs, to solve
>>>>> this.
>>>>>>>
>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> Signed-off-by: Umesh Nerlige Ramappa
>>>>> <umesh.nerlige.ramappa@intel.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
>>>>>>>    drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>>>>>>    2 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>> index 2b63ee31e1b3..669a42e44082 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>>>>>>>       * Signal sampling timer to stop if only engine events are
>>>>> enabled and
>>>>>>>       * GPU went idle.
>>>>>>>       */
>>>>>>> -    pmu->timer_enabled = pmu_needs_timer(pmu, false);
>>>>>>> +    pmu->unparked &= ~BIT(gt->info.id);
>>>>>>> +    if (pmu->unparked == 0)
>>>>>>> +        pmu->timer_enabled = pmu_needs_timer(pmu, false);
>>>>>>>
>>>>>>>      spin_unlock_irq(&pmu->lock);
>>>>>>>    }
>>>>>>> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>>>>>>>      /*
>>>>>>>       * Re-enable sampling timer when GPU goes active.
>>>>>>>       */
>>>>>>> -    __i915_pmu_maybe_start_timer(pmu);
>>>>>>> +    if (pmu->unparked == 0)
>>>>>>> +        __i915_pmu_maybe_start_timer(pmu);
>>>>>>> +
>>>>>>> +    pmu->unparked |= BIT(gt->info.id);
>>>>>>>
>>>>>>>      spin_unlock_irq(&pmu->lock);
>>>>>>>    }
>>>>>>> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct
>>>>> hrtimer *hrtimer)
>>>>>>>       */
>>>>>>>
>>>>>>>      for_each_gt(gt, i915, i) {
>>>>>>> +        if (!(pmu->unparked & BIT(i)))
>>>>>>> +            continue;
>>>>>>> +
>>>>>>
>>>>>> This is not correct. In this series we are at least sampling
>>>>> frequencies
>>>>>> (calling frequency_sample) even when GT is parked. So these 3 lines
>>>>> should be
>>>>>> deleted. engines_sample will get called and will return without doing
>>>>>> anything if engine events are disabled.
>>>>>
>>>>> Not sure I understand. This is checking pmu->'un'parked bits.
>>>>
>>>> Sorry, my bad. Not "engines_sample will get called and will return
>>>> without
>>>> doing anything if engine events are disabled" but "engines_sample will
>>>> get
>>>> called and will return without doing anything if GT is not awake". This
>>>> is
>>>> the same as the previous behavior before this series.
>>>>
>>>> Umesh and I discussed this but writing this out in case Tvrtko takes a
>>>> look.
>>>
>>> Sounds good, Dropping the check here in the new revision.
> 
> Hi Tvrtko,
> 
>> I think it is safe to not have the check, but I didn't quite understand the
>> "this is not correct" part. I can only see the argument that it could be
>> redundant, not that it is incorrect.
> 
> I said that it looks incorrect to me because in this series we are still
> sampling freq when gt is parked and we would be skipping that if we
> included:
> 		if (!(pmu->unparked & BIT(i)))
> 			continue;

Ah okay. We concluded in your upstream patch that looks like an omission.

>> In which case I think it should better stay since it is way more efficient,
>> given this gets called at 200Hz, than the *atomic* atomic_inc_not_zero
>> (from intel_wakeref_get_if_active).
> 
> Where efficiency goes, when we merge the patch below (I have a v2 based on
> your suggestion but I am waiting till Umesh's series gets merged):
> 
> https://patchwork.freedesktop.org/series/117658/
> 
> this will turn off the timer itself which will be even more
> efficient. Rather than use the above code where the timer is running and
> then we skip. So after the link above is merged the above code will be
> truly redundant. That was a second reason why I said delete it.

On multi-tile where not all tiles are being looked at it still pays to 
avoid the atomic check. It doesn't apply to tools like intel_gpu_top, 
which monitor all tiles, but still I think there isn't any harm in 
having the fast check.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer

[Dixit, Ashutosh]

On Tue, 16 May 2023 00:12:45 -0700, Tvrtko Ursulin wrote:
>
> On 15/05/2023 22:24, Dixit, Ashutosh wrote:
> > On Mon, 15 May 2023 02:52:35 -0700, Tvrtko Ursulin wrote:
> >>
> >> On 13/05/2023 00:44, Umesh Nerlige Ramappa wrote:
> >>> On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote:
> >>>> On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote:
> >>>>>
> >>>>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote:
> >>>>>> On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote:
> >>>>>>>
> >>>>>>
> >>>>>> Hi Umesh/Tvrtko,
> >>>>>>
> >>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>>
> >>>>>>> We do not want to have timers per tile and waste CPU cycles and
> >>>>> energy via
> >>>>>>> multiple wake-up sources, for a relatively un-important task of PMU
> >>>>>>> sampling, so keeping a single timer works well. But we also do not
> >>>>> want
> >>>>>>> the first GT which goes idle to turn off the timer.
> >>>>>>>
> >>>>>>> Add some reference counting, via a mask of unparked GTs, to solve
> >>>>> this.
> >>>>>>>
> >>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>> Signed-off-by: Umesh Nerlige Ramappa
> >>>>> <umesh.nerlige.ramappa@intel.com>
> >>>>>>> ---
> >>>>>>>    drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++--
> >>>>>>>    drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
> >>>>>>>    2 files changed, 14 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>>>> index 2b63ee31e1b3..669a42e44082 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>>>> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
> >>>>>>>       * Signal sampling timer to stop if only engine events are
> >>>>> enabled and
> >>>>>>>       * GPU went idle.
> >>>>>>>       */
> >>>>>>> -    pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >>>>>>> +    pmu->unparked &= ~BIT(gt->info.id);
> >>>>>>> +    if (pmu->unparked == 0)
> >>>>>>> +        pmu->timer_enabled = pmu_needs_timer(pmu, false);
> >>>>>>>
> >>>>>>>      spin_unlock_irq(&pmu->lock);
> >>>>>>>    }
> >>>>>>> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
> >>>>>>>      /*
> >>>>>>>       * Re-enable sampling timer when GPU goes active.
> >>>>>>>       */
> >>>>>>> -    __i915_pmu_maybe_start_timer(pmu);
> >>>>>>> +    if (pmu->unparked == 0)
> >>>>>>> +        __i915_pmu_maybe_start_timer(pmu);
> >>>>>>> +
> >>>>>>> +    pmu->unparked |= BIT(gt->info.id);
> >>>>>>>
> >>>>>>>      spin_unlock_irq(&pmu->lock);
> >>>>>>>    }
> >>>>>>> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct
> >>>>> hrtimer *hrtimer)
> >>>>>>>       */
> >>>>>>>
> >>>>>>>      for_each_gt(gt, i915, i) {
> >>>>>>> +        if (!(pmu->unparked & BIT(i)))
> >>>>>>> +            continue;
> >>>>>>> +
> >>>>>>
> >>>>>> This is not correct. In this series we are at least sampling
> >>>>> frequencies
> >>>>>> (calling frequency_sample) even when GT is parked. So these 3 lines
> >>>>> should be
> >>>>>> deleted. engines_sample will get called and will return without doing
> >>>>>> anything if engine events are disabled.
> >>>>>
> >>>>> Not sure I understand. This is checking pmu->'un'parked bits.
> >>>>
> >>>> Sorry, my bad. Not "engines_sample will get called and will return
> >>>> without
> >>>> doing anything if engine events are disabled" but "engines_sample will
> >>>> get
> >>>> called and will return without doing anything if GT is not awake". This
> >>>> is
> >>>> the same as the previous behavior before this series.
> >>>>
> >>>> Umesh and I discussed this but writing this out in case Tvrtko takes a
> >>>> look.
> >>>
> >>> Sounds good, Dropping the check here in the new revision.
> >
> > Hi Tvrtko,
> >
> >> I think it is safe to not have the check, but I didn't quite understand the
> >> "this is not correct" part. I can only see the argument that it could be
> >> redundant, not that it is incorrect.
> >
> > I said that it looks incorrect to me because in this series we are still
> > sampling freq when gt is parked and we would be skipping that if we
> > included:
> >		if (!(pmu->unparked & BIT(i)))
> >			continue;
>
> Ah okay. We concluded in your upstream patch that looks like an omission.
>
> >> In which case I think it should better stay since it is way more efficient,
> >> given this gets called at 200Hz, than the *atomic* atomic_inc_not_zero
> >> (from intel_wakeref_get_if_active).
> >
> > Where efficiency goes, when we merge the patch below (I have a v2 based on
> > your suggestion but I am waiting till Umesh's series gets merged):
> >
> > https://patchwork.freedesktop.org/series/117658/
> >
> > this will turn off the timer itself which will be even more
> > efficient. Rather than use the above code where the timer is running and
> > then we skip. So after the link above is merged the above code will be
> > truly redundant. That was a second reason why I said delete it.
>
> On multi-tile where not all tiles are being looked at it still pays to
> avoid the atomic check.

Ah ok I overlooked there was a single timer shared between multiple tiles.

> It doesn't apply to tools like intel_gpu_top, which
> monitor all tiles, but still I think there isn't any harm in having the
> fast check.

I think in that case the simplest would be to just put this code back (as
it was in Tvrtko's original patch) and not worry about sampling the freq's
when gt is parked, since we want to stop doing that anyway.

So let's just put this code back.

Thanks.
--
Ashutosh