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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@fooishbar.org> wrote:
> > Hey,
> >
> > On 10 December 2014 at 17:17, Rob Clark <robdclark@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel