| Message ID | 20250415140256.299550-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/gem: Internally test import_attach for imported objects | expand |
On Tue, 15 Apr 2025 16:02:20 +0200 Thomas Zimmermann <tzimmermann@suse.de> wrote: > Test struct drm_gem_object.import_attach.dmabuf to detect imported > objects. Warn if the stored state is inconsistent. > > During object clenaup, 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(). > > 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 | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 9b71f7a9f3f8..464b9c7feec0 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje > return (obj->handle_count > 1) || obj->dma_buf; > } > > +/** > + * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object > + * @obj: the GEM object > + * @obj: the DMA buffer > + * > + * Returns: > + * True if the DMA buffer refers to the GEM object's buffer. > + */ > +static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj, > + const struct dma_buf *dma_buf) > +{ > + /* The dma-buf's priv field points to the original GEM object. */ > + return dma_buf->priv == obj; > +} > + > /** > * drm_gem_is_imported() - Tests if GEM object's buffer has been imported > * @obj: the GEM object > @@ -588,8 +603,15 @@ 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); > + const struct dma_buf *dma_buf = NULL; > + > + if (!obj->import_attach) > + return false; > + > + dma_buf = obj->import_attach->dmabuf; > + > + /* Warn if we somehow reimported our own buffer. */ > + return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf)); I'm honestly not sure I see the point of checking obj->import_attach->dmabuf. If obj->import_attach != NULL, we're sure it's a foreign buffer already, otherwise we would get the original GEM object which has ->import_attach=NULL. So why not go for a simple return obj->import_attach != NULL; check, and extend the check when you get to implement imports without import attachments (not sure what those would look like BTW).
Hi Am 15.04.25 um 16:19 schrieb Boris Brezillon: > On Tue, 15 Apr 2025 16:02:20 +0200 > Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> Test struct drm_gem_object.import_attach.dmabuf to detect imported >> objects. Warn if the stored state is inconsistent. >> >> During object clenaup, 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(). >> >> 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 | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >> index 9b71f7a9f3f8..464b9c7feec0 100644 >> --- a/include/drm/drm_gem.h >> +++ b/include/drm/drm_gem.h >> @@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje >> return (obj->handle_count > 1) || obj->dma_buf; >> } >> >> +/** >> + * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object >> + * @obj: the GEM object >> + * @obj: the DMA buffer >> + * >> + * Returns: >> + * True if the DMA buffer refers to the GEM object's buffer. >> + */ >> +static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj, >> + const struct dma_buf *dma_buf) >> +{ >> + /* The dma-buf's priv field points to the original GEM object. */ >> + return dma_buf->priv == obj; >> +} >> + >> /** >> * drm_gem_is_imported() - Tests if GEM object's buffer has been imported >> * @obj: the GEM object >> @@ -588,8 +603,15 @@ 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); >> + const struct dma_buf *dma_buf = NULL; >> + >> + if (!obj->import_attach) >> + return false; >> + >> + dma_buf = obj->import_attach->dmabuf; >> + >> + /* Warn if we somehow reimported our own buffer. */ >> + return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf)); > I'm honestly not sure I see the point of checking > obj->import_attach->dmabuf. If obj->import_attach != NULL, we're sure > it's a foreign buffer already, otherwise we would get the original GEM > object which has ->import_attach=NULL. So why not go for a simple > > return obj->import_attach != NULL; > > check, and extend the check when you get to implement imports without > import attachments (not sure what those would look like BTW). I have no strong opinion. I just found it confusing to rely on import_attach when the dma_buf is what we originally imported. Best regards Thomas
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 9b71f7a9f3f8..464b9c7feec0 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje return (obj->handle_count > 1) || obj->dma_buf; } +/** + * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object + * @obj: the GEM object + * @obj: the DMA buffer + * + * Returns: + * True if the DMA buffer refers to the GEM object's buffer. + */ +static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj, + const struct dma_buf *dma_buf) +{ + /* The dma-buf's priv field points to the original GEM object. */ + return dma_buf->priv == obj; +} + /** * drm_gem_is_imported() - Tests if GEM object's buffer has been imported * @obj: the GEM object @@ -588,8 +603,15 @@ 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); + const struct dma_buf *dma_buf = NULL; + + if (!obj->import_attach) + return false; + + dma_buf = obj->import_attach->dmabuf; + + /* Warn if we somehow reimported our own buffer. */ + return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf)); } #ifdef CONFIG_LOCKDEP