[PATCH v2 1/2] media: cedrus: Program output format during each run

[PATCH v2 1/2] media: cedrus: Program output format during each run

[Samuel Holland]

Previously, the output format was programmed as part of the ioctl()
handler. However, this has two problems:

  1) If there are multiple active streams with different output
     formats, the hardware will use whichever format was set last
     for both streams. Similary, an ioctl() done in an inactive
     context will wrongly affect other active contexts.
  2) The registers are written while the device is not actively
     streaming. To enable runtime PM tied to the streaming state,
     all hardware access needs to be moved inside cedrus_device_run().

The call to cedrus_dst_format_set() is now placed just before the
codec-specific callback that programs the hardware.

Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com>
Suggested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

v2: added patch

---
 drivers/staging/media/sunxi/cedrus/cedrus_dec.c   | 2 ++
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index 4a2fc33a1d79..58c48e4fdfe9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -74,6 +74,8 @@ void cedrus_device_run(void *priv)
 
 	v4l2_m2m_buf_copy_metadata(run.src, run.dst, true);
 
+	cedrus_dst_format_set(dev, &ctx->dst_fmt);
+
 	dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
 
 	/* Complete request(s) controls if needed. */
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 15cf1f10221b..ed3f511f066f 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -273,7 +273,6 @@ static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
 				struct v4l2_format *f)
 {
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
-	struct cedrus_dev *dev = ctx->dev;
 	struct vb2_queue *vq;
 	int ret;
 
@@ -287,8 +286,6 @@ static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
 
 	ctx->dst_fmt = f->fmt.pix;
 
-	cedrus_dst_format_set(dev, &ctx->dst_fmt);
-
 	return 0;
 }
 
-- 
2.24.1

[PATCH v2 2/2] media: cedrus: Implement runtime PM

[Samuel Holland]

This allows the VE clocks and PLL_VE to be disabled most of the time.
A runtime PM reference is held while streaming.

Originally-by: Jernej Škrabec <jernej.skrabec@gmail.com>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

v2: moved PM reference to cedrus_{start,stop}_streaming, based on an
    earlier patch by Jernej Skrabec. Removes the need for autosuspend.
    I tested this with running 2x v4l2-request-test in parallel.

---
 drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
 .../staging/media/sunxi/cedrus/cedrus_hw.c    | 106 ++++++++++++------
 .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
 .../staging/media/sunxi/cedrus/cedrus_video.c |  33 ++++--
 4 files changed, 104 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 05a85517ff60..bc27f9430eeb 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>
@@ -551,12 +552,18 @@ static const struct of_device_id cedrus_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, cedrus_dt_match);
 
+static const struct dev_pm_ops cedrus_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
+			   cedrus_hw_resume, NULL)
+};
+
 static struct platform_driver cedrus_driver = {
 	.probe		= cedrus_probe,
 	.remove		= cedrus_remove,
 	.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..1744e6fcc999 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>
@@ -140,6 +141,64 @@ static irqreturn_t cedrus_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+int cedrus_hw_suspend(struct device *device)
+{
+	struct cedrus_dev *dev = dev_get_drvdata(device);
+
+	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_resume(struct device *device)
+{
+	struct cedrus_dev *dev = dev_get_drvdata(device);
+	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_probe(struct cedrus_dev *dev)
 {
 	const struct cedrus_variant *variant;
@@ -236,42 +295,17 @@ 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_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 +316,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..45f641f0bfa2 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_suspend(struct device *device);
+int cedrus_hw_resume(struct device *device);
+
 int cedrus_hw_probe(struct cedrus_dev *dev);
 void cedrus_hw_remove(struct cedrus_dev *dev);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index ed3f511f066f..16d82309e7b6 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -13,6 +13,8 @@
  * Marek Szyprowski, <m.szyprowski@samsung.com>
  */
 
+#include <linux/pm_runtime.h>
+
 #include <media/videobuf2-dma-contig.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -450,12 +452,24 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count)
 		return -EINVAL;
 	}
 
-	if (V4L2_TYPE_IS_OUTPUT(vq->type) &&
-	    dev->dec_ops[ctx->current_codec]->start)
-		ret = dev->dec_ops[ctx->current_codec]->start(ctx);
+	if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
+		ret = pm_runtime_get_sync(dev->dev);
+		if (ret < 0)
+			goto err_cleanup;
 
-	if (ret)
-		cedrus_queue_cleanup(vq, VB2_BUF_STATE_QUEUED);
+		if (dev->dec_ops[ctx->current_codec]->start) {
+			ret = dev->dec_ops[ctx->current_codec]->start(ctx);
+			if (ret)
+				goto err_pm;
+		}
+	}
+
+	return 0;
+
+err_pm:
+	pm_runtime_put(dev->dev);
+err_cleanup:
+	cedrus_queue_cleanup(vq, VB2_BUF_STATE_QUEUED);
 
 	return ret;
 }
@@ -465,9 +479,12 @@ static void cedrus_stop_streaming(struct vb2_queue *vq)
 	struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
 	struct cedrus_dev *dev = ctx->dev;
 
-	if (V4L2_TYPE_IS_OUTPUT(vq->type) &&
-	    dev->dec_ops[ctx->current_codec]->stop)
-		dev->dec_ops[ctx->current_codec]->stop(ctx);
+	if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
+		if (dev->dec_ops[ctx->current_codec]->stop)
+			dev->dec_ops[ctx->current_codec]->stop(ctx);
+
+		pm_runtime_put(dev->dev);
+	}
 
 	cedrus_queue_cleanup(vq, VB2_BUF_STATE_ERROR);
 }
-- 
2.24.1

Re: [linux-sunxi] [PATCH v2 1/2] media: cedrus: Program output format during each run

[Jernej Škrabec]

Dne sreda, 22. april 2020 ob 06:04:09 CEST je Samuel Holland napisal(a):
> Previously, the output format was programmed as part of the ioctl()
> handler. However, this has two problems:
> 
>   1) If there are multiple active streams with different output
>      formats, the hardware will use whichever format was set last
>      for both streams. Similary, an ioctl() done in an inactive
>      context will wrongly affect other active contexts.
>   2) The registers are written while the device is not actively
>      streaming. To enable runtime PM tied to the streaming state,
>      all hardware access needs to be moved inside cedrus_device_run().
> 
> The call to cedrus_dst_format_set() is now placed just before the
> codec-specific callback that programs the hardware.
> 
> Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
> Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> Suggested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

For what is worth:
Tested-by: Jernej Skrabec <jernej.skrabec@siol.net>
Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

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

[Jernej Škrabec]

Dne sreda, 22. april 2020 ob 06:04:10 CEST je Samuel Holland napisal(a):
> This allows the VE clocks and PLL_VE to be disabled most of the time.
> A runtime PM reference is held while streaming.
> 
> Originally-by: Jernej Škrabec <jernej.skrabec@gmail.com>

I guess this could be SOB? Either way I'm fine.

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> v2: moved PM reference to cedrus_{start,stop}_streaming, based on an
>     earlier patch by Jernej Skrabec. Removes the need for autosuspend.
>     I tested this with running 2x v4l2-request-test in parallel.
> 

I tested previous and this patch with LibreELEC and I didn't noticed any 
regressions:

Tested-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

Re: [linux-sunxi] [PATCH v2 1/2] media: cedrus: Program output format during each run

[Ezequiel Garcia]

On Wed, 22 Apr 2020 at 01:00, Samuel Holland <samuel@sholland.org> wrote:
>
> Previously, the output format was programmed as part of the ioctl()
> handler. However, this has two problems:
>
>   1) If there are multiple active streams with different output
>      formats, the hardware will use whichever format was set last
>      for both streams. Similary, an ioctl() done in an inactive
>      context will wrongly affect other active contexts.
>   2) The registers are written while the device is not actively
>      streaming. To enable runtime PM tied to the streaming state,
>      all hardware access needs to be moved inside cedrus_device_run().
>
> The call to cedrus_dst_format_set() is now placed just before the
> codec-specific callback that programs the hardware.
>

Cc: <stable@vger.kernel.org>

> Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
> Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> Suggested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

> ---
>
> v2: added patch
>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_dec.c   | 2 ++
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index 4a2fc33a1d79..58c48e4fdfe9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -74,6 +74,8 @@ void cedrus_device_run(void *priv)
>
>         v4l2_m2m_buf_copy_metadata(run.src, run.dst, true);
>
> +       cedrus_dst_format_set(dev, &ctx->dst_fmt);
> +
>         dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
>
>         /* Complete request(s) controls if needed. */
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 15cf1f10221b..ed3f511f066f 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -273,7 +273,6 @@ static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
>                                 struct v4l2_format *f)
>  {
>         struct cedrus_ctx *ctx = cedrus_file2ctx(file);
> -       struct cedrus_dev *dev = ctx->dev;
>         struct vb2_queue *vq;
>         int ret;
>
> @@ -287,8 +286,6 @@ static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
>
>         ctx->dst_fmt = f->fmt.pix;
>
> -       cedrus_dst_format_set(dev, &ctx->dst_fmt);
> -
>         return 0;
>  }
>
> --
> 2.24.1
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20200422040410.6251-1-samuel%40sholland.org.

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

[Ezequiel Garcia]

On Wed, 22 Apr 2020 at 01:00, Samuel Holland <samuel@sholland.org> wrote:
>
> This allows the VE clocks and PLL_VE to be disabled most of the time.
> A runtime PM reference is held while streaming.
>
> Originally-by: Jernej Škrabec <jernej.skrabec@gmail.com>

Originally-by is not documented, perhaps just go with Signed-off-by,
as Jernej suggested.

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
> v2: moved PM reference to cedrus_{start,stop}_streaming, based on an
>     earlier patch by Jernej Skrabec. Removes the need for autosuspend.
>     I tested this with running 2x v4l2-request-test in parallel.
>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 106 ++++++++++++------
>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>  .../staging/media/sunxi/cedrus/cedrus_video.c |  33 ++++--
>  4 files changed, 104 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 05a85517ff60..bc27f9430eeb 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>
> @@ -551,12 +552,18 @@ static const struct of_device_id cedrus_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, cedrus_dt_match);
>
> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
> +                          cedrus_hw_resume, NULL)
> +};
> +
>  static struct platform_driver cedrus_driver = {
>         .probe          = cedrus_probe,
>         .remove         = cedrus_remove,
>         .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..1744e6fcc999 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>
> @@ -140,6 +141,64 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>         return IRQ_HANDLED;
>  }
>
> +int cedrus_hw_suspend(struct device *device)
> +{
> +       struct cedrus_dev *dev = dev_get_drvdata(device);
> +
> +       reset_control_assert(dev->rstc);
> +
> +       clk_disable_unprepare(dev->ram_clk);
> +       clk_disable_unprepare(dev->mod_clk);
> +       clk_disable_unprepare(dev->ahb_clk);
> +

You can use clk_bulk API here.

> +       return 0;
> +}
> +
> +int cedrus_hw_resume(struct device *device)
> +{
> +       struct cedrus_dev *dev = dev_get_drvdata(device);
> +       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_probe(struct cedrus_dev *dev)
>  {
>         const struct cedrus_variant *variant;
> @@ -236,42 +295,17 @@ 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_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 +316,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..45f641f0bfa2 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_suspend(struct device *device);
> +int cedrus_hw_resume(struct device *device);
> +
>  int cedrus_hw_probe(struct cedrus_dev *dev);
>  void cedrus_hw_remove(struct cedrus_dev *dev);
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index ed3f511f066f..16d82309e7b6 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -13,6 +13,8 @@
>   * Marek Szyprowski, <m.szyprowski@samsung.com>
>   */
>
> +#include <linux/pm_runtime.h>
> +
>  #include <media/videobuf2-dma-contig.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -450,12 +452,24 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count)
>                 return -EINVAL;
>         }
>
> -       if (V4L2_TYPE_IS_OUTPUT(vq->type) &&
> -           dev->dec_ops[ctx->current_codec]->start)
> -               ret = dev->dec_ops[ctx->current_codec]->start(ctx);
> +       if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
> +               ret = pm_runtime_get_sync(dev->dev);

It's entirely up to you, but you could do get_sync/put
between .device_run, as everything should happen
in the context of v4l2_m2m_ops.device_run.
(In that case, I believe you'd want to enable autosuspend.)

Not sure there's much to gain from an power consumption
pov, though.

With the Originally-by changed:

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

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

[Samuel Holland]

On 5/5/20 7:53 AM, Ezequiel Garcia wrote:
> On Wed, 22 Apr 2020 at 01:00, Samuel Holland <samuel@sholland.org> wrote:
>>
>> This allows the VE clocks and PLL_VE to be disabled most of the time.
>> A runtime PM reference is held while streaming.
>>
>> Originally-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> 
> Originally-by is not documented, perhaps just go with Signed-off-by,
> as Jernej suggested.
> 
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> v2: moved PM reference to cedrus_{start,stop}_streaming, based on an
>>     earlier patch by Jernej Skrabec. Removes the need for autosuspend.
>>     I tested this with running 2x v4l2-request-test in parallel.
>>
>> ---
>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
>>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 106 ++++++++++++------
>>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
>>  .../staging/media/sunxi/cedrus/cedrus_video.c |  33 ++++--
>>  4 files changed, 104 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
>> index 05a85517ff60..bc27f9430eeb 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>
>> @@ -551,12 +552,18 @@ static const struct of_device_id cedrus_dt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, cedrus_dt_match);
>>
>> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
>> +                          cedrus_hw_resume, NULL)
>> +};
>> +
>>  static struct platform_driver cedrus_driver = {
>>         .probe          = cedrus_probe,
>>         .remove         = cedrus_remove,
>>         .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..1744e6fcc999 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>
>> @@ -140,6 +141,64 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>>         return IRQ_HANDLED;
>>  }
>>
>> +int cedrus_hw_suspend(struct device *device)
>> +{
>> +       struct cedrus_dev *dev = dev_get_drvdata(device);
>> +
>> +       reset_control_assert(dev->rstc);
>> +
>> +       clk_disable_unprepare(dev->ram_clk);
>> +       clk_disable_unprepare(dev->mod_clk);
>> +       clk_disable_unprepare(dev->ahb_clk);
>> +
> 
> You can use clk_bulk API here.

Since this change is already tested, I'd prefer to do that as a separate patch.

>> +       return 0;
>> +}
>> +
>> +int cedrus_hw_resume(struct device *device)
>> +{
>> +       struct cedrus_dev *dev = dev_get_drvdata(device);
>> +       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_probe(struct cedrus_dev *dev)
>>  {
>>         const struct cedrus_variant *variant;
>> @@ -236,42 +295,17 @@ 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_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 +316,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..45f641f0bfa2 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_suspend(struct device *device);
>> +int cedrus_hw_resume(struct device *device);
>> +
>>  int cedrus_hw_probe(struct cedrus_dev *dev);
>>  void cedrus_hw_remove(struct cedrus_dev *dev);
>>
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> index ed3f511f066f..16d82309e7b6 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> @@ -13,6 +13,8 @@
>>   * Marek Szyprowski, <m.szyprowski@samsung.com>
>>   */
>>
>> +#include <linux/pm_runtime.h>
>> +
>>  #include <media/videobuf2-dma-contig.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-ioctl.h>
>> @@ -450,12 +452,24 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count)
>>                 return -EINVAL;
>>         }
>>
>> -       if (V4L2_TYPE_IS_OUTPUT(vq->type) &&
>> -           dev->dec_ops[ctx->current_codec]->start)
>> -               ret = dev->dec_ops[ctx->current_codec]->start(ctx);
>> +       if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
>> +               ret = pm_runtime_get_sync(dev->dev);
> 
> It's entirely up to you, but you could do get_sync/put
> between .device_run, as everything should happen
> in the context of v4l2_m2m_ops.device_run.
> (In that case, I believe you'd want to enable autosuspend.)
> 
> Not sure there's much to gain from an power consumption
> pov, though.

Agreed. Either you have a short autosuspend delay, and get per-frame latency; or
a longer delay, and the hardware is on for the entire stream anyway. So I don't
think it's worth the overhead.

> With the Originally-by changed:
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> Thanks,
> Ezequiel
> 

Thanks,
Samuel

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

[Maxime Ripard]

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

On Sat, May 09, 2020 at 03:03:55PM -0500, Samuel Holland wrote:
> On 5/5/20 7:53 AM, Ezequiel Garcia wrote:
> > On Wed, 22 Apr 2020 at 01:00, Samuel Holland <samuel@sholland.org> wrote:
> >>
> >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> >> A runtime PM reference is held while streaming.
> >>
> >> Originally-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> > 
> > Originally-by is not documented, perhaps just go with Signed-off-by,
> > as Jernej suggested.
> > 
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >> v2: moved PM reference to cedrus_{start,stop}_streaming, based on an
> >>     earlier patch by Jernej Skrabec. Removes the need for autosuspend.
> >>     I tested this with running 2x v4l2-request-test in parallel.
> >>
> >> ---
> >>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   7 ++
> >>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 106 ++++++++++++------
> >>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |   3 +
> >>  .../staging/media/sunxi/cedrus/cedrus_video.c |  33 ++++--
> >>  4 files changed, 104 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> >> index 05a85517ff60..bc27f9430eeb 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>
> >> @@ -551,12 +552,18 @@ static const struct of_device_id cedrus_dt_match[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(of, cedrus_dt_match);
> >>
> >> +static const struct dev_pm_ops cedrus_dev_pm_ops = {
> >> +       SET_RUNTIME_PM_OPS(cedrus_hw_suspend,
> >> +                          cedrus_hw_resume, NULL)
> >> +};
> >> +
> >>  static struct platform_driver cedrus_driver = {
> >>         .probe          = cedrus_probe,
> >>         .remove         = cedrus_remove,
> >>         .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..1744e6fcc999 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>
> >> @@ -140,6 +141,64 @@ static irqreturn_t cedrus_irq(int irq, void *data)
> >>         return IRQ_HANDLED;
> >>  }
> >>
> >> +int cedrus_hw_suspend(struct device *device)
> >> +{
> >> +       struct cedrus_dev *dev = dev_get_drvdata(device);
> >> +
> >> +       reset_control_assert(dev->rstc);
> >> +
> >> +       clk_disable_unprepare(dev->ram_clk);
> >> +       clk_disable_unprepare(dev->mod_clk);
> >> +       clk_disable_unprepare(dev->ahb_clk);
> >> +
> > 
> > You can use clk_bulk API here.
> 
> Since this change is already tested, I'd prefer to do that as a separate
> patch.

Given that those three clocks are also pretty different from a semantic point of
view, I'm not sure it's wise to switch to the bulk API anyway.

Maxime

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