mbox series

[v2,0/5] Add StarFive Camera Subsystem hibernation support

Message ID 20240718032834.53876-1-changhuang.liang@starfivetech.com
Headers show
Series Add StarFive Camera Subsystem hibernation support | expand

Message

Changhuang Liang July 18, 2024, 3:28 a.m. UTC
This series implements suspend and resume operation for StarFive Camera
Subsystem on JH7110 SoC. It supports hibernation during streaming and
restarts streaming at the system resume time.

changes since v1:
- csi2rx runtime PM not control the streaming.
- Introduce streaming for ISP subdev due to the struct subdev member
  "enabled_streams" has been deleted.

v1: https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@starfivetech.com/

Changhuang Liang (5):
  media: cadence: csi2rx: Support runtime PM
  media: cadence: csi2rx: Add system PM support
  staging: media: starfive: Extract the ISP stream on as a helper
    function
  staging: media: starfive: Introduce streaming for ISP subdev
  staging: media: starfive: Add system PM support

 drivers/media/platform/cadence/cdns-csi2rx.c  | 153 +++++++++++++-----
 .../staging/media/starfive/camss/stf-camss.c  |  49 ++++++
 .../staging/media/starfive/camss/stf-isp.c    |  27 +++-
 .../staging/media/starfive/camss/stf-isp.h    |   3 +
 4 files changed, 183 insertions(+), 49 deletions(-)

--
2.25.1

Comments

Jacopo Mondi July 18, 2024, 2:24 p.m. UTC | #1
Hello Changhuang

On Wed, Jul 17, 2024 at 08:28:30PM GMT, Changhuang Liang wrote:
> Use runtime power management hooks to save power when CSI-RX is not in
> use.
>

The driver does not depend on PM afaict and the IP can be integrated in
different SoCs and not all of them might select PM.

Either make it depend on PM in Kconfig or manually enable the device during
probe (see the many examples of drivers manually enabling the device
in probe() then calling pm_runtime_set_active(), pm_runtime_enable()
and pm_request_idle()).

Also, you might want to consider autosuspend delay. Autosuspend delay
avoids power cycling the interface by delaying suspend by a certain
amout of time, in case it resumed immediately.

> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  drivers/media/platform/cadence/cdns-csi2rx.c | 121 ++++++++++++-------
>  1 file changed, 80 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 6f7d27a48eff..981819adbb3a 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -211,11 +211,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	u32 reg;
>  	int ret;
>
> -	ret = clk_prepare_enable(csi2rx->p_clk);
> -	if (ret)
> -		return ret;
> -
> -	reset_control_deassert(csi2rx->p_rst);
>  	csi2rx_reset(csi2rx);
>
>  	reg = csi2rx->num_lanes << 8;
> @@ -253,7 +248,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		if (ret) {
>  			dev_err(csi2rx->dev,
>  				"Failed to configure external DPHY: %d\n", ret);
> -			goto err_disable_pclk;
> +			return ret;
>  		}
>  	}
>
> @@ -268,11 +263,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	 * hence the reference counting.
>  	 */
>  	for (i = 0; i < csi2rx->max_streams; i++) {
> -		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> -		if (ret)
> -			goto err_disable_pixclk;
> -
> -		reset_control_deassert(csi2rx->pixel_rst[i]);
>
>  		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
>  		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> @@ -288,34 +278,18 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>  	}
>
> -	ret = clk_prepare_enable(csi2rx->sys_clk);
> -	if (ret)
> -		goto err_disable_pixclk;
> -
> -	reset_control_deassert(csi2rx->sys_rst);
>
>  	ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
>  	if (ret)
> -		goto err_disable_sysclk;
> -
> -	clk_disable_unprepare(csi2rx->p_clk);
> +		goto err_phy_power_off;
>
>  	return 0;
>
> -err_disable_sysclk:
> -	clk_disable_unprepare(csi2rx->sys_clk);
> -err_disable_pixclk:
> -	for (; i > 0; i--) {
> -		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
> -	}
> -
> +err_phy_power_off:
>  	if (csi2rx->dphy) {
>  		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>  		phy_power_off(csi2rx->dphy);
>  	}
> -err_disable_pclk:
> -	clk_disable_unprepare(csi2rx->p_clk);
>
>  	return ret;
>  }
> @@ -326,10 +300,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	u32 val;
>  	int ret;
>
> -	clk_prepare_enable(csi2rx->p_clk);
> -	reset_control_assert(csi2rx->sys_rst);
> -	clk_disable_unprepare(csi2rx->sys_clk);
> -
>  	for (i = 0; i < csi2rx->max_streams; i++) {
>  		writel(CSI2RX_STREAM_CTRL_STOP,
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> @@ -342,14 +312,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  		if (ret)
>  			dev_warn(csi2rx->dev,
>  				 "Failed to stop streaming on pad%u\n", i);
> -
> -		reset_control_assert(csi2rx->pixel_rst[i]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i]);
>  	}
>
> -	reset_control_assert(csi2rx->p_rst);
> -	clk_disable_unprepare(csi2rx->p_clk);
> -
>  	if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
>  		dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
>
> @@ -374,9 +338,15 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>  		 * enable the whole controller.
>  		 */
>  		if (!csi2rx->count) {
> +			ret = pm_runtime_resume_and_get(csi2rx->dev);
> +			if (ret < 0)
> +				goto out;
> +
>  			ret = csi2rx_start(csi2rx);
> -			if (ret)
> +			if (ret) {
> +				pm_runtime_put(csi2rx->dev);
>  				goto out;
> +			}
>  		}
>
>  		csi2rx->count++;
> @@ -386,8 +356,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>  		/*
>  		 * Let the last user turn off the lights.
>  		 */
> -		if (!csi2rx->count)
> +		if (!csi2rx->count) {
>  			csi2rx_stop(csi2rx);
> +			pm_runtime_put(csi2rx->dev);
> +		}
>  	}
>
>  out:
> @@ -707,6 +679,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>
> +	pm_runtime_enable(csi2rx->dev);
>  	ret = v4l2_async_register_subdev(&csi2rx->subdev);
>  	if (ret < 0)
>  		goto err_free_state;
> @@ -721,6 +694,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>
>  err_free_state:
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
> +	pm_runtime_disable(csi2rx->dev);
>  err_cleanup:
>  	v4l2_async_nf_unregister(&csi2rx->notifier);
>  	v4l2_async_nf_cleanup(&csi2rx->notifier);
> @@ -739,9 +713,73 @@ static void csi2rx_remove(struct platform_device *pdev)
>  	v4l2_async_unregister_subdev(&csi2rx->subdev);
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
>  	media_entity_cleanup(&csi2rx->subdev.entity);
> +	pm_runtime_disable(csi2rx->dev);
>  	kfree(csi2rx);
>  }
>
> +static int csi2rx_runtime_suspend(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	reset_control_assert(csi2rx->sys_rst);
> +	clk_disable_unprepare(csi2rx->sys_clk);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		reset_control_assert(csi2rx->pixel_rst[i]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i]);
> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_runtime_resume(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +	int ret;
> +
> +	ret = clk_prepare_enable(csi2rx->p_clk);
> +	if (ret)
> +		return ret;
> +
> +	reset_control_deassert(csi2rx->p_rst);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> +		if (ret)
> +			goto err_disable_pixclk;
> +
> +		reset_control_deassert(csi2rx->pixel_rst[i]);
> +	}
> +
> +	ret = clk_prepare_enable(csi2rx->sys_clk);
> +	if (ret)
> +		goto err_disable_pixclk;
> +
> +	reset_control_deassert(csi2rx->sys_rst);
> +
> +	return ret;

You can return 0 here

Thanks
   j

> +
> +err_disable_pixclk:
> +	for (; i > 0; i--) {
> +		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops csi2rx_pm_ops = {
> +	SET_RUNTIME_PM_OPS(csi2rx_runtime_suspend, csi2rx_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id csi2rx_of_table[] = {
>  	{ .compatible = "starfive,jh7110-csi2rx" },
>  	{ .compatible = "cdns,csi2rx" },
> @@ -756,6 +794,7 @@ static struct platform_driver csi2rx_driver = {
>  	.driver	= {
>  		.name		= "cdns-csi2rx",
>  		.of_match_table	= csi2rx_of_table,
> +		.pm		= &csi2rx_pm_ops,
>  	},
>  };
>  module_platform_driver(csi2rx_driver);
> --
> 2.25.1
>
>
Tomi Valkeinen July 19, 2024, 10:28 a.m. UTC | #2
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
Changhuang Liang July 22, 2024, 2:17 a.m. UTC | #3
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
Tomi Valkeinen July 22, 2024, 8:53 a.m. UTC | #4
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
Laurent Pinchart July 22, 2024, 11:25 a.m. UTC | #5
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.