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

Tue Apr 15 13:10:53 UTC 2025

On Tue, Apr 15, 2025 at 02:52:54PM +0200, Christian König wrote:
> 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.

Uh, given that we already have a confusion between in the importer and
exporter cases I think it'd be better to more strictly separate them than
to mix them up even more for convenience. We need more clarity here
instead.

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

Yeah this sounds better imo.

> > 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 im entirely unrelated because ...

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

... it's for the vmap only case, where you might not even have a struct
device. Or definitely not a reasonable one, like maybe a faux_bus device
or some device on a bus that really doesn't do dma (e.g. spi or i2c), and
where hence dma_buf_map_attachment is just something you never ever want
to do.

I think we might want to transform obj->import_attach into a union or
tagged pointer or something like that, which can cover both cases. And
maybe a drm_gem_bo_imported_dma_buf() helper that gives you the dma_buf no
matter what if it's imported, or NULL if it's allocated on that
drm_device?

Cheers, Sima

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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch