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: Handle remap case for repeated mappings Date: Sat, 14 Mar 2026 06:48:46 +1000 Message-ID: In-Reply-To: <20260313150956.1618635-10-adrian.larumbe@collabora.com> References: <20260313150956.1618635-1-adrian.larumbe@collabora.com> <20260313150956.1618635-10-adrian.larumbe@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The refactoring to extract `panthor_vm_map_range()` is clean. The remap paths now correctly pass through the repeat flag and range. **Potential issue in prev remap path:** ```c + ret = panthor_vm_map_range(vm, op->remap.prev->flags & DRM_GPUVA_REPEAT, + bo->base.sgt, op->remap.prev->va.addr, offset, + size, op->remap.prev->gem.repeat_range, + flags_to_prot(unmap_vma->flags)); ``` The `addr` parameter passed is `op->remap.prev->va.addr`, but the `size` is calculated as `op->remap.prev->va.addr + op->remap.prev->va.range - unmap_start`. This `size` represents the portion of the prev region that overlaps with the unmap range (the part that needs remapping). But the mapping starts at `op->remap.prev->va.addr` (the original prev start), not at `unmap_start`. Looking at this more carefully: in the remap flow, the prev region represents the portion *before* the unmap that needs to be re-mapped after the full unmap. The `unmap_start` may have been adjusted by `unmap_hugepage_align`. The `size` here is the extra area that was unmapped beyond what was requested, which needs to be remapped. The address and offset calculations look correct in context. However, for repeated mappings, `panthor_vm_map_range` is called with `repeat=true`, but the `offset` passed was calculated as `op->remap.prev->gem.offset + unmap_start - op->remap.prev->va.addr`. For a repeated mapping, this offset computation (which adds a VA-space delta to the GEM offset) may not be correct since repeat mappings don't advance the GEM offset linearly with VA. This needs careful verification that patch 6's GPUVM core already handles this (setting offset to the original offset for repeated regions in the remap prev/next operations). Looking back at patch 6, for repeated VAs, the gem.offset is kept the same (offset + 0), so by the time we reach this panthor code, `op->remap.prev->gem.offset` should already be the correct (unchanged) offset. This should be fine. --- Generated by Claude Code Patch Reviewer