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

From: Ben Widawsky <ben@bwidawsk.net>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Liviu Dudau <liviu@dudau.co.uk>,
	Intel GFX <intel-gfx@lists.freedesktop.org>,
	"Kristian H . Kristensen" <hoegsberg@gmail.com>,
	Daniel Stone <daniels@collabora.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/4] drm: Create a format/modifier blob
Date: Thu, 20 Jul 2017 21:20:32 -0700	[thread overview]
Message-ID: <20170721042032.GA15342@mail.bwidawsk.net> (raw)
In-Reply-To: <20170714191015.GS12629@intel.com>

On 17-07-14 22:10:15, Ville Syrjälä wrote:
>On Fri, Jul 14, 2017 at 11:41:49AM -0700, Ben Widawsky wrote:
>> On 17-06-29 22:49:44, Ville Syrjälä wrote:
>>
>> [snip]
>>
>> >
>> >... but here it's ALIGN(formats_offset+formats_size). I think we should
>> >be aligning the same thing in both cases, or we add a BUILD_BUG_ON to
>> >make sure the header size always stays a multiple of 8 bytes.
>> >
>> >That's mainly because the design of the structure seems geared towards
>> >expanding the header in the future (as in why else would you have the
>> >offsets?).
>> >
>>
>> I guess I don't quite understand what you're asking for. The first thing is
>> determining a size, the second is finding an offset into the blob.
>
>If I remember my thinking correctly, then what I was trying to say was
>
>ALIGN(multiple_of_4, 8) + ALIGN(multiple_of_4, 8) != ALIGN(multiple_of_4 + multiple_of_4, 8)
>
>but the code was assuming that it's true, or that at least one of those
>things is a multiple of 8 already.
>

Going with this and calling it sufficient.

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0da6707a8cf7..5c14beee52ff 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -96,6 +96,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
        /* Modifiers offset is a pointer to a struct with a 64 bit field so it
         * should be naturally aligned to 8B.
         */
+       BUILD_BUG_ON(sizeof(struct drm_format_modifier_blob) % 8);
        blob_size += ALIGN(formats_size, 8);
        blob_size += modifiers_size;



>> I don't mind changing this, but tell me what you want.
>>
>> BUILD_BUG_ON sounds good to me regardless.
>>
>> [snip]
>>
>> --
>> Ben Widawsky, Intel Open Source Technology Center
>
>-- 
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
