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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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

> On Mon, 2 Mar 2020 14:57:46 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.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