Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 29/05/2023 à 12:08, Tommaso Merciai a écrit :

>>> +	int avail_fmt_cnt = 0;
>>> +
>>> +	alvium->alvium_csi2_fmt = NULL;
>>> +
>>> +	/* calculate fmt array size */
>>> +	for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
>>> +		if (alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit]) {
>>> +			if (!alvium_csi2_fmts[fmt].is_raw) {
>>> +				sz++;
>>> +			} else if (alvium_csi2_fmts[fmt].is_raw &&
>>> +			      alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
>>
>> It is makes sense, this if/else looks equivalent to:
>>
>> 			if (!alvium_csi2_fmts[fmt].is_raw ||
>> 			    alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
>> 				sz++;
>>
>> The same simplification could also be applied in the for loop below.
> 
> I Don't agree on this.
> This is not a semplification.
> This change the logic of the statement.
> 

Hmmm, unless I missed something obvious, I don't think it changes the logic.

Let me detail my PoV.

The original code is, for the inner if:

+	if (!alvium_csi2_fmts[fmt].is_raw) {
+		sz++;
+	} else if (alvium_csi2_fmts[fmt].is_raw &&
+	      alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
+		sz++;
+	}

So both branchs end to sz++, so it can be written:

+	if ((!alvium_csi2_fmts[fmt].is_raw) ||
+	    (alvium_csi2_fmts[fmt].is_raw &&
+	      alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) {
+		sz++;
+	}

A || (!A && B) is equivalent to A || B, so it can be rewritten as:

+	if ((!alvium_csi2_fmts[fmt].is_raw) ||
+	    (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) {
+		sz++;
+	}

> Also initialization of sz and avail_fmt_cnt is needed.
> Maybe I can include fmt variable initialization into the for loop:

Agreed. I must have been unclear.
I was only speaking about the *inner* 'if', not the whole block of code.

> 
> for (int fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
> 
> But from my perspective this seems clear.

I fully agree with you, but that was not my point.


>>> +struct alvium_pixfmt {
>>> +	u8 id;
>>> +	u32 code;
>>> +	u32 colorspace;
>>> +	u8 fmt_av_bit;
>>> +	u8 bay_av_bit;
>>> +	u64 mipi_fmt_regval;
>>> +	u64 bay_fmt_regval;
>>> +	u8 is_raw;
>>
>> If moved a few lines above, there would be less holes in the struct.
> 
> ?

This is more or less the same comment that you got from Laurent Pinchart.

Using pahole on your struct, as-is, gives:

struct alvium_pixfmt {
	u8                         id;                   /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	u32                        code;                 /*     4     4 */
	u32                        colorspace;           /*     8     4 */
	u8                         fmt_av_bit;           /*    12     1 */
	u8                         bay_av_bit;           /*    13     1 */

	/* XXX 2 bytes hole, try to pack */

	u64                        mipi_fmt_regval;      /*    16     8 */
	u64                        bay_fmt_regval;       /*    24     8 */
	u8                         is_raw;               /*    32     1 */

	/* size: 40, cachelines: 1, members: 8 */
	/* sum members: 28, holes: 2, sum holes: 5 */
	/* padding: 7 */
	/* last cacheline: 40 bytes */
};

If you change the order of some fields, you can get, *for example*:

struct alvium_pixfmt {
	u8                         id;                   /*     0     1 */
	u8                         is_raw;               /*     1     1 */
	u8                         fmt_av_bit;           /*     2     1 */
	u8                         bay_av_bit;           /*     3     1 */
	u32                        code;                 /*     4     4 */
	u32                        colorspace;           /*     8     4 */

	/* XXX 4 bytes hole, try to pack */

	u64                        mipi_fmt_regval;      /*    16     8 */
	u64                        bay_fmt_regval;       /*    24     8 */

	/* size: 32, cachelines: 1, members: 8 */
	/* sum members: 28, holes: 1, sum holes: 4 */
	/* last cacheline: 32 bytes */
};

Now the whole structure require 32 bytes instead of 40, because their 
are less holes in it.
And "rounding" in the memory allocation layer would finally allocate 64 
bytes. So moving some fields would half (32 vs 64) the memory used!

But, the main point is to keep a layout that is logical to you. So up to 
you to re-arrange the struct or not.

(the same applies to some other struct in your patch)

CJ