Hi Steve,
On Fri, 9 Feb 2024 16:51:46 +0000
Steven Price <steven.price@xxxxxxx> wrote:
> On 22/01/2024 16:30, Boris Brezillon wrote:
> > MMU and VM management is related and placed in the same source file.
> >
> > Page table updates are delegated to the io-pgtable-arm driver that's in
> > the iommu subsystem.
> >
> > The VM management logic is based on drm_gpuva_mgr, and is assuming the
> > VA space is mostly managed by the usermode driver, except for a reserved
> > portion of this VA-space that's used for kernel objects (like the heap
> > contexts/chunks).
> >
> > Both asynchronous and synchronous VM operations are supported, and
> > internal helpers are exposed to allow other logical blocks to map their
> > buffers in the GPU VA space.
> >
> > There's one VM_BIND queue per-VM (meaning the Vulkan driver can only
> > expose one sparse-binding queue), and this bind queue is managed with
> > a 1:1 drm_sched_entity:drm_gpu_scheduler, such that each VM gets its own
> > independent execution queue, avoiding VM operation serialization at the
> > device level (things are still serialized at the VM level).
> >
> > The rest is just implementation details that are hopefully well explained
> > in the documentation.
>
> panthor_vm_map_pages() looks a bit suspect (see below) and there's a
> kerneldoc header which needs updating. But generally it looks fine.
>
> >
> > v4:
> > - Add an helper to return the VM state
> > - Check drmm_mutex_init() return code
> > - Remove the VM from the AS reclaim list when panthor_vm_active() is
> > called
> > - Count the number of active VM users instead of considering there's
> > at most one user (several scheduling groups can point to the same
> > vM)
> > - Pre-allocate a VMA object for unmap operations (unmaps can trigger
> > a sm_step_remap() call)
> > - Check vm->root_page_table instead of vm->pgtbl_ops to detect if
> > the io-pgtable is trying to allocate the root page table
> > - Don't memset() the va_node in panthor_vm_alloc_va(), make it a
> > caller requirement
> > - Fix the kernel doc in a few places
> > - Drop the panthor_vm::base offset constraint and modify
> > panthor_vm_put() to explicitly check for a NULL value
> > - Fix unbalanced vm_bo refcount in panthor_gpuva_sm_step_remap()
> > - Drop stale comments about the shared_bos list
> > - Patch mmu_features::va_bits on 32-bit builds to reflect the
> > io_pgtable limitation and let the UMD know about it
> >
> > v3:
> > - Add acks for the MIT/GPL2 relicensing
> > - Propagate MMU faults to the scheduler
> > - Move pages pinning/unpinning out of the dma_signalling path
> > - Fix 32-bit support
> > - Rework the user/kernel VA range calculation
> > - Make the auto-VA range explicit (auto-VA range doesn't cover the full
> > kernel-VA range on the MCU VM)
> > - Let callers of panthor_vm_alloc_va() allocate the drm_mm_node
> > (embedded in panthor_kernel_bo now)
> > - Adjust things to match the latest drm_gpuvm changes (extobj tracking,
> > resv prep and more)
> > - Drop the per-AS lock and use slots_lock (fixes a race on vm->as.id)
> > - Set as.id to -1 when reusing an address space from the LRU list
> > - Drop misleading comment about page faults
> > - Remove check for irq being assigned in panthor_mmu_unplug()
> >
> > Co-developed-by: Steven Price <steven.price@xxxxxxx>
> > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Acked-by: Steven Price <steven.price@xxxxxxx> # MIT+GPL2 relicensing,Arm
> > Acked-by: Grant Likely <grant.likely@xxxxxxxxxx> # MIT+GPL2 relicensing,Linaro
> > Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> # MIT+GPL2 relicensing,Collabora
> > ---
> > drivers/gpu/drm/panthor/panthor_mmu.c | 2760 +++++++++++++++++++++++++
> > drivers/gpu/drm/panthor/panthor_mmu.h | 102 +
> > 2 files changed, 2862 insertions(+)
> > create mode 100644 drivers/gpu/drm/panthor/panthor_mmu.c
> > create mode 100644 drivers/gpu/drm/panthor/panthor_mmu.h
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > new file mode 100644
> > index 000000000000..d3ce29cd0662
>
> <snip>
>
> > +static int
> > +panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
> > + struct sg_table *sgt, u64 offset, u64 size)
> > +{
> > + struct panthor_device *ptdev = vm->ptdev;
> > + unsigned int count;
> > + struct scatterlist *sgl;
> > + struct io_pgtable_ops *ops = vm->pgtbl_ops;
> > + u64 start_iova = iova;
> > + int ret;
> > +
> > + if (!size)
> > + return 0;
> > +
> > + for_each_sgtable_dma_sg(sgt, sgl, count) {
> > + dma_addr_t paddr = sg_dma_address(sgl);
> > + size_t len = sg_dma_len(sgl);
> > +
> > + if (len <= offset) {
> > + offset -= len;
> > + continue;
> > + }
> > +
> > + paddr -= offset;
>
> Shouldn't that be "+="?
It should definitely be +=. Will fix that in v5.
>
> > + len -= offset;
> > +
> > + if (size >= 0) {
>
> size is unsigned... so this is always true!
I'll drop the condition.
>
> > + len = min_t(size_t, len, size);
> > + size -= len;
> > + }
> > +
> > + drm_dbg(&ptdev->base, "map: as=%d, iova=%llx, paddr=%pad, len=%zx",
> > + vm->as.id, iova, &paddr, len);
> > +
> > + while (len) {
> > + size_t pgcount, mapped = 0;
> > + size_t pgsize = get_pgsize(iova | paddr, len, &pgcount);
> > +
> > + ret = ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
> > + GFP_KERNEL, &mapped);
> > + iova += mapped;
> > + paddr += mapped;
> > + len -= mapped;
> > +
> > + if (drm_WARN_ON(&ptdev->base, !ret && !mapped))
> > + ret = -ENOMEM;
> > +
> > + if (ret) {
> > + /* If something failed, unmap what we've already mapped before
> > + * returning. The unmap call is not supposed to fail.
> > + */
> > + drm_WARN_ON(&ptdev->base,
> > + panthor_vm_unmap_pages(vm, start_iova,
> > + iova - start_iova));
> > + return ret;
> > + }
> > + }
> > +
> > + if (!size)
> > + break;
> > + }
> > +
> > + return panthor_vm_flush_range(vm, start_iova, iova - start_iova);
> > +}
> > +
>
> <snip>
>
> > +static void panthor_vm_destroy(struct panthor_vm *vm)
> > +{
> > + if (!vm)
> > + return;
> > +
> > + vm->destroyed = true;
> > +
> > + mutex_lock(&vm->heaps.lock);
> > + panthor_heap_pool_destroy(vm->heaps.pool);
> > + vm->heaps.pool = NULL;
> > + mutex_unlock(&vm->heaps.lock);
> > +
> > + drm_WARN_ON(&vm->ptdev->base,
> > + panthor_vm_unmap_range(vm, vm->base.mm_start, vm->base.mm_range));
> > + panthor_vm_put(vm);
> > +}
> > +
> > +/**
> > + * panthor_vm_destroy() - Destroy a VM.
> ^^^^^^^^^^^^^^^^^^ needs updating to the new function name.
>
And I'll fix the kernel doc.
Thanks,
Boris
