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

[Daniel Stone]

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.

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.

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