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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Thanks!
Maxime
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 <[email protected]>
Cheers,
Paul
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
> ---
>
> 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);
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 <[email protected]>
> ---
>
> 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);
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 <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
> >> ---
> >>
> >> 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
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 <[email protected]>
> > >> ---
> > >>
> > >> 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
Hi Jernej, Paul:
On Mon, 20 Apr 2020 at 13:41, Jernej Škrabec <[email protected]> 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 <[email protected]>
> > > >> ---
> > > >>
> > > >> 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
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 <[email protected]> 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 <[email protected]>
> > > > >> ---
> > > > >>
> > > > >> 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