linux-kernel - Re: [PATCH 2/3] 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] [day] [month] [year] [list]

Message-ID: <f48509bb-c945-1b0d-a429-f1a403a969b1@xs4all.nl>
Date:   Mon, 15 Nov 2021 14:45:06 +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 2/3] media: mtk-vcodec: enc: use "stream_started" flag for
 "stop/start_streaming"

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