Re: 回复: [PATCH v2 2/5] media: cadence: csi2rx: Add system PM support

[Laurent Pinchart]

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