Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Boris Brezillon <boris.brezillon@collabora.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	linux-media@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Tomasz Figa <tfiga@chromium.org>,
	Nicolas Dufresne <nicolas@ndufresne.ca>,
	kernel@collabora.com,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	linux-rockchip@lists.infradead.org,
	Heiko Stuebner <heiko@sntech.de>
Subject: Re: [RESEND PATCH v2 3/4] media: rockchip: Add the rkvdec driver
Date: Fri, 11 Oct 2019 13:15:40 +0200
Message-ID: <20191011131540.002a61a9@dhcp-172-31-174-146.wireless.concordia.ca> (raw)
In-Reply-To: <8d3abbd9-f27d-f17f-8298-b0cc7c63ab3e@xs4all.nl>

Hi Hans,

On Fri, 11 Oct 2019 12:06:35 +0200
Hans Verkuil <hverkuil@xs4all.nl> 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

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-11  9:33 [RESEND PATCH v2 0/4] " Boris Brezillon
2019-10-11  9:33 ` [RESEND PATCH v2 1/4] media: vb2: Add a helper to get the vb2 buffer attached to a request Boris Brezillon
2019-10-11  9:33 ` [RESEND PATCH v2 2/4] media: dt-bindings: rockchip: Document RK3399 Video Decoder bindings Boris Brezillon
2019-10-11 12:50   ` Rob Herring
2019-10-11 12:56     ` Boris Brezillon
2019-10-11 17:37       ` Rob Herring
2019-10-11  9:33 ` [RESEND PATCH v2 3/4] media: rockchip: Add the rkvdec driver Boris Brezillon
2019-10-11 10:06   ` Hans Verkuil
2019-10-11 11:15     ` Boris Brezillon [this message]
2019-10-11 11:21       ` Hans Verkuil
2019-10-13 19:08   ` kbuild test robot
2019-10-13 19:08   ` [RFC PATCH] media: rockchip: rkvdec_queue_ops can be static kbuild test robot
2019-10-11  9:33 ` [RESEND PATCH v2 4/4] arm64: dts: rockchip: rk3399: Define the rockchip Video Decoder node Boris Brezillon
2019-10-11  9:43 ` [RESEND PATCH v2 0/4] media: rockchip: Add the rkvdec driver Hans Verkuil
2019-10-11  9:53   ` Boris Brezillon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191011131540.002a61a9@dhcp-172-31-174-146.wireless.concordia.ca \
    --to=boris.brezillon@collabora.com \
    --cc=devicetree@vger.kernel.org \
    --cc=ezequiel@collabora.com \
    --cc=hans.verkuil@cisco.com \
    --cc=heiko@sntech.de \
    --cc=hverkuil@xs4all.nl \
    --cc=jonas@kwiboo.se \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=nicolas@ndufresne.ca \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@iki.fi \
    --cc=tfiga@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git