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@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
