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@xxxxxxxxxxx a écrit :
> > From: Ming Qian <ming.qian@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
Thanks,
Ming