Claude review: drm/xe/userptr: Defer Waiting for TLB invalidation to the second pass if possible

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

Patch Review

This is the payoff patch. It extends the two-pass model to also defer TLB invalidation waits. However, the implementation effectively creates a **three-pass** flow using a **two-pass** infrastructure, which is somewhat surprising.

**Three-pass flow via two-pass API:**

The commit message describes three flows:
```
  pass1 (fences pending)  -> invalidate_finish -> do_inval (sync TLB)
  pass1 (fences done)     -> do_inval -> invalidate_finish -> complete_tlb_inval (deferred TLB)
  pass1 (finish occupied) -> do_inval (sync TLB, inline)
```

In the second flow, `xe_vma_userptr_do_inval` returns `&userptr->finish`, which means it's requesting another finish callback. But this finish pointer goes through `xe_vma_userptr_invalidate_finish` which checks `tlb_inval_submitted` and dispatches to `xe_vma_userptr_complete_tlb_inval`. This works because `xe_vma_userptr_do_inval` is called from `xe_vma_userptr_invalidate_finish` which is called from `mn_itree_finish_pass`.

**Wait - this doesn't actually achieve the third pass.** Looking at `mn_itree_finish_pass`:

```c
static void mn_itree_finish_pass(struct llist_head *finish_passes)
{
	struct llist_node *first = llist_reverse_order(__llist_del_all(finish_passes));
	struct mmu_interval_notifier_finish *f, *next;

	llist_for_each_entry_safe(f, next, first, link)
		f->notifier->ops->invalidate_finish(f);
}
```

The `invalidate_finish` callback can return a new `finish` pointer from `xe_vma_userptr_do_inval`, but **nobody collects it**. The finish callback is `void` - it doesn't return a pointer. So the flow where `xe_vma_userptr_do_inval` returns `&userptr->finish` from within the `invalidate_finish` callback is problematic: the returned pointer goes to the caller `xe_vma_userptr_invalidate_finish`, which ignores it (the function is `void`).

**Wait, re-reading `xe_vma_userptr_invalidate_finish`:**

```c
static void xe_vma_userptr_invalidate_finish(struct mmu_interval_notifier_finish *finish)
{
	...
	down_write(&vm->svm.gpusvm.notifier_lock);
	if (uvma->userptr.tlb_inval_submitted)
		xe_vma_userptr_complete_tlb_inval(vm, uvma);
	else
		xe_vma_userptr_do_inval(vm, uvma, true);
	up_write(&vm->svm.gpusvm.notifier_lock);
	...
}
```

When `tlb_inval_submitted` is false (the common case for the "fences pending -> finish" path), it calls `xe_vma_userptr_do_inval(vm, uvma, true)`. That function now returns a `struct mmu_interval_notifier_finish *`, but the return value is **ignored** here. If `xe_vma_userptr_do_inval` returns a non-NULL finish (because it submitted TLB invalidation asynchronously and set `finish_inuse = true`, `tlb_inval_submitted = true`), then:

1. The TLB invalidation is submitted but never waited on
2. `finish_inuse` is set to true and never cleared
3. `tlb_inval_submitted` is set to true and never cleared

**This appears to be a bug.** The deferred TLB path from within `invalidate_finish` has no mechanism to schedule another finish callback because the two-pass infrastructure only supports... two passes. The three-pass flow described in the commit message doesn't actually work as described.

Let me re-examine more carefully... Actually, looking again at `xe_vma_userptr_do_inval`:

```c
	if (xe_vm_in_fault_mode(vm) && userptr->initial_bind) {
		if (!userptr->finish_inuse) {
			userptr->finish_inuse = true;
			userptr->tlb_inval_submitted = true;
			err = xe_vm_invalidate_vma_submit(vma, &userptr->inval_batch);
			XE_WARN_ON(err);
			return &userptr->finish;
		}
		err = xe_vm_invalidate_vma(vma);
		XE_WARN_ON(err);
	}

	if (is_deferred)
		userptr->finish_inuse = false;
```

When called from `xe_vma_userptr_invalidate_finish` (i.e., `is_deferred = true`), `finish_inuse` is still `true` (it was set in pass1). So the `!userptr->finish_inuse` check fails, and it falls through to the synchronous `xe_vm_invalidate_vma` path, then clears `finish_inuse`. So the three-pass flow **doesn't actually happen** for the "fences pending" case. The deferred TLB path only triggers when `signaled == true` in pass1 (all fences already done, so do_inval is called inline in pass1).

So the actual flows are:
1. **Fences pending**: pass1 defers -> finish calls do_inval -> `finish_inuse` is true -> falls through to sync TLB -> returns NULL (ignored, but correctly)
2. **Fences done, finish_inuse == false**: pass1 calls do_inval inline -> do_inval sees `!finish_inuse` -> submits TLB async -> returns finish ptr -> pass1 returns this to notifier core -> finish calls `complete_tlb_inval`
3. **Fences done, finish_inuse == true**: pass1 calls do_inval inline -> falls through to sync TLB -> returns NULL

This is actually correct! But flow #2 means the "two-pass" from the MMU notifier perspective is: pass1 (enable sw signaling + submit TLB) -> pass2 (wait for TLB). The dma_resv wait doesn't happen because fences are already signaled. My initial analysis was wrong about the bug.

However, this is still confusing. The `is_deferred` parameter controls whether `finish_inuse` is cleared. In flow #2, `is_deferred = false` (called from pass1), so `finish_inuse` is NOT cleared at the bottom of `do_inval`. It remains true, and is cleared later in `xe_vma_userptr_complete_tlb_inval`. This is correct but tricky.

**`xe_vma_userptr_force_invalidate` chain in patch 4:**

```c
	finish = xe_vma_userptr_invalidate_pass1(vm, uvma);
	if (finish)
		finish = xe_vma_userptr_do_inval(vm, uvma, true);
	if (finish)
		xe_vma_userptr_complete_tlb_inval(vm, uvma);
```

Note that `finish` here is the `static` variable from patch 2 (still a bug). But the logic is: if pass1 deferred, do the inval; if that deferred too, complete TLB. This correctly handles all three steps synchronously for the force-invalidate testing path.

**Embedding `xe_tlb_inval_batch` in `xe_userptr`:**

```c
	struct xe_tlb_inval_batch inval_batch;
```

This embeds 4 `xe_tlb_inval_fence` structs (~400-500 bytes) in every `xe_userptr`. This is a significant memory overhead per-VMA. Consider whether this should be dynamically allocated when needed, or whether the cost is justified by avoiding allocation in the notifier callback path.

**Summary of issues in patch 4:**

1. The `static` variable from patch 2 is still present (bug, should be fixed in patch 2)
2. The code flow is complex and the commit message's three-flow description is somewhat misleading about when each flow actually triggers
3. The return value of `xe_vma_userptr_do_inval` is ignored in `xe_vma_userptr_invalidate_finish` - this is actually correct behavior (it will always return NULL in that context because `finish_inuse` is already set), but deserves a comment
4. Per-VMA memory overhead from embedding `xe_tlb_inval_batch`

---

**Summary of actionable items:**

1. **Bug (Patch 2):** Remove `static` from `finish` variable in `xe_vma_userptr_force_invalidate`
2. **Minor (Patch 1):** Consider adding `WARN_ON` if both `invalidate` and `invalidate_start` are set in ops during registration
3. **Minor (Patch 3):** Inconsistent local variable naming (`_batch` vs `batch`)
4. **Minor (Patch 3):** `xe_tlb_inval_types.h` including `xe_device_types.h` is heavy for a types header
5. **Documentation (Patch 4):** Add a comment in `xe_vma_userptr_invalidate_finish` explaining that `xe_vma_userptr_do_inval` always returns NULL when called from the finish path (because `finish_inuse` is already set)
6. **Design consideration (Patch 4):** Memory cost of embedding `xe_tlb_inval_batch` in every `xe_userptr`

---
Generated by Claude Code Patch Reviewer