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

[Andrzej Pietrasiewicz <andrzej.p@collabora.com>]

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@collabora.com>
>>
>> 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@collabora.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> Co-developed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> ---
>>   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