| Message ID | 20250117075720.4018076-2-ming.qian@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] media: amphion: Reduce decoding latency for HEVC decoder | expand |
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@oss.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
Hi sebastian, On 2025/2/24 22:17, Sebastian Fricke wrote: > [You don't often get email from sebastian.fricke@collabora.com. Learn > why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > 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 ... > Yes, you're right, and your description looks better, I will follow it. >> >> Signed-off-by: Ming Qian <ming.qian@oss.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? > OK, I'll add a comment to describe this mode. >> + >> #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? > Sure，I'll add comments to explain the code. I'm sorry fot the confusion. Thank you very much for your feedback, I will try to fix them in v3 patch. Thanks, Ming >> ret = vpu_malone_add_scode(inst->core->iface, >> inst->id, >> &inst->stream_buffer, >> -- >> 2.43.0-rc1 >> >> > > Regards, > Sebastian Fricke
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); + #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)) { ret = vpu_malone_add_scode(inst->core->iface, inst->id, &inst->stream_buffer,
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. Signed-off-by: Ming Qian <ming.qian@oss.nxp.com> --- drivers/media/platform/amphion/vpu_malone.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)