RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI

From: "Mani, Rajmohan" <rajmohan.mani@intel.com>
To: Tomasz Figa <tfiga@chromium.org>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Cc: "Zhi, Yong" <yong.zhi@intel.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	"Zheng, Jian Xu" <jian.xu.zheng@intel.com>,
	"Hu, Jerry W" <jerry.w.hu@intel.com>,
	"Toivonen, Tuukka" <tuukka.toivonen@intel.com>,
	"Qiu, Tian Shu" <tian.shu.qiu@intel.com>,
	"Cao, Bingbu" <bingbu.cao@intel.com>,
	"Li, Chao C" <chao.c.li@intel.com>
Subject: RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Date: Tue, 6 Nov 2018 23:27:53 +0000	[thread overview]
Message-ID: <6F87890CF0F5204F892DEA1EF0D77A5981524580@fmsmsx122.amr.corp.intel.com> (raw)
In-Reply-To: <CAAFQd5B_OVV-Nh0uOGHdQE4eSKcs5N8Nn1t-Zz-GbvgpB9P38A@mail.gmail.com>

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+samsung@kernel.org> wrote:
> >
> > Em Mon, 29 Oct 2018 15:22:57 -0700
> > Yong Zhi <yong.zhi@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.

Thanks
Raj

> 
> Best regards,
> Tomasz