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

[Daniel Stone]

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.

> 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?

Cheers,
Daniel