The device_run() first enables the clock and then
tries to resume PM runtime, checking for errors.
Well, if for some reason the pm_runtime can not resume,
it would be better to detect it beforehand.
So, change the order inside device_run().
Reviewed-by: Ezequiel Garcia <[email protected]>
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..4387edaa1d0d 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -152,13 +152,14 @@ static void device_run(void *priv)
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
- ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
- if (ret)
- goto err_cancel_job;
ret = pm_runtime_get_sync(ctx->dev->dev);
if (ret < 0)
goto err_cancel_job;
+ ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
+ if (ret)
+ goto err_cancel_job;
+
v4l2_m2m_buf_copy_metadata(src, dst, true);
ctx->codec_ops->run(ctx);
--
2.30.2
On Wed, 5 May 2021 11:41:59 +0200
Mauro Carvalho Chehab <[email protected]> wrote:
> The device_run() first enables the clock and then
> tries to resume PM runtime, checking for errors.
>
> Well, if for some reason the pm_runtime can not resume,
> it would be better to detect it beforehand.
>
> So, change the order inside device_run().
>
> Reviewed-by: Ezequiel Garcia <[email protected]>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Does this move not result in a potential call of clk_bulk_disable() for clocks
that aren't enabled?
> ---
> drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..4387edaa1d0d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -152,13 +152,14 @@ static void device_run(void *priv)
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> ret = pm_runtime_get_sync(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>
> + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> + if (ret)
> + goto err_cancel_job;
> +
> v4l2_m2m_buf_copy_metadata(src, dst, true);
>
> ctx->codec_ops->run(ctx);
Hi Mauro,
Thanks for working on this.
On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> The device_run() first enables the clock and then
> tries to resume PM runtime, checking for errors.
>
> Well, if for some reason the pm_runtime can not resume,
> it would be better to detect it beforehand.
>
> So, change the order inside device_run().
>
> Reviewed-by: Ezequiel Garcia <[email protected]>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
It seems this is wrong now, as this series doesn't have
https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
I don't fully understand why all the back and forth
happening on this series, but the former Hantro patches
looked good (despite perhaps unclear commit messages).
Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
media: hantro: use pm_runtime_resume_and_get()
media: hantro: do a PM resume earlier
?
Thanks,
Ezequiel
> drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..4387edaa1d0d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -152,13 +152,14 @@ static void device_run(void *priv)
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> ret = pm_runtime_get_sync(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>
> + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> + if (ret)
> + goto err_cancel_job;
> +
> v4l2_m2m_buf_copy_metadata(src, dst, true);
>
> ctx->codec_ops->run(ctx);
Em Wed, 05 May 2021 10:22:03 -0300
Ezequiel Garcia <[email protected]> escreveu:
> Hi Mauro,
>
> Thanks for working on this.
>
> On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> > The device_run() first enables the clock and then
> > tries to resume PM runtime, checking for errors.
> >
> > Well, if for some reason the pm_runtime can not resume,
> > it would be better to detect it beforehand.
> >
> > So, change the order inside device_run().
> >
> > Reviewed-by: Ezequiel Garcia <[email protected]>
> > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> It seems this is wrong now, as this series doesn't have
>
> https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
>
> I don't fully understand why all the back and forth
> happening on this series, but the former Hantro patches
> looked good (despite perhaps unclear commit messages).
There was a request to break the original /79 series into smaller ones,
to make easier for reviewers. So, I opted to split it into (probably)
3 series:
1. Fixes (this series);
2. "use pm_runtime_resume_and_get" for the I2C drivers;
3. "use pm_runtime_resume_and_get" for remaining ones.
Before flooding everybody's email's with series (2) and (3), better
to focus at the fixes first. I'll probably send the other two series
by tomorrow.
> Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
>
> media: hantro: use pm_runtime_resume_and_get()
> media: hantro: do a PM resume earlier
The problem is that pm_runtime_resume_and_get() was added only
recently (Kernel v5.10).
So, I opted to place the fix patches before the changes, as this
way, most (all?) patches can be easily be backported to legacy Kernels
as needed.
Thanks,
Mauro
Em Wed, 05 May 2021 11:01:35 -0300
Ezequiel Garcia <[email protected]> escreveu:
> On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 05 May 2021 10:22:03 -0300
> > Ezequiel Garcia <[email protected]> escreveu:
> >
> > > Hi Mauro,
> > >
> > > Thanks for working on this.
> > >
> > > On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> > > > The device_run() first enables the clock and then
> > > > tries to resume PM runtime, checking for errors.
> > > >
> > > > Well, if for some reason the pm_runtime can not resume,
> > > > it would be better to detect it beforehand.
> > > >
> > > > So, change the order inside device_run().
> > > >
> > > > Reviewed-by: Ezequiel Garcia <[email protected]>
> > > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > >
> > > It seems this is wrong now, as this series doesn't have
> > >
> > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
> > >
> > > I don't fully understand why all the back and forth
> > > happening on this series, but the former Hantro patches
> > > looked good (despite perhaps unclear commit messages).
> >
> > There was a request to break the original /79 series into smaller ones,
> > to make easier for reviewers. So, I opted to split it into (probably)
> > 3 series:
> >
> > 1. Fixes (this series);
> > 2. "use pm_runtime_resume_and_get" for the I2C drivers;
> > 3. "use pm_runtime_resume_and_get" for remaining ones.
> >
> > Before flooding everybody's email's with series (2) and (3), better
> > to focus at the fixes first. I'll probably send the other two series
> > by tomorrow.
> >
> > > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
> > >
> > > media: hantro: use pm_runtime_resume_and_get()
> > > media: hantro: do a PM resume earlier
> >
> > The problem is that pm_runtime_resume_and_get() was added only
> > recently (Kernel v5.10).
> >
> > So, I opted to place the fix patches before the changes, as this
> > way, most (all?) patches can be easily be backported to legacy Kernels
> > as needed.
> >
>
> Got it.
>
> Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
> and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync.
>
> And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get.
Works for me. So, the fixes patch will be the enclosed one, right?
Btw, I agree with Jonathan that the best would be to also move this:
clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
out of hantro_job_finish_no_pm(), as, when an error happens at
device_run(), the clock lines won't be enabled at the first place.
> Thanks,
> Ezequiel
Thanks,
Mauro
[PATCH] media: hantro: do a PM resume earlier
The device_run() first enables the clock and then
tries to resume PM runtime, checking for errors.
Well, if for some reason the pm_runtime can not resume,
it would be better to detect it beforehand.
So, change the order inside device_run().
Reviewed-by: Ezequiel Garcia <[email protected]>
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..bdb57fb56f47 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
return hantro_get_dec_buf_addr(ctx, buf);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
- enum vb2_buffer_state result)
+static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
- pm_runtime_mark_last_busy(vpu->dev);
- pm_runtime_put_autosuspend(vpu->dev);
clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
@@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
result);
}
+static void hantro_job_finish(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
+{
+ pm_runtime_mark_last_busy(vpu->dev);
+ pm_runtime_put_autosuspend(vpu->dev);
+
+ hantro_job_finish_no_pm(vpu, ctx, result);
+}
+
void hantro_irq_done(struct hantro_dev *vpu,
enum vb2_buffer_state result)
{
@@ -152,12 +160,15 @@ static void device_run(void *priv)
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
+ ret = pm_runtime_get_sync(ctx->dev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(ctx->dev->dev);
+ goto err_cancel_job;
+ }
+
ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
if (ret)
goto err_cancel_job;
- ret = pm_runtime_get_sync(ctx->dev->dev);
- if (ret < 0)
- goto err_cancel_job;
v4l2_m2m_buf_copy_metadata(src, dst, true);
@@ -165,7 +176,7 @@ static void device_run(void *priv)
return;
err_cancel_job:
- hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
}
static struct v4l2_m2m_ops vpu_m2m_ops = {
On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 05 May 2021 10:22:03 -0300
> Ezequiel Garcia <[email protected]> escreveu:
>
> > Hi Mauro,
> >
> > Thanks for working on this.
> >
> > On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> > > The device_run() first enables the clock and then
> > > tries to resume PM runtime, checking for errors.
> > >
> > > Well, if for some reason the pm_runtime can not resume,
> > > it would be better to detect it beforehand.
> > >
> > > So, change the order inside device_run().
> > >
> > > Reviewed-by: Ezequiel Garcia <[email protected]>
> > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> >
> > It seems this is wrong now, as this series doesn't have
> >
> > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
> >
> > I don't fully understand why all the back and forth
> > happening on this series, but the former Hantro patches
> > looked good (despite perhaps unclear commit messages).
>
> There was a request to break the original /79 series into smaller ones,
> to make easier for reviewers. So, I opted to split it into (probably)
> 3 series:
>
> 1. Fixes (this series);
> 2. "use pm_runtime_resume_and_get" for the I2C drivers;
> 3. "use pm_runtime_resume_and_get" for remaining ones.
>
> Before flooding everybody's email's with series (2) and (3), better
> to focus at the fixes first. I'll probably send the other two series
> by tomorrow.
>
> > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
> >
> > media: hantro: use pm_runtime_resume_and_get()
> > media: hantro: do a PM resume earlier
>
> The problem is that pm_runtime_resume_and_get() was added only
> recently (Kernel v5.10).
>
> So, I opted to place the fix patches before the changes, as this
> way, most (all?) patches can be easily be backported to legacy Kernels
> as needed.
>
Got it.
Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync.
And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get.
Thanks,
Ezequiel