Re: [PATCH v8 17/18] media: ti: j721e-csi2rx: Support runtime suspend

From: Rishikesh Donadkar
Date: Mon Dec 29 2025 - 05:07:46 EST


On 08/12/25 13:06, Jai Luthra wrote:
Hi Tomi,


Hi Jai, Tomi,


Quoting Tomi Valkeinen (2025-12-01 18:52:36)
Hi,

On 12/11/2025 13:54, Rishikesh Donadkar wrote:
From: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>

Add support for runtime power-management to enable powering off the
shared power domain between Cadence CSI2RX and TI CSI2RX wrapper when
the device(s) are not in use.

When powering off the IP, the PSI-L endpoint loses the paired DMA
channels. Thus we have to release the DMA channels at runtime suspend
and request them again at resume.
I'm not an expert on the dmaengine, but to me this sounds like a bug in
the dma driver. It just sounds very wrong...

Cc: Vignesh

IIRC this was done because on AM62 the CSI2RX is on a separate power domain
and it uses DMA channels from the system-wide DMA engine.

And as those two drivers have different set of users, we have situations
where CSI2RX goes to runtime suspend state and turns off the power and
clocks, while the DMA engine remains on as it has other users. This leads
to the paired PSIL channels to become invalid, and needs a re-pairing.

I am not an expert on DMA engine APIs to know how feasible it would be to
do the book-keeping of the power state of various devices in the DMA driver
and manage the re-pairing entirely there.

On AM62A and later devices, there is a dedicated instance of the BCDMA
engine for camera pipeline on the same power domain as CSI2RX, so this is
not a problem. Rishikesh/Vignesh please correct me if I'm wrong, as it has
been a while since I looked at this in depth.


Tested-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
Reviewed-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
---
drivers/media/platform/ti/Kconfig | 1 +
.../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 55 ++++++++++++++++++-
2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/Kconfig b/drivers/media/platform/ti/Kconfig
index 3bc4aa35887e6..a808063e24779 100644
--- a/drivers/media/platform/ti/Kconfig
+++ b/drivers/media/platform/ti/Kconfig
@@ -70,6 +70,7 @@ config VIDEO_TI_J721E_CSI2RX
depends on VIDEO_CADENCE_CSI2RX
depends on PHY_CADENCE_DPHY_RX || COMPILE_TEST
depends on ARCH_K3 || COMPILE_TEST
+ depends on PM
select VIDEOBUF2_DMA_CONTIG
select V4L2_FWNODE
help
diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index 528041ee78cf3..21e032c64b901 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <media/cadence/cdns-csi2rx.h>
@@ -963,12 +964,16 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
unsigned long flags;
int ret = 0;
+ ret = pm_runtime_resume_and_get(csi->dev);
+ if (ret)
+ return ret;
+
spin_lock_irqsave(&dma->lock, flags);
if (list_empty(&dma->queue))
ret = -EIO;
spin_unlock_irqrestore(&dma->lock, flags);
if (ret)
- return ret;
+ goto err;
ret = video_device_pipeline_start(&ctx->vdev, &csi->pipe);
if (ret)
@@ -1024,6 +1029,8 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
err:
ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_QUEUED);
+ pm_runtime_put(csi->dev);
+
return ret;
}
@@ -1055,6 +1062,7 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
ti_csi2rx_stop_dma(ctx);
ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_ERROR);
+ pm_runtime_put(csi->dev);
}
static const struct vb2_ops csi_vb2_qops = {
@@ -1261,7 +1269,9 @@ static void ti_csi2rx_cleanup_notifier(struct ti_csi2rx_dev *csi)
static void ti_csi2rx_cleanup_ctx(struct ti_csi2rx_ctx *ctx)
{
- dma_release_channel(ctx->dma.chan);
+ if (!pm_runtime_status_suspended(ctx->csi->dev))
+ dma_release_channel(ctx->dma.chan);
+
vb2_queue_release(&ctx->vidq);
video_unregister_device(&ctx->vdev);
@@ -1512,6 +1522,39 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
return ret;
}
+static int ti_csi2rx_runtime_suspend(struct device *dev)
+{
+ struct ti_csi2rx_dev *csi = dev_get_drvdata(dev);
+ int i;
+
+ if (csi->enable_count != 0)
+ return -EBUSY;
+
+ for (i = 0; i < csi->num_ctx; i++)
+ dma_release_channel(csi->ctx[i].dma.chan);
+
+ return 0;
+}
+
+static int ti_csi2rx_runtime_resume(struct device *dev)
+{
+ struct ti_csi2rx_dev *csi = dev_get_drvdata(dev);
+ int ret, i;
+
+ for (i = 0; i < csi->num_ctx; i++) {
+ ret = ti_csi2rx_init_dma(&csi->ctx[i]);
If runtime_resume always requests the dma channels, is the call to
ti_csi2rx_init_dma() in ti_csi2rx_init_ctx() needed? If not, you could
inline the code from ti_csi2rx_init_dma() to here and also drop the
dma_release_channel() call from ti_csi2rx_cleanup_ctx(), making the flow
more understandable.

Makes sense.

+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops ti_csi2rx_pm_ops = {
+ RUNTIME_PM_OPS(ti_csi2rx_runtime_suspend, ti_csi2rx_runtime_resume,
+ NULL)
+};
+
static int ti_csi2rx_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -1579,6 +1622,10 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
goto err_notifier;
}
+ pm_runtime_set_active(csi->dev);
+ pm_runtime_enable(csi->dev);
+ pm_request_idle(csi->dev);
I always forget what exactly the runtime_pm funcs do. What's the idea
here? If you do something else than the plain standard
pm_runtime_enable(), I think it's good to mention what/why in a comment.

https://docs.kernel.org/power/runtime_pm.html#runtime-pm-initialization-device-probing-and-removal

The pm_request_idle() is done to queue up a job to suspend the hardware
until userspace starts streaming, to save power.

The runtime_set_active() is used because the power domain for CSI is by
default turned on when system boots up. But Rishikesh, please double check
that before v9 on AM62.


I checked with AM62 when booting up, with CSI drivers being probed/not probed. The device state was OFF by default on boot in both cases. So, plain pm_runtime_enable() should work. That will simplify the dma_request/release_chan() calls  in the runtime suspend/resume flow.


Thanks & Regards,

Rishikesh


return 0;
err_notifier:
@@ -1609,6 +1656,9 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
mutex_destroy(&csi->mutex);
dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
csi->drain.paddr);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+
}
static const struct of_device_id ti_csi2rx_of_match[] = {
@@ -1623,6 +1673,7 @@ static struct platform_driver ti_csi2rx_pdrv = {
.driver = {
.name = TI_CSI2RX_MODULE_NAME,
.of_match_table = ti_csi2rx_of_match,
+ .pm = &ti_csi2rx_pm_ops,
},
};