From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/v3d: Hold v3d_stats references in each job Date: Mon, 09 Mar 2026 09:20:15 +1000 Message-ID: In-Reply-To: <20260305-v3d-reset-locking-improv-v2-4-fd53c91f7726@igalia.com> References: <20260305-v3d-reset-locking-improv-v2-0-fd53c91f7726@igalia.com> <20260305-v3d-reset-locking-improv-v2-4-fd53c91f7726@igalia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the key patch that eliminates the need for `queue_lock` in the stat= s update path. Each job now holds its own refcounted references to both cli= ent and global stats: ```c job->client_stats =3D v3d_stats_get(v3d_priv->stats[queue]); job->global_stats =3D v3d_stats_get(v3d->queue[queue].stats); ``` The critical simplification in `v3d_job_update_stats` is correct =E2=80=94 = the old code needed `queue_lock` and the `if (job->file_priv)` NULL check b= ecause stats were embedded in `v3d_file_priv` which could be freed. With re= fcounting, the stats outlive the fd: ```c void v3d_job_update_stats(struct v3d_job *job) { u64 now =3D local_clock(); preempt_disable(); v3d_stats_update(job->client_stats, now); v3d_stats_update(job->global_stats, now); preempt_enable(); } ``` The refactoring of `v3d_job_start_stats` into `v3d_stats_start` + `v3d_job_= start_stats` is clean and makes both functions independent of queue type, w= hich is why the `enum v3d_queue` parameter can be dropped. All callers are = updated consistently. The `v3d_job_free` cleanup correctly puts both stats references before free= ing the job. The placement after `v3d_perfmon_put` and before `kfree` is ap= propriate. --- Generated by Claude Code Patch Reviewer