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

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

[Thomas Zimmermann]

Test struct drm_gem_object.import_attach to detect imported objects.

During object clenanup, the dma_buf field might be NULL. Testing it in
an object's free callback then incorrectly does a cleanup as for native
objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
clears the dma_buf field in drm_gem_object_exported_dma_buf_free().

v3:
- only test for import_attach (Boris)
v2:
- use import_attach.dmabuf instead of dma_buf (Christian)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
Reported-by: Andy Yan <andyshrk@163.com>
Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
Tested-by: Andy Yan <andyshrk@163.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Anusha Srivatsa <asrivats@redhat.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 include/drm/drm_gem.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 9b71f7a9f3f8..a3133a08267c 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -588,8 +588,7 @@ 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->import_attach;
 }
 
 #ifdef CONFIG_LOCKDEP
-- 
2.49.0

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

[Boris Brezillon]

On Wed, 16 Apr 2025 08:57:45 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Test struct drm_gem_object.import_attach to detect imported objects.
> 
> During object clenanup, the dma_buf field might be NULL. Testing it in
> an object's free callback then incorrectly does a cleanup as for native

Just pointing it for completeness: this happens even outside the free
callback, because the GEM handle lifetime (time userspace holds this
GEM object) might differ from the GEM lifetime itself (KMD might have
refs to the GEM after the handle is released).

> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
> clears the dma_buf field in drm_gem_object_exported_dma_buf_free().

Also happens when the GEM handle is released, which is the case we were
hitting in Panthor.

> 
> v3:
> - only test for import_attach (Boris)

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> v2:
> - use import_attach.dmabuf instead of dma_buf (Christian)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> Reported-by: Andy Yan <andyshrk@163.com>
> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
> Tested-by: Andy Yan <andyshrk@163.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Anusha Srivatsa <asrivats@redhat.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> ---
>  include/drm/drm_gem.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 9b71f7a9f3f8..a3133a08267c 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -588,8 +588,7 @@ 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->import_attach;
>  }
>  
>  #ifdef CONFIG_LOCKDEP

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

[Simona Vetter]

On Wed, Apr 16, 2025 at 08:57:45AM +0200, Thomas Zimmermann wrote:
> Test struct drm_gem_object.import_attach to detect imported objects.
> 
> During object clenanup, the dma_buf field might be NULL. Testing it in
> an object's free callback then incorrectly does a cleanup as for native
> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
> clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
> 
> v3:
> - only test for import_attach (Boris)
> v2:
> - use import_attach.dmabuf instead of dma_buf (Christian)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> Reported-by: Andy Yan <andyshrk@163.com>
> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
> Tested-by: Andy Yan <andyshrk@163.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Anusha Srivatsa <asrivats@redhat.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

> ---
>  include/drm/drm_gem.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 9b71f7a9f3f8..a3133a08267c 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -588,8 +588,7 @@ 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->import_attach;
>  }
>  
>  #ifdef CONFIG_LOCKDEP
> -- 
> 2.49.0
> 

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

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

[Simona Vetter]

On Wed, Apr 16, 2025 at 11:38:03AM +0200, Simona Vetter wrote:
> On Wed, Apr 16, 2025 at 08:57:45AM +0200, Thomas Zimmermann wrote:
> > Test struct drm_gem_object.import_attach to detect imported objects.
> > 
> > During object clenanup, the dma_buf field might be NULL. Testing it in
> > an object's free callback then incorrectly does a cleanup as for native
> > objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
> > clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
> > 
> > v3:
> > - only test for import_attach (Boris)
> > v2:
> > - use import_attach.dmabuf instead of dma_buf (Christian)
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> > Reported-by: Andy Yan <andyshrk@163.com>
> > Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
> > Tested-by: Andy Yan <andyshrk@163.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Anusha Srivatsa <asrivats@redhat.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: Simona Vetter <simona@ffwll.ch>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-media@vger.kernel.org
> > Cc: linaro-mm-sig@lists.linaro.org
> 
> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

Also quick doc request: We do have a bit of overview documentation for
prime here about specifically this lifetime fun, and why there's a chain
of references and hence a distinction between imported foreign dma-buf and
re-imported native dma-buf:

https://dri.freedesktop.org/docs/drm/gpu/drm-mm.html#reference-counting-for-gem-drivers

I think it would be good to augment this with more links to functions
(like this one recently added and fixed in this patch here) and struct
members to that overview. And maybe also link from key function and struct
functions back to that overview doc. Otherwise I think the next person
will get confused by this rather tricky code again and break a corner
cases.

Thanks, Sima

> 
> > ---
> >  include/drm/drm_gem.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 9b71f7a9f3f8..a3133a08267c 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -588,8 +588,7 @@ 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->import_attach;
> >  }
> >  
> >  #ifdef CONFIG_LOCKDEP
> > -- 
> > 2.49.0
> > 
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

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

[Boris Brezillon]

On Wed, 30 Apr 2025 11:07:17 +0200
Simona Vetter <simona.vetter@ffwll.ch> wrote:

> On Wed, Apr 16, 2025 at 11:38:03AM +0200, Simona Vetter wrote:
> > On Wed, Apr 16, 2025 at 08:57:45AM +0200, Thomas Zimmermann wrote:  
> > > Test struct drm_gem_object.import_attach to detect imported objects.
> > > 
> > > During object clenanup, the dma_buf field might be NULL. Testing it in
> > > an object's free callback then incorrectly does a cleanup as for native
> > > objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
> > > clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
> > > 
> > > v3:
> > > - only test for import_attach (Boris)
> > > v2:
> > > - use import_attach.dmabuf instead of dma_buf (Christian)
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> > > Reported-by: Andy Yan <andyshrk@163.com>
> > > Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
> > > Tested-by: Andy Yan <andyshrk@163.com>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Anusha Srivatsa <asrivats@redhat.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: David Airlie <airlied@gmail.com>
> > > Cc: Simona Vetter <simona@ffwll.ch>
> > > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: linux-media@vger.kernel.org
> > > Cc: linaro-mm-sig@lists.linaro.org  
> > 
> > Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>  
> 
> Also quick doc request: We do have a bit of overview documentation for
> prime here about specifically this lifetime fun, and why there's a chain
> of references and hence a distinction between imported foreign dma-buf and
> re-imported native dma-buf:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-mm.html#reference-counting-for-gem-drivers
> 
> I think it would be good to augment this with more links to functions
> (like this one recently added and fixed in this patch here) and struct
> members to that overview. And maybe also link from key function and struct
> functions back to that overview doc. Otherwise I think the next person
> will get confused by this rather tricky code again and break a corner
> cases.

BTW, could we also backmerge 6.15-rc5 into drm-misc-next so the fix is
also present in drm-misc-next?

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

[Thomas Zimmermann]

Hi

Am 05.05.25 um 09:29 schrieb Boris Brezillon:
> On Wed, 30 Apr 2025 11:07:17 +0200
> Simona Vetter <simona.vetter@ffwll.ch> wrote:
>
>> On Wed, Apr 16, 2025 at 11:38:03AM +0200, Simona Vetter wrote:
>>> On Wed, Apr 16, 2025 at 08:57:45AM +0200, Thomas Zimmermann wrote:
>>>> Test struct drm_gem_object.import_attach to detect imported objects.
>>>>
>>>> During object clenanup, the dma_buf field might be NULL. Testing it in
>>>> an object's free callback then incorrectly does a cleanup as for native
>>>> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
>>>> clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
>>>>
>>>> v3:
>>>> - only test for import_attach (Boris)
>>>> v2:
>>>> - use import_attach.dmabuf instead of dma_buf (Christian)
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>> Reported-by: Andy Yan <andyshrk@163.com>
>>>> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
>>>> Tested-by: Andy Yan <andyshrk@163.com>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: Anusha Srivatsa <asrivats@redhat.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: David Airlie <airlied@gmail.com>
>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>>> Cc: "Christian König" <christian.koenig@amd.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-media@vger.kernel.org
>>>> Cc: linaro-mm-sig@lists.linaro.org
>>> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
>> Also quick doc request: We do have a bit of overview documentation for
>> prime here about specifically this lifetime fun, and why there's a chain
>> of references and hence a distinction between imported foreign dma-buf and
>> re-imported native dma-buf:
>>
>> https://dri.freedesktop.org/docs/drm/gpu/drm-mm.html#reference-counting-for-gem-drivers
>>
>> I think it would be good to augment this with more links to functions
>> (like this one recently added and fixed in this patch here) and struct
>> members to that overview. And maybe also link from key function and struct
>> functions back to that overview doc. Otherwise I think the next person
>> will get confused by this rather tricky code again and break a corner
>> cases.
> BTW, could we also backmerge 6.15-rc5 into drm-misc-next so the fix is
> also present in drm-misc-next?

drm-misc-next is now at -rc5

Best regards
Thomas


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