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

From: Brian Starkey <brian.starkey@arm.com>
To: Daniel Stone <daniel@fooishbar.org>
Cc: Ben Widawsky <ben@bwidawsk.net>,
	"Kristian H . Kristensen" <hoegsberg@gmail.com>,
	Intel GFX <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/3] drm: Create a format/modifier blob
Date: Wed, 3 May 2017 15:17:55 +0100	[thread overview]
Message-ID: <20170503141755.GD20378@e106950-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAPj87rOAzN8cR154qyayPQrh_pMLQtw_b_=H+MRGeDRdyKJU6Q@mail.gmail.com>

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@arm.com> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx