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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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 ?

> 
> Signed-off-by: Ming Qian <ming.qian@nxp.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);
>