Re: [PATCH v7 07/11] media: rkvdec: Add the VP9 backend

[Andrzej Pietrasiewicz]

Hi Alex,

W dniu 20.10.2021 o 01:24, Alex Bee pisze:
> Hi Andrzej,
>
> Am 29.09.21 um 18:04 schrieb Andrzej Pietrasiewicz:
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >
> > The Rockchip VDEC supports VP9 profile 0 up to 4096x2304@30fps. Add
> > a backend for this new format.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu@xxxxxxxxxxxxx>
> > Co-developed-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
> > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
> > ---
> >   drivers/staging/media/rkvdec/Kconfig      |    1 +
> >   drivers/staging/media/rkvdec/Makefile     |    2 +-
> >   drivers/staging/media/rkvdec/rkvdec-vp9.c | 1078 +++++++++++++++++++++
> >   drivers/staging/media/rkvdec/rkvdec.c     |   52 +-
> >   drivers/staging/media/rkvdec/rkvdec.h     |   12 +-
> >   5 files changed, 1137 insertions(+), 8 deletions(-)
> >   create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c

<snip>

> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c 
> > b/drivers/staging/media/rkvdec/rkvdec.c
> > index 7131156c1f2c..6aa8aca66547 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -99,10 +99,30 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
> >       .num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
> >   };
> > -static const u32 rkvdec_h264_decoded_fmts[] = {
> > +static const u32 rkvdec_h264_vp9_decoded_fmts[] = {
> >       V4L2_PIX_FMT_NV12,
>
> For H.264 rkvdec HW supports additional formats: V4L2_PIX_FMT_NV15, 
> V4L2_PIX_FMT_NV16 and V4L2_PIX_FMT_NV20. Not all of those are upstreamed yet and 
> thus not supported by rkvdec driver - but I think we should introduce a seperate 
> rkvdec_vp9_decoded_fmts already a this point. (To avoid unnecessary diff 
> afterwards)

I will do it if I get to re-spinning the series for other reasons.

> >   };
> > +static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
> > +    {
> > +        .cfg.id = V4L2_CID_STATELESS_VP9_FRAME,
> > +    },
> > +    {
> > +        .cfg.id = V4L2_CID_STATELESS_VP9_COMPRESSED_HDR,
> > +    },
> > +    {
> > +        .cfg.id = V4L2_CID_MPEG_VIDEO_VP9_PROFILE,
> > +        .cfg.min = V4L2_MPEG_VIDEO_VP9_PROFILE_0,
> > +        .cfg.max = V4L2_MPEG_VIDEO_VP9_PROFILE_0,
> > +        .cfg.def = V4L2_MPEG_VIDEO_VP9_PROFILE_0,
> > +    },
> > +};
> > +
> > +static const struct rkvdec_ctrls rkvdec_vp9_ctrls = {
> > +    .ctrls = rkvdec_vp9_ctrl_descs,
> > +    .num_ctrls = ARRAY_SIZE(rkvdec_vp9_ctrl_descs),
> > +};
> > +
> >   static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
> >       {
> >           .fourcc = V4L2_PIX_FMT_H264_SLICE,
> > @@ -116,8 +136,23 @@ static const struct rkvdec_coded_fmt_desc 
> > rkvdec_coded_fmts[] = {
> >           },
> >           .ctrls = &rkvdec_h264_ctrls,
> >           .ops = &rkvdec_h264_fmt_ops,
> > -        .num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
> > -        .decoded_fmts = rkvdec_h264_decoded_fmts,
> > +        .num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
> > +        .decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
> > +    },
> > +    {
> > +        .fourcc = V4L2_PIX_FMT_VP9_FRAME,
> > +        .frmsize = {
> > +            .min_width = 64,
> > +            .max_width = 4096,
> > +            .step_width = 64,
> > +            .min_height = 64,
> > +            .max_height = 2304,
> > +            .step_height = 64,
> > +        },
> I checked (available) documentation and couldn't find any hint to the 
> .step_width and .step_height, but I'm not sure that's correct: taking
> this values here neither framesize of 3840x2160 nor 1280x720 would be possible - 
> but the HW seems to have no problem with those, i.e. decoding works fine.
> Given the output format is the same as the (only) currently supported H.264 
> output format (NV12) and those steps are usually for alignment purposes need by 
> the HW , I strongly guess .step_height and .step_width are the same as 
> V4L2_PIX_FMT_H264_SLICE has.

Aren't these used primarily by v4l2_apply_frmsize_constraints()? Doesn't
this merely mean that even though userspace requests, say, 48x48,
it will get 64x64 instead?

I tried decoding a 720p video with gstreamer and it worked fine
(I got a properly sized 1280x720 output).

Regards,

Andrzej