Message-ID: <e8ca48e2-264d-462b-89a6-0c788a58ce77@ti.com>
Date: Mon, 20 Oct 2025 14:29:31 -0500
From: Brandon Brnich <b-brnich@...com>
To: Nicolas Dufresne <nicolas.dufresne@...labora.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 Nicolas,

On 10/20/2025 1:41 PM, Nicolas Dufresne wrote:
> 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.

Understood, I provide a better explanation in next patch. I can see how 
some of the below can be confusing.

> 
>>
>> 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.

Understood

> 
>> +		}
>>   		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.

Might not be important to do anymore if your suggestions on the 
conditional at the bottom of this patch work. I only moved this code to 
it's own function since I updated device_run to have capability to 
support the same state change if it was not achieved in start_streaming.

> 
>> +	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

Wouldn't this be the no-op and cause no state change? VPU only goes to 
OPEN when start_streaming is called on the OUTPUT plane.

>     STREAMON(OUT)
>     	- Initialize the sequence and prepare the FB
>     	- Leaving with PIC_RUN state

So this state would be opening and leaving in state OPEN, but since CAP 
was called first, it should be streaming so check would happen.

> 
> 
> Case 2:
>     STREAMON(OUT)
>     	- no-op
This would put into state open.

>     STREAMON(CAP)
>     	- To OPEN

It would already be in state open since STREAMON(OUT) put it there.

>     	- To INIT_SEQ
>     	- To PIC_RUN
Then the above two don't happen due to below condition failing.

>     
> 
> 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)) {

I can test this and I'm sure it will work because this would force it to 
enter that code block to regardless on the 2nd start_streaming case. 
This appears to be the cleaner solution without messing around with the 
state's in job_ready and device_run. So this patch becomes much simpler. 
I will split into two: 1. updating this conditional (assuming it works) 
and 2. updating cmd_stop to go directly to try_schedule. Both patches 
with better commit messages.

Best,
Brandon

> 
> cheers,
> Nicolas

Powered by blists - more mailing lists