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]

Message-ID: <20200302155312.62185b98@coco.lan>
Date:   Mon, 2 Mar 2020 15:53:12 +0100
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Boris Brezillon <boris.brezillon@...labora.com>
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

Em Mon, 2 Mar 2020 15:30:39 +0100
Boris Brezillon <boris.brezillon@...labora.com> escreveu:

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

Well, code reviewers may not have the specs on their hands when
reviewing patches :-)

Keep 80 columns is something we desire, but not at the expense of
making the code harder to maintain or understand. Yet, I suspect
that increasing the name by a few extra bytes will still allow it to
sit at the 80 columns space[1].

[1] This macro passes 9 parameters. If each parameter consumes 4 chars,
    and they're preceded by a tab, that would mean 44 columns.

Perhaps something like CABAC_ENTRY or even MN_VALUES would be better.

> 
> > 
> > -
> > 
> > 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 :-).

Ah, OK! Implementation makes sense then.
> 
> >   
> > > +
> > > +/*
> > > + * 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),
> > 	...  
> 


Thanks,
Mauro