[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 03:11:02PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala at linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
> >
> > It does make me briefly think of just letting us set properties on fb
> >
> > objects :-P (but maybe that is a bit overkill)
> >
> > Yeah I had the same idea at some point. But then I decided that we could
> > just have these as properties on the plane.
> 
> 
> Mm, it does seem a bit weird. Yes, they are relative to how the plane
> interprets things, but then again so is the format surely. Not to mention
> another thing to go wrong, if someone forgets to set and/or clear it when
> changing the plane. Keeping it in the fb eliminates that possibility.
> 
> 
> > > I suppose the semi-custom plane property approach is a bit easier to
> > > extend-as-we-go, and we already have a mechanism so userspace can
> > > query about what is actually supported.  But this does feel a bit more
> > > like attributes of the fb.  I'm interested if anyone has particularly
> > > good arguments one way or another.
> >
> > I guess we could have just specified offset/size/stride as part of the
> > fb and let pixel format and such as properties. That would be a fairly
> > natural line IMO since it would be enough data to do a blit, but not
> > enough to actually interpret the pixel data. But we already went beyond
> > that with pixel formats. So I'm not sure how far we want to go.
> >
> > Also all this chroma siting and colorspace stuff definitely runs into
> > hardware specific limitations so having some way to tell userspace what
> > is possible would be nice as you said. Properties seem a decent match for
> > that.
> 
> 
> Yeah, that's a good idea actually, especially since different planes do
> have different capabilities.
> 
> 
> > > > It might be useful to make the interpretation modifiers bitmaskable,
> > so they
> > > > can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> > siting),
> > > > but I can't think of a usecase for combining multiple layout modifiers
> > (e.g.
> > > > this tiling | that compression).
> > >
> > > Yeah, I think the vendor-range part of the token, the vendor would
> > > probably want to define as a bitmask or set of bitfields so that they
> > > could have things like tiled+compressed
> > >
> > > (otoh, if you try to organize it too nicely now, eventually enough hw
> > > generations in the future that scheme will break down.. so maybe a big
> > > switch of #define cases is better than trying to interpret the
> > > modifier token)
> >
> 
> Having them separated is still kinda nice though, for the same rationale as
> the EGLImage import extension having them as hints. If your hardware
> doesn't support the tiling/compression format you use, then you're going to
> be showing absolute garbage. But if it doesn't support your exact
> chroma-siting or YUV range request, it'll still be totally viewable, just
> not entirely perfect. So I don't see the logic in failing these.

Well, it will look nasty when switching between GL and display
composition the GL path does the right thing an display path doesn't/
And we already have that problem with the fuzzy alignment/scaling
restriction stuff. So I think we will want some kind of strict flag
somewhere to allow the user to specify that they'd rather fail the whole
thing and fall back to GL rather than annoy the user.

But for some simpler cases like Xv it would seem perfectly OK to use the
less strict rules. Well, unless someone implements Xv in a way that can
also transparently switch between display planes and GL/software rendering.

> 
> 
> > > >> TODO how best to deal with assignment of modifier token values?  The
> > > >> rough idea was to namespace things with an 8bit vendor-id, and then
> > > >> beyond that it is treated as an opaque value.  But that was a
> > relatively
> > > >> arbitrary choice.  There are cases where same tiling pattern and/or
> > > >> compression is supported by various different vendors.  So we should
> > > >> standardize to use the vendor-id and value of the first one who
> > > >> documents the format?
> > > >
> > > >
> > > > Yeah, I'd second all of danvet's comments here, as well as adding a new
> > > > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
> > easier
> > > > for generic userspace.
> > >
> > > I've locally made a few tweaks (64b and move some stuff to
> > drm_fourcc.h)..
> > >
> > > I was kicking around the idea of letting plane specify an array of
> > > supported format modifiers, and adding this to getplane ioctl, as an
> > > alternative to a cap.  That plus wiring up some checking to disallow
> > > addfb2 for a format + modifiers not supported by at least one plane.
> > > Although some hw could only support certain tiling patterns for
> > > certain layers of an fb (ie. luma vs chroma).  So I may scrap that
> > > idea and just go back to cap.
> >
> > Indeed the format specific limitations are problem. Properties can't
> > handle that. We'd need to have some kind of caps for each plane+format
> > combination if we want to deal with that. But I don't think we can
> > still make it handle all the hw limitations, so I'm not sure it's worth
> > going down this path.
> 
> 
> Well, you don't have to solve literally everything at once. Just having a
> list of formats which could possibly be supported if you did the right
> thing, would be a hell of a lot better than punting to userspace, which
> either a) has to have hardware-specific knowledge in every component
> (compositor, media library, etc etc), or b) brute-force it. The lack of any
> format query in EGLImage dmabuf import is a serious, serious, serious, pain
> when trying to do generic userspace (e.g. compositor feeds GStreamer a list
> of formats which are supported by the hardware). I get that there are
> combinations that could fail, but that's true of everything. At least
> narrowing down the problem space a bit is an enormous help.

We alredy have a list of supported formats. The problem is when specific
formats impose additonal constraints (eg. more restricted scaling factor
limits).

> 
> Cheers,
> Daniel
> 
> On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala at linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
> > > On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <daniel at fooishbar.org>
> > wrote:
> > > > Hey,
> > > >
> > > > On 10 December 2014 at 17:17, Rob Clark <robdclark at gmail.com> wrote:
> > > >>
> > > >> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > > >> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > > >> we cannot always rely on under-the-hood flags passed to driver
> > specific
> > > >> gem-create ioctl to pass around these extra flags.
> > > >>
> > > >> The proposal is to add a per-plane format modifier.  This allows to,
> > if
> > > >> necessary, use different tiling patters for sub-sampled planes, etc.
> > > >> The format modifiers are added at the end of the ioctl struct, so for
> > > >> legacy userspace it will be zero padded.
> > > >
> > > >
> > > > Cool, thanks a lot for looking at this! My one comment is that we could
> > > > maybe go even further: keep the current modifier as a strict
> > pixel-layout
> > > > modifier (e.g. tiled, compressed - anything that affects how you
> > actually
> > > > determine pixel location), and then support an extra (perhaps
> > > > non-vendor-namespaced) argument for optional pixel _interpretation_
> > > > modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is
> > starting
> > > > to properly attack things like chroma siting, and being able to specify
> > > > narrow/wide YUV range is pretty important for STB/DTV in particular.
> > And
> > > > they're actually starting to move to KMS, too ...
> > >
> > > Up until now I had sort of thought of things like YUV range as plane
> > > properties which could be updated (potentially on flip as part of
> > > atomic ioctl).  But they are also additional metadata about how to
> > > properly interpret the pixel data contained in the buffer.
> > >
> > > I guess chroma siting and YUV range would at least be one value that
> > > applies across all the bos/planes of the fb, rather than per plane?
> >
> > There is the DV case where the chroma is sampled at different points for
> > Cb and Cr. So we could in theory specify chroma siting per-plane. But it
> > seems to me that it'd be enough to have it for the entire fb. I had some
> > ideas posted years ago. Here's [1] one at least.
> >
> > [1]
> > http://lists.freedesktop.org/archives/dri-devel/2011-November/016379.html
> >
> > >
> > > It does make me briefly think of just letting us set properties on fb
> > > objects :-P (but maybe that is a bit overkill)
> >
> > Yeah I had the same idea at some point. But then I decided that we could
> > just have these as properties on the plane.
> >
> > >
> > > I suppose the semi-custom plane property approach is a bit easier to
> > > extend-as-we-go, and we already have a mechanism so userspace can
> > > query about what is actually supported.  But this does feel a bit more
> > > like attributes of the fb.  I'm interested if anyone has particularly
> > > good arguments one way or another.
> >
> > I guess we could have just specified offset/size/stride as part of the
> > fb and let pixel format and such as properties. That would be a fairly
> > natural line IMO since it would be enough data to do a blit, but not
> > enough to actually interpret the pixel data. But we already went beyond
> > that with pixel formats. So I'm not sure how far we want to go.
> >
> > Also all this chroma siting and colorspace stuff definitely runs into
> > hardware specific limitations so having some way to tell userspace what
> > is possible would be nice as you said. Properties seem a decent match for
> > that.
> >
> > >
> > > > It might be useful to make the interpretation modifiers bitmaskable,
> > so they
> > > > can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> > siting),
> > > > but I can't think of a usecase for combining multiple layout modifiers
> > (e.g.
> > > > this tiling | that compression).
> > > >
> > >
> > > Yeah, I think the vendor-range part of the token, the vendor would
> > > probably want to define as a bitmask or set of bitfields so that they
> > > could have things like tiled+compressed
> > >
> > > (otoh, if you try to organize it too nicely now, eventually enough hw
> > > generations in the future that scheme will break down.. so maybe a big
> > > switch of #define cases is better than trying to interpret the
> > > modifier token)
> > >
> > > >>
> > > >> TODO how best to deal with assignment of modifier token values?  The
> > > >> rough idea was to namespace things with an 8bit vendor-id, and then
> > > >> beyond that it is treated as an opaque value.  But that was a
> > relatively
> > > >> arbitrary choice.  There are cases where same tiling pattern and/or
> > > >> compression is supported by various different vendors.  So we should
> > > >> standardize to use the vendor-id and value of the first one who
> > > >> documents the format?
> > > >
> > > >
> > > > Yeah, I'd second all of danvet's comments here, as well as adding a new
> > > > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
> > easier
> > > > for generic userspace.
> > >
> > > I've locally made a few tweaks (64b and move some stuff to
> > drm_fourcc.h)..
> > >
> > > I was kicking around the idea of letting plane specify an array of
> > > supported format modifiers, and adding this to getplane ioctl, as an
> > > alternative to a cap.  That plus wiring up some checking to disallow
> > > addfb2 for a format + modifiers not supported by at least one plane.
> > > Although some hw could only support certain tiling patterns for
> > > certain layers of an fb (ie. luma vs chroma).  So I may scrap that
> > > idea and just go back to cap.
> >
> > Indeed the format specific limitations are problem. Properties can't
> > handle that. We'd need to have some kind of caps for each plane+format
> > combination if we want to deal with that. But I don't think we can
> > still make it handle all the hw limitations, so I'm not sure it's worth
> > going down this path.
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> >

-- 
Ville Syrjälä
Intel OTC