| Message ID | 20240718032834.53876-3-changhuang.liang@starfivetech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add StarFive Camera Subsystem hibernation support | expand |
Hi Changhuang On Wed, Jul 17, 2024 at 08:28:31PM GMT, 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@starfivetech.com> Looks ok to me! Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > 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[] = { > -- > 2.25.1 > >
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@starfivetech.com> > --- > 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. Tomi
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@starfivetech.com> > > --- > > 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@ideasonboard.com> 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@ideasonboard.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Implement the system PM of sensor using bridge. This csi2rx is also a bridge. So I add system PM in this driver. Ragards, Changhuang
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@starfivetech.com> >>> --- >>> 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@ideasonboard.com> > 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@ideasonboard.com> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > 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. Tomi
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@starfivetech.com> > >>> --- > >>> 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@ideasonboard.com> > > 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@ideasonboard.com> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > > 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.
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[] = {
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@starfivetech.com> --- drivers/media/platform/cadence/cdns-csi2rx.c | 32 ++++++++++++++++++++ 1 file changed, 32 insertions(+)