[PATCH] media: hantro: release runtime resources when device_run fails

[PATCH] media: hantro: release runtime resources when device_run fails

[Tharit Tangkijwanichakul]

device_run() acquires a runtime PM reference and enables the VPU clocks
before invoking the codec-specific run callback.

If clk_bulk_enable() fails, the runtime PM reference is left held. If
the codec-specific run callback fails, both the enabled clocks and the
runtime PM reference are left held.

Add separate error paths to release the resources acquired by
device_run(). Disable the clocks when the codec run callback fails, and
drop the runtime PM reference when either clock enabling or the codec
run callback fails.

Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
---
Tested on a Rockchip RK3588 (Rock 5B) board with Fluster:
  H.264 (JVT-AVC_V1):	     129/135, unchanged
  MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
  VP8 (VP8-TEST-VECTORS):    61/61, unchanged

 drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 2e81877f640f..9daaf129211d 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -170,6 +170,7 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
 static void device_run(void *priv)
 {
 	struct hantro_ctx *ctx = priv;
+	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src, *dst;
 	int ret;
 
@@ -178,11 +179,11 @@ static void device_run(void *priv)
 
 	ret = pm_runtime_resume_and_get(ctx->dev->dev);
 	if (ret < 0)
-		goto err_cancel_job;
+		goto err_disable_clock;
 
 	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
 	if (ret)
-		goto err_cancel_job;
+		goto err_pm_put_autosuspend;
 
 	v4l2_m2m_buf_copy_metadata(src, dst);
 
@@ -191,8 +192,12 @@ static void device_run(void *priv)
 
 	return;
 
+err_disable_clock:
+	clk_bulk_disable(vpu->variant->num_clocks, ctx->dev->clocks);
+err_pm_put_autosuspend:
+	pm_runtime_put_autosuspend(vpu->dev);
 err_cancel_job:
-	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+	hantro_job_finish_no_pm(vpu, ctx, VB2_BUF_STATE_ERROR);
 }
 
 static const struct v4l2_m2m_ops vpu_m2m_ops = {

base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.47.3

Re: [PATCH] media: hantro: release runtime resources when device_run fails

[Philipp Zabel]

On Fr, 2026-07-24 at 11:15 +0000, Tharit Tangkijwanichakul wrote:
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
> 
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
> 
> Add separate error paths to release the resources acquired by
> device_run(). Disable the clocks when the codec run callback fails, and
> drop the runtime PM reference when either clock enabling or the codec
> run callback fails.
> 
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Tested on a Rockchip RK3588 (Rock 5B) board with Fluster:
>   H.264 (JVT-AVC_V1):	     129/135, unchanged
>   MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
>   VP8 (VP8-TEST-VECTORS):    61/61, unchanged
> 
>  drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..9daaf129211d 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -170,6 +170,7 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>  static void device_run(void *priv)
>  {
>  	struct hantro_ctx *ctx = priv;
> +	struct hantro_dev *vpu = ctx->dev;
>  	struct vb2_v4l2_buffer *src, *dst;
>  	int ret;
>  
> @@ -178,11 +179,11 @@ static void device_run(void *priv)
>  
>  	ret = pm_runtime_resume_and_get(ctx->dev->dev);
>  	if (ret < 0)
> -		goto err_cancel_job;
> +		goto err_disable_clock;

This doesn't make any sense.

>  	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
>  	if (ret)
> -		goto err_cancel_job;
> +		goto err_pm_put_autosuspend;

This looks fine to me.

>  	v4l2_m2m_buf_copy_metadata(src, dst);

But right below this, you are still letting

	if (ctx->codec_ops->run(ctx))
		goto err_cancel_job;

without disabling the clocks.

>  
> @@ -191,8 +192,12 @@ static void device_run(void *priv)
>  
>  	return;
>  
> +err_disable_clock:
> +	clk_bulk_disable(vpu->variant->num_clocks, ctx->dev->clocks);

You have added vpu = ctx->dev, why not use vpu->clocks as second
parameter?

> +err_pm_put_autosuspend:
> +	pm_runtime_put_autosuspend(vpu->dev);
>  err_cancel_job:
> -	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> +	hantro_job_finish_no_pm(vpu, ctx, VB2_BUF_STATE_ERROR);

Why are you changing ctx->dev to vpu here, but not in the other
function calls in device_run(), e.g. pm_runtime_resume_and_get() and
clk_bulk_enable() above?

regards
Philipp

Re: [PATCH] media: hantro: release runtime resources when device_run fails

[Tharit Tangkijwanichakul]

> But right below this, you are still letting
>
>         if (ctx->codec_ops->run(ctx))
>                 goto err_cancel_job;
>
> without disabling the clocks.
>
> >
> > @@ -191,8 +192,12 @@ static void device_run(void *priv)
> >
> >       return;
> >
> > +err_disable_clock:
> > +     clk_bulk_disable(vpu->variant->num_clocks, ctx->dev->clocks);
>
> You have added vpu = ctx->dev, why not use vpu->clocks as second
> parameter?
>
> > +err_pm_put_autosuspend:
> > +     pm_runtime_put_autosuspend(vpu->dev);
> >  err_cancel_job:
> > -     hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> > +     hantro_job_finish_no_pm(vpu, ctx, VB2_BUF_STATE_ERROR);
>
> Why are you changing ctx->dev to vpu here, but not in the other
> function calls in device_run(), e.g. pm_runtime_resume_and_get() and
> clk_bulk_enable() above?
>
> regards
> Philipp

Hi Philipp,

This is embarrassing. I did notice this, made a local change and
forgot to re-run git add.
I will fix this asap and send a v2.

Best,
Tharit

[PATCH v2] media: hantro: release runtime resources when device_run fails

[Tharit Tangkijwanichakul]

device_run() acquires a runtime PM reference and enables the VPU clocks
before invoking the codec-specific run callback.

If clk_bulk_enable() fails, the runtime PM reference is left held. If
the codec-specific run callback fails, both the enabled clocks and the
runtime PM reference are left held.

Add separate error paths to release the resources acquired by
device_run(). Disable the clocks when the codec run callback fails, and
drop the runtime PM reference when either clock enabling or the codec
run callback fails.

Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
---
Changes in v2:
	- Fix the codec run failure path to disable the clocks before dropping
	  the runtime PM reference.
	- Use a local vpu variable in device_run().

Tested on a Rockchip RK3588 (Rock 5B) board with Fluster:
  H.264 (JVT-AVC_V1):	     129/135, unchanged
  MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
  VP8 (VP8-TEST-VECTORS):    61/61, unchanged

 drivers/media/platform/verisilicon/hantro_drv.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 2e81877f640f..72250bb56872 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -170,29 +170,34 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
 static void device_run(void *priv)
 {
 	struct hantro_ctx *ctx = priv;
+	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src, *dst;
 	int ret;
 
 	src = hantro_get_src_buf(ctx);
 	dst = hantro_get_dst_buf(ctx);
 
-	ret = pm_runtime_resume_and_get(ctx->dev->dev);
+	ret = pm_runtime_resume_and_get(vpu->dev);
 	if (ret < 0)
 		goto err_cancel_job;
 
-	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
+	ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
 	if (ret)
-		goto err_cancel_job;
+		goto err_pm_put_autosuspend;
 
 	v4l2_m2m_buf_copy_metadata(src, dst);
 
 	if (ctx->codec_ops->run(ctx))
-		goto err_cancel_job;
+		goto err_disable_clock;
 
 	return;
 
+err_disable_clock:
+	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+err_pm_put_autosuspend:
+	pm_runtime_put_autosuspend(vpu->dev);
 err_cancel_job:
-	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+	hantro_job_finish_no_pm(vpu, ctx, VB2_BUF_STATE_ERROR);
 }
 
 static const struct v4l2_m2m_ops vpu_m2m_ops = {
-- 
2.47.3

Re: [PATCH] media: hantro: release runtime resources when device_run fails

[Frank Li]

On Fri, Jul 24, 2026 at 11:15:48AM +0000, Tharit Tangkijwanichakul wrote:
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
>
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
>
> Add separate error paths to release the resources acquired by
> device_run(). Disable the clocks when the codec run callback fails, and
> drop the runtime PM reference when either clock enabling or the codec
> run callback fails.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Tested on a Rockchip RK3588 (Rock 5B) board with Fluster:
>   H.264 (JVT-AVC_V1):	     129/135, unchanged
>   MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
>   VP8 (VP8-TEST-VECTORS):    61/61, unchanged
>
>  drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..9daaf129211d 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -170,6 +170,7 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>  static void device_run(void *priv)
>  {
>  	struct hantro_ctx *ctx = priv;
> +	struct hantro_dev *vpu = ctx->dev;
>  	struct vb2_v4l2_buffer *src, *dst;
>  	int ret;
>
> @@ -178,11 +179,11 @@ static void device_run(void *priv)
>
>  	ret = pm_runtime_resume_and_get(ctx->dev->dev);
>  	if (ret < 0)
> -		goto err_cancel_job;
> +		goto err_disable_clock;
>
>  	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
>  	if (ret)
> -		goto err_cancel_job;
> +		goto err_pm_put_autosuspend;

Mix use runtime pm and clock is not good.

You can put clk_bulk_enable() into runtime pm resume call back
and clk_bulk_disable() into runtime pm suspend call back.

Frank

>
>  	v4l2_m2m_buf_copy_metadata(src, dst);
>
> @@ -191,8 +192,12 @@ static void device_run(void *priv)
>
>  	return;
>
> +err_disable_clock:
> +	clk_bulk_disable(vpu->variant->num_clocks, ctx->dev->clocks);
> +err_pm_put_autosuspend:
> +	pm_runtime_put_autosuspend(vpu->dev);
>  err_cancel_job:
> -	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> +	hantro_job_finish_no_pm(vpu, ctx, VB2_BUF_STATE_ERROR);
>  }
>
>  static const struct v4l2_m2m_ops vpu_m2m_ops = {
>
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> --
> 2.47.3
>