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

[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.

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

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

-- 
--
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)