linux-kernel - Re: [PATCH] media: amphion: Start decoding job when both queue are on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cb5df47b-f0f9-4420-8d36-73ea5903d57e@oss.nxp.com>
Date: Fri, 25 Apr 2025 10:51:28 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>, Ming Qian <ming.qian@....com>,
 mchehab@...nel.org, hverkuil-cisco@...all.nl
Cc: shawnguo@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de,
 kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
 xiahong.bao@....com, eagle.zhou@....com, tao.jiang_2@....com,
 imx@...ts.linux.dev, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: amphion: Start decoding job when both queue are on


Hi Nicolas,

On 2025/4/23 3:46, Nicolas Dufresne wrote:
> Hi,
> 
> Le vendredi 19 juillet 2024 à 16:50 +0900, Ming Qian a écrit :
>> Start the decoding job when both queue are on, except the for the
>> initialization sequence.
>>
>> Especially when seeking, the capture streamon may be called after output
>> streamon, driver will start to decode job immediately after output
>> streamo, if seek to a new resolution, then the source change flow may be
>> mixed with the seek, it will cause confusion, then may led to pipeline
>> hang.
>>
>> When both output and capture queue are on, it's ready to start the
>> decoding job, and it can avoid the above potential problem.
> 
> This commit message needs some work and I'm unsure I understand its
> meaning. After reading the change, I am under the impression that you
> simply say that once the seq_hdr is found, the driver should keep
> delaying the processing of output buffer until the capture queue is
> ready ?
> 

I just want to do something like v4l2_m2m, that device_run() is only
scheduled when both queues are ready.

I admit that this patch does not have any substantial functional
changes.

I think this patch can be discarded, as it is confusing.

Thanks a lot for the review and comments.

Regards,
Ming


>>
>> Signed-off-by: Ming Qian <ming.qian@....com>
>> ---
>>   drivers/media/platform/amphion/vdec.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
>> index 6a38a0fa0e2d..ca8f7319503a 100644
>> --- a/drivers/media/platform/amphion/vdec.c
>> +++ b/drivers/media/platform/amphion/vdec.c
>> @@ -1363,6 +1363,12 @@ static int vdec_process_output(struct vpu_inst *inst, struct vb2_buffer *vb)
>>   	if (inst->state == VPU_CODEC_STATE_STARTED)
>>   		vdec_update_state(inst, VPU_CODEC_STATE_ACTIVE, 0);
>>   
>> +	if (vdec->seq_hdr_found &&
>> +	    !vb2_start_streaming_called((v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx)))) {
>> +		vpu_trace(inst->dev, "[%d] capture is not ready, pend input frame\n", inst->id);
>> +		return -EINVAL;
> 
> I got really confused by this error return value. Its clearly not an
> error, but just a delay. I think before we add more on top, you should
> turn all these function for which the return value is unused to void.
> And use a plain "return;" here. That applies to all similar ops.
> 
>> +	}
>> +
>>   	ret = vpu_iface_get_stream_buffer_desc(inst, &desc);
>>   	if (ret)
>>   		return ret;
>> @@ -1555,6 +1561,16 @@ static int vdec_start(struct vpu_inst *inst)
>>   	return ret;
>>   }
>>   
>> +static void vdec_enqueue_pending_frames(struct vpu_inst *inst)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < v4l2_m2m_num_src_bufs_ready(inst->fh.m2m_ctx); i++) {
>> +		if (vpu_process_output_buffer(inst))
>> +			break;
> 
> How well does this work ? Previous you made good care of interleaving
> the processing output and capture, which I could imaging prevents the
> hardware from starving ?
> 
>> +	}
>> +}
>> +
>>   static int vdec_start_session(struct vpu_inst *inst, u32 type)
>>   {
>>   	struct vdec_t *vdec = inst->priv;
>> @@ -1573,10 +1589,10 @@ static int vdec_start_session(struct vpu_inst *inst, u32 type)
>>   	if (V4L2_TYPE_IS_OUTPUT(type)) {
>>   		vdec_update_state(inst, vdec->state, 1);
>>   		vdec->eos_received = 0;
>> -		vpu_process_output_buffer(inst);
>>   	} else {
>>   		vdec_cmd_start(inst);
>>   	}
>> +	vdec_enqueue_pending_frames(inst);
> 
> Was this intentional to reverse the STREAMON(CAPTURE) call flow to:
> 
>    - process_capture() (inside vdec_cmd_start())
>    - proces_output() x n
> 
> Nicolas
> 
>>   	if (inst->state == VPU_CODEC_STATE_ACTIVE)
>>   		vdec_response_fs_request(inst, false);
>>   
>