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

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: "Kristian H. Kristensen" <hoegsberg@google.com>,
	"Kristian H . Kristensen" <hoegsberg@chromium.org>,
	"Kristian H. Kristensen" <hoegsberg@gmail.com>,
	ben@widawsk.net,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm: Add new DRM_IOCTL_MODE_GETPLANE2
Date: Wed, 21 Dec 2016 10:15:15 +0100	[thread overview]
Message-ID: <20161221091515.ucdg57snsybqfmfx@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGvXf-n1U4Nb0+OQzQCt59r+0fHqWacBF=Bq4kNRdELDiw@mail.gmail.com>

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