On Fri, Oct 11, 2024 at 03:54:28PM -0700, Lucas De Marchi wrote:
> Since i915 calls perf_pmu_register/perf_pmu_unregister, let's call the
> variable "registered" so we can flip the logic and rely on it being
> false by default. Looking at other drivers, it's also more common.
> Examples: arch/x86/events/intel/uncore.c and
> drivers/powercap/intel_rapl_common.c.
>
> Signed-off-by: Lucas De Marchi <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 25 +++++++++++++------------
> drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> 2 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 409e10d8190a8..e28c29bae2d9a 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 = >->i915->pmu;
>
> - if (pmu->closed)
> + if (!pmu->registered)
> return;
>
> spin_lock_irq(&pmu->lock);
> @@ -325,7 +325,7 @@ void i915_pmu_gt_unparked(struct intel_gt *gt)
> {
> struct i915_pmu *pmu = >->i915->pmu;
>
> - if (pmu->closed)
> + if (!pmu->registered)
> return;
>
> spin_lock_irq(&pmu->lock);
> @@ -593,7 +593,7 @@ static int i915_pmu_event_init(struct perf_event *event)
> struct drm_i915_private *i915 = pmu_to_i915(pmu);
> int ret;
>
> - if (pmu->closed)
> + if (!pmu->registered)
> return -ENODEV;
>
> if (event->attr.type != event->pmu->type)
> @@ -686,7 +686,7 @@ static void i915_pmu_event_read(struct perf_event *event)
> struct hw_perf_event *hwc = &event->hw;
> u64 prev, new;
>
> - if (pmu->closed) {
> + if (!pmu->registered) {
> event->hw.state = PERF_HES_STOPPED;
> return;
> }
> @@ -812,7 +812,7 @@ static void i915_pmu_event_start(struct perf_event
> *event, int flags)
> {
> struct i915_pmu *pmu = event_to_pmu(event);
>
> - if (pmu->closed)
> + if (!pmu->registered)
> return;
>
> i915_pmu_enable(event);
> @@ -823,7 +823,7 @@ static void i915_pmu_event_stop(struct perf_event *event,
> int flags)
> {
> struct i915_pmu *pmu = event_to_pmu(event);
>
> - if (pmu->closed)
> + if (!pmu->registered)
> goto out;
>
> if (flags & PERF_EF_UPDATE)
> @@ -839,7 +839,7 @@ static int i915_pmu_event_add(struct perf_event *event,
> int flags)
> {
> struct i915_pmu *pmu = event_to_pmu(event);
>
> - if (pmu->closed)
> + if (!pmu->registered)
> return -ENODEV;
>
> if (flags & PERF_EF_START)
> @@ -1186,7 +1186,7 @@ static int i915_pmu_cpu_offline(unsigned int cpu,
> struct hlist_node *node)
> * Unregistering an instance generates a CPU offline event which we must
> * ignore to avoid incorrectly modifying the shared i915_pmu_cpumask.
> */
> - if (pmu->closed)
> + if (!pmu->registered)
> return 0;
>
> if (cpumask_test_and_clear_cpu(cpu, &i915_pmu_cpumask)) {
> @@ -1256,8 +1256,6 @@ void i915_pmu_register(struct drm_i915_private *i915)
> };
> int ret = -ENOMEM;
>
> - pmu->closed = true;
> -
> if (GRAPHICS_VER(i915) <= 2) {
> drm_info(&i915->drm, "PMU not supported for this GPU.");
> return;
> @@ -1316,7 +1314,7 @@ void i915_pmu_register(struct drm_i915_private *i915)
> if (drmm_add_action(&i915->drm, free_pmu, pmu))
> goto err_unreg;
>
> - pmu->closed = false;
> + pmu->registered = true;
>
> return;
>
> @@ -1337,12 +1335,15 @@ void i915_pmu_unregister(struct drm_i915_private
> *i915)
> {
> struct i915_pmu *pmu = &i915->pmu;
>
> + if (!pmu->registered)
> + return;
> +
> /*
> * "Disconnect" the PMU callbacks - since all are atomic synchronize_rcu
> * ensures all currently executing ones will have exited before we
> * proceed with unregistration.
> */
> - pmu->closed = true;
> + pmu->registered = false;
> synchronize_rcu();
>
> hrtimer_cancel(&pmu->timer);
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 41af038c37388..3c1cf594954d9 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -68,9 +68,9 @@ struct i915_pmu {
> */
> struct pmu base;
> /**
> - * @closed: i915 is unregistering.
> + * @closed: PMU is registered and not in the unregistering process.
Missing a name change in the kerneldoc here.
Otherwise,
Reviewed-by: Matt Roper <[email protected]>
> */
> - bool closed;
> + bool registered;
> /**
> * @name: Name as registered with perf core.
> */
> --
> 2.47.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
