From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/sun4i: Use backend/mixer as dedicated DMA device
Date: Wed, 11 Mar 2026 13:48:36 +1000 [thread overview]
Message-ID: <review-patch4-20260310032012.2542334-5-wenst@chromium.org> (raw)
In-Reply-To: <20260310032012.2542334-5-wenst@chromium.org>
Patch Review
This replaces the `of_dma_configure()` hack with proper `drm_dev_set_dma_dev()` calls. The approach is:
```c
- if (of_property_present(dev->of_node, "interconnects")) {
- ret = of_dma_configure(drm->dev, dev->of_node, true);
- if (ret)
- return ret;
- }
+ if (drm_dev_dma_dev(drm) == drm->dev)
+ drm_dev_set_dma_dev(drm, dev);
```
The guard `drm_dev_dma_dev(drm) == drm->dev` ensures only the first backend/mixer to bind sets itself as the DMA device, which matches the comment explaining the rationale.
**Behavioral change to note**: The old backend code was gated on `of_property_present(dev->of_node, "interconnects")`, and the old mixer code was gated on `of_property_present(dev->of_node, "iommus")`. The new code unconditionally sets the DMA device. This changes behavior for devices *without* those DT properties -- previously the DRM device's own (parent) device would be used for DMA, now it will use the backend/mixer device. This should generally be fine because the backend/mixer *is* the actual DMA device regardless of whether an IOMMU or interconnect is involved, but it's a semantic change worth calling out. If any board has a backend/mixer that truly can't do DMA and relies on the parent device, this could be a problem, though that scenario seems unlikely.
**Comment nit**: The comment in `sun8i_mixer.c` says "backends" and "frontends" (copy-pasted from `sun4i_backend.c`) but should probably say "mixers" to be accurate for the mixer context. This existed partially before but was made worse by the patch expanding the comment. Line 1175-1176:
```c
+ * This assume we have the same DMA constraints for all our the
+ * devices in our pipeline (all the backends, but also the
+ * frontends).
```
This text makes sense for `sun4i_backend.c` but is inaccurate in `sun8i_mixer.c` where the original comment correctly said "mixers" instead of "backends"/"frontends". The old mixer comment was already more appropriate.
**Unbind path**: The old code modified `drm->dev`'s DMA configuration directly, so there was no cleanup needed on unbind. With the new approach, `drm_dev_set_dma_dev()` takes a reference on the device via `get_device()`. Is this reference properly released when the DRM device is torn down? Looking at `drm_dev_set_dma_dev()`, it does `get_device(dma_dev)` and `put_device(dev->dma_dev)`. The DRM device cleanup in `drm_dev_release()` should handle this, but it's worth verifying that `dma_dev` is properly cleared. This applies to all drivers calling `drm_dev_set_dma_dev()` from a component bind function -- if the component unbinds before the DRM device is destroyed, the reference on the component's device will keep it alive, which should be fine since the DRM device will outlive individual components.
**Reviewed-by worthy**: Yes, with the comment nit about "backends"/"frontends" in the mixer code.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-03-11 3:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-10 3:20 [PATCH 0/4] drm/gem-dma: Support dedicated DMA device for allocation Chen-Yu Tsai
2026-03-10 3:20 ` [PATCH 1/4] drm/prime: Limit scatter list size with dedicated DMA device Chen-Yu Tsai
2026-03-11 3:48 ` Claude review: " Claude Code Review Bot
2026-03-10 3:20 ` [PATCH 2/4] drm/gem-dma: Support dedicated DMA device for allocation and mapping Chen-Yu Tsai
2026-03-11 3:48 ` Claude review: " Claude Code Review Bot
2026-03-10 3:20 ` [PATCH 3/4] drm/mediatek: Set dedicated DMA device and drop custom GEM callbacks Chen-Yu Tsai
2026-03-11 3:48 ` Claude review: " Claude Code Review Bot
2026-03-10 3:20 ` [PATCH 4/4] drm/sun4i: Use backend/mixer as dedicated DMA device Chen-Yu Tsai
2026-03-11 3:48 ` Claude Code Review Bot [this message]
2026-03-11 3:48 ` Claude review: drm/gem-dma: Support dedicated DMA device for allocation Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-10 3:25 [PATCH RESEND 0/4] " Chen-Yu Tsai
2026-03-10 3:25 ` [PATCH RESEND 4/4] drm/sun4i: Use backend/mixer as dedicated DMA device Chen-Yu Tsai
2026-03-11 3:45 ` Claude review: " Claude Code Review Bot
2026-03-11 9:49 [PATCH v2 0/4] drm/gem-dma: Support dedicated DMA device for allocation Chen-Yu Tsai
2026-03-11 9:49 ` [PATCH v2 4/4] drm/sun4i: Use backend/mixer as dedicated DMA device Chen-Yu Tsai
2026-03-11 21:08 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch4-20260310032012.2542334-5-wenst@chromium.org \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox