Re: [PATCH 2/3] drm: Create a format/modifier blob

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux