Hi
Am 15.04.25 um 16:19 schrieb Boris Brezillon:
On Tue, 15 Apr 2025 16:02:20 +0200
Thomas Zimmermann <[email protected]> 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 <[email protected]>
Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
Reported-by: Andy Yan <[email protected]>
Closes:
https://lore.kernel.org/dri-devel/[email protected]/
Tested-by: Andy Yan <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christian König <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Simona Vetter <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Christian König" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
--
--
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)
