On Wed, Dec 21, 2016 at 7:42 AM Rob Clark <robdclark at gmail.com> wrote:
> 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.
>
Yeah, I like Ville's suggestion too, I'll give it a try.
Kristian
> BR,
> -R
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161221/88a862d8/attachment.html>
