* iosys-map: refactor to reduce struct size
@ 2025-05-22 6:52 Dave Airlie
2025-05-22 6:52 ` [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally Dave Airlie
` (15 more replies)
0 siblings, 16 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
Hey iosys_map users :)
I fell down a bit of a refactor hole today, it was just random and
sometimes you just have to let these things run their course.
I noticed iosys_map has a 7-byte hole in a 16-byte structure, and
it gets embedded into a bunch of other structs and it offended my
sensibilities.
This series makes iosys_map be 8-bytes, using the bottom bit of
the void * to store the is_iomem.
Patch 1: adds new accessors to start hiding internals
Patches 2-7: refactor all users in-tree to use new internals
(hopefully got them all)
Patch8: moves the internals around to catch anything not in-tree.
Patch9: reimplements iosys_map as 8-bytes by hiding the is_iomem
inside the pointer.
Dave.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 11:58 ` Thomas Zimmermann
2025-05-22 6:52 ` [PATCH 2/9] udmabuf: use new iosys_map accessors Dave Airlie
` (14 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This adds accessors inlines to the iosys-map. The intent is to
roll the iomem flag into the lower bits of the vaddr eventually.
First just add accessors to move all current in-tree users over to.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
include/linux/iosys-map.h | 53 +++++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 19 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index 4696abfd311c..5ce5df1db60a 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -114,6 +114,21 @@ struct iosys_map {
bool is_iomem;
};
+static inline bool iosys_map_is_iomem(const struct iosys_map *map)
+{
+ return map->is_iomem;
+}
+
+static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
+{
+ return map->vaddr_iomem;
+}
+
+static inline void *iosys_map_ptr(const struct iosys_map *map)
+{
+ return map->vaddr;
+}
+
/**
* IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address in system memory
* @vaddr_: A system-memory address
@@ -234,9 +249,9 @@ static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
*/
static inline bool iosys_map_is_null(const struct iosys_map *map)
{
- if (map->is_iomem)
- return !map->vaddr_iomem;
- return !map->vaddr;
+ if (iosys_map_is_iomem(map))
+ return !iosys_map_ioptr(map);
+ return !iosys_map_ptr(map);
}
/**
@@ -286,10 +301,10 @@ static inline void iosys_map_clear(struct iosys_map *map)
static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
const void *src, size_t len)
{
- if (dst->is_iomem)
- memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
+ if (iosys_map_is_iomem(dst))
+ memcpy_toio(iosys_map_ioptr(dst) + dst_offset, src, len);
else
- memcpy(dst->vaddr + dst_offset, src, len);
+ memcpy(iosys_map_ptr(dst) + dst_offset, src, len);
}
/**
@@ -306,10 +321,10 @@ static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
size_t src_offset, size_t len)
{
- if (src->is_iomem)
- memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
+ if (iosys_map_is_iomem(src))
+ memcpy_fromio(dst, iosys_map_ioptr(src) + src_offset, len);
else
- memcpy(dst, src->vaddr + src_offset, len);
+ memcpy(dst, iosys_map_ptr(src) + src_offset, len);
}
/**
@@ -322,7 +337,7 @@ static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
*/
static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
{
- if (map->is_iomem)
+ if (iosys_map_is_iomem(map))
map->vaddr_iomem += incr;
else
map->vaddr += incr;
@@ -341,10 +356,10 @@ static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
int value, size_t len)
{
- if (dst->is_iomem)
- memset_io(dst->vaddr_iomem + offset, value, len);
+ if (iosys_map_is_iomem(dst))
+ memset_io(iosys_map_ioptr(dst) + offset, value, len);
else
- memset(dst->vaddr + offset, value, len);
+ memset(iosys_map_ptr(dst) + offset, value, len);
}
#ifdef CONFIG_64BIT
@@ -393,10 +408,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
*/
#define iosys_map_rd(map__, offset__, type__) ({ \
type__ val_; \
- if ((map__)->is_iomem) { \
- __iosys_map_rd_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
+ if (iosys_map_is_iomem(map__)) { \
+ __iosys_map_rd_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
} else { \
- __iosys_map_rd_sys(val_, (map__)->vaddr + (offset__), type__); \
+ __iosys_map_rd_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
} \
val_; \
})
@@ -415,10 +430,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
*/
#define iosys_map_wr(map__, offset__, type__, val__) ({ \
type__ val_ = (val__); \
- if ((map__)->is_iomem) { \
- __iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
+ if (iosys_map_is_iomem(map__)) { \
+ __iosys_map_wr_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
} else { \
- __iosys_map_wr_sys(val_, (map__)->vaddr + (offset__), type__); \
+ __iosys_map_wr_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
} \
})
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/9] udmabuf: use new iosys_map accessors.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
2025-05-22 6:52 ` [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 6:52 ` [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals Dave Airlie
` (13 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This just avoids looking into the internals of the iosys_map.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/dma-buf/udmabuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 7eee3eb47a8e..101cc7853043 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -145,7 +145,7 @@ static void vunmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
dma_resv_assert_held(buf->resv);
- vm_unmap_ram(map->vaddr, ubuf->pagecount);
+ vm_unmap_ram(iosys_map_ptr(map), ubuf->pagecount);
}
static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
2025-05-22 6:52 ` [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally Dave Airlie
2025-05-22 6:52 ` [PATCH 2/9] udmabuf: use new iosys_map accessors Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-23 0:32 ` kernel test robot
2025-05-22 6:52 ` [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map Dave Airlie
` (12 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
Use the new accessor interfaces to avoid directly accessing the
internals.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/firmware/tegra/ivc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/tegra/ivc.c b/drivers/firmware/tegra/ivc.c
index 8c9aff9804c0..eb59acb08065 100644
--- a/drivers/firmware/tegra/ivc.c
+++ b/drivers/firmware/tegra/ivc.c
@@ -629,18 +629,18 @@ static inline void iosys_map_copy(struct iosys_map *dst, const struct iosys_map
static inline unsigned long iosys_map_get_address(const struct iosys_map *map)
{
- if (map->is_iomem)
- return (unsigned long)map->vaddr_iomem;
+ if (iosys_map_is_iomem(map)
+ return (unsigned long)iosys_map_ioptr(map);
- return (unsigned long)map->vaddr;
+ return (unsigned long)iosys_map_ptr(map);
}
static inline void *iosys_map_get_vaddr(const struct iosys_map *map)
{
- if (WARN_ON(map->is_iomem))
+ if (WARN_ON(iosys_map_is_iomem(map)))
return NULL;
- return map->vaddr;
+ return iosys_map_ptr(map);
}
int tegra_ivc_init(struct tegra_ivc *ivc, struct device *peer, const struct iosys_map *rx,
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (2 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 13:56 ` Lucas De Marchi
2025-05-22 6:52 ` [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals Dave Airlie
` (11 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This uses the new accessors to avoid touch iosys_map internals.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/xe/display/intel_fbdev_fb.c | 2 +-
drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
drivers/gpu/drm/xe/xe_eu_stall.c | 2 +-
drivers/gpu/drm/xe/xe_guc_pc.c | 2 +-
drivers/gpu/drm/xe/xe_map.h | 12 ++++++------
drivers/gpu/drm/xe/xe_memirq.c | 16 ++++++++--------
drivers/gpu/drm/xe/xe_oa.c | 4 ++--
drivers/gpu/drm/xe/xe_pt.c | 4 ++--
drivers/gpu/drm/xe/xe_sa.c | 8 ++++----
9 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
index e8191562d122..ad2681c90efb 100644
--- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
+++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
@@ -101,7 +101,7 @@ int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info
}
XE_WARN_ON(iosys_map_is_null(&obj->vmap));
- info->screen_base = obj->vmap.vaddr_iomem;
+ info->screen_base = iosys_map_ioptr(&obj->vmap);
info->screen_size = obj->ttm.base.size;
return 0;
diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index d99d91fe8aa9..c83a54708495 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -1249,7 +1249,7 @@ int xe_bo_evict_pinned(struct xe_bo *bo)
unmap = true;
}
- xe_map_memcpy_from(xe, backup->vmap.vaddr, &bo->vmap, 0,
+ xe_map_memcpy_from(xe, iosys_map_ptr(&backup->vmap), &bo->vmap, 0,
bo->size);
}
@@ -1342,7 +1342,7 @@ int xe_bo_restore_pinned(struct xe_bo *bo)
unmap = true;
}
- xe_map_memcpy_to(xe, &bo->vmap, 0, backup->vmap.vaddr,
+ xe_map_memcpy_to(xe, &bo->vmap, 0, iosys_map_ptr(&backup->vmap),
bo->size);
}
@@ -2226,9 +2226,9 @@ int xe_managed_bo_reinit_in_vram(struct xe_device *xe, struct xe_tile *tile, str
XE_BO_FLAG_PINNED_NORESTORE);
xe_assert(xe, IS_DGFX(xe));
- xe_assert(xe, !(*src)->vmap.is_iomem);
+ xe_assert(xe, !iosys_map_is_iomem(&(*src)->vmap));
- bo = xe_managed_bo_create_from_data(xe, tile, (*src)->vmap.vaddr,
+ bo = xe_managed_bo_create_from_data(xe, tile, iosys_map_ptr(&(*src)->vmap),
(*src)->size, dst_flags);
if (IS_ERR(bo))
return PTR_ERR(bo);
diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
index 96732613b4b7..d8f900efac95 100644
--- a/drivers/gpu/drm/xe/xe_eu_stall.c
+++ b/drivers/gpu/drm/xe/xe_eu_stall.c
@@ -741,7 +741,7 @@ static int xe_eu_stall_stream_init(struct xe_eu_stall_data_stream *stream,
for_each_dss_steering(xecore, gt, group, instance) {
xecore_buf = &stream->xecore_buf[xecore];
vaddr_offset = xecore * stream->per_xecore_buf_size;
- xecore_buf->vaddr = stream->bo->vmap.vaddr + vaddr_offset;
+ xecore_buf->vaddr = iosys_map_ptr(&stream->bo->vmap) + vaddr_offset;
}
return 0;
}
diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
index 18c623992035..c7ad56774c99 100644
--- a/drivers/gpu/drm/xe/xe_guc_pc.c
+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
@@ -1068,7 +1068,7 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
goto out;
}
- memset(pc->bo->vmap.vaddr, 0, size);
+ memset(iosys_map_ptr(&pc->bo->vmap), 0, size);
slpc_shared_data_write(pc, header.size, size);
earlier = ktime_get();
diff --git a/drivers/gpu/drm/xe/xe_map.h b/drivers/gpu/drm/xe/xe_map.h
index f62e0c8b67ab..37842c02c7f9 100644
--- a/drivers/gpu/drm/xe/xe_map.h
+++ b/drivers/gpu/drm/xe/xe_map.h
@@ -49,10 +49,10 @@ static inline u32 xe_map_read32(struct xe_device *xe, struct iosys_map *map)
{
xe_device_assert_mem_access(xe);
- if (map->is_iomem)
- return readl(map->vaddr_iomem);
+ if (iosys_map_is_iomem(map))
+ return readl(iosys_map_ioptr(map));
else
- return READ_ONCE(*(u32 *)map->vaddr);
+ return READ_ONCE(*(u32 *)iosys_map_ptr(map));
}
static inline void xe_map_write32(struct xe_device *xe, struct iosys_map *map,
@@ -60,10 +60,10 @@ static inline void xe_map_write32(struct xe_device *xe, struct iosys_map *map,
{
xe_device_assert_mem_access(xe);
- if (map->is_iomem)
- writel(val, map->vaddr_iomem);
+ if (iosys_map_is_iomem(map))
+ writel(val, iosys_map_ioptr(map));
else
- *(u32 *)map->vaddr = val;
+ *(u32 *)iosys_map_ptr(map) = val;
}
#define xe_map_rd(xe__, map__, offset__, type__) ({ \
diff --git a/drivers/gpu/drm/xe/xe_memirq.c b/drivers/gpu/drm/xe/xe_memirq.c
index 49c45ec3e83c..458955c75e04 100644
--- a/drivers/gpu/drm/xe/xe_memirq.c
+++ b/drivers/gpu/drm/xe/xe_memirq.c
@@ -198,9 +198,9 @@ static int memirq_alloc_pages(struct xe_memirq *memirq)
memirq->status = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_STATUS_OFFSET(0));
memirq->mask = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_ENABLE_OFFSET);
- memirq_assert(memirq, !memirq->source.is_iomem);
- memirq_assert(memirq, !memirq->status.is_iomem);
- memirq_assert(memirq, !memirq->mask.is_iomem);
+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->source));
+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->status));
+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->mask));
memirq_debug(memirq, "page offsets: bo %#x bo_size %zu source %#x status %#x\n",
xe_bo_ggtt_addr(bo), bo_size, XE_MEMIRQ_SOURCE_OFFSET(0),
@@ -418,7 +418,7 @@ static bool memirq_received(struct xe_memirq *memirq, struct iosys_map *vector,
static void memirq_dispatch_engine(struct xe_memirq *memirq, struct iosys_map *status,
struct xe_hw_engine *hwe)
{
- memirq_debug(memirq, "STATUS %s %*ph\n", hwe->name, 16, status->vaddr);
+ memirq_debug(memirq, "STATUS %s %*ph\n", hwe->name, 16, iosys_map_ptr(status));
if (memirq_received(memirq, status, ilog2(GT_RENDER_USER_INTERRUPT), hwe->name))
xe_hw_engine_handle_irq(hwe, GT_RENDER_USER_INTERRUPT);
@@ -429,7 +429,7 @@ static void memirq_dispatch_guc(struct xe_memirq *memirq, struct iosys_map *stat
{
const char *name = guc_name(guc);
- memirq_debug(memirq, "STATUS %s %*ph\n", name, 16, status->vaddr);
+ memirq_debug(memirq, "STATUS %s %*ph\n", name, 16, iosys_map_ptr(status));
if (memirq_received(memirq, status, ilog2(GUC_INTR_GUC2HOST), name))
xe_guc_irq_handler(guc, GUC_INTR_GUC2HOST);
@@ -479,9 +479,9 @@ void xe_memirq_handler(struct xe_memirq *memirq)
if (!memirq->bo)
return;
- memirq_assert(memirq, !memirq->source.is_iomem);
- memirq_debug(memirq, "SOURCE %*ph\n", 32, memirq->source.vaddr);
- memirq_debug(memirq, "SOURCE %*ph\n", 32, memirq->source.vaddr + 32);
+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->source));
+ memirq_debug(memirq, "SOURCE %*ph\n", 32, iosys_map_ptr(&memirq->source));
+ memirq_debug(memirq, "SOURCE %*ph\n", 32, iosys_map_ptr(&memirq->source) + 32);
for_each_gt(gt, xe, gtid) {
if (gt->tile != tile)
diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
index fb842fa0552e..99424d790d84 100644
--- a/drivers/gpu/drm/xe/xe_oa.c
+++ b/drivers/gpu/drm/xe/xe_oa.c
@@ -880,8 +880,8 @@ static int xe_oa_alloc_oa_buffer(struct xe_oa_stream *stream, size_t size)
stream->oa_buffer.bo = bo;
/* mmap implementation requires OA buffer to be in system memory */
- xe_assert(stream->oa->xe, bo->vmap.is_iomem == 0);
- stream->oa_buffer.vaddr = bo->vmap.vaddr;
+ xe_assert(stream->oa->xe, iosys_map_is_iomem(&bo->vmap) == 0);
+ stream->oa_buffer.vaddr = iosys_map_ptr(&bo->vmap);
return 0;
}
diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
index b42cf5d1b20c..af0992aea6b4 100644
--- a/drivers/gpu/drm/xe/xe_pt.c
+++ b/drivers/gpu/drm/xe/xe_pt.c
@@ -1723,12 +1723,12 @@ xe_migrate_clear_pgtable_callback(struct xe_migrate_pt_update *pt_update,
u64 empty = __xe_pt_empty_pte(tile, vm, update->pt->level);
int i;
- if (map && map->is_iomem)
+ if (map && iosys_map_is_iomem(map))
for (i = 0; i < num_qwords; ++i)
xe_map_wr(tile_to_xe(tile), map, (qword_ofs + i) *
sizeof(u64), u64, empty);
else if (map)
- memset64(map->vaddr + qword_ofs * sizeof(u64), empty,
+ memset64(iosys_map_ptr(map) + qword_ofs * sizeof(u64), empty,
num_qwords);
else
memset64(ptr, empty, num_qwords);
diff --git a/drivers/gpu/drm/xe/xe_sa.c b/drivers/gpu/drm/xe/xe_sa.c
index 1d43e183ca21..4ac335c68242 100644
--- a/drivers/gpu/drm/xe/xe_sa.c
+++ b/drivers/gpu/drm/xe/xe_sa.c
@@ -68,15 +68,15 @@ struct xe_sa_manager *__xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u3
return ERR_CAST(bo);
}
sa_manager->bo = bo;
- sa_manager->is_iomem = bo->vmap.is_iomem;
+ sa_manager->is_iomem = iosys_map_is_iomem(&bo->vmap);
sa_manager->gpu_addr = xe_bo_ggtt_addr(bo);
- if (bo->vmap.is_iomem) {
+ if (iosys_map_is_iomem(&bo->vmap)) {
sa_manager->cpu_ptr = kvzalloc(managed_size, GFP_KERNEL);
if (!sa_manager->cpu_ptr)
return ERR_PTR(-ENOMEM);
} else {
- sa_manager->cpu_ptr = bo->vmap.vaddr;
+ sa_manager->cpu_ptr = iosys_map_ptr(&bo->vmap);
memset(sa_manager->cpu_ptr, 0, bo->ttm.base.size);
}
@@ -116,7 +116,7 @@ void xe_sa_bo_flush_write(struct drm_suballoc *sa_bo)
struct xe_sa_manager *sa_manager = to_xe_sa_manager(sa_bo->manager);
struct xe_device *xe = tile_to_xe(sa_manager->bo->tile);
- if (!sa_manager->bo->vmap.is_iomem)
+ if (!iosys_map_is_iomem(&sa_manager->bo->vmap))
return;
xe_map_memcpy_to(xe, &sa_manager->bo->vmap, drm_suballoc_soffset(sa_bo),
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (3 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 13:39 ` Lucas De Marchi
2025-05-22 6:52 ` [PATCH 6/9] drm/ttm: " Dave Airlie
` (10 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This uses the new accessors to avoid touching the iosys_map internals.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
drivers/gpu/drm/qxl/qxl_draw.c | 4 ++--
drivers/gpu/drm/qxl/qxl_object.c | 8 ++++----
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 70aff64ced87..e833b0bbff47 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -602,16 +602,16 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
cursor.chunk.next_chunk = 0;
cursor.chunk.prev_chunk = 0;
cursor.chunk.data_size = size;
- if (cursor_map.is_iomem) {
- memcpy_toio(cursor_map.vaddr_iomem,
+ if (iosys_map_is_iomem(&cursor_map)) {
+ memcpy_toio(iosys_map_ioptr(&cursor_map),
&cursor, sizeof(cursor));
- memcpy_toio(cursor_map.vaddr_iomem + sizeof(cursor),
- user_map.vaddr, size);
+ memcpy_toio(iosys_map_ioptr(&cursor_map) + sizeof(cursor),
+ iosys_map_ptr(&user_map), size);
} else {
- memcpy(cursor_map.vaddr,
+ memcpy(iosys_map_ptr(&cursor_map),
&cursor, sizeof(cursor));
- memcpy(cursor_map.vaddr + sizeof(cursor),
- user_map.vaddr, size);
+ memcpy(iosys_map_ptr(&cursor_map) + sizeof(cursor),
+ iosys_map_ptr(&user_map), size);
}
qxl_bo_vunmap_and_unpin(user_bo);
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 3a3e127ce297..6000936bc8d0 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -52,7 +52,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev,
ret = qxl_bo_vmap_locked(clips_bo, &map);
if (ret)
return NULL;
- dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */
+ dev_clips = iosys_map_ptr(&map); /* TODO: Use mapping abstraction properly */
dev_clips->num_rects = num_clips;
dev_clips->chunk.next_chunk = 0;
@@ -206,7 +206,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
ret = qxl_bo_vmap_locked(bo, &surface_map);
if (ret)
goto out_release_backoff;
- surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */
+ surface_base = iosys_map_ptr(&surface_map); /* TODO: Use mapping abstraction properly */
ret = qxl_image_init(qdev, release, dimage, surface_base,
left - dumb_shadow_offset,
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 66635c55cf85..dcc1f6393885 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -172,10 +172,10 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
bo->map_count = 1;
/* TODO: Remove kptr in favor of map everywhere. */
- if (bo->map.is_iomem)
- bo->kptr = (void *)bo->map.vaddr_iomem;
+ if (iosys_map_is_iomem(&bo->map))
+ bo->kptr = (void *)iosys_map_ioptr(&bo->map);
else
- bo->kptr = bo->map.vaddr;
+ bo->kptr = iosys_map_ptr(&bo->map);
out:
*map = bo->map;
@@ -230,7 +230,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
ret = qxl_bo_vmap_locked(bo, &bo_map);
if (ret)
return NULL;
- rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */
+ rptr = iosys_map_ptr(&bo_map); /* TODO: Use mapping abstraction properly */
rptr += page_offset * PAGE_SIZE;
return rptr;
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 6/9] drm/ttm: avoid accessing iosys_map internals.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (4 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 6:52 ` [PATCH 7/9] drm: " Dave Airlie
` (9 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This uses the new accessors to use iosys_map.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/ttm/ttm_bo_util.c | 12 ++++++------
drivers/gpu/drm/ttm/ttm_resource.c | 8 ++++----
drivers/gpu/drm/ttm/ttm_tt.c | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 15cab9bda17f..ba7075fa9876 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -99,10 +99,10 @@ void ttm_move_memcpy(bool clear,
if (clear) {
for (i = 0; i < num_pages; ++i) {
dst_ops->map_local(dst_iter, &dst_map, i);
- if (dst_map.is_iomem)
- memset_io(dst_map.vaddr_iomem, 0, PAGE_SIZE);
+ if (iosys_map_is_iomem(&dst_map))
+ memset_io(iosys_map_ioptr(&dst_map), 0, PAGE_SIZE);
else
- memset(dst_map.vaddr, 0, PAGE_SIZE);
+ memset(iosys_map_ptr(&dst_map), 0, PAGE_SIZE);
if (dst_ops->unmap_local)
dst_ops->unmap_local(dst_iter, &dst_map);
}
@@ -544,10 +544,10 @@ void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct iosys_map *map)
if (iosys_map_is_null(map))
return;
- if (!map->is_iomem)
- vunmap(map->vaddr);
+ if (!iosys_map_is_iomem(map))
+ vunmap(iosys_map_ptr(map));
else if (!mem->bus.addr)
- iounmap(map->vaddr_iomem);
+ iounmap(iosys_map_ioptr(map));
iosys_map_clear(map);
ttm_mem_io_free(bo->bdev, bo->resource);
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 769b0ca9be47..0e7b4c6c16f8 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -742,7 +742,7 @@ static void ttm_kmap_iter_iomap_map_local(struct ttm_kmap_iter *iter,
static void ttm_kmap_iter_iomap_unmap_local(struct ttm_kmap_iter *iter,
struct iosys_map *map)
{
- io_mapping_unmap_local(map->vaddr_iomem);
+ io_mapping_unmap_local(iosys_map_ioptr(map));
}
static const struct ttm_kmap_iter_ops ttm_kmap_iter_io_ops = {
@@ -886,10 +886,10 @@ ttm_kmap_iter_linear_io_fini(struct ttm_kmap_iter_linear_io *iter_io,
struct ttm_resource *mem)
{
if (iter_io->needs_unmap && iosys_map_is_set(&iter_io->dmap)) {
- if (iter_io->dmap.is_iomem)
- iounmap(iter_io->dmap.vaddr_iomem);
+ if (iosys_map_is_iomem(&iter_io->dmap))
+ iounmap(iosys_map_ioptr(&iter_io->dmap));
else
- memunmap(iter_io->dmap.vaddr);
+ memunmap(iosys_map_ptr(&iter_io->dmap));
}
ttm_mem_io_free(bdev, mem);
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index df0aa6c4b8b8..ff9856e41b43 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -495,7 +495,7 @@ static void ttm_kmap_iter_tt_map_local(struct ttm_kmap_iter *iter,
static void ttm_kmap_iter_tt_unmap_local(struct ttm_kmap_iter *iter,
struct iosys_map *map)
{
- kunmap_local(map->vaddr);
+ kunmap_local(iosys_map_ptr(map));
}
static const struct ttm_kmap_iter_ops ttm_kmap_iter_tt_ops = {
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 7/9] drm: avoid accessing iosys_map internals.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (5 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 6/9] drm/ttm: " Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 6:52 ` [PATCH 8/9] iosys: hide internal details of implementation Dave Airlie
` (8 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This avoids directly accessing the iosys_map internals using new interfaces.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/drm_cache.c | 28 +++++++++----------
drivers/gpu/drm/drm_fbdev_shmem.c | 4 +--
drivers/gpu/drm/drm_format_helper.c | 16 +++++------
drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 2 +-
.../gpu/drm/tests/drm_format_helper_test.c | 26 ++++++++---------
5 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 7051c9c909c2..8945d32d26d8 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -215,12 +215,12 @@ static void memcpy_fallback(struct iosys_map *dst,
const struct iosys_map *src,
unsigned long len)
{
- if (!dst->is_iomem && !src->is_iomem) {
- memcpy(dst->vaddr, src->vaddr, len);
- } else if (!src->is_iomem) {
- iosys_map_memcpy_to(dst, 0, src->vaddr, len);
- } else if (!dst->is_iomem) {
- memcpy_fromio(dst->vaddr, src->vaddr_iomem, len);
+ if (!iosys_map_is_iomem(dst) && !iosys_map_is_iomem(src)) {
+ memcpy(iosys_map_ptr(dst), iosys_map_ptr(src), len);
+ } else if (iosys_map_is_iomem(src)) {
+ iosys_map_memcpy_to(dst, 0, iosys_map_ptr(src), len);
+ } else if (!iosys_map_is_iomem(dst)) {
+ memcpy_fromio(iosys_map_ptr(dst), iosys_map_ioptr(src), len);
} else {
/*
* Bounce size is not performance tuned, but using a
@@ -228,8 +228,8 @@ static void memcpy_fallback(struct iosys_map *dst,
* resorting to ioreadxx() + iowritexx().
*/
char bounce[MEMCPY_BOUNCE_SIZE];
- void __iomem *_src = src->vaddr_iomem;
- void __iomem *_dst = dst->vaddr_iomem;
+ void __iomem *_src = iosys_map_ioptr(src);
+ void __iomem *_dst = iosys_map_ioptr(dst);
while (len >= MEMCPY_BOUNCE_SIZE) {
memcpy_fromio(bounce, _src, MEMCPY_BOUNCE_SIZE);
@@ -312,12 +312,12 @@ void drm_memcpy_from_wc(struct iosys_map *dst,
}
if (static_branch_likely(&has_movntdqa)) {
- __drm_memcpy_from_wc(dst->is_iomem ?
- (void __force *)dst->vaddr_iomem :
- dst->vaddr,
- src->is_iomem ?
- (void const __force *)src->vaddr_iomem :
- src->vaddr,
+ __drm_memcpy_from_wc(iosys_map_is_iomem(dst) ?
+ (void __force *)iosys_map_ioptr(dst) :
+ iosys_map_ptr(dst),
+ iosys_map_is_iomem(src) ?
+ (void const __force *)iosys_map_ioptr(src) :
+ iosys_map_ptr(src),
len);
return;
}
diff --git a/drivers/gpu/drm/drm_fbdev_shmem.c b/drivers/gpu/drm/drm_fbdev_shmem.c
index f824369baacd..d45b6826138b 100644
--- a/drivers/gpu/drm/drm_fbdev_shmem.c
+++ b/drivers/gpu/drm/drm_fbdev_shmem.c
@@ -159,7 +159,7 @@ int drm_fbdev_shmem_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
ret = drm_client_buffer_vmap(buffer, &map);
if (ret) {
goto err_drm_client_buffer_delete;
- } else if (drm_WARN_ON(dev, map.is_iomem)) {
+ } else if (drm_WARN_ON(dev, iosys_map_is_iomem(&map))) {
ret = -ENODEV; /* I/O memory not supported; use generic emulation */
goto err_drm_client_buffer_delete;
}
@@ -183,7 +183,7 @@ int drm_fbdev_shmem_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
if (!shmem->map_wc)
info->flags |= FBINFO_READS_FAST; /* signal caching */
info->screen_size = sizes->surface_height * fb->pitches[0];
- info->screen_buffer = map.vaddr;
+ info->screen_buffer = iosys_map_ptr(&map);
info->fix.smem_len = info->screen_size;
/* deferred I/O */
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index d36e6cacc575..cccec04bec4e 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -236,13 +236,13 @@ static int drm_fb_xfrm(struct iosys_map *dst,
dst_pitch = default_dst_pitch;
/* TODO: handle src in I/O memory here */
- if (dst[0].is_iomem)
- return __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
- src[0].vaddr, fb, clip, vaddr_cached_hint, state,
+ if (iosys_map_is_iomem(&dst[0]))
+ return __drm_fb_xfrm_toio(iosys_map_ioptr(&dst[0]), dst_pitch[0], dst_pixsize[0],
+ iosys_map_ptr(&src[0]), fb, clip, vaddr_cached_hint, state,
xfrm_line);
else
- return __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
- src[0].vaddr, fb, clip, vaddr_cached_hint, state,
+ return __drm_fb_xfrm(iosys_map_ptr(&dst[0]), dst_pitch[0], dst_pixsize[0],
+ iosys_map_ptr(&src[0]), fb, clip, vaddr_cached_hint, state,
xfrm_line);
}
@@ -438,7 +438,7 @@ void drm_fb_memcpy(struct iosys_map *dst, const unsigned int *dst_pitch,
iosys_map_incr(&src_i, clip_offset(clip, fb->pitches[i], cpp_i));
for (y = 0; y < lines; y++) {
/* TODO: handle src_i in I/O memory here */
- iosys_map_memcpy_to(&dst_i, 0, src_i.vaddr, len_i);
+ iosys_map_memcpy_to(&dst_i, 0, iosys_map_ptr(&src_i), len_i);
iosys_map_incr(&src_i, fb->pitches[i]);
iosys_map_incr(&dst_i, dst_pitch_i);
}
@@ -1204,10 +1204,10 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
unsigned int cpp = fb->format->cpp[0];
unsigned int len_src32 = linepixels * cpp;
struct drm_device *dev = fb->dev;
- void *vaddr = src[0].vaddr;
+ void *vaddr = iosys_map_ptr(&src[0]);
unsigned int dst_pitch_0;
unsigned int y;
- u8 *mono = dst[0].vaddr, *gray8;
+ u8 *mono = iosys_map_ptr(&dst[0]), *gray8;
u32 *src32;
if (drm_WARN_ON(dev, fb->format->format != DRM_FORMAT_XRGB8888))
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
index ffaa2522ab96..0e1e1df22426 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
@@ -138,7 +138,7 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
struct drm_sysfb_device *sysfb = to_drm_sysfb_device(dev);
struct iosys_map dst = sysfb->fb_addr;
struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
- void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
+ void __iomem *dst_vmap = iosys_map_ioptr(&dst); /* TODO: Use mapping abstraction */
unsigned int dst_pitch = sysfb->fb_pitch;
const struct drm_format_info *dst_format = sysfb->fb_format;
struct drm_rect dst_clip;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 35cd3405d045..c714dc57d216 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -739,13 +739,13 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
drm_fb_xrgb8888_to_rgb565(&dst, &result->dst_pitch, &src, &fb, ¶ms->clip,
&fmtcnv_state, true);
buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected_swab, dst_size);
- buf = dst.vaddr;
+ buf = iosys_map_ptr(&dst);
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -792,7 +792,7 @@ static void drm_test_fb_xrgb8888_to_xrgb1555(struct kunit *test)
buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -839,7 +839,7 @@ static void drm_test_fb_xrgb8888_to_argb1555(struct kunit *test)
buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -886,7 +886,7 @@ static void drm_test_fb_xrgb8888_to_rgba5551(struct kunit *test)
buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -936,7 +936,7 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
drm_fb_xrgb8888_to_rgb888(&dst, dst_pitch, &src, &fb, ¶ms->clip, &fmtcnv_state);
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -982,7 +982,7 @@ static void drm_test_fb_xrgb8888_to_bgr888(struct kunit *test)
&fmtcnv_state);
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -1027,7 +1027,7 @@ static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -1074,7 +1074,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
buf = le32buf_to_cpu(test, buf, dst_size / sizeof(u32));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -1119,7 +1119,7 @@ static void drm_test_fb_xrgb8888_to_argb2101010(struct kunit *test)
buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result = 0;
@@ -1199,7 +1199,7 @@ static void drm_test_fb_swab(struct kunit *test)
buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr; /* restore original value of buf */
+ buf = iosys_map_ptr(&dst); /* restore original value of buf */
memset(buf, 0, dst_size);
int blit_result;
@@ -1211,7 +1211,7 @@ static void drm_test_fb_swab(struct kunit *test)
KUNIT_EXPECT_FALSE(test, blit_result);
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr;
+ buf = iosys_map_ptr(&dst);
memset(buf, 0, dst_size);
blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_BGRX8888, &src, &fb, ¶ms->clip,
@@ -1221,7 +1221,7 @@ static void drm_test_fb_swab(struct kunit *test)
KUNIT_EXPECT_FALSE(test, blit_result);
KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
- buf = dst.vaddr;
+ buf = iosys_map_ptr(&dst);
memset(buf, 0, dst_size);
struct drm_format_info mock_format = *fb.format;
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 8/9] iosys: hide internal details of implementation.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (6 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 7/9] drm: " Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 12:05 ` Thomas Zimmermann
2025-05-22 23:30 ` kernel test robot
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
` (7 subsequent siblings)
15 siblings, 2 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
Now hide the current implementation details, to catch any new
users entering the tree and trying to trick us up.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
include/linux/iosys-map.h | 48 +++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index 5ce5df1db60a..a6c2cc9ca756 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -108,25 +108,25 @@
*/
struct iosys_map {
union {
- void __iomem *vaddr_iomem;
- void *vaddr;
+ void __iomem *_vaddr_iomem;
+ void *_vaddr;
};
- bool is_iomem;
+ bool _is_iomem;
};
static inline bool iosys_map_is_iomem(const struct iosys_map *map)
{
- return map->is_iomem;
+ return map->_is_iomem;
}
static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
{
- return map->vaddr_iomem;
+ return map->_vaddr_iomem;
}
static inline void *iosys_map_ptr(const struct iosys_map *map)
{
- return map->vaddr;
+ return map->_vaddr;
}
/**
@@ -135,8 +135,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
*/
#define IOSYS_MAP_INIT_VADDR(vaddr_) \
{ \
- .vaddr = (vaddr_), \
- .is_iomem = false, \
+ ._vaddr = (vaddr_), \
+ ._is_iomem = false, \
}
/**
@@ -145,8 +145,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
*/
#define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
{ \
- .vaddr_iomem = (vaddr_iomem_), \
- .is_iomem = true, \
+ ._vaddr_iomem = (vaddr_iomem_), \
+ ._is_iomem = true, \
}
/**
@@ -197,8 +197,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
*/
static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
{
- map->vaddr = vaddr;
- map->is_iomem = false;
+ map->_vaddr = vaddr;
+ map->_is_iomem = false;
}
/**
@@ -211,8 +211,8 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
void __iomem *vaddr_iomem)
{
- map->vaddr_iomem = vaddr_iomem;
- map->is_iomem = true;
+ map->_vaddr_iomem = vaddr_iomem;
+ map->_is_iomem = true;
}
/**
@@ -229,12 +229,12 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
const struct iosys_map *rhs)
{
- if (lhs->is_iomem != rhs->is_iomem)
+ if (lhs->_is_iomem != rhs->_is_iomem)
return false;
- else if (lhs->is_iomem)
- return lhs->vaddr_iomem == rhs->vaddr_iomem;
+ else if (lhs->_is_iomem)
+ return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
else
- return lhs->vaddr == rhs->vaddr;
+ return lhs->_vaddr == rhs->_vaddr;
}
/**
@@ -279,11 +279,11 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
*/
static inline void iosys_map_clear(struct iosys_map *map)
{
- if (map->is_iomem) {
- map->vaddr_iomem = NULL;
- map->is_iomem = false;
+ if (map->_is_iomem) {
+ map->_vaddr_iomem = NULL;
+ map->_is_iomem = false;
} else {
- map->vaddr = NULL;
+ map->_vaddr = NULL;
}
}
@@ -338,9 +338,9 @@ static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
{
if (iosys_map_is_iomem(map))
- map->vaddr_iomem += incr;
+ map->_vaddr_iomem += incr;
else
- map->vaddr += incr;
+ map->_vaddr += incr;
}
/**
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (7 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 8/9] iosys: hide internal details of implementation Dave Airlie
@ 2025-05-22 6:52 ` Dave Airlie
2025-05-22 8:48 ` Jani Nikula
` (2 more replies)
2025-05-22 7:13 ` iosys-map: refactor to reduce struct size Dave Airlie
` (6 subsequent siblings)
15 siblings, 3 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 6:52 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
From: Dave Airlie <airlied@redhat.com>
This reduces this struct from 16 to 8 bytes, and it gets embedded
into a lot of things.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
include/linux/iosys-map.h | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index a6c2cc9ca756..44479966ce24 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -99,29 +99,27 @@
* iosys_map_incr(&map, len); // go to first byte after the memcpy
*/
+#define _IOSYS_MAP_IS_IOMEM 1
/**
* struct iosys_map - Pointer to IO/system memory
* @vaddr_iomem: The buffer's address if in I/O memory
* @vaddr: The buffer's address if in system memory
- * @is_iomem: True if the buffer is located in I/O memory, or false
- * otherwise.
*/
struct iosys_map {
union {
void __iomem *_vaddr_iomem;
void *_vaddr;
};
- bool _is_iomem;
};
static inline bool iosys_map_is_iomem(const struct iosys_map *map)
{
- return map->_is_iomem;
+ return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
}
static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
{
- return map->_vaddr_iomem;
+ return (void __iomem *)((unsigned long)map->_vaddr_iomem & ~_IOSYS_MAP_IS_IOMEM);
}
static inline void *iosys_map_ptr(const struct iosys_map *map)
@@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
#define IOSYS_MAP_INIT_VADDR(vaddr_) \
{ \
._vaddr = (vaddr_), \
- ._is_iomem = false, \
}
/**
@@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
*/
#define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
{ \
- ._vaddr_iomem = (vaddr_iomem_), \
- ._is_iomem = true, \
+ ._vaddr_iomem = (void __iomem *)(((unsigned long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
}
/**
@@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
{
map->_vaddr = vaddr;
- map->_is_iomem = false;
}
/**
@@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
void __iomem *vaddr_iomem)
{
- map->_vaddr_iomem = vaddr_iomem;
- map->_is_iomem = true;
+ map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem | _IOSYS_MAP_IS_IOMEM);
}
/**
@@ -229,12 +223,9 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
const struct iosys_map *rhs)
{
- if (lhs->_is_iomem != rhs->_is_iomem)
+ if (lhs->_vaddr != rhs->_vaddr)
return false;
- else if (lhs->_is_iomem)
- return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
- else
- return lhs->_vaddr == rhs->_vaddr;
+ return true;
}
/**
@@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
*/
static inline void iosys_map_clear(struct iosys_map *map)
{
- if (map->_is_iomem) {
- map->_vaddr_iomem = NULL;
- map->_is_iomem = false;
- } else {
- map->_vaddr = NULL;
- }
+ map->_vaddr = NULL;
}
/**
--
2.49.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: iosys-map: refactor to reduce struct size
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (8 preceding siblings ...)
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
@ 2025-05-22 7:13 ` Dave Airlie
2025-05-22 7:20 ` ✓ CI.Patch_applied: success for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally Patchwork
` (5 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 7:13 UTC (permalink / raw)
To: dri-devel, tzimmermann; +Cc: intel-xe
On Thu, 22 May 2025 at 16:57, Dave Airlie <airlied@gmail.com> wrote:
>
> Hey iosys_map users :)
>
> I fell down a bit of a refactor hole today, it was just random and
> sometimes you just have to let these things run their course.
I've put the tree here, and I realised I sent the patches, but when I
rebased onto latest drm-next, there were some new users.
https://github.com/airlied/linux/tree/iosys-map-rewrite
This just fixes up the new drm and sitronix users I found (or missed
earlier). There might still be others around.
Dave.
>
> I noticed iosys_map has a 7-byte hole in a 16-byte structure, and
> it gets embedded into a bunch of other structs and it offended my
> sensibilities.
>
> This series makes iosys_map be 8-bytes, using the bottom bit of
> the void * to store the is_iomem.
>
> Patch 1: adds new accessors to start hiding internals
> Patches 2-7: refactor all users in-tree to use new internals
> (hopefully got them all)
> Patch8: moves the internals around to catch anything not in-tree.
> Patch9: reimplements iosys_map as 8-bytes by hiding the is_iomem
> inside the pointer.
>
> Dave.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* ✓ CI.Patch_applied: success for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (9 preceding siblings ...)
2025-05-22 7:13 ` iosys-map: refactor to reduce struct size Dave Airlie
@ 2025-05-22 7:20 ` Patchwork
2025-05-22 7:20 ` ✗ CI.checkpatch: warning " Patchwork
` (4 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Patchwork @ 2025-05-22 7:20 UTC (permalink / raw)
To: Dave Airlie; +Cc: intel-xe
== Series Details ==
Series: series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
URL : https://patchwork.freedesktop.org/series/149367/
State : success
== Summary ==
=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 09130f929f7c drm-tip: 2025y-05m-22d-06h-26m-11s UTC integration manifest
=== git am output follows ===
Applying: iosys-map: add new accessor interfaces and use them internally.
Applying: udmabuf: use new iosys_map accessors.
Applying: firmware/tegra: avoid accessing iosys_map internals.
Applying: drm/xe: avoid accessing internals of iosys_map
Applying: drm/qxl: avoid accessing iosys_map internals.
Applying: drm/ttm: avoid accessing iosys_map internals.
Applying: drm: avoid accessing iosys_map internals.
Applying: iosys: hide internal details of implementation.
Applying: iosys_map: embed the is_iomem bit into the pointer.
^ permalink raw reply [flat|nested] 34+ messages in thread
* ✗ CI.checkpatch: warning for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (10 preceding siblings ...)
2025-05-22 7:20 ` ✓ CI.Patch_applied: success for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally Patchwork
@ 2025-05-22 7:20 ` Patchwork
2025-05-22 7:21 ` ✗ CI.KUnit: failure " Patchwork
` (3 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Patchwork @ 2025-05-22 7:20 UTC (permalink / raw)
To: Dave Airlie; +Cc: intel-xe
== Series Details ==
Series: series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
URL : https://patchwork.freedesktop.org/series/149367/
State : warning
== Summary ==
+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
202708c00696422fd217223bb679a353a5936e23
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit 8bc1c198eb5eb298ab50bcc4a30101489a29ea33
Author: Dave Airlie <airlied@redhat.com>
Date: Thu May 22 16:52:18 2025 +1000
iosys_map: embed the is_iomem bit into the pointer.
This reduces this struct from 16 to 8 bytes, and it gets embedded
into a lot of things.
Signed-off-by: Dave Airlie <airlied@redhat.com>
+ /mt/dim checkpatch 09130f929f7c79440cc8890bcce976843f439f01 drm-intel
93ca1ec74532 iosys-map: add new accessor interfaces and use them internally.
-:34: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#34: FILE: include/linux/iosys-map.h:129:
+ return map->vaddr;$
total: 0 errors, 1 warnings, 0 checks, 106 lines checked
646df55e4311 udmabuf: use new iosys_map accessors.
b856280fd799 firmware/tegra: avoid accessing iosys_map internals.
9dd7637632f3 drm/xe: avoid accessing internals of iosys_map
9bcf92ad70ef drm/qxl: avoid accessing iosys_map internals.
ae5ba27c421b drm/ttm: avoid accessing iosys_map internals.
e379757ce5a7 drm: avoid accessing iosys_map internals.
-:6: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#6:
This avoids directly accessing the iosys_map internals using new interfaces.
-:98: WARNING:LONG_LINE: line length of 101 exceeds 100 columns
#98: FILE: drivers/gpu/drm/drm_format_helper.c:241:
+ iosys_map_ptr(&src[0]), fb, clip, vaddr_cached_hint, state,
total: 0 errors, 2 warnings, 0 checks, 211 lines checked
0c3dba69aea2 iosys: hide internal details of implementation.
-:43: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#43: FILE: include/linux/iosys-map.h:129:
+ return map->_vaddr;$
total: 0 errors, 1 warnings, 0 checks, 113 lines checked
8bc1c198eb5e iosys_map: embed the is_iomem bit into the pointer.
-:62: WARNING:LONG_LINE: line length of 106 exceeds 100 columns
#62: FILE: include/linux/iosys-map.h:145:
+ ._vaddr_iomem = (void __iomem *)(((unsigned long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
total: 0 errors, 1 warnings, 0 checks, 91 lines checked
^ permalink raw reply [flat|nested] 34+ messages in thread
* ✗ CI.KUnit: failure for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (11 preceding siblings ...)
2025-05-22 7:20 ` ✗ CI.checkpatch: warning " Patchwork
@ 2025-05-22 7:21 ` Patchwork
2025-05-22 12:00 ` iosys-map: refactor to reduce struct size Thomas Zimmermann
` (2 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Patchwork @ 2025-05-22 7:21 UTC (permalink / raw)
To: Dave Airlie; +Cc: intel-xe
== Series Details ==
Series: series starting with [1/9] iosys-map: add new accessor interfaces and use them internally.
URL : https://patchwork.freedesktop.org/series/149367/
State : failure
== Summary ==
+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
[07:20:34] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[07:20:38] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=48
ERROR:root:../drivers/gpu/drm/drm_bridge.c:1406:6: error: redefinition of ‘devm_drm_put_bridge’
1406 | void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge)
| ^~~~~~~~~~~~~~~~~~~
In file included from ../drivers/gpu/drm/drm_bridge.c:31:
../include/drm/drm_bridge.h:1314:20: note: previous definition of ‘devm_drm_put_bridge’ with type ‘void(struct device *, struct drm_bridge *)’
1314 | static inline void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge) {}
| ^~~~~~~~~~~~~~~~~~~
make[6]: *** [../scripts/Makefile.build:203: drivers/gpu/drm/drm_bridge.o] Error 1
make[6]: *** Waiting for unfinished jobs....
../drivers/gpu/drm/xe/xe_lrc.c: In function ‘xe_lrc_setup_utilization’:
../drivers/gpu/drm/xe/xe_lrc.c:951:40: error: ‘struct iosys_map’ has no member named ‘vaddr’; did you mean ‘_vaddr’?
951 | cmd = lrc->bb_per_ctx_bo->vmap.vaddr;
| ^~~~~
| _vaddr
make[7]: *** [../scripts/Makefile.build:203: drivers/gpu/drm/xe/xe_lrc.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:461: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:461: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:461: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:461: drivers] Error 2
make[2]: *** [/kernel/Makefile:2003: .] Error 2
make[1]: *** [/kernel/Makefile:248: __sub-make] Error 2
make: *** [Makefile:248: __sub-make] Error 2
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
@ 2025-05-22 8:48 ` Jani Nikula
2025-05-22 12:10 ` Thomas Zimmermann
2025-05-22 15:09 ` Lucas De Marchi
2 siblings, 0 replies; 34+ messages in thread
From: Jani Nikula @ 2025-05-22 8:48 UTC (permalink / raw)
To: Dave Airlie, dri-devel, tzimmermann; +Cc: intel-xe
On Thu, 22 May 2025, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This reduces this struct from 16 to 8 bytes, and it gets embedded
> into a lot of things.
It's an abomination.
And i915 has helpers for it. Because of course it does.
i915_utils.h contains ptr_mask_bits(), ptr_unmask_bits(),
ptr_pack_bits() and ptr_unpack_bits() to do this, uh, cleanly.
Should we try to promote the helpers somewhere in include/linux and use
them here?
BR,
Jani.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> include/linux/iosys-map.h | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index a6c2cc9ca756..44479966ce24 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -99,29 +99,27 @@
> * iosys_map_incr(&map, len); // go to first byte after the memcpy
> */
>
> +#define _IOSYS_MAP_IS_IOMEM 1
> /**
> * struct iosys_map - Pointer to IO/system memory
> * @vaddr_iomem: The buffer's address if in I/O memory
> * @vaddr: The buffer's address if in system memory
> - * @is_iomem: True if the buffer is located in I/O memory, or false
> - * otherwise.
> */
> struct iosys_map {
> union {
> void __iomem *_vaddr_iomem;
> void *_vaddr;
> };
> - bool _is_iomem;
> };
>
> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
> {
> - return map->_is_iomem;
> + return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
> }
>
> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
> {
> - return map->_vaddr_iomem;
> + return (void __iomem *)((unsigned long)map->_vaddr_iomem & ~_IOSYS_MAP_IS_IOMEM);
> }
>
> static inline void *iosys_map_ptr(const struct iosys_map *map)
> @@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> #define IOSYS_MAP_INIT_VADDR(vaddr_) \
> { \
> ._vaddr = (vaddr_), \
> - ._is_iomem = false, \
> }
>
> /**
> @@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
> { \
> - ._vaddr_iomem = (vaddr_iomem_), \
> - ._is_iomem = true, \
> + ._vaddr_iomem = (void __iomem *)(((unsigned long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
> }
>
> /**
> @@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> {
> map->_vaddr = vaddr;
> - map->_is_iomem = false;
> }
>
> /**
> @@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> void __iomem *vaddr_iomem)
> {
> - map->_vaddr_iomem = vaddr_iomem;
> - map->_is_iomem = true;
> + map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem | _IOSYS_MAP_IS_IOMEM);
> }
>
> /**
> @@ -229,12 +223,9 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> const struct iosys_map *rhs)
> {
> - if (lhs->_is_iomem != rhs->_is_iomem)
> + if (lhs->_vaddr != rhs->_vaddr)
> return false;
> - else if (lhs->_is_iomem)
> - return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
> - else
> - return lhs->_vaddr == rhs->_vaddr;
> + return true;
> }
>
> /**
> @@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
> */
> static inline void iosys_map_clear(struct iosys_map *map)
> {
> - if (map->_is_iomem) {
> - map->_vaddr_iomem = NULL;
> - map->_is_iomem = false;
> - } else {
> - map->_vaddr = NULL;
> - }
> + map->_vaddr = NULL;
> }
>
> /**
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 6:52 ` [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally Dave Airlie
@ 2025-05-22 11:58 ` Thomas Zimmermann
2025-05-22 13:34 ` Lucas De Marchi
0 siblings, 1 reply; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-22 11:58 UTC (permalink / raw)
To: Dave Airlie, dri-devel; +Cc: intel-xe
Am 22.05.25 um 08:52 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This adds accessors inlines to the iosys-map. The intent is to
> roll the iomem flag into the lower bits of the vaddr eventually.
>
> First just add accessors to move all current in-tree users over to.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> include/linux/iosys-map.h | 53 +++++++++++++++++++++++++--------------
> 1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index 4696abfd311c..5ce5df1db60a 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -114,6 +114,21 @@ struct iosys_map {
> bool is_iomem;
> };
>
> +static inline bool iosys_map_is_iomem(const struct iosys_map *map)
> +{
> + return map->is_iomem;
> +}
> +
> +static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
> +{
> + return map->vaddr_iomem;
> +}
> +
> +static inline void *iosys_map_ptr(const struct iosys_map *map)
> +{
> + return map->vaddr;
> +}
> +
These helpers need documentation.
We should encourage users to the other helpers for interacting with
iosys-map structures instead of decoding them manually. OTOH there are
cases where decoding them by hand is clearly better. I'd suggest to
prefix the new helpers with __ so mark them an internal/special.
Best regards
Thomas
> /**
> * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address in system memory
> * @vaddr_: A system-memory address
> @@ -234,9 +249,9 @@ static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> */
> static inline bool iosys_map_is_null(const struct iosys_map *map)
> {
> - if (map->is_iomem)
> - return !map->vaddr_iomem;
> - return !map->vaddr;
> + if (iosys_map_is_iomem(map))
> + return !iosys_map_ioptr(map);
> + return !iosys_map_ptr(map);
> }
>
> /**
> @@ -286,10 +301,10 @@ static inline void iosys_map_clear(struct iosys_map *map)
> static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
> const void *src, size_t len)
> {
> - if (dst->is_iomem)
> - memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
> + if (iosys_map_is_iomem(dst))
> + memcpy_toio(iosys_map_ioptr(dst) + dst_offset, src, len);
> else
> - memcpy(dst->vaddr + dst_offset, src, len);
> + memcpy(iosys_map_ptr(dst) + dst_offset, src, len);
> }
>
> /**
> @@ -306,10 +321,10 @@ static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
> static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
> size_t src_offset, size_t len)
> {
> - if (src->is_iomem)
> - memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
> + if (iosys_map_is_iomem(src))
> + memcpy_fromio(dst, iosys_map_ioptr(src) + src_offset, len);
> else
> - memcpy(dst, src->vaddr + src_offset, len);
> + memcpy(dst, iosys_map_ptr(src) + src_offset, len);
> }
>
> /**
> @@ -322,7 +337,7 @@ static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
> */
> static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
> {
> - if (map->is_iomem)
> + if (iosys_map_is_iomem(map))
> map->vaddr_iomem += incr;
> else
> map->vaddr += incr;
> @@ -341,10 +356,10 @@ static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
> static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
> int value, size_t len)
> {
> - if (dst->is_iomem)
> - memset_io(dst->vaddr_iomem + offset, value, len);
> + if (iosys_map_is_iomem(dst))
> + memset_io(iosys_map_ioptr(dst) + offset, value, len);
> else
> - memset(dst->vaddr + offset, value, len);
> + memset(iosys_map_ptr(dst) + offset, value, len);
> }
>
> #ifdef CONFIG_64BIT
> @@ -393,10 +408,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
> */
> #define iosys_map_rd(map__, offset__, type__) ({ \
> type__ val_; \
> - if ((map__)->is_iomem) { \
> - __iosys_map_rd_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
> + if (iosys_map_is_iomem(map__)) { \
> + __iosys_map_rd_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
> } else { \
> - __iosys_map_rd_sys(val_, (map__)->vaddr + (offset__), type__); \
> + __iosys_map_rd_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
> } \
> val_; \
> })
> @@ -415,10 +430,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
> */
> #define iosys_map_wr(map__, offset__, type__, val__) ({ \
> type__ val_ = (val__); \
> - if ((map__)->is_iomem) { \
> - __iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
> + if (iosys_map_is_iomem(map__)) { \
> + __iosys_map_wr_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
> } else { \
> - __iosys_map_wr_sys(val_, (map__)->vaddr + (offset__), type__); \
> + __iosys_map_wr_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
> } \
> })
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: iosys-map: refactor to reduce struct size
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (12 preceding siblings ...)
2025-05-22 7:21 ` ✗ CI.KUnit: failure " Patchwork
@ 2025-05-22 12:00 ` Thomas Zimmermann
2025-05-22 13:59 ` Lucas De Marchi
2025-06-27 17:31 ` Lucas De Marchi
15 siblings, 0 replies; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-22 12:00 UTC (permalink / raw)
To: Dave Airlie, dri-devel, Lucas De Marchi; +Cc: intel-xe
cc'ing Lucas, who should also take a look.
Am 22.05.25 um 08:52 schrieb Dave Airlie:
> Hey iosys_map users :)
>
> I fell down a bit of a refactor hole today, it was just random and
> sometimes you just have to let these things run their course.
>
> I noticed iosys_map has a 7-byte hole in a 16-byte structure, and
> it gets embedded into a bunch of other structs and it offended my
> sensibilities.
>
> This series makes iosys_map be 8-bytes, using the bottom bit of
> the void * to store the is_iomem.
>
> Patch 1: adds new accessors to start hiding internals
> Patches 2-7: refactor all users in-tree to use new internals
> (hopefully got them all)
> Patch8: moves the internals around to catch anything not in-tree.
> Patch9: reimplements iosys_map as 8-bytes by hiding the is_iomem
> inside the pointer.
>
> Dave.
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] iosys: hide internal details of implementation.
2025-05-22 6:52 ` [PATCH 8/9] iosys: hide internal details of implementation Dave Airlie
@ 2025-05-22 12:05 ` Thomas Zimmermann
2025-05-22 23:30 ` kernel test robot
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-22 12:05 UTC (permalink / raw)
To: Dave Airlie, dri-devel; +Cc: intel-xe
Hi
Am 22.05.25 um 08:52 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> Now hide the current implementation details, to catch any new
> users entering the tree and trying to trick us up.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> include/linux/iosys-map.h | 48 +++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index 5ce5df1db60a..a6c2cc9ca756 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -108,25 +108,25 @@
> */
> struct iosys_map {
Instead of renaming, can we simply add a /* private: */ comment like in
[1]? This could be done in patch 1.
Best regards
Thomas
[1]
https://elixir.bootlin.com/linux/v6.14.7/source/include/drm/drm_mm.h#L164
> union {
> - void __iomem *vaddr_iomem;
> - void *vaddr;
> + void __iomem *_vaddr_iomem;
> + void *_vaddr;
> };
> - bool is_iomem;
> + bool _is_iomem;
> };
>
> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
> {
> - return map->is_iomem;
> + return map->_is_iomem;
> }
>
> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
> {
> - return map->vaddr_iomem;
> + return map->_vaddr_iomem;
> }
>
> static inline void *iosys_map_ptr(const struct iosys_map *map)
> {
> - return map->vaddr;
> + return map->_vaddr;
> }
>
> /**
> @@ -135,8 +135,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> #define IOSYS_MAP_INIT_VADDR(vaddr_) \
> { \
> - .vaddr = (vaddr_), \
> - .is_iomem = false, \
> + ._vaddr = (vaddr_), \
> + ._is_iomem = false, \
> }
>
> /**
> @@ -145,8 +145,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
> { \
> - .vaddr_iomem = (vaddr_iomem_), \
> - .is_iomem = true, \
> + ._vaddr_iomem = (vaddr_iomem_), \
> + ._is_iomem = true, \
> }
>
> /**
> @@ -197,8 +197,8 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> {
> - map->vaddr = vaddr;
> - map->is_iomem = false;
> + map->_vaddr = vaddr;
> + map->_is_iomem = false;
> }
>
> /**
> @@ -211,8 +211,8 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> void __iomem *vaddr_iomem)
> {
> - map->vaddr_iomem = vaddr_iomem;
> - map->is_iomem = true;
> + map->_vaddr_iomem = vaddr_iomem;
> + map->_is_iomem = true;
> }
>
> /**
> @@ -229,12 +229,12 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> const struct iosys_map *rhs)
> {
> - if (lhs->is_iomem != rhs->is_iomem)
> + if (lhs->_is_iomem != rhs->_is_iomem)
> return false;
> - else if (lhs->is_iomem)
> - return lhs->vaddr_iomem == rhs->vaddr_iomem;
> + else if (lhs->_is_iomem)
> + return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
> else
> - return lhs->vaddr == rhs->vaddr;
> + return lhs->_vaddr == rhs->_vaddr;
> }
>
> /**
> @@ -279,11 +279,11 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
> */
> static inline void iosys_map_clear(struct iosys_map *map)
> {
> - if (map->is_iomem) {
> - map->vaddr_iomem = NULL;
> - map->is_iomem = false;
> + if (map->_is_iomem) {
> + map->_vaddr_iomem = NULL;
> + map->_is_iomem = false;
> } else {
> - map->vaddr = NULL;
> + map->_vaddr = NULL;
> }
> }
>
> @@ -338,9 +338,9 @@ static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
> static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
> {
> if (iosys_map_is_iomem(map))
> - map->vaddr_iomem += incr;
> + map->_vaddr_iomem += incr;
> else
> - map->vaddr += incr;
> + map->_vaddr += incr;
> }
>
> /**
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
2025-05-22 8:48 ` Jani Nikula
@ 2025-05-22 12:10 ` Thomas Zimmermann
2025-05-22 15:09 ` Lucas De Marchi
2 siblings, 0 replies; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-22 12:10 UTC (permalink / raw)
To: Dave Airlie, dri-devel; +Cc: intel-xe
Hi
Am 22.05.25 um 08:52 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This reduces this struct from 16 to 8 bytes, and it gets embedded
> into a lot of things.
IIRC this has been discussed before. Makes sense to me.
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> include/linux/iosys-map.h | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index a6c2cc9ca756..44479966ce24 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -99,29 +99,27 @@
> * iosys_map_incr(&map, len); // go to first byte after the memcpy
> */
>
> +#define _IOSYS_MAP_IS_IOMEM 1
Two underscores would be preferable to me. And please use the BIT()
macro to define this constant.
Best regards
Thomas
> /**
> * struct iosys_map - Pointer to IO/system memory
> * @vaddr_iomem: The buffer's address if in I/O memory
> * @vaddr: The buffer's address if in system memory
> - * @is_iomem: True if the buffer is located in I/O memory, or false
> - * otherwise.
> */
> struct iosys_map {
> union {
> void __iomem *_vaddr_iomem;
> void *_vaddr;
> };
> - bool _is_iomem;
> };
>
> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
> {
> - return map->_is_iomem;
> + return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
> }
>
> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
> {
> - return map->_vaddr_iomem;
> + return (void __iomem *)((unsigned long)map->_vaddr_iomem & ~_IOSYS_MAP_IS_IOMEM);
Macros for encoding and decoding the pointer+bit would be preferred.
Jani seems to have helpers for that.
Best regards
Thomas
> }
>
> static inline void *iosys_map_ptr(const struct iosys_map *map)
> @@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> #define IOSYS_MAP_INIT_VADDR(vaddr_) \
> { \
> ._vaddr = (vaddr_), \
> - ._is_iomem = false, \
> }
>
> /**
> @@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
> { \
> - ._vaddr_iomem = (vaddr_iomem_), \
> - ._is_iomem = true, \
> + ._vaddr_iomem = (void __iomem *)(((unsigned long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
> }
>
> /**
> @@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> {
> map->_vaddr = vaddr;
> - map->_is_iomem = false;
> }
>
> /**
> @@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> void __iomem *vaddr_iomem)
> {
> - map->_vaddr_iomem = vaddr_iomem;
> - map->_is_iomem = true;
> + map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem | _IOSYS_MAP_IS_IOMEM);
> }
>
> /**
> @@ -229,12 +223,9 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> const struct iosys_map *rhs)
> {
> - if (lhs->_is_iomem != rhs->_is_iomem)
> + if (lhs->_vaddr != rhs->_vaddr)
> return false;
> - else if (lhs->_is_iomem)
> - return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
> - else
> - return lhs->_vaddr == rhs->_vaddr;
> + return true;
> }
>
> /**
> @@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
> */
> static inline void iosys_map_clear(struct iosys_map *map)
> {
> - if (map->_is_iomem) {
> - map->_vaddr_iomem = NULL;
> - map->_is_iomem = false;
> - } else {
> - map->_vaddr = NULL;
> - }
> + map->_vaddr = NULL;
> }
>
> /**
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 11:58 ` Thomas Zimmermann
@ 2025-05-22 13:34 ` Lucas De Marchi
2025-05-22 14:00 ` Thomas Zimmermann
0 siblings, 1 reply; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 13:34 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: Dave Airlie, dri-devel, intel-xe
On Thu, May 22, 2025 at 01:58:54PM +0200, Thomas Zimmermann wrote:
>
>
>Am 22.05.25 um 08:52 schrieb Dave Airlie:
>>From: Dave Airlie <airlied@redhat.com>
>>
>>This adds accessors inlines to the iosys-map. The intent is to
>>roll the iomem flag into the lower bits of the vaddr eventually.
>>
>>First just add accessors to move all current in-tree users over to.
>>
>>Signed-off-by: Dave Airlie <airlied@redhat.com>
>>---
>> include/linux/iosys-map.h | 53 +++++++++++++++++++++++++--------------
>> 1 file changed, 34 insertions(+), 19 deletions(-)
>>
>>diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>>index 4696abfd311c..5ce5df1db60a 100644
>>--- a/include/linux/iosys-map.h
>>+++ b/include/linux/iosys-map.h
>>@@ -114,6 +114,21 @@ struct iosys_map {
>> bool is_iomem;
>> };
>>+static inline bool iosys_map_is_iomem(const struct iosys_map *map)
>>+{
>>+ return map->is_iomem;
>>+}
>>+
>>+static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
>>+{
>>+ return map->vaddr_iomem;
>>+}
>>+
>>+static inline void *iosys_map_ptr(const struct iosys_map *map)
>>+{
>>+ return map->vaddr;
>>+}
>>+
>
>These helpers need documentation.
agreed
>
>We should encourage users to the other helpers for interacting with
>iosys-map structures instead of decoding them manually. OTOH there are
>cases where decoding them by hand is clearly better. I'd suggest to
>prefix the new helpers with __ so mark them an internal/special.
From the other patches there are quite a few cases that would be using
"internal" API. From those there are just a few cases in which we'd
have a direct translation to existing API... so I wouldn't make this
internal when they are clearly needed externally.
Lucas De Marchi
>
>Best regards
>Thomas
>
>> /**
>> * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address in system memory
>> * @vaddr_: A system-memory address
>>@@ -234,9 +249,9 @@ static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
>> */
>> static inline bool iosys_map_is_null(const struct iosys_map *map)
>> {
>>- if (map->is_iomem)
>>- return !map->vaddr_iomem;
>>- return !map->vaddr;
>>+ if (iosys_map_is_iomem(map))
>>+ return !iosys_map_ioptr(map);
>>+ return !iosys_map_ptr(map);
>> }
>> /**
>>@@ -286,10 +301,10 @@ static inline void iosys_map_clear(struct iosys_map *map)
>> static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
>> const void *src, size_t len)
>> {
>>- if (dst->is_iomem)
>>- memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
>>+ if (iosys_map_is_iomem(dst))
>>+ memcpy_toio(iosys_map_ioptr(dst) + dst_offset, src, len);
>> else
>>- memcpy(dst->vaddr + dst_offset, src, len);
>>+ memcpy(iosys_map_ptr(dst) + dst_offset, src, len);
>> }
>> /**
>>@@ -306,10 +321,10 @@ static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
>> static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
>> size_t src_offset, size_t len)
>> {
>>- if (src->is_iomem)
>>- memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
>>+ if (iosys_map_is_iomem(src))
>>+ memcpy_fromio(dst, iosys_map_ioptr(src) + src_offset, len);
>> else
>>- memcpy(dst, src->vaddr + src_offset, len);
>>+ memcpy(dst, iosys_map_ptr(src) + src_offset, len);
>> }
>> /**
>>@@ -322,7 +337,7 @@ static inline void iosys_map_memcpy_from(void *dst, const struct iosys_map *src,
>> */
>> static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
>> {
>>- if (map->is_iomem)
>>+ if (iosys_map_is_iomem(map))
>> map->vaddr_iomem += incr;
>> else
>> map->vaddr += incr;
>>@@ -341,10 +356,10 @@ static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
>> static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
>> int value, size_t len)
>> {
>>- if (dst->is_iomem)
>>- memset_io(dst->vaddr_iomem + offset, value, len);
>>+ if (iosys_map_is_iomem(dst))
>>+ memset_io(iosys_map_ioptr(dst) + offset, value, len);
>> else
>>- memset(dst->vaddr + offset, value, len);
>>+ memset(iosys_map_ptr(dst) + offset, value, len);
>> }
>> #ifdef CONFIG_64BIT
>>@@ -393,10 +408,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
>> */
>> #define iosys_map_rd(map__, offset__, type__) ({ \
>> type__ val_; \
>>- if ((map__)->is_iomem) { \
>>- __iosys_map_rd_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
>>+ if (iosys_map_is_iomem(map__)) { \
>>+ __iosys_map_rd_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
>> } else { \
>>- __iosys_map_rd_sys(val_, (map__)->vaddr + (offset__), type__); \
>>+ __iosys_map_rd_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
>> } \
>> val_; \
>> })
>>@@ -415,10 +430,10 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
>> */
>> #define iosys_map_wr(map__, offset__, type__, val__) ({ \
>> type__ val_ = (val__); \
>>- if ((map__)->is_iomem) { \
>>- __iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__), type__); \
>>+ if (iosys_map_is_iomem(map__)) { \
>>+ __iosys_map_wr_io(val_, iosys_map_ioptr(map__) + (offset__), type__); \
>> } else { \
>>- __iosys_map_wr_sys(val_, (map__)->vaddr + (offset__), type__); \
>>+ __iosys_map_wr_sys(val_, iosys_map_ptr(map__) + (offset__), type__); \
>> } \
>> })
>
>--
>--
>Thomas Zimmermann
>Graphics Driver Developer
>SUSE Software Solutions Germany GmbH
>Frankenstrasse 146, 90461 Nuernberg, Germany
>GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>HRB 36809 (AG Nuernberg)
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals.
2025-05-22 6:52 ` [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals Dave Airlie
@ 2025-05-22 13:39 ` Lucas De Marchi
0 siblings, 0 replies; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 13:39 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe
On Thu, May 22, 2025 at 04:52:14PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airlied@redhat.com>
>
>This uses the new accessors to avoid touching the iosys_map internals.
>
>Signed-off-by: Dave Airlie <airlied@redhat.com>
>---
> drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
> drivers/gpu/drm/qxl/qxl_draw.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_object.c | 8 ++++----
> 3 files changed, 13 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
>index 70aff64ced87..e833b0bbff47 100644
>--- a/drivers/gpu/drm/qxl/qxl_display.c
>+++ b/drivers/gpu/drm/qxl/qxl_display.c
>@@ -602,16 +602,16 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
> cursor.chunk.next_chunk = 0;
> cursor.chunk.prev_chunk = 0;
> cursor.chunk.data_size = size;
>- if (cursor_map.is_iomem) {
>- memcpy_toio(cursor_map.vaddr_iomem,
>+ if (iosys_map_is_iomem(&cursor_map)) {
>+ memcpy_toio(iosys_map_ioptr(&cursor_map),
> &cursor, sizeof(cursor));
>- memcpy_toio(cursor_map.vaddr_iomem + sizeof(cursor),
>- user_map.vaddr, size);
>+ memcpy_toio(iosys_map_ioptr(&cursor_map) + sizeof(cursor),
>+ iosys_map_ptr(&user_map), size);
> } else {
>- memcpy(cursor_map.vaddr,
>+ memcpy(iosys_map_ptr(&cursor_map),
> &cursor, sizeof(cursor));
>- memcpy(cursor_map.vaddr + sizeof(cursor),
>- user_map.vaddr, size);
>+ memcpy(iosys_map_ptr(&cursor_map) + sizeof(cursor),
>+ iosys_map_ptr(&user_map), size);
these would better use iosys_map_memcpy_*, but could be a follow up to
this (automated?) conversion.
> }
>
> qxl_bo_vunmap_and_unpin(user_bo);
>diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
>index 3a3e127ce297..6000936bc8d0 100644
>--- a/drivers/gpu/drm/qxl/qxl_draw.c
>+++ b/drivers/gpu/drm/qxl/qxl_draw.c
>@@ -52,7 +52,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev,
> ret = qxl_bo_vmap_locked(clips_bo, &map);
> if (ret)
> return NULL;
>- dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */
>+ dev_clips = iosys_map_ptr(&map); /* TODO: Use mapping abstraction properly */
>
> dev_clips->num_rects = num_clips;
> dev_clips->chunk.next_chunk = 0;
>@@ -206,7 +206,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
> ret = qxl_bo_vmap_locked(bo, &surface_map);
> if (ret)
> goto out_release_backoff;
>- surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */
>+ surface_base = iosys_map_ptr(&surface_map); /* TODO: Use mapping abstraction properly */
>
> ret = qxl_image_init(qdev, release, dimage, surface_base,
> left - dumb_shadow_offset,
>diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
>index 66635c55cf85..dcc1f6393885 100644
>--- a/drivers/gpu/drm/qxl/qxl_object.c
>+++ b/drivers/gpu/drm/qxl/qxl_object.c
>@@ -172,10 +172,10 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
> bo->map_count = 1;
>
> /* TODO: Remove kptr in favor of map everywhere. */
>- if (bo->map.is_iomem)
>- bo->kptr = (void *)bo->map.vaddr_iomem;
>+ if (iosys_map_is_iomem(&bo->map))
>+ bo->kptr = (void *)iosys_map_ioptr(&bo->map);
this looks wrong as we then lose the __iomem. Or is this dead code and
it's never iomem? Anyway, pre-existent issue unrelated to this patch.
Lucas De Marchi
> else
>- bo->kptr = bo->map.vaddr;
>+ bo->kptr = iosys_map_ptr(&bo->map);
>
> out:
> *map = bo->map;
>@@ -230,7 +230,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> ret = qxl_bo_vmap_locked(bo, &bo_map);
> if (ret)
> return NULL;
>- rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */
>+ rptr = iosys_map_ptr(&bo_map); /* TODO: Use mapping abstraction properly */
>
> rptr += page_offset * PAGE_SIZE;
> return rptr;
>--
>2.49.0
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map
2025-05-22 6:52 ` [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map Dave Airlie
@ 2025-05-22 13:56 ` Lucas De Marchi
0 siblings, 0 replies; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 13:56 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe
On Thu, May 22, 2025 at 04:52:13PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airlied@redhat.com>
>
>This uses the new accessors to avoid touch iosys_map internals.
>
>Signed-off-by: Dave Airlie <airlied@redhat.com>
>---
> drivers/gpu/drm/xe/display/intel_fbdev_fb.c | 2 +-
> drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
> drivers/gpu/drm/xe/xe_eu_stall.c | 2 +-
> drivers/gpu/drm/xe/xe_guc_pc.c | 2 +-
> drivers/gpu/drm/xe/xe_map.h | 12 ++++++------
> drivers/gpu/drm/xe/xe_memirq.c | 16 ++++++++--------
> drivers/gpu/drm/xe/xe_oa.c | 4 ++--
> drivers/gpu/drm/xe/xe_pt.c | 4 ++--
> drivers/gpu/drm/xe/xe_sa.c | 8 ++++----
> 9 files changed, 29 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>index e8191562d122..ad2681c90efb 100644
>--- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>+++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>@@ -101,7 +101,7 @@ int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info
> }
> XE_WARN_ON(iosys_map_is_null(&obj->vmap));
>
>- info->screen_base = obj->vmap.vaddr_iomem;
>+ info->screen_base = iosys_map_ioptr(&obj->vmap);
> info->screen_size = obj->ttm.base.size;
>
> return 0;
>diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>index d99d91fe8aa9..c83a54708495 100644
>--- a/drivers/gpu/drm/xe/xe_bo.c
>+++ b/drivers/gpu/drm/xe/xe_bo.c
>@@ -1249,7 +1249,7 @@ int xe_bo_evict_pinned(struct xe_bo *bo)
> unmap = true;
> }
>
>- xe_map_memcpy_from(xe, backup->vmap.vaddr, &bo->vmap, 0,
>+ xe_map_memcpy_from(xe, iosys_map_ptr(&backup->vmap), &bo->vmap, 0,
> bo->size);
> }
>
>@@ -1342,7 +1342,7 @@ int xe_bo_restore_pinned(struct xe_bo *bo)
> unmap = true;
> }
>
>- xe_map_memcpy_to(xe, &bo->vmap, 0, backup->vmap.vaddr,
>+ xe_map_memcpy_to(xe, &bo->vmap, 0, iosys_map_ptr(&backup->vmap),
> bo->size);
> }
>
>@@ -2226,9 +2226,9 @@ int xe_managed_bo_reinit_in_vram(struct xe_device *xe, struct xe_tile *tile, str
> XE_BO_FLAG_PINNED_NORESTORE);
>
> xe_assert(xe, IS_DGFX(xe));
>- xe_assert(xe, !(*src)->vmap.is_iomem);
>+ xe_assert(xe, !iosys_map_is_iomem(&(*src)->vmap));
>
>- bo = xe_managed_bo_create_from_data(xe, tile, (*src)->vmap.vaddr,
>+ bo = xe_managed_bo_create_from_data(xe, tile, iosys_map_ptr(&(*src)->vmap),
> (*src)->size, dst_flags);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
>index 96732613b4b7..d8f900efac95 100644
>--- a/drivers/gpu/drm/xe/xe_eu_stall.c
>+++ b/drivers/gpu/drm/xe/xe_eu_stall.c
>@@ -741,7 +741,7 @@ static int xe_eu_stall_stream_init(struct xe_eu_stall_data_stream *stream,
> for_each_dss_steering(xecore, gt, group, instance) {
> xecore_buf = &stream->xecore_buf[xecore];
> vaddr_offset = xecore * stream->per_xecore_buf_size;
>- xecore_buf->vaddr = stream->bo->vmap.vaddr + vaddr_offset;
>+ xecore_buf->vaddr = iosys_map_ptr(&stream->bo->vmap) + vaddr_offset;
> }
> return 0;
> }
>diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>index 18c623992035..c7ad56774c99 100644
>--- a/drivers/gpu/drm/xe/xe_guc_pc.c
>+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>@@ -1068,7 +1068,7 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
> goto out;
> }
>
>- memset(pc->bo->vmap.vaddr, 0, size);
>+ memset(iosys_map_ptr(&pc->bo->vmap), 0, size);
this is wrong, we need to move it to iosys_map_memset().
> slpc_shared_data_write(pc, header.size, size);
>
> earlier = ktime_get();
>diff --git a/drivers/gpu/drm/xe/xe_map.h b/drivers/gpu/drm/xe/xe_map.h
>index f62e0c8b67ab..37842c02c7f9 100644
>--- a/drivers/gpu/drm/xe/xe_map.h
>+++ b/drivers/gpu/drm/xe/xe_map.h
>@@ -49,10 +49,10 @@ static inline u32 xe_map_read32(struct xe_device *xe, struct iosys_map *map)
> {
> xe_device_assert_mem_access(xe);
>
>- if (map->is_iomem)
>- return readl(map->vaddr_iomem);
>+ if (iosys_map_is_iomem(map))
>+ return readl(iosys_map_ioptr(map));
> else
>- return READ_ONCE(*(u32 *)map->vaddr);
>+ return READ_ONCE(*(u32 *)iosys_map_ptr(map));
we added this because of the mem_access. But I have no idea why exactly
this is hand rolling the read.
It seems we'd benefit from also having fixed-types (at least u32) in
iosys_map so drivers don't seem tempted to do this.
Lucas De Marchi
> }
>
> static inline void xe_map_write32(struct xe_device *xe, struct iosys_map *map,
>@@ -60,10 +60,10 @@ static inline void xe_map_write32(struct xe_device *xe, struct iosys_map *map,
> {
> xe_device_assert_mem_access(xe);
>
>- if (map->is_iomem)
>- writel(val, map->vaddr_iomem);
>+ if (iosys_map_is_iomem(map))
>+ writel(val, iosys_map_ioptr(map));
> else
>- *(u32 *)map->vaddr = val;
>+ *(u32 *)iosys_map_ptr(map) = val;
> }
>
> #define xe_map_rd(xe__, map__, offset__, type__) ({ \
>diff --git a/drivers/gpu/drm/xe/xe_memirq.c b/drivers/gpu/drm/xe/xe_memirq.c
>index 49c45ec3e83c..458955c75e04 100644
>--- a/drivers/gpu/drm/xe/xe_memirq.c
>+++ b/drivers/gpu/drm/xe/xe_memirq.c
>@@ -198,9 +198,9 @@ static int memirq_alloc_pages(struct xe_memirq *memirq)
> memirq->status = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_STATUS_OFFSET(0));
> memirq->mask = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_ENABLE_OFFSET);
>
>- memirq_assert(memirq, !memirq->source.is_iomem);
>- memirq_assert(memirq, !memirq->status.is_iomem);
>- memirq_assert(memirq, !memirq->mask.is_iomem);
>+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->source));
>+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->status));
>+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->mask));
>
> memirq_debug(memirq, "page offsets: bo %#x bo_size %zu source %#x status %#x\n",
> xe_bo_ggtt_addr(bo), bo_size, XE_MEMIRQ_SOURCE_OFFSET(0),
>@@ -418,7 +418,7 @@ static bool memirq_received(struct xe_memirq *memirq, struct iosys_map *vector,
> static void memirq_dispatch_engine(struct xe_memirq *memirq, struct iosys_map *status,
> struct xe_hw_engine *hwe)
> {
>- memirq_debug(memirq, "STATUS %s %*ph\n", hwe->name, 16, status->vaddr);
>+ memirq_debug(memirq, "STATUS %s %*ph\n", hwe->name, 16, iosys_map_ptr(status));
>
> if (memirq_received(memirq, status, ilog2(GT_RENDER_USER_INTERRUPT), hwe->name))
> xe_hw_engine_handle_irq(hwe, GT_RENDER_USER_INTERRUPT);
>@@ -429,7 +429,7 @@ static void memirq_dispatch_guc(struct xe_memirq *memirq, struct iosys_map *stat
> {
> const char *name = guc_name(guc);
>
>- memirq_debug(memirq, "STATUS %s %*ph\n", name, 16, status->vaddr);
>+ memirq_debug(memirq, "STATUS %s %*ph\n", name, 16, iosys_map_ptr(status));
>
> if (memirq_received(memirq, status, ilog2(GUC_INTR_GUC2HOST), name))
> xe_guc_irq_handler(guc, GUC_INTR_GUC2HOST);
>@@ -479,9 +479,9 @@ void xe_memirq_handler(struct xe_memirq *memirq)
> if (!memirq->bo)
> return;
>
>- memirq_assert(memirq, !memirq->source.is_iomem);
>- memirq_debug(memirq, "SOURCE %*ph\n", 32, memirq->source.vaddr);
>- memirq_debug(memirq, "SOURCE %*ph\n", 32, memirq->source.vaddr + 32);
>+ memirq_assert(memirq, !iosys_map_is_iomem(&memirq->source));
>+ memirq_debug(memirq, "SOURCE %*ph\n", 32, iosys_map_ptr(&memirq->source));
>+ memirq_debug(memirq, "SOURCE %*ph\n", 32, iosys_map_ptr(&memirq->source) + 32);
>
> for_each_gt(gt, xe, gtid) {
> if (gt->tile != tile)
>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>index fb842fa0552e..99424d790d84 100644
>--- a/drivers/gpu/drm/xe/xe_oa.c
>+++ b/drivers/gpu/drm/xe/xe_oa.c
>@@ -880,8 +880,8 @@ static int xe_oa_alloc_oa_buffer(struct xe_oa_stream *stream, size_t size)
>
> stream->oa_buffer.bo = bo;
> /* mmap implementation requires OA buffer to be in system memory */
>- xe_assert(stream->oa->xe, bo->vmap.is_iomem == 0);
>- stream->oa_buffer.vaddr = bo->vmap.vaddr;
>+ xe_assert(stream->oa->xe, iosys_map_is_iomem(&bo->vmap) == 0);
>+ stream->oa_buffer.vaddr = iosys_map_ptr(&bo->vmap);
> return 0;
> }
>
>diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>index b42cf5d1b20c..af0992aea6b4 100644
>--- a/drivers/gpu/drm/xe/xe_pt.c
>+++ b/drivers/gpu/drm/xe/xe_pt.c
>@@ -1723,12 +1723,12 @@ xe_migrate_clear_pgtable_callback(struct xe_migrate_pt_update *pt_update,
> u64 empty = __xe_pt_empty_pte(tile, vm, update->pt->level);
> int i;
>
>- if (map && map->is_iomem)
>+ if (map && iosys_map_is_iomem(map))
> for (i = 0; i < num_qwords; ++i)
> xe_map_wr(tile_to_xe(tile), map, (qword_ofs + i) *
> sizeof(u64), u64, empty);
> else if (map)
>- memset64(map->vaddr + qword_ofs * sizeof(u64), empty,
>+ memset64(iosys_map_ptr(map) + qword_ofs * sizeof(u64), empty,
> num_qwords);
> else
> memset64(ptr, empty, num_qwords);
>diff --git a/drivers/gpu/drm/xe/xe_sa.c b/drivers/gpu/drm/xe/xe_sa.c
>index 1d43e183ca21..4ac335c68242 100644
>--- a/drivers/gpu/drm/xe/xe_sa.c
>+++ b/drivers/gpu/drm/xe/xe_sa.c
>@@ -68,15 +68,15 @@ struct xe_sa_manager *__xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u3
> return ERR_CAST(bo);
> }
> sa_manager->bo = bo;
>- sa_manager->is_iomem = bo->vmap.is_iomem;
>+ sa_manager->is_iomem = iosys_map_is_iomem(&bo->vmap);
> sa_manager->gpu_addr = xe_bo_ggtt_addr(bo);
>
>- if (bo->vmap.is_iomem) {
>+ if (iosys_map_is_iomem(&bo->vmap)) {
> sa_manager->cpu_ptr = kvzalloc(managed_size, GFP_KERNEL);
> if (!sa_manager->cpu_ptr)
> return ERR_PTR(-ENOMEM);
> } else {
>- sa_manager->cpu_ptr = bo->vmap.vaddr;
>+ sa_manager->cpu_ptr = iosys_map_ptr(&bo->vmap);
> memset(sa_manager->cpu_ptr, 0, bo->ttm.base.size);
> }
>
>@@ -116,7 +116,7 @@ void xe_sa_bo_flush_write(struct drm_suballoc *sa_bo)
> struct xe_sa_manager *sa_manager = to_xe_sa_manager(sa_bo->manager);
> struct xe_device *xe = tile_to_xe(sa_manager->bo->tile);
>
>- if (!sa_manager->bo->vmap.is_iomem)
>+ if (!iosys_map_is_iomem(&sa_manager->bo->vmap))
> return;
>
> xe_map_memcpy_to(xe, &sa_manager->bo->vmap, drm_suballoc_soffset(sa_bo),
>--
>2.49.0
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: iosys-map: refactor to reduce struct size
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (13 preceding siblings ...)
2025-05-22 12:00 ` iosys-map: refactor to reduce struct size Thomas Zimmermann
@ 2025-05-22 13:59 ` Lucas De Marchi
2025-06-27 17:31 ` Lucas De Marchi
15 siblings, 0 replies; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 13:59 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe
On Thu, May 22, 2025 at 04:52:09PM +1000, Dave Airlie wrote:
>Hey iosys_map users :)
>
>I fell down a bit of a refactor hole today, it was just random and
>sometimes you just have to let these things run their course.
>
>I noticed iosys_map has a 7-byte hole in a 16-byte structure, and
>it gets embedded into a bunch of other structs and it offended my
>sensibilities.
>
>This series makes iosys_map be 8-bytes, using the bottom bit of
>the void * to store the is_iomem.
>
>Patch 1: adds new accessors to start hiding internals
>Patches 2-7: refactor all users in-tree to use new internals
>(hopefully got them all)
>Patch8: moves the internals around to catch anything not in-tree.
>Patch9: reimplements iosys_map as 8-bytes by hiding the is_iomem
>inside the pointer.
I went through the series and left a few nit comments and things we may
need to fix on top that I only found because of reviewing this.
For the series:
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
We will probably have quite a few conflicts in drm-xe-next as there are
some changes touching those vmaps, but we can handle those.
Lucas De Marchi
>
>Dave.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally.
2025-05-22 13:34 ` Lucas De Marchi
@ 2025-05-22 14:00 ` Thomas Zimmermann
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-22 14:00 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: Dave Airlie, dri-devel, intel-xe
Hi
Am 22.05.25 um 15:34 schrieb Lucas De Marchi:
[...]
>>
>> We should encourage users to the other helpers for interacting with
>> iosys-map structures instead of decoding them manually. OTOH there
>> are cases where decoding them by hand is clearly better. I'd suggest
>> to prefix the new helpers with __ so mark them an internal/special.
>
> From the other patches there are quite a few cases that would be using
> "internal" API. From those there are just a few cases in which we'd
> have a direct translation to existing API... so I wouldn't make this
> internal when they are clearly needed externally.
FTR I'm fine with either.
Best regards
Thomas
>
> Lucas De Marchi
>
>>
>> Best regards
>> Thomas
>>
>>> /**
>>> * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an
>>> address in system memory
>>> * @vaddr_: A system-memory address
>>> @@ -234,9 +249,9 @@ static inline bool iosys_map_is_equal(const
>>> struct iosys_map *lhs,
>>> */
>>> static inline bool iosys_map_is_null(const struct iosys_map *map)
>>> {
>>> - if (map->is_iomem)
>>> - return !map->vaddr_iomem;
>>> - return !map->vaddr;
>>> + if (iosys_map_is_iomem(map))
>>> + return !iosys_map_ioptr(map);
>>> + return !iosys_map_ptr(map);
>>> }
>>> /**
>>> @@ -286,10 +301,10 @@ static inline void iosys_map_clear(struct
>>> iosys_map *map)
>>> static inline void iosys_map_memcpy_to(struct iosys_map *dst,
>>> size_t dst_offset,
>>> const void *src, size_t len)
>>> {
>>> - if (dst->is_iomem)
>>> - memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
>>> + if (iosys_map_is_iomem(dst))
>>> + memcpy_toio(iosys_map_ioptr(dst) + dst_offset, src, len);
>>> else
>>> - memcpy(dst->vaddr + dst_offset, src, len);
>>> + memcpy(iosys_map_ptr(dst) + dst_offset, src, len);
>>> }
>>> /**
>>> @@ -306,10 +321,10 @@ static inline void iosys_map_memcpy_to(struct
>>> iosys_map *dst, size_t dst_offset,
>>> static inline void iosys_map_memcpy_from(void *dst, const struct
>>> iosys_map *src,
>>> size_t src_offset, size_t len)
>>> {
>>> - if (src->is_iomem)
>>> - memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
>>> + if (iosys_map_is_iomem(src))
>>> + memcpy_fromio(dst, iosys_map_ioptr(src) + src_offset, len);
>>> else
>>> - memcpy(dst, src->vaddr + src_offset, len);
>>> + memcpy(dst, iosys_map_ptr(src) + src_offset, len);
>>> }
>>> /**
>>> @@ -322,7 +337,7 @@ static inline void iosys_map_memcpy_from(void
>>> *dst, const struct iosys_map *src,
>>> */
>>> static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
>>> {
>>> - if (map->is_iomem)
>>> + if (iosys_map_is_iomem(map))
>>> map->vaddr_iomem += incr;
>>> else
>>> map->vaddr += incr;
>>> @@ -341,10 +356,10 @@ static inline void iosys_map_incr(struct
>>> iosys_map *map, size_t incr)
>>> static inline void iosys_map_memset(struct iosys_map *dst, size_t
>>> offset,
>>> int value, size_t len)
>>> {
>>> - if (dst->is_iomem)
>>> - memset_io(dst->vaddr_iomem + offset, value, len);
>>> + if (iosys_map_is_iomem(dst))
>>> + memset_io(iosys_map_ioptr(dst) + offset, value, len);
>>> else
>>> - memset(dst->vaddr + offset, value, len);
>>> + memset(iosys_map_ptr(dst) + offset, value, len);
>>> }
>>> #ifdef CONFIG_64BIT
>>> @@ -393,10 +408,10 @@ static inline void iosys_map_memset(struct
>>> iosys_map *dst, size_t offset,
>>> */
>>> #define iosys_map_rd(map__, offset__, type__) ({ \
>>> type__ val_; \
>>> - if ((map__)->is_iomem) { \
>>> - __iosys_map_rd_io(val_, (map__)->vaddr_iomem + (offset__),
>>> type__); \
>>> + if (iosys_map_is_iomem(map__)) { \
>>> + __iosys_map_rd_io(val_, iosys_map_ioptr(map__) +
>>> (offset__), type__); \
>>> } else { \
>>> - __iosys_map_rd_sys(val_, (map__)->vaddr + (offset__),
>>> type__); \
>>> + __iosys_map_rd_sys(val_, iosys_map_ptr(map__) + (offset__),
>>> type__); \
>>> } \
>>> val_; \
>>> })
>>> @@ -415,10 +430,10 @@ static inline void iosys_map_memset(struct
>>> iosys_map *dst, size_t offset,
>>> */
>>> #define iosys_map_wr(map__, offset__, type__, val__)
>>> ({ \
>>> type__ val_ = (val__); \
>>> - if ((map__)->is_iomem) { \
>>> - __iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__),
>>> type__); \
>>> + if (iosys_map_is_iomem(map__)) { \
>>> + __iosys_map_wr_io(val_, iosys_map_ioptr(map__) +
>>> (offset__), type__); \
>>> } else { \
>>> - __iosys_map_wr_sys(val_, (map__)->vaddr + (offset__),
>>> type__); \
>>> + __iosys_map_wr_sys(val_, iosys_map_ptr(map__) + (offset__),
>>> type__); \
>>> } \
>>> })
>>
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
2025-05-22 8:48 ` Jani Nikula
2025-05-22 12:10 ` Thomas Zimmermann
@ 2025-05-22 15:09 ` Lucas De Marchi
2025-05-22 20:32 ` Dave Airlie
2025-05-26 6:39 ` Thomas Zimmermann
2 siblings, 2 replies; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 15:09 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe, Michal.Wajdeczko
On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airlied@redhat.com>
>
>This reduces this struct from 16 to 8 bytes, and it gets embedded
>into a lot of things.
>
>Signed-off-by: Dave Airlie <airlied@redhat.com>
Replied too early on cover. Chatting with Michal Wajdeczko today, this
may break things as we then can't byte-address anymore. It seems
particularly dangerous when using the iosys_map_wr/iosys_map_rd as
there's nothing preventing an unaligned address and we increment the map
with the sizeof() of a struct that could be __packed. Example: in
xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
In this particular case it doesn't give unaligned address, but we should
probably then protect iosys_map from doing the wrong thing.
So, if we are keeping this patch, we should probably protect
initially-unaligned addresses and the iosys_map_incr() call?
thanks
Lucas De Marchi
>---
> include/linux/iosys-map.h | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
>diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>index a6c2cc9ca756..44479966ce24 100644
>--- a/include/linux/iosys-map.h
>+++ b/include/linux/iosys-map.h
>@@ -99,29 +99,27 @@
> * iosys_map_incr(&map, len); // go to first byte after the memcpy
> */
>
>+#define _IOSYS_MAP_IS_IOMEM 1
> /**
> * struct iosys_map - Pointer to IO/system memory
> * @vaddr_iomem: The buffer's address if in I/O memory
> * @vaddr: The buffer's address if in system memory
>- * @is_iomem: True if the buffer is located in I/O memory, or false
>- * otherwise.
> */
> struct iosys_map {
> union {
> void __iomem *_vaddr_iomem;
> void *_vaddr;
> };
>- bool _is_iomem;
> };
>
> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
> {
>- return map->_is_iomem;
>+ return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
> }
>
> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
> {
>- return map->_vaddr_iomem;
>+ return (void __iomem *)((unsigned long)map->_vaddr_iomem & ~_IOSYS_MAP_IS_IOMEM);
> }
>
> static inline void *iosys_map_ptr(const struct iosys_map *map)
>@@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> #define IOSYS_MAP_INIT_VADDR(vaddr_) \
> { \
> ._vaddr = (vaddr_), \
>- ._is_iomem = false, \
> }
>
> /**
>@@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> */
> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
> { \
>- ._vaddr_iomem = (vaddr_iomem_), \
>- ._is_iomem = true, \
>+ ._vaddr_iomem = (void __iomem *)(((unsigned long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
> }
>
> /**
>@@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct iosys_map *map)
> static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> {
> map->_vaddr = vaddr;
>- map->_is_iomem = false;
> }
>
> /**
>@@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> void __iomem *vaddr_iomem)
> {
>- map->_vaddr_iomem = vaddr_iomem;
>- map->_is_iomem = true;
>+ map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem | _IOSYS_MAP_IS_IOMEM);
> }
>
> /**
>@@ -229,12 +223,9 @@ static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> const struct iosys_map *rhs)
> {
>- if (lhs->_is_iomem != rhs->_is_iomem)
>+ if (lhs->_vaddr != rhs->_vaddr)
> return false;
>- else if (lhs->_is_iomem)
>- return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
>- else
>- return lhs->_vaddr == rhs->_vaddr;
>+ return true;
> }
>
> /**
>@@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct iosys_map *map)
> */
> static inline void iosys_map_clear(struct iosys_map *map)
> {
>- if (map->_is_iomem) {
>- map->_vaddr_iomem = NULL;
>- map->_is_iomem = false;
>- } else {
>- map->_vaddr = NULL;
>- }
>+ map->_vaddr = NULL;
> }
>
> /**
>--
>2.49.0
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 15:09 ` Lucas De Marchi
@ 2025-05-22 20:32 ` Dave Airlie
2025-05-22 21:05 ` Lucas De Marchi
2025-05-26 6:39 ` Thomas Zimmermann
1 sibling, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-22 20:32 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: dri-devel, tzimmermann, intel-xe, Michal.Wajdeczko
On Fri, 23 May 2025 at 01:10, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
> >From: Dave Airlie <airlied@redhat.com>
> >
> >This reduces this struct from 16 to 8 bytes, and it gets embedded
> >into a lot of things.
> >
> >Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Replied too early on cover. Chatting with Michal Wajdeczko today, this
> may break things as we then can't byte-address anymore. It seems
> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
> there's nothing preventing an unaligned address and we increment the map
> with the sizeof() of a struct that could be __packed. Example: in
> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
> In this particular case it doesn't give unaligned address, but we should
> probably then protect iosys_map from doing the wrong thing.
>
> So, if we are keeping this patch, we should probably protect
> initially-unaligned addresses and the iosys_map_incr() call?
oh interesting, my thoughts was of course nobody would want to use
this for < 32-byte aligned ptrs :-)
but I forgot about using the incr for stuff, I do wonder if the incr
could be modelled on a base + offset, as I do think for a lot of stuff
we'd want to retain the original vaddr for unmapping or other things,
I'll play around a bit more next week with at least protecting against bad uses.
Dave.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 20:32 ` Dave Airlie
@ 2025-05-22 21:05 ` Lucas De Marchi
2025-05-23 12:31 ` Jocelyn Falempe
0 siblings, 1 reply; 34+ messages in thread
From: Lucas De Marchi @ 2025-05-22 21:05 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe, Michal.Wajdeczko
On Fri, May 23, 2025 at 06:32:43AM +1000, Dave Airlie wrote:
>On Fri, 23 May 2025 at 01:10, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>> >From: Dave Airlie <airlied@redhat.com>
>> >
>> >This reduces this struct from 16 to 8 bytes, and it gets embedded
>> >into a lot of things.
>> >
>> >Signed-off-by: Dave Airlie <airlied@redhat.com>
>>
>> Replied too early on cover. Chatting with Michal Wajdeczko today, this
>> may break things as we then can't byte-address anymore. It seems
>> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
>> there's nothing preventing an unaligned address and we increment the map
>> with the sizeof() of a struct that could be __packed. Example: in
>> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
>> In this particular case it doesn't give unaligned address, but we should
>> probably then protect iosys_map from doing the wrong thing.
>>
>> So, if we are keeping this patch, we should probably protect
>> initially-unaligned addresses and the iosys_map_incr() call?
>
>oh interesting, my thoughts was of course nobody would want to use
>this for < 32-byte aligned ptrs :-)
>
>but I forgot about using the incr for stuff, I do wonder if the incr
>could be modelled on a base + offset, as I do think for a lot of stuff
>we'd want to retain the original vaddr for unmapping or other things,
for the parts of the code that want to update "inner blocks", the
approach is to copy the struct and operate on that. Example from
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:guc_prep_golden_context:
info_map = IOSYS_MAP_INIT_OFFSET(&guc->ads_map,
offsetof(struct __guc_ads_blob, system_info));
then that function can manipulate its "local map" without affecting the
one used for really mapping the memory.
>
>I'll play around a bit more next week with at least protecting against bad uses.
thanks
Lucas De Marchi
>
>Dave.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] iosys: hide internal details of implementation.
2025-05-22 6:52 ` [PATCH 8/9] iosys: hide internal details of implementation Dave Airlie
2025-05-22 12:05 ` Thomas Zimmermann
@ 2025-05-22 23:30 ` kernel test robot
1 sibling, 0 replies; 34+ messages in thread
From: kernel test robot @ 2025-05-22 23:30 UTC (permalink / raw)
To: Dave Airlie, dri-devel, tzimmermann; +Cc: oe-kbuild-all, intel-xe
Hi Dave,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20250522]
[cannot apply to drm-xe/drm-xe-next drm-exynos/exynos-drm-next linus/master v6.15-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Airlie/udmabuf-use-new-iosys_map-accessors/20250522-155237
base: git://anongit.freedesktop.org/drm/drm drm-next
patch link: https://lore.kernel.org/r/20250522065519.318013-9-airlied%40gmail.com
patch subject: [PATCH 8/9] iosys: hide internal details of implementation.
config: x86_64-buildonly-randconfig-001-20250523 (https://download.01.org/0day-ci/archive/20250523/202505230735.HJoyQtMT-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250523/202505230735.HJoyQtMT-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505230735.HJoyQtMT-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/gpu/drm/drm_gem_dma_helper.c: In function 'drm_gem_dma_prime_import_sg_table_vmap':
>> drivers/gpu/drm/drm_gem_dma_helper.c:596:30: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
596 | dma_obj->vaddr = map.vaddr;
| ^~~~~
| _vaddr
--
drivers/gpu/drm/drm_fbdev_dma.c: In function 'drm_fbdev_dma_driver_fbdev_probe_tail':
>> drivers/gpu/drm/drm_fbdev_dma.c:218:35: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
218 | info->screen_buffer = map.vaddr;
| ^~~~~
| _vaddr
>> drivers/gpu/drm/drm_fbdev_dma.c:209:26: warning: variable 'map' set but not used [-Wunused-but-set-variable]
209 | struct iosys_map map = buffer->map;
| ^~~
In file included from arch/x86/include/asm/bug.h:103,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/linux/spinlock.h:60,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from include/uapi/linux/fb.h:6,
from include/linux/fb.h:5,
from drivers/gpu/drm/drm_fbdev_dma.c:3:
drivers/gpu/drm/drm_fbdev_dma.c: In function 'drm_fbdev_dma_driver_fbdev_probe':
>> drivers/gpu/drm/drm_fbdev_dma.c:297:41: error: 'struct iosys_map' has no member named 'is_iomem'; did you mean '_is_iomem'?
297 | } else if (drm_WARN_ON(dev, map.is_iomem)) {
| ^~~~~~~~
include/asm-generic/bug.h:132:32: note: in definition of macro 'WARN'
132 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
include/drm/drm_print.h:789:9: note: in expansion of macro 'drm_WARN'
789 | drm_WARN((drm), (x), "%s", \
| ^~~~~~~~
drivers/gpu/drm/drm_fbdev_dma.c:297:20: note: in expansion of macro 'drm_WARN_ON'
297 | } else if (drm_WARN_ON(dev, map.is_iomem)) {
| ^~~~~~~~~~~
--
drivers/gpu/drm/drm_mipi_dbi.c: In function 'mipi_dbi_fb_dirty':
>> drivers/gpu/drm/drm_mipi_dbi.c:293:27: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
293 | tr = src->vaddr; /* TODO: Use mapping abstraction properly */
| ^~~~~
| _vaddr
--
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c: In function 'etnaviv_gem_prime_vmap_impl':
>> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c:88:20: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
88 | return map.vaddr;
| ^~~~~
| _vaddr
>> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c:89:1: warning: control reaches end of non-void function [-Wreturn-type]
89 | }
| ^
--
drivers/gpu/drm/tiny/ili9225.c: In function 'ili9225_fb_dirty':
>> drivers/gpu/drm/tiny/ili9225.c:106:27: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
106 | tr = src->vaddr; /* TODO: Use mapping abstraction properly */
| ^~~~~
| _vaddr
--
drivers/gpu/drm/tiny/gm12u320.c: In function 'gm12u320_copy_fb_to_blocks':
>> drivers/gpu/drm/tiny/gm12u320.c:270:45: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
270 | vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping abstraction properly */
| ^~~~~
| _vaddr
--
drivers/gpu/drm/gud/gud_pipe.c: In function 'gud_prep_flush':
>> drivers/gpu/drm/gud/gud_pipe.c:169:24: error: 'const struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
169 | vaddr = src[0].vaddr;
| ^~~~~
| _vaddr
--
drivers/gpu/drm/vkms/vkms_formats.c: In function 'packed_pixels_addr':
>> drivers/gpu/drm/vkms/vkms_formats.c:79:42: error: 'const struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
79 | *addr = (u8 *)frame_info->map[0].vaddr + offset;
| ^~~~~
| _vaddr
drivers/gpu/drm/vkms/vkms_formats.c: In function 'packed_pixels_addr_1x1':
drivers/gpu/drm/vkms/vkms_formats.c:140:42: error: 'const struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
140 | *addr = (u8 *)frame_info->map[0].vaddr + offset;
| ^~~~~
| _vaddr
--
In file included from drivers/gpu/drm/panthor/panthor_heap.c:10:
drivers/gpu/drm/panthor/panthor_gem.h: In function 'panthor_kernel_bo_vmap':
>> drivers/gpu/drm/panthor/panthor_gem.h:190:24: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
190 | bo->kmap = map.vaddr;
| ^~~~~
| _vaddr
--
In file included from drivers/gpu/drm/panthor/panthor_sched.c:29:
drivers/gpu/drm/panthor/panthor_gem.h: In function 'panthor_kernel_bo_vmap':
>> drivers/gpu/drm/panthor/panthor_gem.h:190:24: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
190 | bo->kmap = map.vaddr;
| ^~~~~
| _vaddr
drivers/gpu/drm/panthor/panthor_sched.c: In function 'panthor_queue_get_syncwait_obj':
>> drivers/gpu/drm/panthor/panthor_sched.c:873:36: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
873 | queue->syncwait.kmap = map.vaddr;
| ^~~~~
| _vaddr
--
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump':
>> drivers/gpu/drm/panfrost/panfrost_dump.c:228:29: error: 'struct iosys_map' has no member named 'vaddr'; did you mean '_vaddr'?
228 | vaddr = map.vaddr;
| ^~~~~
| _vaddr
..
vim +596 drivers/gpu/drm/drm_gem_dma_helper.c
f5ca8eb6f9bd5e drivers/gpu/drm/drm_gem_cma_helper.c Thomas Zimmermann 2020-11-23 552
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 553 /**
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 554 * drm_gem_dma_prime_import_sg_table_vmap - PRIME import another driver's
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 555 * scatter/gather table and get the virtual address of the buffer
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 556 * @dev: DRM device
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 557 * @attach: DMA-BUF attachment
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 558 * @sgt: Scatter/gather table of pinned pages
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 559 *
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 560 * This function imports a scatter/gather table using
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 561 * drm_gem_dma_prime_import_sg_table() and uses dma_buf_vmap() to get the kernel
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 562 * virtual address. This ensures that a DMA GEM object always has its virtual
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 563 * address set. This address is released when the object is freed.
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 564 *
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 565 * This function can be used as the &drm_driver.gem_prime_import_sg_table
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 566 * callback. The &DRM_GEM_DMA_DRIVER_OPS_VMAP macro provides a shortcut to set
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 567 * the necessary DRM driver operations.
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 568 *
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 569 * Returns:
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 570 * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 571 * error code on failure.
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 572 */
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 573 struct drm_gem_object *
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 574 drm_gem_dma_prime_import_sg_table_vmap(struct drm_device *dev,
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 575 struct dma_buf_attachment *attach,
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 576 struct sg_table *sgt)
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 577 {
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 578 struct drm_gem_dma_object *dma_obj;
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 579 struct drm_gem_object *obj;
7938f4218168ae drivers/gpu/drm/drm_gem_cma_helper.c Lucas De Marchi 2022-02-04 580 struct iosys_map map;
6619ccf1bb1d0e drivers/gpu/drm/drm_gem_cma_helper.c Thomas Zimmermann 2020-09-25 581 int ret;
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 582
79e2cf2e7a1934 drivers/gpu/drm/drm_gem_dma_helper.c Dmitry Osipenko 2022-10-17 583 ret = dma_buf_vmap_unlocked(attach->dmabuf, &map);
6619ccf1bb1d0e drivers/gpu/drm/drm_gem_cma_helper.c Thomas Zimmermann 2020-09-25 584 if (ret) {
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 585 DRM_ERROR("Failed to vmap PRIME buffer\n");
6619ccf1bb1d0e drivers/gpu/drm/drm_gem_cma_helper.c Thomas Zimmermann 2020-09-25 586 return ERR_PTR(ret);
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 587 }
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 588
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 589 obj = drm_gem_dma_prime_import_sg_table(dev, attach, sgt);
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 590 if (IS_ERR(obj)) {
79e2cf2e7a1934 drivers/gpu/drm/drm_gem_dma_helper.c Dmitry Osipenko 2022-10-17 591 dma_buf_vunmap_unlocked(attach->dmabuf, &map);
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 592 return obj;
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 593 }
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 594
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 595 dma_obj = to_drm_gem_dma_obj(obj);
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 @596 dma_obj->vaddr = map.vaddr;
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 597
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 598 return obj;
b9068cde51eea1 drivers/gpu/drm/drm_gem_cma_helper.c Noralf Trønnes 2018-11-10 599 }
4a83c26a1d8702 drivers/gpu/drm/drm_gem_dma_helper.c Danilo Krummrich 2022-08-02 600 EXPORT_SYMBOL(drm_gem_dma_prime_import_sg_table_vmap);
4b2b5e142ff499 drivers/gpu/drm/drm_gem_cma_helper.c Thomas Zimmermann 2021-10-20 601
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals.
2025-05-22 6:52 ` [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals Dave Airlie
@ 2025-05-23 0:32 ` kernel test robot
0 siblings, 0 replies; 34+ messages in thread
From: kernel test robot @ 2025-05-23 0:32 UTC (permalink / raw)
To: Dave Airlie, dri-devel, tzimmermann; +Cc: oe-kbuild-all, intel-xe
Hi Dave,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip v6.15-rc7 next-20250522]
[cannot apply to drm-xe/drm-xe-next drm-exynos/exynos-drm-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Airlie/udmabuf-use-new-iosys_map-accessors/20250522-155237
base: git://anongit.freedesktop.org/drm/drm drm-next
patch link: https://lore.kernel.org/r/20250522065519.318013-4-airlied%40gmail.com
patch subject: [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals.
config: arm-randconfig-002-20250523 (https://download.01.org/0day-ci/archive/20250523/202505230806.C7HIxQ4g-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250523/202505230806.C7HIxQ4g-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505230806.C7HIxQ4g-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/firmware/tegra/ivc.c: In function 'iosys_map_get_address':
>> drivers/firmware/tegra/ivc.c:632:29: error: expected ')' before 'return'
if (iosys_map_is_iomem(map)
~ ^
)
return (unsigned long)iosys_map_ioptr(map);
~~~~~~
>> drivers/firmware/tegra/ivc.c:636:1: error: expected expression before '}' token
}
^
>> drivers/firmware/tegra/ivc.c:636:1: warning: no return statement in function returning non-void [-Wreturn-type]
vim +632 drivers/firmware/tegra/ivc.c
629
630 static inline unsigned long iosys_map_get_address(const struct iosys_map *map)
631 {
> 632 if (iosys_map_is_iomem(map)
633 return (unsigned long)iosys_map_ioptr(map);
634
635 return (unsigned long)iosys_map_ptr(map);
> 636 }
637
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 21:05 ` Lucas De Marchi
@ 2025-05-23 12:31 ` Jocelyn Falempe
0 siblings, 0 replies; 34+ messages in thread
From: Jocelyn Falempe @ 2025-05-23 12:31 UTC (permalink / raw)
To: Lucas De Marchi, Dave Airlie
Cc: dri-devel, tzimmermann, intel-xe, Michal.Wajdeczko
On 22/05/2025 23:05, Lucas De Marchi wrote:
> On Fri, May 23, 2025 at 06:32:43AM +1000, Dave Airlie wrote:
>> On Fri, 23 May 2025 at 01:10, Lucas De Marchi
>> <lucas.demarchi@intel.com> wrote:
>>>
>>> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>>> >From: Dave Airlie <airlied@redhat.com>
>>> >
>>> >This reduces this struct from 16 to 8 bytes, and it gets embedded
>>> >into a lot of things.
>>> >
>>> >Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>
>>> Replied too early on cover. Chatting with Michal Wajdeczko today, this
>>> may break things as we then can't byte-address anymore. It seems
>>> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
>>> there's nothing preventing an unaligned address and we increment the map
>>> with the sizeof() of a struct that could be __packed. Example: in
>>> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
>>> In this particular case it doesn't give unaligned address, but we should
>>> probably then protect iosys_map from doing the wrong thing.
>>>
>>> So, if we are keeping this patch, we should probably protect
>>> initially-unaligned addresses and the iosys_map_incr() call?
>>
>> oh interesting, my thoughts was of course nobody would want to use
>> this for < 32-byte aligned ptrs :-)
>>
>> but I forgot about using the incr for stuff, I do wonder if the incr
>> could be modelled on a base + offset, as I do think for a lot of stuff
>> we'd want to retain the original vaddr for unmapping or other things,
>
> for the parts of the code that want to update "inner blocks", the
> approach is to copy the struct and operate on that. Example from
> drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:guc_prep_golden_context:
>
> info_map = IOSYS_MAP_INIT_OFFSET(&guc->ads_map,
> offsetof(struct __guc_ads_blob, system_info));
>
> then that function can manipulate its "local map" without affecting the
> one used for really mapping the memory.
Also drm_panic uses that to draw into the frame buffer:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_panic.c#L181
So in case of 16bits or 24bits pixel width, it might be unaligned.
As it is used only to save one argument to the "blit" function, I think
saving 8 bytes in the iosys_map struct, and using an additional offset
argument is doable.
It is also used the same way in drm_format_helper:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_format_helper.c#L290
Best regards,
--
Jocelyn
>
>>
>> I'll play around a bit more next week with at least protecting against
>> bad uses.
>
> thanks
> Lucas De Marchi
>
>>
>> Dave.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-22 15:09 ` Lucas De Marchi
2025-05-22 20:32 ` Dave Airlie
@ 2025-05-26 6:39 ` Thomas Zimmermann
2025-05-26 7:58 ` Dave Airlie
1 sibling, 1 reply; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-26 6:39 UTC (permalink / raw)
To: Lucas De Marchi, Dave Airlie; +Cc: dri-devel, intel-xe, Michal.Wajdeczko
Hi
Am 22.05.25 um 17:09 schrieb Lucas De Marchi:
> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This reduces this struct from 16 to 8 bytes, and it gets embedded
>> into a lot of things.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Replied too early on cover. Chatting with Michal Wajdeczko today, this
> may break things as we then can't byte-address anymore. It seems
> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
> there's nothing preventing an unaligned address and we increment the map
> with the sizeof() of a struct that could be __packed. Example: in
> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
> In this particular case it doesn't give unaligned address, but we should
> probably then protect iosys_map from doing the wrong thing.
>
> So, if we are keeping this patch, we should probably protect
> initially-unaligned addresses and the iosys_map_incr() call?
That sounds like a blocker to me. And there's another thing to keep in
mind. We have use cases where we need to know the caching of the memory
area. I never got to fully implement this, but it would be stored in the
iosys-map struct as well. We'd need 2 additional bits to represent UC,
WC and WT caching. If we don't have at least 3-bit alignment, shrinking
iosys-map might not be feasible anyway.
Best regards
Thomas
>
> thanks
> Lucas De Marchi
>
>> ---
>> include/linux/iosys-map.h | 30 ++++++++----------------------
>> 1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>> index a6c2cc9ca756..44479966ce24 100644
>> --- a/include/linux/iosys-map.h
>> +++ b/include/linux/iosys-map.h
>> @@ -99,29 +99,27 @@
>> * iosys_map_incr(&map, len); // go to first byte after the memcpy
>> */
>>
>> +#define _IOSYS_MAP_IS_IOMEM 1
>> /**
>> * struct iosys_map - Pointer to IO/system memory
>> * @vaddr_iomem: The buffer's address if in I/O memory
>> * @vaddr: The buffer's address if in system memory
>> - * @is_iomem: True if the buffer is located in I/O memory, or
>> false
>> - * otherwise.
>> */
>> struct iosys_map {
>> union {
>> void __iomem *_vaddr_iomem;
>> void *_vaddr;
>> };
>> - bool _is_iomem;
>> };
>>
>> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
>> {
>> - return map->_is_iomem;
>> + return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
>> }
>>
>> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
>> {
>> - return map->_vaddr_iomem;
>> + return (void __iomem *)((unsigned long)map->_vaddr_iomem &
>> ~_IOSYS_MAP_IS_IOMEM);
>> }
>>
>> static inline void *iosys_map_ptr(const struct iosys_map *map)
>> @@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct
>> iosys_map *map)
>> #define IOSYS_MAP_INIT_VADDR(vaddr_) \
>> { \
>> ._vaddr = (vaddr_), \
>> - ._is_iomem = false, \
>> }
>>
>> /**
>> @@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct
>> iosys_map *map)
>> */
>> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_) \
>> { \
>> - ._vaddr_iomem = (vaddr_iomem_), \
>> - ._is_iomem = true, \
>> + ._vaddr_iomem = (void __iomem *)(((unsigned
>> long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
>> }
>>
>> /**
>> @@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct
>> iosys_map *map)
>> static inline void iosys_map_set_vaddr(struct iosys_map *map, void
>> *vaddr)
>> {
>> map->_vaddr = vaddr;
>> - map->_is_iomem = false;
>> }
>>
>> /**
>> @@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct
>> iosys_map *map, void *vaddr)
>> static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
>> void __iomem *vaddr_iomem)
>> {
>> - map->_vaddr_iomem = vaddr_iomem;
>> - map->_is_iomem = true;
>> + map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem
>> | _IOSYS_MAP_IS_IOMEM);
>> }
>>
>> /**
>> @@ -229,12 +223,9 @@ static inline void
>> iosys_map_set_vaddr_iomem(struct iosys_map *map,
>> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
>> const struct iosys_map *rhs)
>> {
>> - if (lhs->_is_iomem != rhs->_is_iomem)
>> + if (lhs->_vaddr != rhs->_vaddr)
>> return false;
>> - else if (lhs->_is_iomem)
>> - return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
>> - else
>> - return lhs->_vaddr == rhs->_vaddr;
>> + return true;
>> }
>>
>> /**
>> @@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct
>> iosys_map *map)
>> */
>> static inline void iosys_map_clear(struct iosys_map *map)
>> {
>> - if (map->_is_iomem) {
>> - map->_vaddr_iomem = NULL;
>> - map->_is_iomem = false;
>> - } else {
>> - map->_vaddr = NULL;
>> - }
>> + map->_vaddr = NULL;
>> }
>>
>> /**
>> --
>> 2.49.0
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-26 6:39 ` Thomas Zimmermann
@ 2025-05-26 7:58 ` Dave Airlie
2025-05-26 8:14 ` Thomas Zimmermann
0 siblings, 1 reply; 34+ messages in thread
From: Dave Airlie @ 2025-05-26 7:58 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: Lucas De Marchi, dri-devel, intel-xe, Michal.Wajdeczko
On Mon, 26 May 2025 at 16:39, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 22.05.25 um 17:09 schrieb Lucas De Marchi:
> > On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <airlied@redhat.com>
> >>
> >> This reduces this struct from 16 to 8 bytes, and it gets embedded
> >> into a lot of things.
> >>
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >
> > Replied too early on cover. Chatting with Michal Wajdeczko today, this
> > may break things as we then can't byte-address anymore. It seems
> > particularly dangerous when using the iosys_map_wr/iosys_map_rd as
> > there's nothing preventing an unaligned address and we increment the map
> > with the sizeof() of a struct that could be __packed. Example: in
> > xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
> > In this particular case it doesn't give unaligned address, but we should
> > probably then protect iosys_map from doing the wrong thing.
> >
> > So, if we are keeping this patch, we should probably protect
> > initially-unaligned addresses and the iosys_map_incr() call?
>
> That sounds like a blocker to me. And there's another thing to keep in
> mind. We have use cases where we need to know the caching of the memory
> area. I never got to fully implement this, but it would be stored in the
> iosys-map struct as well. We'd need 2 additional bits to represent UC,
> WC and WT caching. If we don't have at least 3-bit alignment, shrinking
> iosys-map might not be feasible anyway.
I've played around a bit, and it's starting to seem like it might be
difficult to get this across the line.
I can add the iter stuff separately to fix the sub-dword offsets if
needed, we probably don't want to be doing 8-bit iomem accesses
anyways.
But if we need 3-bits then it's messier, what's the use case out of
interest to store the info? do you need all 3 states?
Dave.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.
2025-05-26 7:58 ` Dave Airlie
@ 2025-05-26 8:14 ` Thomas Zimmermann
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Zimmermann @ 2025-05-26 8:14 UTC (permalink / raw)
To: Dave Airlie; +Cc: Lucas De Marchi, dri-devel, intel-xe, Michal.Wajdeczko
Hi
Am 26.05.25 um 09:58 schrieb Dave Airlie:
> On Mon, 26 May 2025 at 16:39, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 22.05.25 um 17:09 schrieb Lucas De Marchi:
>>> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>>>> From: Dave Airlie <airlied@redhat.com>
>>>>
>>>> This reduces this struct from 16 to 8 bytes, and it gets embedded
>>>> into a lot of things.
>>>>
>>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>> Replied too early on cover. Chatting with Michal Wajdeczko today, this
>>> may break things as we then can't byte-address anymore. It seems
>>> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
>>> there's nothing preventing an unaligned address and we increment the map
>>> with the sizeof() of a struct that could be __packed. Example: in
>>> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
>>> In this particular case it doesn't give unaligned address, but we should
>>> probably then protect iosys_map from doing the wrong thing.
>>>
>>> So, if we are keeping this patch, we should probably protect
>>> initially-unaligned addresses and the iosys_map_incr() call?
>> That sounds like a blocker to me. And there's another thing to keep in
>> mind. We have use cases where we need to know the caching of the memory
>> area. I never got to fully implement this, but it would be stored in the
>> iosys-map struct as well. We'd need 2 additional bits to represent UC,
>> WC and WT caching. If we don't have at least 3-bit alignment, shrinking
>> iosys-map might not be feasible anyway.
> I've played around a bit, and it's starting to seem like it might be
> difficult to get this across the line.
>
> I can add the iter stuff separately to fix the sub-dword offsets if
> needed, we probably don't want to be doing 8-bit iomem accesses
> anyways.
>
> But if we need 3-bits then it's messier, what's the use case out of
> interest to store the info? do you need all 3 states?
Various drivers perform format conversion or a memcpy of BO data as part
of their display update. For buffers without caching, it's fastest to
read the buffer once into system memory and do any further processing
from there. If the buffer is already in cached memory, this would be
overhead. Due to the lack of caching information, the gud driver treats
all imported BOs as uncached. (Fourth argument at [1]). Storing the
caching info in the iosys-map would benefit this driver and several others.
Best regards
Thomas
[1]
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/gpu/drm/gud/gud_pipe.c#L446
>
> Dave.
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: iosys-map: refactor to reduce struct size
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
` (14 preceding siblings ...)
2025-05-22 13:59 ` Lucas De Marchi
@ 2025-06-27 17:31 ` Lucas De Marchi
15 siblings, 0 replies; 34+ messages in thread
From: Lucas De Marchi @ 2025-06-27 17:31 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, tzimmermann, intel-xe
On Thu, May 22, 2025 at 04:52:09PM +1000, Dave Airlie wrote:
>Hey iosys_map users :)
>
>I fell down a bit of a refactor hole today, it was just random and
>sometimes you just have to let these things run their course.
>
>I noticed iosys_map has a 7-byte hole in a 16-byte structure, and
>it gets embedded into a bunch of other structs and it offended my
>sensibilities.
>
>This series makes iosys_map be 8-bytes, using the bottom bit of
>the void * to store the is_iomem.
>
>Patch 1: adds new accessors to start hiding internals
>Patches 2-7: refactor all users in-tree to use new internals
>(hopefully got them all)
>Patch8: moves the internals around to catch anything not in-tree.
>Patch9: reimplements iosys_map as 8-bytes by hiding the is_iomem
>inside the pointer.
Even if not being able to use patch 9, I think there's a value in the
previous patches to hide the internals. Is this something you're going
to re-submit without the last one?
thanks
Lucas De Marchi
>
>Dave.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2025-06-27 17:32 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-22 6:52 iosys-map: refactor to reduce struct size Dave Airlie
2025-05-22 6:52 ` [PATCH 1/9] iosys-map: add new accessor interfaces and use them internally Dave Airlie
2025-05-22 11:58 ` Thomas Zimmermann
2025-05-22 13:34 ` Lucas De Marchi
2025-05-22 14:00 ` Thomas Zimmermann
2025-05-22 6:52 ` [PATCH 2/9] udmabuf: use new iosys_map accessors Dave Airlie
2025-05-22 6:52 ` [PATCH 3/9] firmware/tegra: avoid accessing iosys_map internals Dave Airlie
2025-05-23 0:32 ` kernel test robot
2025-05-22 6:52 ` [PATCH 4/9] drm/xe: avoid accessing internals of iosys_map Dave Airlie
2025-05-22 13:56 ` Lucas De Marchi
2025-05-22 6:52 ` [PATCH 5/9] drm/qxl: avoid accessing iosys_map internals Dave Airlie
2025-05-22 13:39 ` Lucas De Marchi
2025-05-22 6:52 ` [PATCH 6/9] drm/ttm: " Dave Airlie
2025-05-22 6:52 ` [PATCH 7/9] drm: " Dave Airlie
2025-05-22 6:52 ` [PATCH 8/9] iosys: hide internal details of implementation Dave Airlie
2025-05-22 12:05 ` Thomas Zimmermann
2025-05-22 23:30 ` kernel test robot
2025-05-22 6:52 ` [PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer Dave Airlie
2025-05-22 8:48 ` Jani Nikula
2025-05-22 12:10 ` Thomas Zimmermann
2025-05-22 15:09 ` Lucas De Marchi
2025-05-22 20:32 ` Dave Airlie
2025-05-22 21:05 ` Lucas De Marchi
2025-05-23 12:31 ` Jocelyn Falempe
2025-05-26 6:39 ` Thomas Zimmermann
2025-05-26 7:58 ` Dave Airlie
2025-05-26 8:14 ` Thomas Zimmermann
2025-05-22 7:13 ` iosys-map: refactor to reduce struct size Dave Airlie
2025-05-22 7:20 ` ✓ CI.Patch_applied: success for series starting with [1/9] iosys-map: add new accessor interfaces and use them internally Patchwork
2025-05-22 7:20 ` ✗ CI.checkpatch: warning " Patchwork
2025-05-22 7:21 ` ✗ CI.KUnit: failure " Patchwork
2025-05-22 12:00 ` iosys-map: refactor to reduce struct size Thomas Zimmermann
2025-05-22 13:59 ` Lucas De Marchi
2025-06-27 17:31 ` Lucas De Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).