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>