Message-ID: <351e25ea533c440e3fa5131fe44796f66bc4ff82.camel@collabora.com>
Date: Mon, 20 Oct 2025 14:41:18 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Brandon Brnich <b-brnich@...com>, Nas Chung <nas.chung@...psnmedia.com>,
  Jackson Lee <jackson.lee@...psnmedia.com>, Mauro Carvalho Chehab
 <mchehab@...nel.org>, 	linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: Darren Etheridge <detheridge@...com>
Subject: Re: [PATCH] media: chips-media: wave5: Fix Hang in Encoder

Hi Brandon,


Le lundi 20 octobre 2025 à 12:33 -0500, Brandon Brnich a écrit :
> Wave5 encoder driver only changed states to PIC_RUN in start streaming by
> making the assumption that VIDIOC STREAMON call has already been called.
> In frameworks like FFMPEG, this condition has not been met when in the
> start streaming function resulting in the application hanging. Therefore,
> job_ready and device_run need to be extended to have support for this case.

I'm afraid you will have to rework that commit message in V2, I could not make
much sense out of it. See comments below, but by spliting your patch, it might
get easier to explain what you are trying to fix.

> 
> Signed-off-by: Brandon Brnich <b-brnich@...com>
> ---
>  .../chips-media/wave5/wave5-vpu-enc.c         | 74 +++++++++++++------
>  1 file changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> index a02853d42d61..3a3b585ceb8e 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> @@ -705,6 +705,11 @@ static int wave5_vpu_enc_encoder_cmd(struct file *file, void *fh, struct v4l2_en
>  
>  		m2m_ctx->last_src_buf = v4l2_m2m_last_src_buf(m2m_ctx);
>  		m2m_ctx->is_draining = true;
> +
> +		if (v4l2_m2m_num_dst_bufs_ready(m2m_ctx) > 0) {

Its job_ready callback and framework task to check this, I think you can go
directly to try to schedule.

> +			dev_dbg(inst->dev->dev, "Forcing job run for draining\n");
> +			v4l2_m2m_try_schedule(m2m_ctx);

This is fair, and the decoder does the same. Though, it has nothing to do with
the transition from OPEN -> SEQ_INIT -> PIC_RUN. Do this in its own commit with
its own explanation.

> +		}
>  		break;
>  	case V4L2_ENC_CMD_START:
>  		break;
> @@ -1411,6 +1416,34 @@ static int prepare_fb(struct vpu_instance *inst)
>  	return ret;
>  }
>  
> +static int wave5_vpu_enc_prepare_cap_seq(struct vpu_instance *inst)
> +{

Factor-out in its own commit, with a message this is preparation work and with
no function changes. Its really hard to review code that moves around and may
have changes in it.

> +	int ret = 0;
> +
> +	ret = initialize_sequence(inst);
> +	if (ret) {
> +		dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
> +		return ret;
> +	}
> +	ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The sequence must be analyzed first to calculate the proper
> +	 * size of the auxiliary buffers.
> +	 */
> +	ret = prepare_fb(inst);
> +	if (ret) {
> +		dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
> +
> +	return ret;
> +}
> +
>  static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>  	struct vpu_instance *inst = vb2_get_drv_priv(q);
> @@ -1453,27 +1486,8 @@ static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
>  		if (ret)
>  			goto return_buffers;
>  	}
> -	if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {
> -		ret = initialize_sequence(inst);
> -		if (ret) {
> -			dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
> -			goto return_buffers;
> -		}
> -		ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> -		if (ret)
> -			goto return_buffers;
> -		/*
> -		 * The sequence must be analyzed first to calculate the proper
> -		 * size of the auxiliary buffers.
> -		 */
> -		ret = prepare_fb(inst);
> -		if (ret) {
> -			dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> -			goto return_buffers;
> -		}
> -
> -		ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
> -	}
> +	if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming)
> +		ret = wave5_vpu_enc_prepare_cap_seq(inst);
>  	if (ret)
>  		goto return_buffers;
>  
> @@ -1598,6 +1612,14 @@ static void wave5_vpu_enc_device_run(void *priv)
>  
>  	pm_runtime_resume_and_get(inst->dev->dev);
>  	switch (inst->state) {
> +	case VPU_INST_STATE_OPEN:
> +		ret = wave5_vpu_enc_prepare_cap_seq(inst);
> +		if (ret) {
> +			dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> +			switch_state(inst, VPU_INST_STATE_STOP);
> +			break;
> +		}
> +		fallthrough;
>  	case VPU_INST_STATE_PIC_RUN:
>  		ret = start_encode(inst, &fail_res);
>  		if (ret) {
> @@ -1633,6 +1655,12 @@ static int wave5_vpu_enc_job_ready(void *priv)
>  	case VPU_INST_STATE_NONE:
>  		dev_dbg(inst->dev->dev, "Encoder must be open to start queueing M2M jobs!\n");
>  		return false;
> +	case VPU_INST_STATE_OPEN:
> +		if (wave5_vpu_both_queues_are_streaming(inst)) {
> +			dev_dbg(inst->dev->dev, "Both queues have been turned on now, M2M job can occur\n");
> +			return true;
> +		}
> +		return false;
>  	case VPU_INST_STATE_PIC_RUN:
>  		if (m2m_ctx->is_draining || v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
>  			dev_dbg(inst->dev->dev, "Encoder ready for a job, state: %s\n",
> @@ -1642,9 +1670,9 @@ static int wave5_vpu_enc_job_ready(void *priv)
>  		fallthrough;
>  	default:
>  		dev_dbg(inst->dev->dev,
> -			"Encoder not ready for a job, state: %s, %s draining, %d src bufs ready\n",
> +			"Encoder not ready for a job, state: %s, %s draining, %d src bufs ready, %d dst bufs ready\n",
>  			state_to_str(inst->state), m2m_ctx->is_draining ? "is" : "is not",
> -			v4l2_m2m_num_src_bufs_ready(m2m_ctx));
> +			v4l2_m2m_num_src_bufs_ready(m2m_ctx), v4l2_m2m_num_dst_bufs_ready(m2m_ctx));
>  		break;
>  	}
>  	return false;

Perhaps its going to be clear with proper commit message, but I'm still not
clear how you can endup with both queues streaming without two calls to
wave5_vpu_enc_start_streaming(). I don't deny the condition might be broken
then, but the intent is for this code to bring the driver to PIC_RUN on the
second call.

From VPU_INST_STATE_NONE:

Case 1:
   STREAMON(CAP)
   	- bring it to OPEN state
   STREAMON(OUT)
   	- Initialize the sequence and prepare the FB
   	- Leaving with PIC_RUN state


Case 2:
   STREAMON(OUT)
   	- no-op
   STREAMON(CAP)
   	- To OPEN
   	- To INIT_SEQ
   	- To PIC_RUN
   

So in case 2, the code fails this condition:

	if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {


Basically type == CAP, and vb2 won't be setting the .streaming state before this
function returns. A possible solution would be:

	if (inst->state == VPU_INST_STATE_OPEN &&
	    (m2m_ctx->cap_q_ctx.q.streaming || type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)) {

cheers,
Nicolas

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists