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/panthor: Lazily allocate pages on mmap() Date: Tue, 10 Mar 2026 12:15:41 +1000 Message-ID: In-Reply-To: <20260309151119.290217-7-boris.brezillon@collabora.com> References: <20260309151119.290217-1-boris.brezillon@collabora.com> <20260309151119.290217-7-boris.brezillon@collabora.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 patch defers page allocation from `mmap()` time to fault time. The fau= lt handler is significantly reworked with a nonblocking fast path and a blo= cking slow path. 1. **`nonblocking_page_setup`**: Uses `dma_resv_trylock()` =E2=80=94 return= s `VM_FAULT_RETRY` if pages aren't allocated or lock is contended. This is = correct for the fast path. 2. **`blocking_page_setup`**: Uses `dma_resv_lock_interruptible()`. If it f= ails: - With mmap lock held: returns `VM_FAULT_NOPAGE` (will retry) - Without mmap lock: returns `VM_FAULT_RETRY` Minor concern: when `dma_resv_lock_interruptible()` returns `-EINTR`, re= turning `VM_FAULT_NOPAGE` with `mmap_lock_held=3Dtrue` seems correct (the M= M will retry), but if `err =3D=3D -EDEADLK` (shouldn't happen with NULL ctx= ), the behavior would be the same. 3. **mmap lock release pattern**: The pattern of calling `mmap_read_unlock(= )`, doing work, then returning `VM_FAULT_RETRY` is standard for VM fault ha= ndlers that need to sleep. 4. **`drm_gem_object_get/put` around blocking path**: Correctly prevents th= e BO from being freed while the mmap lock is released. 5. **`vm_open` simplification**: The old `panthor_gem_vm_open` asserted pag= es were present and took the resv lock. With lazy allocation, it's simplifi= ed to just `drm_gem_vm_open`. This makes sense =E2=80=94 pages may not be a= llocated yet. No issues. --- Generated by Claude Code Patch Reviewer