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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux