Hi Dafna,
On 17/11/2021 14:06, Dafna Hirschfeld wrote:
> Currently the mtk-vcodec encoder does runtime pm resume
> in "start_streaming" cb if both queues are streaming
> and does runtime pm 'put' in "stop_streaming" if both
> queues are not 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_*
>
> Fixes: 4e855a6efa547 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> to reproduce the issue:
> patch v4l-utils as follow:
>
> static void stateful_m2m(cv4l_fd &fd, cv4l_queue &in, cv4l_queue &out,
>
> if (fd.streamon(out.g_type()))
> return;
> + if (fd.streamoff(out.g_type()))
> + return;
> +
> + exit(1);
>
> fd.s_trace(0);
> if (exp_fd_p)
>
> and call:
> v4l2-ctl -x width=160,height=128,pixelformat=NM12 -v pixelformat=VP80 --stream-mmap --stream-out-mmap -d5
> then the file /sys/devices/platform/soc/19002000.vcodec/power/runtime_usage
> will show a negative number and further streaming (with e.g, gstreamer) is not possible.
>
> 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 &&
(copy-and-pasted from my reply to the same patch in the v1 series, you probably
missed that reply...)
I don't think this patch 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 patches
this series (with patch 4/7 fixed to take care of the kernel test robot report).
Regards,
Hans
> @@ -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)
>
