linux-kernel - Re: [PATCH v6 5/6] media: rkvdec: Add the rkvdec driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Mar 2020 15:30:39 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Ezequiel Garcia <ezequiel@...labora.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Rob Herring <robh+dt@...nel.org>,
        Tomasz Figa <tfiga@...omium.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>, kernel@...labora.com,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Jonas Karlman <jonas@...boo.se>,
        Heiko Stuebner <heiko@...ech.de>,
        Sakari Ailus <sakari.ailus@....fi>,
        Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH v6 5/6] media: rkvdec: Add the rkvdec driver

On Mon, 2 Mar 2020 14:57:46 +0100
Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:

> > +#define M_N(ctxidx, idc0_m, idc0_n, idc1_m, idc1_n,		\
> > +	    idc2_m, idc2_n, intra_m, intra_n)			\
> > +	[0][(ctxidx)] = {idc0_m, idc0_n},			\
> > +	[1][(ctxidx)] = {idc1_m, idc1_n},			\
> > +	[2][(ctxidx)] = {idc2_m, idc2_n},			\
> > +	[3][(ctxidx)] = {intra_m, intra_n}  
> 
> Hmm... I can't even imagine what a macro named "M_N" would do.
> Please use a better name for it.

Well, the meaning of those fields is explained in the spec, and the
name itself has been chosen so it's short enough to not have lines
exceeding 80 chars while still keeping the number of lines used for the
cabac_table[] definition acceptable. But, I'm open to any other
suggestion.

> 
> -
> 
> With regards to the macro itself, at least for my eyes, it looked bad,
> from long-term maintenance PoV, to have a first argument (ctxidx) whose
> value is just a monotonic linearly-incremented counter.

It's not, we have holes in the middle, hence the explicit indexing. I
also tried to have something as close as possible to the spec, so
people can easily see where it comes from.

> 
> I mean, the way it is, it sounds risky, as one might miss a number
> and one entire line of the array would be filled with zeros.

That's exactly why I used explicit indexing: I want specific portions
of the table to be 0-filled :-).

> 
> > +
> > +/*
> > + * Constant CABAC table.
> > + * Built from the tables described in section '9.3.1.1 Initialisation process
> > + * for context variables' of the H264 spec.
> > + */
> > +static const s8 rkvdec_h264_cabac_table[4][464][2] = {
> > +	/* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */
> > +	M_N(0, 20, -15, 20, -15, 20, -15, 20, -15),  
> 
> So, (maybe except if the ctxidx value has some real meaning),
> perhaps you could, instead, switch the array order at the tables,
> and get rid of ctxidx parameter for good, so the above code would
> be like:

I can't switch the array order since the HW expects things to be
organized this way (that table is directly copied to a memory region
that's passed to the HW).

> 
> #define INIT_MN_PAIRS(idc0_m, idc0_n, idc1_m, idc1_n,	\
> 	       idc2_m, idc2_n, intra_m, intra_n)	\
> 	{						\
> 		[0] = {idc0_m, idc0_n},			\
> 		[1] = {idc1_m, idc1_n},			\
> 		[2] = {idc2_m, idc2_n},			\
> 		[3] = {intra_m, intra_n}		\
> 	},
> 
> static const s8 rkvdec_h264_cabac_table[464][4][2] = {
> 	/* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */
> 	INIT_MN_PAIRS(20, -15, 20, -15, 20, -15, 20, -15),
> 	...