diff mbox series

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

Message ID 20250305062630.2329032-2-ming.qian@oss.nxp.com (mailing list archive)
State New
Headers show
Series [v3,1/2] media: amphion: Reduce decoding latency for HEVC decoder | expand

Commit Message

Ming Qian(OSS) March 5, 2025, 6:26 a.m. UTC 
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(-)

Comments

Nicolas Dufresne March 26, 2025, 8:55 p.m. UTC | #1 
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>
Ming Qian(OSS) March 27, 2025, 1:41 a.m. UTC | #2 
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
Sebastian Fricke March 27, 2025, 7:48 a.m. UTC | #3 
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
Ming Qian(OSS) March 27, 2025, 8:07 a.m. UTC | #4 
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
diff mbox series

Patch

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,