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

[Frank Li]

On Sun, Jul 26, 2026 at 02:42:43AM +0000, Tharit Tangkijwanichakul wrote:
> [You don't often get email from tharitt97@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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.
>
> Make the clocks part of the device's runtime PM state by enabling them
> from the runtime-resume callback and disabling them from the
> runtime-suspend callback.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@xxxxxxxxx>
> ---
> Changes in v3:
> - Move clock enable and disable operations into the runtime PM
>   callbacks, as suggested by Frank Li.
> - Remove vpu from argument in hantro_job_finish() and
>   hantro_job_finish_no_pm(). It is accessible by ctx.
>
> 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 ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
>
>   H.264 (JVT-AVC_V1):          129/135, unchanged
>   MPEG-2 (MPEG2_VIDEO-MAIN):    23/43, unchanged
>   VP8 (VP8-TEST-VECTORS):       61/61, unchanged
>
>  .../media/platform/verisilicon/hantro_drv.c   | 68 +++++++++++--------
>  1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..7978bb72569e 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
>         .type = V4L2_EVENT_EOS
>  };
>
> -static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> -                                   struct hantro_ctx *ctx,
> +static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
>                                     enum vb2_buffer_state result)
>  {
>         struct vb2_v4l2_buffer *src, *dst;
> @@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
>                                          result);
>  }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> -                             struct hantro_ctx *ctx,
> +static void hantro_job_finish(struct hantro_ctx *ctx,
>                               enum vb2_buffer_state result)
>  {
> -       pm_runtime_put_autosuspend(vpu->dev);
> -
> -       clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> +       struct hantro_dev *vpu = ctx->dev;
>
> -       hantro_job_finish_no_pm(vpu, ctx, result);
> +       pm_runtime_put_autosuspend(vpu->dev);
> +       hantro_job_finish_no_pm(ctx, result);
>  }
>
>  void hantro_irq_done(struct hantro_dev *vpu,
> @@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
>         if (cancel_delayed_work(&vpu->watchdog_work)) {
>                 if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
>                         ctx->codec_ops->done(ctx);
> -               hantro_job_finish(vpu, ctx, result);
> +               hantro_job_finish(ctx, result);
>         }
>  }
>
> @@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
>                 vpu_err("frame processing timed out!\n");
>                 if (ctx->codec_ops->reset)
>                         ctx->codec_ops->reset(ctx);
> -               hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
> +               hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
>         }
>  }
>
> @@ -170,29 +167,24 @@ 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);
> -       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;
> +       ret = pm_runtime_resume_and_get(vpu->dev);
> +       if (ret < 0) {
> +               hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> +               return;
> +       }
>
>         v4l2_m2m_buf_copy_metadata(src, dst);
>
> -       if (ctx->codec_ops->run(ctx))
> -               goto err_cancel_job;
> -
> -       return;
> -
> -err_cancel_job:
> -       hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> +       ret = ctx->codec_ops->run(ctx);
> +       if (ret)
> +               hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
>  }
>
>  static const struct v4l2_m2m_ops vpu_m2m_ops = {
> @@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
>         pm_runtime_disable(vpu->dev);
>  }
>
> -#ifdef CONFIG_PM

You need sperate patch to remove this. simple remove this here may cause
build error or warning at NO CONFIG_PM config

I suggest use following patch do below thing

1. Remove CONFIG_PM

2. Use DEFINE_RUNTIME_DEV_PM_OP()

3, Need update

.pm = pm_ptr(&hantro_pm_ops),

Frank

>  static int hantro_runtime_resume(struct device *dev)
>  {
>         struct hantro_dev *vpu = dev_get_drvdata(dev);
> +       int ret;
>
> -       if (vpu->variant->runtime_resume)
> -               return vpu->variant->runtime_resume(vpu);
> +       ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
> +       if (ret)
> +               return ret;
> +
> +       if (vpu->variant->runtime_resume) {
> +               ret = vpu->variant->runtime_resume(vpu);
> +               if (ret)
> +                       goto err_disable_clocks;
> +       }
>
>         return 0;
> +
> +err_disable_clocks:
> +       clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> +       return ret;
> +}
> +
> +static int hantro_runtime_suspend(struct device *dev)
> +{
> +       struct hantro_dev *vpu = dev_get_drvdata(dev);
> +
> +       clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> +       return 0;
>  }
> -#endif
>
>  static const struct dev_pm_ops hantro_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>                                 pm_runtime_force_resume)
> -       SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
> +       SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
>  };
>
>  static struct platform_driver hantro_driver = {
> --
> 2.47.3
>