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

[Daniel Stone]

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. 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;

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

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

Cheers,
Daniel