Re: [PATCH v3 1/2] drm/v3d: Refactor perfmon locking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Perfmon hardware counters are permanently lost if a capture (via ioctl or stop) races with runtime PM autosuspend.
- [Medium] A global perfmon is incorrectly stopped and silently disabled if userspace sets it to the same perfmon object that is already attached to a pending job.
--

commit 8ce781eec267bf896e263ac16cfc79142b7af5ee
Author: Maíra Canal <mcanal@igalia.com>
Subject: drm/v3d: Refactor perfmon locking

This commit refactors the performance monitor locking in the v3d driver to
serialize access to the device-wide active performance monitor. It
introduces a new device-wide spinlock and centralizes state transitions to
protect the hardware counters and software state from concurrent access.

> diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c b/drivers/gpu/drm/v3d/v3d_perfmon.c
> index 48ae748247be8..3ad0f022753c8 100644
> --- a/drivers/gpu/drm/v3d/v3d_perfmon.c
> +++ b/drivers/gpu/drm/v3d/v3d_perfmon.c

[ ... ]

> @@ -288,6 +300,21 @@ static void v3d_perfmon_capture_locked(struct v3d_dev *v3d,
>  	v3d_pm_runtime_put(v3d);
>  }
>  
> +void v3d_perfmon_stop_locked(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
> +			     bool capture)
> +{
> +	lockdep_assert_held(&v3d->perfmon_state.lock);
> +
> +	if (!perfmon || perfmon != v3d->perfmon_state.active)
> +		return;
> +

[Severity: Medium]
Could a global perfmon be incorrectly stopped and silently disabled if
userspace sets it to the same perfmon object already attached to a pending
job?

If userspace submits a job with perfmon X attached, and before the job starts,
calls v3d_perfmon_set_global_ioctl() to set X as the global perfmon, the ioctl
succeeds, setting v3d->global_perfmon = X and v3d->perfmon_state.active = X.

When the job completes, its IRQ handler calls v3d_perfmon_stop() with X.
Because the job's perfmon matches the active perfmon here,
v3d_perfmon_stop_locked() clears active to NULL and stops the hardware,
breaking the global perfmon while leaving v3d->global_perfmon set.

> +	v3d->perfmon_state.active = NULL;
> +
> +	/* If the device is suspended, the HW has already stopped counting. */
> +	if (!pm_runtime_get_if_active(v3d->drm.dev))
> +		return;
> +

[Severity: Medium]
Can the hardware counters be permanently lost if a capture (via ioctl or
stop) races with runtime PM autosuspend?

If v3d_perfmon_set_global_ioctl() (with the CLEAR flag) executes while the
device state is RPM_SUSPENDING, pm_runtime_get_if_active() returns 0.

Because v3d->perfmon_state.active is set to NULL just before the check,
this function returns early without reading the hardware registers.

When the driver's suspend callback subsequently calls v3d_perfmon_suspend(),
it sees active == NULL and also skips capturing the counters, resulting in
lost counter data.

> +	v3d_perfmon_hw_stop(v3d, perfmon, capture);
> +	v3d_pm_runtime_put(v3d);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704-v3d-perfmon-lifetime-v3-0-fd26dfdc2153@igalia.com?part=1