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