Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Concurrent toggling of the global perfmon state bypasses cross-queue serialization for non-global perfmon jobs, breaking isolation and causing counter pollution. Pre-existing issues: - [High] Incorrect error check of pm_runtime_get_if_active() misses negative error codes, leading to a usage counter underflow when runtime PM is disabled. -- commit b3331fc0fee9a96bbf0068661ed07711e8ff9155 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, but the scheduler can run jobs from different queues concurrently on the same V3D core. This patch enforces cross-queue serialization when a perfmon-carrying submission is detected. > 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; > > v3d_perfmon_hw_start(v3d, perfmon); > v3d->perfmon_state.active = perfmon; > > v3d_pm_runtime_put(v3d); [Severity: High] This isn't a bug introduced by this patch, but does this correctly handle negative error codes? When runtime PM is disabled, pm_runtime_get_if_active() returns -EINVAL. Because the ! operator is used, !(-EINVAL) evaluates to false, causing the function to proceed instead of returning early. It then calls hardware access functions and v3d_pm_runtime_put(). Since the usage counter was not incremented, the put call would decrement it below zero, underflowing the counter. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 27cc98770b37e..d32defa7da73a 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > @@ -320,6 +322,73 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, > u32 perfmon_id) > return 0; > } > > +/* > + * Prepare fences to enforce job serialization when a perfmon is active. A > job > + * that carries a non-global perfmon must wait for every job currently > in-flight > + * across all HW queues to finish, otherwise concurrent unrelated work on the > + * same core would pollute the performance counters. Symmetrically, while > such a > + * job is still in-flight, all subsequently submitted jobs must wait for it. > + * > + * We don't serialize the jobs when using a global perfmon as it's expected > to > + * track concurrent activity from all jobs. > + */ > +static int > +v3d_serialize_for_perfmon(struct v3d_job *job) > +{ > + struct v3d_dev *v3d = job->v3d; > + struct dma_fence *merged; > + 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: High] Can a concurrent toggle of the global perfmon state bypass cross-queue serialization for non-global perfmon jobs? If Thread 1 submits a job with a local perfmon (succeeding in v3d_attach_perfmon_to_jobs() because the global perfmon is off), and Thread 2 turns on the global perfmon, Thread 1 could then reach v3d_serialize_for_perfmon(), read is_global_perfmon == true, and skip adding cross-queue serialization fences. If Thread 3 then turns off the global perfmon before Thread 1's job executes, v3d_perfmon_start() will see the global perfmon is off and activate the local perfmon. Because the job wasn't serialized, it runs concurrently with other jobs, violating isolation and polluting hardware performance counters. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
