public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning
@ 2026-03-31  6:16 Mikhail Gavrilov
  2026-03-31  6:42 ` Claude review: " Claude Code Review Bot
  2026-03-31  6:42 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Mikhail Gavrilov @ 2026-03-31  6:16 UTC (permalink / raw)
  To: kraxel, vivek.kasireddy
  Cc: sumit.semwal, christian.koenig, dri-devel, linux-media,
	linaro-mm-sig, linux-kernel, stable, Mikhail Gavrilov

When CONFIG_DMA_API_DEBUG_SG is enabled, importing a udmabuf into a DRM
driver (e.g. amdgpu for video playback in GNOME Videos / Showtime)
triggers a spurious warning:

  DMA-API: amdgpu 0000:03:00.0: cacheline tracking EEXIST, \
      overlapping mappings aren't supported
  WARNING: kernel/dma/debug.c:619 at add_dma_entry+0x473/0x5f0

The call chain is:

  amdgpu_cs_ioctl
   -> amdgpu_ttm_backend_bind
    -> dma_buf_map_attachment
     -> [udmabuf] map_udmabuf -> get_sg_table
      -> dma_map_sgtable(dev, sg, direction, 0)  // attrs=0
       -> debug_dma_map_sg -> add_dma_entry -> EEXIST

This happens because udmabuf builds a per-page scatter-gather list via
sg_set_folio().  When begin_cpu_udmabuf() has already created an sg
table mapped for the misc device, and an importer such as amdgpu maps
the same pages for its own device via map_udmabuf(), the DMA debug
infrastructure sees two active mappings whose physical addresses share
cacheline boundaries and warns about the overlap.

The DMA_ATTR_SKIP_CPU_SYNC flag suppresses this check in
add_dma_entry() because it signals that no CPU cache maintenance is
performed at map/unmap time, making the cacheline overlap harmless.

All other major dma-buf exporters already pass this flag:
  - drm_gem_map_dma_buf() passes DMA_ATTR_SKIP_CPU_SYNC
  - amdgpu_dma_buf_map() passes DMA_ATTR_SKIP_CPU_SYNC

The CPU sync at map/unmap time is also redundant for udmabuf:
begin_cpu_udmabuf() and end_cpu_udmabuf() already perform explicit
cache synchronization via dma_sync_sgtable_for_cpu/device() when CPU
access is requested through the dma-buf interface.

Pass DMA_ATTR_SKIP_CPU_SYNC to dma_map_sgtable() and
dma_unmap_sgtable() in udmabuf to suppress the spurious warning and
skip the redundant sync.

Fixes: 284562e1f348 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks")
Cc: stable@vger.kernel.org
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---

v1 -> v2:
  - Rebased on drm-tip to resolve conflict with folio conversion
    patches. No code change, same two-line fix.

v1: https://lore.kernel.org/all/20260317053653.28888-1-mikhail.v.gavrilov@gmail.com/

 drivers/dma-buf/udmabuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 94b26ea706a3..bced421c0d65 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -145,7 +145,7 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
 	if (ret < 0)
 		goto err_alloc;
 
-	ret = dma_map_sgtable(dev, sg, direction, 0);
+	ret = dma_map_sgtable(dev, sg, direction, DMA_ATTR_SKIP_CPU_SYNC);
 	if (ret < 0)
 		goto err_map;
 	return sg;
@@ -160,7 +160,7 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
 static void put_sg_table(struct device *dev, struct sg_table *sg,
 			 enum dma_data_direction direction)
 {
-	dma_unmap_sgtable(dev, sg, direction, 0);
+	dma_unmap_sgtable(dev, sg, direction, DMA_ATTR_SKIP_CPU_SYNC);
 	sg_free_table(sg);
 	kfree(sg);
 }
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning
  2026-03-31  6:16 [PATCH v2] dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning Mikhail Gavrilov
@ 2026-03-31  6:42 ` Claude Code Review Bot
  2026-03-31  6:42 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-03-31  6:42 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Patches: 1
Reviewed: 2026-03-31T16:42:34.337924

---

This is a clean, well-justified single-patch fix. The commit message is thorough — it documents the call chain triggering the warning, explains why `DMA_ATTR_SKIP_CPU_SYNC` is appropriate, cites precedent from other exporters (`drm_gem_map_dma_buf`, `amdgpu_dma_buf_map`), and explains why the CPU sync is redundant given `begin_cpu_udmabuf`/`end_cpu_udmabuf` already handle explicit cache synchronization.

The fix is correct and minimal. The `Fixes:` tag and `Cc: stable` are appropriate since this is a regression fix for a real user-visible warning triggered during normal video playback.

**Verdict: Looks good.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning
  2026-03-31  6:16 [PATCH v2] dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning Mikhail Gavrilov
  2026-03-31  6:42 ` Claude review: " Claude Code Review Bot
@ 2026-03-31  6:42 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-03-31  6:42 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Analysis of the change:**

The patch changes two lines in `get_sg_table()` and `put_sg_table()`:

```c
-	ret = dma_map_sgtable(dev, sg, direction, 0);
+	ret = dma_map_sgtable(dev, sg, direction, DMA_ATTR_SKIP_CPU_SYNC);
```

```c
-	dma_unmap_sgtable(dev, sg, direction, 0);
+	dma_unmap_sgtable(dev, sg, direction, DMA_ATTR_SKIP_CPU_SYNC);
```

**Correctness:** The change is correct. `get_sg_table()` is called from two places:
1. `map_udmabuf()` (line 186) — the dma-buf `map_dma_buf` op, called by importers. CPU sync at map time is redundant; importers manage their own cache coherency.
2. `begin_cpu_udmabuf()` (line 252) — on the first call, creates the sg table. Subsequent calls use `dma_sync_sgtable_for_cpu()` explicitly (line 258). The initial map doesn't need a sync either, as the pages are freshly mapped.

Similarly, `put_sg_table()` is called from `unmap_udmabuf()` and `release_udmabuf()`, where the CPU sync at unmap time is unnecessary.

**Precedent:** I confirmed that both `drm_gem_map_dma_buf()` (`drm_prime.c:684`) and `amdgpu_dma_buf_map()` (`amdgpu_dma_buf.c:213`) already pass `DMA_ATTR_SKIP_CPU_SYNC`, matching the commit message's claim.

**One minor observation:** The commit message says the flag "suppresses this check in `add_dma_entry()`" — that's slightly imprecise. `DMA_ATTR_SKIP_CPU_SYNC` doesn't directly affect `add_dma_entry()`. Rather, by skipping the CPU cache maintenance, the overlapping mappings become harmless, and the debug infrastructure's concern about cacheline sharing is moot. The warning is still technically possible if both mappings exist simultaneously, but in practice the flag prevents the cache ops that make the overlap dangerous. The actual suppression mechanism may be more nuanced, but the end result is correct — the warning goes away and the behavior is safe.

**Verdict:** Reviewed-by worthy. Clean two-line fix, well-motivated, consistent with how other dma-buf exporters handle this.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-03-31  6:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-31  6:16 [PATCH v2] dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning Mikhail Gavrilov
2026-03-31  6:42 ` Claude review: " Claude Code Review Bot
2026-03-31  6:42 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox