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
