* [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
@ 2025-05-01 6:43 oushixiong1025
2025-05-01 6:43 ` [PATCH 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS oushixiong1025
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: oushixiong1025 @ 2025-05-01 6:43 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Sean Paul, Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
[WHY]
1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
sg_table import.
They only need dma_buf_vmap() to access the shared buffer's
kernel virtual address.
2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
restricts the maximum DMA streaming mapping memory, resulting in
errors like:
ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
[HOW]
Provide a gem_prime_import implementation without sg_table mapping
to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
require sg_table can adopt this.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
include/drm/drm_gem_shmem_helper.h | 24 +++++++
2 files changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index d99dee67353a..9e41e350ff6f 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
.free = drm_gem_shmem_object_free,
.print_info = drm_gem_shmem_object_print_info,
+ .export = drm_gem_shmem_object_prime_export,
.pin = drm_gem_shmem_object_pin,
.unpin = drm_gem_shmem_object_unpin,
.get_sg_table = drm_gem_shmem_object_get_sg_table,
@@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
+const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
+ .cache_sgt_mapping = true,
+ .attach = drm_gem_map_attach,
+ .detach = drm_gem_map_detach,
+ .map_dma_buf = drm_gem_map_dma_buf,
+ .unmap_dma_buf = drm_gem_unmap_dma_buf,
+ .release = drm_gem_dmabuf_release,
+ .mmap = drm_gem_dmabuf_mmap,
+ .vmap = drm_gem_dmabuf_vmap,
+ .vunmap = drm_gem_dmabuf_vunmap,
+};
+
+/**
+ * drm_gem_shmem_prime_export - implementation of the export callback
+ * @shmem: shmem GEM object
+ */
+struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
+ int flags)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct drm_device *dev = obj->dev;
+ struct dma_buf_export_info exp_info = {
+ .exp_name = KBUILD_MODNAME, /* white lie for debug */
+ .owner = dev->driver->fops->owner,
+ .ops = &drm_gem_shmem_prime_dmabuf_ops,
+ .size = obj->size,
+ .flags = flags,
+ .priv = obj,
+ .resv = obj->resv,
+ };
+
+ return drm_gem_dmabuf_export(dev, &exp_info);
+}
+
+/**
+ * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
+ * @dev: Device to import into
+ * @dma_buf: dma-buf object to import
+ *
+ * Drivers that use the shmem helpers but also wants to import dmabuf without
+ * mapping its sg_table can use this as their &drm_driver.gem_prime_import
+ * implementation.
+ */
+struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
+ struct dma_buf *dma_buf)
+{
+ struct dma_buf_attachment *attach;
+ struct drm_gem_shmem_object *shmem;
+ size_t size;
+ int ret;
+
+ if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
+ struct drm_gem_object *obj;
+
+ obj = dma_buf->priv;
+ if (obj->dev == dev) {
+ /*
+ * Importing dmabuf exported from our own gem increases
+ * refcount on gem itself instead of f_count of dmabuf.
+ */
+ drm_gem_object_get(obj);
+ return obj;
+ }
+ }
+
+ attach = dma_buf_attach(dma_buf, dev->dev);
+ if (IS_ERR(attach))
+ return ERR_CAST(attach);
+
+ get_dma_buf(dma_buf);
+
+ size = PAGE_ALIGN(attach->dmabuf->size);
+
+ shmem = __drm_gem_shmem_create(dev, size, true, NULL);
+ if (IS_ERR(shmem)) {
+ ret = PTR_ERR(shmem);
+ goto fail_detach;
+ }
+
+ drm_dbg_prime(dev, "size = %zu\n", size);
+
+ shmem->base.import_attach = attach;
+ shmem->base.resv = dma_buf->resv;
+
+ return &shmem->base;
+
+fail_detach:
+ dma_buf_detach(dma_buf, attach);
+ dma_buf_put(dma_buf);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
+
MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
MODULE_IMPORT_NS("DMA_BUF");
MODULE_LICENSE("GPL v2");
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index cef5a6b5a4d6..78ef91593a8e 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
struct iosys_map *map);
int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
+struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
+ int flags);
int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
@@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
drm_gem_shmem_print_info(shmem, p, indent);
}
+/**
+ * drm_gem_shmem_object_prime_export - GEM object function for export()
+ * @obj: GEM object
+ *
+ */
+static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
+ int flags)
+{
+ struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+
+ return drm_gem_shmem_prime_export(shmem, flags);
+}
/**
* drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
* @obj: GEM object
@@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
struct sg_table *sgt);
int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
struct drm_mode_create_dumb *args);
+struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
+ struct dma_buf *buf);
/**
* DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
@@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
.dumb_create = drm_gem_shmem_dumb_create
+/**
+ * This macro provides a shmem GEM operations that implementate a simple
+ * gem_prime_import.
+ */
+#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
+ .gem_prime_import = drm_gem_shmem_prime_import, \
+ .dumb_create = drm_gem_shmem_dumb_create
+
#endif /* __DRM_GEM_SHMEM_HELPER_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS
2025-05-01 6:43 [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table oushixiong1025
@ 2025-05-01 6:43 ` oushixiong1025
2025-05-01 6:43 ` [PATCH 3/3] drm/udl: " oushixiong1025
2025-05-05 11:12 ` [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table Thomas Zimmermann
2 siblings, 0 replies; 9+ messages in thread
From: oushixiong1025 @ 2025-05-01 6:43 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Sean Paul, Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Import dmabuf without mapping its sg_table to avoid issues likes:
ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/gpu/drm/ast/ast_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 6fbf62a99c48..2dac6acf79e7 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -64,7 +64,7 @@ static const struct drm_driver ast_driver = {
.minor = DRIVER_MINOR,
.patchlevel = DRIVER_PATCHLEVEL,
- DRM_GEM_SHMEM_DRIVER_OPS,
+ DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS,
DRM_FBDEV_SHMEM_DRIVER_OPS,
};
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/udl: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS
2025-05-01 6:43 [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table oushixiong1025
2025-05-01 6:43 ` [PATCH 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS oushixiong1025
@ 2025-05-01 6:43 ` oushixiong1025
2025-05-05 11:12 ` [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table Thomas Zimmermann
2 siblings, 0 replies; 9+ messages in thread
From: oushixiong1025 @ 2025-05-01 6:43 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Sean Paul, Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Import dmabuf without mapping its sg_table to avoid issues likes:
udl 2-1.4:1.0: swiotlb buffer is full (sz: 2097152 bytes), total 65536 (slots), used 1 (slots)
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/gpu/drm/udl/udl_drv.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 05b3a152cc33..8107549b12e5 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -49,22 +49,6 @@ static int udl_usb_reset_resume(struct usb_interface *interface)
return drm_mode_config_helper_resume(dev);
}
-/*
- * FIXME: Dma-buf sharing requires DMA support by the importing device.
- * This function is a workaround to make USB devices work as well.
- * See todo.rst for how to fix the issue in the dma-buf framework.
- */
-static struct drm_gem_object *udl_driver_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf)
-{
- struct udl_device *udl = to_udl(dev);
-
- if (!udl->dmadev)
- return ERR_PTR(-ENODEV);
-
- return drm_gem_prime_import_dev(dev, dma_buf, udl->dmadev);
-}
-
DEFINE_DRM_GEM_FOPS(udl_driver_fops);
static const struct drm_driver driver = {
@@ -72,8 +56,7 @@ static const struct drm_driver driver = {
/* GEM hooks */
.fops = &udl_driver_fops,
- DRM_GEM_SHMEM_DRIVER_OPS,
- .gem_prime_import = udl_driver_gem_prime_import,
+ DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS,
DRM_FBDEV_SHMEM_DRIVER_OPS,
.name = DRIVER_NAME,
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-01 6:43 [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table oushixiong1025
2025-05-01 6:43 ` [PATCH 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS oushixiong1025
2025-05-01 6:43 ` [PATCH 3/3] drm/udl: " oushixiong1025
@ 2025-05-05 11:12 ` Thomas Zimmermann
2025-05-05 11:25 ` Christian König
2 siblings, 1 reply; 9+ messages in thread
From: Thomas Zimmermann @ 2025-05-05 11:12 UTC (permalink / raw)
To: oushixiong1025, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou,
Christian König
(cc'ing Christian)
Hi,
I don't feel qualified to fully review this patch.
It would be good to have the issue with sg-tables solved, but I dislike
the dedicated initializer macros. So my question is if this has any
drawbacks. Or could we make this available and the default for all
shmem-based drivers?
Best regards
Thomas
Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> [WHY]
> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
> sg_table import.
> They only need dma_buf_vmap() to access the shared buffer's
> kernel virtual address.
>
> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
> restricts the maximum DMA streaming mapping memory, resulting in
> errors like:
>
> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>
> [HOW]
> Provide a gem_prime_import implementation without sg_table mapping
> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
> require sg_table can adopt this.
>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
> include/drm/drm_gem_shmem_helper.h | 24 +++++++
> 2 files changed, 119 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index d99dee67353a..9e41e350ff6f 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> .free = drm_gem_shmem_object_free,
> .print_info = drm_gem_shmem_object_print_info,
> + .export = drm_gem_shmem_object_prime_export,
> .pin = drm_gem_shmem_object_pin,
> .unpin = drm_gem_shmem_object_unpin,
> .get_sg_table = drm_gem_shmem_object_get_sg_table,
> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>
> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
> + .cache_sgt_mapping = true,
> + .attach = drm_gem_map_attach,
> + .detach = drm_gem_map_detach,
> + .map_dma_buf = drm_gem_map_dma_buf,
> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
> + .release = drm_gem_dmabuf_release,
> + .mmap = drm_gem_dmabuf_mmap,
> + .vmap = drm_gem_dmabuf_vmap,
> + .vunmap = drm_gem_dmabuf_vunmap,
> +};
> +
> +/**
> + * drm_gem_shmem_prime_export - implementation of the export callback
> + * @shmem: shmem GEM object
> + */
> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
> + int flags)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_device *dev = obj->dev;
> + struct dma_buf_export_info exp_info = {
> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
> + .owner = dev->driver->fops->owner,
> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
> + .size = obj->size,
> + .flags = flags,
> + .priv = obj,
> + .resv = obj->resv,
> + };
> +
> + return drm_gem_dmabuf_export(dev, &exp_info);
> +}
> +
> +/**
> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
> + * @dev: Device to import into
> + * @dma_buf: dma-buf object to import
> + *
> + * Drivers that use the shmem helpers but also wants to import dmabuf without
> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
> + * implementation.
> + */
> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
> + struct dma_buf *dma_buf)
> +{
> + struct dma_buf_attachment *attach;
> + struct drm_gem_shmem_object *shmem;
> + size_t size;
> + int ret;
> +
> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
> + struct drm_gem_object *obj;
> +
> + obj = dma_buf->priv;
> + if (obj->dev == dev) {
> + /*
> + * Importing dmabuf exported from our own gem increases
> + * refcount on gem itself instead of f_count of dmabuf.
> + */
> + drm_gem_object_get(obj);
> + return obj;
> + }
> + }
> +
> + attach = dma_buf_attach(dma_buf, dev->dev);
> + if (IS_ERR(attach))
> + return ERR_CAST(attach);
> +
> + get_dma_buf(dma_buf);
> +
> + size = PAGE_ALIGN(attach->dmabuf->size);
> +
> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
> + if (IS_ERR(shmem)) {
> + ret = PTR_ERR(shmem);
> + goto fail_detach;
> + }
> +
> + drm_dbg_prime(dev, "size = %zu\n", size);
> +
> + shmem->base.import_attach = attach;
> + shmem->base.resv = dma_buf->resv;
> +
> + return &shmem->base;
> +
> +fail_detach:
> + dma_buf_detach(dma_buf, attach);
> + dma_buf_put(dma_buf);
> +
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
> +
> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
> MODULE_IMPORT_NS("DMA_BUF");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index cef5a6b5a4d6..78ef91593a8e 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
> struct iosys_map *map);
> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
> + int flags);
>
> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
> drm_gem_shmem_print_info(shmem, p, indent);
> }
>
> +/**
> + * drm_gem_shmem_object_prime_export - GEM object function for export()
> + * @obj: GEM object
> + *
> + */
> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
> + int flags)
> +{
> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> +
> + return drm_gem_shmem_prime_export(shmem, flags);
> +}
> /**
> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
> * @obj: GEM object
> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> struct sg_table *sgt);
> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
> struct drm_mode_create_dumb *args);
> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
> + struct dma_buf *buf);
>
> /**
> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
> .dumb_create = drm_gem_shmem_dumb_create
>
> +/**
> + * This macro provides a shmem GEM operations that implementate a simple
> + * gem_prime_import.
> + */
> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
> + .gem_prime_import = drm_gem_shmem_prime_import, \
> + .dumb_create = drm_gem_shmem_dumb_create
> +
> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
--
--
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] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-05 11:12 ` [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table Thomas Zimmermann
@ 2025-05-05 11:25 ` Christian König
2025-05-05 14:22 ` oushixiong
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2025-05-05 11:25 UTC (permalink / raw)
To: Thomas Zimmermann, oushixiong1025, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
Hi Thomas & Shixiong,
first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
So if I'm not completely mistaken the issue is already resolved upstream.
Regards,
Christian.
On 5/5/25 13:12, Thomas Zimmermann wrote:
> (cc'ing Christian)
>
> Hi,
>
> I don't feel qualified to fully review this patch.
>
> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>
> Best regards
> Thomas
>
> Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>
>> [WHY]
>> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
>> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
>> sg_table import.
>> They only need dma_buf_vmap() to access the shared buffer's
>> kernel virtual address.
>>
>> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
>> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
>> restricts the maximum DMA streaming mapping memory, resulting in
>> errors like:
>>
>> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>
>> [HOW]
>> Provide a gem_prime_import implementation without sg_table mapping
>> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
>> require sg_table can adopt this.
>>
>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>> ---
>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>> 2 files changed, 119 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index d99dee67353a..9e41e350ff6f 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>> .free = drm_gem_shmem_object_free,
>> .print_info = drm_gem_shmem_object_print_info,
>> + .export = drm_gem_shmem_object_prime_export,
>> .pin = drm_gem_shmem_object_pin,
>> .unpin = drm_gem_shmem_object_unpin,
>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>> }
>> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
>> + .cache_sgt_mapping = true,
>> + .attach = drm_gem_map_attach,
>> + .detach = drm_gem_map_detach,
>> + .map_dma_buf = drm_gem_map_dma_buf,
>> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
>> + .release = drm_gem_dmabuf_release,
>> + .mmap = drm_gem_dmabuf_mmap,
>> + .vmap = drm_gem_dmabuf_vmap,
>> + .vunmap = drm_gem_dmabuf_vunmap,
>> +};
>> +
>> +/**
>> + * drm_gem_shmem_prime_export - implementation of the export callback
>> + * @shmem: shmem GEM object
>> + */
>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>> + int flags)
>> +{
>> + struct drm_gem_object *obj = &shmem->base;
>> + struct drm_device *dev = obj->dev;
>> + struct dma_buf_export_info exp_info = {
>> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
>> + .owner = dev->driver->fops->owner,
>> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
>> + .size = obj->size,
>> + .flags = flags,
>> + .priv = obj,
>> + .resv = obj->resv,
>> + };
>> +
>> + return drm_gem_dmabuf_export(dev, &exp_info);
>> +}
>> +
>> +/**
>> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
>> + * @dev: Device to import into
>> + * @dma_buf: dma-buf object to import
>> + *
>> + * Drivers that use the shmem helpers but also wants to import dmabuf without
>> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
>> + * implementation.
>> + */
>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>> + struct dma_buf *dma_buf)
>> +{
>> + struct dma_buf_attachment *attach;
>> + struct drm_gem_shmem_object *shmem;
>> + size_t size;
>> + int ret;
>> +
>> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
>> + struct drm_gem_object *obj;
>> +
>> + obj = dma_buf->priv;
>> + if (obj->dev == dev) {
>> + /*
>> + * Importing dmabuf exported from our own gem increases
>> + * refcount on gem itself instead of f_count of dmabuf.
>> + */
>> + drm_gem_object_get(obj);
>> + return obj;
>> + }
>> + }
>> +
>> + attach = dma_buf_attach(dma_buf, dev->dev);
>> + if (IS_ERR(attach))
>> + return ERR_CAST(attach);
>> +
>> + get_dma_buf(dma_buf);
>> +
>> + size = PAGE_ALIGN(attach->dmabuf->size);
>> +
>> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>> + if (IS_ERR(shmem)) {
>> + ret = PTR_ERR(shmem);
>> + goto fail_detach;
>> + }
>> +
>> + drm_dbg_prime(dev, "size = %zu\n", size);
>> +
>> + shmem->base.import_attach = attach;
>> + shmem->base.resv = dma_buf->resv;
>> +
>> + return &shmem->base;
>> +
>> +fail_detach:
>> + dma_buf_detach(dma_buf, attach);
>> + dma_buf_put(dma_buf);
>> +
>> + return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
>> +
>> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>> MODULE_IMPORT_NS("DMA_BUF");
>> MODULE_LICENSE("GPL v2");
>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>> index cef5a6b5a4d6..78ef91593a8e 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>> struct iosys_map *map);
>> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>> + int flags);
>> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
>> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
>> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>> drm_gem_shmem_print_info(shmem, p, indent);
>> }
>> +/**
>> + * drm_gem_shmem_object_prime_export - GEM object function for export()
>> + * @obj: GEM object
>> + *
>> + */
>> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
>> + int flags)
>> +{
>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> +
>> + return drm_gem_shmem_prime_export(shmem, flags);
>> +}
>> /**
>> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
>> * @obj: GEM object
>> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>> struct sg_table *sgt);
>> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>> struct drm_mode_create_dumb *args);
>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>> + struct dma_buf *buf);
>> /**
>> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>> .dumb_create = drm_gem_shmem_dumb_create
>> +/**
>> + * This macro provides a shmem GEM operations that implementate a simple
>> + * gem_prime_import.
>> + */
>> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>> + .gem_prime_import = drm_gem_shmem_prime_import, \
>> + .dumb_create = drm_gem_shmem_dumb_create
>> +
>> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-05 11:25 ` Christian König
@ 2025-05-05 14:22 ` oushixiong
2025-05-05 14:32 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: oushixiong @ 2025-05-05 14:22 UTC (permalink / raw)
To: Christian König, Thomas Zimmermann, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
Hi Christian,
My patch is based on linux-next, so this patch is not based on the
latest code. Then, I'd like to ask which patch resolved the issue with
sg-tables?
Thanks and Regards,
Shixiong.
在 2025/5/5 19:25, Christian König 写道:
> Hi Thomas & Shixiong,
>
> first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
>
> So if I'm not completely mistaken the issue is already resolved upstream.
>
> Regards,
> Christian.
>
> On 5/5/25 13:12, Thomas Zimmermann wrote:
>> (cc'ing Christian)
>>
>> Hi,
>>
>> I don't feel qualified to fully review this patch.
>>
>> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>>
>> Best regards
>> Thomas
>>
>> Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
>>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>>
>>> [WHY]
>>> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
>>> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
>>> sg_table import.
>>> They only need dma_buf_vmap() to access the shared buffer's
>>> kernel virtual address.
>>>
>>> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
>>> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
>>> restricts the maximum DMA streaming mapping memory, resulting in
>>> errors like:
>>>
>>> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>>
>>> [HOW]
>>> Provide a gem_prime_import implementation without sg_table mapping
>>> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
>>> require sg_table can adopt this.
>>>
>>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>>> ---
>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>>> 2 files changed, 119 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> index d99dee67353a..9e41e350ff6f 100644
>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>> .free = drm_gem_shmem_object_free,
>>> .print_info = drm_gem_shmem_object_print_info,
>>> + .export = drm_gem_shmem_object_prime_export,
>>> .pin = drm_gem_shmem_object_pin,
>>> .unpin = drm_gem_shmem_object_unpin,
>>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>> }
>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
>>> + .cache_sgt_mapping = true,
>>> + .attach = drm_gem_map_attach,
>>> + .detach = drm_gem_map_detach,
>>> + .map_dma_buf = drm_gem_map_dma_buf,
>>> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
>>> + .release = drm_gem_dmabuf_release,
>>> + .mmap = drm_gem_dmabuf_mmap,
>>> + .vmap = drm_gem_dmabuf_vmap,
>>> + .vunmap = drm_gem_dmabuf_vunmap,
>>> +};
>>> +
>>> +/**
>>> + * drm_gem_shmem_prime_export - implementation of the export callback
>>> + * @shmem: shmem GEM object
>>> + */
>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>> + int flags)
>>> +{
>>> + struct drm_gem_object *obj = &shmem->base;
>>> + struct drm_device *dev = obj->dev;
>>> + struct dma_buf_export_info exp_info = {
>>> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
>>> + .owner = dev->driver->fops->owner,
>>> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
>>> + .size = obj->size,
>>> + .flags = flags,
>>> + .priv = obj,
>>> + .resv = obj->resv,
>>> + };
>>> +
>>> + return drm_gem_dmabuf_export(dev, &exp_info);
>>> +}
>>> +
>>> +/**
>>> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
>>> + * @dev: Device to import into
>>> + * @dma_buf: dma-buf object to import
>>> + *
>>> + * Drivers that use the shmem helpers but also wants to import dmabuf without
>>> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
>>> + * implementation.
>>> + */
>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>> + struct dma_buf *dma_buf)
>>> +{
>>> + struct dma_buf_attachment *attach;
>>> + struct drm_gem_shmem_object *shmem;
>>> + size_t size;
>>> + int ret;
>>> +
>>> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
>>> + struct drm_gem_object *obj;
>>> +
>>> + obj = dma_buf->priv;
>>> + if (obj->dev == dev) {
>>> + /*
>>> + * Importing dmabuf exported from our own gem increases
>>> + * refcount on gem itself instead of f_count of dmabuf.
>>> + */
>>> + drm_gem_object_get(obj);
>>> + return obj;
>>> + }
>>> + }
>>> +
>>> + attach = dma_buf_attach(dma_buf, dev->dev);
>>> + if (IS_ERR(attach))
>>> + return ERR_CAST(attach);
>>> +
>>> + get_dma_buf(dma_buf);
>>> +
>>> + size = PAGE_ALIGN(attach->dmabuf->size);
>>> +
>>> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>>> + if (IS_ERR(shmem)) {
>>> + ret = PTR_ERR(shmem);
>>> + goto fail_detach;
>>> + }
>>> +
>>> + drm_dbg_prime(dev, "size = %zu\n", size);
>>> +
>>> + shmem->base.import_attach = attach;
>>> + shmem->base.resv = dma_buf->resv;
>>> +
>>> + return &shmem->base;
>>> +
>>> +fail_detach:
>>> + dma_buf_detach(dma_buf, attach);
>>> + dma_buf_put(dma_buf);
>>> +
>>> + return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
>>> +
>>> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>> MODULE_IMPORT_NS("DMA_BUF");
>>> MODULE_LICENSE("GPL v2");
>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>> index cef5a6b5a4d6..78ef91593a8e 100644
>>> --- a/include/drm/drm_gem_shmem_helper.h
>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>>> struct iosys_map *map);
>>> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>> + int flags);
>>> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
>>> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
>>> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>>> drm_gem_shmem_print_info(shmem, p, indent);
>>> }
>>> +/**
>>> + * drm_gem_shmem_object_prime_export - GEM object function for export()
>>> + * @obj: GEM object
>>> + *
>>> + */
>>> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
>>> + int flags)
>>> +{
>>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>> +
>>> + return drm_gem_shmem_prime_export(shmem, flags);
>>> +}
>>> /**
>>> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
>>> * @obj: GEM object
>>> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>> struct sg_table *sgt);
>>> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>> struct drm_mode_create_dumb *args);
>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>> + struct dma_buf *buf);
>>> /**
>>> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>>> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>>> .dumb_create = drm_gem_shmem_dumb_create
>>> +/**
>>> + * This macro provides a shmem GEM operations that implementate a simple
>>> + * gem_prime_import.
>>> + */
>>> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>>> + .gem_prime_import = drm_gem_shmem_prime_import, \
>>> + .dumb_create = drm_gem_shmem_dumb_create
>>> +
>>> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-05 14:22 ` oushixiong
@ 2025-05-05 14:32 ` Christian König
2025-05-05 15:11 ` oushixiong
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2025-05-05 14:32 UTC (permalink / raw)
To: oushixiong, Thomas Zimmermann, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
Hi Shixiong,
for drm changes please base your patches on drm-misc-next or drm-next.
That is probably fixed by this one here:
commit b72f66f22c0e39ae6684c43fead774c13db24e73
Author: Christian König <christian.koenig@amd.com>
Date: Tue Feb 11 17:20:53 2025 +0100
dma-buf: drop caching of sg_tables
That was purely for the transition from static to dynamic dma-buf
handling and can be removed again now.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20250211163109.12200-5-christian.koenig@amd.com
After this patch SG tables where only created when necessary.
Regards,
Christian.
On 5/5/25 16:22, oushixiong wrote:
> Hi Christian,
>
> My patch is based on linux-next, so this patch is not based on the latest code. Then, I'd like to ask which patch resolved the issue with sg-tables?
>
>
> Thanks and Regards,
>
> Shixiong.
>
>
> 在 2025/5/5 19:25, Christian König 写道:
>> Hi Thomas & Shixiong,
>>
>> first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
>>
>> So if I'm not completely mistaken the issue is already resolved upstream.
>>
>> Regards,
>> Christian.
>>
>> On 5/5/25 13:12, Thomas Zimmermann wrote:
>>> (cc'ing Christian)
>>>
>>> Hi,
>>>
>>> I don't feel qualified to fully review this patch.
>>>
>>> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>>>
>>> Best regards
>>> Thomas
>>>
>>> Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
>>>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>>>
>>>> [WHY]
>>>> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
>>>> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
>>>> sg_table import.
>>>> They only need dma_buf_vmap() to access the shared buffer's
>>>> kernel virtual address.
>>>>
>>>> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
>>>> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
>>>> restricts the maximum DMA streaming mapping memory, resulting in
>>>> errors like:
>>>>
>>>> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>>>
>>>> [HOW]
>>>> Provide a gem_prime_import implementation without sg_table mapping
>>>> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
>>>> require sg_table can adopt this.
>>>>
>>>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>>>> ---
>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>>>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>>>> 2 files changed, 119 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> index d99dee67353a..9e41e350ff6f 100644
>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>>> .free = drm_gem_shmem_object_free,
>>>> .print_info = drm_gem_shmem_object_print_info,
>>>> + .export = drm_gem_shmem_object_prime_export,
>>>> .pin = drm_gem_shmem_object_pin,
>>>> .unpin = drm_gem_shmem_object_unpin,
>>>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>>> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>> }
>>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>>> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
>>>> + .cache_sgt_mapping = true,
>>>> + .attach = drm_gem_map_attach,
>>>> + .detach = drm_gem_map_detach,
>>>> + .map_dma_buf = drm_gem_map_dma_buf,
>>>> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
>>>> + .release = drm_gem_dmabuf_release,
>>>> + .mmap = drm_gem_dmabuf_mmap,
>>>> + .vmap = drm_gem_dmabuf_vmap,
>>>> + .vunmap = drm_gem_dmabuf_vunmap,
>>>> +};
>>>> +
>>>> +/**
>>>> + * drm_gem_shmem_prime_export - implementation of the export callback
>>>> + * @shmem: shmem GEM object
>>>> + */
>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>> + int flags)
>>>> +{
>>>> + struct drm_gem_object *obj = &shmem->base;
>>>> + struct drm_device *dev = obj->dev;
>>>> + struct dma_buf_export_info exp_info = {
>>>> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
>>>> + .owner = dev->driver->fops->owner,
>>>> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
>>>> + .size = obj->size,
>>>> + .flags = flags,
>>>> + .priv = obj,
>>>> + .resv = obj->resv,
>>>> + };
>>>> +
>>>> + return drm_gem_dmabuf_export(dev, &exp_info);
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
>>>> + * @dev: Device to import into
>>>> + * @dma_buf: dma-buf object to import
>>>> + *
>>>> + * Drivers that use the shmem helpers but also wants to import dmabuf without
>>>> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
>>>> + * implementation.
>>>> + */
>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>> + struct dma_buf *dma_buf)
>>>> +{
>>>> + struct dma_buf_attachment *attach;
>>>> + struct drm_gem_shmem_object *shmem;
>>>> + size_t size;
>>>> + int ret;
>>>> +
>>>> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
>>>> + struct drm_gem_object *obj;
>>>> +
>>>> + obj = dma_buf->priv;
>>>> + if (obj->dev == dev) {
>>>> + /*
>>>> + * Importing dmabuf exported from our own gem increases
>>>> + * refcount on gem itself instead of f_count of dmabuf.
>>>> + */
>>>> + drm_gem_object_get(obj);
>>>> + return obj;
>>>> + }
>>>> + }
>>>> +
>>>> + attach = dma_buf_attach(dma_buf, dev->dev);
>>>> + if (IS_ERR(attach))
>>>> + return ERR_CAST(attach);
>>>> +
>>>> + get_dma_buf(dma_buf);
>>>> +
>>>> + size = PAGE_ALIGN(attach->dmabuf->size);
>>>> +
>>>> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>>>> + if (IS_ERR(shmem)) {
>>>> + ret = PTR_ERR(shmem);
>>>> + goto fail_detach;
>>>> + }
>>>> +
>>>> + drm_dbg_prime(dev, "size = %zu\n", size);
>>>> +
>>>> + shmem->base.import_attach = attach;
>>>> + shmem->base.resv = dma_buf->resv;
>>>> +
>>>> + return &shmem->base;
>>>> +
>>>> +fail_detach:
>>>> + dma_buf_detach(dma_buf, attach);
>>>> + dma_buf_put(dma_buf);
>>>> +
>>>> + return ERR_PTR(ret);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
>>>> +
>>>> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>>> MODULE_IMPORT_NS("DMA_BUF");
>>>> MODULE_LICENSE("GPL v2");
>>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>>> index cef5a6b5a4d6..78ef91593a8e 100644
>>>> --- a/include/drm/drm_gem_shmem_helper.h
>>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>>> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>>>> struct iosys_map *map);
>>>> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>> + int flags);
>>>> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
>>>> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
>>>> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>>>> drm_gem_shmem_print_info(shmem, p, indent);
>>>> }
>>>> +/**
>>>> + * drm_gem_shmem_object_prime_export - GEM object function for export()
>>>> + * @obj: GEM object
>>>> + *
>>>> + */
>>>> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
>>>> + int flags)
>>>> +{
>>>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>>> +
>>>> + return drm_gem_shmem_prime_export(shmem, flags);
>>>> +}
>>>> /**
>>>> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
>>>> * @obj: GEM object
>>>> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>> struct sg_table *sgt);
>>>> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>> struct drm_mode_create_dumb *args);
>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>> + struct dma_buf *buf);
>>>> /**
>>>> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>>>> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>>>> .dumb_create = drm_gem_shmem_dumb_create
>>>> +/**
>>>> + * This macro provides a shmem GEM operations that implementate a simple
>>>> + * gem_prime_import.
>>>> + */
>>>> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>>>> + .gem_prime_import = drm_gem_shmem_prime_import, \
>>>> + .dumb_create = drm_gem_shmem_dumb_create
>>>> +
>>>> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-05 14:32 ` Christian König
@ 2025-05-05 15:11 ` oushixiong
2025-05-05 15:23 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: oushixiong @ 2025-05-05 15:11 UTC (permalink / raw)
To: Christian König, Thomas Zimmermann, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
Hi Christian,
I don't see strong relevance between my patch and the patches you're
referring to.
Because the drm_gem_prime_import() function imports sg_table by default,
my patch provides an alternative
import callback implementation for SHMEM-based drivers that differs from
drm_gem_prime_import().
drm_gem_shmem_prime_import_sg_table() doesn't need to call
dma_buf_map_attachment_unlocked() to import sg_table.
Alternatively, I might not have fully understood the patches you mentioned.
Thanks and Regards,
Shixiong.
在 2025/5/5 22:32, Christian König 写道:
> Hi Shixiong,
>
> for drm changes please base your patches on drm-misc-next or drm-next.
>
> That is probably fixed by this one here:
>
> commit b72f66f22c0e39ae6684c43fead774c13db24e73
> Author: Christian König <christian.koenig@amd.com>
> Date: Tue Feb 11 17:20:53 2025 +0100
>
> dma-buf: drop caching of sg_tables
>
> That was purely for the transition from static to dynamic dma-buf
> handling and can be removed again now.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20250211163109.12200-5-christian.koenig@amd.com
>
> After this patch SG tables where only created when necessary.
>
> Regards,
> Christian.
>
> On 5/5/25 16:22, oushixiong wrote:
>> Hi Christian,
>>
>> My patch is based on linux-next, so this patch is not based on the latest code. Then, I'd like to ask which patch resolved the issue with sg-tables?
>>
>>
>> Thanks and Regards,
>>
>> Shixiong.
>>
>>
>> 在 2025/5/5 19:25, Christian König 写道:
>>> Hi Thomas & Shixiong,
>>>
>>> first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
>>>
>>> So if I'm not completely mistaken the issue is already resolved upstream.
>>>
>>> Regards,
>>> Christian.
>>>
>>> On 5/5/25 13:12, Thomas Zimmermann wrote:
>>>> (cc'ing Christian)
>>>>
>>>> Hi,
>>>>
>>>> I don't feel qualified to fully review this patch.
>>>>
>>>> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
>>>>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>>>>
>>>>> [WHY]
>>>>> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
>>>>> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
>>>>> sg_table import.
>>>>> They only need dma_buf_vmap() to access the shared buffer's
>>>>> kernel virtual address.
>>>>>
>>>>> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
>>>>> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
>>>>> restricts the maximum DMA streaming mapping memory, resulting in
>>>>> errors like:
>>>>>
>>>>> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>>>>
>>>>> [HOW]
>>>>> Provide a gem_prime_import implementation without sg_table mapping
>>>>> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
>>>>> require sg_table can adopt this.
>>>>>
>>>>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>>>>> ---
>>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>>>>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>>>>> 2 files changed, 119 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> index d99dee67353a..9e41e350ff6f 100644
>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>>>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>>>> .free = drm_gem_shmem_object_free,
>>>>> .print_info = drm_gem_shmem_object_print_info,
>>>>> + .export = drm_gem_shmem_object_prime_export,
>>>>> .pin = drm_gem_shmem_object_pin,
>>>>> .unpin = drm_gem_shmem_object_unpin,
>>>>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>>>> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>>>> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
>>>>> + .cache_sgt_mapping = true,
>>>>> + .attach = drm_gem_map_attach,
>>>>> + .detach = drm_gem_map_detach,
>>>>> + .map_dma_buf = drm_gem_map_dma_buf,
>>>>> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
>>>>> + .release = drm_gem_dmabuf_release,
>>>>> + .mmap = drm_gem_dmabuf_mmap,
>>>>> + .vmap = drm_gem_dmabuf_vmap,
>>>>> + .vunmap = drm_gem_dmabuf_vunmap,
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * drm_gem_shmem_prime_export - implementation of the export callback
>>>>> + * @shmem: shmem GEM object
>>>>> + */
>>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>>> + int flags)
>>>>> +{
>>>>> + struct drm_gem_object *obj = &shmem->base;
>>>>> + struct drm_device *dev = obj->dev;
>>>>> + struct dma_buf_export_info exp_info = {
>>>>> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
>>>>> + .owner = dev->driver->fops->owner,
>>>>> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
>>>>> + .size = obj->size,
>>>>> + .flags = flags,
>>>>> + .priv = obj,
>>>>> + .resv = obj->resv,
>>>>> + };
>>>>> +
>>>>> + return drm_gem_dmabuf_export(dev, &exp_info);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
>>>>> + * @dev: Device to import into
>>>>> + * @dma_buf: dma-buf object to import
>>>>> + *
>>>>> + * Drivers that use the shmem helpers but also wants to import dmabuf without
>>>>> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
>>>>> + * implementation.
>>>>> + */
>>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>>> + struct dma_buf *dma_buf)
>>>>> +{
>>>>> + struct dma_buf_attachment *attach;
>>>>> + struct drm_gem_shmem_object *shmem;
>>>>> + size_t size;
>>>>> + int ret;
>>>>> +
>>>>> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
>>>>> + struct drm_gem_object *obj;
>>>>> +
>>>>> + obj = dma_buf->priv;
>>>>> + if (obj->dev == dev) {
>>>>> + /*
>>>>> + * Importing dmabuf exported from our own gem increases
>>>>> + * refcount on gem itself instead of f_count of dmabuf.
>>>>> + */
>>>>> + drm_gem_object_get(obj);
>>>>> + return obj;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + attach = dma_buf_attach(dma_buf, dev->dev);
>>>>> + if (IS_ERR(attach))
>>>>> + return ERR_CAST(attach);
>>>>> +
>>>>> + get_dma_buf(dma_buf);
>>>>> +
>>>>> + size = PAGE_ALIGN(attach->dmabuf->size);
>>>>> +
>>>>> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>>>>> + if (IS_ERR(shmem)) {
>>>>> + ret = PTR_ERR(shmem);
>>>>> + goto fail_detach;
>>>>> + }
>>>>> +
>>>>> + drm_dbg_prime(dev, "size = %zu\n", size);
>>>>> +
>>>>> + shmem->base.import_attach = attach;
>>>>> + shmem->base.resv = dma_buf->resv;
>>>>> +
>>>>> + return &shmem->base;
>>>>> +
>>>>> +fail_detach:
>>>>> + dma_buf_detach(dma_buf, attach);
>>>>> + dma_buf_put(dma_buf);
>>>>> +
>>>>> + return ERR_PTR(ret);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
>>>>> +
>>>>> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>>>> MODULE_IMPORT_NS("DMA_BUF");
>>>>> MODULE_LICENSE("GPL v2");
>>>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>>>> index cef5a6b5a4d6..78ef91593a8e 100644
>>>>> --- a/include/drm/drm_gem_shmem_helper.h
>>>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>>>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>>>> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>>>>> struct iosys_map *map);
>>>>> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
>>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>>> + int flags);
>>>>> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
>>>>> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
>>>>> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>>>>> drm_gem_shmem_print_info(shmem, p, indent);
>>>>> }
>>>>> +/**
>>>>> + * drm_gem_shmem_object_prime_export - GEM object function for export()
>>>>> + * @obj: GEM object
>>>>> + *
>>>>> + */
>>>>> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
>>>>> + int flags)
>>>>> +{
>>>>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>>>> +
>>>>> + return drm_gem_shmem_prime_export(shmem, flags);
>>>>> +}
>>>>> /**
>>>>> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
>>>>> * @obj: GEM object
>>>>> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>>> struct sg_table *sgt);
>>>>> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>>> struct drm_mode_create_dumb *args);
>>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>>> + struct dma_buf *buf);
>>>>> /**
>>>>> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>>>>> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>>> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>>>>> .dumb_create = drm_gem_shmem_dumb_create
>>>>> +/**
>>>>> + * This macro provides a shmem GEM operations that implementate a simple
>>>>> + * gem_prime_import.
>>>>> + */
>>>>> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>>>>> + .gem_prime_import = drm_gem_shmem_prime_import, \
>>>>> + .dumb_create = drm_gem_shmem_dumb_create
>>>>> +
>>>>> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
2025-05-05 15:11 ` oushixiong
@ 2025-05-05 15:23 ` Christian König
0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2025-05-05 15:23 UTC (permalink / raw)
To: oushixiong, Thomas Zimmermann, Maarten Lankhorst
Cc: Maxime Ripard, David Airlie, Simona Vetter, Sean Paul,
Jocelyn Falempe, dri-devel, linux-kernel, Shixiong Ou
Hi Shixiong,
could also be that I didn't fully got what you try to do here.
Between which drivers and implementations are the BOs shared exactly?
You could also just rebase your patches on drm-misc-next and send them out again and I can take another look.
Regards,
Christian.
On 5/5/25 17:11, oushixiong wrote:
> Hi Christian,
>
> I don't see strong relevance between my patch and the patches you're referring to.
> Because the drm_gem_prime_import() function imports sg_table by default, my patch provides an alternative
> import callback implementation for SHMEM-based drivers that differs from drm_gem_prime_import().
> drm_gem_shmem_prime_import_sg_table() doesn't need to call dma_buf_map_attachment_unlocked() to import sg_table.
>
> Alternatively, I might not have fully understood the patches you mentioned.
>
>
> Thanks and Regards,
>
> Shixiong.
>
> 在 2025/5/5 22:32, Christian König 写道:
>> Hi Shixiong,
>>
>> for drm changes please base your patches on drm-misc-next or drm-next.
>>
>> That is probably fixed by this one here:
>>
>> commit b72f66f22c0e39ae6684c43fead774c13db24e73
>> Author: Christian König <christian.koenig@amd.com>
>> Date: Tue Feb 11 17:20:53 2025 +0100
>>
>> dma-buf: drop caching of sg_tables
>> That was purely for the transition from static to dynamic dma-buf
>> handling and can be removed again now.
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20250211163109.12200-5-christian.koenig@amd.com
>>
>> After this patch SG tables where only created when necessary.
>>
>> Regards,
>> Christian.
>>
>> On 5/5/25 16:22, oushixiong wrote:
>>> Hi Christian,
>>>
>>> My patch is based on linux-next, so this patch is not based on the latest code. Then, I'd like to ask which patch resolved the issue with sg-tables?
>>>
>>>
>>> Thanks and Regards,
>>>
>>> Shixiong.
>>>
>>>
>>> 在 2025/5/5 19:25, Christian König 写道:
>>>> Hi Thomas & Shixiong,
>>>>
>>>> first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
>>>>
>>>> So if I'm not completely mistaken the issue is already resolved upstream.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> On 5/5/25 13:12, Thomas Zimmermann wrote:
>>>>> (cc'ing Christian)
>>>>>
>>>>> Hi,
>>>>>
>>>>> I don't feel qualified to fully review this patch.
>>>>>
>>>>> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>> Am 01.05.25 um 08:43 schrieb oushixiong1025@163.com:
>>>>>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>>>>>
>>>>>> [WHY]
>>>>>> 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and
>>>>>> DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require
>>>>>> sg_table import.
>>>>>> They only need dma_buf_vmap() to access the shared buffer's
>>>>>> kernel virtual address.
>>>>>>
>>>>>> 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may
>>>>>> trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE
>>>>>> restricts the maximum DMA streaming mapping memory, resulting in
>>>>>> errors like:
>>>>>>
>>>>>> ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>>>>>
>>>>>> [HOW]
>>>>>> Provide a gem_prime_import implementation without sg_table mapping
>>>>>> to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not
>>>>>> require sg_table can adopt this.
>>>>>>
>>>>>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>>>>>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>>>>>> 2 files changed, 119 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>> index d99dee67353a..9e41e350ff6f 100644
>>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>> @@ -39,6 +39,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>>>>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>>>>> .free = drm_gem_shmem_object_free,
>>>>>> .print_info = drm_gem_shmem_object_print_info,
>>>>>> + .export = drm_gem_shmem_object_prime_export,
>>>>>> .pin = drm_gem_shmem_object_pin,
>>>>>> .unpin = drm_gem_shmem_object_unpin,
>>>>>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>>>>> @@ -799,6 +800,100 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>>>>> +const struct dma_buf_ops drm_gem_shmem_prime_dmabuf_ops = {
>>>>>> + .cache_sgt_mapping = true,
>>>>>> + .attach = drm_gem_map_attach,
>>>>>> + .detach = drm_gem_map_detach,
>>>>>> + .map_dma_buf = drm_gem_map_dma_buf,
>>>>>> + .unmap_dma_buf = drm_gem_unmap_dma_buf,
>>>>>> + .release = drm_gem_dmabuf_release,
>>>>>> + .mmap = drm_gem_dmabuf_mmap,
>>>>>> + .vmap = drm_gem_dmabuf_vmap,
>>>>>> + .vunmap = drm_gem_dmabuf_vunmap,
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>> + * drm_gem_shmem_prime_export - implementation of the export callback
>>>>>> + * @shmem: shmem GEM object
>>>>>> + */
>>>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>>>> + int flags)
>>>>>> +{
>>>>>> + struct drm_gem_object *obj = &shmem->base;
>>>>>> + struct drm_device *dev = obj->dev;
>>>>>> + struct dma_buf_export_info exp_info = {
>>>>>> + .exp_name = KBUILD_MODNAME, /* white lie for debug */
>>>>>> + .owner = dev->driver->fops->owner,
>>>>>> + .ops = &drm_gem_shmem_prime_dmabuf_ops,
>>>>>> + .size = obj->size,
>>>>>> + .flags = flags,
>>>>>> + .priv = obj,
>>>>>> + .resv = obj->resv,
>>>>>> + };
>>>>>> +
>>>>>> + return drm_gem_dmabuf_export(dev, &exp_info);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * drm_gem_shmem_prime_import - Import dmabuf without mapping its sg_table
>>>>>> + * @dev: Device to import into
>>>>>> + * @dma_buf: dma-buf object to import
>>>>>> + *
>>>>>> + * Drivers that use the shmem helpers but also wants to import dmabuf without
>>>>>> + * mapping its sg_table can use this as their &drm_driver.gem_prime_import
>>>>>> + * implementation.
>>>>>> + */
>>>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>>>> + struct dma_buf *dma_buf)
>>>>>> +{
>>>>>> + struct dma_buf_attachment *attach;
>>>>>> + struct drm_gem_shmem_object *shmem;
>>>>>> + size_t size;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (dma_buf->ops == &drm_gem_shmem_prime_dmabuf_ops) {
>>>>>> + struct drm_gem_object *obj;
>>>>>> +
>>>>>> + obj = dma_buf->priv;
>>>>>> + if (obj->dev == dev) {
>>>>>> + /*
>>>>>> + * Importing dmabuf exported from our own gem increases
>>>>>> + * refcount on gem itself instead of f_count of dmabuf.
>>>>>> + */
>>>>>> + drm_gem_object_get(obj);
>>>>>> + return obj;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + attach = dma_buf_attach(dma_buf, dev->dev);
>>>>>> + if (IS_ERR(attach))
>>>>>> + return ERR_CAST(attach);
>>>>>> +
>>>>>> + get_dma_buf(dma_buf);
>>>>>> +
>>>>>> + size = PAGE_ALIGN(attach->dmabuf->size);
>>>>>> +
>>>>>> + shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>>>>>> + if (IS_ERR(shmem)) {
>>>>>> + ret = PTR_ERR(shmem);
>>>>>> + goto fail_detach;
>>>>>> + }
>>>>>> +
>>>>>> + drm_dbg_prime(dev, "size = %zu\n", size);
>>>>>> +
>>>>>> + shmem->base.import_attach = attach;
>>>>>> + shmem->base.resv = dma_buf->resv;
>>>>>> +
>>>>>> + return &shmem->base;
>>>>>> +
>>>>>> +fail_detach:
>>>>>> + dma_buf_detach(dma_buf, attach);
>>>>>> + dma_buf_put(dma_buf);
>>>>>> +
>>>>>> + return ERR_PTR(ret);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import);
>>>>>> +
>>>>>> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>>>>> MODULE_IMPORT_NS("DMA_BUF");
>>>>>> MODULE_LICENSE("GPL v2");
>>>>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>>>>> index cef5a6b5a4d6..78ef91593a8e 100644
>>>>>> --- a/include/drm/drm_gem_shmem_helper.h
>>>>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>>>>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>>>>> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>>>>>> struct iosys_map *map);
>>>>>> int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
>>>>>> +struct dma_buf *drm_gem_shmem_prime_export(struct drm_gem_shmem_object *shmem,
>>>>>> + int flags);
>>>>>> int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem);
>>>>>> void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem);
>>>>>> @@ -168,6 +170,18 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>>>>>> drm_gem_shmem_print_info(shmem, p, indent);
>>>>>> }
>>>>>> +/**
>>>>>> + * drm_gem_shmem_object_prime_export - GEM object function for export()
>>>>>> + * @obj: GEM object
>>>>>> + *
>>>>>> + */
>>>>>> +static inline struct dma_buf *drm_gem_shmem_object_prime_export(struct drm_gem_object *obj,
>>>>>> + int flags)
>>>>>> +{
>>>>>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>>>>> +
>>>>>> + return drm_gem_shmem_prime_export(shmem, flags);
>>>>>> +}
>>>>>> /**
>>>>>> * drm_gem_shmem_object_pin - GEM object function for drm_gem_shmem_pin()
>>>>>> * @obj: GEM object
>>>>>> @@ -276,6 +290,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>>>> struct sg_table *sgt);
>>>>>> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>>>> struct drm_mode_create_dumb *args);
>>>>>> +struct drm_gem_object *drm_gem_shmem_prime_import(struct drm_device *dev,
>>>>>> + struct dma_buf *buf);
>>>>>> /**
>>>>>> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>>>>>> @@ -287,4 +303,12 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>>>> .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>>>>>> .dumb_create = drm_gem_shmem_dumb_create
>>>>>> +/**
>>>>>> + * This macro provides a shmem GEM operations that implementate a simple
>>>>>> + * gem_prime_import.
>>>>>> + */
>>>>>> +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>>>>>> + .gem_prime_import = drm_gem_shmem_prime_import, \
>>>>>> + .dumb_create = drm_gem_shmem_dumb_create
>>>>>> +
>>>>>> #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-05-05 15:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-01 6:43 [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table oushixiong1025
2025-05-01 6:43 ` [PATCH 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS oushixiong1025
2025-05-01 6:43 ` [PATCH 3/3] drm/udl: " oushixiong1025
2025-05-05 11:12 ` [PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table Thomas Zimmermann
2025-05-05 11:25 ` Christian König
2025-05-05 14:22 ` oushixiong
2025-05-05 14:32 ` Christian König
2025-05-05 15:11 ` oushixiong
2025-05-05 15:23 ` Christian König
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).