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

[Daniel Vetter <daniel@ffwll.ch>]

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

> >With igt for the new cases in addfb and review this is imo good to get in.
> 
> I can do the IGT, but who is doing the libdrm part? :)

Generally when we do igts for new interfaces we just copypaste the new
struct definitions with local_ prefixed to avoid blocking the test on a
new libdrm release. So no one needs to do a libdrm patch ;-)
-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