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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)