Message-ID: <7cfa2fb3-f419-4037-8c86-7559570582e3@amd.com>
Date: Tue, 6 May 2025 14:10:07 +0200
From: Christian König <christian.koenig@....com>
To: oushixiong1025@....com
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Sean Paul <sean@...rly.run>, Jocelyn Falempe <jfalempe@...hat.com>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 Shixiong Ou <oushixiong@...inos.cn>
Subject: Re: [PATCH v2 1/3] drm/shmem-helper: Import dmabuf without mapping
 its sg_table

On 5/6/25 11:47, oushixiong1025@....com wrote:
> From: Shixiong Ou <oushixiong@...inos.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@...inos.cn>
> ---
> v1->v2:
> 	patch rebase.
> 
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>  include/drm/drm_gem_shmem_helper.h     | 25 +++++++
>  2 files changed, 120 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index aa43265f4f4f..0c81a4f97684 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,
> @@ -800,6 +801,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 =  {
> +	.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,
> +};

Ok, I think I got it now.

You basically don't want to block providing the sg_table, but rather just avoid that it is created al the time.

In that case duplicating the exporting side indeed doesn't make much sense.

> +
> +/**
> + * 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);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_export);

And that here is then identical to drm_gem_prime_export().

> +
> +/**
> + * 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)
> +{

This is the only function which actually has some functional difference.

> +	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;
> +		}
> +	}

And you have the other two just to be able to check the dma_buf->ops in this chunk here.

I suggest that you either expose drm_gem_prime_dmabuf_ops to drm_gem_shmem_helper.c or make this chunk a separate function which is called from drm_gem_shmem_prime_import().

And BTW you need a better name than drm_gem_shmem_prime_import(), something which makes clear that only vmap is supported on the buffer.

Apart from that this seems to make sense to me now,
Christian.

> +
> +	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 b4f993da3cae..f2b4cc85b7f9 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -121,6 +121,8 @@ int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
>  void drm_gem_shmem_vunmap_locked(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);
> @@ -179,6 +181,19 @@ 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
> @@ -287,6 +302,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
> @@ -298,4 +315,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__ */

Powered by blists - more mailing lists