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

[Intel-gfx] [PATCH v5 0/7] 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
v5:
- Include "drm/i915/pmu: Change bitmask of enabled events to u32"

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

Tvrtko Ursulin (7):
  drm/i915/pmu: Change bitmask of enabled events to u32
  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       | 292 ++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_pmu.h       |  24 ++-
 include/uapi/drm/i915_drm.h           |  17 +-
 4 files changed, 240 insertions(+), 97 deletions(-)

-- 
2.36.1

[Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Umesh Nerlige Ramappa]

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

Having it as u64 was a confusing (but harmless) mistake.

Also add some asserts to make sure the internal field does not overflow
in the future.

v2: Fix WARN_ON firing for INTERRUPT event (Umesh)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..96543dce2db1 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event)
 	return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
 }
 
-static bool is_engine_config(u64 config)
+static bool is_engine_config(const u64 config)
 {
 	return config < __I915_PMU_OTHER(0);
 }
@@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
 		return other_bit(config);
 }
 
-static u64 config_mask(u64 config)
+static u32 config_mask(const u64 config)
 {
-	return BIT_ULL(config_bit(config));
+	unsigned int bit = config_bit(config);
+
+	if (__builtin_constant_p(config))
+		BUILD_BUG_ON(bit >
+			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
+							 enable)) - 1);
+	else
+		WARN_ON_ONCE(bit >
+			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
+							 enable)) - 1);
+
+	return BIT(config_bit(config));
 }
 
 static bool is_engine_event(struct perf_event *event)
@@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event *event)
 {
 	struct drm_i915_private *i915 =
 		container_of(event->pmu, typeof(*i915), pmu.base);
+	const unsigned int bit = event_bit(event);
 	struct i915_pmu *pmu = &i915->pmu;
 	unsigned long flags;
-	unsigned int bit;
 
-	bit = event_bit(event);
 	if (bit == -1)
 		goto update;
 
@@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event *event)
 	GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
 	GEM_BUG_ON(pmu->enable_count[bit] == ~0);
 
-	pmu->enable |= BIT_ULL(bit);
+	pmu->enable |= BIT(bit);
 	pmu->enable_count[bit]++;
 
 	/*
@@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event *event)
 {
 	struct drm_i915_private *i915 =
 		container_of(event->pmu, typeof(*i915), pmu.base);
-	unsigned int bit = event_bit(event);
+	const unsigned int bit = event_bit(event);
 	struct i915_pmu *pmu = &i915->pmu;
 	unsigned long flags;
 
@@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event *event)
 	 * bitmask when the last listener on an event goes away.
 	 */
 	if (--pmu->enable_count[bit] == 0) {
-		pmu->enable &= ~BIT_ULL(bit);
+		pmu->enable &= ~BIT(bit);
 		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
 	}
 
-- 
2.36.1

[Intel-gfx] [PATCH v5 2/7] 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 96543dce2db1..9edf87ee5d10 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"
@@ -425,8 +426,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))
@@ -442,8 +444,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 v5 3/7] 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 9edf87ee5d10..6d594f67f365 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -350,6 +350,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 v5 4/7] 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 6d594f67f365..890693fdaf9e 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -228,11 +228,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();
 }
 
@@ -247,16 +247,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
@@ -267,9 +267,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 v5 5/7] 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)
v3: Revert v2 (Tvrtko)

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 | 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 890693fdaf9e..ecb57a94143e 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -262,7 +262,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);
 }
@@ -279,7 +281,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);
 }
@@ -449,6 +454,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

[Intel-gfx] [PATCH v5 6/7] 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

v4: Set I915_PMU_MAX_GTS to 2 (Tvrtko)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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 | 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 ecb57a94143e..dc1ca3a15ff6 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(const 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)
@@ -115,10 +127,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.
@@ -131,9 +155,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
@@ -175,9 +197,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;
@@ -192,7 +242,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.
@@ -201,14 +251,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);
 
@@ -218,13 +268,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();
+		}
 	}
 }
 
@@ -232,8 +289,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)
@@ -373,34 +430,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;
 
 		/*
@@ -416,12 +469,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);
 	}
@@ -458,9 +511,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 			continue;
 
 		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));
@@ -502,7 +553,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! */
@@ -513,6 +570,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)
@@ -610,22 +669,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..f88de9ae1ebb 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 2
+
 /*
  * 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 ba40855dbc93..f31dfacde601 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 v5 7/7] 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>
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 | 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 dc1ca3a15ff6..dbb24c0c6093 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -940,11 +940,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) \
@@ -983,15 +992,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;
@@ -1006,12 +1016,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) {
@@ -1041,26 +1056,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

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Dixit, Ashutosh]

On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh/Tvrtko,

Mostly repeating comments/questions made on the previous patch below.

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Having it as u64 was a confusing (but harmless) mistake.
>
> Also add some asserts to make sure the internal field does not overflow
> in the future.
>
> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 7ece883a7d95..96543dce2db1 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event)
>	return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>  }
>
> -static bool is_engine_config(u64 config)
> +static bool is_engine_config(const u64 config)
>  {
>	return config < __I915_PMU_OTHER(0);
>  }
> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>		return other_bit(config);
>  }
>
> -static u64 config_mask(u64 config)
> +static u32 config_mask(const u64 config)
>  {
> -	return BIT_ULL(config_bit(config));
> +	unsigned int bit = config_bit(config);

Give that config_bit() can return -1 (I understand it is avoided in moving
the code to config_mask from config_bit), maybe the code below should also
have that check?

	int bit = config_bit(config);

	if (bit != -1)
	{
		...
	}

Though as mentioned below the 'if (__builtin_constant_p())' would have to
go. Maybe the code could even have stayed in config_bit with the check.

> +
> +	if (__builtin_constant_p(config))
> +		BUILD_BUG_ON(bit >
> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
> +							 enable)) - 1);

Given that config comes from the event (it is event->attr.config), can this
ever be a builtin constant?

> +	else
> +		WARN_ON_ONCE(bit >
> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
> +							 enable)) - 1);

There is really an even stricter limit on what the bit can be, which is the
total number of possible events but anyway this is good enough.

After addressing the above, this patch is:

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

> +
> +	return BIT(config_bit(config));
>  }
>
>  static bool is_engine_event(struct perf_event *event)
> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event *event)
>  {
>	struct drm_i915_private *i915 =
>		container_of(event->pmu, typeof(*i915), pmu.base);
> +	const unsigned int bit = event_bit(event);
>	struct i915_pmu *pmu = &i915->pmu;
>	unsigned long flags;
> -	unsigned int bit;
>
> -	bit = event_bit(event);
>	if (bit == -1)
>		goto update;
>
> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event *event)
>	GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>	GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>
> -	pmu->enable |= BIT_ULL(bit);
> +	pmu->enable |= BIT(bit);
>	pmu->enable_count[bit]++;
>
>	/*
> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event *event)
>  {
>	struct drm_i915_private *i915 =
>		container_of(event->pmu, typeof(*i915), pmu.base);
> -	unsigned int bit = event_bit(event);
> +	const unsigned int bit = event_bit(event);
>	struct i915_pmu *pmu = &i915->pmu;
>	unsigned long flags;
>
> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event *event)
>	 * bitmask when the last listener on an event goes away.
>	 */
>	if (--pmu->enable_count[bit] == 0) {
> -		pmu->enable &= ~BIT_ULL(bit);
> +		pmu->enable &= ~BIT(bit);
>		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
>	}
>
> --
> 2.36.1
>

[Intel-gfx] ✓ Fi.CI.BAT: success for Add MTL PMU support for multi-gt

[Patchwork]

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

== Series Details ==

Series: Add MTL PMU support for multi-gt
URL   : https://patchwork.freedesktop.org/series/117843/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13154 -> Patchwork_117843v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

  Missing    (2): fi-kbl-soraka fi-snb-2520m 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       [PASS][1] -> [FAIL][2] ([fdo#103375])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-cfl-8109u:       [PASS][3] -> [DMESG-FAIL][4] ([i915#5334])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_mocs:
    - bat-dg1-7:          [PASS][5] -> [ABORT][6] ([i915#4983])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-dg1-7/igt@i915_selftest@live@gt_mocs.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg1-7/igt@i915_selftest@live@gt_mocs.html

  * igt@i915_selftest@live@migrate:
    - bat-adls-5:         [PASS][7] -> [ABORT][8] ([i915#4391] / [i915#7913])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-adls-5/igt@i915_selftest@live@migrate.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-adls-5/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [PASS][9] -> [ABORT][10] ([i915#4983] / [i915#7911] / [i915#7920])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-rpls-1/igt@i915_selftest@live@requests.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][11] ([i915#1845] / [i915#5354])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@prime_vgem@basic-userptr:
    - bat-atsm-1:         NOTRUN -> [SKIP][12] ([fdo#109295])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-atsm-1/igt@prime_vgem@basic-userptr.html
    - fi-cfl-guc:         NOTRUN -> [SKIP][13] ([fdo#109271])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-cfl-guc/igt@prime_vgem@basic-userptr.html
    - bat-jsl-3:          NOTRUN -> [SKIP][14] ([fdo#109295] / [i915#3301])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-jsl-3/igt@prime_vgem@basic-userptr.html
    - bat-dg2-9:          NOTRUN -> [SKIP][15] ([i915#3708] / [i915#4873])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg2-9/igt@prime_vgem@basic-userptr.html
    - fi-kbl-x1275:       NOTRUN -> [SKIP][16] ([fdo#109271])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-kbl-x1275/igt@prime_vgem@basic-userptr.html
    - fi-hsw-4770:        NOTRUN -> [SKIP][17] ([fdo#109271])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-hsw-4770/igt@prime_vgem@basic-userptr.html
    - fi-cfl-8109u:       NOTRUN -> [SKIP][18] ([fdo#109271])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-cfl-8109u/igt@prime_vgem@basic-userptr.html
    - fi-kbl-8809g:       NOTRUN -> [SKIP][19] ([fdo#109271])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-kbl-8809g/igt@prime_vgem@basic-userptr.html
    - bat-rpls-2:         NOTRUN -> [SKIP][20] ([fdo#109295] / [i915#3708])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-rpls-2/igt@prime_vgem@basic-userptr.html
    - fi-elk-e7500:       NOTRUN -> [SKIP][21] ([fdo#109271])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-elk-e7500/igt@prime_vgem@basic-userptr.html
    - bat-dg2-8:          NOTRUN -> [SKIP][22] ([i915#3708] / [i915#4873])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg2-8/igt@prime_vgem@basic-userptr.html
    - fi-kbl-guc:         NOTRUN -> [SKIP][23] ([fdo#109271])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-kbl-guc/igt@prime_vgem@basic-userptr.html
    - bat-adlm-1:         NOTRUN -> [SKIP][24] ([i915#3708])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-adlm-1/igt@prime_vgem@basic-userptr.html
    - bat-jsl-1:          NOTRUN -> [SKIP][25] ([fdo#109295] / [i915#3301])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-jsl-1/igt@prime_vgem@basic-userptr.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][26] ([fdo#109295] / [i915#3301])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-tgl-1115g4/igt@prime_vgem@basic-userptr.html
    - fi-blb-e6850:       NOTRUN -> [SKIP][27] ([fdo#109271])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-blb-e6850/igt@prime_vgem@basic-userptr.html
    - fi-bsw-n3050:       NOTRUN -> [SKIP][28] ([fdo#109271])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-bsw-n3050/igt@prime_vgem@basic-userptr.html
    - bat-rpls-1:         NOTRUN -> [SKIP][29] ([fdo#109295] / [i915#3708])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-rpls-1/igt@prime_vgem@basic-userptr.html
    - fi-skl-6600u:       NOTRUN -> [SKIP][30] ([fdo#109271])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-skl-6600u/igt@prime_vgem@basic-userptr.html
    - fi-apl-guc:         NOTRUN -> [SKIP][31] ([fdo#109271])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-apl-guc/igt@prime_vgem@basic-userptr.html
    - bat-dg1-5:          NOTRUN -> [SKIP][32] ([i915#3708] / [i915#4873])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg1-5/igt@prime_vgem@basic-userptr.html
    - bat-dg1-7:          NOTRUN -> [SKIP][33] ([i915#3708] / [i915#4873])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg1-7/igt@prime_vgem@basic-userptr.html
    - fi-glk-j4005:       NOTRUN -> [SKIP][34] ([fdo#109271])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-glk-j4005/igt@prime_vgem@basic-userptr.html
    - bat-adlp-9:         NOTRUN -> [SKIP][35] ([fdo#109295] / [i915#3301] / [i915#3708])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-adlp-9/igt@prime_vgem@basic-userptr.html
    - fi-skl-guc:         NOTRUN -> [SKIP][36] ([fdo#109271])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-skl-guc/igt@prime_vgem@basic-userptr.html
    - bat-dg2-11:         NOTRUN -> [SKIP][37] ([i915#3708] / [i915#4873])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg2-11/igt@prime_vgem@basic-userptr.html
    - fi-kbl-7567u:       NOTRUN -> [SKIP][38] ([fdo#109271])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-kbl-7567u/igt@prime_vgem@basic-userptr.html
    - fi-cfl-8700k:       NOTRUN -> [SKIP][39] ([fdo#109271])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-cfl-8700k/igt@prime_vgem@basic-userptr.html
    - fi-bsw-nick:        NOTRUN -> [SKIP][40] ([fdo#109271])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-bsw-nick/igt@prime_vgem@basic-userptr.html
    - fi-rkl-11600:       NOTRUN -> [SKIP][41] ([fdo#109295] / [i915#3301] / [i915#3708])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/fi-rkl-11600/igt@prime_vgem@basic-userptr.html
    - bat-adls-5:         NOTRUN -> [SKIP][42] ([fdo#109295] / [i915#3301])
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-adls-5/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][43] ([i915#7699]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - {bat-mtlp-6}:       [ABORT][45] ([i915#4983] / [i915#7920]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-mtlp-6/igt@i915_selftest@live@requests.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-mtlp-6/igt@i915_selftest@live@requests.html

  
#### Warnings ####

  * igt@kms_psr@primary_mmap_gtt:
    - bat-rplp-1:         [SKIP][47] ([i915#1072]) -> [ABORT][48] ([i915#8434] / [i915#8442])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.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
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#8368]: https://gitlab.freedesktop.org/drm/intel/issues/8368
  [i915#8434]: https://gitlab.freedesktop.org/drm/intel/issues/8434
  [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442


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

  * IGT: IGT_7292 -> IGTPW_8954
  * Linux: CI_DRM_13154 -> Patchwork_117843v1

  CI-20190529: 20190529
  CI_DRM_13154: d04e82f5245c285e7ae36955d89c4d217d04d664 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8954: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8954/index.html
  IGT_7292: 9d9475ffd3b5ae18fd8ec120595385f6c562f249 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117843v1: d04e82f5245c285e7ae36955d89c4d217d04d664 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

deff6babe74a drm/i915/pmu: Export counters from all tiles
db43e077608d drm/i915/pmu: Prepare for multi-tile non-engine counters
9fe6e4309c42 drm/i915/pmu: Add reference counting to the sampling timer
ecbf513bc4d7 drm/i915/pmu: Transform PMU parking code to be GT based
f856a075bf3f drm/i915/pmu: Skip sampling engines with no enabled counters
c35d113398f4 drm/i915/pmu: Support PMU for all engines
4aa266c7fddd drm/i915/pmu: Change bitmask of enabled events to u32

== Logs ==

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

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

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

[Dixit, Ashutosh]

On Tue, 16 May 2023 16:35:33 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> +static u64 frequency_enabled_mask(void)

u32

> +{
> +	unsigned int i;
> +	u64 mask = 0;

u32

> +
> +	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;

u32

>
>	/*
>	 * Only some counters need the sampling timer.
> @@ -131,9 +155,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

/snip/

> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 3a811266ac6a..f88de9ae1ebb 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 2
> +
>  /*
>   * 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;

u32

Thanks.
--
Ashutosh

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Umesh Nerlige Ramappa]

On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh/Tvrtko,
>
>Mostly repeating comments/questions made on the previous patch below.
>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Having it as u64 was a confusing (but harmless) mistake.
>>
>> Also add some asserts to make sure the internal field does not overflow
>> in the future.
>>
>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index 7ece883a7d95..96543dce2db1 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event)
>>	return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>>  }
>>
>> -static bool is_engine_config(u64 config)
>> +static bool is_engine_config(const u64 config)
>>  {
>>	return config < __I915_PMU_OTHER(0);
>>  }
>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>		return other_bit(config);
>>  }
>>
>> -static u64 config_mask(u64 config)
>> +static u32 config_mask(const u64 config)
>>  {
>> -	return BIT_ULL(config_bit(config));
>> +	unsigned int bit = config_bit(config);
>
>Give that config_bit() can return -1 (I understand it is avoided in moving
>the code to config_mask from config_bit), maybe the code below should also
>have that check?

config_mask is only called to check frequency related events in the 
code, so I don't see it returing -1 here.

>
>	int bit = config_bit(config);
>
>	if (bit != -1)
>	{
>		...
>	}
>
>Though as mentioned below the 'if (__builtin_constant_p())' would have to
>go. Maybe the code could even have stayed in config_bit with the check.
>
>> +
>> +	if (__builtin_constant_p(config))
>> +		BUILD_BUG_ON(bit >
>> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
>> +							 enable)) - 1);
>
>Given that config comes from the event (it is event->attr.config), can this
>ever be a builtin constant?

Not sure about earlier code where these checks were inside config_bit(), 
but with changes I made, I don't see this being a builtin constant.  
However, nothing prevents a caller from just passing a builtin_constant 
to this in future.

Thanks,
Umesh

>
>> +	else
>> +		WARN_ON_ONCE(bit >
>> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
>> +							 enable)) - 1);
>
>There is really an even stricter limit on what the bit can be, which is the
>total number of possible events but anyway this is good enough.
>
>After addressing the above, this patch is:
>
>Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
>> +
>> +	return BIT(config_bit(config));
>>  }
>>
>>  static bool is_engine_event(struct perf_event *event)
>> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event *event)
>>  {
>>	struct drm_i915_private *i915 =
>>		container_of(event->pmu, typeof(*i915), pmu.base);
>> +	const unsigned int bit = event_bit(event);
>>	struct i915_pmu *pmu = &i915->pmu;
>>	unsigned long flags;
>> -	unsigned int bit;
>>
>> -	bit = event_bit(event);
>>	if (bit == -1)
>>		goto update;
>>
>> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event *event)
>>	GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>>	GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>>
>> -	pmu->enable |= BIT_ULL(bit);
>> +	pmu->enable |= BIT(bit);
>>	pmu->enable_count[bit]++;
>>
>>	/*
>> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event *event)
>>  {
>>	struct drm_i915_private *i915 =
>>		container_of(event->pmu, typeof(*i915), pmu.base);
>> -	unsigned int bit = event_bit(event);
>> +	const unsigned int bit = event_bit(event);
>>	struct i915_pmu *pmu = &i915->pmu;
>>	unsigned long flags;
>>
>> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event *event)
>>	 * bitmask when the last listener on an event goes away.
>>	 */
>>	if (--pmu->enable_count[bit] == 0) {
>> -		pmu->enable &= ~BIT_ULL(bit);
>> +		pmu->enable &= ~BIT(bit);
>>		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
>>	}
>>
>> --
>> 2.36.1
>>

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

[Umesh Nerlige Ramappa]

On Tue, May 16, 2023 at 05:39:02PM -0700, Dixit, Ashutosh wrote:
>On Tue, 16 May 2023 16:35:33 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> +static u64 frequency_enabled_mask(void)
>
>u32
>
>> +{
>> +	unsigned int i;
>> +	u64 mask = 0;
>
>u32
>
>> +
>> +	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;
>
>u32
>
>>
>>	/*
>>	 * Only some counters need the sampling timer.
>> @@ -131,9 +155,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
>
>/snip/
>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>> index 3a811266ac6a..f88de9ae1ebb 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 2
>> +
>>  /*
>>   * 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;
>
>u32

Hmm, I missed that. Will fix.

Thanks,
Umesh
>
>Thanks.
>--
>Ashutosh

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Tvrtko Ursulin]

On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>>
>>
>> Hi Umesh/Tvrtko,
>>
>> Mostly repeating comments/questions made on the previous patch below.

First of all thanks for improving this, my v1 obviously wasn't good enough.

>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Having it as u64 was a confusing (but harmless) mistake.
>>>
>>> Also add some asserts to make sure the internal field does not overflow
>>> in the future.
>>>
>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c 
>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>> index 7ece883a7d95..96543dce2db1 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event 
>>> *event)
>>>     return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>>>  }
>>>
>>> -static bool is_engine_config(u64 config)
>>> +static bool is_engine_config(const u64 config)
>>>  {
>>>     return config < __I915_PMU_OTHER(0);
>>>  }
>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>>         return other_bit(config);
>>>  }
>>>
>>> -static u64 config_mask(u64 config)
>>> +static u32 config_mask(const u64 config)
>>>  {
>>> -    return BIT_ULL(config_bit(config));
>>> +    unsigned int bit = config_bit(config);
>>
>> Give that config_bit() can return -1 (I understand it is avoided in 
>> moving
>> the code to config_mask from config_bit), maybe the code below should 
>> also
>> have that check?
> 
> config_mask is only called to check frequency related events in the 
> code, so I don't see it returing -1 here.

Yeah that should be fine since -1 would make the below asserts fire 
anyway. (If it would get called from a different path in the future.)

>>
>>     int bit = config_bit(config);
>>
>>     if (bit != -1)
>>     {
>>         ...
>>     }
>>
>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>> go. Maybe the code could even have stayed in config_bit with the check.
>>
>>> +
>>> +    if (__builtin_constant_p(config))
>>> +        BUILD_BUG_ON(bit >
>>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
>>> +                             enable)) - 1);
>>
>> Given that config comes from the event (it is event->attr.config), can 
>> this
>> ever be a builtin constant?
> 
> Not sure about earlier code where these checks were inside config_bit(), 
> but with changes I made, I don't see this being a builtin constant. 
> However, nothing prevents a caller from just passing a builtin_constant 
> to this in future.

Are you sure? I would have thought it would always be a compile time 
constant now that the check is in config_mask. Aahhh.. with the 
multi-tile changes maybe it can't unroll the loops and calculate the 
masks at compile time. Maybe it is a bit too much and we should drop the 
__builtin_constant_p branch? Probably.. But I guess it is safe to use 
GEM_WARN_ON_ONCE instead of WARN_ON_ONCE since there are no external 
callers (nothing coming from event) now. That way at least production 
builds don't have to have the check.

Regards,

Tvrtko

> 
> Thanks,
> Umesh
> 
>>
>>> +    else
>>> +        WARN_ON_ONCE(bit >
>>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
>>> +                             enable)) - 1);
>>
>> There is really an even stricter limit on what the bit can be, which 
>> is the
>> total number of possible events but anyway this is good enough.
>>
>> After addressing the above, this patch is:
>>
>> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>
>>> +
>>> +    return BIT(config_bit(config));
>>>  }
>>>
>>>  static bool is_engine_event(struct perf_event *event)
>>> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event 
>>> *event)
>>>  {
>>>     struct drm_i915_private *i915 =
>>>         container_of(event->pmu, typeof(*i915), pmu.base);
>>> +    const unsigned int bit = event_bit(event);
>>>     struct i915_pmu *pmu = &i915->pmu;
>>>     unsigned long flags;
>>> -    unsigned int bit;
>>>
>>> -    bit = event_bit(event);
>>>     if (bit == -1)
>>>         goto update;
>>>
>>> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event 
>>> *event)
>>>     GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>>>     GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>>>
>>> -    pmu->enable |= BIT_ULL(bit);
>>> +    pmu->enable |= BIT(bit);
>>>     pmu->enable_count[bit]++;
>>>
>>>     /*
>>> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event 
>>> *event)
>>>  {
>>>     struct drm_i915_private *i915 =
>>>         container_of(event->pmu, typeof(*i915), pmu.base);
>>> -    unsigned int bit = event_bit(event);
>>> +    const unsigned int bit = event_bit(event);
>>>     struct i915_pmu *pmu = &i915->pmu;
>>>     unsigned long flags;
>>>
>>> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event 
>>> *event)
>>>      * bitmask when the last listener on an event goes away.
>>>      */
>>>     if (--pmu->enable_count[bit] == 0) {
>>> -        pmu->enable &= ~BIT_ULL(bit);
>>> +        pmu->enable &= ~BIT(bit);
>>>         pmu->timer_enabled &= pmu_needs_timer(pmu, true);
>>>     }
>>>
>>> -- 
>>> 2.36.1
>>>

[Intel-gfx] ✓ Fi.CI.IGT: success for Add MTL PMU support for multi-gt

[Patchwork]

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

== Series Details ==

Series: Add MTL PMU support for multi-gt
URL   : https://patchwork.freedesktop.org/series/117843/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13154_full -> Patchwork_117843v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

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

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

### IGT changes ###

#### Possible regressions ####

  * {igt@perf_pmu@rc6-all-gts} (NEW):
    - {shard-dg1}:        NOTRUN -> [SKIP][1] +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-dg1-16/igt@perf_pmu@rc6-all-gts.html

  * {igt@perf_pmu@rc6@other-idle-gt0} (NEW):
    - {shard-rkl}:        NOTRUN -> [SKIP][2] +1 similar issue
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-rkl-1/igt@perf_pmu@rc6@other-idle-gt0.html
    - {shard-tglu}:       NOTRUN -> [SKIP][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-tglu-5/igt@perf_pmu@rc6@other-idle-gt0.html

  
New tests
---------

  New tests have been introduced between CI_DRM_13154_full and Patchwork_117843v1_full:

### New IGT tests (7) ###

  * igt@perf_pmu@frequency@gt0:
    - Statuses : 5 skip(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@frequency@idle-gt0:
    - Statuses : 5 pass(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@rc6-all-gts:
    - Statuses : 4 skip(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@rc6@gt0:
    - Statuses : 6 pass(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@rc6@other-idle-gt0:
    - Statuses : 6 skip(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@rc6@runtime-pm-gt0:
    - Statuses : 5 pass(s) 1 skip(s)
    - Exec time: [0.0] s

  * igt@perf_pmu@rc6@runtime-pm-long-gt0:
    - Statuses : 5 pass(s) 1 skip(s)
    - Exec time: [0.0] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][4] -> [FAIL][5] ([i915#2842]) +1 similar issue
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [PASS][6] -> [FAIL][7] ([i915#2842])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl3/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl7/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_huc_copy@huc-copy:
    - shard-apl:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#2190])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl3/igt@gem_huc_copy@huc-copy.html

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-snb:          [PASS][9] -> [DMESG-FAIL][10] ([i915#8295])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-snb4/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-snb6/igt@gem_ppgtt@blt-vs-render-ctx0.html

  * igt@i915_pm_rps@reset:
    - shard-snb:          [PASS][11] -> [DMESG-FAIL][12] ([i915#8319])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-snb4/igt@i915_pm_rps@reset.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-snb2/igt@i915_pm_rps@reset.html

  * igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_rc_ccs_cc:
    - shard-apl:          NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#3886]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl6/igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_chamelium_color@ctm-0-75:
    - shard-apl:          NOTRUN -> [SKIP][14] ([fdo#109271]) +43 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl1/igt@kms_chamelium_color@ctm-0-75.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-glk:          [PASS][15] -> [FAIL][16] ([i915#2346])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-glk5/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_flip@flip-vs-suspend@a-dp1:
    - shard-apl:          [PASS][17] -> [ABORT][18] ([i915#180])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl1/igt@kms_flip@flip-vs-suspend@a-dp1.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl7/igt@kms_flip@flip-vs-suspend@a-dp1.html

  * igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1:
    - shard-glk:          [PASS][19] -> [FAIL][20] ([i915#2122]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-glk6/igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-glk4/igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1.html

  * igt@kms_panel_fitting@legacy:
    - shard-apl:          NOTRUN -> [SKIP][21] ([fdo#109271] / [i915#4579]) +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl3/igt@kms_panel_fitting@legacy.html

  * igt@kms_psr2_su@frontbuffer-xrgb8888:
    - shard-apl:          NOTRUN -> [SKIP][22] ([fdo#109271] / [i915#658])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl3/igt@kms_psr2_su@frontbuffer-xrgb8888.html

  * {igt@perf_pmu@rc6-all-gts} (NEW):
    - shard-snb:          NOTRUN -> [SKIP][23] ([fdo#109271]) +12 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-snb6/igt@perf_pmu@rc6-all-gts.html

  * igt@prime_vgem@basic-userptr:
    - shard-glk:          NOTRUN -> [SKIP][24] ([fdo#109271]) +3 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-glk1/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@device_reset@unbind-reset-rebind:
    - {shard-rkl}:        [ABORT][25] ([i915#5507]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-rkl-6/igt@device_reset@unbind-reset-rebind.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-rkl-7/igt@device_reset@unbind-reset-rebind.html
    - shard-apl:          [ABORT][27] ([i915#5507]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl4/igt@device_reset@unbind-reset-rebind.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl3/igt@device_reset@unbind-reset-rebind.html
    - {shard-tglu}:       [ABORT][29] ([i915#5507]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-tglu-7/igt@device_reset@unbind-reset-rebind.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-tglu-9/igt@device_reset@unbind-reset-rebind.html

  * igt@gem_barrier_race@remote-request@rcs0:
    - {shard-dg1}:        [ABORT][31] ([i915#7461] / [i915#8234]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-dg1-14/igt@gem_barrier_race@remote-request@rcs0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-dg1-16/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][33] ([i915#2842]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl1/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - {shard-rkl}:        [FAIL][35] ([i915#2842]) -> [PASS][36] +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-rkl-6/igt@gem_exec_fair@basic-pace@rcs0.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-rkl-2/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-apl:          [ABORT][37] ([i915#5566]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl3/igt@gen9_exec_parse@allowed-single.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl2/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_pm_rpm@dpms-mode-unset-lpsp:
    - {shard-rkl}:        [SKIP][39] ([i915#1397]) -> [PASS][40] +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-rkl-4/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-rkl-7/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html

  * igt@i915_selftest@perf@engine_cs:
    - shard-snb:          [ABORT][41] ([i915#4528] / [i915#4579]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-snb6/igt@i915_selftest@perf@engine_cs.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-snb2/igt@i915_selftest@perf@engine_cs.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [FAIL][43] ([i915#2346]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * {igt@kms_plane_scaling@intel-max-src-size@pipe-a-hdmi-a-2}:
    - {shard-rkl}:        [FAIL][45] ([i915#8292]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13154/shard-rkl-2/igt@kms_plane_scaling@intel-max-src-size@pipe-a-hdmi-a-2.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117843v1/shard-rkl-4/igt@kms_plane_scaling@intel-max-src-size@pipe-a-hdmi-a-2.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
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110542]: https://bugs.freedesktop.org/show_bug.cgi?id=110542
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#3023]: https://gitlab.freedesktop.org/drm/intel/issues/3023
  [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#433]: https://gitlab.freedesktop.org/drm/intel/issues/433
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4936]: https://gitlab.freedesktop.org/drm/intel/issues/4936
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5493]: https://gitlab.freedesktop.org/drm/intel/issues/5493
  [i915#5507]: https://gitlab.freedesktop.org/drm/intel/issues/5507
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8213]: https://gitlab.freedesktop.org/drm/intel/issues/8213
  [i915#8234]: https://gitlab.freedesktop.org/drm/intel/issues/8234
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292
  [i915#8295]: https://gitlab.freedesktop.org/drm/intel/issues/8295
  [i915#8319]: https://gitlab.freedesktop.org/drm/intel/issues/8319
  [i915#8414]: https://gitlab.freedesktop.org/drm/intel/issues/8414


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

  * IGT: IGT_7292 -> IGTPW_8954
  * Linux: CI_DRM_13154 -> Patchwork_117843v1

  CI-20190529: 20190529
  CI_DRM_13154: d04e82f5245c285e7ae36955d89c4d217d04d664 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8954: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8954/index.html
  IGT_7292: 9d9475ffd3b5ae18fd8ec120595385f6c562f249 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117843v1: d04e82f5245c285e7ae36955d89c4d217d04d664 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Dixit, Ashutosh]

On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>
>
> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> > On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
> >> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
> >>>
> >>
> >> Hi Umesh/Tvrtko,
> >>
> >> Mostly repeating comments/questions made on the previous patch below.
>
> First of all thanks for improving this, my v1 obviously wasn't good enough.
>
> >>
> >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> Having it as u64 was a confusing (but harmless) mistake.
> >>>
> >>> Also add some asserts to make sure the internal field does not overflow
> >>> in the future.
> >>>
> >>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
> >>>  1 file changed, 18 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>> index 7ece883a7d95..96543dce2db1 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event
> >>> *event)
> >>>     return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
> >>>  }
> >>>
> >>> -static bool is_engine_config(u64 config)
> >>> +static bool is_engine_config(const u64 config)
> >>>  {
> >>>     return config < __I915_PMU_OTHER(0);
> >>>  }
> >>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
> >>>         return other_bit(config);
> >>>  }
> >>>
> >>> -static u64 config_mask(u64 config)
> >>> +static u32 config_mask(const u64 config)
> >>>  {
> >>> -    return BIT_ULL(config_bit(config));
> >>> +    unsigned int bit = config_bit(config);
> >>
> >> Give that config_bit() can return -1 (I understand it is avoided in
> >> moving
> >> the code to config_mask from config_bit), maybe the code below should
> >> also
> >> have that check?
> >
> > config_mask is only called to check frequency related events in the code,
> > so I don't see it returing -1 here.
>
> Yeah that should be fine since -1 would make the below asserts fire
> anyway. (If it would get called from a different path in the future.)
>
> >>
> >>     int bit = config_bit(config);
> >>
> >>     if (bit != -1)
> >>     {
> >>         ...
> >>     }
> >>
> >> Though as mentioned below the 'if (__builtin_constant_p())' would have to
> >> go. Maybe the code could even have stayed in config_bit with the check.
> >>
> >>> +
> >>> +    if (__builtin_constant_p(config))
> >>> +        BUILD_BUG_ON(bit >
> >>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
> >>> +                             enable)) - 1);
> >>
> >> Given that config comes from the event (it is event->attr.config), can
> >> this
> >> ever be a builtin constant?
> >
> > Not sure about earlier code where these checks were inside config_bit(),
> > but with changes I made, I don't see this being a builtin
> > constant. However, nothing prevents a caller from just passing a
> > builtin_constant to this in future.
>
> Are you sure? I would have thought it would always be a compile time
> constant now that the check is in config_mask. Aahhh.. with the multi-tile
> changes maybe it can't unroll the loops and calculate the masks at compile
> time. Maybe it is a bit too much and we should drop the
> __builtin_constant_p branch? Probably..

Ah yes, with the code move to config_mask, they really all are compile time
constants (provided compiler can unroll the loops) so at least that is the
justfication for leaving the __builtin_constant_p in. So I'd probably just
leave it as is (though it is a bit too much).

> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
> since there are no external callers (nothing coming from event) now. That
> way at least production builds don't have to have the check.

Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
I guess.

So I'm ok with the code staying as is. Enough bike-shed on this already.

Thanks.
--
Ashutosh


>
> Regards,
>
> Tvrtko
>
> >
> > Thanks,
> > Umesh
> >
> >>
> >>> +    else
> >>> +        WARN_ON_ONCE(bit >
> >>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
> >>> +                             enable)) - 1);
> >>
> >> There is really an even stricter limit on what the bit can be, which is
> >> the
> >> total number of possible events but anyway this is good enough.
> >>
> >> After addressing the above, this patch is:
> >>
> >> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>
> >>> +
> >>> +    return BIT(config_bit(config));
> >>>  }
> >>>
> >>>  static bool is_engine_event(struct perf_event *event)
> >>> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event
> >>> *event)
> >>>  {
> >>>     struct drm_i915_private *i915 =
> >>>         container_of(event->pmu, typeof(*i915), pmu.base);
> >>> +    const unsigned int bit = event_bit(event);
> >>>     struct i915_pmu *pmu = &i915->pmu;
> >>>     unsigned long flags;
> >>> -    unsigned int bit;
> >>>
> >>> -    bit = event_bit(event);
> >>>     if (bit == -1)
> >>>         goto update;
> >>>
> >>> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event
> >>> *event)
> >>>     GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
> >>>     GEM_BUG_ON(pmu->enable_count[bit] == ~0);
> >>>
> >>> -    pmu->enable |= BIT_ULL(bit);
> >>> +    pmu->enable |= BIT(bit);
> >>>     pmu->enable_count[bit]++;
> >>>
> >>>     /*
> >>> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event
> >>> *event)
> >>>  {
> >>>     struct drm_i915_private *i915 =
> >>>         container_of(event->pmu, typeof(*i915), pmu.base);
> >>> -    unsigned int bit = event_bit(event);
> >>> +    const unsigned int bit = event_bit(event);
> >>>     struct i915_pmu *pmu = &i915->pmu;
> >>>     unsigned long flags;
> >>>
> >>> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event
> >>> *event)
> >>>      * bitmask when the last listener on an event goes away.
> >>>      */
> >>>     if (--pmu->enable_count[bit] == 0) {
> >>> -        pmu->enable &= ~BIT_ULL(bit);
> >>> +        pmu->enable &= ~BIT(bit);
> >>>         pmu->timer_enabled &= pmu_needs_timer(pmu, true);
> >>>     }
> >>>
> >>> --
> >>> 2.36.1
> >>>

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Umesh Nerlige Ramappa]

On Wed, May 17, 2023 at 09:25:03AM -0700, Dixit, Ashutosh wrote:
>On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>>
>>
>> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
>> > On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>> >> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>> >>>
>> >>
>> >> Hi Umesh/Tvrtko,
>> >>
>> >> Mostly repeating comments/questions made on the previous patch below.
>>
>> First of all thanks for improving this, my v1 obviously wasn't good enough.
>>
>> >>
>> >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >>>
>> >>> Having it as u64 was a confusing (but harmless) mistake.
>> >>>
>> >>> Also add some asserts to make sure the internal field does not overflow
>> >>> in the future.
>> >>>
>> >>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>> >>>
>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> >>> ---
>> >>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>> >>>  1 file changed, 18 insertions(+), 8 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>> >>> b/drivers/gpu/drm/i915/i915_pmu.c
>> >>> index 7ece883a7d95..96543dce2db1 100644
>> >>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> >>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event
>> >>> *event)
>> >>>     return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>> >>>  }
>> >>>
>> >>> -static bool is_engine_config(u64 config)
>> >>> +static bool is_engine_config(const u64 config)
>> >>>  {
>> >>>     return config < __I915_PMU_OTHER(0);
>> >>>  }
>> >>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>> >>>         return other_bit(config);
>> >>>  }
>> >>>
>> >>> -static u64 config_mask(u64 config)
>> >>> +static u32 config_mask(const u64 config)
>> >>>  {
>> >>> -    return BIT_ULL(config_bit(config));
>> >>> +    unsigned int bit = config_bit(config);
>> >>
>> >> Give that config_bit() can return -1 (I understand it is avoided in
>> >> moving
>> >> the code to config_mask from config_bit), maybe the code below should
>> >> also
>> >> have that check?
>> >
>> > config_mask is only called to check frequency related events in the code,
>> > so I don't see it returing -1 here.
>>
>> Yeah that should be fine since -1 would make the below asserts fire
>> anyway. (If it would get called from a different path in the future.)
>>
>> >>
>> >>     int bit = config_bit(config);
>> >>
>> >>     if (bit != -1)
>> >>     {
>> >>         ...
>> >>     }
>> >>
>> >> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>> >> go. Maybe the code could even have stayed in config_bit with the check.
>> >>
>> >>> +
>> >>> +    if (__builtin_constant_p(config))
>> >>> +        BUILD_BUG_ON(bit >
>> >>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
>> >>> +                             enable)) - 1);
>> >>
>> >> Given that config comes from the event (it is event->attr.config), can
>> >> this
>> >> ever be a builtin constant?
>> >
>> > Not sure about earlier code where these checks were inside config_bit(),
>> > but with changes I made, I don't see this being a builtin
>> > constant. However, nothing prevents a caller from just passing a
>> > builtin_constant to this in future.
>>
>> Are you sure? I would have thought it would always be a compile time
>> constant now that the check is in config_mask. Aahhh.. with the multi-tile
>> changes maybe it can't unroll the loops and calculate the masks at compile
>> time. Maybe it is a bit too much and we should drop the
>> __builtin_constant_p branch? Probably..
>
>Ah yes, with the code move to config_mask, they really all are compile time
>constants (provided compiler can unroll the loops) so at least that is the
>justfication for leaving the __builtin_constant_p in. So I'd probably just
>leave it as is (though it is a bit too much).
>
>> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
>> since there are no external callers (nothing coming from event) now. That
>> way at least production builds don't have to have the check.
>
>Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
>I guess.
>
>So I'm ok with the code staying as is. Enough bike-shed on this already.

Leaving it as is. @Ashutosh, okay to use your R-b without any changes to 
this patch?

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh
>
>
>>
>> Regards,
>>
>> Tvrtko
>>
>> >
>> > Thanks,
>> > Umesh
>> >
>> >>
>> >>> +    else
>> >>> +        WARN_ON_ONCE(bit >
>> >>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
>> >>> +                             enable)) - 1);
>> >>
>> >> There is really an even stricter limit on what the bit can be, which is
>> >> the
>> >> total number of possible events but anyway this is good enough.
>> >>
>> >> After addressing the above, this patch is:
>> >>
>> >> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> >>
>> >>> +
>> >>> +    return BIT(config_bit(config));
>> >>>  }
>> >>>
>> >>>  static bool is_engine_event(struct perf_event *event)
>> >>> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event
>> >>> *event)
>> >>>  {
>> >>>     struct drm_i915_private *i915 =
>> >>>         container_of(event->pmu, typeof(*i915), pmu.base);
>> >>> +    const unsigned int bit = event_bit(event);
>> >>>     struct i915_pmu *pmu = &i915->pmu;
>> >>>     unsigned long flags;
>> >>> -    unsigned int bit;
>> >>>
>> >>> -    bit = event_bit(event);
>> >>>     if (bit == -1)
>> >>>         goto update;
>> >>>
>> >>> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event
>> >>> *event)
>> >>>     GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>> >>>     GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>> >>>
>> >>> -    pmu->enable |= BIT_ULL(bit);
>> >>> +    pmu->enable |= BIT(bit);
>> >>>     pmu->enable_count[bit]++;
>> >>>
>> >>>     /*
>> >>> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event
>> >>> *event)
>> >>>  {
>> >>>     struct drm_i915_private *i915 =
>> >>>         container_of(event->pmu, typeof(*i915), pmu.base);
>> >>> -    unsigned int bit = event_bit(event);
>> >>> +    const unsigned int bit = event_bit(event);
>> >>>     struct i915_pmu *pmu = &i915->pmu;
>> >>>     unsigned long flags;
>> >>>
>> >>> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event
>> >>> *event)
>> >>>      * bitmask when the last listener on an event goes away.
>> >>>      */
>> >>>     if (--pmu->enable_count[bit] == 0) {
>> >>> -        pmu->enable &= ~BIT_ULL(bit);
>> >>> +        pmu->enable &= ~BIT(bit);
>> >>>         pmu->timer_enabled &= pmu_needs_timer(pmu, true);
>> >>>     }
>> >>>
>> >>> --
>> >>> 2.36.1
>> >>>

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Dixit, Ashutosh]

On Wed, 17 May 2023 13:15:14 -0700, Umesh Nerlige Ramappa wrote:
>
> Leaving it as is. @Ashutosh, okay to use your R-b without any changes to
> this patch?

Yes.

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

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Tvrtko Ursulin]

On 17/05/2023 17:25, Dixit, Ashutosh wrote:
> On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>>
>>
>> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
>>> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>>>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>>>>
>>>>
>>>> Hi Umesh/Tvrtko,
>>>>
>>>> Mostly repeating comments/questions made on the previous patch below.
>>
>> First of all thanks for improving this, my v1 obviously wasn't good enough.
>>
>>>>
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Having it as u64 was a confusing (but harmless) mistake.
>>>>>
>>>>> Also add some asserts to make sure the internal field does not overflow
>>>>> in the future.
>>>>>
>>>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>>>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>>>> index 7ece883a7d95..96543dce2db1 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>>>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event
>>>>> *event)
>>>>>      return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>>>>>   }
>>>>>
>>>>> -static bool is_engine_config(u64 config)
>>>>> +static bool is_engine_config(const u64 config)
>>>>>   {
>>>>>      return config < __I915_PMU_OTHER(0);
>>>>>   }
>>>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>>>>          return other_bit(config);
>>>>>   }
>>>>>
>>>>> -static u64 config_mask(u64 config)
>>>>> +static u32 config_mask(const u64 config)
>>>>>   {
>>>>> -    return BIT_ULL(config_bit(config));
>>>>> +    unsigned int bit = config_bit(config);
>>>>
>>>> Give that config_bit() can return -1 (I understand it is avoided in
>>>> moving
>>>> the code to config_mask from config_bit), maybe the code below should
>>>> also
>>>> have that check?
>>>
>>> config_mask is only called to check frequency related events in the code,
>>> so I don't see it returing -1 here.
>>
>> Yeah that should be fine since -1 would make the below asserts fire
>> anyway. (If it would get called from a different path in the future.)
>>
>>>>
>>>>      int bit = config_bit(config);
>>>>
>>>>      if (bit != -1)
>>>>      {
>>>>          ...
>>>>      }
>>>>
>>>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>>>> go. Maybe the code could even have stayed in config_bit with the check.
>>>>
>>>>> +
>>>>> +    if (__builtin_constant_p(config))
>>>>> +        BUILD_BUG_ON(bit >
>>>>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
>>>>> +                             enable)) - 1);
>>>>
>>>> Given that config comes from the event (it is event->attr.config), can
>>>> this
>>>> ever be a builtin constant?
>>>
>>> Not sure about earlier code where these checks were inside config_bit(),
>>> but with changes I made, I don't see this being a builtin
>>> constant. However, nothing prevents a caller from just passing a
>>> builtin_constant to this in future.
>>
>> Are you sure? I would have thought it would always be a compile time
>> constant now that the check is in config_mask. Aahhh.. with the multi-tile
>> changes maybe it can't unroll the loops and calculate the masks at compile
>> time. Maybe it is a bit too much and we should drop the
>> __builtin_constant_p branch? Probably..
> 
> Ah yes, with the code move to config_mask, they really all are compile time
> constants (provided compiler can unroll the loops) so at least that is the
> justfication for leaving the __builtin_constant_p in. So I'd probably just
> leave it as is (though it is a bit too much).
> 
>> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
>> since there are no external callers (nothing coming from event) now. That
>> way at least production builds don't have to have the check.
> 
> Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
> I guess.
> 
> So I'm ok with the code staying as is. Enough bike-shed on this already.

Latest series looks fine to me and thanks for your patience. Hope you 
would agree changing that one thing to u32 made more sense than changing 
the other to u64 so bike shed wasn't for nothing.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

[Dixit, Ashutosh]

On Thu, 18 May 2023 02:07:55 -0700, Tvrtko Ursulin wrote:
>
> On 17/05/2023 17:25, Dixit, Ashutosh wrote:
> > On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
> >>
> >>
> >> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> >>> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
> >>>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
> >>>>>
> >>>>
> >>>> Hi Umesh/Tvrtko,
> >>>>
> >>>> Mostly repeating comments/questions made on the previous patch below.
> >>
> >> First of all thanks for improving this, my v1 obviously wasn't good enough.
> >>
> >>>>
> >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>
> >>>>> Having it as u64 was a confusing (but harmless) mistake.
> >>>>>
> >>>>> Also add some asserts to make sure the internal field does not overflow
> >>>>> in the future.
> >>>>>
> >>>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >>>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>>>> ---
> >>>>>   drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
> >>>>>   1 file changed, 18 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> index 7ece883a7d95..96543dce2db1 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event
> >>>>> *event)
> >>>>>      return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
> >>>>>   }
> >>>>>
> >>>>> -static bool is_engine_config(u64 config)
> >>>>> +static bool is_engine_config(const u64 config)
> >>>>>   {
> >>>>>      return config < __I915_PMU_OTHER(0);
> >>>>>   }
> >>>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
> >>>>>          return other_bit(config);
> >>>>>   }
> >>>>>
> >>>>> -static u64 config_mask(u64 config)
> >>>>> +static u32 config_mask(const u64 config)
> >>>>>   {
> >>>>> -    return BIT_ULL(config_bit(config));
> >>>>> +    unsigned int bit = config_bit(config);
> >>>>
> >>>> Give that config_bit() can return -1 (I understand it is avoided in
> >>>> moving
> >>>> the code to config_mask from config_bit), maybe the code below should
> >>>> also
> >>>> have that check?
> >>>
> >>> config_mask is only called to check frequency related events in the code,
> >>> so I don't see it returing -1 here.
> >>
> >> Yeah that should be fine since -1 would make the below asserts fire
> >> anyway. (If it would get called from a different path in the future.)
> >>
> >>>>
> >>>>      int bit = config_bit(config);
> >>>>
> >>>>      if (bit != -1)
> >>>>      {
> >>>>          ...
> >>>>      }
> >>>>
> >>>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
> >>>> go. Maybe the code could even have stayed in config_bit with the check.
> >>>>
> >>>>> +
> >>>>> +    if (__builtin_constant_p(config))
> >>>>> +        BUILD_BUG_ON(bit >
> >>>>> +                 BITS_PER_TYPE(typeof_member(struct i915_pmu,
> >>>>> +                             enable)) - 1);
> >>>>
> >>>> Given that config comes from the event (it is event->attr.config), can
> >>>> this
> >>>> ever be a builtin constant?
> >>>
> >>> Not sure about earlier code where these checks were inside config_bit(),
> >>> but with changes I made, I don't see this being a builtin
> >>> constant. However, nothing prevents a caller from just passing a
> >>> builtin_constant to this in future.
> >>
> >> Are you sure? I would have thought it would always be a compile time
> >> constant now that the check is in config_mask. Aahhh.. with the multi-tile
> >> changes maybe it can't unroll the loops and calculate the masks at compile
> >> time. Maybe it is a bit too much and we should drop the
> >> __builtin_constant_p branch? Probably..
> >
> > Ah yes, with the code move to config_mask, they really all are compile time
> > constants (provided compiler can unroll the loops) so at least that is the
> > justfication for leaving the __builtin_constant_p in. So I'd probably just
> > leave it as is (though it is a bit too much).
> >
> >> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
> >> since there are no external callers (nothing coming from event) now. That
> >> way at least production builds don't have to have the check.
> >
> > Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
> > I guess.
> >
> > So I'm ok with the code staying as is. Enough bike-shed on this already.
>
> Latest series looks fine to me and thanks for your patience. Hope you would
> agree changing that one thing to u32 made more sense than changing the
> other to u64 so bike shed wasn't for nothing.

Hi Tvrtko, yes definitely, no issues :)

Thanks for your patience too.
--
Ashutosh