[PATCH 1/3] drm/shmem-helper: Import dmabuf without mapping its sg_table
Christian König
christian.koenig at amd.com
Mon May 5 15:23:40 UTC 2025
Hi Shixiong,
could also be that I didn't fully got what you try to do here.
Between which drivers and implementations are the BOs shared exactly?
You could also just rebase your patches on drm-misc-next and send them out again and I can take another look.
Regards,
Christian.
On 5/5/25 17:11, oushixiong wrote:
> Hi Christian,
>
> I don't see strong relevance between my patch and the patches you're referring to.
> Because the drm_gem_prime_import() function imports sg_table by default, my patch provides an alternative
> import callback implementation for SHMEM-based drivers that differs from drm_gem_prime_import().
> drm_gem_shmem_prime_import_sg_table() doesn't need to call dma_buf_map_attachment_unlocked() to import sg_table.
>
> Alternatively, I might not have fully understood the patches you mentioned.
>
>
> Thanks and Regards,
>
> Shixiong.
>
> 在 2025/5/5 22:32, Christian König 写道:
>> Hi Shixiong,
>>
>> for drm changes please base your patches on drm-misc-next or drm-next.
>>
>> That is probably fixed by this one here:
>>
>> commit b72f66f22c0e39ae6684c43fead774c13db24e73
>> Author: Christian König <christian.koenig at amd.com>
>> Date: Tue Feb 11 17:20:53 2025 +0100
>>
>> dma-buf: drop caching of sg_tables
>> That was purely for the transition from static to dynamic dma-buf
>> handling and can be removed again now.
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> Reviewed-by: Simona Vetter <simona.vetter at ffwll.ch>
>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko at collabora.com>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20250211163109.12200-5-christian.koenig@amd.com
>>
>> After this patch SG tables where only created when necessary.
>>
>> Regards,
>> Christian.
>>
>> On 5/5/25 16:22, oushixiong wrote:
>>> Hi Christian,
>>>
>>> My patch is based on linux-next, so this patch is not based on the latest code. Then, I'd like to ask which patch resolved the issue with sg-tables?
>>>
>>>
>>> Thanks and Regards,
>>>
>>> Shixiong.
>>>
>>>
>>> 在 2025/5/5 19:25, Christian König 写道:
>>>> Hi Thomas & Shixiong,
>>>>
>>>> first of all the patch is still based on outdated code. For example the cache_sgt_mapping member is already removed in drm-misc-next.
>>>>
>>>> So if I'm not completely mistaken the issue is already resolved upstream.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> On 5/5/25 13:12, Thomas Zimmermann wrote:
>>>>> (cc'ing Christian)
>>>>>
>>>>> Hi,
>>>>>
>>>>> I don't feel qualified to fully review this patch.
>>>>>
>>>>> It would be good to have the issue with sg-tables solved, but I dislike the dedicated initializer macros. So my question is if this has any drawbacks. Or could we make this available and the default for all shmem-based drivers?
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>> Am 01.05.25 um 08:43 schrieb oushixiong1025 at 163.com:
>>>>>> From: Shixiong Ou <oushixiong at 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 at kylinos.cn>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 95 ++++++++++++++++++++++++++
>>>>>> include/drm/drm_gem_shmem_helper.h | 24 +++++++
>>>>>> 2 files changed, 119 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>> index d99dee67353a..9e41e350ff6f 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,
>>>>>> @@ -799,6 +800,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 = {
>>>>>> + .cache_sgt_mapping = true,
>>>>>> + .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,
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>> + * 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);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * 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)
>>>>>> +{
>>>>>> + 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;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + 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 cef5a6b5a4d6..78ef91593a8e 100644
>>>>>> --- a/include/drm/drm_gem_shmem_helper.h
>>>>>> +++ b/include/drm/drm_gem_shmem_helper.h
>>>>>> @@ -110,6 +110,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>>>>> void drm_gem_shmem_vunmap(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);
>>>>>> @@ -168,6 +170,18 @@ 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
>>>>>> @@ -276,6 +290,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
>>>>>> @@ -287,4 +303,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__ */
>
More information about the dri-devel
mailing list