Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 May 2023 15:26:17 +0200
From:   Tommaso Merciai <tomm.merciai@gmail.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     jacopo.mondi@ideasonboard.com, laurent.pinchart@ideasonboard.com,
        martin.hecht@avnet.eu, linuxfancy@googlegroups.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Nicholas Roth <nicholas@rothemail.net>,
        Shawn Tu <shawnx.tu@intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Benjamin Mugnier <benjamin.mugnier@foss.st.com>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera
Message-ID: <ZHSn+UJt+3DQ6KNc@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
References: <20230526173955.797226-1-tomm.merciai@gmail.com>
 <20230526173955.797226-3-tomm.merciai@gmail.com>
 <8563d09d-fa63-43e3-98a9-8008d53034aa@wanadoo.fr>
 <ZHR5qup6412nLgzz@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
 <7aaa6abc-09a7-7be5-9184-6cd7d702baf2@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7aaa6abc-09a7-7be5-9184-6cd7d702baf2@wanadoo.fr>
Precedence: bulk

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