[PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[oushixiong1025]

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>
---
v1->v2:
	Patch rebase.
v2->v3:
	Rename the import callback function.
	Remove drm_gem_shmem_prime_export() and separate some codes
	to drm_gem_prime_import_self(). 
v3->v4:
	Separate the test from the policy.
	Rename the macro.

 drivers/gpu/drm/drm_gem_shmem_helper.c | 57 ++++++++++++++++++++++++++
 drivers/gpu/drm/drm_prime.c            | 36 ++++++++++++----
 include/drm/drm_gem_shmem_helper.h     | 15 +++++++
 include/drm/drm_prime.h                |  3 ++
 4 files changed, 102 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index aa43265f4f4f..8fa160c3635e 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -800,6 +800,63 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
 
+/**
+ * drm_gem_shmem_prime_import_no_sgt - 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_no_sgt(struct drm_device *dev,
+							 struct dma_buf *dma_buf)
+{
+	struct dma_buf_attachment *attach;
+	struct drm_gem_shmem_object *shmem;
+	struct drm_gem_object *obj;
+	size_t size;
+	int ret;
+
+	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
+		/*
+		 * Importing dmabuf exported from our own gem increases
+		 * refcount on gem itself instead of f_count of dmabuf.
+		 */
+		obj = dma_buf->priv;
+		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_no_sgt);
+
 MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
 MODULE_IMPORT_NS("DMA_BUF");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d828502268b8..5bcf483520b8 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -910,6 +910,26 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
 }
 EXPORT_SYMBOL(drm_gem_prime_export);
 
+
+/**
+ * drm_gem_prime_exported_dma_buf -
+ * checks if the DMA-BUF was exported from a GEM object belonging to @dev.
+ * @dev: drm_device to check against
+ * @dma_buf: dma-buf object to import
+ *
+ * Return: true if the DMA-BUF was exported from a GEM object belonging
+ * to @dev, false otherwise.
+ */
+
+bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
+				    struct dma_buf *dma_buf)
+{
+	struct drm_gem_object *obj = dma_buf->priv;
+
+	return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && (obj->dev == dev);
+}
+EXPORT_SYMBOL(drm_gem_prime_exported_dma_buf);
+
 /**
  * drm_gem_prime_import_dev - core implementation of the import callback
  * @dev: drm_device to import into
@@ -933,16 +953,14 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	int ret;
 
-	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
+	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
+		/*
+		 * Importing dmabuf exported from our own gem increases
+		 * refcount on gem itself instead of f_count of dmabuf.
+		 */
 		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;
-		}
+		drm_gem_object_get(obj);
+		return obj;
 	}
 
 	if (!dev->driver->gem_prime_import_sg_table)
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index b4f993da3cae..9ee697ff52ea 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -287,6 +287,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_no_sgt(struct drm_device *dev,
+							 struct dma_buf *buf);
 
 /**
  * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
@@ -298,4 +300,17 @@ 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
 
+/**
+ * DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT - shmem GEM operations
+ *                                   without mapping sg_table on
+ *                                   imported buffer.
+ *
+ * This macro provides a shortcut for setting the shmem GEM operations in
+ * the &drm_driver structure for drivers that do not require a sg_table on
+ * imported buffers.
+ */
+#define DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT \
+	.gem_prime_import       = drm_gem_shmem_prime_import_no_sgt, \
+	.dumb_create            = drm_gem_shmem_dumb_create
+
 #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index fa085c44d4ca..ec83f1c077a4 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -100,6 +100,9 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
 unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
 
 /* helper functions for importing */
+bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
+				    struct dma_buf *dma_buf);
+
 struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 						struct dma_buf *dma_buf,
 						struct device *attach_dev);
-- 
2.17.1

[PATCH v4 2/3] drm/ast: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS

[oushixiong1025]

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..8efb8ec89f50 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_DRIVER_OPS_NO_SGT,
 	DRM_FBDEV_SHMEM_DRIVER_OPS,
 };
 
-- 
2.17.1

[PATCH v4 3/3] drm/udl: use DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS

[oushixiong1025]

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 1922988625eb..9c385771b483 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -57,7 +57,7 @@ static const struct drm_driver driver = {
 
 	/* GEM hooks */
 	.fops = &udl_driver_fops,
-	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT,
 	DRM_FBDEV_SHMEM_DRIVER_OPS,
 
 	.name = DRIVER_NAME,
-- 
2.17.1

Re: [PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[Christian König]

On 5/9/25 05:20, oushixiong1025@163.com wrote:
> 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>
> ---
> v1->v2:
> 	Patch rebase.
> v2->v3:
> 	Rename the import callback function.
> 	Remove drm_gem_shmem_prime_export() and separate some codes
> 	to drm_gem_prime_import_self(). 
> v3->v4:
> 	Separate the test from the policy.
> 	Rename the macro.
> 
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 57 ++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_prime.c            | 36 ++++++++++++----
>  include/drm/drm_gem_shmem_helper.h     | 15 +++++++
>  include/drm/drm_prime.h                |  3 ++
>  4 files changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index aa43265f4f4f..8fa160c3635e 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -800,6 +800,63 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>  
> +/**
> + * drm_gem_shmem_prime_import_no_sgt - 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_no_sgt(struct drm_device *dev,
> +							 struct dma_buf *dma_buf)


Please don't mention "no sgt" in the name, that we use sgtable is an implementation detail.

Maybe use something like "no map" or similar.


> +{
> +	struct dma_buf_attachment *attach;
> +	struct drm_gem_shmem_object *shmem;
> +	struct drm_gem_object *obj;
> +	size_t size;
> +	int ret;
> +
> +	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
> +		/*
> +		 * Importing dmabuf exported from our own gem increases
> +		 * refcount on gem itself instead of f_count of dmabuf.
> +		 */
> +		obj = dma_buf->priv;
> +		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_no_sgt);
> +
>  MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>  MODULE_IMPORT_NS("DMA_BUF");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index d828502268b8..5bcf483520b8 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -910,6 +910,26 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>  }
>  EXPORT_SYMBOL(drm_gem_prime_export);
>  
> +
> +/**
> + * drm_gem_prime_exported_dma_buf -
> + * checks if the DMA-BUF was exported from a GEM object belonging to @dev.
> + * @dev: drm_device to check against
> + * @dma_buf: dma-buf object to import
> + *
> + * Return: true if the DMA-BUF was exported from a GEM object belonging
> + * to @dev, false otherwise.
> + */
> +
> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
> +				    struct dma_buf *dma_buf)

If I remember the naming conventions correctly this should be something like drm_gem_is_prime_exported_dma_buf().

Regards,
Christian.

> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +
> +	return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && (obj->dev == dev);
> +}
> +EXPORT_SYMBOL(drm_gem_prime_exported_dma_buf);
> +
>  /**
>   * drm_gem_prime_import_dev - core implementation of the import callback
>   * @dev: drm_device to import into
> @@ -933,16 +953,14 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	int ret;
>  
> -	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
> +	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
> +		/*
> +		 * Importing dmabuf exported from our own gem increases
> +		 * refcount on gem itself instead of f_count of dmabuf.
> +		 */
>  		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;
> -		}
> +		drm_gem_object_get(obj);
> +		return obj;
>  	}
>  
>  	if (!dev->driver->gem_prime_import_sg_table)
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index b4f993da3cae..9ee697ff52ea 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -287,6 +287,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_no_sgt(struct drm_device *dev,
> +							 struct dma_buf *buf);
>  
>  /**
>   * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
> @@ -298,4 +300,17 @@ 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
>  
> +/**
> + * DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT - shmem GEM operations
> + *                                   without mapping sg_table on
> + *                                   imported buffer.
> + *
> + * This macro provides a shortcut for setting the shmem GEM operations in
> + * the &drm_driver structure for drivers that do not require a sg_table on
> + * imported buffers.
> + */
> +#define DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT \
> +	.gem_prime_import       = drm_gem_shmem_prime_import_no_sgt, \
> +	.dumb_create            = drm_gem_shmem_dumb_create
> +
>  #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index fa085c44d4ca..ec83f1c077a4 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -100,6 +100,9 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>  unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
>  
>  /* helper functions for importing */
> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
> +				    struct dma_buf *dma_buf);
> +
>  struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  						struct dma_buf *dma_buf,
>  						struct device *attach_dev);

Re: [PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[Thomas Zimmermann]

Hi

Am 09.05.25 um 08:22 schrieb Christian König:
> On 5/9/25 05:20, oushixiong1025@163.com wrote:
>> 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>
>> ---
>> v1->v2:
>> 	Patch rebase.
>> v2->v3:
>> 	Rename the import callback function.
>> 	Remove drm_gem_shmem_prime_export() and separate some codes
>> 	to drm_gem_prime_import_self().
>> v3->v4:
>> 	Separate the test from the policy.
>> 	Rename the macro.
>>
>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 57 ++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_prime.c            | 36 ++++++++++++----
>>   include/drm/drm_gem_shmem_helper.h     | 15 +++++++
>>   include/drm/drm_prime.h                |  3 ++
>>   4 files changed, 102 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index aa43265f4f4f..8fa160c3635e 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -800,6 +800,63 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>   
>> +/**
>> + * drm_gem_shmem_prime_import_no_sgt - 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_no_sgt(struct drm_device *dev,
>> +							 struct dma_buf *dma_buf)
>
> Please don't mention "no sgt" in the name, that we use sgtable is an implementation detail.
>
> Maybe use something like "no map" or similar.

To be fair, I asked to not named it something like _vmap(), but rather 
_no_sgt().  These vmap-only names purely describe a use case. I'd be OK 
with any name that refers to the difference between the various 
functions; not just their effect.

>
>
>> +{
>> +	struct dma_buf_attachment *attach;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct drm_gem_object *obj;
>> +	size_t size;
>> +	int ret;
>> +
>> +	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
>> +		/*
>> +		 * Importing dmabuf exported from our own gem increases
>> +		 * refcount on gem itself instead of f_count of dmabuf.
>> +		 */
>> +		obj = dma_buf->priv;
>> +		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;
>> +	}

Unrelated to this series, we might reconsider 
drm_driver.gem_prime_import_sg_table. If we move the call to 
dma_buf_map_attachment_unlocked() into the callback and rename it to 
gem_prime_import_attachment, using sg tables would become optional for 
all drivers. SHMEM would be able to create the object without SG table 
without having to reimplement the prime boiler-plate code. Thoughts?


>> +
>> +	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_no_sgt);
>> +
>>   MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>   MODULE_IMPORT_NS("DMA_BUF");
>>   MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index d828502268b8..5bcf483520b8 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -910,6 +910,26 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>>   }
>>   EXPORT_SYMBOL(drm_gem_prime_export);
>>   
>> +
>> +/**
>> + * drm_gem_prime_exported_dma_buf -
>> + * checks if the DMA-BUF was exported from a GEM object belonging to @dev.
>> + * @dev: drm_device to check against
>> + * @dma_buf: dma-buf object to import
>> + *
>> + * Return: true if the DMA-BUF was exported from a GEM object belonging
>> + * to @dev, false otherwise.
>> + */
>> +
>> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
>> +				    struct dma_buf *dma_buf)
> If I remember the naming conventions correctly this should be something like drm_gem_is_prime_exported_dma_buf().

Again my fault. But the name you suggested sounds correct.

Best regards
Thomas

>
> Regards,
> Christian.
>
>> +{
>> +	struct drm_gem_object *obj = dma_buf->priv;
>> +
>> +	return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && (obj->dev == dev);
>> +}
>> +EXPORT_SYMBOL(drm_gem_prime_exported_dma_buf);
>> +
>>   /**
>>    * drm_gem_prime_import_dev - core implementation of the import callback
>>    * @dev: drm_device to import into
>> @@ -933,16 +953,14 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>>   	struct drm_gem_object *obj;
>>   	int ret;
>>   
>> -	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>> +	if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
>> +		/*
>> +		 * Importing dmabuf exported from our own gem increases
>> +		 * refcount on gem itself instead of f_count of dmabuf.
>> +		 */
>>   		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;
>> -		}
>> +		drm_gem_object_get(obj);
>> +		return obj;
>>   	}
>>   
>>   	if (!dev->driver->gem_prime_import_sg_table)
>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>> index b4f993da3cae..9ee697ff52ea 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -287,6 +287,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_no_sgt(struct drm_device *dev,
>> +							 struct dma_buf *buf);
>>   
>>   /**
>>    * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>> @@ -298,4 +300,17 @@ 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
>>   
>> +/**
>> + * DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT - shmem GEM operations
>> + *                                   without mapping sg_table on
>> + *                                   imported buffer.
>> + *
>> + * This macro provides a shortcut for setting the shmem GEM operations in
>> + * the &drm_driver structure for drivers that do not require a sg_table on
>> + * imported buffers.
>> + */
>> +#define DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT \
>> +	.gem_prime_import       = drm_gem_shmem_prime_import_no_sgt, \
>> +	.dumb_create            = drm_gem_shmem_dumb_create
>> +
>>   #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
>> index fa085c44d4ca..ec83f1c077a4 100644
>> --- a/include/drm/drm_prime.h
>> +++ b/include/drm/drm_prime.h
>> @@ -100,6 +100,9 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>>   unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
>>   
>>   /* helper functions for importing */
>> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
>> +				    struct dma_buf *dma_buf);
>> +
>>   struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>>   						struct dma_buf *dma_buf,
>>   						struct device *attach_dev);

-- 
--
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)

Re: [PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[Shixiong Ou]

在 2025/5/9 14:51, Thomas Zimmermann 写道:
> Hi
>
> Am 09.05.25 um 08:22 schrieb Christian König:
>> On 5/9/25 05:20, oushixiong1025@163.com wrote:
>>> 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>
>>> ---
>>> v1->v2:
>>>     Patch rebase.
>>> v2->v3:
>>>     Rename the import callback function.
>>>     Remove drm_gem_shmem_prime_export() and separate some codes
>>>     to drm_gem_prime_import_self().
>>> v3->v4:
>>>     Separate the test from the policy.
>>>     Rename the macro.
>>>
>>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 57 
>>> ++++++++++++++++++++++++++
>>>   drivers/gpu/drm/drm_prime.c            | 36 ++++++++++++----
>>>   include/drm/drm_gem_shmem_helper.h     | 15 +++++++
>>>   include/drm/drm_prime.h                |  3 ++
>>>   4 files changed, 102 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c 
>>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> index aa43265f4f4f..8fa160c3635e 100644
>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> @@ -800,6 +800,63 @@ drm_gem_shmem_prime_import_sg_table(struct 
>>> drm_device *dev,
>>>   }
>>>   EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>>   +/**
>>> + * drm_gem_shmem_prime_import_no_sgt - 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_no_sgt(struct 
>>> drm_device *dev,
>>> +                             struct dma_buf *dma_buf)
>>
>> Please don't mention "no sgt" in the name, that we use sgtable is an 
>> implementation detail.
>>
>> Maybe use something like "no map" or similar.
>
> To be fair, I asked to not named it something like _vmap(), but rather 
> _no_sgt().  These vmap-only names purely describe a use case. I'd be 
> OK with any name that refers to the difference between the various 
> functions; not just their effect.
>
>>
>>
>>> +{
>>> +    struct dma_buf_attachment *attach;
>>> +    struct drm_gem_shmem_object *shmem;
>>> +    struct drm_gem_object *obj;
>>> +    size_t size;
>>> +    int ret;
>>> +
>>> +    if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
>>> +        /*
>>> +         * Importing dmabuf exported from our own gem increases
>>> +         * refcount on gem itself instead of f_count of dmabuf.
>>> +         */
>>> +        obj = dma_buf->priv;
>>> +        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;
>>> +    }
>
> Unrelated to this series, we might reconsider 
> drm_driver.gem_prime_import_sg_table. If we move the call to 
> dma_buf_map_attachment_unlocked() into the callback and rename it to 
> gem_prime_import_attachment, using sg tables would become optional for 
> all drivers. SHMEM would be able to create the object without SG table 
> without having to reimplement the prime boiler-plate code. Thoughts?
>
>
>>> +
>>> +    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_no_sgt);
>>> +
>>>   MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>>>   MODULE_IMPORT_NS("DMA_BUF");
>>>   MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index d828502268b8..5bcf483520b8 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -910,6 +910,26 @@ struct dma_buf *drm_gem_prime_export(struct 
>>> drm_gem_object *obj,
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_prime_export);
>>>   +
>>> +/**
>>> + * drm_gem_prime_exported_dma_buf -
>>> + * checks if the DMA-BUF was exported from a GEM object belonging 
>>> to @dev.
>>> + * @dev: drm_device to check against
>>> + * @dma_buf: dma-buf object to import
>>> + *
>>> + * Return: true if the DMA-BUF was exported from a GEM object 
>>> belonging
>>> + * to @dev, false otherwise.
>>> + */
>>> +
>>> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
>>> +                    struct dma_buf *dma_buf)
>> If I remember the naming conventions correctly this should be 
>> something like drm_gem_is_prime_exported_dma_buf().
>
> Again my fault. But the name you suggested sounds correct.
>
> Best regards
> Thomas
>
>>
>> Regards,
>> Christian.
>>
>>> +{
>>> +    struct drm_gem_object *obj = dma_buf->priv;
>>> +
>>> +    return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && (obj->dev 
>>> == dev);
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_prime_exported_dma_buf);
>>> +
>>>   /**
>>>    * drm_gem_prime_import_dev - core implementation of the import 
>>> callback
>>>    * @dev: drm_device to import into
>>> @@ -933,16 +953,14 @@ struct drm_gem_object 
>>> *drm_gem_prime_import_dev(struct drm_device *dev,
>>>       struct drm_gem_object *obj;
>>>       int ret;
>>>   -    if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>>> +    if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) {
>>> +        /*
>>> +         * Importing dmabuf exported from our own gem increases
>>> +         * refcount on gem itself instead of f_count of dmabuf.
>>> +         */
>>>           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;
>>> -        }
>>> +        drm_gem_object_get(obj);
>>> +        return obj;
>>>       }
>>>         if (!dev->driver->gem_prime_import_sg_table)
>>> diff --git a/include/drm/drm_gem_shmem_helper.h 
>>> b/include/drm/drm_gem_shmem_helper.h
>>> index b4f993da3cae..9ee697ff52ea 100644
>>> --- a/include/drm/drm_gem_shmem_helper.h
>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>> @@ -287,6 +287,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_no_sgt(struct 
>>> drm_device *dev,
>>> +                             struct dma_buf *buf);
>>>     /**
>>>    * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
>>> @@ -298,4 +300,17 @@ 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
>>>   +/**
>>> + * DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT - shmem GEM operations
>>> + *                                   without mapping sg_table on
>>> + *                                   imported buffer.
>>> + *
>>> + * This macro provides a shortcut for setting the shmem GEM 
>>> operations in
>>> + * the &drm_driver structure for drivers that do not require a 
>>> sg_table on
>>> + * imported buffers.
>>> + */
>>> +#define DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT \
>>> +    .gem_prime_import       = drm_gem_shmem_prime_import_no_sgt, \
>>> +    .dumb_create            = drm_gem_shmem_dumb_create
>>> +

Name DRM_GEM_SHMEM_DRIVER_OPS_NO_MAP or 
DRM_GEM_SHMEM_DRIVER_OPS_NO_MAP_SGT ?

Best regards
Shixiong Ou.

>>>   #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
>>> index fa085c44d4ca..ec83f1c077a4 100644
>>> --- a/include/drm/drm_prime.h
>>> +++ b/include/drm/drm_prime.h
>>> @@ -100,6 +100,9 @@ struct dma_buf *drm_gem_prime_export(struct 
>>> drm_gem_object *obj,
>>>   unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
>>>     /* helper functions for importing */
>>> +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev,
>>> +                    struct dma_buf *dma_buf);
>>> +
>>>   struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device 
>>> *dev,
>>>                           struct dma_buf *dma_buf,
>>>                           struct device *attach_dev);
>

Re: [PATCH v4 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table

[Christian König]

On 5/9/25 08:51, Thomas Zimmermann wrote:
>>> +struct drm_gem_object *drm_gem_shmem_prime_import_no_sgt(struct drm_device *dev,
>>> +                             struct dma_buf *dma_buf)
>>
>> Please don't mention "no sgt" in the name, that we use sgtable is an implementation detail.
>>
>> Maybe use something like "no map" or similar.
> 
> To be fair, I asked to not named it something like _vmap(), but rather _no_sgt().  These vmap-only names purely describe a use case. I'd be OK with any name that refers to the difference between the various functions; not just their effect.

Yeah, but "no sgt" is as equally bad as vmap. I don't know a good naming of hand either.

> 
> Unrelated to this series, we might reconsider drm_driver.gem_prime_import_sg_table. If we move the call to dma_buf_map_attachment_unlocked() into the callback and rename it to gem_prime_import_attachment, using sg tables would become optional for all drivers. SHMEM would be able to create the object without SG table without having to reimplement the prime boiler-plate code. Thoughts?

I'm not sure if that would actually be better, but calling dma_buf_map_attachment() should indeed only be done when necessary.

For amdgpu we put that into the TTM TT backend (see function amdgpu_ttm_backend_bind) and I considered switching over radeon and nouveau as well but then just abandoned the plan as to much work.

Regards,
Christian.