Hi Hans,
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> On 11/07/18 00:27, Mani, Rajmohan wrote:
> > Hi Mauro,
> >
> > Thanks for the reviews.
> >
> >> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> >>
> >> Hi Mauro,
> >>
> >> On Fri, Nov 2, 2018 at 10:49 PM Mauro Carvalho Chehab
> >> <mchehab+sams...@kernel.org> wrote:
> >>>
> >>> Em Mon, 29 Oct 2018 15:22:57 -0700
> >>> Yong Zhi <yong....@intel.com> escreveu:
> >> [snip]
> >>>> +struct ipu3_uapi_awb_config_s {
> >>>> + __u16 rgbs_thr_gr;
> >>>> + __u16 rgbs_thr_r;
> >>>> + __u16 rgbs_thr_gb;
> >>>> + __u16 rgbs_thr_b;
> >>>> + struct ipu3_uapi_grid_config grid; }
> >>>> +__attribute__((aligned(32))) __packed;
> >>>
> >>> Hmm... Kernel defines a macro for aligned attribute:
> >>>
> >>> include/linux/compiler_types.h:#define __aligned(x)
> >> __attribute__((aligned(x)))
> >>>
> >>
> >> First, thanks for review!
> >>
> >> Maybe I missed something, but last time I checked, it wasn't
> >> accessible from UAPI headers in userspace.
> >
> > Ack. We see that's still the case.
> >
> >>
> >>> I'm not a gcc expert, but it sounds weird to first ask it to align
> >>> with 32 bits and then have __packed (with means that pads should be
> >>> removed).
> >>>
> >>> In other words, I *guess* is it should either be __packed or
> >>> __aligned(32).
> >>>
> >>> Not that it would do any difference, in practice, as this specific
> >>> struct has a size with is multiple of 32 bits, but let's do the
> >>> right annotation here, not mixing two incompatible alignment
> requirements.
> >>>
> >>
> >> My understanding was that __packed makes the compiler not insert any
> >> alignment between particular fields of the struct, while __aligned
> >> makes the whole struct be aligned at given boundary, if placed in
> >> another struct. If I didn't miss anything, having both should make perfect
> sense here.
> >
> > Ack
> >
> > I also recall that as part of addressing review comments (from Hans
> > and Sakari), on earlier versions of this patch series, we added
> > __packed attribute to all structs to ensure the size of the structs remains
> > the
> same between 32 and 64 bit builds.
> >
> > The addition of structure members of the name padding[x] in some of
> > the structs ensures that respective members are aligned at 32 byte
> > boundaries, while the overall size of the structs remain the same between 32
> and 64 bit builds.
>
> I recommend that this is documented in the header. It's not a common
> construction so an explanation will help.
Ack.
>
> Regards,
>
> Hans
>
> >
> > Thanks
> > Raj
> >
> >>
> >> Best regards,
> >> Tomasz
