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

[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.koc...@bootlin.com>

Cheers,

Paul

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

[Jernej Škrabec]

Hi Samuel!

Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):

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

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

[Paul Kocialkowski]

Hi,

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

[Jernej Škrabec]

Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
napisal(a):

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

[Paul Kocialkowski]

Hi Jernej,

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

Cheers,

Paul

[Jernej Škrabec]

Hi!

Dne ponedeljek, 20. april 2020 ob 19:56:51 CEST je Paul Kocialkowski

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

Best

[Ezequiel Garcia]

Hi Jernej, Paul:

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

[Paul Kocialkowski]

Hi,

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

[Paul Kocialkowski]

Hi Ezequiel,

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!

[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....@gmail.com>

Signed-off-by: Samuel Holland <sam...@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);

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

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

[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....@gmail.com>

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

> Signed-off-by: Samuel Holland <sam...@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....@siol.net>

Best regards,
Jernej

[Ezequiel Garcia]

On Wed, 22 Apr 2020 at 01:00, Samuel Holland <sam...@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....@gmail.com>

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

You can use clk_bulk API here.

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 <ezeq...@collabora.com>

Thanks,
Ezequiel

[Samuel Holland]

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

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 <ezeq...@collabora.com>
>
> Thanks,
> Ezequiel
>

Thanks,
Samuel