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

[Rob Clark]

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?

It does make me briefly think of just letting us set properties on fb
objects :-P (but maybe that is a bit overkill)

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.

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

BR,
-R

>
> Cheers,
> Daniel