On Mon, Jul 22, 2024 at 11:53:02AM +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 22/07/2024 05:17, Changhuang Liang wrote:
> > Hi, Tomi
> >
> >> Hi,
> >>
> >> On 18/07/2024 06:28, Changhuang Liang wrote:
> >>> Add system PM support make it stopping streaming at system suspend
> >>> time, and restarting streaming at system resume time.
> >>>
> >>> Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/platform/cadence/cdns-csi2rx.c | 32
> >> ++++++++++++++++++++
> >>> 1 file changed, 32 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> b/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> index 981819adbb3a..81e90b31e9f8 100644
> >>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> @@ -776,8 +776,40 @@ static int csi2rx_runtime_resume(struct device
> >> *dev)
> >>> return ret;
> >>> }
> >>>
> >>> +static int __maybe_unused csi2rx_suspend(struct device *dev) {
> >>> + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> >>> +
> >>> + mutex_lock(&csi2rx->lock);
> >>> + if (csi2rx->count)
> >>> + csi2rx_stop(csi2rx);
> >>> + mutex_unlock(&csi2rx->lock);
> >>> +
> >>> + pm_runtime_force_suspend(dev);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int __maybe_unused csi2rx_resume(struct device *dev) {
> >>> + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> >>> + int ret;
> >>> +
> >>> + ret = pm_runtime_force_resume(dev);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + mutex_lock(&csi2rx->lock);
> >>> + if (csi2rx->count)
> >>> + csi2rx_start(csi2rx);
> >>> + mutex_unlock(&csi2rx->lock);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static const struct dev_pm_ops csi2rx_pm_ops = {
> >>> SET_RUNTIME_PM_OPS(csi2rx_runtime_suspend,
> >> csi2rx_runtime_resume,
> >>> NULL)
> >>> + SET_SYSTEM_SLEEP_PM_OPS(csi2rx_suspend, csi2rx_resume)
> >>> };
> >>>
> >>> static const struct of_device_id csi2rx_of_table[] = {
> >>
> >> If I'm not mistaken, this is a subdev driver, and is somewhere in the middle of
> >> the pipeline. Afaiu, only the driver that handles the v4l2 video devices should
> >> have system suspend hooks. The job of that driver is then to disable or enable
> >> the pipeline using v4l2 functions, and for the rest of the pipeline system
> >> suspend looks just like a normal pipeline disable.
> >>
> >
> > I see that the imx219 has a commit:
> >
> > commit b8074db07429b845b805416d261b502f814a80fe
> > Author: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Date: Thu Sep 14 21:16:49 2023 +0300
> >
> > media: i2c: imx219: Drop system suspend and resume handlers
> >
> > Stopping streaming on a camera pipeline at system suspend time, and
> > restarting it at system resume time, requires coordinated action between
> > the bridge driver and the camera sensor driver. This is handled by the
> > bridge driver calling the sensor's .s_stream() handler at system suspend
> > and resume time. There is thus no need for the sensor to independently
> > implement system sleep PM operations. Drop them.
> >
> > The streaming field of the driver's private structure is now unused,
> > drop it as well.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> >
> > Implement the system PM of sensor using bridge. This csi2rx is also a bridge.
> > So I add system PM in this driver.
>
> It is not a bridge in the sense that the commit message means. The
> system suspend should be handled in the last (or first, depending on
> which way you think about it) driver in the pipeline, the one that
> handles the VIDIOC_STREAMON.
That's right. Suspending/resuming a running camera pipeline is a complex
operation that requires careful sequencing. For instance, if you resume
a MIPI CSI-2 camera sensor before the CSI-2 receiver, it will switch its
clock and data lanes to HS mode before the CSI-2 receiver is ready to
synchronize.
These sequencing requirements are already handled at VIDIOC_STREAMON and
VIDIOC_STREAMOFF time. That's why we leverage that code for the
suspend/resume implementation, the "main" driver that handles the video
nodes and implements stream start/stop has to stop the pipeline at
suspend time and restart it at resume time. Drivers for components along
the pipeline will then see their .s_stream() operation being called, as
done when starting/stopping the pipeline normally. They don't have to
implement anything specific related to pipeline stop/start in their
system suspend/resume handlers.
--
Regards,
Laurent Pinchart
