Thomas Gleixner
2021-Mar-03 13:20 UTC
[patch 0/7] drm, highmem: Cleanup io/kmap_atomic*() usage
None of the DRM usage sites of temporary mappings requires the side effects of io/kmap_atomic(), i.e. preemption and pagefault disable. Replace them with the io/kmap_local() variants, simplify the copy_to/from_user() error handling and remove the atomic variants. Thanks, tglx --- Documentation/driver-api/io-mapping.rst | 22 +++------- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 +-- drivers/gpu/drm/i915/i915_gem.c | 40 ++++++------------- drivers/gpu/drm/i915/selftests/i915_gem.c | 4 - drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 8 +-- drivers/gpu/drm/nouveau/nvkm/subdev/devinit/fbmem.h | 8 +-- drivers/gpu/drm/qxl/qxl_image.c | 18 ++++---- drivers/gpu/drm/qxl/qxl_ioctl.c | 27 ++++++------ drivers/gpu/drm/qxl/qxl_object.c | 12 ++--- drivers/gpu/drm/qxl/qxl_object.h | 4 - drivers/gpu/drm/qxl/qxl_release.c | 4 - drivers/gpu/drm/ttm/ttm_bo_util.c | 20 +++++---- drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 30 +++++--------- include/linux/highmem-internal.h | 14 ------ include/linux/io-mapping.h | 42 -------------------- 15 files changed, 93 insertions(+), 167 deletions(-)
From: Thomas Gleixner <tglx at linutronix.de> There is no reason to disable pagefaults and preemption as a side effect of kmap_atomic_prot(). Use kmap_local_page_prot() instead and document the reasoning for the mapping usage with the given pgprot. Remove the NULL pointer check for the map. These functions return a valid address for valid pages and the return was bogus anyway as it would have left preemption and pagefaults disabled. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: Christian Koenig <christian.koenig at amd.com> Cc: Huang Rui <ray.huang at amd.com> Cc: David Airlie <airlied at linux.ie> Cc: Daniel Vetter <daniel at ffwll.ch> Cc: dri-devel at lists.freedesktop.org --- drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -181,13 +181,15 @@ static int ttm_copy_io_ttm_page(struct t return -ENOMEM; src = (void *)((unsigned long)src + (page << PAGE_SHIFT)); - dst = kmap_atomic_prot(d, prot); - if (!dst) - return -ENOMEM; + /* + * Ensure that a highmem page is mapped with the correct + * pgprot. For non highmem the mapping is already there. + */ + dst = kmap_local_page_prot(d, prot); memcpy_fromio(dst, src, PAGE_SIZE); - kunmap_atomic(dst); + kunmap_local(dst); return 0; } @@ -203,13 +205,15 @@ static int ttm_copy_ttm_io_page(struct t return -ENOMEM; dst = (void *)((unsigned long)dst + (page << PAGE_SHIFT)); - src = kmap_atomic_prot(s, prot); - if (!src) - return -ENOMEM; + /* + * Ensure that a highmem page is mapped with the correct + * pgprot. For non highmem the mapping is already there. + */ + src = kmap_local_page_prot(s, prot); memcpy_toio(dst, src, PAGE_SIZE); - kunmap_atomic(src); + kunmap_local(src); return 0; }
From: Thomas Gleixner <tglx at linutronix.de> There is no reason to disable pagefaults and preemption as a side effect of kmap_atomic_prot(). Use kmap_local_page_prot() instead and document the reasoning for the mapping usage with the given pgprot. Remove the NULL pointer check for the map. These functions return a valid address for valid pages and the return was bogus anyway as it would have left preemption and pagefaults disabled. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> Cc: Roland Scheidegger <sroland at vmware.com> Cc: Zack Rusin <zackr at vmware.com> Cc: David Airlie <airlied at linux.ie> Cc: Daniel Vetter <daniel at ffwll.ch> Cc: dri-devel at lists.freedesktop.org --- drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) --- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c @@ -375,12 +375,12 @@ static int vmw_bo_cpu_blit_line(struct v copy_size = min_t(u32, copy_size, PAGE_SIZE - src_page_offset); if (unmap_src) { - kunmap_atomic(d->src_addr); + kunmap_local(d->src_addr); d->src_addr = NULL; } if (unmap_dst) { - kunmap_atomic(d->dst_addr); + kunmap_local(d->dst_addr); d->dst_addr = NULL; } @@ -388,12 +388,8 @@ static int vmw_bo_cpu_blit_line(struct v if (WARN_ON_ONCE(dst_page >= d->dst_num_pages)) return -EINVAL; - d->dst_addr - kmap_atomic_prot(d->dst_pages[dst_page], - d->dst_prot); - if (!d->dst_addr) - return -ENOMEM; - + d->dst_addr = kmap_local_page_prot(d->dst_pages[dst_page], + d->dst_prot); d->mapped_dst = dst_page; } @@ -401,12 +397,8 @@ static int vmw_bo_cpu_blit_line(struct v if (WARN_ON_ONCE(src_page >= d->src_num_pages)) return -EINVAL; - d->src_addr - kmap_atomic_prot(d->src_pages[src_page], - d->src_prot); - if (!d->src_addr) - return -ENOMEM; - + d->src_addr = kmap_local_page_prot(d->src_pages[src_page], + d->src_prot); d->mapped_src = src_page; } diff->do_cpy(diff, d->dst_addr + dst_page_offset, @@ -436,8 +428,10 @@ static int vmw_bo_cpu_blit_line(struct v * * Performs a CPU blit from one buffer object to another avoiding a full * bo vmap which may exhaust- or fragment vmalloc space. - * On supported architectures (x86), we're using kmap_atomic which avoids - * cross-processor TLB- and cache flushes and may, on non-HIGHMEM systems + * + * On supported architectures (x86), we're using kmap_local_prot() which + * avoids cross-processor TLB- and cache flushes. kmap_local_prot() will + * either map a highmem page with the proper pgprot on HIGHMEM=y systems or * reference already set-up mappings. * * Neither of the buffer objects may be placed in PCI memory @@ -500,9 +494,9 @@ int vmw_bo_cpu_blit(struct ttm_buffer_ob } out: if (d.src_addr) - kunmap_atomic(d.src_addr); + kunmap_local(d.src_addr); if (d.dst_addr) - kunmap_atomic(d.dst_addr); + kunmap_local(d.dst_addr); return ret; }
From: Thomas Gleixner <tglx at linutronix.de> No more users. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: Andrew Morton <akpm at linux-foundation.org> Cc: linux-mm at kvack.org --- include/linux/highmem-internal.h | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) --- a/include/linux/highmem-internal.h +++ b/include/linux/highmem-internal.h @@ -88,16 +88,11 @@ static inline void __kunmap_local(void * kunmap_local_indexed(vaddr); } -static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot) +static inline void *kmap_atomic(struct page *page) { preempt_disable(); pagefault_disable(); - return __kmap_local_page_prot(page, prot); -} - -static inline void *kmap_atomic(struct page *page) -{ - return kmap_atomic_prot(page, kmap_prot); + return __kmap_local_page_prot(page, kmap_prot); } static inline void *kmap_atomic_pfn(unsigned long pfn) @@ -184,11 +179,6 @@ static inline void *kmap_atomic(struct p return page_address(page); } -static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot) -{ - return kmap_atomic(page); -} - static inline void *kmap_atomic_pfn(unsigned long pfn) { return kmap_atomic(pfn_to_page(pfn));
Thomas Gleixner
2021-Mar-03 13:20 UTC
[patch 4/7] drm/qxl: Replace io_mapping_map_atomic_wc()
From: Thomas Gleixner <tglx at linutronix.de> None of these mapping requires the side effect of disabling pagefaults and preemption. Use io_mapping_map_local_wc() instead, rename the related functions accordingly and clean up qxl_process_single_command() to use a plain copy_from_user() as the local maps are not disabling pagefaults. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: David Airlie <airlied at linux.ie> Cc: Gerd Hoffmann <kraxel at redhat.com> Cc: Daniel Vetter <daniel at ffwll.ch> Cc: virtualization at lists.linux-foundation.org Cc: spice-devel at lists.freedesktop.org Cc: dri-devel at lists.freedesktop.org --- drivers/gpu/drm/qxl/qxl_image.c | 18 +++++++++--------- drivers/gpu/drm/qxl/qxl_ioctl.c | 27 +++++++++++++-------------- drivers/gpu/drm/qxl/qxl_object.c | 12 ++++++------ drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_release.c | 4 ++-- 5 files changed, 32 insertions(+), 33 deletions(-) --- a/drivers/gpu/drm/qxl/qxl_image.c +++ b/drivers/gpu/drm/qxl/qxl_image.c @@ -124,12 +124,12 @@ qxl_image_init_helper(struct qxl_device wrong (check the bitmaps are sent correctly first) */ - ptr = qxl_bo_kmap_atomic_page(qdev, chunk_bo, 0); + ptr = qxl_bo_kmap_local_page(qdev, chunk_bo, 0); chunk = ptr; chunk->data_size = height * chunk_stride; chunk->prev_chunk = 0; chunk->next_chunk = 0; - qxl_bo_kunmap_atomic_page(qdev, chunk_bo, ptr); + qxl_bo_kunmap_local_page(qdev, chunk_bo, ptr); { void *k_data, *i_data; @@ -143,7 +143,7 @@ qxl_image_init_helper(struct qxl_device i_data = (void *)data; while (remain > 0) { - ptr = qxl_bo_kmap_atomic_page(qdev, chunk_bo, page << PAGE_SHIFT); + ptr = qxl_bo_kmap_local_page(qdev, chunk_bo, page << PAGE_SHIFT); if (page == 0) { chunk = ptr; @@ -157,7 +157,7 @@ qxl_image_init_helper(struct qxl_device memcpy(k_data, i_data, size); - qxl_bo_kunmap_atomic_page(qdev, chunk_bo, ptr); + qxl_bo_kunmap_local_page(qdev, chunk_bo, ptr); i_data += size; remain -= size; page++; @@ -175,10 +175,10 @@ qxl_image_init_helper(struct qxl_device page_offset = offset_in_page(out_offset); size = min((int)(PAGE_SIZE - page_offset), remain); - ptr = qxl_bo_kmap_atomic_page(qdev, chunk_bo, page_base); + ptr = qxl_bo_kmap_local_page(qdev, chunk_bo, page_base); k_data = ptr + page_offset; memcpy(k_data, i_data, size); - qxl_bo_kunmap_atomic_page(qdev, chunk_bo, ptr); + qxl_bo_kunmap_local_page(qdev, chunk_bo, ptr); remain -= size; i_data += size; out_offset += size; @@ -189,7 +189,7 @@ qxl_image_init_helper(struct qxl_device qxl_bo_kunmap(chunk_bo); image_bo = dimage->bo; - ptr = qxl_bo_kmap_atomic_page(qdev, image_bo, 0); + ptr = qxl_bo_kmap_local_page(qdev, image_bo, 0); image = ptr; image->descriptor.id = 0; @@ -212,7 +212,7 @@ qxl_image_init_helper(struct qxl_device break; default: DRM_ERROR("unsupported image bit depth\n"); - qxl_bo_kunmap_atomic_page(qdev, image_bo, ptr); + qxl_bo_kunmap_local_page(qdev, image_bo, ptr); return -EINVAL; } image->u.bitmap.flags = QXL_BITMAP_TOP_DOWN; @@ -222,7 +222,7 @@ qxl_image_init_helper(struct qxl_device image->u.bitmap.palette = 0; image->u.bitmap.data = qxl_bo_physical_address(qdev, chunk_bo, 0); - qxl_bo_kunmap_atomic_page(qdev, image_bo, ptr); + qxl_bo_kunmap_local_page(qdev, image_bo, ptr); return 0; } --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -89,11 +89,11 @@ apply_reloc(struct qxl_device *qdev, str { void *reloc_page; - reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); + reloc_page = qxl_bo_kmap_local_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); *(uint64_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = qxl_bo_physical_address(qdev, info->src_bo, info->src_offset); - qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page); + qxl_bo_kunmap_local_page(qdev, info->dst_bo, reloc_page); } static void @@ -105,9 +105,9 @@ apply_surf_reloc(struct qxl_device *qdev if (info->src_bo && !info->src_bo->is_primary) id = info->src_bo->surface_id; - reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); + reloc_page = qxl_bo_kmap_local_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); *(uint32_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = id; - qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page); + qxl_bo_kunmap_local_page(qdev, info->dst_bo, reloc_page); } /* return holding the reference to this object */ @@ -149,7 +149,6 @@ static int qxl_process_single_command(st struct qxl_bo *cmd_bo; void *fb_cmd; int i, ret, num_relocs; - int unwritten; switch (cmd->type) { case QXL_CMD_DRAW: @@ -184,21 +183,21 @@ static int qxl_process_single_command(st goto out_free_reloc; /* TODO copy slow path code from i915 */ - fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK)); - unwritten = __copy_from_user_inatomic_nocache - (fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_MASK), - u64_to_user_ptr(cmd->command), cmd->command_size); + fb_cmd = qxl_bo_kmap_local_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK)); - { + if (copy_from_user(fb_cmd + sizeof(union qxl_release_info) + + (release->release_offset & ~PAGE_MASK), + u64_to_user_ptr(cmd->command), cmd->command_size)) { + ret = -EFAULT; + } else { struct qxl_drawable *draw = fb_cmd; draw->mm_time = qdev->rom->mm_clock; } - qxl_bo_kunmap_atomic_page(qdev, cmd_bo, fb_cmd); - if (unwritten) { - DRM_ERROR("got unwritten %d\n", unwritten); - ret = -EFAULT; + qxl_bo_kunmap_local_page(qdev, cmd_bo, fb_cmd); + if (ret) { + DRM_ERROR("copy from user failed %d\n", ret); goto out_free_release; } --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -178,8 +178,8 @@ int qxl_bo_kmap(struct qxl_bo *bo, struc return 0; } -void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, - struct qxl_bo *bo, int page_offset) +void *qxl_bo_kmap_local_page(struct qxl_device *qdev, + struct qxl_bo *bo, int page_offset) { unsigned long offset; void *rptr; @@ -195,7 +195,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl goto fallback; offset = bo->tbo.mem.start << PAGE_SHIFT; - return io_mapping_map_atomic_wc(map, offset + page_offset); + return io_mapping_map_local_wc(map, offset + page_offset); fallback: if (bo->kptr) { rptr = bo->kptr + (page_offset * PAGE_SIZE); @@ -222,14 +222,14 @@ void qxl_bo_kunmap(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); } -void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, - struct qxl_bo *bo, void *pmap) +void qxl_bo_kunmap_local_page(struct qxl_device *qdev, + struct qxl_bo *bo, void *pmap) { if ((bo->tbo.mem.mem_type != TTM_PL_VRAM) && (bo->tbo.mem.mem_type != TTM_PL_PRIV)) goto fallback; - io_mapping_unmap_atomic(pmap); + io_mapping_unmap_local(pmap); return; fallback: qxl_bo_kunmap(bo); --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -65,8 +65,8 @@ extern int qxl_bo_create(struct qxl_devi struct qxl_bo **bo_ptr); extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); extern void qxl_bo_kunmap(struct qxl_bo *bo); -void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); -void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); +void *qxl_bo_kmap_local_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); +void qxl_bo_kunmap_local_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); extern void qxl_bo_unref(struct qxl_bo **bo); extern int qxl_bo_pin(struct qxl_bo *bo); --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -408,7 +408,7 @@ union qxl_release_info *qxl_release_map( union qxl_release_info *info; struct qxl_bo *bo = release->release_bo; - ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_MASK); + ptr = qxl_bo_kmap_local_page(qdev, bo, release->release_offset & PAGE_MASK); if (!ptr) return NULL; info = ptr + (release->release_offset & ~PAGE_MASK); @@ -423,7 +423,7 @@ void qxl_release_unmap(struct qxl_device void *ptr; ptr = ((void *)info) - (release->release_offset & ~PAGE_MASK); - qxl_bo_kunmap_atomic_page(qdev, bo, ptr); + qxl_bo_kunmap_local_page(qdev, bo, ptr); } void qxl_release_fence_buffer_objects(struct qxl_release *release)
Thomas Gleixner
2021-Mar-03 13:20 UTC
[patch 5/7] drm/nouveau/device: Replace io_mapping_map_atomic_wc()
From: Thomas Gleixner <tglx at linutronix.de> Neither fbmem_peek() nor fbmem_poke() require to disable pagefaults and preemption as a side effect of io_mapping_map_atomic_wc(). Use io_mapping_map_local_wc() instead. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: Ben Skeggs <bskeggs at redhat.com> Cc: David Airlie <airlied at linux.ie> Cc: Daniel Vetter <daniel at ffwll.ch> Cc: dri-devel at lists.freedesktop.org Cc: nouveau at lists.freedesktop.org --- drivers/gpu/drm/nouveau/nvkm/subdev/devinit/fbmem.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- a/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/fbmem.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/fbmem.h @@ -60,19 +60,19 @@ fbmem_fini(struct io_mapping *fb) static inline u32 fbmem_peek(struct io_mapping *fb, u32 off) { - u8 __iomem *p = io_mapping_map_atomic_wc(fb, off & PAGE_MASK); + u8 __iomem *p = io_mapping_map_local_wc(fb, off & PAGE_MASK); u32 val = ioread32(p + (off & ~PAGE_MASK)); - io_mapping_unmap_atomic(p); + io_mapping_unmap_local(p); return val; } static inline void fbmem_poke(struct io_mapping *fb, u32 off, u32 val) { - u8 __iomem *p = io_mapping_map_atomic_wc(fb, off & PAGE_MASK); + u8 __iomem *p = io_mapping_map_local_wc(fb, off & PAGE_MASK); iowrite32(val, p + (off & ~PAGE_MASK)); wmb(); - io_mapping_unmap_atomic(p); + io_mapping_unmap_local(p); } static inline bool
Thomas Gleixner
2021-Mar-03 13:20 UTC
[patch 6/7] drm/i915: Replace io_mapping_map_atomic_wc()
From: Thomas Gleixner <tglx at linutronix.de> None of these mapping requires the side effect of disabling pagefaults and preemption. Use io_mapping_map_local_wc() instead, and clean up gtt_user_read() and gtt_user_write() to use a plain copy_from_user() as the local maps are not disabling pagefaults. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: Jani Nikula <jani.nikula at linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com> Cc: David Airlie <airlied at linux.ie> Cc: Daniel Vetter <daniel at ffwll.ch> Cc: Chris Wilson <chris at chris-wilson.co.uk> Cc: intel-gfx at lists.freedesktop.org Cc: dri-devel at lists.freedesktop.org --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 +--- drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++----------------- drivers/gpu/drm/i915/selftests/i915_gem.c | 4 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 8 ++--- 4 files changed, 22 insertions(+), 37 deletions(-) --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1080,7 +1080,7 @@ static void reloc_cache_reset(struct rel struct i915_ggtt *ggtt = cache_to_ggtt(cache); intel_gt_flush_ggtt_writes(ggtt->vm.gt); - io_mapping_unmap_atomic((void __iomem *)vaddr); + io_mapping_unmap_local((void __iomem *)vaddr); if (drm_mm_node_allocated(&cache->node)) { ggtt->vm.clear_range(&ggtt->vm, @@ -1146,7 +1146,7 @@ static void *reloc_iomap(struct drm_i915 if (cache->vaddr) { intel_gt_flush_ggtt_writes(ggtt->vm.gt); - io_mapping_unmap_atomic((void __force __iomem *) unmask_page(cache->vaddr)); + io_mapping_unmap_local((void __force __iomem *) unmask_page(cache->vaddr)); } else { struct i915_vma *vma; int err; @@ -1194,8 +1194,7 @@ static void *reloc_iomap(struct drm_i915 offset += page << PAGE_SHIFT; } - vaddr = (void __force *)io_mapping_map_atomic_wc(&ggtt->iomap, - offset); + vaddr = (void __force *)io_mapping_map_local_wc(&ggtt->iomap, offset); cache->page = page; cache->vaddr = (unsigned long)vaddr; --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -253,22 +253,15 @@ gtt_user_read(struct io_mapping *mapping char __user *user_data, int length) { void __iomem *vaddr; - unsigned long unwritten; + bool fail = false; /* We can use the cpu mem copy function because this is X86. */ - vaddr = io_mapping_map_atomic_wc(mapping, base); - unwritten = __copy_to_user_inatomic(user_data, - (void __force *)vaddr + offset, - length); - io_mapping_unmap_atomic(vaddr); - if (unwritten) { - vaddr = io_mapping_map_wc(mapping, base, PAGE_SIZE); - unwritten = copy_to_user(user_data, - (void __force *)vaddr + offset, - length); - io_mapping_unmap(vaddr); - } - return unwritten; + vaddr = io_mapping_map_local_wc(mapping, base); + if (copy_to_user(user_data, (void __force *)vaddr + offset, length)) + fail = true; + io_mapping_unmap_local(vaddr); + + return fail; } static int @@ -437,21 +430,14 @@ ggtt_write(struct io_mapping *mapping, char __user *user_data, int length) { void __iomem *vaddr; - unsigned long unwritten; + bool fail = false; /* We can use the cpu mem copy function because this is X86. */ - vaddr = io_mapping_map_atomic_wc(mapping, base); - unwritten = __copy_from_user_inatomic_nocache((void __force *)vaddr + offset, - user_data, length); - io_mapping_unmap_atomic(vaddr); - if (unwritten) { - vaddr = io_mapping_map_wc(mapping, base, PAGE_SIZE); - unwritten = copy_from_user((void __force *)vaddr + offset, - user_data, length); - io_mapping_unmap(vaddr); - } - - return unwritten; + vaddr = io_mapping_map_local_wc(mapping, base); + if (copy_from_user((void __force *)vaddr + offset, user_data, length)) + fail = true; + io_mapping_unmap_local(vaddr); + return fail; } /** --- a/drivers/gpu/drm/i915/selftests/i915_gem.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c @@ -58,12 +58,12 @@ static void trash_stolen(struct drm_i915 ggtt->vm.insert_page(&ggtt->vm, dma, slot, I915_CACHE_NONE, 0); - s = io_mapping_map_atomic_wc(&ggtt->iomap, slot); + s = io_mapping_map_local_wc(&ggtt->iomap, slot); for (x = 0; x < PAGE_SIZE / sizeof(u32); x++) { prng = next_pseudo_random32(prng); iowrite32(prng, &s[x]); } - io_mapping_unmap_atomic(s); + io_mapping_unmap_local(s); } ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE); --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -1201,9 +1201,9 @@ static int igt_ggtt_page(void *arg) u64 offset = tmp.start + order[n] * PAGE_SIZE; u32 __iomem *vaddr; - vaddr = io_mapping_map_atomic_wc(&ggtt->iomap, offset); + vaddr = io_mapping_map_local_wc(&ggtt->iomap, offset); iowrite32(n, vaddr + n); - io_mapping_unmap_atomic(vaddr); + io_mapping_unmap_local(vaddr); } intel_gt_flush_ggtt_writes(ggtt->vm.gt); @@ -1213,9 +1213,9 @@ static int igt_ggtt_page(void *arg) u32 __iomem *vaddr; u32 val; - vaddr = io_mapping_map_atomic_wc(&ggtt->iomap, offset); + vaddr = io_mapping_map_local_wc(&ggtt->iomap, offset); val = ioread32(vaddr + n); - io_mapping_unmap_atomic(vaddr); + io_mapping_unmap_local(vaddr); if (val != n) { pr_err("insert page failed: found %d, expected %d\n",
Thomas Gleixner
2021-Mar-03 13:20 UTC
[patch 7/7] io-mapping: Remove io_mapping_map_atomic_wc()
From: Thomas Gleixner <tglx at linutronix.de> No more users. Get rid of it and remove the traces in documentation. Signed-off-by: Thomas Gleixner <tglx at linutronix.de> Cc: Andrew Morton <akpm at linux-foundation.org> Cc: linux-mm at kvack.org --- Documentation/driver-api/io-mapping.rst | 22 +++++----------- include/linux/io-mapping.h | 42 +------------------------------- 2 files changed, 9 insertions(+), 55 deletions(-) --- a/Documentation/driver-api/io-mapping.rst +++ b/Documentation/driver-api/io-mapping.rst @@ -21,19 +21,15 @@ mappable, while 'size' indicates how lar enable. Both are in bytes. This _wc variant provides a mapping which may only be used with -io_mapping_map_atomic_wc(), io_mapping_map_local_wc() or -io_mapping_map_wc(). +io_mapping_map_local_wc() or io_mapping_map_wc(). With this mapping object, individual pages can be mapped either temporarily or long term, depending on the requirements. Of course, temporary maps are -more efficient. They come in two flavours:: +more efficient. void *io_mapping_map_local_wc(struct io_mapping *mapping, unsigned long offset) - void *io_mapping_map_atomic_wc(struct io_mapping *mapping, - unsigned long offset) - 'offset' is the offset within the defined mapping region. Accessing addresses beyond the region specified in the creation function yields undefined results. Using an offset which is not page aligned yields an @@ -50,9 +46,6 @@ io_mapping_map_local_wc() has a side eff migration to make the mapping code work. No caller can rely on this side effect. -io_mapping_map_atomic_wc() has the side effect of disabling preemption and -pagefaults. Don't use in new code. Use io_mapping_map_local_wc() instead. - Nested mappings need to be undone in reverse order because the mapping code uses a stack for keeping track of them:: @@ -65,11 +58,10 @@ Nested mappings need to be undone in rev The mappings are released with:: void io_mapping_unmap_local(void *vaddr) - void io_mapping_unmap_atomic(void *vaddr) -'vaddr' must be the value returned by the last io_mapping_map_local_wc() or -io_mapping_map_atomic_wc() call. This unmaps the specified mapping and -undoes the side effects of the mapping functions. +'vaddr' must be the value returned by the last io_mapping_map_local_wc() +call. This unmaps the specified mapping and undoes eventual side effects of +the mapping function. If you need to sleep while holding a mapping, you can use the regular variant, although this may be significantly slower:: @@ -77,8 +69,8 @@ If you need to sleep while holding a map void *io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset) -This works like io_mapping_map_atomic/local_wc() except it has no side -effects and the pointer is globaly visible. +This works like io_mapping_map_local_wc() except it has no side effects and +the pointer is globaly visible. The mappings are released with:: --- a/include/linux/io-mapping.h +++ b/include/linux/io-mapping.h @@ -60,28 +60,7 @@ io_mapping_fini(struct io_mapping *mappi iomap_free(mapping->base, mapping->size); } -/* Atomic map/unmap */ -static inline void __iomem * -io_mapping_map_atomic_wc(struct io_mapping *mapping, - unsigned long offset) -{ - resource_size_t phys_addr; - - BUG_ON(offset >= mapping->size); - phys_addr = mapping->base + offset; - preempt_disable(); - pagefault_disable(); - return __iomap_local_pfn_prot(PHYS_PFN(phys_addr), mapping->prot); -} - -static inline void -io_mapping_unmap_atomic(void __iomem *vaddr) -{ - kunmap_local_indexed((void __force *)vaddr); - pagefault_enable(); - preempt_enable(); -} - +/* Temporary mappings which are only valid in the current context */ static inline void __iomem * io_mapping_map_local_wc(struct io_mapping *mapping, unsigned long offset) { @@ -163,24 +142,7 @@ io_mapping_unmap(void __iomem *vaddr) { } -/* Atomic map/unmap */ -static inline void __iomem * -io_mapping_map_atomic_wc(struct io_mapping *mapping, - unsigned long offset) -{ - preempt_disable(); - pagefault_disable(); - return io_mapping_map_wc(mapping, offset, PAGE_SIZE); -} - -static inline void -io_mapping_unmap_atomic(void __iomem *vaddr) -{ - io_mapping_unmap(vaddr); - pagefault_enable(); - preempt_enable(); -} - +/* Temporary mappings which are only valid in the current context */ static inline void __iomem * io_mapping_map_local_wc(struct io_mapping *mapping, unsigned long offset) {
> On Mar 3, 2021, at 08:20, Thomas Gleixner <tglx at linutronix.de> wrote: > > From: Thomas Gleixner <tglx at linutronix.de> > > There is no reason to disable pagefaults and preemption as a side effect of > kmap_atomic_prot(). > > Use kmap_local_page_prot() instead and document the reasoning for the > mapping usage with the given pgprot. > > Remove the NULL pointer check for the map. These functions return a valid > address for valid pages and the return was bogus anyway as it would have > left preemption and pagefaults disabled. > > Signed-off-by: Thomas Gleixner <tglx at linutronix.de> > Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> > Cc: Roland Scheidegger <sroland at vmware.com> > Cc: Zack Rusin <zackr at vmware.com> > Cc: David Airlie <airlied at linux.ie> > Cc: Daniel Vetter <daniel at ffwll.ch> > Cc: dri-devel at lists.freedesktop.org > --- > drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 30 ++++++++++++------------------ > 1 file changed, 12 insertions(+), 18 deletions(-) > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c > @@ -375,12 +375,12 @@ static int vmw_bo_cpu_blit_line(struct v > copy_size = min_t(u32, copy_size, PAGE_SIZE - src_page_offset); > > if (unmap_src) { > - kunmap_atomic(d->src_addr); > + kunmap_local(d->src_addr); > d->src_addr = NULL; > } > > if (unmap_dst) { > - kunmap_atomic(d->dst_addr); > + kunmap_local(d->dst_addr); > d->dst_addr = NULL; > } > > @@ -388,12 +388,8 @@ static int vmw_bo_cpu_blit_line(struct v > if (WARN_ON_ONCE(dst_page >= d->dst_num_pages)) > return -EINVAL; > > - d->dst_addr > - kmap_atomic_prot(d->dst_pages[dst_page], > - d->dst_prot); > - if (!d->dst_addr) > - return -ENOMEM; > - > + d->dst_addr = kmap_local_page_prot(d->dst_pages[dst_page], > + d->dst_prot); > d->mapped_dst = dst_page; > } > > @@ -401,12 +397,8 @@ static int vmw_bo_cpu_blit_line(struct v > if (WARN_ON_ONCE(src_page >= d->src_num_pages)) > return -EINVAL; > > - d->src_addr > - kmap_atomic_prot(d->src_pages[src_page], > - d->src_prot); > - if (!d->src_addr) > - return -ENOMEM; > - > + d->src_addr = kmap_local_page_prot(d->src_pages[src_page], > + d->src_prot); > d->mapped_src = src_page; > } > diff->do_cpy(diff, d->dst_addr + dst_page_offset, > @@ -436,8 +428,10 @@ static int vmw_bo_cpu_blit_line(struct v > * > * Performs a CPU blit from one buffer object to another avoiding a full > * bo vmap which may exhaust- or fragment vmalloc space. > - * On supported architectures (x86), we're using kmap_atomic which avoids > - * cross-processor TLB- and cache flushes and may, on non-HIGHMEM systems > + * > + * On supported architectures (x86), we're using kmap_local_prot() which > + * avoids cross-processor TLB- and cache flushes. kmap_local_prot() will > + * either map a highmem page with the proper pgprot on HIGHMEM=y systems or > * reference already set-up mappings. > * > * Neither of the buffer objects may be placed in PCI memory > @@ -500,9 +494,9 @@ int vmw_bo_cpu_blit(struct ttm_buffer_ob > } > out: > if (d.src_addr) > - kunmap_atomic(d.src_addr); > + kunmap_local(d.src_addr); > if (d.dst_addr) > - kunmap_atomic(d.dst_addr); > + kunmap_local(d.dst_addr); > > return ret; > }Looks good. Thanks. Reviewed-by: Zack Rusin <zackr at vmware.com> z
Roland Scheidegger
2021-Mar-05 15:35 UTC
[Nouveau] [patch 2/7] drm/vmgfx: Replace kmap_atomic()
On 03.03.21 14:20, Thomas Gleixner wrote:> From: Thomas Gleixner <tglx at linutronix.de> > > There is no reason to disable pagefaults and preemption as a side effect of > kmap_atomic_prot(). > > Use kmap_local_page_prot() instead and document the reasoning for the > mapping usage with the given pgprot. > > Remove the NULL pointer check for the map. These functions return a valid > address for valid pages and the return was bogus anyway as it would have > left preemption and pagefaults disabled. > > Signed-off-by: Thomas Gleixner <tglx at linutronix.de> > Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> > Cc: Roland Scheidegger <sroland at vmware.com> > Cc: Zack Rusin <zackr at vmware.com> > Cc: David Airlie <airlied at linux.ie> > Cc: Daniel Vetter <daniel at ffwll.ch> > Cc: dri-devel at lists.freedesktop.org > --- > drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 30 ++++++++++++------------------ > 1 file changed, 12 insertions(+), 18 deletions(-) > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c > @@ -375,12 +375,12 @@ static int vmw_bo_cpu_blit_line(struct v > copy_size = min_t(u32, copy_size, PAGE_SIZE - src_page_offset); > > if (unmap_src) { > - kunmap_atomic(d->src_addr); > + kunmap_local(d->src_addr); > d->src_addr = NULL; > } > > if (unmap_dst) { > - kunmap_atomic(d->dst_addr); > + kunmap_local(d->dst_addr); > d->dst_addr = NULL; > } > > @@ -388,12 +388,8 @@ static int vmw_bo_cpu_blit_line(struct v > if (WARN_ON_ONCE(dst_page >= d->dst_num_pages)) > return -EINVAL; > > - d->dst_addr > - kmap_atomic_prot(d->dst_pages[dst_page], > - d->dst_prot); > - if (!d->dst_addr) > - return -ENOMEM; > - > + d->dst_addr = kmap_local_page_prot(d->dst_pages[dst_page], > + d->dst_prot); > d->mapped_dst = dst_page; > } > > @@ -401,12 +397,8 @@ static int vmw_bo_cpu_blit_line(struct v > if (WARN_ON_ONCE(src_page >= d->src_num_pages)) > return -EINVAL; > > - d->src_addr > - kmap_atomic_prot(d->src_pages[src_page], > - d->src_prot); > - if (!d->src_addr) > - return -ENOMEM; > - > + d->src_addr = kmap_local_page_prot(d->src_pages[src_page], > + d->src_prot); > d->mapped_src = src_page; > } > diff->do_cpy(diff, d->dst_addr + dst_page_offset, > @@ -436,8 +428,10 @@ static int vmw_bo_cpu_blit_line(struct v > * > * Performs a CPU blit from one buffer object to another avoiding a full > * bo vmap which may exhaust- or fragment vmalloc space. > - * On supported architectures (x86), we're using kmap_atomic which avoids > - * cross-processor TLB- and cache flushes and may, on non-HIGHMEM systems > + * > + * On supported architectures (x86), we're using kmap_local_prot() which > + * avoids cross-processor TLB- and cache flushes. kmap_local_prot() will > + * either map a highmem page with the proper pgprot on HIGHMEM=y systems or > * reference already set-up mappings. > * > * Neither of the buffer objects may be placed in PCI memory > @@ -500,9 +494,9 @@ int vmw_bo_cpu_blit(struct ttm_buffer_ob > } > out: > if (d.src_addr) > - kunmap_atomic(d.src_addr); > + kunmap_local(d.src_addr); > if (d.dst_addr) > - kunmap_atomic(d.dst_addr); > + kunmap_local(d.dst_addr); > > return ret; > } > >Seems reasonable to me. Reviewed-by: Roland Scheidegger <sroland at vmware.com>