linux-kernel - Re: [PATCH v2 3/7] media: mtk-vcodec: enc: use "stream_started" flag for "stop/start

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <759d6336-f63a-cd9c-80bf-666b47bb9cf7@xs4all.nl>
Date:   Thu, 25 Nov 2021 09:30:31 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Dafna Hirschfeld <dafna.hirschfeld@...labora.com>,
        linux-media@...r.kernel.org
Cc:     kernel@...labora.com, acourbot@...omium.org,
        andrew-ct.chen@...iatek.com, courbot@...omium.org,
        dafna3@...il.com, eizan@...omium.org, houlong.wei@...iatek.com,
        hsinyi@...omium.org, irui.wang@...iatek.com,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        maoguang.meng@...iatek.com, matthias.bgg@...il.com,
        mchehab@...nel.org, minghsiu.tsai@...iatek.com, tfiga@...omium.org,
        tiffany.lin@...iatek.com
Subject: Re: [PATCH v2 3/7] media: mtk-vcodec: enc: use "stream_started" flag
 for "stop/start_streaming"

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@...labora.com>
> ---
> 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)
>