<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Dec 21, 2016 at 7:42 AM Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Dec 21, 2016 at 4:19 AM, Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com" class="gmail_msg" target="_blank">ville.syrjala@linux.intel.com</a>> 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 >> > <<a href="mailto:hoegsberg@gmail.com" class="gmail_msg" target="_blank">hoegsberg@gmail.com</a>> 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. </blockquote><div> </div><div>Yeah, I like Ville's suggestion too, I'll give it a try.</div><div> </div><div>Kristian</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> BR, -R </blockquote></div></div>