On Wed, Dec 21, 2016 at 4:19 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
