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