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

[Brian Starkey]

On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 13:51, Brian Starkey <brian.starkey at arm.com> wrote:
>> On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>>> +       modifiers_size =
>>> +               sizeof(struct drm_format_modifier) *
>>> format_modifier_count;
>>> +
>>> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>>> +       blob_size += ALIGN(formats_size, 8);
>>> +       blob_size += modifiers_size;
>>> +
>>> +       blob = drm_property_create_blob(dev, blob_size, NULL);
>>> +       if (IS_ERR(blob))
>>> +               return -1;
>>> +
>>> +       blob_data = (struct drm_format_modifier_blob *)blob->data;
>>> +       blob_data->version = FORMAT_BLOB_CURRENT;
>>> +       blob_data->count_formats = format_count;
>>> +       blob_data->formats_offset = sizeof(struct
>>> drm_format_modifier_blob);
>>
>> This looks to be missing some alignment.
>>
>> Definitely needs to be at least to 4 bytes, but given you aligned
>> it up to 8 for the initial "blob_size" I assume the intention was to
>> put the formats on the next 8-byte aligned address after the end of
>> the struct, e.g.:
>>
>>         blob_data->formats_offset = ALIGN(sizeof(struct
>> drm_format_modifier_blob), 8);
>
>It's fairly subtle, but I think it's correct.

It's the exact subtlety that I was concerned about.

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

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.

Cheers,
-Brian

>
>If we have an odd number of formats supported, the formats[] array
>will end on a 4-byte rather than 8-byte boundary, so the ALIGN() on
>formats_size guarantees that modifiers_offset will be aligned to an
>8-byte quantity, which is required as it has 64-bit elements.
>
>The size of a pointer is not relevant since we're not passing pointers
>across the kernel/userspace boundary, just offsets within a struct.
>The alignment of those offsets has to correspond to the types located
>at those offsets, i.e. 4 bytes for formats (guaranteed by fixed header
>layout), and 8 bytes for modifiers (guaranteed by explicit alignment).
>
>Cheers,
>Daniel