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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 22.05.25 um 09:07 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>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Looks good to me, but Christian might want to take a look as well. 
Thanks for addressing the issue.

Best regards
Thomas

> ---
> 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.
> v4->v5:
> 	Rename some functions.
>
>   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..126aa79042ad 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_map - 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_map(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_is_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_map);
> +
>   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..b825b71038d6 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_is_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_is_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_is_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_is_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..35f7466dca84 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_map(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_MAP_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_MAP_SGT \
> +	.gem_prime_import       = drm_gem_shmem_prime_import_no_map, \
> +	.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..f50f862f0d8b 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_is_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)