| Message ID | 20250428205619.227835-1-robdclark@gmail.com |
|---|---|
| Headers | show |
| Series | drm/msm: sparse / "VM_BIND" support | expand |
On Mon, Apr 28, 2025 at 01:54:10PM -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > In situations where mapping/unmapping squence can be controlled by > userspace, attempting to map over a region that has not yet been > unmapped is an error. But not something that should spam dmesg. I think if you want to do something like that using the iommu API the expectation is for the caller to do a iova_to_phys to check what is mapped first? That seems kind of lame.. Maybe page table driver should not not be doing these WARNs at all. If we want to check for that the core iommu code should have the WARN_ON? eg iommufd already has a WARN_ON around iommu_unmap failures so having one in the ARM page table is a double WARN. Don't really like using a quirk to change the API contract. Jason
On Tue, Apr 29, 2025 at 5:38 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 28/04/2025 9:54 pm, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > In situations where mapping/unmapping squence can be controlled by > > userspace, attempting to map over a region that has not yet been > > unmapped is an error. But not something that should spam dmesg. > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/iommu/io-pgtable-arm.c | 18 ++++++++++++------ > > include/linux/io-pgtable.h | 8 ++++++++ > > 2 files changed, 20 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > > index f27965caf6a1..99523505dac5 100644 > > --- a/drivers/iommu/io-pgtable-arm.c > > +++ b/drivers/iommu/io-pgtable-arm.c > > @@ -475,7 +475,7 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, > > cptep = iopte_deref(pte, data); > > } else if (pte) { > > /* We require an unmap first */ > > - WARN_ON(!selftest_running); > > + WARN_ON(!selftest_running && !(cfg->quirks & IO_PGTABLE_QUIRK_NO_WARN_ON)); > > If we are going to have this as a general mechanism then the selftests > should use it as well. Makes sense, I can remove the selftest_running hack in the next version. BR, -R > Thanks, > Robin. > > > return -EEXIST; > > } > > > > @@ -649,8 +649,10 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, > > unmap_idx_start = ARM_LPAE_LVL_IDX(iova, lvl, data); > > ptep += unmap_idx_start; > > pte = READ_ONCE(*ptep); > > - if (WARN_ON(!pte)) > > - return 0; > > + if (!pte) { > > + WARN_ON(!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NO_WARN_ON)); > > + return -ENOENT; > > + } > > > > /* If the size matches this level, we're in the right place */ > > if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) { > > @@ -660,8 +662,10 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, > > /* Find and handle non-leaf entries */ > > for (i = 0; i < num_entries; i++) { > > pte = READ_ONCE(ptep[i]); > > - if (WARN_ON(!pte)) > > + if (!pte) { > > + WARN_ON(!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NO_WARN_ON)); > > break; > > + } > > > > if (!iopte_leaf(pte, lvl, iop->fmt)) { > > __arm_lpae_clear_pte(&ptep[i], &iop->cfg, 1); > > @@ -976,7 +980,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie) > > if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | > > IO_PGTABLE_QUIRK_ARM_TTBR1 | > > IO_PGTABLE_QUIRK_ARM_OUTER_WBWA | > > - IO_PGTABLE_QUIRK_ARM_HD)) > > + IO_PGTABLE_QUIRK_ARM_HD | > > + IO_PGTABLE_QUIRK_NO_WARN_ON)) > > return NULL; > > > > data = arm_lpae_alloc_pgtable(cfg); > > @@ -1079,7 +1084,8 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie) > > struct arm_lpae_io_pgtable *data; > > typeof(&cfg->arm_lpae_s2_cfg.vtcr) vtcr = &cfg->arm_lpae_s2_cfg.vtcr; > > > > - if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_S2FWB)) > > + if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_S2FWB | > > + IO_PGTABLE_QUIRK_NO_WARN_ON)) > > return NULL; > > > > data = arm_lpae_alloc_pgtable(cfg); > > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h > > index bba2a51c87d2..639b8f4fb87d 100644 > > --- a/include/linux/io-pgtable.h > > +++ b/include/linux/io-pgtable.h > > @@ -88,6 +88,13 @@ struct io_pgtable_cfg { > > * > > * IO_PGTABLE_QUIRK_ARM_HD: Enables dirty tracking in stage 1 pagetable. > > * IO_PGTABLE_QUIRK_ARM_S2FWB: Use the FWB format for the MemAttrs bits > > + * > > + * IO_PGTABLE_QUIRK_NO_WARN_ON: Do not WARN_ON() on conflicting > > + * mappings, but silently return -EEXISTS. Normally an attempt > > + * to map over an existing mapping would indicate some sort of > > + * kernel bug, which would justify the WARN_ON(). But for GPU > > + * drivers, this could be under control of userspace. Which > > + * deserves an error return, but not to spam dmesg. > > */ > > #define IO_PGTABLE_QUIRK_ARM_NS BIT(0) > > #define IO_PGTABLE_QUIRK_NO_PERMS BIT(1) > > @@ -97,6 +104,7 @@ struct io_pgtable_cfg { > > #define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA BIT(6) > > #define IO_PGTABLE_QUIRK_ARM_HD BIT(7) > > #define IO_PGTABLE_QUIRK_ARM_S2FWB BIT(8) > > + #define IO_PGTABLE_QUIRK_NO_WARN_ON BIT(9) > > unsigned long quirks; > > unsigned long pgsize_bitmap; > > unsigned int ias;
From: Rob Clark <robdclark@chromium.org> Conversion to DRM GPU VA Manager[1], and adding support for Vulkan Sparse Memory[2] in the form of: 1. A new VM_BIND submitqueue type for executing VM MSM_SUBMIT_BO_OP_MAP/ MAP_NULL/UNMAP commands 2. A new VM_BIND ioctl to allow submitting batches of one or more MAP/MAP_NULL/UNMAP commands to a VM_BIND submitqueue I did not implement support for synchronous VM_BIND commands. Since userspace could just immediately wait for the `SUBMIT` to complete, I don't think we need this extra complexity in the kernel. Synchronous/immediate VM_BIND operations could be implemented with a 2nd VM_BIND submitqueue. The corresponding mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/32533 Changes in v3: - Switched to seperate VM_BIND ioctl. This makes the UABI a bit cleaner, but OTOH the userspace code was cleaner when the end result of either type of VkQueue lead to the same ioctl. So I'm a bit on the fence. - Switched to doing the gpuvm bookkeeping synchronously, and only deferring the pgtable updates. This avoids needing to hold any resv locks in the fence signaling path, resolving the last shrinker related lockdep complaints. OTOH it means userspace can trigger invalid pgtable updates with multiple VM_BIND queues. In this case, we ensure that unmaps happen completely (to prevent userspace from using this to access free'd pages), mark the context as unusable, and move on with life. - Link to v2: https://lore.kernel.org/all/20250319145425.51935-1-robdclark@gmail.com/ Changes in v2: - Dropped Bibek Kumar Patro's arm-smmu patches[3], which have since been merged. - Pre-allocate all the things, and drop HACK patch which disabled shrinker. This includes ensuring that vm_bo objects are allocated up front, pre- allocating VMA objects, and pre-allocating pages used for pgtable updates. The latter utilizes io_pgtable_cfg callbacks for pgtable alloc/free, that were initially added for panthor. - Add back support for BO dumping for devcoredump. - Link to v1 (RFC): https://lore.kernel.org/dri-devel/20241207161651.410556-1-robdclark@gmail.com/T/#t [1] https://www.kernel.org/doc/html/next/gpu/drm-mm.html#drm-gpuvm [2] https://docs.vulkan.org/spec/latest/chapters/sparsemem.html [3] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=909700 Rob Clark (33): drm/gpuvm: Don't require obj lock in destructor path drm/gpuvm: Allow VAs to hold soft reference to BOs iommu/io-pgtable-arm: Add quirk to quiet WARN_ON() drm/msm: Rename msm_file_private -> msm_context drm/msm: Improve msm_context comments drm/msm: Rename msm_gem_address_space -> msm_gem_vm drm/msm: Remove vram carveout support drm/msm: Collapse vma allocation and initialization drm/msm: Collapse vma close and delete drm/msm: Don't close VMAs on purge drm/msm: drm_gpuvm conversion drm/msm: Convert vm locking drm/msm: Use drm_gpuvm types more drm/msm: Split out helper to get iommu prot flags drm/msm: Add mmu support for non-zero offset drm/msm: Add PRR support drm/msm: Rename msm_gem_vma_purge() -> _unmap() drm/msm: Lazily create context VM drm/msm: Add opt-in for VM_BIND drm/msm: Mark VM as unusable on GPU hangs drm/msm: Add _NO_SHARE flag drm/msm: Crashdump prep for sparse mappings drm/msm: rd dumping prep for sparse mappings drm/msm: Crashdec support for sparse drm/msm: rd dumping support for sparse drm/msm: Extract out syncobj helpers drm/msm: Use DMA_RESV_USAGE_BOOKKEEP/KERNEL drm/msm: Add VM_BIND submitqueue drm/msm: Support IO_PGTABLE_QUIRK_NO_WARN_ON drm/msm: Support pgtable preallocation drm/msm: Split out map/unmap ops drm/msm: Add VM_BIND ioctl drm/msm: Bump UAPI version drivers/gpu/drm/drm_gpuvm.c | 15 +- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 25 +- drivers/gpu/drm/msm/adreno/a2xx_gpummu.c | 5 +- drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 17 +- drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 17 +- drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 22 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 10 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 32 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 49 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 +- drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 10 +- drivers/gpu/drm/msm/adreno/adreno_device.c | 4 - drivers/gpu/drm/msm/adreno/adreno_gpu.c | 88 +- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 23 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 14 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 14 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 6 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 12 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 19 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 12 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 14 +- drivers/gpu/drm/msm/msm_drv.c | 183 +-- drivers/gpu/drm/msm/msm_drv.h | 35 +- drivers/gpu/drm/msm/msm_fb.c | 18 +- drivers/gpu/drm/msm/msm_fbdev.c | 2 +- drivers/gpu/drm/msm/msm_gem.c | 489 +++---- drivers/gpu/drm/msm/msm_gem.h | 217 ++- drivers/gpu/drm/msm/msm_gem_prime.c | 15 + drivers/gpu/drm/msm/msm_gem_shrinker.c | 4 +- drivers/gpu/drm/msm/msm_gem_submit.c | 295 ++-- drivers/gpu/drm/msm/msm_gem_vma.c | 1265 +++++++++++++++-- drivers/gpu/drm/msm/msm_gpu.c | 171 ++- drivers/gpu/drm/msm/msm_gpu.h | 132 +- drivers/gpu/drm/msm/msm_iommu.c | 298 +++- drivers/gpu/drm/msm/msm_kms.c | 18 +- drivers/gpu/drm/msm/msm_kms.h | 2 +- drivers/gpu/drm/msm/msm_mmu.h | 38 +- drivers/gpu/drm/msm/msm_rd.c | 62 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 4 +- drivers/gpu/drm/msm/msm_submitqueue.c | 86 +- drivers/gpu/drm/msm/msm_syncobj.c | 172 +++ drivers/gpu/drm/msm/msm_syncobj.h | 37 + drivers/iommu/io-pgtable-arm.c | 18 +- include/drm/drm_gpuvm.h | 12 +- include/linux/io-pgtable.h | 8 + include/uapi/drm/msm_drm.h | 149 +- 57 files changed, 3012 insertions(+), 1216 deletions(-) create mode 100644 drivers/gpu/drm/msm/msm_syncobj.c create mode 100644 drivers/gpu/drm/msm/msm_syncobj.h