linux-kernel - Re: [PATCH v2 2/2] media: amphion: Add a frame flush mode for decoder

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: <20250224141712.soexl43hrimwf236@basti-XPS-13-9310>
Date: Mon, 24 Feb 2025 15:17:12 +0100
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Ming Qian <ming.qian@....nxp.com>
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl, nicolas@...fresne.ca,
	shawnguo@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de,
	kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
	xiahong.bao@....com, eagle.zhou@....com, tao.jiang_2@....com,
	imx@...ts.linux.dev, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] media: amphion: Add a frame flush mode for decoder

Hey Ming,

On 17.01.2025 16:57, Ming Qian wrote:
>The amphion decoder will pre-parse 3 frames before decoding the first
>frame. If we append a flush padding data after frame, the decoder
>will finish parsing and start to decode when the flush data is parsed.
>It can reduce the decoding latency.
>In the past, we only enable this mode when the display delay is set to
>0. But we still can enable this mode without changing the display order,
>so we add a frame_flush_mode parameter to enable it.

My recommendation:

By default the amphion decoder will pre-parse 3 frames before starting
to decode the first frame. Alternatively, a block of flush padding data
can be appended to the frame, which will ensure that the decoder can
start decoding immediately after parsing the flush padding data, thus
potentially reducing decoding latency.
This mode was previously only enabled, when the display delay was set to
0. Allow the user to manually toggle the use of that mode via a module
parameter called frame_flush_mode, which enables the mode without
changing the display order.

Which fixes a few grammatical issues and tries to be a bit more clear.
But please confirm to me that I hit your intended meaning.

More comments below ...

>
>Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>---
> drivers/media/platform/amphion/vpu_malone.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
>index 1d9e10d9bec1..f07660dc3b07 100644
>--- a/drivers/media/platform/amphion/vpu_malone.c
>+++ b/drivers/media/platform/amphion/vpu_malone.c
>@@ -25,6 +25,9 @@
> #include "vpu_imx8q.h"
> #include "vpu_malone.h"
>
>+static bool frame_flush_mode;
>+module_param(frame_flush_mode, bool, 0644);

Could you add a comment here that makes clear to the reader briefly what
the expected behavior of frame_flush_mode = 0 and frame_flush_mode = 1
is?

>+
> #define CMD_SIZE			25600
> #define MSG_SIZE			25600
> #define CODEC_SIZE			0x1000
>@@ -1579,7 +1582,7 @@ static int vpu_malone_input_frame_data(struct vpu_malone_str_buffer __iomem *str
>
> 	vpu_malone_update_wptr(str_buf, wptr);
>
>-	if (disp_imm && !vpu_vb_is_codecconfig(vbuf)) {
>+	if ((disp_imm || frame_flush_mode) && !vpu_vb_is_codecconfig(vbuf)) {

So you say that the mode was enabled with display delay set to 0,
meaning (disp_imm = 1) == (display delay = 0), right? E.g. disp_imm
means display_immediately I guess.

I think this all deserves a lot better documentation, otherwise the code
becomes quite cryptic. Could you add a comment before this line, which
explains the entry conditions disp_imm & frame_flush_mode and the
codeconfig thing and that explains briefly what kind of mode we are
entering here?

> 		ret = vpu_malone_add_scode(inst->core->iface,
> 					   inst->id,
> 					   &inst->stream_buffer,
>-- 
>2.43.0-rc1
>
>

Regards,
Sebastian Fricke