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

[Jai Luthra <jai.luthra@ideasonboard.com>]

Hi 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@ideasonboard.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.
> 
> 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@ti.com>
> > Reviewed-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > ---
> >  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.

> >       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,
> >       },
> >  };
> >  
>