Re: [PATCH] media: verisilicon: vp9: do not arm watchdog on preparation failure

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 23/07/2026 à 17:36, Tharit Tangkijwanichakul a écrit :
> hantro_g2_vp9_dec_run() calls hantro_end_prepare_run() when
> start_prepare_run() fails to find one of the required VP9 controls.
>
> hantro_end_prepare_run() completes the control request, but also schedules
> the hardware watchdog. Since the decode operation has not been started on
> this error path, the watchdog should not be scheduled.
>
> Factor the control-request completion out of hantro_end_prepare_run() and
> call it directly from the VP9 preparation error path. This preserves the
> pairing with hantro_start_prepare_run() without scheduling the watchdog.

Nack.
First it schedules a software timer not an hardware timer, which led after
timeout to call hantro_job_finish_no_pm() with VB2_BUF_STATE_ERROR parameter.
We need this call to complete v4l2_m2m job.

If you want to stop the task before the timeout you need to add something like
hantro_end_prepare_run_with_error() where you do the correct sequence.

Regards,
Benjamin

>
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Testing:
>
> - Built with the Verisilicon Hantro driver enabled.
> - Not runtime-tested: the available RK3588 board does not expose a Hantro
>    G2 VP9 decoder.
>
>   drivers/media/platform/verisilicon/hantro_drv.c    | 13 +++++++++----
>   .../media/platform/verisilicon/hantro_g2_vp9_dec.c | 14 +++++++++-----
>   drivers/media/platform/verisilicon/hantro_hw.h     |  1 +
>   3 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..04167e4be003 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -147,10 +147,17 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>   	}
>   }
>   
> -void hantro_end_prepare_run(struct hantro_ctx *ctx)
> +void hantro_complete_ctrl_request(struct hantro_ctx *ctx)
>   {
>   	struct vb2_v4l2_buffer *src_buf;
>   
> +	src_buf = hantro_get_src_buf(ctx);
> +	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> +				   &ctx->ctrl_handler);
> +}
> +
> +void hantro_end_prepare_run(struct hantro_ctx *ctx)
> +{
>   	if (!ctx->is_encoder && ctx->dev->variant->late_postproc) {
>   		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
>   			hantro_postproc_enable(ctx);
> @@ -158,9 +165,7 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>   			hantro_postproc_disable(ctx);
>   	}
>   
> -	src_buf = hantro_get_src_buf(ctx);
> -	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> -				   &ctx->ctrl_handler);
> +	hantro_complete_ctrl_request(ctx);
>   
>   	/* Kick the watchdog. */
>   	schedule_delayed_work(&ctx->dev->watchdog_work,
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> index 56c79e339030..e08db6e3f6f9 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> @@ -36,12 +36,14 @@ static int start_prepare_run(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_
>   
>   	ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, V4L2_CID_STATELESS_VP9_FRAME);
>   	if (WARN_ON(!ctrl))
> -		return -EINVAL;
> +		goto err_complete_request;
> +
>   	*dec_params = ctrl->p_cur.p;
>   
>   	ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, V4L2_CID_STATELESS_VP9_COMPRESSED_HDR);
>   	if (WARN_ON(!ctrl))
> -		return -EINVAL;
> +		goto err_complete_request;
> +
>   	prob_updates = ctrl->p_cur.p;
>   	vp9_ctx->cur.tx_mode = prob_updates->tx_mode;
>   
> @@ -86,6 +88,10 @@ static int start_prepare_run(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_
>   	v4l2_vp9_fw_update_probs(&vp9_ctx->probability_tables, prob_updates, *dec_params);
>   
>   	return 0;
> +
> +err_complete_request:
> +	hantro_complete_ctrl_request(ctx);
> +	return -EINVAL;
>   }
>   
>   static struct hantro_decoded_buffer *
> @@ -894,10 +900,8 @@ int hantro_g2_vp9_dec_run(struct hantro_ctx *ctx)
>   	int ret;
>   
>   	ret = start_prepare_run(ctx, &decode_params);
> -	if (ret) {
> -		hantro_end_prepare_run(ctx);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	src = hantro_get_src_buf(ctx);
>   	dst = hantro_get_dst_buf(ctx);
> diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
> index 13e573f1f19d..9d4e858f17c7 100644
> --- a/drivers/media/platform/verisilicon/hantro_hw.h
> +++ b/drivers/media/platform/verisilicon/hantro_hw.h
> @@ -430,6 +430,7 @@ void hantro_watchdog(struct work_struct *work);
>   void hantro_irq_done(struct hantro_dev *vpu,
>   		     enum vb2_buffer_state result);
>   void hantro_start_prepare_run(struct hantro_ctx *ctx);
> +void hantro_complete_ctrl_request(struct hantro_ctx *ctx);
>   void hantro_end_prepare_run(struct hantro_ctx *ctx);
>   
>   irqreturn_t hantro_g1_irq(int irq, void *dev_id);
>
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482