Re: [PATCH v3 2/2] media: amphion: Add a frame flush mode for decoder

[Ming Qian(OSS)]

Hi Nicolas,

On 2025/3/27 4:55, Nicolas Dufresne wrote:
> Le mercredi 05 mars 2025 à 14:26 +0800, ming.qian@xxxxxxxxxxx a écrit :
> > From: Ming Qian <ming.qian@xxxxxxxxxxx>
> >
> > 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.
>
> Ok, so in short the DISPLAY_DELAY breaks the reodering like intended,
> while this module parameter only reduce the delay. Perhaps I'll ask
> again, is is compliant or does it break some test vectors ?

In my test, it doesn't break any test vectors, the result of fluster is
same as previous, the number of passes is same as before.
There are still some fail cases of fluster, but it's related to the
decoder, and not caused by this low latency flush mode.

The purpose of this mode is only reduce decoding latency, but not to
change the decoding result.

> > Signed-off-by: Ming Qian <ming.qian@xxxxxxxxxxx>
> > ---
> > v3
> > - Improve commit message as recommended
> > - Add some comments to avoid code looks cryptic
> >
> >   drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> > index 1d9e10d9bec1..4ef9810d8142 100644
> > --- a/drivers/media/platform/amphion/vpu_malone.c
> > +++ b/drivers/media/platform/amphion/vpu_malone.c
> > @@ -25,6 +25,10 @@
> >   #include "vpu_imx8q.h"
> >   #include "vpu_malone.h"
> >   
> > +static bool frame_flush_mode;
> > +module_param(frame_flush_mode, bool, 0644);
> > +MODULE_PARM_DESC(frame_flush_mode, "Set low latency flush mode: 0 (disable) or 1 (enable)");
>
> Depending on the explanation, I may come back and suggest a different
> name for it. Meanwhile, have you consider simply "low_latency" ?

Sure, I will apply your suggestion.

> > +
> >   #define CMD_SIZE			25600
> >   #define MSG_SIZE			25600
> >   #define CODEC_SIZE			0x1000
> > @@ -1579,7 +1583,15 @@ 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)) {
> > +	/*
> > +	 * Enable the low latency flush mode if display delay is set to 0
> > +	 * or parameter frame_flush_mode is set to 1.
> > +	 * The low latency flush mode requires some padding data to be appended after each frame,
> > +	 * but don't put it in between the sequence header and frame.
> > +	 * Only H264 and HEVC decoder support this module yet,
> > +	 * for other formats, vpu_malone_add_scode() will return 0.
> > +	 */
> > +	if ((disp_imm || frame_flush_mode) && !vpu_vb_is_codecconfig(vbuf)) {
> >   		ret = vpu_malone_add_scode(inst->core->iface,
> >   					   inst->id,
> >   					   &inst->stream_buffer,
>
> In principle I'm fine with adding a module parameters, I just want to
> know more about it, perhaps we should add small hints in the
> description (or a comment in the code).
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
Thanks,
Ming