| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <176880264444.9154.1426306631873007814@freya>
Date: Mon, 19 Jan 2026 11:34:04 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Rishikesh Donadkar <r-donadkar@...com>, Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: y-abhilashchandra@...com, devarsht@...com, s-jain1@...com, vigneshr@...com, mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org, p.zabel@...gutronix.de, conor+dt@...nel.org, sakari.ailus@...ux.intel.com, hverkuil-cisco@...all.nl, changhuang.liang@...rfivetech.com, jack.zhu@...rfivetech.com, sjoerd@...labora.com, dan.carpenter@...aro.org, hverkuil+cisco@...nel.org, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, devicetree@...r.kernel.org, jai.luthra@...ux.dev, laurent.pinchart@...asonboard.com, mripard@...nel.org
Subject: Re: [PATCH v9 18/19] media: ti: j721e-csi2rx: Support runtime suspend
Hi
Quoting Tomi Valkeinen (2026-01-15 18:16:14)
> Hi,
>
> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> > From: Jai Luthra <jai.luthra@...asonboard.com>
> >
> > 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.
> >
> > Tested-by: Rishikesh Donadkar <r-donadkar@...com>
> > Reviewed-by: Rishikesh Donadkar <r-donadkar@...com>
> > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > Co-developed-by: Rishikesh Donadkar <r-donadkar@...com>
> > Signed-off-by: Rishikesh Donadkar <r-donadkar@...com>
> > ---
> > drivers/media/platform/ti/Kconfig | 1 +
> > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 59 +++++++++++++++----
> > 2 files changed, 50 insertions(+), 10 deletions(-)
>
> Should pixel interface reset belong to the runtime suspend/resume
> functions? (Not a suggestion, just a question =).
Yeah that would indeed make things cleaner, but the problem is that pixel
reset needs to be asserted before we stop streaming on the source, as it is
currently done in ti_csi2rx_stop_streaming(), to prevent issues with stale
data on SoCs where the power domain doesn't turn off due to other
dependencies.
I am also not sure the correct ordering would be possible if it is tied to
the pm_runtime_put due to the two subdevs (cadence and shim) between the
video node and the camera, where both need to be awake for DMA transactions
to complete.
It still might be worth investigating, but I think for this iteration of
the driver it is better to keep it separate, as moving the pixel reset has
lead to weird bugs due to stale data in the past.
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>
> Tomi
>
Thanks,
Jai
> > 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 3922bd67e78da..72da58738e16e 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>
> > @@ -964,12 +965,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)
> > @@ -991,6 +996,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;
> > }
> >
> > @@ -1022,6 +1029,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 = {
> > @@ -1263,7 +1271,6 @@ 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);
> > vb2_queue_release(&ctx->vidq);
> >
> > video_unregister_device(&ctx->vdev);
> > @@ -1283,7 +1290,7 @@ static int ti_csi2rx_init_vb2q(struct ti_csi2rx_ctx *ctx)
> > q->ops = &csi_vb2_qops;
> > q->mem_ops = &vb2_dma_contig_memops;
> > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > - q->dev = dmaengine_get_dma_device(ctx->dma.chan);
> > + q->dev = ctx->csi->dev;
> > q->lock = &ctx->mutex;
> > q->min_queued_buffers = 1;
> > q->allow_cache_hints = 1;
> > @@ -1497,21 +1504,46 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
> > spin_lock_init(&ctx->dma.lock);
> > ctx->dma.state = TI_CSI2RX_DMA_STOPPED;
> >
> > - ret = ti_csi2rx_init_dma(ctx);
> > + ret = ti_csi2rx_init_vb2q(ctx);
> > if (ret)
> > return ret;
> >
> > - ret = ti_csi2rx_init_vb2q(ctx);
> > - if (ret)
> > - goto cleanup_dma;
> > + return 0;
> > +}
> > +
> > +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;
> > +}
> >
> > -cleanup_dma:
> > - dma_release_channel(ctx->dma.chan);
> > - return ret;
> > +static int ti_csi2rx_runtime_resume(struct device *dev)
> > +{
> > + struct ti_csi2rx_dev *csi = dev_get_drvdata(dev);
> > + unsigned int ret, i;
> > +
> > + for (i = 0; i < csi->num_ctx; i++) {
> > + ret = ti_csi2rx_init_dma(&csi->ctx[i]);
> > + 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;
> > @@ -1569,6 +1601,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
> > goto err_ctx;
> > }
> >
> > + pm_runtime_enable(csi->dev);
> > +
> > ret = ti_csi2rx_notifier_register(csi);
> > if (ret)
> > goto err_ctx;
> > @@ -1601,6 +1635,9 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
> > struct ti_csi2rx_dev *csi = platform_get_drvdata(pdev);
> > unsigned int i;
> >
> > + if (!pm_runtime_status_suspended(&pdev->dev))
> > + pm_runtime_set_suspended(&pdev->dev);
> > +
> > for (i = 0; i < csi->num_ctx; i++)
> > ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
> >
> > @@ -1609,6 +1646,7 @@ 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);
> > }
> >
> > static const struct of_device_id ti_csi2rx_of_match[] = {
> > @@ -1623,6 +1661,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,
> > },
> > };
> >
>
Powered by blists - more mailing lists