Hi Daniel,
On Wed, May 03, 2017 at 03:07:40PM +0100, Daniel Stone wrote:
Hi Brian,
On 3 May 2017 at 15:03, Brian Starkey <brian.starkey@xxxxxxx> wrote:
On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
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.
Indeed that's the case, for most ioctls at least. A comment would
definitely be in order.
No need for a comment if the code is fixed to do the right thing
irrespective of the value of sizeof(drm_formats_modifier_blob) - which
IMO is mandatory.
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.
All this is correct - the __packed declaration is unnecessary, and so
is the rounding up when calculating the size. And with that fixed, I
believe it should be correct, no?
Yes, with those problems fixed it looks correct to me.
Thanks,
-Brian
Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
