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

[Daniel Stone]

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.


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

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141212/eb565cf6/attachment-0001.html>