Dear All,
This patchset fixes the incorrect use of dma_sync_sg_*() calls in
media and related drivers. They are replaced with much safer
dma_sync_sgtable_*() variants, which take care of passing the proper
number of elements for the sync operation.
Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Patch summary:
Marek Szyprowski (3):
media: videobuf2: use sgtable-based scatterlist wrappers
udmabuf: use sgtable-based scatterlist wrappers
omap3isp:: use sgtable-based scatterlist wrappers
drivers/dma-buf/udmabuf.c | 5 ++---
drivers/media/common/videobuf2/videobuf2-dma-sg.c | 4 ++--
drivers/media/platform/ti/omap3isp/ispccdc.c | 8 ++++----
drivers/media/platform/ti/omap3isp/ispstat.c | 6 ++----
4 files changed, 10 insertions(+), 13 deletions(-)
--
2.34.1
Hi Jared,
Thanks for working on this
On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote:
> The CMA heap's name in devtmpfs can vary depending on how the heap is
> defined. Its name defaults to "reserved", but if a CMA area is defined
> in the devicetree, the heap takes on the devicetree node's name, such as
> "default-pool" or "linux,cma". To simplify naming, just name it
> "default_cma", and keep a legacy node in place backed by the same
> underlying structure for backwards compatibility.
>
> Signed-off-by: Jared Kangas <jkangas(a)redhat.com>
> ---
> Documentation/userspace-api/dma-buf-heaps.rst | 11 +++++++----
> drivers/dma-buf/heaps/Kconfig | 10 ++++++++++
> drivers/dma-buf/heaps/cma_heap.c | 14 +++++++++++++-
> 3 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst
> index 535f49047ce64..577de813ba461 100644
> --- a/Documentation/userspace-api/dma-buf-heaps.rst
> +++ b/Documentation/userspace-api/dma-buf-heaps.rst
> @@ -19,7 +19,10 @@ following heaps:
> - The ``cma`` heap allocates physically contiguous, cacheable,
> buffers. Only present if a CMA region is present. Such a region is
> usually created either through the kernel commandline through the
> - `cma` parameter, a memory region Device-Tree node with the
> - `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or
> - `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it
> - might be called ``reserved``, ``linux,cma``, or ``default-pool``.
> + ``cma`` parameter, a memory region Device-Tree node with the
> + ``linux,cma-default`` property set, or through the ``CMA_SIZE_MBYTES`` or
> + ``CMA_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is
> + ``default_cma``. For backwards compatibility, when the
> + ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is
> + created following legacy naming conventions; the legacy name might be
> + ``reserved``, ``linux,cma``, or ``default-pool``.
It looks like, in addition to documenting the new naming, you also
changed all the backticks to double backticks. Why did you do so? It
seems mostly unrelated to that patch, so it would be better in a
separate patch.
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c42264..83f3770fa820a 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA
> Choose this option to enable dma-buf CMA heap. This heap is backed
> by the Contiguous Memory Allocator (CMA). If your system has these
> regions, you should say Y here.
> +
> +config DMABUF_HEAPS_CMA_LEGACY
> + bool "DMA-BUF CMA Heap"
> + default y
> + depends on DMABUF_HEAPS_CMA
> + help
> + Add a duplicate CMA-backed dma-buf heap with legacy naming derived
> + from the CMA area's devicetree node, or "reserved" if the area is not
> + defined in the devicetree. This uses the same underlying allocator as
> + CONFIG_DMABUF_HEAPS_CMA.
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index e998d8ccd1dc6..cd742c961190d 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -22,6 +22,7 @@
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
>
> +#define DEFAULT_CMA_NAME "default_cma"
I appreciate this is kind of bikeshed-color territory, but I think "cma"
would be a better option here. There's nothing "default" about it.
> struct cma_heap {
> struct dma_heap *heap;
> @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name)
> static int __init add_default_cma_heap(void)
> {
> struct cma *default_cma = dev_get_cma_area(NULL);
> + const char *legacy_cma_name;
> int ret;
>
> if (!default_cma)
> return 0;
>
> - ret = __add_cma_heap(default_cma, cma_get_name(default_cma));
> + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME);
> if (ret)
> return ret;
>
> + legacy_cma_name = cma_get_name(default_cma);
> +
> + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) &&
> + strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) {
> + ret = __add_cma_heap(default_cma, legacy_cma_name);
> + if (ret)
> + pr_warn("cma_heap: failed to add legacy heap: %pe\n",
> + ERR_PTR(-ret));
> + }
> +
It would also simplify this part, since you would always create the legacy heap.
Maxime
Hi
Am 05.05.25 um 09:29 schrieb Boris Brezillon:
> On Wed, 30 Apr 2025 11:07:17 +0200
> Simona Vetter <simona.vetter(a)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(a)suse.de>
>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>>>> Reported-by: Andy Yan <andyshrk(a)163.com>
>>>> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh…
>>>> Tested-by: Andy Yan <andyshrk(a)163.com>
>>>> Cc: Thomas Zimmermann <tzimmermann(a)suse.de>
>>>> Cc: Anusha Srivatsa <asrivats(a)redhat.com>
>>>> Cc: Christian König <christian.koenig(a)amd.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
>>>> Cc: Maxime Ripard <mripard(a)kernel.org>
>>>> Cc: David Airlie <airlied(a)gmail.com>
>>>> Cc: Simona Vetter <simona(a)ffwll.ch>
>>>> Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
>>>> Cc: "Christian König" <christian.koenig(a)amd.com>
>>>> Cc: dri-devel(a)lists.freedesktop.org
>>>> Cc: linux-media(a)vger.kernel.org
>>>> Cc: linaro-mm-sig(a)lists.linaro.org
>>> Reviewed-by: Simona Vetter <simona.vetter(a)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…
>>
>> 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)
From: Thomas Zimmermann <tzimmermann(a)suse.de>
[ Upstream commit b57aa47d39e94dc47403a745e2024664e544078c ]
Add drm_gem_is_imported() that tests if a GEM object's buffer has
been imported. Update the GEM code accordingly.
GEM code usually tests for imports if import_attach has been set
in struct drm_gem_object. But attaching a dma-buf on import requires
a DMA-capable importer device, which is not the case for many serial
busses like USB or I2C. The new helper tests if a GEM object's dma-buf
has been created from the GEM object.
Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de>
Reviewed-by: Anusha Srivatsa <asrivats(a)redhat.com>
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20250226172457.217725-2-tzimm…
Signed-off-by: Sasha Levin <sashal(a)kernel.org>
---
drivers/gpu/drm/drm_gem.c | 4 ++--
include/drm/drm_gem.h | 14 ++++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 44a948b80ee14..deb93f78ce344 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -322,7 +322,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
return -ENOENT;
/* Don't allow imported objects to be mapped */
- if (obj->import_attach) {
+ if (drm_gem_is_imported(obj)) {
ret = -EINVAL;
goto out;
}
@@ -1155,7 +1155,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
drm_vma_node_start(&obj->vma_node));
drm_printf_indent(p, indent, "size=%zu\n", obj->size);
drm_printf_indent(p, indent, "imported=%s\n",
- str_yes_no(obj->import_attach));
+ str_yes_no(drm_gem_is_imported(obj)));
if (obj->funcs->print_info)
obj->funcs->print_info(p, indent, obj);
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 7c2ec139c464a..fbfccb96dd17b 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -35,6 +35,7 @@
*/
#include <linux/kref.h>
+#include <linux/dma-buf.h>
#include <linux/dma-resv.h>
#include <linux/list.h>
#include <linux/mutex.h>
@@ -557,6 +558,19 @@ 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_is_imported() - Tests if GEM object's buffer has been imported
+ * @obj: the GEM object
+ *
+ * Returns:
+ * True if the GEM object's buffer has been imported, false otherwise
+ */
+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);
+}
+
#ifdef CONFIG_LOCKDEP
/**
* drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list.
--
2.39.5
From: Thomas Zimmermann <tzimmermann(a)suse.de>
[ Upstream commit b57aa47d39e94dc47403a745e2024664e544078c ]
Add drm_gem_is_imported() that tests if a GEM object's buffer has
been imported. Update the GEM code accordingly.
GEM code usually tests for imports if import_attach has been set
in struct drm_gem_object. But attaching a dma-buf on import requires
a DMA-capable importer device, which is not the case for many serial
busses like USB or I2C. The new helper tests if a GEM object's dma-buf
has been created from the GEM object.
Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de>
Reviewed-by: Anusha Srivatsa <asrivats(a)redhat.com>
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20250226172457.217725-2-tzimm…
Signed-off-by: Sasha Levin <sashal(a)kernel.org>
---
drivers/gpu/drm/drm_gem.c | 4 ++--
include/drm/drm_gem.h | 14 ++++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 149b8e25da5bb..426d0867882df 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -322,7 +322,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
return -ENOENT;
/* Don't allow imported objects to be mapped */
- if (obj->import_attach) {
+ if (drm_gem_is_imported(obj)) {
ret = -EINVAL;
goto out;
}
@@ -1152,7 +1152,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
drm_vma_node_start(&obj->vma_node));
drm_printf_indent(p, indent, "size=%zu\n", obj->size);
drm_printf_indent(p, indent, "imported=%s\n",
- str_yes_no(obj->import_attach));
+ str_yes_no(drm_gem_is_imported(obj)));
if (obj->funcs->print_info)
obj->funcs->print_info(p, indent, obj);
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index d8b86df2ec0da..70c0f8c83629d 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -35,6 +35,7 @@
*/
#include <linux/kref.h>
+#include <linux/dma-buf.h>
#include <linux/dma-resv.h>
#include <linux/list.h>
#include <linux/mutex.h>
@@ -570,6 +571,19 @@ 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_is_imported() - Tests if GEM object's buffer has been imported
+ * @obj: the GEM object
+ *
+ * Returns:
+ * True if the GEM object's buffer has been imported, false otherwise
+ */
+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);
+}
+
#ifdef CONFIG_LOCKDEP
/**
* drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list.
--
2.39.5
From: Thomas Zimmermann <tzimmermann(a)suse.de>
[ Upstream commit 8260731ccad0451207b45844bb66eb161a209218 ]
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(a)suse.de>
Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
Reported-by: Andy Yan <andyshrk(a)163.com>
Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh…
Tested-by: Andy Yan <andyshrk(a)163.com>
Cc: Thomas Zimmermann <tzimmermann(a)suse.de>
Cc: Anusha Srivatsa <asrivats(a)redhat.com>
Cc: Christian König <christian.koenig(a)amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Maxime Ripard <mripard(a)kernel.org>
Cc: David Airlie <airlied(a)gmail.com>
Cc: Simona Vetter <simona(a)ffwll.ch>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: "Christian König" <christian.koenig(a)amd.com>
Cc: dri-devel(a)lists.freedesktop.org
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com>
Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch>
Link: https://lore.kernel.org/r/20250416065820.26076-1-tzimmermann@suse.de
Signed-off-by: Sasha Levin <sashal(a)kernel.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 2bf893eabb4b2..bcd54020d6ba5 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -585,8 +585,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.39.5
From: Thomas Zimmermann <tzimmermann(a)suse.de>
[ Upstream commit b57aa47d39e94dc47403a745e2024664e544078c ]
Add drm_gem_is_imported() that tests if a GEM object's buffer has
been imported. Update the GEM code accordingly.
GEM code usually tests for imports if import_attach has been set
in struct drm_gem_object. But attaching a dma-buf on import requires
a DMA-capable importer device, which is not the case for many serial
busses like USB or I2C. The new helper tests if a GEM object's dma-buf
has been created from the GEM object.
Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de>
Reviewed-by: Anusha Srivatsa <asrivats(a)redhat.com>
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20250226172457.217725-2-tzimm…
Signed-off-by: Sasha Levin <sashal(a)kernel.org>
---
drivers/gpu/drm/drm_gem.c | 4 ++--
include/drm/drm_gem.h | 14 ++++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ee811764c3df4..c6240bab3fa55 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -348,7 +348,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
return -ENOENT;
/* Don't allow imported objects to be mapped */
- if (obj->import_attach) {
+ if (drm_gem_is_imported(obj)) {
ret = -EINVAL;
goto out;
}
@@ -1178,7 +1178,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
drm_vma_node_start(&obj->vma_node));
drm_printf_indent(p, indent, "size=%zu\n", obj->size);
drm_printf_indent(p, indent, "imported=%s\n",
- str_yes_no(obj->import_attach));
+ str_yes_no(drm_gem_is_imported(obj)));
if (obj->funcs->print_info)
obj->funcs->print_info(p, indent, obj);
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index fdae947682cd0..2bf893eabb4b2 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -35,6 +35,7 @@
*/
#include <linux/kref.h>
+#include <linux/dma-buf.h>
#include <linux/dma-resv.h>
#include <linux/list.h>
#include <linux/mutex.h>
@@ -575,6 +576,19 @@ 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_is_imported() - Tests if GEM object's buffer has been imported
+ * @obj: the GEM object
+ *
+ * Returns:
+ * True if the GEM object's buffer has been imported, false otherwise
+ */
+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);
+}
+
#ifdef CONFIG_LOCKDEP
/**
* drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list.
--
2.39.5