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

[Boris Brezillon]

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