| 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
| ||
|
Message-ID: <caaa10071d063069a4f479862b38f304c89da21e.camel@ndufresne.ca>
Date: Fri, 25 Apr 2025 15:53:07 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: "Ming Qian(OSS)" <ming.qian@....nxp.com>, 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 v2] media: amphion: Add H264 and HEVC profile and level
control
Le vendredi 25 avril 2025 à 10:33 +0800, Ming Qian(OSS) a écrit :
>
> Hi Nicolas,
>
> On 2025/4/23 4:15, Nicolas Dufresne wrote:
> > Hi Ming,
> >
> > Le mardi 10 décembre 2024 à 18:33 +0900, Ming Qian a écrit :
> > > From: Ming Qian <ming.qian@....com>
> > >
> > > For format H264 and HEVC, the firmware can report the parsed profile idc
> > > and level idc to driver, the information may be useful.
> > > Implement the H264 and HEVC profile and level control to report them.
> > >
> > > Signed-off-by: Ming Qian <ming.qian@....com>
> > > ---
> > > v2
> > > -- add support for V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE
> > >
> > > drivers/media/platform/amphion/vdec.c | 61 +++++++++++++
> > > drivers/media/platform/amphion/vpu_defs.h | 1 +
> > > drivers/media/platform/amphion/vpu_helpers.c | 93 ++++++++++++++++++++
> > > drivers/media/platform/amphion/vpu_helpers.h | 5 ++
> > > drivers/media/platform/amphion/vpu_malone.c | 3 +-
> > > 5 files changed, 162 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
> > > index ca8f7319503a..61d5598ee6a1 100644
> > > --- a/drivers/media/platform/amphion/vdec.c
> > > +++ b/drivers/media/platform/amphion/vdec.c
> > > @@ -232,6 +232,37 @@ static int vdec_ctrl_init(struct vpu_inst *inst)
> > > V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
> > > 0, 1, 1, 0);
> > >
> > > + v4l2_ctrl_new_std_menu(&inst->ctrl_handler, NULL,
> > > + V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > + V4L2_MPEG_VIDEO_H264_PROFILE_MULTIVIEW_HIGH,
> > > + ~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
> >
> > You've added it here, but you will never report it, see my comment
> > below.
> Yes, you're right,
> firmware can't report the sps_constraint_set1_flag yet,
> so we need to request a firmware interface change to report the
> sps_constraint_set1_flag.
>
> If we remove PROFILE_CONSTRAINED_BASELINE here, then these streams will
> failed to play with gstreamer.
> If we keep it here, then these stremas can be played, but driver will
> report wrong profile.
>
> Maybe we need to change the firmware interface firstly.
Well, all software have bugs, even the firm ones. Just comment about
it.
Perhaps you already said, but does Amphion really support ASO/FMO ?
Since if you don't really support it, you may just drop BASELINE and
always report CONSTRAINED_BASELINE. Accuracy will come with a firmware
update.
>
> >
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED) |
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH) |
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MULTIVIEW_HIGH) |
> > > + (1 << V4L2_MPEG_VIDEO_H264_PROFILE_STEREO_HIGH)),
> >
> > Shall we advertise multiview and stereo ? My impression is that we lack
> > a mechanism to actually signal the stereo layout, or which view each
> > buffers came from. I'm thinking, you can can't test it, we should just
> > fail on these ?
> >
>
> You're right, they're not tested, I will remove them.
>
>
> > > + V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
> > > +
> > > + v4l2_ctrl_new_std_menu(&inst->ctrl_handler, NULL,
> > > + V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> > > + V4L2_MPEG_VIDEO_H264_LEVEL_6_2,
> > > + 0,
> > > + V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
> > > +
> > > + v4l2_ctrl_new_std_menu(&inst->ctrl_handler, NULL,
> > > + V4L2_CID_MPEG_VIDEO_HEVC_PROFILE,
> > > + V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10,
> > > + ~((1 << V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN) |
> > > + (1 << V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10)),
> > > + V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN);
> > > +
> > > + v4l2_ctrl_new_std_menu(&inst->ctrl_handler, NULL,
> > > + V4L2_CID_MPEG_VIDEO_HEVC_LEVEL,
> > > + V4L2_MPEG_VIDEO_HEVC_LEVEL_6_2,
> > > + 0,
> > > + V4L2_MPEG_VIDEO_HEVC_LEVEL_4);
> > > +
> > > ctrl = v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
> > > V4L2_CID_MIN_BUFFERS_FOR_CAPTURE, 1, 32, 1, 2);
> > > if (ctrl)
> > > @@ -1166,6 +1197,35 @@ static void vdec_clear_slots(struct vpu_inst *inst)
> > > }
> > > }
> > >
> > > +static void vdec_update_v4l2_ctrl(struct vpu_inst *inst, u32 id, u32 val)
> > > +{
> > > + struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&inst->ctrl_handler, id);
> > > +
> > > + if (ctrl)
> > > + v4l2_ctrl_s_ctrl(ctrl, val);
> > > +}
> > > +
> > > +static void vdec_update_v4l2_profile_level(struct vpu_inst *inst, struct vpu_dec_codec_info *hdr)
> > > +{
> > > + switch (inst->out_format.pixfmt) {
> > > + case V4L2_PIX_FMT_H264:
> > > + case V4L2_PIX_FMT_H264_MVC:
> > > + vdec_update_v4l2_ctrl(inst, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > + vpu_get_h264_v4l2_profile(hdr->profile_idc));
> > > + vdec_update_v4l2_ctrl(inst, V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> > > + vpu_get_h264_v4l2_level(hdr->level_idc));
> > > + break;
> > > + case V4L2_PIX_FMT_HEVC:
> > > + vdec_update_v4l2_ctrl(inst, V4L2_CID_MPEG_VIDEO_HEVC_PROFILE,
> > > + vpu_get_hevc_v4l2_profile(hdr->profile_idc));
> > > + vdec_update_v4l2_ctrl(inst, V4L2_CID_MPEG_VIDEO_HEVC_LEVEL,
> > > + vpu_get_hevc_v4l2_level(hdr->level_idc));
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +}
> > > +
> > > static void vdec_event_seq_hdr(struct vpu_inst *inst, struct vpu_dec_codec_info *hdr)
> > > {
> > > struct vdec_t *vdec = inst->priv;
> > > @@ -1189,6 +1249,7 @@ static void vdec_event_seq_hdr(struct vpu_inst *inst, struct vpu_dec_codec_info
> > > vdec_init_crop(inst);
> > > vdec_init_mbi(inst);
> > > vdec_init_dcp(inst);
> > > + vdec_update_v4l2_profile_level(inst, hdr);
> > > if (!vdec->seq_hdr_found) {
> > > vdec->seq_tag = vdec->codec_info.tag;
> > > if (vdec->is_source_changed) {
> > > diff --git a/drivers/media/platform/amphion/vpu_defs.h b/drivers/media/platform/amphion/vpu_defs.h
> > > index 428d988cf2f7..606f9d61a265 100644
> > > --- a/drivers/media/platform/amphion/vpu_defs.h
> > > +++ b/drivers/media/platform/amphion/vpu_defs.h
> > > @@ -134,6 +134,7 @@ struct vpu_dec_codec_info {
> > > u32 decoded_height;
> > > struct v4l2_fract frame_rate;
> > > u32 dsp_asp_ratio;
> > > + u32 profile_idc;
> > > u32 level_idc;
> > > u32 bit_depth_luma;
> > > u32 bit_depth_chroma;
> > > diff --git a/drivers/media/platform/amphion/vpu_helpers.c b/drivers/media/platform/amphion/vpu_helpers.c
> > > index d12310af9ebc..108b75ceb4ae 100644
> > > --- a/drivers/media/platform/amphion/vpu_helpers.c
> > > +++ b/drivers/media/platform/amphion/vpu_helpers.c
> > > @@ -509,3 +509,96 @@ const char *vpu_codec_state_name(enum vpu_codec_state state)
> > > }
> > > return "<unknown>";
> > > }
> > > +
> > > +struct codec_id_mapping {
> > > + u32 id;
> > > + u32 v4l2_id;
> > > +};
> > > +
> > > +static struct codec_id_mapping h264_profiles[] = {
> > > + {66, V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE},
> > > + {77, V4L2_MPEG_VIDEO_H264_PROFILE_MAIN},
> > > + {88, V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED},
> > > + {100, V4L2_MPEG_VIDEO_H264_PROFILE_HIGH},
> > > + {118, V4L2_MPEG_VIDEO_H264_PROFILE_MULTIVIEW_HIGH},
> > > + {128, V4L2_MPEG_VIDEO_H264_PROFILE_STEREO_HIGH}
> > > +};
> > > +
> > > +static struct codec_id_mapping h264_levels[] = {
> > > + {10, V4L2_MPEG_VIDEO_H264_LEVEL_1_0},
> > > + {9, V4L2_MPEG_VIDEO_H264_LEVEL_1B},
> > > + {11, V4L2_MPEG_VIDEO_H264_LEVEL_1_1},
> > > + {12, V4L2_MPEG_VIDEO_H264_LEVEL_1_2},
> > > + {13, V4L2_MPEG_VIDEO_H264_LEVEL_1_3},
> > > + {20, V4L2_MPEG_VIDEO_H264_LEVEL_2_0},
> > > + {21, V4L2_MPEG_VIDEO_H264_LEVEL_2_1},
> > > + {22, V4L2_MPEG_VIDEO_H264_LEVEL_2_2},
> > > + {30, V4L2_MPEG_VIDEO_H264_LEVEL_3_0},
> > > + {31, V4L2_MPEG_VIDEO_H264_LEVEL_3_1},
> > > + {32, V4L2_MPEG_VIDEO_H264_LEVEL_3_2},
> > > + {40, V4L2_MPEG_VIDEO_H264_LEVEL_4_0},
> > > + {41, V4L2_MPEG_VIDEO_H264_LEVEL_4_1},
> > > + {42, V4L2_MPEG_VIDEO_H264_LEVEL_4_2},
> > > + {50, V4L2_MPEG_VIDEO_H264_LEVEL_5_0},
> > > + {51, V4L2_MPEG_VIDEO_H264_LEVEL_5_1},
> > > + {52, V4L2_MPEG_VIDEO_H264_LEVEL_5_2},
> > > + {60, V4L2_MPEG_VIDEO_H264_LEVEL_6_0},
> > > + {61, V4L2_MPEG_VIDEO_H264_LEVEL_6_1},
> > > + {62, V4L2_MPEG_VIDEO_H264_LEVEL_6_2}
> > > +};
> > > +
> > > +static struct codec_id_mapping hevc_profiles[] = {
> > > + {1, V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN},
> > > + {2, V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10}
> > > +};
> > > +
> > > +static struct codec_id_mapping hevc_levels[] = {
> > > + {30, V4L2_MPEG_VIDEO_HEVC_LEVEL_1},
> > > + {60, V4L2_MPEG_VIDEO_HEVC_LEVEL_2},
> > > + {63, V4L2_MPEG_VIDEO_HEVC_LEVEL_2_1},
> > > + {90, V4L2_MPEG_VIDEO_HEVC_LEVEL_3},
> > > + {93, V4L2_MPEG_VIDEO_HEVC_LEVEL_3_1},
> > > + {120, V4L2_MPEG_VIDEO_HEVC_LEVEL_4},
> > > + {123, V4L2_MPEG_VIDEO_HEVC_LEVEL_4_1},
> > > + {150, V4L2_MPEG_VIDEO_HEVC_LEVEL_5},
> > > + {153, V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1},
> > > + {156, V4L2_MPEG_VIDEO_HEVC_LEVEL_5_2},
> > > + {180, V4L2_MPEG_VIDEO_HEVC_LEVEL_6},
> > > + {183, V4L2_MPEG_VIDEO_HEVC_LEVEL_6_1},
> > > + {186, V4L2_MPEG_VIDEO_HEVC_LEVEL_6_2}
> > > +};
> > > +
> > > +static u32 vpu_find_v4l2_id(u32 id, struct codec_id_mapping *array, u32 array_sz)
> > > +{
> > > + u32 i;
> > > +
> > > + if (!array || !array_sz)
> > > + return 0;
> > > +
> > > + for (i = 0; i < array_sz; i++) {
> > > + if (id == array[i].id)
> > > + return array[i].v4l2_id;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +u32 vpu_get_h264_v4l2_profile(u32 idc)
> > > +{
> > > + return vpu_find_v4l2_id(idc, h264_profiles, ARRAY_SIZE(h264_profiles));
> > > +}
> > > +
> > > +u32 vpu_get_h264_v4l2_level(u32 idc)
> > > +{
> > > + return vpu_find_v4l2_id(idc, h264_levels, ARRAY_SIZE(h264_levels));
> > > +}
> > > +
> > > +u32 vpu_get_hevc_v4l2_profile(u32 idc)
> > > +{
> > > + return vpu_find_v4l2_id(idc, hevc_profiles, ARRAY_SIZE(hevc_profiles));
> > > +}
> > > +
> > > +u32 vpu_get_hevc_v4l2_level(u32 idc)
> > > +{
> > > + return vpu_find_v4l2_id(idc, hevc_levels, ARRAY_SIZE(hevc_levels));
> > > +}
> > > diff --git a/drivers/media/platform/amphion/vpu_helpers.h b/drivers/media/platform/amphion/vpu_helpers.h
> > > index 0eaddb07190d..dc5fb1ca2d33 100644
> > > --- a/drivers/media/platform/amphion/vpu_helpers.h
> > > +++ b/drivers/media/platform/amphion/vpu_helpers.h
> > > @@ -70,4 +70,9 @@ int vpu_color_get_default(u32 primaries, u32 *ptransfers, u32 *pmatrix, u32 *pfu
> > >
> > > int vpu_find_dst_by_src(struct vpu_pair *pairs, u32 cnt, u32 src);
> > > int vpu_find_src_by_dst(struct vpu_pair *pairs, u32 cnt, u32 dst);
> > > +
> > > +u32 vpu_get_h264_v4l2_profile(u32 idc);
> > > +u32 vpu_get_h264_v4l2_level(u32 idc);
> > > +u32 vpu_get_hevc_v4l2_profile(u32 idc);
> > > +u32 vpu_get_hevc_v4l2_level(u32 idc);
> > > #endif
> > > diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> > > index 4769c053c6c2..5c6b2a841b6f 100644
> > > --- a/drivers/media/platform/amphion/vpu_malone.c
> > > +++ b/drivers/media/platform/amphion/vpu_malone.c
> > > @@ -889,7 +889,8 @@ static void vpu_malone_unpack_seq_hdr(struct vpu_rpc_event *pkt,
> > > info->frame_rate.numerator = 1000;
> > > info->frame_rate.denominator = pkt->data[8];
> > > info->dsp_asp_ratio = pkt->data[9];
> > > - info->level_idc = pkt->data[10];
> > > + info->profile_idc = (pkt->data[10] >> 8) & 0xff;
> >
> > The data should normally also include the sps_constraint_set1_flag,
> > which differentiate baseline from constrained-baseline. I would also be
> > very surprised if the decoders supports ASO/FMO.
> >
> > Nicolas
> >
>
> As mentioned above, I'll try to request a firmware change to report
> sps_constraint_set1_flag, then driver can report
> PROFILE_CONSTRAINED_BASELINE correctly.
Ack.
regards,
Nicolas
>
> Regards,
> Ming
>
> > > + info->level_idc = pkt->data[10] & 0xff;
> > > info->bit_depth_luma = pkt->data[13];
> > > info->bit_depth_chroma = pkt->data[14];
> > > info->chroma_fmt = pkt->data[15];
Powered by blists - more mailing lists