[PATCH 1/2] drm: Add new DRM_IOCTL_MODE_GETPLANE2

[Kristian H. Kristensen]

Rob Clark <robdclark at gmail.com> writes:

> On Tue, Dec 20, 2016 at 7:12 PM, Kristian H. Kristensen
> <hoegsberg at gmail.com> wrote:
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2..cea3de3 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -209,6 +209,33 @@ struct drm_mode_get_plane {
>>         __u64 format_type_ptr;
>>  };
>>
>> +struct drm_format_modifier {
>> +       /* Bitmask of formats in get_plane format list this info
>> +        * applies to. */
>> +       uint64_t formats;
>
> Hmm, this seems a bit clunky/brittle when you have a lot of supported
> formats (esp. if format order changes or new formats are not added at
> end).  I guess fine when you support a four or five different formats,
> but I think you'll start to hate maintaining those tables on hw that
> supports more.

I can't disagree, it was pretty finicky getting the bit masks right for
the Intel patch. On the other hand, in userspace, the interface is
easier to work with as you create the bitmask programatically, and
there's precedence for bitmask already (possible_crtcs).

> Also I guess it limits you to modifiers only with the first 64
> formats.. maybe not a problem right away, but a quick look and drm/msm
> is already at 23 formats (and there are probably some more it could
> do.. without even starting to get into "exotic" float/etc formats and
> whatever else might come in the future.
>
> Not that I really have a better idea..  maybe just instead of
> getplane2 we just add a querymodifiers ioctl (ie. fourcc in, list of
> modifiers out), with the idea that userspace probably knows what
> format/fourcc it wants to use, and only just cares about modifiers for
> that particular fourcc..

I'll reworking this with your base idea and Ville's suggestion and see
how it comes out. Thanks for the feedback.

Kristian

> BR,
> -R
>
>> +       /* This modifier can be used with the format for this plane. */
>> +       uint64_t modifier;
>> +};
>> +