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