[PATCH 1/2] media: amphion: Support display delay for Hevc format

[PATCH 1/2] media: amphion: Support display delay for Hevc format

[Ming Qian]

The amphion decoder firmware v1.9.0 supports display delay 0 for
hevc format, then driver can enable this feature.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
index 5c6b2a841b6f..8f4aa48b2d65 100644
--- a/drivers/media/platform/amphion/vpu_malone.c
+++ b/drivers/media/platform/amphion/vpu_malone.c
@@ -332,6 +332,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,8 +656,10 @@ 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;
 	hc->codec_param[instance].dbglog_enable = 0;
 	iface->dbglog_desc.level = 0;
@@ -1024,6 +1028,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 +1063,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 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 8f4aa48b2d65..3750a2618b0e 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
@@ -1571,7 +1574,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 1/2] media: amphion: Support display delay for Hevc format

[Nicolas Dufresne]

Hi,

nit: use capital HEVC in the subject

Le jeudi 19 décembre 2024 à 10:51 +0900, Ming Qian a écrit :
> The amphion decoder firmware v1.9.0 supports display delay 0 for
> hevc format, then driver can enable this feature.

nit: HEVC

I think this added "feature" hides a bug you haven't fixed in this patch.


        v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
                          V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
                          0, 1, 1, 0);

With the control registered this way, 0 is the default, and the range of 0-1.
But from your commit message, this is only supported from firmware 1.9.0 and up.
I think the patch should basically adjust the min and def values according to
the detected firmware version.

This might actually be more complex, aka per CODEC, and for that you may want to
use v4l2_ctrl_config structure.

Nicolas


> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
>  drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> index 5c6b2a841b6f..8f4aa48b2d65 100644
> --- a/drivers/media/platform/amphion/vpu_malone.c
> +++ b/drivers/media/platform/amphion/vpu_malone.c
> @@ -332,6 +332,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,8 +656,10 @@ 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;
>  	hc->codec_param[instance].dbglog_enable = 0;
>  	iface->dbglog_desc.level = 0;
> @@ -1024,6 +1028,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 +1063,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)

Re: [PATCH 1/2] media: amphion: Support display delay for Hevc format

[Ming Qian(OSS)]

Hi Nicolas,


On 2025/1/7 6:16, Nicolas Dufresne wrote:
> Hi,
> 
> nit: use capital HEVC in the subject
> 

I'll fix it in the v2 patch

> Le jeudi 19 décembre 2024 à 10:51 +0900, Ming Qian a écrit :
>> The amphion decoder firmware v1.9.0 supports display delay 0 for
>> hevc format, then driver can enable this feature.
> 
> nit: HEVC
> 
> I think this added "feature" hides a bug you haven't fixed in this patch.
> 
> 
>          v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
>                            V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
>                            0, 1, 1, 0);
> 
> With the control registered this way, 0 is the default, and the range of 0-1.
> But from your commit message, this is only supported from firmware 1.9.0 and up.
> I think the patch should basically adjust the min and def values according to
> the detected firmware version.
> 
> This might actually be more complex, aka per CODEC, and for that you may want to
> use v4l2_ctrl_config structure.
> 
> Nicolas
> 

Thanks for the tip.
By the way, how to define different ctrl values for each CODEC format?
Is it reasonable to new a ctrl after set capture format?
Or can we change the min/max value after set capture format?

Thanks,
Ming

> 
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>>   drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
>> index 5c6b2a841b6f..8f4aa48b2d65 100644
>> --- a/drivers/media/platform/amphion/vpu_malone.c
>> +++ b/drivers/media/platform/amphion/vpu_malone.c
>> @@ -332,6 +332,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,8 +656,10 @@ 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;
>>   	hc->codec_param[instance].dbglog_enable = 0;
>>   	iface->dbglog_desc.level = 0;
>> @@ -1024,6 +1028,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 +1063,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)
> 

Re: [PATCH 1/2] media: amphion: Support display delay for Hevc format

[Ming Qian(OSS)]

Hi Nicolas,

On 2025/1/7 9:29, Ming Qian(OSS) wrote:
> Hi Nicolas,
> 
> 
> On 2025/1/7 6:16, Nicolas Dufresne wrote:
>> Hi,
>>
>> nit: use capital HEVC in the subject
>>
> 
> I'll fix it in the v2 patch
> 
>> Le jeudi 19 décembre 2024 à 10:51 +0900, Ming Qian a écrit :
>>> The amphion decoder firmware v1.9.0 supports display delay 0 for
>>> hevc format, then driver can enable this feature.
>>
>> nit: HEVC
>>
>> I think this added "feature" hides a bug you haven't fixed in this patch.
>>
>>
>>          v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
>>                            V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
>>                            0, 1, 1, 0);
>>
>> With the control registered this way, 0 is the default, and the range 
>> of 0-1.
>> But from your commit message, this is only supported from firmware 
>> 1.9.0 and up.
>> I think the patch should basically adjust the min and def values 
>> according to
>> the detected firmware version.
>>
>> This might actually be more complex, aka per CODEC, and for that you 
>> may want to
>> use v4l2_ctrl_config structure.
>>
>> Nicolas
>>
> 
> Thanks for the tip.
> By the way, how to define different ctrl values for each CODEC format?
> Is it reasonable to new a ctrl after set capture format?
> Or can we change the min/max value after set capture format?
> 
> Thanks,
> Ming

I checked the driver again, and I think there is no issue with ctrl
V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
it has supported to display frame immediately after it's decoded for all
supported CODEC formats.

But there is a issue with amphion vpu, the decoder will pre-parse 3
frames before decoding the first frame, in other words, the delay
between frame input and frame decoding is relatively large, then
firmware difined a low-latency flush mode, that adding some flush
padding data after every frame, then decoder can support to input 1
frame and decode 1 frame, the decoding latency can be reduced.
Only H264 decoder support this format currently, but since v1.9.0,
it can support HEVC format too.

I think my commit message is not accurate. I'll improve the commit
message in V2 patch.

Thanks,
Ming

> 
>>
>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>> ---
>>>   drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/amphion/vpu_malone.c 
>>> b/drivers/media/platform/amphion/vpu_malone.c
>>> index 5c6b2a841b6f..8f4aa48b2d65 100644
>>> --- a/drivers/media/platform/amphion/vpu_malone.c
>>> +++ b/drivers/media/platform/amphion/vpu_malone.c
>>> @@ -332,6 +332,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,8 +656,10 @@ 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;
>>>       hc->codec_param[instance].dbglog_enable = 0;
>>>       iface->dbglog_desc.level = 0;
>>> @@ -1024,6 +1028,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 +1063,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)
>>

Re: [PATCH 1/2] media: amphion: Support display delay for Hevc format

[Nicolas Dufresne]

Le mardi 07 janvier 2025 à 09:29 +0800, Ming Qian(OSS) a écrit :
> Hi Nicolas,
> 
> 
> On 2025/1/7 6:16, Nicolas Dufresne wrote:
> > Hi,
> > 
> > nit: use capital HEVC in the subject
> > 
> 
> I'll fix it in the v2 patch
> 
> > Le jeudi 19 décembre 2024 à 10:51 +0900, Ming Qian a écrit :
> > > The amphion decoder firmware v1.9.0 supports display delay 0 for
> > > hevc format, then driver can enable this feature.
> > 
> > nit: HEVC
> > 
> > I think this added "feature" hides a bug you haven't fixed in this patch.
> > 
> > 
> >          v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
> >                            V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
> >                            0, 1, 1, 0);
> > 
> > With the control registered this way, 0 is the default, and the range of 0-1.
> > But from your commit message, this is only supported from firmware 1.9.0 and up.
> > I think the patch should basically adjust the min and def values according to
> > the detected firmware version.
> > 
> > This might actually be more complex, aka per CODEC, and for that you may want to
> > use v4l2_ctrl_config structure.
> > 
> > Nicolas
> > 
> 
> Thanks for the tip.
> By the way, how to define different ctrl values for each CODEC format?
> Is it reasonable to new a ctrl after set capture format?
> Or can we change the min/max value after set capture format?

Very good question, I clearly didn't think through my reply. Seems fine to leave
it that way. Per-codec, would mean having multiple CID, which is not your fault
it has been combined.

Nicolas

> 
> Thanks,
> Ming
> 
> > 
> > > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > > ---
> > >   drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> > > index 5c6b2a841b6f..8f4aa48b2d65 100644
> > > --- a/drivers/media/platform/amphion/vpu_malone.c
> > > +++ b/drivers/media/platform/amphion/vpu_malone.c
> > > @@ -332,6 +332,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,8 +656,10 @@ 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;
> > >   	hc->codec_param[instance].dbglog_enable = 0;
> > >   	iface->dbglog_desc.level = 0;
> > > @@ -1024,6 +1028,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 +1063,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)
> >