[PATCH v2 1/2] media: amphion: Reduce decoding latency for HEVC decoder

[PATCH v2 1/2] media: amphion: Reduce decoding latency for HEVC decoder

[Ming Qian]

The amphion decoder firmware supports low latency flush mode for
HEVC format since v1.9.0, it can help reduce the decoding latency by
appending some padding data after every frame, then driver can enable
this feature when the display delay 0 is enabled.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
v2
- Improve commit message
- Add firmware version check

 drivers/media/platform/amphion/vpu_malone.c | 22 ++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
index 5c6b2a841b6f..1d9e10d9bec1 100644
--- a/drivers/media/platform/amphion/vpu_malone.c
+++ b/drivers/media/platform/amphion/vpu_malone.c
@@ -68,6 +68,9 @@
 
 #define MALONE_DEC_FMT_RV_MASK			BIT(21)
 
+#define MALONE_VERSION_MASK			0xFFFFF
+#define MALONE_MIN_VERSION_HEVC_BUFFLUSH	(((1 << 16) | (9 << 8) | 0) & MALONE_VERSION_MASK)
+
 enum vpu_malone_stream_input_mode {
 	INVALID_MODE = 0,
 	FRAME_LVL,
@@ -332,6 +335,8 @@ struct vpu_dec_ctrl {
 	u32 buf_addr[VID_API_NUM_STREAMS];
 };
 
+static const struct malone_padding_scode *get_padding_scode(u32 type, u32 fmt);
+
 u32 vpu_malone_get_data_size(void)
 {
 	return sizeof(struct vpu_dec_ctrl);
@@ -654,9 +659,16 @@ static int vpu_malone_set_params(struct vpu_shared_addr *shared,
 		hc->jpg[instance].jpg_mjpeg_interlaced = 0;
 	}
 
-	hc->codec_param[instance].disp_imm = params->display_delay_enable ? 1 : 0;
-	if (malone_format != MALONE_FMT_AVC)
+	if (params->display_delay_enable &&
+	    get_padding_scode(SCODE_PADDING_BUFFLUSH, params->codec_format))
+		hc->codec_param[instance].disp_imm = 1;
+	else
 		hc->codec_param[instance].disp_imm = 0;
+
+	if (params->codec_format == V4L2_PIX_FMT_HEVC &&
+	    (iface->fw_version & MALONE_VERSION_MASK) < MALONE_MIN_VERSION_HEVC_BUFFLUSH)
+		hc->codec_param[instance].disp_imm = 0;
+
 	hc->codec_param[instance].dbglog_enable = 0;
 	iface->dbglog_desc.level = 0;
 
@@ -1024,6 +1036,7 @@ static const struct malone_padding_scode padding_scodes[] = {
 	{SCODE_PADDING_EOS,      V4L2_PIX_FMT_JPEG,        {0x0, 0x0}},
 	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264,        {0x15010000, 0x0}},
 	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264_MVC,    {0x15010000, 0x0}},
+	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_HEVC,        {0x3e010000, 0x20}},
 };
 
 static const struct malone_padding_scode padding_scode_dft = {0x0, 0x0};
@@ -1058,8 +1071,11 @@ static int vpu_malone_add_padding_scode(struct vpu_buffer *stream_buffer,
 	int ret;
 
 	ps = get_padding_scode(scode_type, pixelformat);
-	if (!ps)
+	if (!ps) {
+		if (scode_type == SCODE_PADDING_BUFFLUSH)
+			return 0;
 		return -EINVAL;
+	}
 
 	wptr = readl(&str_buf->wptr);
 	if (wptr < stream_buffer->phys || wptr > stream_buffer->phys + stream_buffer->length)
-- 
2.43.0-rc1

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

[Ming Qian]

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(-)

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,
-- 
2.43.0-rc1

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

[Sebastian Fricke]

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

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

[Ming Qian(OSS)]

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

Re: [PATCH v2 1/2] media: amphion: Reduce decoding latency for HEVC decoder

[Sebastian Fricke]

Hey Ming,

thank you for the patch!

On 17.01.2025 16:57, Ming Qian wrote:
>The amphion decoder firmware supports low latency flush mode for
>HEVC format since v1.9.0, it can help reduce the decoding latency by
>appending some padding data after every frame, then driver can enable
>this feature when the display delay 0 is enabled.

I see that you already changed the commit message for this version, but
I still have a few recommendations for the description:

     The amphion decoder firmware supports a low latency flush mode for
     the HEVC format since v1.9.0. This feature, which is enabled when
     the display delay is set to 0, can help to reduce the decoding
     latency by appending some padding data to every frame.

The rest looks good to me.

Regards,
Sebastian

>
>Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>---
>v2
>- Improve commit message
>- Add firmware version check
>
> drivers/media/platform/amphion/vpu_malone.c | 22 ++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
>index 5c6b2a841b6f..1d9e10d9bec1 100644
>--- a/drivers/media/platform/amphion/vpu_malone.c
>+++ b/drivers/media/platform/amphion/vpu_malone.c
>@@ -68,6 +68,9 @@
>
> #define MALONE_DEC_FMT_RV_MASK			BIT(21)
>
>+#define MALONE_VERSION_MASK			0xFFFFF
>+#define MALONE_MIN_VERSION_HEVC_BUFFLUSH	(((1 << 16) | (9 << 8) | 0) & MALONE_VERSION_MASK)
>+
> enum vpu_malone_stream_input_mode {
> 	INVALID_MODE = 0,
> 	FRAME_LVL,
>@@ -332,6 +335,8 @@ struct vpu_dec_ctrl {
> 	u32 buf_addr[VID_API_NUM_STREAMS];
> };
>
>+static const struct malone_padding_scode *get_padding_scode(u32 type, u32 fmt);
>+
> u32 vpu_malone_get_data_size(void)
> {
> 	return sizeof(struct vpu_dec_ctrl);
>@@ -654,9 +659,16 @@ static int vpu_malone_set_params(struct vpu_shared_addr *shared,
> 		hc->jpg[instance].jpg_mjpeg_interlaced = 0;
> 	}
>
>-	hc->codec_param[instance].disp_imm = params->display_delay_enable ? 1 : 0;
>-	if (malone_format != MALONE_FMT_AVC)
>+	if (params->display_delay_enable &&
>+	    get_padding_scode(SCODE_PADDING_BUFFLUSH, params->codec_format))
>+		hc->codec_param[instance].disp_imm = 1;
>+	else
> 		hc->codec_param[instance].disp_imm = 0;
>+
>+	if (params->codec_format == V4L2_PIX_FMT_HEVC &&
>+	    (iface->fw_version & MALONE_VERSION_MASK) < MALONE_MIN_VERSION_HEVC_BUFFLUSH)
>+		hc->codec_param[instance].disp_imm = 0;
>+
> 	hc->codec_param[instance].dbglog_enable = 0;
> 	iface->dbglog_desc.level = 0;
>
>@@ -1024,6 +1036,7 @@ static const struct malone_padding_scode padding_scodes[] = {
> 	{SCODE_PADDING_EOS,      V4L2_PIX_FMT_JPEG,        {0x0, 0x0}},
> 	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264,        {0x15010000, 0x0}},
> 	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264_MVC,    {0x15010000, 0x0}},
>+	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_HEVC,        {0x3e010000, 0x20}},
> };
>
> static const struct malone_padding_scode padding_scode_dft = {0x0, 0x0};
>@@ -1058,8 +1071,11 @@ static int vpu_malone_add_padding_scode(struct vpu_buffer *stream_buffer,
> 	int ret;
>
> 	ps = get_padding_scode(scode_type, pixelformat);
>-	if (!ps)
>+	if (!ps) {
>+		if (scode_type == SCODE_PADDING_BUFFLUSH)
>+			return 0;
> 		return -EINVAL;
>+	}
>
> 	wptr = readl(&str_buf->wptr);
> 	if (wptr < stream_buffer->phys || wptr > stream_buffer->phys + stream_buffer->length)
>-- 
>2.43.0-rc1
>
>

Re: [PATCH v2 1/2] media: amphion: Reduce decoding latency for HEVC decoder

[Ming Qian(OSS)]

Hi Sebastian,

On 2025/2/26 17:45, 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,
> 
> thank you for the patch!
> 
> On 17.01.2025 16:57, Ming Qian wrote:
>> The amphion decoder firmware supports low latency flush mode for
>> HEVC format since v1.9.0, it can help reduce the decoding latency by
>> appending some padding data after every frame, then driver can enable
>> this feature when the display delay 0 is enabled.
> 
> I see that you already changed the commit message for this version, but
> I still have a few recommendations for the description:
> 
>      The amphion decoder firmware supports a low latency flush mode for
>      the HEVC format since v1.9.0. This feature, which is enabled when
>      the display delay is set to 0, can help to reduce the decoding
>      latency by appending some padding data to every frame.
> 
> The rest looks good to me.

Thanks for your feedback, I'm preparing the v3 patch, and it will follow
your advice.

Thanks,
Ming

> 
> Regards,
> Sebastian
> 
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> v2
>> - Improve commit message
>> - Add firmware version check
>>
>> drivers/media/platform/amphion/vpu_malone.c | 22 ++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/amphion/vpu_malone.c 
>> b/drivers/media/platform/amphion/vpu_malone.c
>> index 5c6b2a841b6f..1d9e10d9bec1 100644
>> --- a/drivers/media/platform/amphion/vpu_malone.c
>> +++ b/drivers/media/platform/amphion/vpu_malone.c
>> @@ -68,6 +68,9 @@
>>
>> #define MALONE_DEC_FMT_RV_MASK                        BIT(21)
>>
>> +#define MALONE_VERSION_MASK                   0xFFFFF
>> +#define MALONE_MIN_VERSION_HEVC_BUFFLUSH      (((1 << 16) | (9 << 8) 
>> | 0) & MALONE_VERSION_MASK)
>> +
>> enum vpu_malone_stream_input_mode {
>>       INVALID_MODE = 0,
>>       FRAME_LVL,
>> @@ -332,6 +335,8 @@ struct vpu_dec_ctrl {
>>       u32 buf_addr[VID_API_NUM_STREAMS];
>> };
>>
>> +static const struct malone_padding_scode *get_padding_scode(u32 type, 
>> u32 fmt);
>> +
>> u32 vpu_malone_get_data_size(void)
>> {
>>       return sizeof(struct vpu_dec_ctrl);
>> @@ -654,9 +659,16 @@ static int vpu_malone_set_params(struct 
>> vpu_shared_addr *shared,
>>               hc->jpg[instance].jpg_mjpeg_interlaced = 0;
>>       }
>>
>> -      hc->codec_param[instance].disp_imm = 
>> params->display_delay_enable ? 1 : 0;
>> -      if (malone_format != MALONE_FMT_AVC)
>> +      if (params->display_delay_enable &&
>> +          get_padding_scode(SCODE_PADDING_BUFFLUSH, 
>> params->codec_format))
>> +              hc->codec_param[instance].disp_imm = 1;
>> +      else
>>               hc->codec_param[instance].disp_imm = 0;
>> +
>> +      if (params->codec_format == V4L2_PIX_FMT_HEVC &&
>> +          (iface->fw_version & MALONE_VERSION_MASK) < 
>> MALONE_MIN_VERSION_HEVC_BUFFLUSH)
>> +              hc->codec_param[instance].disp_imm = 0;
>> +
>>       hc->codec_param[instance].dbglog_enable = 0;
>>       iface->dbglog_desc.level = 0;
>>
>> @@ -1024,6 +1036,7 @@ static const struct malone_padding_scode 
>> padding_scodes[] = {
>>       {SCODE_PADDING_EOS,      V4L2_PIX_FMT_JPEG,        {0x0, 0x0}},
>>       {SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264,        {0x15010000, 
>> 0x0}},
>>       {SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264_MVC,    {0x15010000, 
>> 0x0}},
>> +      {SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_HEVC,        {0x3e010000, 
>> 0x20}},
>> };
>>
>> static const struct malone_padding_scode padding_scode_dft = {0x0, 0x0};
>> @@ -1058,8 +1071,11 @@ static int vpu_malone_add_padding_scode(struct 
>> vpu_buffer *stream_buffer,
>>       int ret;
>>
>>       ps = get_padding_scode(scode_type, pixelformat);
>> -      if (!ps)
>> +      if (!ps) {
>> +              if (scode_type == SCODE_PADDING_BUFFLUSH)
>> +                      return 0;
>>               return -EINVAL;
>> +      }
>>
>>       wptr = readl(&str_buf->wptr);
>>       if (wptr < stream_buffer->phys || wptr > stream_buffer->phys + 
>> stream_buffer->length)
>> -- 
>> 2.43.0-rc1
>>
>>