Re: [PATCH v5 1/2] media: hantro: release runtime resources when device_run fails

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Jul 29, 2026 at 06:04:39AM +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.
>
> 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@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  .../media/platform/verisilicon/hantro_drv.c   | 66 +++++++++++--------
>  1 file changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..d6fcd4f7f9da 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 = {
> @@ -1296,10 +1288,30 @@ static void hantro_remove(struct platform_device *pdev)
>  static int hantro_runtime_resume(struct device *dev)
>  {
>  	struct hantro_dev *vpu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	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;
>
> -	if (vpu->variant->runtime_resume)
> -		return vpu->variant->runtime_resume(vpu);
> +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
> @@ -1307,7 +1319,7 @@ static int hantro_runtime_resume(struct device *dev)
>  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
>