On 10/11/19 1:15 PM, Boris Brezillon wrote:
> Hi Hans,
>
> On Fri, 11 Oct 2019 12:06:35 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
>>> diff --git a/drivers/staging/media/rockchip/Kconfig b/drivers/staging/media/rockchip/Kconfig
>>> new file mode 100644
>>> index 000000000000..8c617ae2c84f
>>> --- /dev/null
>>> +++ b/drivers/staging/media/rockchip/Kconfig
>>> @@ -0,0 +1,16 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +config VIDEO_ROCKCHIP
>>> + bool "Rockchip Video Devices"
>>> + depends on ARCH_ROCKCHIP || COMPILE_TEST
>>> + help
>>> + If you have a Rockchip SoC based embedding a video codec.
>>> +
>>> + Note that this option doesn't include new drivers in the
>>> + kernel: saying N will just cause Kconfig to skip all the
>>> + questions about Allwinner media devices.
>>> +
>>> +if VIDEO_ROCKCHIP
>>> +
>>> +source "drivers/staging/media/rockchip/vdec/Kconfig"
>>> +
>>> +endif
>>
>> Is this really necessary? I think the 'source' line can just be added to
>> drivers/staging/media/Kconfig. This config option here is rather vague.
>
> I based it on the Allwinner/Cedrus model (even left an 'Allwinner' in
> the description :)), but I can definitely move the source line in
> drivers/staging/media/Kconfig or even get rid of the rockchip dir if
> you prefer.
>
>>
>>> diff --git a/drivers/staging/media/rockchip/Makefile b/drivers/staging/media/rockchip/Makefile
>>> new file mode 100644
>>> index 000000000000..b3705b51f1b9
>>> --- /dev/null
>>> +++ b/drivers/staging/media/rockchip/Makefile
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +obj-$(CONFIG_VIDEO_ROCKCHIP_VDEC) += vdec/
>>> diff --git a/drivers/staging/media/rockchip/vdec/Kconfig b/drivers/staging/media/rockchip/vdec/Kconfig
>>> new file mode 100644
>>> index 000000000000..329b4a813c47
>>> --- /dev/null
>>> +++ b/drivers/staging/media/rockchip/vdec/Kconfig
>>> @@ -0,0 +1,14 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +config VIDEO_ROCKCHIP_VDEC
>>> + tristate "Rockchip Video Decoder driver"
>>> + depends on ARCH_ROCKCHIP || COMPILE_TEST
>>> + depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER
>>> + depends on MEDIA_CONTROLLER_REQUEST_API
>>> + select VIDEOBUF2_DMA_CONTIG
>>> + select VIDEOBUF2_VMALLOC
>>> + select V4L2_MEM2MEM_DEV
>>> + help
>>> + Support for the Rockchip Video Decoder IP present on Rockchip SoC,
>>> + which accelerates video decoding.
>>> + To compile this driver as a module, choose M here: the module
>>> + will be called hantro-vpu.
>>
>> hantro-vpu? Not rkvdec?
>
> Should be rkvdec, indeed. That's what happens when you copy things from
> an existing driver :-).
>
>
>>> +
>>> +/* Constant CABAC table. */
>>> +static const u32 rkvdec_h264_cabac_table[] = {
>>
>> Where does this table come from? It needs some provenance.
>
> Chromeos kernels [1] and MPP code base [2]. I'll add a comment pointing
> to those trees.
>
> [...]
>
>>> +
>>> +struct rkvdec_h264_reflist_builder {
>>> + const struct v4l2_h264_dpb_entry *dpb;
>>> + s32 pocs[RKVDEC_H264_DPB_SIZE];
>>> + u8 unordered_reflist[RKVDEC_H264_DPB_SIZE];
>>> + int frame_nums[RKVDEC_H264_DPB_SIZE];
>>> + s32 curpoc;
>>> + u8 num_valid;
>>> +};
>>
>> What's a 'poc'? Perhaps this can use some comments.
>
> Picture Order Count. I'll add comment.
>
>>
>> It looks like code is copied from hantro_h264.c. Is there anything that
>> can be reasonably shared between the two drivers?
>
> I was planning on exporting those helpers at some point, but I'd like
> the reflist creation logic to settle before doing (we still need to fix
> things for interlaced streams, which is what Jonas is working on).
>
>
>
>>> +
>>> +static int p_ref_list_cmp(const void *ptra, const void *ptrb, const void *data)
>>> +{
>>> + const struct rkvdec_h264_reflist_builder *builder = data;
>>> + const struct v4l2_h264_dpb_entry *a, *b;
>>> + u8 idxa, idxb;
>>> +
>>> + idxa = *((u8 *)ptra);
>>> + idxb = *((u8 *)ptrb);
>>> + a = &builder->dpb[idxa];
>>> + b = &builder->dpb[idxb];
>>> +
>>> + if ((a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) !=
>>> + (b->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)) {
>>> + /* Short term pics firt. */
>>
>> firt -> first
>
> Will fix the typo.
>
>>
>>> + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
>>> + return -1;
>>> + else
>>> + return 1;
>>> + }
>>> +
>>> + /*
>>> + * Short term pics in descending pic num order, long term ones in
>>> + * ascending order.
>>> + */
>>> + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)) {
>>> + int frame_num_a, frame_num_b;
>>> +
>>> + frame_num_a = builder->frame_nums[idxa];
>>> + frame_num_b = builder->frame_nums[idxb];
>>> + return frame_num_b > frame_num_a ? 1 : -1;
>>> + }
>>> +
>>> + return a->pic_num > b->pic_num ? 1 : -1;
>>> +}
>>> +
>>> +static int b0_ref_list_cmp(const void *ptra, const void *ptrb, const void *data)
>>> +{
>>> + const struct rkvdec_h264_reflist_builder *builder = data;
>>> + const struct v4l2_h264_dpb_entry *a, *b;
>>> + s32 poca, pocb;
>>> + u8 idxa, idxb;
>>> +
>>> + idxa = *((u8 *)ptra);
>>> + idxb = *((u8 *)ptrb);
>>> + a = &builder->dpb[idxa];
>>> + b = &builder->dpb[idxb];
>>> +
>>> + if ((a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) !=
>>> + (b->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)) {
>>> + /* Short term pics firt. */
>>
>> Ditto. Check the code for this typo. It's in the hantro code as well.
>
> Will do.
>
>>
>>> + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
>>> + return -1;
>>> + else
>>> + return 1;
>>> + }
>>> +
>>> + /* Long term pics in ascending pic num order. */
>>> + if (a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
>>> + return a->pic_num > b->pic_num ? 1 : -1;
>>> +
>>> + poca = builder->pocs[idxa];
>>> + pocb = builder->pocs[idxb];
>>> +
>>> + /*
>>> + * Short term pics with POC < cur POC first in POC descending order
>>> + * followed by short term pics with POC > cur POC in POC ascending
>>> + * order.
>>> + */
>>> + if ((poca < builder->curpoc) != (pocb < builder->curpoc))
>>> + return poca > pocb ? 1 : -1;
>>> + else if (poca < builder->curpoc)
>>> + return pocb > poca ? 1 : -1;
>>> +
>>> + return poca > pocb ? 1 : -1;
>>> +}
>
>>> +static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, struct v4l2_format *f,
>>> + u32 fourcc)
>>> +{
>>> + memset(f, 0, sizeof(*f));
>>> + f->fmt.pix_mp.pixelformat = fourcc;
>>> + f->fmt.pix_mp.field = V4L2_FIELD_NONE;
>>> + f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_JPEG,
>>
>> Don't use this colorspace. I see it is also used in the hantro driver, that should
>> be corrected as well.
>>
>> The colorimetry comes from the stream metadata, and for a stateless driver I
>> assume that it is userspace that parses that.
>>
>> V4L2_COLORSPACE_JPEG should only be used by (M)JPEG codecs. And even there is it
>> dubious. See section 2.17.10 in the spec for more info.
>
> What should I use when the formats are reset then?
Depends on the resolution: SDTV should use V4L2_COLORSPACE_SMPTE170M, others REC709.
If you don't know the resolution here, then stick to REC709.
Regards,
Hans
>
> Thanks for the prompt review!
>
> Boris
>
> [1]https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-4.4/drivers/media/platform/rockchip-vpu/rk3399_vdec_hw_h264d.c#45
> [2]https://github.com/rockchip-linux/mpp/blob/release/mpp/hal/rkdec/h264d/hal_h264d_rkv_reg.c#L67
>
