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

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

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

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.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
v3
- Improve commit message as recommended
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 v3 2/2] media: amphion: Add a frame flush mode for decoder

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

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@oss.nxp.com>
---
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.
+	 * 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,
-- 
2.43.0-rc1

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

[Nicolas Dufresne]

Hi,

Le mercredi 05 mars 2025 à 14:26 +0800, ming.qian@oss.nxp.com a écrit :
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> 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.

Just curiosity, does it stay spec compliant or not ? Perhaps share some
compliance (fluster) results ?

> 
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
> v3
> - Improve commit message as recommended
> 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)

Just a suggestion, could also use an inline function.

#define MALONE_VERSION(maj, min, inc)   (((maj) << 16) | ((min) << 16) | (inc)) & MALONE_VERSION_MASK)
#define CHECK_VERSION(iface, maj, min)	((iface->fw_version & MALONE_VERSION_MASK) >= MALONE_VERSION(maj, min, 0))

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

So if could be:
	    !CHECK_VERSION(iface, 1, 9)

There might be even better ways, this is not a hard request from me
though.

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

With or without the adjustments.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com

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

[Nicolas Dufresne]

Le mercredi 05 mars 2025 à 14:26 +0800, ming.qian@oss.nxp.com a écrit :
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> 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 ? 

> 
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
> 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" ?

> +
>  #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@collabora.com>

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

[Ming Qian(OSS)]

Hi Nicolas,

On 2025/3/27 4:48, Nicolas Dufresne wrote:
> Hi,
> 
> Le mercredi 05 mars 2025 à 14:26 +0800, ming.qian@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> 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.
> 
> Just curiosity, does it stay spec compliant or not ? Perhaps share some
> compliance (fluster) results ?
> 

I don't think this will affect spec compliant, and the v4l2-compliance
results are all pass:
Total for amphion-vpu device /dev/video0: 48, Succeeded: 48, Failed: 0, 
Warnings: 0
Total for amphion-vpu device /dev/video1: 48, Succeeded: 48, Failed: 0, 
Warnings: 0

And the result of fluster is same as previous. The number of passes is
the same as before.

>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> v3
>> - Improve commit message as recommended
>> 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)
> 
> Just a suggestion, could also use an inline function.
> 
> #define MALONE_VERSION(maj, min, inc)   (((maj) << 16) | ((min) << 16) | (inc)) & MALONE_VERSION_MASK)
> #define CHECK_VERSION(iface, maj, min)	((iface->fw_version & MALONE_VERSION_MASK) >= MALONE_VERSION(maj, min, 0))
> 
>> +
>>   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)
> 
> So if could be:
> 	    !CHECK_VERSION(iface, 1, 9)
> 
> There might be even better ways, this is not a hard request from me
> though.
> 
Good suggestion, I'll apply it.

>> +		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)
> 
> With or without the adjustments.
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com
> 
Thanks,
Ming

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@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> 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@oss.nxp.com>
>> ---
>> 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@collabora.com>
> 
Thanks,
Ming

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

[Sebastian Fricke]

Hey Ming,

On 05.03.2025 14:26, ming.qian@oss.nxp.com wrote:
>From: Ming Qian <ming.qian@oss.nxp.com>
>
>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@oss.nxp.com>
>---
>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./

>+	 * 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)

>+	 * 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)

>+	 * 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.

>+	 * 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

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@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> 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@oss.nxp.com>
>> ---
>> 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