[Intel-gfx] [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Rob Clark]

On Thu, Jan 29, 2015 at 7:55 AM, Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
> On 01/29/2015 11:57 AM, Daniel Vetter wrote:
>>
>> On Thu, Jan 29, 2015 at 11:43:07AM +0000, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 01/29/2015 11:30 AM, Daniel Vetter wrote:
>>>>
>>>> On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote:
>>>>>
>>>>>
>>>>> On 01/28/2015 05:37 PM, Daniel Vetter wrote:
>>>>>>
>>>>>> From: Rob Clark <robdclark at gmail.com>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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?
>>>>>
>>>>>
>>>>> Maybe:
>>>>>         __u64 modifier[4];
>>>>>         __u64 vendor_modifier[4];
>>>>
>>>>
>>>> Seems rendundant since the modifier added in this patch is already
>>>> vendor
>>>> specific. Or what exactly are you trying to accomplish here?
>>>
>>>
>>> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor
>>> id
>>> on the head followed by maybe standardized or maybe vendor specific tag.
>>> Feels funny. Would it not be simpler to put a struct in there?
>>
>>
>> The u64 modifier is just an opaque thing, with 8 bit to identifier the
>> vendor (for easier number management really) and the low 56 bits can be
>> whatever we want them. On i915 I think we should just enumerate all the
>> various tiling modes we have. And if the modifiers aren't set we use the
>> tiling mode of the underlying gem bo. We already have code in place to
>> guarantee that the underlying bo's tiling can't change as long as there's
>> a kms fb around, which means all code which checks for tiling can switch
>> over to these flags.
>>
>> struct won't work since by definition this is vendor-specific, and every
>> vendor is slightly insane in a different way.
>
>
> Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits
> expensive? :) That was first part of my point.

tbh, we could decide to do something different from 8+56b later if
needed..  nothing should really *depend* on the 8+56, since it is
intended to be an opaque token.  The 8+56 was just intended to make it
easier to merge values coming from different driver trees with less
conflicts.

> Secondly, "vendor who registers first" part of discussion is what came to my
> attention as well.
>
> With this, a hypothetical standard format would be under a vendor id which
> looks funny to me. While you could split standard formats/modifiers and
> vendor specific modifiers.

maybe we should s/vendor/driver/

> I don't care really, it was just an idea, but I don't get this arguments why
> it is, not only not better, but worse than a bitfield. :)

I guess it gets into bikeshed territory a bit, but I've tried to avoid
giving userspace the temptation to assume it is much more than an
opaque value.  The 8+56 thing was mainly just intended for logistical
convenience ;-)

BR,
-R


> Regards,
>
> Tvrtko