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

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

[Changhuang Liang]

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

[PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM

[Changhuang Liang]

Use runtime power management hooks to save power when CSI-RX is not in
use.

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;
+
+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

Re: [PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM

[Jacopo Mondi]

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
>
>

Re: [PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM

[kernel test robot]

Hi Changhuang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linuxtv-media-stage/master staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.10 next-20240718]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Changhuang-Liang/media-cadence-csi2rx-Support-runtime-PM/20240718-131216
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240718032834.53876-2-changhuang.liang%40starfivetech.com
patch subject: [PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240718/202407182235.kxDoVX8T-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240718/202407182235.kxDoVX8T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407182235.kxDoVX8T-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/platform/cadence/cdns-csi2rx.c:739:12: warning: 'csi2rx_runtime_resume' defined but not used [-Wunused-function]
     739 | static int csi2rx_runtime_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/cadence/cdns-csi2rx.c:720:12: warning: 'csi2rx_runtime_suspend' defined but not used [-Wunused-function]
     720 | static int csi2rx_runtime_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~


vim +/csi2rx_runtime_resume +739 drivers/media/platform/cadence/cdns-csi2rx.c

   719	
 > 720	static int csi2rx_runtime_suspend(struct device *dev)
   721	{
   722		struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
   723		unsigned int i;
   724	
   725		reset_control_assert(csi2rx->sys_rst);
   726		clk_disable_unprepare(csi2rx->sys_clk);
   727	
   728		for (i = 0; i < csi2rx->max_streams; i++) {
   729			reset_control_assert(csi2rx->pixel_rst[i]);
   730			clk_disable_unprepare(csi2rx->pixel_clk[i]);
   731		}
   732	
   733		reset_control_assert(csi2rx->p_rst);
   734		clk_disable_unprepare(csi2rx->p_clk);
   735	
   736		return 0;
   737	}
   738	
 > 739	static int csi2rx_runtime_resume(struct device *dev)
   740	{
   741		struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
   742		unsigned int i;
   743		int ret;
   744	
   745		ret = clk_prepare_enable(csi2rx->p_clk);
   746		if (ret)
   747			return ret;
   748	
   749		reset_control_deassert(csi2rx->p_rst);
   750	
   751		for (i = 0; i < csi2rx->max_streams; i++) {
   752			ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
   753			if (ret)
   754				goto err_disable_pixclk;
   755	
   756			reset_control_deassert(csi2rx->pixel_rst[i]);
   757		}
   758	
   759		ret = clk_prepare_enable(csi2rx->sys_clk);
   760		if (ret)
   761			goto err_disable_pixclk;
   762	
   763		reset_control_deassert(csi2rx->sys_rst);
   764	
   765		return ret;
   766	
   767	err_disable_pixclk:
   768		for (; i > 0; i--) {
   769			reset_control_assert(csi2rx->pixel_rst[i - 1]);
   770			clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
   771		}
   772	
   773		reset_control_assert(csi2rx->p_rst);
   774		clk_disable_unprepare(csi2rx->p_clk);
   775	
   776		return ret;
   777	}
   778	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Changhuang Liang]

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[] = {
-- 
2.25.1

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

[Jacopo Mondi]

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
>
>

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

[Tomi Valkeinen]

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

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

[Changhuang Liang]

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

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

[Tomi Valkeinen]

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

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@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.

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/5] staging: media: starfive: Extract the ISP stream on as a helper function

[Changhuang Liang]

Extract the ISP stream on as a helper function and open it, Let the
other files can use it.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../staging/media/starfive/camss/stf-isp.c    | 27 ++++++++++++-------
 .../staging/media/starfive/camss/stf-isp.h    |  2 ++
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/starfive/camss/stf-isp.c b/drivers/staging/media/starfive/camss/stf-isp.c
index 4e6e26736852..8c6388edf049 100644
--- a/drivers/staging/media/starfive/camss/stf-isp.c
+++ b/drivers/staging/media/starfive/camss/stf-isp.c
@@ -55,23 +55,30 @@ int stf_isp_init(struct stfcamss *stfcamss)
 	return 0;
 }
 
-static int isp_set_stream(struct v4l2_subdev *sd, int enable)
+void stf_isp_stream_on(struct stf_isp_dev *isp_dev,
+		       struct v4l2_subdev_state *sd_state)
 {
-	struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
-	struct v4l2_subdev_state *sd_state;
 	struct v4l2_mbus_framefmt *fmt;
 	struct v4l2_rect *crop;
 
-	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
 	fmt = v4l2_subdev_state_get_format(sd_state, STF_ISP_PAD_SINK);
 	crop = v4l2_subdev_state_get_crop(sd_state, STF_ISP_PAD_SRC);
 
-	if (enable) {
-		stf_isp_reset(isp_dev);
-		stf_isp_init_cfg(isp_dev);
-		stf_isp_settings(isp_dev, crop, fmt->code);
-		stf_isp_stream_set(isp_dev);
-	}
+	stf_isp_reset(isp_dev);
+	stf_isp_init_cfg(isp_dev);
+	stf_isp_settings(isp_dev, crop, fmt->code);
+	stf_isp_stream_set(isp_dev);
+}
+
+static int isp_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
+	struct v4l2_subdev_state *sd_state;
+
+	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	if (enable)
+		stf_isp_stream_on(isp_dev, sd_state);
 
 	v4l2_subdev_call(isp_dev->source_subdev, video, s_stream, enable);
 
diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
index 955cbb048363..1a3e8cf7859c 100644
--- a/drivers/staging/media/starfive/camss/stf-isp.h
+++ b/drivers/staging/media/starfive/camss/stf-isp.h
@@ -421,6 +421,8 @@ void stf_isp_init_cfg(struct stf_isp_dev *isp_dev);
 void stf_isp_settings(struct stf_isp_dev *isp_dev,
 		      struct v4l2_rect *crop, u32 mcode);
 void stf_isp_stream_set(struct stf_isp_dev *isp_dev);
+void stf_isp_stream_on(struct stf_isp_dev *isp_dev,
+		       struct v4l2_subdev_state *sd_state);
 int stf_isp_init(struct stfcamss *stfcamss);
 int stf_isp_register(struct stf_isp_dev *isp_dev, struct v4l2_device *v4l2_dev);
 int stf_isp_unregister(struct stf_isp_dev *isp_dev);
-- 
2.25.1

[PATCH v2 4/5] staging: media: starfive: Introduce streaming for ISP subdev

[Changhuang Liang]

Use "streaming" to indicate whether the ISP is in the streaming state.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/staging/media/starfive/camss/stf-isp.c | 6 +++++-
 drivers/staging/media/starfive/camss/stf-isp.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/starfive/camss/stf-isp.c b/drivers/staging/media/starfive/camss/stf-isp.c
index 8c6388edf049..d71b95ebd3d3 100644
--- a/drivers/staging/media/starfive/camss/stf-isp.c
+++ b/drivers/staging/media/starfive/camss/stf-isp.c
@@ -77,8 +77,12 @@ static int isp_set_stream(struct v4l2_subdev *sd, int enable)
 
 	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
 
-	if (enable)
+	if (enable) {
+		isp_dev->streaming = true;
 		stf_isp_stream_on(isp_dev, sd_state);
+	} else {
+		isp_dev->streaming = false;
+	}
 
 	v4l2_subdev_call(isp_dev->source_subdev, video, s_stream, enable);
 
diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
index 1a3e8cf7859c..85ca1c210639 100644
--- a/drivers/staging/media/starfive/camss/stf-isp.h
+++ b/drivers/staging/media/starfive/camss/stf-isp.h
@@ -414,6 +414,7 @@ struct stf_isp_dev {
 	unsigned int nformats;
 	struct v4l2_subdev *source_subdev;
 	const struct stf_isp_format *current_fmt;
+	bool streaming;
 };
 
 int stf_isp_reset(struct stf_isp_dev *isp_dev);
-- 
2.25.1

[PATCH v2 5/5] staging: media: starfive: Add system PM support

[Changhuang Liang]

This patch implements system suspend and system resume operation for
StarFive Camera Subsystem. It supports hibernation during streaming
and restarts streaming at system resume time.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../staging/media/starfive/camss/stf-camss.c  | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/staging/media/starfive/camss/stf-camss.c b/drivers/staging/media/starfive/camss/stf-camss.c
index fecd3e67c7a1..8dcd35aef69d 100644
--- a/drivers/staging/media/starfive/camss/stf-camss.c
+++ b/drivers/staging/media/starfive/camss/stf-camss.c
@@ -416,10 +416,59 @@ static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused stfcamss_suspend(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+	struct stfcamss_video *video;
+	unsigned int i;
+
+	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
+		video = &stfcamss->captures[i].video;
+		if (video->vb2_q.streaming) {
+			video->ops->stop_streaming(video);
+			video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
+		}
+	}
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused stfcamss_resume(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
+	struct v4l2_subdev_state *sd_state;
+	struct stfcamss_video *video;
+	unsigned int i;
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to resume\n");
+		return ret;
+	}
+
+	sd_state = v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
+
+	if (isp_dev->streaming)
+		stf_isp_stream_on(isp_dev, sd_state);
+
+	v4l2_subdev_unlock_state(sd_state);
+
+	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
+		video = &stfcamss->captures[i].video;
+		if (video->vb2_q.streaming)
+			video->ops->start_streaming(video);
+	}
+
+	return 0;
+}
+
 static const struct dev_pm_ops stfcamss_pm_ops = {
 	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
 			   stfcamss_runtime_resume,
 			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(stfcamss_suspend, stfcamss_resume)
 };
 
 static struct platform_driver stfcamss_driver = {
-- 
2.25.1

Re: [PATCH v2 5/5] staging: media: starfive: Add system PM support

[Jacopo Mondi]

Hi Changhuang

On Wed, Jul 17, 2024 at 08:28:34PM GMT, Changhuang Liang wrote:
> This patch implements system suspend and system resume operation for
> StarFive Camera Subsystem. It supports hibernation during streaming
> and restarts streaming at system resume time.
>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../staging/media/starfive/camss/stf-camss.c  | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/staging/media/starfive/camss/stf-camss.c b/drivers/staging/media/starfive/camss/stf-camss.c
> index fecd3e67c7a1..8dcd35aef69d 100644
> --- a/drivers/staging/media/starfive/camss/stf-camss.c
> +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> @@ -416,10 +416,59 @@ static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
>  	return 0;
>  }
>
> +static int __maybe_unused stfcamss_suspend(struct device *dev)
> +{
> +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> +	struct stfcamss_video *video;

Can be declared inside the for loop

> +	unsigned int i;
> +
> +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {

Likewise, if you like it, you can

        for (unsigned int i...

> +		video = &stfcamss->captures[i].video;
> +		if (video->vb2_q.streaming) {
> +			video->ops->stop_streaming(video);
> +			video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> +		}
> +	}
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int __maybe_unused stfcamss_resume(struct device *dev)
> +{
> +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> +	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> +	struct v4l2_subdev_state *sd_state;
> +	struct stfcamss_video *video;
> +	unsigned int i;

same here

> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to resume\n");
> +		return ret;
> +	}
> +
> +	sd_state = v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
> +
> +	if (isp_dev->streaming)
> +		stf_isp_stream_on(isp_dev, sd_state);

I was wondering if you shouldn't propagate start_streaming along the
whole pipline, but I presume the connected subdevs have to handle
resuming streaming after a system resume themselves ?


> +
> +	v4l2_subdev_unlock_state(sd_state);
> +
> +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> +		video = &stfcamss->captures[i].video;
> +		if (video->vb2_q.streaming)
> +			video->ops->start_streaming(video);

You can use vb2_is_streaming() maybe.
If the queue is streaming, do you need to keep a 'streaming' flag for
the isp ? Probably yes, as the ISP subdev is used by several video
nodes ?

> +	}
> +
> +	return 0;
> +}
> +
>  static const struct dev_pm_ops stfcamss_pm_ops = {
>  	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
>  			   stfcamss_runtime_resume,
>  			   NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(stfcamss_suspend, stfcamss_resume)
>  };
>
>  static struct platform_driver stfcamss_driver = {
> --
> 2.25.1
>

回复: [PATCH v2 5/5] staging: media: starfive: Add system PM support

[Changhuang Liang]

Hi Jacopo,

Thanks for your comments.

> 
> Hi Changhuang
> 
> On Wed, Jul 17, 2024 at 08:28:34PM GMT, Changhuang Liang wrote:
> > This patch implements system suspend and system resume operation for
> > StarFive Camera Subsystem. It supports hibernation during streaming
> > and restarts streaming at system resume time.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > ---
> >  .../staging/media/starfive/camss/stf-camss.c  | 49
> > +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > b/drivers/staging/media/starfive/camss/stf-camss.c
> > index fecd3e67c7a1..8dcd35aef69d 100644
> > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > @@ -416,10 +416,59 @@ static int __maybe_unused
> stfcamss_runtime_resume(struct device *dev)
> >  	return 0;
> >  }
> >
> > +static int __maybe_unused stfcamss_suspend(struct device *dev) {
> > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > +	struct stfcamss_video *video;
> 
> Can be declared inside the for loop
> 
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> 
> Likewise, if you like it, you can
> 
>         for (unsigned int i...
> 
> > +		video = &stfcamss->captures[i].video;
> > +		if (video->vb2_q.streaming) {
> > +			video->ops->stop_streaming(video);
> > +			video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> > +		}
> > +	}
> > +
> > +	return pm_runtime_force_suspend(dev); }
> > +
> > +static int __maybe_unused stfcamss_resume(struct device *dev) {
> > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > +	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > +	struct v4l2_subdev_state *sd_state;
> > +	struct stfcamss_video *video;
> > +	unsigned int i;
> 
> same here
> 
> > +	int ret;
> > +
> > +	ret = pm_runtime_force_resume(dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to resume\n");
> > +		return ret;
> > +	}
> > +
> > +	sd_state = v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
> > +
> > +	if (isp_dev->streaming)
> > +		stf_isp_stream_on(isp_dev, sd_state);
> 
> I was wondering if you shouldn't propagate start_streaming along the whole
> pipline, but I presume the connected subdevs have to handle resuming
> streaming after a system resume themselves ?
> 

Currently our Camera subsystem contains ISP subdev , capture_raw video device, and capture_yuv
video device. So you can see only one system PM hook can be used by them.

> 
> > +
> > +	v4l2_subdev_unlock_state(sd_state);
> > +
> > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> > +		video = &stfcamss->captures[i].video;
> > +		if (video->vb2_q.streaming)
> > +			video->ops->start_streaming(video);
> 
> You can use vb2_is_streaming() maybe.
> If the queue is streaming, do you need to keep a 'streaming' flag for the isp ?
> Probably yes, as the ISP subdev is used by several video nodes ?
> 

I set the "streaming" flag in PATCH 4, so it does not affect that even if several video 
nodes use it.

Regards,
Changhuang

Re: 回复: [PATCH v2 5/5] staging: media: starfive: Add system PM support

[Jacopo Mondi]

Hi Changhuang

On Fri, Jul 19, 2024 at 02:08:20AM GMT, Changhuang Liang wrote:
> Hi Jacopo,
>
> Thanks for your comments.
>
> >
> > Hi Changhuang
> >
> > On Wed, Jul 17, 2024 at 08:28:34PM GMT, Changhuang Liang wrote:
> > > This patch implements system suspend and system resume operation for
> > > StarFive Camera Subsystem. It supports hibernation during streaming
> > > and restarts streaming at system resume time.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  .../staging/media/starfive/camss/stf-camss.c  | 49
> > > +++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > > b/drivers/staging/media/starfive/camss/stf-camss.c
> > > index fecd3e67c7a1..8dcd35aef69d 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > > @@ -416,10 +416,59 @@ static int __maybe_unused
> > stfcamss_runtime_resume(struct device *dev)
> > >  	return 0;
> > >  }
> > >
> > > +static int __maybe_unused stfcamss_suspend(struct device *dev) {
> > > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > > +	struct stfcamss_video *video;
> >
> > Can be declared inside the for loop
> >
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> >
> > Likewise, if you like it, you can
> >
> >         for (unsigned int i...
> >
> > > +		video = &stfcamss->captures[i].video;
> > > +		if (video->vb2_q.streaming) {
> > > +			video->ops->stop_streaming(video);
> > > +			video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> > > +		}
> > > +	}
> > > +
> > > +	return pm_runtime_force_suspend(dev); }
> > > +
> > > +static int __maybe_unused stfcamss_resume(struct device *dev) {
> > > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > > +	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > > +	struct v4l2_subdev_state *sd_state;
> > > +	struct stfcamss_video *video;
> > > +	unsigned int i;
> >
> > same here
> >
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_force_resume(dev);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "Failed to resume\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
> > > +
> > > +	if (isp_dev->streaming)
> > > +		stf_isp_stream_on(isp_dev, sd_state);
> >
> > I was wondering if you shouldn't propagate start_streaming along the whole
> > pipline, but I presume the connected subdevs have to handle resuming
> > streaming after a system resume themselves ?
> >
>
> Currently our Camera subsystem contains ISP subdev , capture_raw video device, and capture_yuv
> video device. So you can see only one system PM hook can be used by them.
>

Sorry, maybe I was not clear (and I was probably confused as well).

You are right this is the main entry point for system sleep PM hooks

> >
> > > +
> > > +	v4l2_subdev_unlock_state(sd_state);
> > > +
> > > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> > > +		video = &stfcamss->captures[i].video;
> > > +		if (video->vb2_q.streaming)
> > > +			video->ops->start_streaming(video);

And here you propagate the start_streaming (and stop_streaming on
suspend) call to all your video devices.

I see your video devices propagating the s_stream call to their
'source_subdev'. And your ISP subdev doing the same in
'isp_set_stream()'.

According to the media graph in
Documentation/admin-guide/media/starfive_camss_graph.dot

your 'capture_yuv' video device is connected to your ISP, and your
'capture_raw' video device is connected to your 'CSI-RX' subdev.

If my understanding is correct, your CSI-RX subdev will receive 2
calls to s_stream() (one from the ISP subdev and one from the
'capture_raw' video device). Am I mistaken maybe ?

Also, if the CSI-RX subdev is already part of a capture pipeline, as
Tomi pointed out in his review of patch [2/5] it doesn't need to
implement handlers for system suspend/resume.


> >
> > You can use vb2_is_streaming() maybe.

I was suggesting to use vb2_is_streaming() instead of openly code

		if (video->vb2_q.streaming)

> > If the queue is streaming, do you need to keep a 'streaming' flag for the isp ?
> > Probably yes, as the ISP subdev is used by several video nodes ?
> >
>
> I set the "streaming" flag in PATCH 4, so it does not affect that even if several video
> nodes use it.

Yeah I was wondering if you could have saved manually tracking the
streaming state in the isp (re-reading the patches, where do you
actually use the 'streaming' flag in the ISP subdev ?) by tracking the
vb2_queue state.

>
> Regards,
> Changhuang

回复: 回复: [PATCH v2 5/5] staging: media: starfive: Add system PM support

[Changhuang Liang]

Hi, Jacopo

> 
> Hi Changhuang
> 
> On Fri, Jul 19, 2024 at 02:08:20AM GMT, Changhuang Liang wrote:
> > Hi Jacopo,
> >
> > Thanks for your comments.
> >
> > >
> > > Hi Changhuang
> > >
> > > On Wed, Jul 17, 2024 at 08:28:34PM GMT, Changhuang Liang wrote:
> > > > This patch implements system suspend and system resume operation
> > > > for StarFive Camera Subsystem. It supports hibernation during
> > > > streaming and restarts streaming at system resume time.
> > > >
> > > > Signed-off-by: Changhuang Liang
> > > > <changhuang.liang@starfivetech.com>
> > > > ---
> > > >  .../staging/media/starfive/camss/stf-camss.c  | 49
> > > > +++++++++++++++++++
> > > >  1 file changed, 49 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > > > b/drivers/staging/media/starfive/camss/stf-camss.c
> > > > index fecd3e67c7a1..8dcd35aef69d 100644
> > > > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > > > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > > > @@ -416,10 +416,59 @@ static int __maybe_unused
> > > stfcamss_runtime_resume(struct device *dev)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int __maybe_unused stfcamss_suspend(struct device *dev) {
> > > > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > > > +	struct stfcamss_video *video;
> > >
> > > Can be declared inside the for loop
> > >
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> > >
> > > Likewise, if you like it, you can
> > >
> > >         for (unsigned int i...
> > >
> > > > +		video = &stfcamss->captures[i].video;
> > > > +		if (video->vb2_q.streaming) {
> > > > +			video->ops->stop_streaming(video);
> > > > +			video->ops->flush_buffers(video,
> VB2_BUF_STATE_ERROR);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return pm_runtime_force_suspend(dev); }
> > > > +
> > > > +static int __maybe_unused stfcamss_resume(struct device *dev) {
> > > > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > > > +	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > > > +	struct v4l2_subdev_state *sd_state;
> > > > +	struct stfcamss_video *video;
> > > > +	unsigned int i;
> > >
> > > same here
> > >
> > > > +	int ret;
> > > > +
> > > > +	ret = pm_runtime_force_resume(dev);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to resume\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	sd_state =
> > > > +v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
> > > > +
> > > > +	if (isp_dev->streaming)
> > > > +		stf_isp_stream_on(isp_dev, sd_state);
> > >
> > > I was wondering if you shouldn't propagate start_streaming along the
> > > whole pipline, but I presume the connected subdevs have to handle
> > > resuming streaming after a system resume themselves ?
> > >
> >
> > Currently our Camera subsystem contains ISP subdev , capture_raw video
> > device, and capture_yuv video device. So you can see only one system PM
> hook can be used by them.
> >
> 
> Sorry, maybe I was not clear (and I was probably confused as well).
> 
> You are right this is the main entry point for system sleep PM hooks
> 
> > >
> > > > +
> > > > +	v4l2_subdev_unlock_state(sd_state);
> > > > +
> > > > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> > > > +		video = &stfcamss->captures[i].video;
> > > > +		if (video->vb2_q.streaming)
> > > > +			video->ops->start_streaming(video);
> 
> And here you propagate the start_streaming (and stop_streaming on
> suspend) call to all your video devices.
> 
> I see your video devices propagating the s_stream call to their
> 'source_subdev'. And your ISP subdev doing the same in 'isp_set_stream()'.
> 
> According to the media graph in
> Documentation/admin-guide/media/starfive_camss_graph.dot
> 
> your 'capture_yuv' video device is connected to your ISP, and your
> 'capture_raw' video device is connected to your 'CSI-RX' subdev.
> 
> If my understanding is correct, your CSI-RX subdev will receive 2 calls to
> s_stream() (one from the ISP subdev and one from the 'capture_raw' video
> device). Am I mistaken maybe ?
> 
> Also, if the CSI-RX subdev is already part of a capture pipeline, as Tomi pointed
> out in his review of patch [2/5] it doesn't need to implement handlers for
> system suspend/resume.
> 

Currently this version start streaming and stop streaming are called

static const struct stfcamss_video_ops stf_capture_ops = {
	.queue_buffer = stf_queue_buffer,
	.flush_buffers = stf_flush_buffers,
	.start_streaming = stf_capture_start,
	.stop_streaming = stf_capture_stop,
};

This two hooks will not propagate streaming.

Maybe I should change to use this in next version:

static const struct vb2_ops stf_video_vb2_q_ops = {
	.queue_setup     = video_queue_setup,
	.wait_prepare    = vb2_ops_wait_prepare,
	.wait_finish     = vb2_ops_wait_finish,
	.buf_init        = video_buf_init,
	.buf_prepare     = video_buf_prepare,
	.buf_queue       = video_buf_queue,
	.start_streaming = video_start_streaming,
	.stop_streaming  = video_stop_streaming,
};

This two hooks will propagate streaming.

> 
> > >
> > > You can use vb2_is_streaming() maybe.
> 
> I was suggesting to use vb2_is_streaming() instead of openly code
> 
> 		if (video->vb2_q.streaming)
> 
> > > If the queue is streaming, do you need to keep a 'streaming' flag for the
> isp ?
> > > Probably yes, as the ISP subdev is used by several video nodes ?
> > >
> >
> > I set the "streaming" flag in PATCH 4, so it does not affect that even
> > if several video nodes use it.
> 
> Yeah I was wondering if you could have saved manually tracking the streaming
> state in the isp (re-reading the patches, where do you actually use the
> 'streaming' flag in the ISP subdev ?) by tracking the vb2_queue state.
> 

If use propagate streaming on pipeline, maybe can drop the streaming flag
for subdev. 

Regards,
Changhuang