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>,
	Daniel Stone <daniels@collabora.com>
Subject: Re: [PATCH 2/3] drm: Create a format/modifier blob
Date: Wed, 3 May 2017 13:39:47 +0100	[thread overview]
Message-ID: <20170503123946.GA20378@e106950-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAPj87rP=_S2GeD=ye6otCQBdGzeuw2qjtCjFvBA+gzaLdJkjgg@mail.gmail.com>

Hi Daniel,

On Wed, May 03, 2017 at 12:47:18PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 12:00, Brian Starkey <brian.starkey@arm.com> wrote:
>> Having just caught up on IRC I'm sure I'm far too late for this party,
>> but...
>>
>> Wouldn't this whole thing be a whole lot simpler if "IN_FORMATS"
>> stored "pointers" to separate blobs for the format and modifier lists?
>>
>> I've used/written a few interfaces with offsets to
>> variable-length-arrays and IMO the code to handle them is always
>> hideous.
>
>I don't agree ...
>
>The idea is that the header can grow because the offsets allow it to;
>adding a new member slots in between the end of static data and the
>start of variable data, and since all the variable data is accessed by
>an array, userspace doesn't have to be aware of the new members. 

This is the same in both approaches.

> The
>code for that is very clean (modulo the casts for pointer maths), cf.
>formats_ptr and modifiers_ptr; I'd expect userspace to copy and use
>those with version guards:
>    uint32_t *formats = formats_ptr(blob);
>    struct drm_format_modifier *modifiers = modifiers_ptr(blob);
>    struct drm_format_unifier *unifiiers = (blob->version >= 2) ?
>unifiers_ptr(blob) : NULL;

I concede the tricky code is all confined to the single implementation
in the kernel (encoding is far harder than decoding) - I just think
create_in_format_blob() is cumbersome, ugly and error-prone.

>
>That's shorter than fetching separate blobs, and doesn't have multiple
>error paths either. What am I missing?

Yes, this is a convincing argument.

>
>> Added bonus: With smart enough helper code the FourCC and modifiers
>> blobs can be shared across planes.
>
>I think this is a serious case of premature optimisation; the memory
>savings might be outweighed by the additional number of objects to
>track, and userspace would have to jump through serious hoops with a
>blob ID cache - something which is a very bad idea regardless - to
>take any advantage of it.

Hey I'm just saying it's an option - not that it should be implemented
in V1.

Where both the formats and the modifiers are the same, Ben's approach
lets the blob be shared anyway.

Thanks,
Brian

>
>Cheers, Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel