Re: [PATCH v3 2/2] drm/v3d: Serialize jobs across queues when a perfmon is attached

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition between local perfmon job submission and global perfmon toggling allows jobs to bypass cross-queue serialization.
- [Medium] Out-of-order execution across different DRM scheduler entities breaks the single-fence tracking mechanism, allowing perfmon counter pollution.

Pre-existing issues:
- [High] Runtime PM usage counter underflow due to incorrect error checking on pm_runtime_get_if_active().
--

commit bade95affe13f920ce39322b17d721c548957d90
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Serialize jobs across queues when a perfmon is attached

A non-global perfmon is meant to count events generated by a specific
submission. This commit adds cross-queue serialization by attaching fence
dependencies so a perfmon-carrying job waits for in-flight jobs, and
subsequent jobs wait for it to finish.

> diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c b/drivers/gpu/drm/v3d/v3d_perfmon.c
> index 3ad0f022753c8..07dab7fb30607 100644
> --- a/drivers/gpu/drm/v3d/v3d_perfmon.c
> +++ b/drivers/gpu/drm/v3d/v3d_perfmon.c
> @@ -275,6 +275,12 @@ void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
>  	if (!perfmon || v3d->global_perfmon)
>  		return;
>  
> +	/* Cross-queue serialization should have drained any previous perfmon
> +	 * job before this one runs.
> +	 */
> +	if (WARN_ON_ONCE(v3d->perfmon_state.active))
> +		return;
> +
>  	if (!pm_runtime_get_if_active(v3d->drm.dev))
>  		return;

[Severity: High]
This is a pre-existing issue, but does this correctly handle the case where
Runtime PM is disabled?

pm_runtime_get_if_active() returns -EINVAL if Runtime PM is disabled (for
example, during system suspend). In C, -EINVAL evaluates as truthy, so the
condition !(-22) evaluates to false. 

Because the early return is skipped, the function proceeds to interact with
hardware and eventually drops a usage counter via v3d_pm_runtime_put().
Could this cause an unbalanced PM usage counter underflow?

[ ... ]

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 27cc98770b37e..2bb955a9ed5e9 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -320,6 +320,62 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)

[ ... ]

> +static int
> +v3d_serialize_for_perfmon(struct v3d_job *job)
> +{
> +	struct v3d_dev *v3d = job->v3d;
> +	bool is_global_perfmon;
> +	int ret;
> +
> +	lockdep_assert_held(&v3d->sched_lock);
> +
> +	scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock)
> +		is_global_perfmon = !!v3d->global_perfmon;
> +
> +	if (is_global_perfmon)
> +		goto publish;

[Severity: Medium]
Could a race condition bypass cross-queue serialization here if the global
perfmon state changes while jobs are queued?

If userspace submits Job A with a local perfmon (which stays queued in the
scheduler without immediately setting perfmon_state.active), and then calls
the ioctl to set the global perfmon, a subsequent Job B without a perfmon 
would see is_global_perfmon as true and skip serialization.

If userspace then clears the global perfmon before execution, could Job A
and Job B end up running concurrently on the hardware and polluting the
performance counters?

> +
> +	if (job->perfmon) {
> +		for (enum v3d_queue q = 0; q < V3D_MAX_QUEUES; q++) {
> +			struct dma_fence *f = v3d->perfmon_state.last_hw_fence[q];

[ ... ]

> +publish:
> +	dma_fence_put(v3d->perfmon_state.last_hw_fence[job->queue]);
> +	v3d->perfmon_state.last_hw_fence[job->queue] = dma_fence_get(job->done_fence);

[Severity: Medium]
Does overwriting the tracked fence for the queue lose track of earlier jobs
that might still be pending due to external dependencies?

If userspace submits Job 1 (blocked on an external syncobj) via File A,
then submits Job 2 (no dependencies) via File B to the same queue, they
can execute out of order because they use different scheduler entities.

The last_hw_fence array would be overwritten by Job 2's fence. If a
subsequent perfmon Job 3 is submitted, it will only wait on Job 2.

When Job 1's dependency is finally satisfied, could Job 1 and Job 3 run
concurrently and bypass the intended serialization?

> +
> +	if (job->perfmon && !is_global_perfmon) {
> +		dma_fence_put(v3d->perfmon_state.fence);
> +		v3d->perfmon_state.fence = dma_fence_get(job->done_fence);
> +	}
> +
> +	return 0;
> +}

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