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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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);

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.


> -Daniel
> 
> > 
> > BR,
> > -R
> > 
> > > +       /* This modifier can be used with the format for this plane. */
> > > +       uint64_t modifier;
> > > +};
> > > +
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel