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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 04, 2015 at 03:44:58PM +0000, Tvrtko Ursulin wrote:
> 
> 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@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@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..

Nope, we'd allow the in framebuffer_init, but the only thing you can do is
ask for TILE_X, and it must match with obj->tiling. Hm, that gives a
slightly different check than what you have now, but really shouldn't be a
restriction since scanout stuff is always allocated as separate buffers
and userspace does an unconditional set_tiling when using X-tiled.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx