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?
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
