diff mbox series

[5/8] drm/i915/pmu: Stop setting event_init to NULL

Message ID 20241011225430.1219345-6-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Fix i915 pmu on bind/unbind | expand

Commit Message

Lucas De Marchi Oct. 11, 2024, 10:54 p.m. UTC 
Setting event_init to NULL is mostly done to detect when the driver is
partially working: i915 probed, but pmu is not registered. However,
checking for event_init is odd as it was supposed to always be set and
kernel/events/ would just crash if it found it set to NULL.

Since there's already a "closed" boolean, use that instead and extend
it's meaning to unregistered/unregistering.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Comments

Matt Roper Oct. 30, 2024, 12:09 a.m. UTC | #1 
On Fri, Oct 11, 2024 at 03:54:27PM -0700, Lucas De Marchi wrote:
> Setting event_init to NULL is mostly done to detect when the driver is
> partially working: i915 probed, but pmu is not registered. However,
> checking for event_init is odd as it was supposed to always be set and
> kernel/events/ would just crash if it found it set to NULL.
> 
> Since there's already a "closed" boolean, use that instead and extend
> it's meaning to unregistered/unregistering.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 2e435f51867db..409e10d8190a8 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -303,7 +303,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>  {
>  	struct i915_pmu *pmu = &gt->i915->pmu;
>  
> -	if (!pmu->base.event_init)
> +	if (pmu->closed)
>  		return;
>  
>  	spin_lock_irq(&pmu->lock);
> @@ -325,7 +325,7 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
>  {
>  	struct i915_pmu *pmu = &gt->i915->pmu;
>  
> -	if (!pmu->base.event_init)
> +	if (pmu->closed)
>  		return;
>  
>  	spin_lock_irq(&pmu->lock);
> @@ -1170,8 +1170,6 @@ static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node *node)
>  {
>  	struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
>  
> -	GEM_BUG_ON(!pmu->base.event_init);
> -
>  	/* Select the first online CPU as a designated reader. */
>  	if (cpumask_empty(&i915_pmu_cpumask))
>  		cpumask_set_cpu(cpu, &i915_pmu_cpumask);
> @@ -1184,8 +1182,6 @@ static int i915_pmu_cpu_offline(unsigned int cpu, struct hlist_node *node)
>  	struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
>  	unsigned int target = i915_pmu_target_cpu;
>  
> -	GEM_BUG_ON(!pmu->base.event_init);
> -
>  	/*
>  	 * Unregistering an instance generates a CPU offline event which we must
>  	 * ignore to avoid incorrectly modifying the shared i915_pmu_cpumask.
> @@ -1258,9 +1254,10 @@ void i915_pmu_register(struct drm_i915_private *i915)
>  		&i915_pmu_cpumask_attr_group,
>  		NULL
>  	};
> -
>  	int ret = -ENOMEM;
>  
> +	pmu->closed = true;
> +
>  	if (GRAPHICS_VER(i915) <= 2) {
>  		drm_info(&i915->drm, "PMU not supported for this GPU.");
>  		return;
> @@ -1319,6 +1316,8 @@ void i915_pmu_register(struct drm_i915_private *i915)
>  	if (drmm_add_action(&i915->drm, free_pmu, pmu))
>  		goto err_unreg;
>  
> +	pmu->closed = false;
> +
>  	return;
>  
>  err_unreg:
> @@ -1326,7 +1325,6 @@ void i915_pmu_register(struct drm_i915_private *i915)
>  err_groups:
>  	kfree(pmu->base.attr_groups);
>  err_attr:
> -	pmu->base.event_init = NULL;
>  	free_event_attributes(pmu);
>  err_name:
>  	if (IS_DGFX(i915))
> @@ -1339,9 +1337,6 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
>  {
>  	struct i915_pmu *pmu = &i915->pmu;
>  
> -	if (!pmu->base.event_init)
> -		return;
> -
>  	/*
>  	 * "Disconnect" the PMU callbacks - since all are atomic synchronize_rcu
>  	 * ensures all currently executing ones will have exited before we
> @@ -1354,6 +1349,4 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
>  
>  	i915_pmu_unregister_cpuhp_state(pmu);
>  	perf_pmu_unregister(&pmu->base);
> -
> -	pmu->base.event_init = NULL;
>  }
> -- 
> 2.47.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 2e435f51867db..409e10d8190a8 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -303,7 +303,7 @@  void i915_pmu_gt_parked(struct intel_gt *gt)
 {
 	struct i915_pmu *pmu = &gt->i915->pmu;
 
-	if (!pmu->base.event_init)
+	if (pmu->closed)
 		return;
 
 	spin_lock_irq(&pmu->lock);
@@ -325,7 +325,7 @@  void i915_pmu_gt_unparked(struct intel_gt *gt)
 {
 	struct i915_pmu *pmu = &gt->i915->pmu;
 
-	if (!pmu->base.event_init)
+	if (pmu->closed)
 		return;
 
 	spin_lock_irq(&pmu->lock);
@@ -1170,8 +1170,6 @@  static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node *node)
 {
 	struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
 
-	GEM_BUG_ON(!pmu->base.event_init);
-
 	/* Select the first online CPU as a designated reader. */
 	if (cpumask_empty(&i915_pmu_cpumask))
 		cpumask_set_cpu(cpu, &i915_pmu_cpumask);
@@ -1184,8 +1182,6 @@  static int i915_pmu_cpu_offline(unsigned int cpu, struct hlist_node *node)
 	struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
 	unsigned int target = i915_pmu_target_cpu;
 
-	GEM_BUG_ON(!pmu->base.event_init);
-
 	/*
 	 * Unregistering an instance generates a CPU offline event which we must
 	 * ignore to avoid incorrectly modifying the shared i915_pmu_cpumask.
@@ -1258,9 +1254,10 @@  void i915_pmu_register(struct drm_i915_private *i915)
 		&i915_pmu_cpumask_attr_group,
 		NULL
 	};
-
 	int ret = -ENOMEM;
 
+	pmu->closed = true;
+
 	if (GRAPHICS_VER(i915) <= 2) {
 		drm_info(&i915->drm, "PMU not supported for this GPU.");
 		return;
@@ -1319,6 +1316,8 @@  void i915_pmu_register(struct drm_i915_private *i915)
 	if (drmm_add_action(&i915->drm, free_pmu, pmu))
 		goto err_unreg;
 
+	pmu->closed = false;
+
 	return;
 
 err_unreg:
@@ -1326,7 +1325,6 @@  void i915_pmu_register(struct drm_i915_private *i915)
 err_groups:
 	kfree(pmu->base.attr_groups);
 err_attr:
-	pmu->base.event_init = NULL;
 	free_event_attributes(pmu);
 err_name:
 	if (IS_DGFX(i915))
@@ -1339,9 +1337,6 @@  void i915_pmu_unregister(struct drm_i915_private *i915)
 {
 	struct i915_pmu *pmu = &i915->pmu;
 
-	if (!pmu->base.event_init)
-		return;
-
 	/*
 	 * "Disconnect" the PMU callbacks - since all are atomic synchronize_rcu
 	 * ensures all currently executing ones will have exited before we
@@ -1354,6 +1349,4 @@  void i915_pmu_unregister(struct drm_i915_private *i915)
 
 	i915_pmu_unregister_cpuhp_state(pmu);
 	perf_pmu_unregister(&pmu->base);
-
-	pmu->base.event_init = NULL;
 }