diff mbox series

drm/i915/pmu: Avoid using globals for per-device state

Message ID 20200219150804.27158-1-michal.winiarski@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/pmu: Avoid using globals for per-device state | expand

Commit Message

Michał Winiarski Feb. 19, 2020, 3:08 p.m. UTC
Attempting to bind / unbind module from devices where we have both
integrated and discreete GPU handed by i915 may lead to interesting
results where we're keeping per-device state in per-module globals.

Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 56 ++++++++++++++++++---------------
 drivers/gpu/drm/i915/i915_pmu.h |  8 +++++
 2 files changed, 39 insertions(+), 25 deletions(-)

Comments

Chris Wilson Feb. 19, 2020, 3:25 p.m. UTC | #1
Quoting Michał Winiarski (2020-02-19 15:08:04)
> Attempting to bind / unbind module from devices where we have both
> integrated and discreete GPU handed by i915 may lead to interesting
> results where we're keeping per-device state in per-module globals.
> 
> Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> -       i915_pmu_events_attr_group.attrs = create_event_attributes(pmu);
> -       if (!i915_pmu_events_attr_group.attrs)
> +       pmu->events_attr_group.name = "events";
> +       pmu->events_attr_group.attrs = create_event_attributes(pmu);
> +       if (!pmu->events_attr_group.attrs)
>                 goto err_name;
>  
> -       pmu->base.attr_groups   = i915_pmu_attr_groups;
> +       pmu->base.attr_groups = kcalloc(ARRAY_SIZE(attr_groups),
> +                                       sizeof(*attr_groups),
> +                                       GFP_KERNEL);
> +       if (!pmu->base.attr_groups)
> +               goto err_attr;
> +       memcpy(attr_groups, pmu->base.attr_groups,
> +              ARRAY_SIZE(attr_groups) * sizeof(*attr_groups));

kmemdup(attr_groups, sizeof(attr_groups));

> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 6c1647c5daf2..dc1361e8e27a 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -42,6 +42,10 @@ struct i915_pmu {
>          * @node: List node for CPU hotplug handling.
>          */
>         struct hlist_node node;
> +       /**
> +        * @cpuhp_slot: State for CPU hotplug handling.
> +        */
> +       enum cpuhp_state cpuhp_slot;

Perhaps struct {
		struct hlist_node node;
		enum cpuhp_state slot;
	} cpuhp;

Fwiw, separate this into a separate patch, so we have one to deglobal
cpuhp and one for event groups.

Just grimacing over the wasted strings that we could intern for actual
attr.

But the essence of the patch is correct, since the events group is
created at runtime from probing the device, it is not global.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index a3b61fb96226..1b4fdcb9045d 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -822,11 +822,6 @@  static ssize_t i915_pmu_event_show(struct device *dev,
 	return sprintf(buf, "config=0x%lx\n", eattr->val);
 }
 
-static struct attribute_group i915_pmu_events_attr_group = {
-	.name = "events",
-	/* Patch in attrs at runtime. */
-};
-
 static ssize_t
 i915_pmu_get_attr_cpumask(struct device *dev,
 			  struct device_attribute *attr,
@@ -846,13 +841,6 @@  static const struct attribute_group i915_pmu_cpumask_attr_group = {
 	.attrs = i915_cpumask_attrs,
 };
 
-static const struct attribute_group *i915_pmu_attr_groups[] = {
-	&i915_pmu_format_attr_group,
-	&i915_pmu_events_attr_group,
-	&i915_pmu_cpumask_attr_group,
-	NULL
-};
-
 #define __event(__config, __name, __unit) \
 { \
 	.config = (__config), \
@@ -1026,16 +1014,16 @@  err:;
 
 static void free_event_attributes(struct i915_pmu *pmu)
 {
-	struct attribute **attr_iter = i915_pmu_events_attr_group.attrs;
+	struct attribute **attr_iter = pmu->events_attr_group.attrs;
 
 	for (; *attr_iter; attr_iter++)
 		kfree((*attr_iter)->name);
 
-	kfree(i915_pmu_events_attr_group.attrs);
+	kfree(pmu->events_attr_group.attrs);
 	kfree(pmu->i915_attr);
 	kfree(pmu->pmu_attr);
 
-	i915_pmu_events_attr_group.attrs = NULL;
+	pmu->events_attr_group.attrs = NULL;
 	pmu->i915_attr = NULL;
 	pmu->pmu_attr = NULL;
 }
@@ -1072,8 +1060,6 @@  static int i915_pmu_cpu_offline(unsigned int cpu, struct hlist_node *node)
 	return 0;
 }
 
-static enum cpuhp_state cpuhp_slot = CPUHP_INVALID;
-
 static int i915_pmu_register_cpuhp_state(struct i915_pmu *pmu)
 {
 	enum cpuhp_state slot;
@@ -1093,15 +1079,16 @@  static int i915_pmu_register_cpuhp_state(struct i915_pmu *pmu)
 		return ret;
 	}
 
-	cpuhp_slot = slot;
+	pmu->cpuhp_slot = slot;
 	return 0;
 }
 
 static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu)
 {
-	WARN_ON(cpuhp_slot == CPUHP_INVALID);
-	WARN_ON(cpuhp_state_remove_instance(cpuhp_slot, &pmu->node));
-	cpuhp_remove_multi_state(cpuhp_slot);
+	WARN_ON(pmu->cpuhp_slot == CPUHP_INVALID);
+	WARN_ON(cpuhp_state_remove_instance(pmu->cpuhp_slot, &pmu->node));
+	cpuhp_remove_multi_state(pmu->cpuhp_slot);
+	pmu->cpuhp_slot = CPUHP_INVALID;
 }
 
 static bool is_igp(struct drm_i915_private *i915)
@@ -1118,6 +1105,13 @@  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
+	};
+
 	int ret = -ENOMEM;
 
 	if (INTEL_GEN(i915) <= 2) {
@@ -1128,6 +1122,7 @@  void i915_pmu_register(struct drm_i915_private *i915)
 	spin_lock_init(&pmu->lock);
 	hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	pmu->timer.function = i915_sample;
+	pmu->cpuhp_slot = CPUHP_INVALID;
 
 	if (!is_igp(i915)) {
 		pmu->name = kasprintf(GFP_KERNEL,
@@ -1143,11 +1138,19 @@  void i915_pmu_register(struct drm_i915_private *i915)
 	if (!pmu->name)
 		goto err;
 
-	i915_pmu_events_attr_group.attrs = create_event_attributes(pmu);
-	if (!i915_pmu_events_attr_group.attrs)
+	pmu->events_attr_group.name = "events";
+	pmu->events_attr_group.attrs = create_event_attributes(pmu);
+	if (!pmu->events_attr_group.attrs)
 		goto err_name;
 
-	pmu->base.attr_groups	= i915_pmu_attr_groups;
+	pmu->base.attr_groups = kcalloc(ARRAY_SIZE(attr_groups),
+					sizeof(*attr_groups),
+					GFP_KERNEL);
+	if (!pmu->base.attr_groups)
+		goto err_attr;
+	memcpy(attr_groups, pmu->base.attr_groups,
+	       ARRAY_SIZE(attr_groups) * sizeof(*attr_groups));
+
 	pmu->base.task_ctx_nr	= perf_invalid_context;
 	pmu->base.event_init	= i915_pmu_event_init;
 	pmu->base.add		= i915_pmu_event_add;
@@ -1159,7 +1162,7 @@  void i915_pmu_register(struct drm_i915_private *i915)
 
 	ret = perf_pmu_register(&pmu->base, pmu->name, -1);
 	if (ret)
-		goto err_attr;
+		goto err_groups;
 
 	ret = i915_pmu_register_cpuhp_state(pmu);
 	if (ret)
@@ -1169,6 +1172,8 @@  void i915_pmu_register(struct drm_i915_private *i915)
 
 err_unreg:
 	perf_pmu_unregister(&pmu->base);
+err_groups:
+	kfree(pmu->base.attr_groups);
 err_attr:
 	pmu->base.event_init = NULL;
 	free_event_attributes(pmu);
@@ -1194,6 +1199,7 @@  void i915_pmu_unregister(struct drm_i915_private *i915)
 
 	perf_pmu_unregister(&pmu->base);
 	pmu->base.event_init = NULL;
+	kfree(pmu->base.attr_groups);
 	if (!is_igp(i915))
 		kfree(pmu->name);
 	free_event_attributes(pmu);
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 6c1647c5daf2..dc1361e8e27a 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -42,6 +42,10 @@  struct i915_pmu {
 	 * @node: List node for CPU hotplug handling.
 	 */
 	struct hlist_node node;
+	/**
+	 * @cpuhp_slot: State for CPU hotplug handling.
+	 */
+	enum cpuhp_state cpuhp_slot;
 	/**
 	 * @base: PMU base.
 	 */
@@ -104,6 +108,10 @@  struct i915_pmu {
 	 * @sleep_last: Last time GT parked for RC6 estimation.
 	 */
 	ktime_t sleep_last;
+	/**
+	 * @events_attr_group: Device events attribute group.
+	 */
+	struct attribute_group events_attr_group;
 	/**
 	 * @i915_attr: Memory block holding device attributes.
 	 */