Hi Brian, On 3 May 2017 at 13:51, Brian Starkey <brian.starkey@xxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
