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

[Daniel Vetter]

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.

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

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

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch