[PATCH] drm/gem: Internally test import_attach for imported objects

[Christian König]

Am 15.04.25 um 14:40 schrieb Thomas Zimmermann:
> Hi
>
> Am 15.04.25 um 14:19 schrieb Christian König:
>> Am 15.04.25 um 12:45 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 15.04.25 um 11:39 schrieb Christian König:
>>>> Am 15.04.25 um 11:20 schrieb Thomas Zimmermann:
>>>>> Test struct drm_gem_object.import_attach to detect imported objects
>>>>> during cleanup. At that point, the imported DMA buffer might have
>>>>> already been released and the dma_buf field is NULL. The object's
>>>>> free callback then does a cleanup as for native objects.
>>>> I don't think that this is a good idea.
>>>>
>>>> The DMA-buf is separately reference counted through the import attachment.
>>> I understand that it's not ideal, but testing for import_attach to be set is what we currently do throughout drivers. Putting this behind an interface is already a step forward.
>>>
>>>>> Happens for calls to drm_mode_destroy_dumb_ioctl() that eventually
>>>>> clear the dma_buf field in drm_gem_object_exported_dma_buf_free().
>>>> That is for exported DMA-buf and should *NEVER* be used for imported ones.
>>> Did you look at the discussion at the Closes tag? Where else could dma-buf be cleared?
>> Yeah, I've seen that. The solution is just completely wrong.
>>
>> See for the export case obj->dma_buf points to the exported DMA-buf and causes a circle dependency when not set to NULL when the last handle is released.
>>
>> But for the import case obj->dma_buf is actually not that relevant. Instead obj->import_attach->dma_buf should be used.
>
> So if I understand correctly, the tests in that helper should be done by looking at import_attach->dma_buf.

At least in theory yes. IIRC we also set obj->dma_buf to the same value as import_attach->dma_buf for convenient so that code doesn't need to check both.

But it can be that obj->dma_buf is already NULL while the import attachment is still in the process of being cleaned up. So there are a couple of cases where we have to look at obj->import_attach->dma_buf.

> The long-term goal is to make import_attach optional because its setup requires a DMA-capable device.

HUI WHAT?

Dmitry and I put quite some effort into being able to create an import_attach without the requirement to have a DMA-capable device.

The last puzzle piece of that landed a month ago in the form of this patch 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

When you don't create an import attachment the exporter wouldn't know that his buffer is actually used which is usually a quite bad idea.

This is for devices who only want to do a vmap of the buffer, isn't it?

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> Regards,
>> Christian.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>>> Reported-by: Andy Yan <andyshrk at 163.com>
>>>>> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
>>>>> Tested-by: Andy Yan <andyshrk at 163.com>
>>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>>>> Cc: Anusha Srivatsa <asrivats at redhat.com>
>>>>> Cc: Christian König <christian.koenig at amd.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>> Cc: Maxime Ripard <mripard at kernel.org>
>>>>> Cc: David Airlie <airlied at gmail.com>
>>>>> Cc: Simona Vetter <simona at ffwll.ch>
>>>>> Cc: Sumit Semwal <sumit.semwal at linaro.org>
>>>>> Cc: "Christian König" <christian.koenig at amd.com>
>>>>> Cc: dri-devel at lists.freedesktop.org
>>>>> Cc: linux-media at vger.kernel.org
>>>>> Cc: linaro-mm-sig at lists.linaro.org
>>>>> ---
>>>>>    include/drm/drm_gem.h | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>> index 9b71f7a9f3f8..f09b8afcf86d 100644
>>>>> --- a/include/drm/drm_gem.h
>>>>> +++ b/include/drm/drm_gem.h
>>>>> @@ -589,7 +589,13 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
>>>>>    static inline bool drm_gem_is_imported(const struct drm_gem_object *obj)
>>>>>    {
>>>>>        /* The dma-buf's priv field points to the original GEM object. */
>>>>> -    return obj->dma_buf && (obj->dma_buf->priv != obj);
>>>>> +    return (obj->dma_buf && (obj->dma_buf->priv != obj)) ||
>>>>> +           /*
>>>>> +        * TODO: During object release, the dma-buf might already
>>>>> +        *       be gone. For now keep testing import_attach, but
>>>>> +        *       this should be removed at some point.
>>>>> +        */
>>>>> +           obj->import_attach;
>>>>>    }
>>>>>      #ifdef CONFIG_LOCKDEP
>