Re: [PATCH 1/2] drm: Add new DRM_IOCTL_MODE_GETPLANE2

From: Daniel Vetter <daniel@ffwll.ch>
To: "Kristian Høgsberg" <hoegsberg@gmail.com>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Kristian H. Kristensen" <hoegsberg@google.com>,
	"Kristian H . Kristensen" <hoegsberg@chromium.org>
Subject: Re: [PATCH 1/2] drm: Add new DRM_IOCTL_MODE_GETPLANE2
Date: Thu, 22 Dec 2016 08:04:56 +0100	[thread overview]
Message-ID: <20161222070456.f3w7zyezwgz5gcn7@phenom.ffwll.local> (raw)
In-Reply-To: <CAOeoa-eHRD5YH7UMHgvbRtAOFzo0i01jkLYw+GUXaPD8siCbsA@mail.gmail.com>

On Wed, Dec 21, 2016 at 05:26:21PM +0000, Kristian Høgsberg wrote:
> On Wed, Dec 21, 2016 at 7:42 AM Rob Clark <robdclark@gmail.com> wrote:
> 
> > On Wed, Dec 21, 2016 at 4:19 AM, Ville Syrjälä
> > <ville.syrjala@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@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);

bool (*format_mod_supported)(struct drm_device *dev, u32 format, u64 modifier);

... since the point is to apply device restrictions.

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

+1, I like too. Especially if we reuse this in addfb2 to filter out
unsupported combinations.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel