El vie, 08-05-2026 a las 10:41 -0300, Maíra Canal escribió:
>
(...)
> +void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon
> *perfmon)
> +{
> + guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
> +
> + perfmon = v3d->global_perfmon ?: perfmon;
> +
What's the rationale for having this function do this? The caller
explicitly passes a perfmon to be started, but this here can ignore
that and instead act on the global perform under the hood.
I think it would be better if this decision is made expicitly before
calling the function and have the function always act on the perfmon
requested instead.
> + if (!perfmon)
> + return;
> +
> + if (perfmon == v3d->perfmon_state.active)
> + return;
> +
> + if (!pm_runtime_get_if_active(v3d->drm.dev))
> + return;
> +
> + v3d_perfmon_hw_start(v3d, perfmon);
> + v3d->perfmon_state.active = perfmon;
> +
> + v3d_pm_runtime_put(v3d);
> +}
> +
>
(...)
>
> struct v3d_perfmon *v3d_perfmon_find(struct v3d_file_priv *v3d_priv,
> int id)
> @@ -316,11 +354,15 @@ static void v3d_perfmon_delete(struct
> v3d_file_priv *v3d_priv,
> struct v3d_dev *v3d = v3d_priv->v3d;
>
> /* If the active perfmon is being destroyed, stop it first
> */
> - if (perfmon == v3d->active_perfmon)
> - v3d_perfmon_stop(v3d, perfmon, false);
> + scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
> + /* If the global perfmon is being destroyed, set it
> to NULL */
> + if (v3d->global_perfmon == perfmon) {
> + v3d->global_perfmon = NULL;
> + v3d_perfmon_put(perfmon);
> + }
>
> - /* If the global perfmon is being destroyed, set it to NULL
> */
> - cmpxchg(&v3d->global_perfmon, perfmon, NULL);
> + v3d_perfmon_stop_locked(v3d, perfmon, false);
I think it would be better to stop the perfmon first and check if it is
global second, even if we are certain the put above is never going to
delete the last reference in that scenario.
Also, maybe we would want to document here with a comment that we
increase the refcnt when we set a global perfmon, which is why in that
case we put it twice when it is destroyed.
Iago
