From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH v2] drm/virtio: move cursor resv lock acquisition to prepare_fb Date: Sat, 16 May 2026 14:23:37 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Sound** The prepare =E2=86=92 update =E2=86=92 cleanup lifecycle is handled correct= ly: 1. **prepare_fb** allocates and locks objs, stashes on `vgplane_st->objs` 2. **atomic_update** consumes objs (passes to queue, sets pointer to NULL) 3. **cleanup_fb** releases unconsumed objs (rollback case) The error paths in prepare_fb are correct: ```c + objs =3D virtio_gpu_array_alloc(1); + if (!objs) { + ret =3D -ENOMEM; + goto err_fence; + } + virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]); + ret =3D virtio_gpu_array_lock_resv(objs); + if (ret) { + virtio_gpu_array_put_free(objs); + goto err_fence; + } ``` On `lock_resv` failure, `put_free` correctly drops the GEM reference taken = by `array_add_obj` and frees the array before falling through to `err_fence= ` which releases the already-allocated fence. On `array_alloc` failure, we = jump directly to `err_fence` since no GEM reference has been taken yet. Bot= h are correct. **Ownership transfer in update is clean:** ```c + virtio_gpu_cmd_transfer_to_host_2d + (vgdev, 0, + plane->state->crtc_w, + plane->state->crtc_h, + 0, 0, vgplane_st->objs, vgplane_st->fence); virtio_gpu_notify(vgdev); dma_fence_wait(&vgplane_st->fence->f, true); + vgplane_st->objs =3D NULL; ``` The queue path (`virtio_gpu_queue_ctrl_sgs` at vq.c:410-411) adds the fence= and unlocks the reservation after submission. The delayed free work (vq.c:= 271) releases the objects after host completion. Setting `vgplane_st->objs = =3D NULL` prevents double-free in cleanup_fb. Correct. **Cleanup_fb rollback handling:** ```c + if (vgplane_st->objs) { + virtio_gpu_array_unlock_resv(vgplane_st->objs); + virtio_gpu_array_put_free(vgplane_st->objs); + vgplane_st->objs =3D NULL; + } ``` This correctly handles the rollback case where prepare_fb succeeded but the= commit was aborted before atomic_update ran. The NULL check guards against= the normal-path case where update already consumed and cleared objs. **State duplication is safe:** `virtio_gpu_plane_duplicate_state` uses `kza= lloc_obj` which zeroes the struct, so the new state's `objs` starts as NULL= . No inherited stale pointer. **Minor observations (non-blocking):** 1. **Unnecessary work on cursor moves:** The `prepare_fb` condition is `pla= ne->type =3D=3D DRM_PLANE_TYPE_CURSOR && bo->dumb`, but `cursor_plane_updat= e` only consumes objs when `plane->state->fb !=3D old_state->fb`. For pure = cursor moves (same framebuffer, different position), prepare_fb will alloca= te and lock objs that cleanup_fb immediately releases. This mirrors the exi= sting fence allocation pattern =E2=80=94 `fence` is also allocated uncondit= ionally for dumb cursor BOs and only used on fb change =E2=80=94 so it's co= nsistent, but both could be optimized in a followup by gating on `new_state= ->fb !=3D plane->state->fb` in prepare_fb. 2. **Same unchecked `lock_resv` pattern exists in `virtio_gpu_resource_flus= h`** (virtgpu_plane.c:218) for primary planes. That's a separate bug worth = a followup patch. 3. **Nit =E2=80=94 comment style:** The multi-line `/* objs and fence were = prepared... */` comment in cursor_plane_update is fine for documenting the = non-obvious ownership semantics. Could be shortened to one line but not imp= ortant. --- Generated by Claude Code Patch Reviewer