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

[Ming Qian(OSS)]

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@xxxxxxx>
> > ---
> >   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);
> >