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

[Tommaso Merciai]

Hi Cristophe,

On Mon, May 29, 2023 at 02:34:09PM +0200, Christophe JAILLET wrote:
> 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.

Mb, got it now.
Thanks for the explanation! :)

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

Thanks for the explanation.
I will keep in mind this! :)

Regards,
Tommaso

> 
> CJ