linux-kernel - Re: [PATCH v6 3/3] media: mediatek: vcodec: add description for vsi struct

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241211171052.wl4wqoka7yyeso2l@basti-XPS-13-9310>
Date: Wed, 11 Dec 2024 18:10:52 +0100
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Yunfei Dong <yunfei.dong@...iatek.com>
Cc: Nícolas F . R . A . Prado <nfraprado@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Benjamin Gaignard <benjamin.gaignard@...labora.com>,
	Nathan Hebert <nhebert@...omium.org>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Hsin-Yi Wang <hsinyi@...omium.org>,
	Fritz Koenig <frkoenig@...omium.org>,
	Daniel Vetter <daniel@...ll.ch>, Steve Cho <stevecho@...omium.org>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v6 3/3] media: mediatek: vcodec: add description for vsi
 struct

Hey Yunfei,

On 16.11.2024 11:16, Yunfei Dong wrote:
>If the video shared information (vsi) is changed accidentally,

How can that struct be changed accidentally?

>will leading to play h264 bitstream fail if the firmware won't
>be changed at the same time.

Okay I guess you mean that the struct has to be memcpy'd to the firmware
to synchronize it right?
Also is this really just a H264 thing? I would imagine that incorrect
data in the firmware will cause issues no matter which codec.

>Marking the shared struct with "shared interface with firmware".

Can we do anything more to ensure that the firmware doesn't fall out of
sync besides adding a comment to the description?

To fix grammatical issues the description above should be changed to:

The vsi (video shared information) struct needs to be synchronized
between the firmware and the host, as a change that is only done in the
host version of the struct but isn't synchronized to the firmware can
lead to decoding issues with H264 bitstreams. Highlight this requirement
within the struct descriptions.

But as highlighted above it is not clear to me whether the content of
this message is right yet.

>
>Signed-off-by: Yunfei Dong <yunfei.dong@...iatek.com>
>Reviewed-by: Chen-Yu Tsai <wenst@...omium.org>
>Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>---
> .../mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c    | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
>index a7de95b9a7c0..5a202691e209 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
>@@ -30,6 +30,7 @@ enum vdec_h264_core_dec_err_type {
>
> /**
>  * struct vdec_h264_slice_lat_dec_param  - parameters for decode current frame
>+ *        (shared interface with firmware)
>  *
>  * @sps:		h264 sps syntax parameters
>  * @pps:		h264 pps syntax parameters
>@@ -48,7 +49,7 @@ struct vdec_h264_slice_lat_dec_param {
> };
>
> /**
>- * struct vdec_h264_slice_info - decode information
>+ * struct vdec_h264_slice_info - decode information (shared interface with firmware)
>  *
>  * @nal_info:		nal info of current picture
>  * @timeout:		Decode timeout: 1 timeout, 0 no timeout
>@@ -72,7 +73,7 @@ struct vdec_h264_slice_info {
>
> /**
>  * struct vdec_h264_slice_vsi - shared memory for decode information exchange
>- *        between SCP and Host.
>+ *        between SCP and Host (shared interface with firmware).

In this case, I feel like the previous description made the fact, that
this is shared data between the host and the firmware, rather clear
already.

>  *
>  * @wdma_err_addr:		wdma error dma address
>  * @wdma_start_addr:		wdma start dma address
>-- 
>2.46.0
>
>
Regards,
Sebastian Fricke