| 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: <c72cb0fc-867d-4c10-8af1-19a93c396f1d@163.com>
Date: Fri, 9 May 2025 16:24:03 +0800
From: Shixiong Ou <oushixiong1025@....com>
To: Thomas Zimmermann <tzimmermann@...e.de>,
Christian König <christian.koenig@....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
在 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@....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
>>> +
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);
>
Powered by blists - more mailing lists