Hi Dafna,
On 22/10/2021 17:04, Dafna Hirschfeld wrote:
> Currently the mtk-vcodec encoder init the hardware
> upon "start_streaming" cb when both queues are streaming and turns off
> the hardware upon "stop_streaming" when both queues stop
> streaming. This is wrong since the same queue might be started and
> then stopped causing the driver to turn off the hardware without
> turning it on. This cause for example unbalanced
> calls to pm_runtime_*
I don't think this is the right fix. I think the real problem is that
vb2ops_venc_start_streaming() calls vb2_start_streaming_called() to
check that the other queue is also ready to start streaming, whereas
vb2ops_venc_stop_streaming() incorrectly calls vb2_is_streaming()
instead of vb2_start_streaming_called().
So my guess is that this mismatch is what causes the problem. Regardless,
it is definitely a bug that vb2ops_venc_stop_streaming() calls vb2_is_streaming().
I'm marking this patch as 'Changes Requested', but I'll accept the other two of
this series.
Regards,
Hans
>
> Fixes: 4e855a6efa547 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 4 ++++
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 8 ++++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> index 9d36e3d27369..84c5289f872b 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> @@ -259,6 +259,9 @@ struct vdec_pic_info {
> * @decoded_frame_cnt: number of decoded frames
> * @lock: protect variables accessed by V4L2 threads and worker thread such as
> * mtk_video_dec_buf.
> + * @stream_started: this flag is turned on when both queues (cap and out) starts streaming
> + * and it is turned off once both queues stop streaming. It is used for a correct
> + * setup and set-down of the hardware when starting and stopping streaming.
> */
> struct mtk_vcodec_ctx {
> enum mtk_instance_type type;
> @@ -301,6 +304,7 @@ struct mtk_vcodec_ctx {
>
> int decoded_frame_cnt;
> struct mutex lock;
> + bool stream_started;
>
> };
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> index 87a5114bf680..fb3cf804c96a 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> @@ -890,6 +890,9 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
> goto err_start_stream;
> }
>
> + if (ctx->stream_started)
> + return 0;
> +
> /* Do the initialization when both start_streaming have been called */
> if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> if (!vb2_start_streaming_called(&ctx->m2m_ctx->cap_q_ctx.q))
> @@ -928,6 +931,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
> ctx->state = MTK_STATE_HEADER;
> }
>
> + ctx->stream_started = true;
> return 0;
>
> err_set_param:
> @@ -1002,6 +1006,9 @@ static void vb2ops_venc_stop_streaming(struct vb2_queue *q)
> }
> }
>
> + if (!ctx->stream_started)
> + return;
> +
> if ((q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> vb2_is_streaming(&ctx->m2m_ctx->out_q_ctx.q)) ||
> (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> @@ -1023,6 +1030,7 @@ static void vb2ops_venc_stop_streaming(struct vb2_queue *q)
> mtk_v4l2_err("pm_runtime_put fail %d", ret);
>
> ctx->state = MTK_STATE_FREE;
> + ctx->stream_started = false;
> }
>
> static int vb2ops_venc_buf_out_validate(struct vb2_buffer *vb)
>
