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

[Ming Qian(OSS)]

Hi Sebastian ,

On 2025/3/27 15:48, Sebastian Fricke wrote:
> Hey Ming,
>
> On 05.03.2025 14:26, ming.qian@xxxxxxxxxxx wrote:
> > 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.
> >
> > 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)");
> > +
> > #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.
>
> s/or parameter frame_flush_mode is set to 1./
>    or the frame flush mode if it is set to 1./
I will apply your suggestion.

> > +     * The low latency flush mode requires some padding data to be 
> > appended after each frame,
>
> s/appended after each/appended to each/
> (the word append implies that something is added after something else)
Got it
> > +     * but don't put it in between the sequence header and frame.
>
> s/but don't put it in between the sequence header and frame./
>    but there must not be any padding data between the sequence header 
> and the frame./
>
> (As this is not a suggestion for the developer but a description of what
> the code does)
Got it
> > +     * Only H264 and HEVC decoder support this module yet,
>
> s/decoder/formats/
>
> I'd rewrite this part:
> This module is currently only supported for the H264 and HEVC formats,
>
> but that is only because this sounds more natural to me.
I will apply your suggestion.

> > +     * 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,
> > --
> > 2.43.0-rc1
>
> Thank you!
>
> Regards,
> Sebastian Fricke

Thank you very much for your help in expressing.

Regards,
Ming