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 <[email protected]> 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/[email protected]?part=2
