| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a93a62b0-3252-4fed-b634-18a2e097ab7d@suse.de>
Date: Fri, 9 May 2025 08:51:57 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Christian König <christian.koenig@....com>,
oushixiong1025@....com
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, 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 v4 1/3] drm/shmem-helper: Import dmabuf without mapping
its sg_table
Hi
Am 09.05.25 um 08:22 schrieb Christian König:
> On 5/9/25 05:20, 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.
>> 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)
Powered by blists - more mailing lists