[PATCH 1/2] drm: Add new DRM_IOCTL_MODE_GETPLANE2

[Rob Clark]

On Wed, Dec 21, 2016 at 4:19 AM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Dec 21, 2016 at 10:15:15AM +0100, Daniel Vetter wrote:
>> On Tue, Dec 20, 2016 at 07:46:12PM -0500, Rob Clark wrote:
>> > 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.
>> >
>> > 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.
>>
>> Hm, I'd have said with max 23 currently used 64 is good enough.
>> >
>> > 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..
>>
>> Could we do a table of tables instead, at least internally? So
>>
>> struct drm_format_support {
>>       u64 modifiers;
>>       u32 *formats;
>> };
>>
>> And then supply an array of those? Maybe with some magic to convert it in
>> the ioctl since the proposed ioctl struct is easier to transport to
>> userspace. Could also switch over to terminating arrays with a final 0
>> element (like with pci tables and everything else used in the kernel) to
>> get rid of all those silly ARRAY_SIZE lines. Totalling up:
>>
>> u32 format_list_untiled[] = { RGBX8888, RGBA8888, 0 };
>> u32 format_list_tiled[] = { RGBX8888, 0 };
>>
>> struct drm_format_support formats[] = {
>>       { MODE_NODE, format_untiled },
>>       { MODE_TILED, format_tiled },
>>       { 0, NULL }
>> };
>>
>> Not sure that's any better really.
>
> I think what we might want is:
>
> u32 formats[]
> u64 modifiers[]
> bool (*format_mod_supported)(u32 format, u64 modifier);
>
> The driver provides those, and the core will then just go through the
> combinations and build up the masks. We could then also reuse that stuff
> for addfb2 so that the core will call that same hook to check whether
> the format+modifier is supported. That way the driver .fb_create() will
> never see any unsupported combinations and we avoid having to duplicate
> any logic there to see which hardware supports which formats.
>

I do like this for internal API better, rather than driver building up
tables itself.

BR,
-R