Em Mon, 2 Mar 2020 15:30:39 +0100
Boris Brezillon [off-list ref] escreveu:

On Mon, 2 Mar 2020 14:57:46 +0100
Mauro Carvalho Chehab [off-list ref] wrote:

quoted

quoted

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

quoted

-

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.

quoted

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.

quoted

quoted

+
+/*
+ * 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).

quoted

#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