[PATCH] media: cedrus: Implement runtime PM

[PATCH] media: cedrus: Implement runtime PM

[Samuel Holland]

This allows the VE clocks and PLL_VE to be disabled most of the time.

Since the device is stateless, each frame gets a separate runtime PM
reference. Enable autosuspend so the PM callbacks are not run before and
after every frame.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

I tested this with v4l2-request-test. I don't have the setup to do
anything more complicated at the moment.

---
 drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
 .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
 .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
 3 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 3fad5edccd17..9aa1fc8a6c26 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm.h>
 
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -474,6 +475,11 @@ static int cedrus_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct dev_pm_ops cedrus_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
+			   cedrus_hw_resume, NULL)
+};
+
 static const struct cedrus_variant sun4i_a10_cedrus_variant = {
 	.mod_rate	= 320000000,
 };
@@ -559,6 +565,7 @@ static struct platform_driver cedrus_driver = {
 	.driver		= {
 		.name		= CEDRUS_NAME,
 		.of_match_table	= of_match_ptr(cedrus_dt_match),
+		.pm		= &cedrus_dev_pm_ops,
 	},
 };
 module_platform_driver(cedrus_driver);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index daf5f244f93b..b84814d5afe4 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -19,6 +19,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/clk.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/soc/sunxi/sunxi_sram.h>
@@ -63,6 +64,8 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
 	if (ctx->src_fmt.width > 2048)
 		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
 
+	pm_runtime_get_sync(ctx->dev->dev);
+
 	cedrus_write(ctx->dev, VE_MODE, reg);
 
 	return 0;
@@ -71,6 +74,9 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
 void cedrus_engine_disable(struct cedrus_dev *dev)
 {
 	cedrus_write(dev, VE_MODE, VE_MODE_DISABLED);
+
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
 }
 
 void cedrus_dst_format_set(struct cedrus_dev *dev,
@@ -134,12 +140,72 @@ static irqreturn_t cedrus_irq(int irq, void *data)
 	else
 		state = VB2_BUF_STATE_DONE;
 
+	cedrus_engine_disable(dev);
+
 	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
 					 state);
 
 	return IRQ_HANDLED;
 }
 
+int cedrus_hw_resume(struct device *d)
+{
+	struct cedrus_dev *dev = dev_get_drvdata(d);
+	int ret;
+
+	ret = clk_prepare_enable(dev->ahb_clk);
+	if (ret) {
+		dev_err(dev->dev, "Failed to enable AHB clock\n");
+
+		return ret;
+	}
+
+	ret = clk_prepare_enable(dev->mod_clk);
+	if (ret) {
+		dev_err(dev->dev, "Failed to enable MOD clock\n");
+
+		goto err_ahb_clk;
+	}
+
+	ret = clk_prepare_enable(dev->ram_clk);
+	if (ret) {
+		dev_err(dev->dev, "Failed to enable RAM clock\n");
+
+		goto err_mod_clk;
+	}
+
+	ret = reset_control_reset(dev->rstc);
+	if (ret) {
+		dev_err(dev->dev, "Failed to apply reset\n");
+
+		goto err_ram_clk;
+	}
+
+	return 0;
+
+err_ram_clk:
+	clk_disable_unprepare(dev->ram_clk);
+err_mod_clk:
+	clk_disable_unprepare(dev->mod_clk);
+err_ahb_clk:
+	clk_disable_unprepare(dev->ahb_clk);
+
+	return ret;
+}
+
+int cedrus_hw_suspend(struct device *d)
+{
+	struct cedrus_dev *dev = dev_get_drvdata(d);
+
+	reset_control_assert(dev->rstc);
+
+	clk_disable_unprepare(dev->ram_clk);
+	clk_disable_unprepare(dev->mod_clk);
+	clk_disable_unprepare(dev->ahb_clk);
+
+	return 0;
+}
+
 int cedrus_hw_probe(struct cedrus_dev *dev)
 {
 	const struct cedrus_variant *variant;
@@ -236,42 +302,19 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
 		goto err_sram;
 	}
 
-	ret = clk_prepare_enable(dev->ahb_clk);
-	if (ret) {
-		dev_err(dev->dev, "Failed to enable AHB clock\n");
-
-		goto err_sram;
-	}
-
-	ret = clk_prepare_enable(dev->mod_clk);
-	if (ret) {
-		dev_err(dev->dev, "Failed to enable MOD clock\n");
-
-		goto err_ahb_clk;
-	}
-
-	ret = clk_prepare_enable(dev->ram_clk);
-	if (ret) {
-		dev_err(dev->dev, "Failed to enable RAM clock\n");
-
-		goto err_mod_clk;
-	}
-
-	ret = reset_control_reset(dev->rstc);
-	if (ret) {
-		dev_err(dev->dev, "Failed to apply reset\n");
-
-		goto err_ram_clk;
+	pm_runtime_set_autosuspend_delay(dev->dev, 1000);
+	pm_runtime_use_autosuspend(dev->dev);
+	pm_runtime_enable(dev->dev);
+	if (!pm_runtime_enabled(dev->dev)) {
+		ret = cedrus_hw_resume(dev->dev);
+		if (ret)
+			goto err_pm;
 	}
 
 	return 0;
 
-err_ram_clk:
-	clk_disable_unprepare(dev->ram_clk);
-err_mod_clk:
-	clk_disable_unprepare(dev->mod_clk);
-err_ahb_clk:
-	clk_disable_unprepare(dev->ahb_clk);
+err_pm:
+	pm_runtime_disable(dev->dev);
 err_sram:
 	sunxi_sram_release(dev->dev);
 err_mem:
@@ -282,11 +325,9 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
 
 void cedrus_hw_remove(struct cedrus_dev *dev)
 {
-	reset_control_assert(dev->rstc);
-
-	clk_disable_unprepare(dev->ram_clk);
-	clk_disable_unprepare(dev->mod_clk);
-	clk_disable_unprepare(dev->ahb_clk);
+	pm_runtime_disable(dev->dev);
+	if (!pm_runtime_status_suspended(dev->dev))
+		cedrus_hw_suspend(dev->dev);
 
 	sunxi_sram_release(dev->dev);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
index 604ff932fbf5..17822b470a1e 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
@@ -22,6 +22,9 @@ void cedrus_engine_disable(struct cedrus_dev *dev);
 void cedrus_dst_format_set(struct cedrus_dev *dev,
 			   struct v4l2_pix_format *fmt);
 
+int cedrus_hw_resume(struct device *dev);
+int cedrus_hw_suspend(struct device *dev);
+
 int cedrus_hw_probe(struct cedrus_dev *dev);
 void cedrus_hw_remove(struct cedrus_dev *dev);
 
-- 
2.24.1

Re: [PATCH] media: cedrus: Implement runtime PM

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On Tue, Apr 07, 2020 at 08:02:32PM -0500, Samuel Holland wrote:
> This allows the VE clocks and PLL_VE to be disabled most of the time.
>
> Since the device is stateless, each frame gets a separate runtime PM
> reference. Enable autosuspend so the PM callbacks are not run before and
> after every frame.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: cedrus: Implement runtime PM

[Paul Kocialkowski]

Hi,

On Tue 07 Apr 20, 20:02, Samuel Holland wrote:
> This allows the VE clocks and PLL_VE to be disabled most of the time.
> 
> Since the device is stateless, each frame gets a separate runtime PM
> reference. Enable autosuspend so the PM callbacks are not run before and
> after every frame.

Looks good, thanks for the contribution :)

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> I tested this with v4l2-request-test. I don't have the setup to do
> anything more complicated at the moment.
> 
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>  3 files changed, 88 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 3fad5edccd17..9aa1fc8a6c26 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pm.h>
>  
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -474,6 +475,11 @@ static int cedrus_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
> +			   cedrus_hw_resume, NULL)
> +};
> +
>  static const struct cedrus_variant sun4i_a10_cedrus_variant = {
>  	.mod_rate	= 320000000,
>  };
> @@ -559,6 +565,7 @@ static struct platform_driver cedrus_driver = {
>  	.driver		= {
>  		.name		= CEDRUS_NAME,
>  		.of_match_table	= of_match_ptr(cedrus_dt_match),
> +		.pm		= &cedrus_dev_pm_ops,
>  	},
>  };
>  module_platform_driver(cedrus_driver);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index daf5f244f93b..b84814d5afe4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -19,6 +19,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/soc/sunxi/sunxi_sram.h>
> @@ -63,6 +64,8 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
>  	if (ctx->src_fmt.width > 2048)
>  		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
>  
> +	pm_runtime_get_sync(ctx->dev->dev);
> +
>  	cedrus_write(ctx->dev, VE_MODE, reg);
>  
>  	return 0;
> @@ -71,6 +74,9 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
>  void cedrus_engine_disable(struct cedrus_dev *dev)
>  {
>  	cedrus_write(dev, VE_MODE, VE_MODE_DISABLED);
> +
> +	pm_runtime_mark_last_busy(dev->dev);
> +	pm_runtime_put_autosuspend(dev->dev);
>  }
>  
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
> @@ -134,12 +140,72 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>  	else
>  		state = VB2_BUF_STATE_DONE;
>  
> +	cedrus_engine_disable(dev);
> +
>  	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
>  					 state);
>  
>  	return IRQ_HANDLED;
>  }
>  
> +int cedrus_hw_resume(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +	int ret;
> +
> +	ret = clk_prepare_enable(dev->ahb_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable AHB clock\n");
> +
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dev->mod_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable MOD clock\n");
> +
> +		goto err_ahb_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dev->ram_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable RAM clock\n");
> +
> +		goto err_mod_clk;
> +	}
> +
> +	ret = reset_control_reset(dev->rstc);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to apply reset\n");
> +
> +		goto err_ram_clk;
> +	}
> +
> +	return 0;
> +
> +err_ram_clk:
> +	clk_disable_unprepare(dev->ram_clk);
> +err_mod_clk:
> +	clk_disable_unprepare(dev->mod_clk);
> +err_ahb_clk:
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return ret;
> +}
> +
> +int cedrus_hw_suspend(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +
> +	reset_control_assert(dev->rstc);
> +
> +	clk_disable_unprepare(dev->ram_clk);
> +	clk_disable_unprepare(dev->mod_clk);
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return 0;
> +}
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev)
>  {
>  	const struct cedrus_variant *variant;
> @@ -236,42 +302,19 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  		goto err_sram;
>  	}
>  
> -	ret = clk_prepare_enable(dev->ahb_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable AHB clock\n");
> -
> -		goto err_sram;
> -	}
> -
> -	ret = clk_prepare_enable(dev->mod_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable MOD clock\n");
> -
> -		goto err_ahb_clk;
> -	}
> -
> -	ret = clk_prepare_enable(dev->ram_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable RAM clock\n");
> -
> -		goto err_mod_clk;
> -	}
> -
> -	ret = reset_control_reset(dev->rstc);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to apply reset\n");
> -
> -		goto err_ram_clk;
> +	pm_runtime_set_autosuspend_delay(dev->dev, 1000);
> +	pm_runtime_use_autosuspend(dev->dev);
> +	pm_runtime_enable(dev->dev);
> +	if (!pm_runtime_enabled(dev->dev)) {
> +		ret = cedrus_hw_resume(dev->dev);
> +		if (ret)
> +			goto err_pm;
>  	}
>  
>  	return 0;
>  
> -err_ram_clk:
> -	clk_disable_unprepare(dev->ram_clk);
> -err_mod_clk:
> -	clk_disable_unprepare(dev->mod_clk);
> -err_ahb_clk:
> -	clk_disable_unprepare(dev->ahb_clk);
> +err_pm:
> +	pm_runtime_disable(dev->dev);
>  err_sram:
>  	sunxi_sram_release(dev->dev);
>  err_mem:
> @@ -282,11 +325,9 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  
>  void cedrus_hw_remove(struct cedrus_dev *dev)
>  {
> -	reset_control_assert(dev->rstc);
> -
> -	clk_disable_unprepare(dev->ram_clk);
> -	clk_disable_unprepare(dev->mod_clk);
> -	clk_disable_unprepare(dev->ahb_clk);
> +	pm_runtime_disable(dev->dev);
> +	if (!pm_runtime_status_suspended(dev->dev))
> +		cedrus_hw_suspend(dev->dev);
>  
>  	sunxi_sram_release(dev->dev);
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> index 604ff932fbf5..17822b470a1e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -22,6 +22,9 @@ void cedrus_engine_disable(struct cedrus_dev *dev);
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
>  			   struct v4l2_pix_format *fmt);
>  
> +int cedrus_hw_resume(struct device *dev);
> +int cedrus_hw_suspend(struct device *dev);
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev);
>  void cedrus_hw_remove(struct cedrus_dev *dev);
>  
> -- 
> 2.24.1
> 

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Jernej Škrabec]

Hi Samuel!

Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> This allows the VE clocks and PLL_VE to be disabled most of the time.
> 
> Since the device is stateless, each frame gets a separate runtime PM
> reference. Enable autosuspend so the PM callbacks are not run before and
> after every frame.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> I tested this with v4l2-request-test. I don't have the setup to do
> anything more complicated at the moment.

Let me test this in LibreELEC with several videos and different SoCs.

Best regards,
Jernej

> 
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>  3 files changed, 88 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> 3fad5edccd17..9aa1fc8a6c26 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pm.h>
> 
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -474,6 +475,11 @@ static int cedrus_remove(struct platform_device *pdev)
>  	return 0;
>  }
> 
> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
> +			   cedrus_hw_resume, NULL)
> +};
> +
>  static const struct cedrus_variant sun4i_a10_cedrus_variant = {
>  	.mod_rate	= 320000000,
>  };
> @@ -559,6 +565,7 @@ static struct platform_driver cedrus_driver = {
>  	.driver		= {
>  		.name		= CEDRUS_NAME,
>  		.of_match_table	= of_match_ptr(cedrus_dt_match),
> +		.pm		= &cedrus_dev_pm_ops,
>  	},
>  };
>  module_platform_driver(cedrus_driver);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> daf5f244f93b..b84814d5afe4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -19,6 +19,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/soc/sunxi/sunxi_sram.h>
> @@ -63,6 +64,8 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum
> cedrus_codec codec) if (ctx->src_fmt.width > 2048)
>  		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> 
> +	pm_runtime_get_sync(ctx->dev->dev);
> +
>  	cedrus_write(ctx->dev, VE_MODE, reg);
> 
>  	return 0;
> @@ -71,6 +74,9 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum
> cedrus_codec codec) void cedrus_engine_disable(struct cedrus_dev *dev)
>  {
>  	cedrus_write(dev, VE_MODE, VE_MODE_DISABLED);
> +
> +	pm_runtime_mark_last_busy(dev->dev);
> +	pm_runtime_put_autosuspend(dev->dev);
>  }
> 
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
> @@ -134,12 +140,72 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>  	else
>  		state = VB2_BUF_STATE_DONE;
> 
> +	cedrus_engine_disable(dev);
> +
>  	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx-
>fh.m2m_ctx,
>  					 state);
> 
>  	return IRQ_HANDLED;
>  }
> 
> +int cedrus_hw_resume(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +	int ret;
> +
> +	ret = clk_prepare_enable(dev->ahb_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable AHB clock\n");
> +
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dev->mod_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable MOD clock\n");
> +
> +		goto err_ahb_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dev->ram_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable RAM clock\n");
> +
> +		goto err_mod_clk;
> +	}
> +
> +	ret = reset_control_reset(dev->rstc);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to apply reset\n");
> +
> +		goto err_ram_clk;
> +	}
> +
> +	return 0;
> +
> +err_ram_clk:
> +	clk_disable_unprepare(dev->ram_clk);
> +err_mod_clk:
> +	clk_disable_unprepare(dev->mod_clk);
> +err_ahb_clk:
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return ret;
> +}
> +
> +int cedrus_hw_suspend(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +
> +	reset_control_assert(dev->rstc);
> +
> +	clk_disable_unprepare(dev->ram_clk);
> +	clk_disable_unprepare(dev->mod_clk);
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return 0;
> +}
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev)
>  {
>  	const struct cedrus_variant *variant;
> @@ -236,42 +302,19 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  		goto err_sram;
>  	}
> 
> -	ret = clk_prepare_enable(dev->ahb_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable AHB clock\n");
> -
> -		goto err_sram;
> -	}
> -
> -	ret = clk_prepare_enable(dev->mod_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable MOD clock\n");
> -
> -		goto err_ahb_clk;
> -	}
> -
> -	ret = clk_prepare_enable(dev->ram_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable RAM clock\n");
> -
> -		goto err_mod_clk;
> -	}
> -
> -	ret = reset_control_reset(dev->rstc);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to apply reset\n");
> -
> -		goto err_ram_clk;
> +	pm_runtime_set_autosuspend_delay(dev->dev, 1000);
> +	pm_runtime_use_autosuspend(dev->dev);
> +	pm_runtime_enable(dev->dev);
> +	if (!pm_runtime_enabled(dev->dev)) {
> +		ret = cedrus_hw_resume(dev->dev);
> +		if (ret)
> +			goto err_pm;
>  	}
> 
>  	return 0;
> 
> -err_ram_clk:
> -	clk_disable_unprepare(dev->ram_clk);
> -err_mod_clk:
> -	clk_disable_unprepare(dev->mod_clk);
> -err_ahb_clk:
> -	clk_disable_unprepare(dev->ahb_clk);
> +err_pm:
> +	pm_runtime_disable(dev->dev);
>  err_sram:
>  	sunxi_sram_release(dev->dev);
>  err_mem:
> @@ -282,11 +325,9 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
> 
>  void cedrus_hw_remove(struct cedrus_dev *dev)
>  {
> -	reset_control_assert(dev->rstc);
> -
> -	clk_disable_unprepare(dev->ram_clk);
> -	clk_disable_unprepare(dev->mod_clk);
> -	clk_disable_unprepare(dev->ahb_clk);
> +	pm_runtime_disable(dev->dev);
> +	if (!pm_runtime_status_suspended(dev->dev))
> +		cedrus_hw_suspend(dev->dev);
> 
>  	sunxi_sram_release(dev->dev);
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index
> 604ff932fbf5..17822b470a1e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -22,6 +22,9 @@ void cedrus_engine_disable(struct cedrus_dev *dev);
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
>  			   struct v4l2_pix_format *fmt);
> 
> +int cedrus_hw_resume(struct device *dev);
> +int cedrus_hw_suspend(struct device *dev);
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev);
>  void cedrus_hw_remove(struct cedrus_dev *dev);

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Jernej Škrabec]

Hi Samuel!

Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> This allows the VE clocks and PLL_VE to be disabled most of the time.
> 
> Since the device is stateless, each frame gets a separate runtime PM
> reference. Enable autosuspend so the PM callbacks are not run before and
> after every frame.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> I tested this with v4l2-request-test. I don't have the setup to do
> anything more complicated at the moment.
> 
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>  3 files changed, 88 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> 3fad5edccd17..9aa1fc8a6c26 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pm.h>
> 
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -474,6 +475,11 @@ static int cedrus_remove(struct platform_device *pdev)
>  	return 0;
>  }
> 
> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
> +			   cedrus_hw_resume, NULL)
> +};
> +
>  static const struct cedrus_variant sun4i_a10_cedrus_variant = {
>  	.mod_rate	= 320000000,
>  };
> @@ -559,6 +565,7 @@ static struct platform_driver cedrus_driver = {
>  	.driver		= {
>  		.name		= CEDRUS_NAME,
>  		.of_match_table	= of_match_ptr(cedrus_dt_match),
> +		.pm		= &cedrus_dev_pm_ops,
>  	},
>  };
>  module_platform_driver(cedrus_driver);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> daf5f244f93b..b84814d5afe4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -19,6 +19,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/soc/sunxi/sunxi_sram.h>
> @@ -63,6 +64,8 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum
> cedrus_codec codec) if (ctx->src_fmt.width > 2048)
>  		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> 
> +	pm_runtime_get_sync(ctx->dev->dev);
> +
>  	cedrus_write(ctx->dev, VE_MODE, reg);
> 
>  	return 0;
> @@ -71,6 +74,9 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum
> cedrus_codec codec) void cedrus_engine_disable(struct cedrus_dev *dev)
>  {
>  	cedrus_write(dev, VE_MODE, VE_MODE_DISABLED);
> +
> +	pm_runtime_mark_last_busy(dev->dev);
> +	pm_runtime_put_autosuspend(dev->dev);
>  }
> 
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
> @@ -134,12 +140,72 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>  	else
>  		state = VB2_BUF_STATE_DONE;
> 
> +	cedrus_engine_disable(dev);
> +
>  	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx-
>fh.m2m_ctx,
>  					 state);
> 
>  	return IRQ_HANDLED;
>  }
> 
> +int cedrus_hw_resume(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +	int ret;
> +
> +	ret = clk_prepare_enable(dev->ahb_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable AHB clock\n");
> +
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dev->mod_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable MOD clock\n");
> +
> +		goto err_ahb_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dev->ram_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable RAM clock\n");
> +
> +		goto err_mod_clk;
> +	}
> +
> +	ret = reset_control_reset(dev->rstc);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to apply reset\n");
> +
> +		goto err_ram_clk;
> +	}

Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap() a 
function is called, which sets few registers in HW. Of course, there is no 
guarantee that someone will start decoding immediately after capture format is 
set. So, if the driver puts VPU to sleep in the mean time, reset will clear 
those registers and decoded video will be in different format than expected. It 
could be even argued that registers should not be set in that function and 
that this is design issue or bug in driver.

Anyway, I made a runtime PM support long time ago, but never do anything 
besides running few tests:
https://github.com/jernejsk/linux-1/commit/
d245b7fa2a26e519ff675a255c45230575a4a848

It takes a bit different approach. Power is enabled in start streaming and 
disabled in stop streaming. This is simpler approach and doesn't need 
autosuspend functionality. I also moved call to a function which sets format 
in HW registers to start streaming handler, so it's guaranteed to be set at 
the beginning.

Note that some registers are only set in start streaming handler. With your 
approach, if first frame is submitted too late, asserting and de-asserting 
reset line could reset those registers.

Best regards,
Jernej

> +
> +	return 0;
> +
> +err_ram_clk:
> +	clk_disable_unprepare(dev->ram_clk);
> +err_mod_clk:
> +	clk_disable_unprepare(dev->mod_clk);
> +err_ahb_clk:
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return ret;
> +}
> +
> +int cedrus_hw_suspend(struct device *d)
> +{
> +	struct cedrus_dev *dev = dev_get_drvdata(d);
> +
> +	reset_control_assert(dev->rstc);
> +
> +	clk_disable_unprepare(dev->ram_clk);
> +	clk_disable_unprepare(dev->mod_clk);
> +	clk_disable_unprepare(dev->ahb_clk);
> +
> +	return 0;
> +}
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev)
>  {
>  	const struct cedrus_variant *variant;
> @@ -236,42 +302,19 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  		goto err_sram;
>  	}
> 
> -	ret = clk_prepare_enable(dev->ahb_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable AHB clock\n");
> -
> -		goto err_sram;
> -	}
> -
> -	ret = clk_prepare_enable(dev->mod_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable MOD clock\n");
> -
> -		goto err_ahb_clk;
> -	}
> -
> -	ret = clk_prepare_enable(dev->ram_clk);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to enable RAM clock\n");
> -
> -		goto err_mod_clk;
> -	}
> -
> -	ret = reset_control_reset(dev->rstc);
> -	if (ret) {
> -		dev_err(dev->dev, "Failed to apply reset\n");
> -
> -		goto err_ram_clk;
> +	pm_runtime_set_autosuspend_delay(dev->dev, 1000);
> +	pm_runtime_use_autosuspend(dev->dev);
> +	pm_runtime_enable(dev->dev);
> +	if (!pm_runtime_enabled(dev->dev)) {
> +		ret = cedrus_hw_resume(dev->dev);
> +		if (ret)
> +			goto err_pm;
>  	}
> 
>  	return 0;
> 
> -err_ram_clk:
> -	clk_disable_unprepare(dev->ram_clk);
> -err_mod_clk:
> -	clk_disable_unprepare(dev->mod_clk);
> -err_ahb_clk:
> -	clk_disable_unprepare(dev->ahb_clk);
> +err_pm:
> +	pm_runtime_disable(dev->dev);
>  err_sram:
>  	sunxi_sram_release(dev->dev);
>  err_mem:
> @@ -282,11 +325,9 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
> 
>  void cedrus_hw_remove(struct cedrus_dev *dev)
>  {
> -	reset_control_assert(dev->rstc);
> -
> -	clk_disable_unprepare(dev->ram_clk);
> -	clk_disable_unprepare(dev->mod_clk);
> -	clk_disable_unprepare(dev->ahb_clk);
> +	pm_runtime_disable(dev->dev);
> +	if (!pm_runtime_status_suspended(dev->dev))
> +		cedrus_hw_suspend(dev->dev);
> 
>  	sunxi_sram_release(dev->dev);
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index
> 604ff932fbf5..17822b470a1e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -22,6 +22,9 @@ void cedrus_engine_disable(struct cedrus_dev *dev);
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
>  			   struct v4l2_pix_format *fmt);
> 
> +int cedrus_hw_resume(struct device *dev);
> +int cedrus_hw_suspend(struct device *dev);
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev);
>  void cedrus_hw_remove(struct cedrus_dev *dev);

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Samuel Holland]

On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> Hi Samuel!
> 
> Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
>> This allows the VE clocks and PLL_VE to be disabled most of the time.
>>
>> Since the device is stateless, each frame gets a separate runtime PM
>> reference. Enable autosuspend so the PM callbacks are not run before and
>> after every frame.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> I tested this with v4l2-request-test. I don't have the setup to do
>> anything more complicated at the moment.
>>
>> ---
>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
>>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>>  3 files changed, 88 insertions(+), 37 deletions(-)

[snip]

> Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap() a 
> function is called, which sets few registers in HW. Of course, there is no 
> guarantee that someone will start decoding immediately after capture format is 
> set. So, if the driver puts VPU to sleep in the mean time, reset will clear 
> those registers and decoded video will be in different format than expected. It 
> could be even argued that registers should not be set in that function and 
> that this is design issue or bug in driver.

You're right. I didn't see that cedrus_dst_format_set() was called outside
cedrus_engine_enable/disable().

> Anyway, I made a runtime PM support long time ago, but never do anything 
> besides running few tests:
> https://github.com/jernejsk/linux-1/commit/
> d245b7fa2a26e519ff675a255c45230575a4a848
> 
> It takes a bit different approach. Power is enabled in start streaming and 
> disabled in stop streaming. This is simpler approach and doesn't need 
> autosuspend functionality. I also moved call to a function which sets format 
> in HW registers to start streaming handler, so it's guaranteed to be set at 
> the beginning.

Assuming cedrus_device_run() only gets called between streamon and streamoff
(which appears to be the case), this looks like a better design.

> Note that some registers are only set in start streaming handler. With your 
> approach, if first frame is submitted too late, asserting and de-asserting 
> reset line could reset those registers.

I only see registers being set in cedrus_start_streaming() after your patch,
where you add a call to cedrus_dst_format_set(). I can't find any registers
being written in any of the ->start() callbacks.

I'll send a v2 which combines the two patches: yours which handles the runtime
part better, and mine which handles the probe/remove part better with !CONFIG_PM.

> Best regards,
> Jernej

Thanks for the review!
Samuel

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3446 bytes --]

Hi,

On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > Hi Samuel!
> > 
> > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> >>
> >> Since the device is stateless, each frame gets a separate runtime PM
> >> reference. Enable autosuspend so the PM callbacks are not run before and
> >> after every frame.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >> I tested this with v4l2-request-test. I don't have the setup to do
> >> anything more complicated at the moment.
> >>
> >> ---
> >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> >>  3 files changed, 88 insertions(+), 37 deletions(-)
> 
> [snip]
> 
> > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap() a 
> > function is called, which sets few registers in HW. Of course, there is no 
> > guarantee that someone will start decoding immediately after capture format is 
> > set. So, if the driver puts VPU to sleep in the mean time, reset will clear 
> > those registers and decoded video will be in different format than expected. It 
> > could be even argued that registers should not be set in that function and 
> > that this is design issue or bug in driver.
> 
> You're right. I didn't see that cedrus_dst_format_set() was called outside
> cedrus_engine_enable/disable().

This might indeed be an issue with multiple decoding contexts in parallel, with
potentially different formats. For that reason, it looks like the right thing to
do would be to set the format at each decoding run based on the format set in
the context by s_fmt.

> > Anyway, I made a runtime PM support long time ago, but never do anything 
> > besides running few tests:
> > https://github.com/jernejsk/linux-1/commit/
> > d245b7fa2a26e519ff675a255c45230575a4a848
> > 
> > It takes a bit different approach. Power is enabled in start streaming and 
> > disabled in stop streaming. This is simpler approach and doesn't need 
> > autosuspend functionality. I also moved call to a function which sets format 
> > in HW registers to start streaming handler, so it's guaranteed to be set at 
> > the beginning.
> 
> Assuming cedrus_device_run() only gets called between streamon and streamoff
> (which appears to be the case), this looks like a better design.

Yes, this is defintiely ensured by the V4L2 framework. I agree that streamon/off
are the correct sync points.

> > Note that some registers are only set in start streaming handler. With your 
> > approach, if first frame is submitted too late, asserting and de-asserting 
> > reset line could reset those registers.
> 
> I only see registers being set in cedrus_start_streaming() after your patch,
> where you add a call to cedrus_dst_format_set(). I can't find any registers
> being written in any of the ->start() callbacks.
> 
> I'll send a v2 which combines the two patches: yours which handles the runtime
> part better, and mine which handles the probe/remove part better with !CONFIG_PM.

Thanks, that should do it!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Jernej Škrabec]

Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski 
napisal(a):
> Hi,
> 
> On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > Hi Samuel!
> > > 
> > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > >> 
> > >> Since the device is stateless, each frame gets a separate runtime PM
> > >> reference. Enable autosuspend so the PM callbacks are not run before
> > >> and
> > >> after every frame.
> > >> 
> > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > >> ---
> > >> 
> > >> I tested this with v4l2-request-test. I don't have the setup to do
> > >> anything more complicated at the moment.
> > >> 
> > >> ---
> > >> 
> > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > 
> > [snip]
> > 
> > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > a
> > > function is called, which sets few registers in HW. Of course, there is
> > > no
> > > guarantee that someone will start decoding immediately after capture
> > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > reset will clear those registers and decoded video will be in different
> > > format than expected. It could be even argued that registers should not
> > > be set in that function and that this is design issue or bug in driver.
> > 
> > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > cedrus_engine_enable/disable().
> 
> This might indeed be an issue with multiple decoding contexts in parallel,
> with potentially different formats. For that reason, it looks like the
> right thing to do would be to set the format at each decoding run based on
> the format set in the context by s_fmt.

So you are suggesting that cedrus_dst_format_set() should be moved to 
cedrus_device_run(), right? This way all registers are set at each run, which 
is then truly stateless.

Best regards,
Jernej

> 
> > > Anyway, I made a runtime PM support long time ago, but never do anything
> > > besides running few tests:
> > > https://github.com/jernejsk/linux-1/commit/
> > > d245b7fa2a26e519ff675a255c45230575a4a848
> > > 
> > > It takes a bit different approach. Power is enabled in start streaming
> > > and
> > > disabled in stop streaming. This is simpler approach and doesn't need
> > > autosuspend functionality. I also moved call to a function which sets
> > > format in HW registers to start streaming handler, so it's guaranteed
> > > to be set at the beginning.
> > 
> > Assuming cedrus_device_run() only gets called between streamon and
> > streamoff (which appears to be the case), this looks like a better
> > design.
> 
> Yes, this is defintiely ensured by the V4L2 framework. I agree that
> streamon/off are the correct sync points.
> 
> > > Note that some registers are only set in start streaming handler. With
> > > your
> > > approach, if first frame is submitted too late, asserting and
> > > de-asserting
> > > reset line could reset those registers.
> > 
> > I only see registers being set in cedrus_start_streaming() after your
> > patch, where you add a call to cedrus_dst_format_set(). I can't find any
> > registers being written in any of the ->start() callbacks.
> > 
> > I'll send a v2 which combines the two patches: yours which handles the
> > runtime part better, and mine which handles the probe/remove part better
> > with !CONFIG_PM.
> Thanks, that should do it!
> 
> Cheers,
> 
> Paul

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 4295 bytes --]

Hi Jernej,

On Mon 20 Apr 20, 18:41, Jernej Škrabec wrote:
> Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski 
> napisal(a):
> > Hi,
> > 
> > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > Hi Samuel!
> > > > 
> > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > > >> 
> > > >> Since the device is stateless, each frame gets a separate runtime PM
> > > >> reference. Enable autosuspend so the PM callbacks are not run before
> > > >> and
> > > >> after every frame.
> > > >> 
> > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > >> ---
> > > >> 
> > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > >> anything more complicated at the moment.
> > > >> 
> > > >> ---
> > > >> 
> > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > > 
> > > [snip]
> > > 
> > > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > > a
> > > > function is called, which sets few registers in HW. Of course, there is
> > > > no
> > > > guarantee that someone will start decoding immediately after capture
> > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > reset will clear those registers and decoded video will be in different
> > > > format than expected. It could be even argued that registers should not
> > > > be set in that function and that this is design issue or bug in driver.
> > > 
> > > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > > cedrus_engine_enable/disable().
> > 
> > This might indeed be an issue with multiple decoding contexts in parallel,
> > with potentially different formats. For that reason, it looks like the
> > right thing to do would be to set the format at each decoding run based on
> > the format set in the context by s_fmt.
> 
> So you are suggesting that cedrus_dst_format_set() should be moved to 
> cedrus_device_run(), right? This way all registers are set at each run, which 
> is then truly stateless.

Yes, exactly! But this is outside of the scope of this series though.

Cheers,

Paul

> Best regards,
> Jernej
> 
> > 
> > > > Anyway, I made a runtime PM support long time ago, but never do anything
> > > > besides running few tests:
> > > > https://github.com/jernejsk/linux-1/commit/
> > > > d245b7fa2a26e519ff675a255c45230575a4a848
> > > > 
> > > > It takes a bit different approach. Power is enabled in start streaming
> > > > and
> > > > disabled in stop streaming. This is simpler approach and doesn't need
> > > > autosuspend functionality. I also moved call to a function which sets
> > > > format in HW registers to start streaming handler, so it's guaranteed
> > > > to be set at the beginning.
> > > 
> > > Assuming cedrus_device_run() only gets called between streamon and
> > > streamoff (which appears to be the case), this looks like a better
> > > design.
> > 
> > Yes, this is defintiely ensured by the V4L2 framework. I agree that
> > streamon/off are the correct sync points.
> > 
> > > > Note that some registers are only set in start streaming handler. With
> > > > your
> > > > approach, if first frame is submitted too late, asserting and
> > > > de-asserting
> > > > reset line could reset those registers.
> > > 
> > > I only see registers being set in cedrus_start_streaming() after your
> > > patch, where you add a call to cedrus_dst_format_set(). I can't find any
> > > registers being written in any of the ->start() callbacks.
> > > 
> > > I'll send a v2 which combines the two patches: yours which handles the
> > > runtime part better, and mine which handles the probe/remove part better
> > > with !CONFIG_PM.
> > Thanks, that should do it!
> > 
> > Cheers,
> > 
> > Paul
> 
> 
> 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Jernej Škrabec]

Hi!

Dne ponedeljek, 20. april 2020 ob 19:56:51 CEST je Paul Kocialkowski 
napisal(a):
> Hi Jernej,
> 
> On Mon 20 Apr 20, 18:41, Jernej Škrabec wrote:
> > Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> > 
> > napisal(a):
> > > Hi,
> > > 
> > > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > > Hi Samuel!
> > > > > 
> > > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland 
napisal(a):
> > > > >> This allows the VE clocks and PLL_VE to be disabled most of the
> > > > >> time.
> > > > >> 
> > > > >> Since the device is stateless, each frame gets a separate runtime
> > > > >> PM
> > > > >> reference. Enable autosuspend so the PM callbacks are not run
> > > > >> before
> > > > >> and
> > > > >> after every frame.
> > > > >> 
> > > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > > >> ---
> > > > >> 
> > > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > > >> anything more complicated at the moment.
> > > > >> 
> > > > >> ---
> > > > >> 
> > > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115
> > > > >>  ++++++++++++------
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > > > 
> > > > [snip]
> > > > 
> > > > > Reset above causes problem. When format is set in
> > > > > cedrus_s_fmt_vid_cap()
> > > > > a
> > > > > function is called, which sets few registers in HW. Of course, there
> > > > > is
> > > > > no
> > > > > guarantee that someone will start decoding immediately after capture
> > > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > > reset will clear those registers and decoded video will be in
> > > > > different
> > > > > format than expected. It could be even argued that registers should
> > > > > not
> > > > > be set in that function and that this is design issue or bug in
> > > > > driver.
> > > > 
> > > > You're right. I didn't see that cedrus_dst_format_set() was called
> > > > outside
> > > > cedrus_engine_enable/disable().
> > > 
> > > This might indeed be an issue with multiple decoding contexts in
> > > parallel,
> > > with potentially different formats. For that reason, it looks like the
> > > right thing to do would be to set the format at each decoding run based
> > > on
> > > the format set in the context by s_fmt.
> > 
> > So you are suggesting that cedrus_dst_format_set() should be moved to
> > cedrus_device_run(), right? This way all registers are set at each run,
> > which is then truly stateless.
> 
> Yes, exactly! But this is outside of the scope of this series though.

I'm not sure about being out of scope. It has to be moved anyway, so why not 
put it in best place?

Best

> 
> Cheers,
> 
> Paul
> 
> > Best regards,
> > Jernej
> > 
> > > > > Anyway, I made a runtime PM support long time ago, but never do
> > > > > anything
> > > > > besides running few tests:
> > > > > https://github.com/jernejsk/linux-1/commit/
> > > > > d245b7fa2a26e519ff675a255c45230575a4a848
> > > > > 
> > > > > It takes a bit different approach. Power is enabled in start
> > > > > streaming
> > > > > and
> > > > > disabled in stop streaming. This is simpler approach and doesn't
> > > > > need
> > > > > autosuspend functionality. I also moved call to a function which
> > > > > sets
> > > > > format in HW registers to start streaming handler, so it's
> > > > > guaranteed
> > > > > to be set at the beginning.
> > > > 
> > > > Assuming cedrus_device_run() only gets called between streamon and
> > > > streamoff (which appears to be the case), this looks like a better
> > > > design.
> > > 
> > > Yes, this is defintiely ensured by the V4L2 framework. I agree that
> > > streamon/off are the correct sync points.
> > > 
> > > > > Note that some registers are only set in start streaming handler.
> > > > > With
> > > > > your
> > > > > approach, if first frame is submitted too late, asserting and
> > > > > de-asserting
> > > > > reset line could reset those registers.
> > > > 
> > > > I only see registers being set in cedrus_start_streaming() after your
> > > > patch, where you add a call to cedrus_dst_format_set(). I can't find
> > > > any
> > > > registers being written in any of the ->start() callbacks.
> > > > 
> > > > I'll send a v2 which combines the two patches: yours which handles the
> > > > runtime part better, and mine which handles the probe/remove part
> > > > better
> > > > with !CONFIG_PM.
> > > 
> > > Thanks, that should do it!
> > > 
> > > Cheers,
> > > 
> > > Paul

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Ezequiel Garcia]

Hi Jernej, Paul:

On Mon, 20 Apr 2020 at 13:41, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> napisal(a):
> > Hi,
> >
> > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > Hi Samuel!
> > > >
> > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > > >>
> > > >> Since the device is stateless, each frame gets a separate runtime PM
> > > >> reference. Enable autosuspend so the PM callbacks are not run before
> > > >> and
> > > >> after every frame.
> > > >>
> > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > >> ---
> > > >>
> > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > >> anything more complicated at the moment.
> > > >>
> > > >> ---
> > > >>
> > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > >
> > > [snip]
> > >
> > > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > > a
> > > > function is called, which sets few registers in HW. Of course, there is
> > > > no
> > > > guarantee that someone will start decoding immediately after capture
> > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > reset will clear those registers and decoded video will be in different
> > > > format than expected. It could be even argued that registers should not
> > > > be set in that function and that this is design issue or bug in driver.
> > >
> > > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > > cedrus_engine_enable/disable().
> >
> > This might indeed be an issue with multiple decoding contexts in parallel,
> > with potentially different formats. For that reason, it looks like the
> > right thing to do would be to set the format at each decoding run based on
> > the format set in the context by s_fmt.
>
> So you are suggesting that cedrus_dst_format_set() should be moved to
> cedrus_device_run(), right? This way all registers are set at each run, which
> is then truly stateless.
>

BTW, this is how the Hantro and Rockchip's Rkvdec
drivers are implemented. One of the main reasons is
to have simultaneous decoding support.

Thanks,
Ezequiel

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 5744 bytes --]

Hi,

On Mon 20 Apr 20, 20:09, Jernej Škrabec wrote:
> Hi!
> 
> Dne ponedeljek, 20. april 2020 ob 19:56:51 CEST je Paul Kocialkowski 
> napisal(a):
> > Hi Jernej,
> > 
> > On Mon 20 Apr 20, 18:41, Jernej Škrabec wrote:
> > > Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> > > 
> > > napisal(a):
> > > > Hi,
> > > > 
> > > > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > > > Hi Samuel!
> > > > > > 
> > > > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland 
> napisal(a):
> > > > > >> This allows the VE clocks and PLL_VE to be disabled most of the
> > > > > >> time.
> > > > > >> 
> > > > > >> Since the device is stateless, each frame gets a separate runtime
> > > > > >> PM
> > > > > >> reference. Enable autosuspend so the PM callbacks are not run
> > > > > >> before
> > > > > >> and
> > > > > >> after every frame.
> > > > > >> 
> > > > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > > > >> ---
> > > > > >> 
> > > > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > > > >> anything more complicated at the moment.
> > > > > >> 
> > > > > >> ---
> > > > > >> 
> > > > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115
> > > > > >>  ++++++++++++------
> > > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > Reset above causes problem. When format is set in
> > > > > > cedrus_s_fmt_vid_cap()
> > > > > > a
> > > > > > function is called, which sets few registers in HW. Of course, there
> > > > > > is
> > > > > > no
> > > > > > guarantee that someone will start decoding immediately after capture
> > > > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > > > reset will clear those registers and decoded video will be in
> > > > > > different
> > > > > > format than expected. It could be even argued that registers should
> > > > > > not
> > > > > > be set in that function and that this is design issue or bug in
> > > > > > driver.
> > > > > 
> > > > > You're right. I didn't see that cedrus_dst_format_set() was called
> > > > > outside
> > > > > cedrus_engine_enable/disable().
> > > > 
> > > > This might indeed be an issue with multiple decoding contexts in
> > > > parallel,
> > > > with potentially different formats. For that reason, it looks like the
> > > > right thing to do would be to set the format at each decoding run based
> > > > on
> > > > the format set in the context by s_fmt.
> > > 
> > > So you are suggesting that cedrus_dst_format_set() should be moved to
> > > cedrus_device_run(), right? This way all registers are set at each run,
> > > which is then truly stateless.
> > 
> > Yes, exactly! But this is outside of the scope of this series though.
> 
> I'm not sure about being out of scope. It has to be moved anyway, so why not 
> put it in best place?

I agree it should be moved to cedrus_device_run, for sure!

What I meant is that Samuel doesn't have to make that change with the runtime PM
patch series because it's a different issue. But reading the discussion again,
it's true that there is a chance of the VPU going to sleep between set fmt and
start streaming, so we should probably take care of it before merging runtime
PM.

Samuel, do you feel like making that change with your series?
I can do it otherwise.

Cheers,

Paul

> Best
> 
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > > > > Anyway, I made a runtime PM support long time ago, but never do
> > > > > > anything
> > > > > > besides running few tests:
> > > > > > https://github.com/jernejsk/linux-1/commit/
> > > > > > d245b7fa2a26e519ff675a255c45230575a4a848
> > > > > > 
> > > > > > It takes a bit different approach. Power is enabled in start
> > > > > > streaming
> > > > > > and
> > > > > > disabled in stop streaming. This is simpler approach and doesn't
> > > > > > need
> > > > > > autosuspend functionality. I also moved call to a function which
> > > > > > sets
> > > > > > format in HW registers to start streaming handler, so it's
> > > > > > guaranteed
> > > > > > to be set at the beginning.
> > > > > 
> > > > > Assuming cedrus_device_run() only gets called between streamon and
> > > > > streamoff (which appears to be the case), this looks like a better
> > > > > design.
> > > > 
> > > > Yes, this is defintiely ensured by the V4L2 framework. I agree that
> > > > streamon/off are the correct sync points.
> > > > 
> > > > > > Note that some registers are only set in start streaming handler.
> > > > > > With
> > > > > > your
> > > > > > approach, if first frame is submitted too late, asserting and
> > > > > > de-asserting
> > > > > > reset line could reset those registers.
> > > > > 
> > > > > I only see registers being set in cedrus_start_streaming() after your
> > > > > patch, where you add a call to cedrus_dst_format_set(). I can't find
> > > > > any
> > > > > registers being written in any of the ->start() callbacks.
> > > > > 
> > > > > I'll send a v2 which combines the two patches: yours which handles the
> > > > > runtime part better, and mine which handles the probe/remove part
> > > > > better
> > > > > with !CONFIG_PM.
> > > > 
> > > > Thanks, that should do it!
> > > > 
> > > > Cheers,
> > > > 
> > > > Paul
> 
> 
> 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

Hi Ezequiel,

On Tue 21 Apr 20, 00:51, Ezequiel Garcia wrote:
> Hi Jernej, Paul:
> 
> On Mon, 20 Apr 2020 at 13:41, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> >
> > Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> > napisal(a):
> > > Hi,
> > >
> > > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > > Hi Samuel!
> > > > >
> > > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > > > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > > > >>
> > > > >> Since the device is stateless, each frame gets a separate runtime PM
> > > > >> reference. Enable autosuspend so the PM callbacks are not run before
> > > > >> and
> > > > >> after every frame.
> > > > >>
> > > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > > >> ---
> > > > >>
> > > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > > >> anything more complicated at the moment.
> > > > >>
> > > > >> ---
> > > > >>
> > > > >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 115 ++++++++++++------
> > > > >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> > > > >>  3 files changed, 88 insertions(+), 37 deletions(-)
> > > >
> > > > [snip]
> > > >
> > > > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > > > a
> > > > > function is called, which sets few registers in HW. Of course, there is
> > > > > no
> > > > > guarantee that someone will start decoding immediately after capture
> > > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > > reset will clear those registers and decoded video will be in different
> > > > > format than expected. It could be even argued that registers should not
> > > > > be set in that function and that this is design issue or bug in driver.
> > > >
> > > > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > > > cedrus_engine_enable/disable().
> > >
> > > This might indeed be an issue with multiple decoding contexts in parallel,
> > > with potentially different formats. For that reason, it looks like the
> > > right thing to do would be to set the format at each decoding run based on
> > > the format set in the context by s_fmt.
> >
> > So you are suggesting that cedrus_dst_format_set() should be moved to
> > cedrus_device_run(), right? This way all registers are set at each run, which
> > is then truly stateless.
> >
> 
> BTW, this is how the Hantro and Rockchip's Rkvdec
> drivers are implemented. One of the main reasons is
> to have simultaneous decoding support.

That's a wise decision :)

I had actually tested that cedrus can decode multiple streams in parallel, but
I must have used the same format settings for both and didn't notice the issue
then.

Good that we are fixing it now!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]