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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc536f83-912a-4b9f-9523-e6cdb4463eb9@oss.nxp.com>
Date: Tue, 7 Jan 2025 13:18:15 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>, mchehab@...nel.org,
 hverkuil-cisco@...all.nl
Cc: shawnguo@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de,
 kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
 xiahong.bao@....com, eagle.zhou@....com, tao.jiang_2@....com,
 imx@...ts.linux.dev, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] media: amphion: Support display delay for Hevc format

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@....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)
>>