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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <ef261695-79fa-4c0e-9894-6fe19de823bb@amd.com>
Date: Wed, 30 Apr 2025 16:48:16 +0200
From: Christian König <christian.koenig@....com>
To: oushixiong <oushixiong1025@....com>,
 Sumit Semwal <sumit.semwal@...aro.org>
Cc: linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org,
 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>,
 Dave Airlie <airlied@...hat.com>, Sean Paul <sean@...rly.run>,
 Shixiong Ou <oushixiong@...inos.cn>
Subject: Re: [PATCH 2/3] drm/prime: Support importing DMA-BUF without sg_table

On 4/30/25 16:13, oushixiong wrote:
> 
> 在 2025/4/30 19:03, Christian König 写道:
>> On 4/30/25 10:56,oushixiong1025@....com wrote:
>>> From: Shixiong Ou<oushixiong@...inos.cn>
>>>
>>> [WHY]
>>> On some boards, the dma_mask of Aspeed devices is 0xffff_ffff, this
>>> quite possibly causes the SWIOTLB to be triggered when importing dmabuf.
>>> However IO_TLB_SEGSIZE limits the maximum amount of available memory
>>> for DMA Streaming Mapping, as dmesg following:
>>>
>>> [   24.885303][ T1947] ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots)
>>>
>>> [HOW] Provide an interface so that attachment is not mapped when
>>> importing dma-buf.
>> This is unecessary. The extra abstraction in DRM is only useful when you want to implement the obj->funcs->get_sg_table() callback.
>>
>> When a driver doesn't want to expose an sg_table for a buffer or want some other special handling it can simply do so by implementing the DMA-buf interface directly.
>>
>> See drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c for an example on how to do this.
>>
>> Regards,
>> Christian.
> 
> 
> Thanks for the reminder,
> 
> most drivers that use DRM_GEM_SHADOW_PLANE_HELPER_FUNCSand DRM_GEM_SHMEM_DRIVER_OPS
> 
> don't need to import the sg_table, such as the udl and the ast and so on at the moment.
> 
> They just need to call dma_buf_vmap() to get the kernel virtual address of the shared buffer.
> 
> So I wondered if there was a simple generic PRIME implementation for these drivers.
> 
> If you don't recommend this, Maybe try to implement it in DRM_GEM_SHMEM_DRIVER_OPS ?

Well if you only want to implement vmap/vunmap the necessary code in the driver would look something like this:

const struct dma_buf_ops amdgpu_dmabuf_ops = {
        .map_dma_buf = dummy_map_function,
        .release = drm_gem_dmabuf_release,
        .mmap = drm_gem_dmabuf_mmap,
        .vmap = drm_gem_dmabuf_vmap,
        .vunmap = drm_gem_dmabuf_vunmap,
};

struct dma_buf *drv_gem_prime_export(struct drm_gem_object *gobj,
                                        int flags)
{
        struct dma_buf *buf;

        buf = drm_gem_prime_export(gobj, flags);
        if (!IS_ERR(buf))
                buf->ops = &amdgpu_dmabuf_ops;

        return buf;
}

The only thing which could be improved is the dummy_map_function. As far as I can see we could make the map function optional in DMA-buf now.

Apart from that you could make a DRM helper from that few lines, but to be honest I don't think it's worth it. It reduces the loc a bit, but there is no real complexity here which drivers could share.

Regards,
Christian.


> 
> Regards,
> 
> Shixiong Ou.
> 
>>> Signed-off-by: Shixiong Ou<oushixiong@...inos.cn>
>>> ---
>>>   drivers/gpu/drm/ast/ast_drv.c          |  2 +-
>>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 17 +++++++
>>>   drivers/gpu/drm/drm_prime.c            | 67 ++++++++++++++++++++++++--
>>>   drivers/gpu/drm/udl/udl_drv.c          |  2 +-
>>>   include/drm/drm_drv.h                  |  3 ++
>>>   include/drm/drm_gem_shmem_helper.h     |  6 +++
>>>   6 files changed, 91 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
>>> index 6fbf62a99c48..2dac6acf79e7 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.c
>>> +++ b/drivers/gpu/drm/ast/ast_drv.c
>>> @@ -64,7 +64,7 @@ static const struct drm_driver ast_driver = {
>>>       .minor = DRIVER_MINOR,
>>>       .patchlevel = DRIVER_PATCHLEVEL,
>>>   -    DRM_GEM_SHMEM_DRIVER_OPS,
>>> +    DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS,
>>>       DRM_FBDEV_SHMEM_DRIVER_OPS,
>>>   };
>>>   diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> index d99dee67353a..655d841df933 100644
>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> @@ -799,6 +799,23 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>   }
>>>   EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>>   +struct drm_gem_object *
>>> +drm_gem_shmem_prime_import_attachment(struct drm_device *dev,
>>> +                      struct dma_buf_attachment *attach)
>>> +{
>>> +    size_t size = PAGE_ALIGN(attach->dmabuf->size);
>>> +    struct drm_gem_shmem_object *shmem;
>>> +
>>> +    shmem = __drm_gem_shmem_create(dev, size, true, NULL);
>>> +    if (IS_ERR(shmem))
>>> +        return ERR_CAST(shmem);
>>> +
>>> +    drm_dbg_prime(dev, "size = %zu\n", size);
>>> +
>>> +    return &shmem->base;
>>> +}
>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_attachment);
>>> +
>>>   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 8e70abca33b9..522cf974e202 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -911,6 +911,62 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_prime_export);
>>>   +/**
>>> + * drm_gem_prime_import_dev_skip_map - core implementation of the import callback
>>> + * @dev: drm_device to import into
>>> + * @dma_buf: dma-buf object to import
>>> + * @attach_dev: struct device to dma_buf attach
>>> + *
>>> + * This function exports a dma-buf without get it's scatter/gather table.
>>> + *
>>> + * Drivers who need to get an scatter/gather table for objects need to call
>>> + * drm_gem_prime_import_dev() instead.
>>> + */
>>> +struct drm_gem_object *drm_gem_prime_import_dev_skip_map(struct drm_device *dev,
>>> +                             struct dma_buf *dma_buf,
>>> +                             struct device *attach_dev)
>>> +{
>>> +    struct dma_buf_attachment *attach;
>>> +    struct drm_gem_object *obj;
>>> +    int ret;
>>> +
>>> +    if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>>> +        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;
>>> +        }
>>> +    }
>>> +
>>> +    attach = dma_buf_attach(dma_buf, attach_dev, true);
>>> +    if (IS_ERR(attach))
>>> +        return ERR_CAST(attach);
>>> +
>>> +    get_dma_buf(dma_buf);
>>> +
>>> +    obj = dev->driver->gem_prime_import_attachment(dev, attach);
>>> +    if (IS_ERR(obj)) {
>>> +        ret = PTR_ERR(obj);
>>> +        goto fail_detach;
>>> +    }
>>> +
>>> +    obj->import_attach = attach;
>>> +    obj->resv = dma_buf->resv;
>>> +
>>> +    return obj;
>>> +
>>> +fail_detach:
>>> +    dma_buf_detach(dma_buf, attach);
>>> +    dma_buf_put(dma_buf);
>>> +
>>> +    return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_prime_import_dev_skip_map);
>>> +
>>>   /**
>>>    * drm_gem_prime_import_dev - core implementation of the import callback
>>>    * @dev: drm_device to import into
>>> @@ -946,9 +1002,6 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>>>           }
>>>       }
>>>   -    if (!dev->driver->gem_prime_import_sg_table)
>>> -        return ERR_PTR(-EINVAL);
>>> -
>>>       attach = dma_buf_attach(dma_buf, attach_dev, false);
>>>       if (IS_ERR(attach))
>>>           return ERR_CAST(attach);
>>> @@ -998,7 +1051,13 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev);
>>>   struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>>>                           struct dma_buf *dma_buf)
>>>   {
>>> -    return drm_gem_prime_import_dev(dev, dma_buf, dev->dev);
>>> +    if (dev->driver->gem_prime_import_sg_table)
>>> +        return drm_gem_prime_import_dev(dev, dma_buf, dev->dev);
>>> +    else if (dev->driver->gem_prime_import_attachment)
>>> +        return drm_gem_prime_import_dev_skip_map(dev, dma_buf, dev->dev);
>>> +    else
>>> +        return ERR_PTR(-EINVAL);
>>> +
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_prime_import);
>>>   diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>> index 05b3a152cc33..c00d8b8834f2 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>> @@ -72,7 +72,7 @@ static const struct drm_driver driver = {
>>>         /* GEM hooks */
>>>       .fops = &udl_driver_fops,
>>> -    DRM_GEM_SHMEM_DRIVER_OPS,
>>> +    DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS,
>>>       .gem_prime_import = udl_driver_gem_prime_import,
>>>       DRM_FBDEV_SHMEM_DRIVER_OPS,
>>>   diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index a43d707b5f36..aef8d9051fcd 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -326,6 +326,9 @@ struct drm_driver {
>>>                   struct dma_buf_attachment *attach,
>>>                   struct sg_table *sgt);
>>>   +    struct drm_gem_object *(*gem_prime_import_attachment)(
>>> +                struct drm_device *dev,
>>> +                struct dma_buf_attachment *attach);
>>>       /**
>>>        * @dumb_create:
>>>        *
>>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>>> index cef5a6b5a4d6..39a93c222aaa 100644
>>> --- a/include/drm/drm_gem_shmem_helper.h
>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>> @@ -274,6 +274,9 @@ struct drm_gem_object *
>>>   drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>>                       struct dma_buf_attachment *attach,
>>>                       struct sg_table *sgt);
>>> +struct drm_gem_object *
>>> +drm_gem_shmem_prime_import_attachment(struct drm_device *dev,
>>> +                      struct dma_buf_attachment *attach);
>>>   int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>>                     struct drm_mode_create_dumb *args);
>>>   @@ -287,4 +290,7 @@ 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
>>>   +#define DRM_GEM_SHMEM_SIMPLE_DRIVER_OPS \
>>> +    .gem_prime_import_attachment = drm_gem_shmem_prime_import_attachment, \
>>> +    .dumb_create                 = drm_gem_shmem_dumb_create
>>>   #endif /* __DRM_GEM_SHMEM_HELPER_H__ */


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ