linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).