drm/gem: Internally test import_attach for imported objects

Commit Message

Thomas Zimmermann April 15, 2025, 9:20 a.m. UTC

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.

Happens for calls to drm_mode_destroy_dumb_ioctl() that eventually
clear the dma_buf field in drm_gem_object_exported_dma_buf_free().

Signed-off-by: Thomas Zimmermann <[e-mail]>
Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
Reported-by: Andy Yan <[e-mail]>
Closes: https://lore.kernel.org/dri-devel/[e-mail]/
Tested-by: Andy Yan <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Anusha Srivatsa <[e-mail]>
Cc: Christian König <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Simona Vetter <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: "Christian König" <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 include/drm/drm_gem.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Christian König April 15, 2025, 9:39 a.m. UTC | #1

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.

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

Regards,
Christian.

>
> Signed-off-by: Thomas Zimmermann <[e-mail]>
> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> Reported-by: Andy Yan <[e-mail]>
> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
> Tested-by: Andy Yan <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Anusha Srivatsa <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Simona Vetter <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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 April 15, 2025, 10:45 a.m. UTC | #2

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?

Best regards
Thomas

>
> Regards,
> Christian.
>
>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>> Reported-by: Andy Yan <[e-mail]>
>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>> Tested-by: Andy Yan <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: Anusha Srivatsa <[e-mail]>
>> Cc: Christian König <[e-mail]>
>> Cc: Maarten Lankhorst <[e-mail]>
>> Cc: Maxime Ripard <[e-mail]>
>> Cc: David Airlie <[e-mail]>
>> Cc: Simona Vetter <[e-mail]>
>> Cc: Sumit Semwal <[e-mail]>
>> Cc: "Christian König" <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>   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

Christian König April 15, 2025, 12:19 p.m. UTC | #3

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.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>> Reported-by: Andy Yan <[e-mail]>
>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>>> Tested-by: Andy Yan <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: Anusha Srivatsa <[e-mail]>
>>> Cc: Christian König <[e-mail]>
>>> Cc: Maarten Lankhorst <[e-mail]>
>>> Cc: Maxime Ripard <[e-mail]>
>>> Cc: David Airlie <[e-mail]>
>>> Cc: Simona Vetter <[e-mail]>
>>> Cc: Sumit Semwal <[e-mail]>
>>> Cc: "Christian König" <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>   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 April 15, 2025, 12:40 p.m. UTC | #4

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 <[e-mail]>
>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>> Reported-by: Andy Yan <[e-mail]>
>>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>>>> Tested-by: Andy Yan <[e-mail]>
>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>> Cc: Anusha Srivatsa <[e-mail]>
>>>> Cc: Christian König <[e-mail]>
>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>> Cc: Maxime Ripard <[e-mail]>
>>>> Cc: David Airlie <[e-mail]>
>>>> Cc: Simona Vetter <[e-mail]>
>>>> Cc: Sumit Semwal <[e-mail]>
>>>> Cc: "Christian König" <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> ---
>>>>    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

Christian König April 15, 2025, 12:52 p.m. UTC | #5

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 <[e-mail]>
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 <[e-mail]>
    Reviewed-by: Simona Vetter <[e-mail]>
    Reviewed-by: Dmitry Osipenko <[e-mail]>
    Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]

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 <[e-mail]>
>>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>>> Reported-by: Andy Yan <[e-mail]>
>>>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>>>>> Tested-by: Andy Yan <[e-mail]>
>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>> Cc: Anusha Srivatsa <[e-mail]>
>>>>> Cc: Christian König <[e-mail]>
>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>> Cc: David Airlie <[e-mail]>
>>>>> Cc: Simona Vetter <[e-mail]>
>>>>> Cc: Sumit Semwal <[e-mail]>
>>>>> Cc: "Christian König" <[e-mail]>
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> ---
>>>>>    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 April 15, 2025, 1:10 p.m. UTC | #6

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 <[e-mail]>
> 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 <[e-mail]>
>     Reviewed-by: Simona Vetter <[e-mail]>
>     Reviewed-by: Dmitry Osipenko <[e-mail]>
>     Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
> 
> 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 <[e-mail]>
> >>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> >>>>> Reported-by: Andy Yan <[e-mail]>
> >>>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
> >>>>> Tested-by: Andy Yan <[e-mail]>
> >>>>> Cc: Thomas Zimmermann <[e-mail]>
> >>>>> Cc: Anusha Srivatsa <[e-mail]>
> >>>>> Cc: Christian König <[e-mail]>
> >>>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>>> Cc: Maxime Ripard <[e-mail]>
> >>>>> Cc: David Airlie <[e-mail]>
> >>>>> Cc: Simona Vetter <[e-mail]>
> >>>>> Cc: Sumit Semwal <[e-mail]>
> >>>>> Cc: "Christian König" <[e-mail]>
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> ---
> >>>>>    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 April 15, 2025, 1:14 p.m. UTC | #7

Hi

Am 15.04.25 um 14:52 schrieb 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.

Ok, that should also solve the problem for now. The point here is to 
limit exposure of import_attach.

>
>> 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 <[e-mail]>
> 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 <[e-mail]>
>      Reviewed-by: Simona Vetter <[e-mail]>
>      Reviewed-by: Dmitry Osipenko <[e-mail]>
>      Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
>
> 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?

Yeah, the vmap needs the sgtable, which needs import_attach IIRC. 
Somewhere in there a DMA device is required. But it's not of high 
priority as we have workarounds.

Best regards
Thomas

>
> Regards,
> Christian.
>
>> Best regards
>> Thomas
>>
>>> Regards,
>>> Christian.
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>>>> Reported-by: Andy Yan <[e-mail]>
>>>>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>>>>>> Tested-by: Andy Yan <[e-mail]>
>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>> Cc: Anusha Srivatsa <[e-mail]>
>>>>>> Cc: Christian König <[e-mail]>
>>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>>> Cc: David Airlie <[e-mail]>
>>>>>> Cc: Simona Vetter <[e-mail]>
>>>>>> Cc: Sumit Semwal <[e-mail]>
>>>>>> Cc: "Christian König" <[e-mail]>
>>>>>> Cc: [e-mail]
>>>>>> Cc: [e-mail]
>>>>>> Cc: [e-mail]
>>>>>> ---
>>>>>>     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

Boris Brezillon April 15, 2025, 1:15 p.m. UTC | #8

On Tue, 15 Apr 2025 14:19:20 +0200
Christian König <[e-mail]> wrote:

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

I can confirm this is causing a regression in Panthor, where the driver
holds references to GEMs that might have been released by userspace, so
it's not just drivers calling drm_mode_destroy_dumb_ioctl(). This leads
drm_gem_is_imported() to return inconsistent values depending on when
the function is called (before/after the handle creation/destruction).
This patch seems to fix the problem, but I can't tell if it's the right
thing to do.

Christian König April 15, 2025, 2:07 p.m. UTC | #9

Am 15.04.25 um 15:14 schrieb Thomas Zimmermann:
>>
>>> 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 <[e-mail]>
>> 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 <[e-mail]>
>>      Reviewed-by: Simona Vetter <[e-mail]>
>>      Reviewed-by: Dmitry Osipenko <[e-mail]>
>>      Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
>>
>> 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?
>
> Yeah, the vmap needs the sgtable, which needs import_attach IIRC. Somewhere in there a DMA device is required. But it's not of high priority as we have workarounds.

I've removed the need to create an sgtable just to create an import_attach.

The crux is that exporters sometimes need to distinct between the case when DMA-buf is just used for inter process passing of buffers and inter device passing of buffers. Usually we use the list of attachments for that.

Because of this I though of changing the dma_buf_vmap functions to take an attachment instead of an dma_buf as parameter. That would also resolve the problem is making sure to signal buffer movement through the move notifier.

BTW: Where do we currently pin the buffers to make sure that the pointers returned by dma_buf_vmap() stays valid after dropping the reservation lock?

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> Regards,
>> Christian.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>>>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>>>>> Reported-by: Andy Yan <[e-mail]>
>>>>>>> Closes: https://lore.kernel.org/dri-devel/[e-mail]/
>>>>>>> Tested-by: Andy Yan <[e-mail]>
>>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>>> Cc: Anusha Srivatsa <[e-mail]>
>>>>>>> Cc: Christian König <[e-mail]>
>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>>>> Cc: David Airlie <[e-mail]>
>>>>>>> Cc: Simona Vetter <[e-mail]>
>>>>>>> Cc: Sumit Semwal <[e-mail]>
>>>>>>> Cc: "Christian König" <[e-mail]>
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> ---
>>>>>>>     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 April 16, 2025, 9:15 a.m. UTC | #10

On Tue, Apr 15, 2025 at 03:57:11PM +0200, Christian König wrote:
> Am 15.04.25 um 15:10 schrieb Simona Vetter:
> >> 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.
> 
> Even in that case I would still suggest to at least create an attachment to let the exporter know that somebody is doing something with it's buffer.
> 
> That is also important for move notification since you can't do those without an attachment.
> 
> BTW: What is keeping a vmap alive after dropping the reservation lock? There is no pinning whatsoever as far as I can see.

dma_buf_vmap should also pin, or something will go wrong very badly. And
that also can tell the exporter what exactly the buffer is used for
(kernel cpu access). I really don't think we should mix that up with
device access as a dma_buf_attachment.
-Sima

> 
> > 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?
> 
> Yeah, I had the same idea before as well. Just didn't know if that was something worth looking into.
> 
> Regards,
> Christian.
> 
> >
> > Cheers, Sima

Patch

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