Claude review: Re: [PATCH v2] drm/virtio: move cursor resv lock acquisition to prepare_fb

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Correctness: Sound**

The prepare → update → cleanup lifecycle is handled correctly:

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 = virtio_gpu_array_alloc(1);
+		if (!objs) {
+			ret = -ENOMEM;
+			goto err_fence;
+		}
+		virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
+		ret = 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. Both 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 = 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 = 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 = 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 `kzalloc_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 `plane->type == DRM_PLANE_TYPE_CURSOR && bo->dumb`, but `cursor_plane_update` only consumes objs when `plane->state->fb != old_state->fb`. For pure cursor moves (same framebuffer, different position), prepare_fb will allocate and lock objs that cleanup_fb immediately releases. This mirrors the existing fence allocation pattern — `fence` is also allocated unconditionally for dumb cursor BOs and only used on fb change — so it's consistent, but both could be optimized in a followup by gating on `new_state->fb != plane->state->fb` in prepare_fb.

2. **Same unchecked `lock_resv` pattern exists in `virtio_gpu_resource_flush`** (virtgpu_plane.c:218) for primary planes. That's a separate bug worth a followup patch.

3. **Nit — 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 important.

---
Generated by Claude Code Patch Reviewer