[Intel-gfx] [PATCH 3/4] drm/i915: Use frame buffer modifiers for tiled display

[Tvrtko Ursulin]

On 02/04/2015 03:33 PM, Daniel Vetter wrote:
> On Wed, Feb 04, 2015 at 03:09:38PM +0000, Tvrtko Ursulin wrote:
>> On 02/04/2015 02:25 PM, Daniel Vetter wrote:
>>> On Wed, Feb 04, 2015 at 10:01:45AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 02/03/2015 07:47 PM, Daniel Vetter wrote:
>>>>> On Tue, Feb 03, 2015 at 05:22:31PM +0000, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>
>>>>>> Start using frame buffer modifiers instead of object tiling mode
>>>>>> for display purposes.
>>>>>>
>>>>>> To ensure compatibility with old userspace which is using set_tiling
>>>>>> and does not know about frame buffer modifiers, the latter are faked
>>>>>> internally when tile object is set for display. This way all interested
>>>>>> call sites can use fb modifiers exclusively.
>>>>>>
>>>>>> Also ensure tiling specified via fb modifiers must match object tiling
>>>>>> used for fencing if both are specified.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/intel_display.c | 95 +++++++++++++++++++++++++-----------
>>>>>>   drivers/gpu/drm/i915/intel_drv.h     |  2 +
>>>>>>   drivers/gpu/drm/i915/intel_pm.c      |  7 +--
>>>>>>   drivers/gpu/drm/i915/intel_sprite.c  | 26 +++++-----
>>>>>>   4 files changed, 85 insertions(+), 45 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index 7a3ed61..6825016 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -2198,6 +2198,19 @@ intel_fb_align_height(struct drm_device *dev, int height, unsigned int tiling)
>>>>>>   	return ALIGN(height, tile_height);
>>>>>>   }
>>>>>>
>>>>>> +static unsigned int intel_fb_modifier_to_tiling(u64 mod)
>>>>>> +{
>>>>>> +	BUILD_BUG_ON((I915_FORMAT_MOD_X_TILED & 0x00ffffffffffffffL) !=
>>>>>> +		     I915_TILING_X);
>>>>>> +
>>>>>> +	return mod & 1;
>>>>>> +}
>>>>>> +
>>>>>> +unsigned int intel_fb_tiling_mode(struct drm_framebuffer *fb)
>>>>>> +{
>>>>>> +	return intel_fb_modifier_to_tiling(fb->modifier[0]);
>>>>>> +}
>>>>>
>>>>> I expect that these here will create a bit of churn with the skl patches
>>>>> you have based, since I really don't want a new I915_TILING_FANCY define
>>>>> in the enum space used by obj->tiling mode. But makes sense for backwards
>>>>> compat with older platforms and less churn in code.
>>>>
>>>> I thought we talked about effectively creating a new enum space for fb
>>>> tiling? By masking out bits from the fb modifier, no? Only thing for
>>>> backward compatibility is that object X tiling and fb X tiling == 1.
>>>
>>> intel_fb_tiling_mode maps modifier (the new enum space) to
>>> obj->tiling_mode (the old enum space). Means a notch less churn in legacy
>>> code (but if that's the metric I'd just have kept using obj->tiling_mode
>>> there). But means that you get to chance skl code twice, because I very
>>> much don't want a new I915_TILING_DEFINE but instead the skl code should
>>> check the new modifiers directly. Otherwise we can mash up tiling modes
>>> valid just for ggtt fencing and fb modifiers in general.
>>>
>>> Maybe I wasn't really clear with what I've meant ...
>>
>> It does seem it is taking very long to get on the same page here. :/
>>
>> I did not plan to add new I915_TILING_xxx. I was exploiting the fact both
>> map to the same value, with masking. So legacy continues to work since this
>> will be true forever. (ABI)
>>
>> Then the plan was to add a new namespace for display tiling enums.
>>
>> This was since fb modifier could contain more than tiling and this way it is
>> possible to mask out and case-switch just as the current code does.
>>
>> There are three namespaces here:
>>
>> 1. I915_TILING_xxx
>> 2. I915_FORMAT_MOD_ (fb modifiers)
>> 3. Tiling as programmed to display hardware
>>
>> And then add a fourth one:
>>
>> 4. I915_DISPLAY_TILING_xxx
>>
>> At this step also add something like I915_FORMAT_MOD_TILING_MASK and
>> redefine I915_FORMAT_MOD_X_TILE to be fourcc_mod(INTEL,
>> I915_DISPLAY_TILING_X). (Instead of hardcoded 1)
>>
>> At call sites (opencoded):
>>
>> switch (fb->modifier[0] & I915_FORMAT_MOD_TILING) {
>> case I915_DISPLAY_TILING_X:
>
> This is kinda what I'd have done, expect that you can cleverly define the
> mask to include the vendor prefix, i.e.
>
> #define I915_FORMAT_MOD_TILING_MASK ((0xff << 56) | 0xff)
>
> and then you don't need yet another set of defines. And still have the
> clear separation between I915_TILING_FOO and the new fb modifier stuff.

Hm side question - maybe DRM patch could instead of allow_fb_modifiers 
boolean take allow_fb_modifier = VENDORA | VENDORB, and then stem at the 
source any attempts to pass unsupported ones to the driver. :)

>> ...
>>
>> I mean we could do:
>>
>> switch (fb->modifier[0]) {
>> case I915_FORMAT_MOD_X_TILE:
>
> Or this. Since we don't yet have anything else than tiling modes you'll
> get away with it and can postpone the mask stuff to whomever ends up
> implementing the non-tiling fb modifiers.

Not nice but you told me to do it. :D

>> ...
>>
>> If fb modifiers won't have any overlap, like for example:
>>
>> #define I915_FORMAT_MOD_X_TILE fourcc_mod(INTEL, 1)
>> #define I915_FORMAT_MOD_X_TILE_AND_UNRELATED fourcc_mod(INTEL, 1<<8 && 1)
>>
>> Then the direct usage stops working..
>>
>> Up to you, I have to unblock other stuff so we can't strangle this for too
>> long.
>
> The super-minimal approach would be to shrink this patch down to the
> fixup/check code in framebuffer_init and then move the conversion for skl
> display code (and just that) into the next series which adds the fancy skl
> patches. And use one of the switch statements above to decode the fb
> modifier. Goes well with my default stance of "in case of doubt, pick less
> churn".

Disallow fb modifiers on gen < 9 regardless of DRM_CAP? Sounds nasty..

Regards,

Tvrtko