[Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Jani Nikula]

The implementation details of pmu should be implementation details
hidden inside i915_pmu.c. Make it so.

BR,
Jani.


Jani Nikula (5):
  drm/i915/pmu: report irqs to pmu code
  drm/i915/pmu: convert one more container_of() to event_to_pmu()
  drm/i915/pmu: change attr_group allocation and initialization
  drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c
  drm/i915: add a number of explicit includes to avoid implicit ones

 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   1 +
 drivers/gpu/drm/i915/gem/i915_gem_mman.c      |   1 +
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |   1 +
 .../drm/i915/gem/selftests/i915_gem_context.c |   2 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   1 +
 drivers/gpu/drm/i915/gt/selftest_execlists.c  |   1 +
 drivers/gpu/drm/i915/gt/selftest_migrate.c    |   1 +
 drivers/gpu/drm/i915/gt/selftest_slpc.c       |   2 +
 drivers/gpu/drm/i915/i915_drv.h               |   5 +-
 drivers/gpu/drm/i915/i915_irq.c               |   6 +-
 drivers/gpu/drm/i915/i915_pmu.c               | 216 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_pmu.h               | 138 +----------
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   1 +
 drivers/gpu/drm/i915/selftests/i915_request.c |   4 +-
 drivers/gpu/drm/i915/selftests/igt_mmap.c     |   2 +
 .../drm/i915/selftests/intel_memory_region.c  |   1 +
 16 files changed, 214 insertions(+), 169 deletions(-)

-- 
2.39.2

[Intel-gfx] [PATCH 1/5] drm/i915/pmu: report irqs to pmu code

[Jani Nikula]

Avoid accessing PMU details directly from irq code.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 +-----
 drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++++
 drivers/gpu/drm/i915/i915_pmu.h | 2 ++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8130f043693b..183520ba06bd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -71,11 +71,7 @@ static inline void pmu_irq_stats(struct drm_i915_private *i915,
 	if (unlikely(res != IRQ_HANDLED))
 		return;
 
-	/*
-	 * A clever compiler translates that into INC. A not so clever one
-	 * should at least prevent store tearing.
-	 */
-	WRITE_ONCE(i915->pmu.irq_count, i915->pmu.irq_count + 1);
+	i915_pmu_irq(i915);
 }
 
 void gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 878a27e1c8ef..ef4b907a799b 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -1348,3 +1348,12 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
 		kfree(pmu->name);
 	free_event_attributes(pmu);
 }
+
+void i915_pmu_irq(struct drm_i915_private *i915)
+{
+	/*
+	 * A clever compiler translates that into INC. A not so clever one
+	 * should at least prevent store tearing.
+	 */
+	WRITE_ONCE(i915->pmu.irq_count, i915->pmu.irq_count + 1);
+}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 41af038c3738..26b06132a44f 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -159,6 +159,7 @@ 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_irq(struct drm_i915_private *i915);
 void i915_pmu_gt_parked(struct intel_gt *gt);
 void i915_pmu_gt_unparked(struct intel_gt *gt);
 #else
@@ -166,6 +167,7 @@ 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_irq(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
-- 
2.39.2

[Intel-gfx] [PATCH 2/5] drm/i915/pmu: convert one more container_of() to event_to_pmu()

[Jani Nikula]

Use the helpers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index ef4b907a799b..21ef76a11ed7 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -832,9 +832,7 @@ static void i915_pmu_event_start(struct perf_event *event, int flags)
 
 static void i915_pmu_event_stop(struct perf_event *event, int flags)
 {
-	struct drm_i915_private *i915 =
-		container_of(event->pmu, typeof(*i915), pmu.base);
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu = event_to_pmu(event);
 
 	if (pmu->closed)
 		goto out;
-- 
2.39.2

[Intel-gfx] [PATCH 3/5] drm/i915/pmu: change attr_group allocation and initialization

[Jani Nikula]

Prepare for dynamically allocating struct i915_pmu by changing the
allocation and initialization of the attr_group. With pmu allocated
dynamically, pmu->events_attr_group can't be used for local attr_group
array initialization.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 21ef76a11ed7..3c6191b7fc82 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -1242,13 +1242,7 @@ static bool is_igp(struct drm_i915_private *i915)
 void i915_pmu_register(struct drm_i915_private *i915)
 {
 	struct i915_pmu *pmu = &i915->pmu;
-	const struct attribute_group *attr_groups[] = {
-		&i915_pmu_format_attr_group,
-		&pmu->events_attr_group,
-		&i915_pmu_cpumask_attr_group,
-		NULL
-	};
-
+	const struct attribute_group **attr_groups;
 	int ret = -ENOMEM;
 
 	if (GRAPHICS_VER(i915) <= 2) {
@@ -1281,11 +1275,17 @@ void i915_pmu_register(struct drm_i915_private *i915)
 	if (!pmu->events_attr_group.attrs)
 		goto err_name;
 
-	pmu->base.attr_groups = kmemdup(attr_groups, sizeof(attr_groups),
-					GFP_KERNEL);
-	if (!pmu->base.attr_groups)
+	attr_groups = kcalloc(4, sizeof(struct attribute_group *), GFP_KERNEL);
+	if (!attr_groups)
 		goto err_attr;
 
+	attr_groups[0] = &i915_pmu_format_attr_group;
+	attr_groups[1] = &pmu->events_attr_group;
+	attr_groups[2] = &i915_pmu_cpumask_attr_group;
+	attr_groups[3] = NULL; /* sentinel */
+
+	pmu->base.attr_groups = attr_groups;
+
 	pmu->base.module	= THIS_MODULE;
 	pmu->base.task_ctx_nr	= perf_invalid_context;
 	pmu->base.event_init	= i915_pmu_event_init;
-- 
2.39.2

[Intel-gfx] [PATCH 4/5] drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c

[Jani Nikula]

Abstract pmu code better by hiding struct i915_pmu and its internals in
i915_pmu.c. Allocate struct i915_pmu dynamically.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   5 +-
 drivers/gpu/drm/i915/i915_pmu.c | 185 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_pmu.h | 133 -----------------------
 3 files changed, 175 insertions(+), 148 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 259884b10d9a..29834432e7b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -65,8 +65,9 @@
 #include "intel_uncore.h"
 
 struct drm_i915_clock_gating_funcs;
-struct vlv_s0ix_state;
+struct i915_pmu;
 struct intel_pxp;
+struct vlv_s0ix_state;
 
 #define GEM_QUIRK_PIN_SWIZZLED_PAGES	BIT(0)
 
@@ -363,7 +364,7 @@ struct drm_i915_private {
 
 	bool irq_enabled;
 
-	struct i915_pmu pmu;
+	struct i915_pmu *pmu;
 
 	/* The TTM device structure. */
 	struct ttm_device bdev;
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 3c6191b7fc82..d26e3c421663 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -4,7 +4,10 @@
  * Copyright © 2017-2018 Intel Corporation
  */
 
+#include <linux/hrtimer.h>
+#include <linux/perf_event.h>
 #include <linux/pm_runtime.h>
+#include <linux/spinlock_types.h>
 
 #include "gt/intel_engine.h"
 #include "gt/intel_engine_pm.h"
@@ -19,6 +22,143 @@
 #include "i915_drv.h"
 #include "i915_pmu.h"
 
+/*
+ * Non-engine events that we need to track enabled-disabled transition and
+ * current state.
+ */
+enum i915_pmu_tracked_events {
+	__I915_PMU_ACTUAL_FREQUENCY_ENABLED = 0,
+	__I915_PMU_REQUESTED_FREQUENCY_ENABLED,
+	__I915_PMU_RC6_RESIDENCY_ENABLED,
+	__I915_PMU_TRACKED_EVENT_COUNT, /* count marker */
+};
+
+/*
+ * Slots used from the sampling timer (non-engine events) with some extras for
+ * convenience.
+ */
+enum {
+	__I915_SAMPLE_FREQ_ACT = 0,
+	__I915_SAMPLE_FREQ_REQ,
+	__I915_SAMPLE_RC6,
+	__I915_SAMPLE_RC6_LAST_REPORTED,
+	__I915_NUM_PMU_SAMPLERS
+};
+
+#define I915_PMU_MAX_GT 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_MAX_GT * __I915_PMU_TRACKED_EVENT_COUNT)
+
+struct i915_pmu {
+	/**
+	 * @i915: i915 device backpointer.
+	 */
+	struct drm_i915_private *i915;
+	/**
+	 * @cpuhp: Struct used for CPU hotplug handling.
+	 */
+	struct {
+		struct hlist_node node;
+		unsigned int cpu;
+	} cpuhp;
+	/**
+	 * @base: PMU base.
+	 */
+	struct pmu base;
+	/**
+	 * @closed: i915 is unregistering.
+	 */
+	bool closed;
+	/**
+	 * @name: Name as registered with perf core.
+	 */
+	const char *name;
+	/**
+	 * @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.
+	 */
+	struct hrtimer timer;
+	/**
+	 * @enable: Bitmask of specific enabled events.
+	 *
+	 * For some events we need to track their state and do some internal
+	 * house keeping.
+	 *
+	 * Each engine event sampler type and event listed in enum
+	 * i915_pmu_tracked_events gets a bit in this field.
+	 *
+	 * Low bits are engine samplers and other events continue from there.
+	 */
+	u32 enable;
+
+	/**
+	 * @timer_last:
+	 *
+	 * Timestmap of the previous timer invocation.
+	 */
+	ktime_t timer_last;
+
+	/**
+	 * @enable_count: Reference counts for the enabled events.
+	 *
+	 * Array indices are mapped in the same way as bits in the @enable field
+	 * and they are used to control sampling on/off when multiple clients
+	 * are using the PMU API.
+	 */
+	unsigned int enable_count[I915_PMU_MASK_BITS];
+	/**
+	 * @timer_enabled: Should the internal sampling timer be running.
+	 */
+	bool timer_enabled;
+	/**
+	 * @sample: Current and previous (raw) counters for sampling events.
+	 *
+	 * These counters are updated from the i915 PMU sampling timer.
+	 *
+	 * Only global counters are held here, while the per-engine ones are in
+	 * struct intel_engine_cs.
+	 */
+	struct i915_pmu_sample sample[I915_PMU_MAX_GT][__I915_NUM_PMU_SAMPLERS];
+	/**
+	 * @sleep_last: Last time GT parked for RC6 estimation.
+	 */
+	ktime_t sleep_last[I915_PMU_MAX_GT];
+	/**
+	 * @irq_count: Number of interrupts
+	 *
+	 * Intentionally unsigned long to avoid atomics or heuristics on 32bit.
+	 * 4e9 interrupts are a lot and postprocessing can really deal with an
+	 * occasional wraparound easily. It's 32bit after all.
+	 */
+	unsigned long irq_count;
+	/**
+	 * @events_attr_group: Device events attribute group.
+	 */
+	struct attribute_group events_attr_group;
+	/**
+	 * @i915_attr: Memory block holding device attributes.
+	 */
+	void *i915_attr;
+	/**
+	 * @pmu_attr: Memory block holding device attributes.
+	 */
+	void *pmu_attr;
+};
+
 /* Frequency for the sampling timer for events which need it. */
 #define FREQUENCY 200
 #define PERIOD max_t(u64, 10000, NSEC_PER_SEC / FREQUENCY)
@@ -38,7 +178,7 @@ static struct i915_pmu *event_to_pmu(struct perf_event *event)
 
 static struct drm_i915_private *pmu_to_i915(struct i915_pmu *pmu)
 {
-	return container_of(pmu, struct drm_i915_private, pmu);
+	return pmu->i915;
 }
 
 static u8 engine_config_sample(u64 config)
@@ -222,7 +362,7 @@ 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;
+	struct i915_pmu *pmu = i915->pmu;
 	unsigned long flags;
 	bool awake = false;
 	u64 val;
@@ -281,7 +421,7 @@ static void init_rc6(struct i915_pmu *pmu)
 
 static void park_rc6(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &gt->i915->pmu;
+	struct i915_pmu *pmu = gt->i915->pmu;
 
 	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
 	pmu->sleep_last[gt->info.id] = ktime_get_raw();
@@ -300,9 +440,9 @@ static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
 
 void i915_pmu_gt_parked(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &gt->i915->pmu;
+	struct i915_pmu *pmu = gt->i915->pmu;
 
-	if (!pmu->base.event_init)
+	if (!pmu || !pmu->base.event_init)
 		return;
 
 	spin_lock_irq(&pmu->lock);
@@ -322,9 +462,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
 
 void i915_pmu_gt_unparked(struct intel_gt *gt)
 {
-	struct i915_pmu *pmu = &gt->i915->pmu;
+	struct i915_pmu *pmu = gt->i915->pmu;
 
-	if (!pmu->base.event_init)
+	if (!pmu || !pmu->base.event_init)
 		return;
 
 	spin_lock_irq(&pmu->lock);
@@ -399,7 +539,7 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
 	enum intel_engine_id id;
 	unsigned long flags;
 
-	if ((i915->pmu.enable & ENGINE_SAMPLE_MASK) == 0)
+	if ((i915->pmu->enable & ENGINE_SAMPLE_MASK) == 0)
 		return;
 
 	if (!intel_gt_pm_is_awake(gt))
@@ -437,7 +577,7 @@ 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 i915_pmu *pmu = i915->pmu;
 	struct intel_rps *rps = &gt->rps;
 
 	if (!frequency_sampling_enabled(pmu, gt_id))
@@ -1241,7 +1381,7 @@ static bool is_igp(struct drm_i915_private *i915)
 
 void i915_pmu_register(struct drm_i915_private *i915)
 {
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu;
 	const struct attribute_group **attr_groups;
 	int ret = -ENOMEM;
 
@@ -1250,6 +1390,13 @@ void i915_pmu_register(struct drm_i915_private *i915)
 		return;
 	}
 
+	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return;
+
+	i915->pmu = pmu;
+	pmu->i915 = i915;
+
 	spin_lock_init(&pmu->lock);
 	hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	pmu->timer.function = i915_sample;
@@ -1317,16 +1464,21 @@ void i915_pmu_register(struct drm_i915_private *i915)
 	if (!is_igp(i915))
 		kfree(pmu->name);
 err:
+	kfree(pmu);
+	i915->pmu = NULL;
 	drm_notice(&i915->drm, "Failed to register PMU!\n");
 }
 
 void i915_pmu_unregister(struct drm_i915_private *i915)
 {
-	struct i915_pmu *pmu = &i915->pmu;
+	struct i915_pmu *pmu = i915->pmu;
 
-	if (!pmu->base.event_init)
+	if (!pmu)
 		return;
 
+	if (!pmu->base.event_init)
+		goto out;
+
 	/*
 	 * "Disconnect" the PMU callbacks - since all are atomic synchronize_rcu
 	 * ensures all currently executing ones will have exited before we
@@ -1345,13 +1497,20 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
 	if (!is_igp(i915))
 		kfree(pmu->name);
 	free_event_attributes(pmu);
+
+out:
+	kfree(i915->pmu);
+	i915->pmu = NULL;
 }
 
 void i915_pmu_irq(struct drm_i915_private *i915)
 {
+	if (!i915->pmu)
+		return;
+
 	/*
 	 * A clever compiler translates that into INC. A not so clever one
 	 * should at least prevent store tearing.
 	 */
-	WRITE_ONCE(i915->pmu.irq_count, i915->pmu.irq_count + 1);
+	WRITE_ONCE(i915->pmu->irq_count, i915->pmu->irq_count + 1);
 }
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 26b06132a44f..bd2f9a62413e 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -15,145 +15,12 @@
 struct drm_i915_private;
 struct intel_gt;
 
-/*
- * Non-engine events that we need to track enabled-disabled transition and
- * current state.
- */
-enum i915_pmu_tracked_events {
-	__I915_PMU_ACTUAL_FREQUENCY_ENABLED = 0,
-	__I915_PMU_REQUESTED_FREQUENCY_ENABLED,
-	__I915_PMU_RC6_RESIDENCY_ENABLED,
-	__I915_PMU_TRACKED_EVENT_COUNT, /* count marker */
-};
-
-/*
- * Slots used from the sampling timer (non-engine events) with some extras for
- * convenience.
- */
-enum {
-	__I915_SAMPLE_FREQ_ACT = 0,
-	__I915_SAMPLE_FREQ_REQ,
-	__I915_SAMPLE_RC6,
-	__I915_SAMPLE_RC6_LAST_REPORTED,
-	__I915_NUM_PMU_SAMPLERS
-};
-
-#define I915_PMU_MAX_GT 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_MAX_GT * __I915_PMU_TRACKED_EVENT_COUNT)
-
 #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
 
 struct i915_pmu_sample {
 	u64 cur;
 };
 
-struct i915_pmu {
-	/**
-	 * @cpuhp: Struct used for CPU hotplug handling.
-	 */
-	struct {
-		struct hlist_node node;
-		unsigned int cpu;
-	} cpuhp;
-	/**
-	 * @base: PMU base.
-	 */
-	struct pmu base;
-	/**
-	 * @closed: i915 is unregistering.
-	 */
-	bool closed;
-	/**
-	 * @name: Name as registered with perf core.
-	 */
-	const char *name;
-	/**
-	 * @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.
-	 */
-	struct hrtimer timer;
-	/**
-	 * @enable: Bitmask of specific enabled events.
-	 *
-	 * For some events we need to track their state and do some internal
-	 * house keeping.
-	 *
-	 * Each engine event sampler type and event listed in enum
-	 * i915_pmu_tracked_events gets a bit in this field.
-	 *
-	 * Low bits are engine samplers and other events continue from there.
-	 */
-	u32 enable;
-
-	/**
-	 * @timer_last:
-	 *
-	 * Timestmap of the previous timer invocation.
-	 */
-	ktime_t timer_last;
-
-	/**
-	 * @enable_count: Reference counts for the enabled events.
-	 *
-	 * Array indices are mapped in the same way as bits in the @enable field
-	 * and they are used to control sampling on/off when multiple clients
-	 * are using the PMU API.
-	 */
-	unsigned int enable_count[I915_PMU_MASK_BITS];
-	/**
-	 * @timer_enabled: Should the internal sampling timer be running.
-	 */
-	bool timer_enabled;
-	/**
-	 * @sample: Current and previous (raw) counters for sampling events.
-	 *
-	 * These counters are updated from the i915 PMU sampling timer.
-	 *
-	 * Only global counters are held here, while the per-engine ones are in
-	 * struct intel_engine_cs.
-	 */
-	struct i915_pmu_sample sample[I915_PMU_MAX_GT][__I915_NUM_PMU_SAMPLERS];
-	/**
-	 * @sleep_last: Last time GT parked for RC6 estimation.
-	 */
-	ktime_t sleep_last[I915_PMU_MAX_GT];
-	/**
-	 * @irq_count: Number of interrupts
-	 *
-	 * Intentionally unsigned long to avoid atomics or heuristics on 32bit.
-	 * 4e9 interrupts are a lot and postprocessing can really deal with an
-	 * occasional wraparound easily. It's 32bit after all.
-	 */
-	unsigned long irq_count;
-	/**
-	 * @events_attr_group: Device events attribute group.
-	 */
-	struct attribute_group events_attr_group;
-	/**
-	 * @i915_attr: Memory block holding device attributes.
-	 */
-	void *i915_attr;
-	/**
-	 * @pmu_attr: Memory block holding device attributes.
-	 */
-	void *pmu_attr;
-};
-
 #ifdef CONFIG_PERF_EVENTS
 int i915_pmu_init(void);
 void i915_pmu_exit(void);
-- 
2.39.2

[Intel-gfx] [PATCH 5/5] drm/i915: add a number of explicit includes to avoid implicit ones

[Jani Nikula]

There are a number of places that depend on including <linux/file.h> and
<linux/kthread.h> implicitly and indirectly via includes in
i915_pmu.h. Make them explicit so we can remove includes from
i915_pmu.h.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c        | 1 +
 drivers/gpu/drm/i915/gem/i915_gem_mman.c              | 1 +
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c       | 1 +
 drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 2 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c             | 1 +
 drivers/gpu/drm/i915/gt/selftest_execlists.c          | 1 +
 drivers/gpu/drm/i915/gt/selftest_migrate.c            | 1 +
 drivers/gpu/drm/i915/gt/selftest_slpc.c               | 2 ++
 drivers/gpu/drm/i915/i915_pmu.h                       | 3 ---
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c         | 1 +
 drivers/gpu/drm/i915/selftests/i915_request.c         | 4 +++-
 drivers/gpu/drm/i915/selftests/igt_mmap.c             | 2 ++
 drivers/gpu/drm/i915/selftests/intel_memory_region.c  | 1 +
 13 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 683fd8d3151c..34618e3ae926 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/dma-resv.h>
+#include <linux/file.h>
 #include <linux/highmem.h>
 #include <linux/sync_file.h>
 #include <linux/uaccess.h>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 310654542b42..2a1416cd9d95 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/anon_inodes.h>
+#include <linux/file.h>
 #include <linux/mman.h>
 #include <linux/pfn_t.h>
 #include <linux/sizes.h>
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index 6b9f6cf50bf6..3dd29f6e3446 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -4,6 +4,7 @@
  * Copyright © 2017 Intel Corporation
  */
 
+#include <linux/file.h>
 #include <linux/prime_numbers.h>
 #include <linux/string_helpers.h>
 #include <linux/swap.h>
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index 7021b6e9b219..45935ef5b59d 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -4,6 +4,8 @@
  * Copyright © 2017 Intel Corporation
  */
 
+#include <linux/file.h>
+#include <linux/kthread.h>
 #include <linux/prime_numbers.h>
 #include <linux/string_helpers.h>
 
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 4a11219e560e..7b66814eb00e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -3,6 +3,7 @@
  * Copyright © 2016 Intel Corporation
  */
 
+#include <linux/file.h>
 #include <linux/string_helpers.h>
 
 #include <drm/drm_print.h>
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index 4202df5b8c12..e938dcab2f40 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -3,6 +3,7 @@
  * Copyright © 2018 Intel Corporation
  */
 
+#include <linux/kthread.h>
 #include <linux/prime_numbers.h>
 
 #include "gem/i915_gem_internal.h"
diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c
index 1a34cbe04fb6..f03e3c1d43ab 100644
--- a/drivers/gpu/drm/i915/gt/selftest_migrate.c
+++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c
@@ -3,6 +3,7 @@
  * Copyright © 2020 Intel Corporation
  */
 
+#include <linux/kthread.h>
 #include <linux/sort.h>
 
 #include "gem/i915_gem_internal.h"
diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index 952c8d52d68a..a8b48fbfd833 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -3,6 +3,8 @@
  * Copyright © 2021 Intel Corporation
  */
 
+#include <linux/kthread.h>
+
 #define NUM_STEPS 5
 #define H2G_DELAY 50000
 #define delay_for_h2g() usleep_range(H2G_DELAY, H2G_DELAY + 10000)
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index bd2f9a62413e..5655104846da 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -7,9 +7,6 @@
 #ifndef __I915_PMU_H__
 #define __I915_PMU_H__
 
-#include <linux/hrtimer.h>
-#include <linux/perf_event.h>
-#include <linux/spinlock_types.h>
 #include <uapi/drm/i915_drm.h>
 
 struct drm_i915_private;
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
index 5c397a2df70e..5e9fea5784bf 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
@@ -22,6 +22,7 @@
  *
  */
 
+#include <linux/file.h>
 #include <linux/list_sort.h>
 #include <linux/prime_numbers.h>
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c
index acae30a04a94..e9dff93a6b54 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -22,8 +22,10 @@
  *
  */
 
-#include <linux/prime_numbers.h>
+#include <linux/file.h>
+#include <linux/kthread.h>
 #include <linux/pm_qos.h>
+#include <linux/prime_numbers.h>
 #include <linux/sort.h>
 
 #include "gem/i915_gem_internal.h"
diff --git a/drivers/gpu/drm/i915/selftests/igt_mmap.c b/drivers/gpu/drm/i915/selftests/igt_mmap.c
index e920a461bd36..2f8f927f71df 100644
--- a/drivers/gpu/drm/i915/selftests/igt_mmap.c
+++ b/drivers/gpu/drm/i915/selftests/igt_mmap.c
@@ -4,6 +4,8 @@
  * Copyright © 2019 Intel Corporation
  */
 
+#include <linux/file.h>
+
 #include <drm/drm_file.h>
 
 #include "i915_drv.h"
diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
index d985d9bae2e8..355d4af1f884 100644
--- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
@@ -3,6 +3,7 @@
  * Copyright © 2019 Intel Corporation
  */
 
+#include <linux/file.h>
 #include <linux/prime_numbers.h>
 #include <linux/sort.h>
 
-- 
2.39.2

Re: [Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Tvrtko Ursulin]

On 02/11/2023 15:42, Jani Nikula wrote:
> The implementation details of pmu should be implementation details
> hidden inside i915_pmu.c. Make it so.

Don't tell me i915->pmu bothers xe somehow? I am not a fan of the series 
on a glance. Replacing an increment with a function call for instance.

Regards,

Tvrtko

> BR,
> Jani.
> 
> 
> Jani Nikula (5):
>    drm/i915/pmu: report irqs to pmu code
>    drm/i915/pmu: convert one more container_of() to event_to_pmu()
>    drm/i915/pmu: change attr_group allocation and initialization
>    drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c
>    drm/i915: add a number of explicit includes to avoid implicit ones
> 
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   1 +
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c      |   1 +
>   .../gpu/drm/i915/gem/selftests/huge_pages.c   |   1 +
>   .../drm/i915/gem/selftests/i915_gem_context.c |   2 +
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   1 +
>   drivers/gpu/drm/i915/gt/selftest_execlists.c  |   1 +
>   drivers/gpu/drm/i915/gt/selftest_migrate.c    |   1 +
>   drivers/gpu/drm/i915/gt/selftest_slpc.c       |   2 +
>   drivers/gpu/drm/i915/i915_drv.h               |   5 +-
>   drivers/gpu/drm/i915/i915_irq.c               |   6 +-
>   drivers/gpu/drm/i915/i915_pmu.c               | 216 ++++++++++++++++--
>   drivers/gpu/drm/i915/i915_pmu.h               | 138 +----------
>   drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   1 +
>   drivers/gpu/drm/i915/selftests/i915_request.c |   4 +-
>   drivers/gpu/drm/i915/selftests/igt_mmap.c     |   2 +
>   .../drm/i915/selftests/intel_memory_region.c  |   1 +
>   16 files changed, 214 insertions(+), 169 deletions(-)
> 

Re: [Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Jani Nikula]

On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 02/11/2023 15:42, Jani Nikula wrote:
>> The implementation details of pmu should be implementation details
>> hidden inside i915_pmu.c. Make it so.
>
> Don't tell me i915->pmu bothers xe somehow?

It doesn't bother xe, it bothers me...

> I am not a fan of the series 
> on a glance. Replacing an increment with a function call for instance.

I think you glanced at the wart of this series. ;)

It just bugs me that we expose a plethora of data that should be
internal to pmu, basically just for that one increment.

And we pull in a bunch of headers to define struct i915_pmu, and then we
implicitly depend on those headers in a ton of places through incredible
chains of includes.

And we rebuild everything and a half when those headers change. Or when
the pmu implementation details change.


BR,
Jani.

>
> Regards,
>
> Tvrtko
>
>> BR,
>> Jani.
>> 
>> 
>> Jani Nikula (5):
>>    drm/i915/pmu: report irqs to pmu code
>>    drm/i915/pmu: convert one more container_of() to event_to_pmu()
>>    drm/i915/pmu: change attr_group allocation and initialization
>>    drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c
>>    drm/i915: add a number of explicit includes to avoid implicit ones
>> 
>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   1 +
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c      |   1 +
>>   .../gpu/drm/i915/gem/selftests/huge_pages.c   |   1 +
>>   .../drm/i915/gem/selftests/i915_gem_context.c |   2 +
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   1 +
>>   drivers/gpu/drm/i915/gt/selftest_execlists.c  |   1 +
>>   drivers/gpu/drm/i915/gt/selftest_migrate.c    |   1 +
>>   drivers/gpu/drm/i915/gt/selftest_slpc.c       |   2 +
>>   drivers/gpu/drm/i915/i915_drv.h               |   5 +-
>>   drivers/gpu/drm/i915/i915_irq.c               |   6 +-
>>   drivers/gpu/drm/i915/i915_pmu.c               | 216 ++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_pmu.h               | 138 +----------
>>   drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   1 +
>>   drivers/gpu/drm/i915/selftests/i915_request.c |   4 +-
>>   drivers/gpu/drm/i915/selftests/igt_mmap.c     |   2 +
>>   .../drm/i915/selftests/intel_memory_region.c  |   1 +
>>   16 files changed, 214 insertions(+), 169 deletions(-)
>> 

-- 
Jani Nikula, Intel

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/pmu: hide struct i915_pmu

[Patchwork]

== Series Details ==

Series: drm/i915/pmu: hide struct i915_pmu
URL   : https://patchwork.freedesktop.org/series/125910/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/i915_drm_client.o
In file included from ./drivers/gpu/drm/i915/i915_scheduler.h:14,
                 from drivers/gpu/drm/i915/gem/i915_gem_context_types.h:22,
                 from drivers/gpu/drm/i915/gem/i915_gem_context.h:10,
                 from drivers/gpu/drm/i915/i915_drm_client.c:14:
./drivers/gpu/drm/i915/i915_scheduler_types.h:130:24: error: field ‘tasklet’ has incomplete type
  130 |  struct tasklet_struct tasklet;
      |                        ^~~~~~~
make[6]: *** [scripts/Makefile.build:243: drivers/gpu/drm/i915/i915_drm_client.o] Error 1
make[5]: *** [scripts/Makefile.build:480: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:480: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:480: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:480: drivers] Error 2
make[1]: *** [/home/kbuild2/kernel/Makefile:1913: .] Error 2
make: *** [Makefile:234: __sub-make] Error 2
Build failed, no error log produced

Re: [Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Tvrtko Ursulin]

On 02/11/2023 16:47, Jani Nikula wrote:
> On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 02/11/2023 15:42, Jani Nikula wrote:
>>> The implementation details of pmu should be implementation details
>>> hidden inside i915_pmu.c. Make it so.
>>
>> Don't tell me i915->pmu bothers xe somehow?
> 
> It doesn't bother xe, it bothers me...
> 
>> I am not a fan of the series
>> on a glance. Replacing an increment with a function call for instance.
> 
> I think you glanced at the wart of this series. ;)
> 
> It just bugs me that we expose a plethora of data that should be
> internal to pmu, basically just for that one increment.
> 
> And we pull in a bunch of headers to define struct i915_pmu, and then we
> implicitly depend on those headers in a ton of places through incredible
> chains of includes.
> 
> And we rebuild everything and a half when those headers change. Or when
> the pmu implementation details change.

On the other hand i915_pmu.h is a small header file, which does not 
include _any_ other internal i915 headers (only uapi) and is always 
present (if you ignore gens <= 2) which does not driver the allocate on 
demand approach. In the past three years it had like seven edits.

Given all that, the change of direction you propose here sounds a bit 
radical and I would rather not replace that increment with a function 
call, when it is questionable if the same separation of components 
approach can be, or will be, applied to the whole driver. Gut feeling 
says it is bound to be hard and possibly not happen in which case I 
don't see what is gained by churning on the tiny and quiet i915_pmu.h|c.

Regards,

Tvrtko

> 
> 
> BR,
> Jani.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> BR,
>>> Jani.
>>>
>>>
>>> Jani Nikula (5):
>>>     drm/i915/pmu: report irqs to pmu code
>>>     drm/i915/pmu: convert one more container_of() to event_to_pmu()
>>>     drm/i915/pmu: change attr_group allocation and initialization
>>>     drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c
>>>     drm/i915: add a number of explicit includes to avoid implicit ones
>>>
>>>    .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   1 +
>>>    drivers/gpu/drm/i915/gem/i915_gem_mman.c      |   1 +
>>>    .../gpu/drm/i915/gem/selftests/huge_pages.c   |   1 +
>>>    .../drm/i915/gem/selftests/i915_gem_context.c |   2 +
>>>    drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   1 +
>>>    drivers/gpu/drm/i915/gt/selftest_execlists.c  |   1 +
>>>    drivers/gpu/drm/i915/gt/selftest_migrate.c    |   1 +
>>>    drivers/gpu/drm/i915/gt/selftest_slpc.c       |   2 +
>>>    drivers/gpu/drm/i915/i915_drv.h               |   5 +-
>>>    drivers/gpu/drm/i915/i915_irq.c               |   6 +-
>>>    drivers/gpu/drm/i915/i915_pmu.c               | 216 ++++++++++++++++--
>>>    drivers/gpu/drm/i915/i915_pmu.h               | 138 +----------
>>>    drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   1 +
>>>    drivers/gpu/drm/i915/selftests/i915_request.c |   4 +-
>>>    drivers/gpu/drm/i915/selftests/igt_mmap.c     |   2 +
>>>    .../drm/i915/selftests/intel_memory_region.c  |   1 +
>>>    16 files changed, 214 insertions(+), 169 deletions(-)
>>>
> 

Re: [Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Jani Nikula]

On Fri, 03 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 02/11/2023 16:47, Jani Nikula wrote:
>> On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 02/11/2023 15:42, Jani Nikula wrote:
>>>> The implementation details of pmu should be implementation details
>>>> hidden inside i915_pmu.c. Make it so.
>>>
>>> Don't tell me i915->pmu bothers xe somehow?
>> 
>> It doesn't bother xe, it bothers me...
>> 
>>> I am not a fan of the series
>>> on a glance. Replacing an increment with a function call for instance.
>> 
>> I think you glanced at the wart of this series. ;)
>> 
>> It just bugs me that we expose a plethora of data that should be
>> internal to pmu, basically just for that one increment.
>> 
>> And we pull in a bunch of headers to define struct i915_pmu, and then we
>> implicitly depend on those headers in a ton of places through incredible
>> chains of includes.
>> 
>> And we rebuild everything and a half when those headers change. Or when
>> the pmu implementation details change.
>
> On the other hand i915_pmu.h is a small header file, which does not 
> include _any_ other internal i915 headers (only uapi) and is always 
> present (if you ignore gens <= 2) which does not driver the allocate on 
> demand approach. In the past three years it had like seven edits.

It's really not about allocation on demand at all. It's about hiding the
implementation, using the mechanisms that C provides us for
abstractions, and reducing the header dependencies.

> Given all that, the change of direction you propose here sounds a bit 
> radical and I would rather not replace that increment with a function 
> call, when it is questionable if the same separation of components 
> approach can be, or will be, applied to the whole driver. Gut feeling 
> says it is bound to be hard and possibly not happen in which case I 
> don't see what is gained by churning on the tiny and quiet i915_pmu.h|c.

Yeah, the approach probably won't be applied to the whole driver anytime
soon. Maybe never. What matters to me though is that all of these set
the example. People look at drm_i915_private, and always just add a new
struct member, and never even stop to think if they could make it an
opaque pointer instead.

Heck, the same damn approach was copy-pasted to the xe driver, warts and
all.

Anyway.

All that said, I think I'm going to drop this, along with all
refactoring that isn't strictly related to display.

BR,
Jani.

-- 
Jani Nikula, Intel