[Intel-gfx] [PATCH 2/3] drm: Create a format/modifier blob

[Brian Starkey]

Hi Daniel,

On Wed, May 03, 2017 at 03:07:40PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 15:03, Brian Starkey <brian.starkey at arm.com> wrote:
>> On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
>>> formats_offset is the end of the fixed-size element, which is
>>> currently aligned to 32 bytes, and practically speaking would always
>>> have to be anyway. As it is an array of uint32_t, this gives natural
>>> alignment.
>>
>> Why must it always be? The __packed attribute means it'll have no
>> padding - so any u16 or u8 added to the end will break it - putting
>> the formats array on a non-aligned boundary.
>>
>> If the assumption is that the struct will always be made of only
>> u32/u64 members (and the implementation will break otherwise) then
>> there had better be a really big comment saying so, and preferably a
>> compile-time assertion too.
>
>Indeed that's the case, for most ioctls at least. A comment would
>definitely be in order.
>

No need for a comment if the code is fixed to do the right thing
irrespective of the value of sizeof(drm_formats_modifier_blob) - which
IMO is mandatory.

>> I'm missing the reason for it being __packed in the first place -
>> perhaps that's just a left over and needs to be removed.
>>
>> Either way, this line aligns to 8:
>>
>> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>>
>> ...and the rest of the blob_size calculation looks like it assumes the
>> formats array starts at that 8-byte boundary. So, for clarity and
>> consistency I reckon the blob_size code and the code that builds the
>> blob should do the same thing.
>
>All this is correct - the __packed declaration is unnecessary, and so
>is the rounding up when calculating the size. And with that fixed, I
>believe it should be correct, no?

Yes, with those problems fixed it looks correct to me.

Thanks,
-Brian

>
>Cheers,
>Daniel