[Intel-gfx] [PATCH 2/3] drm: Create a format/modifier blob

[Brian Starkey]

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 at 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