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+samsung@xxxxxxxxxx> wrote:
>>>
>>> Em Mon, 29 Oct 2018 15:22:57 -0700
>>> Yong Zhi <yong.zhi@xxxxxxxxx> 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.
Regards,
Hans
>
> Thanks
> Raj
>
>>
>> Best regards,
>> Tomasz
