Re: [RFC 5/6] drm/i915: Allow fb modifier to set framebuffer tiling

From: Daniel Vetter <daniel@ffwll.ch>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [RFC 5/6] drm/i915: Allow fb modifier to set framebuffer tiling
Date: Mon, 2 Feb 2015 18:15:51 +0100	[thread overview]
Message-ID: <20150202171551.GA14009@phenom.ffwll.local> (raw)
In-Reply-To: <54CF532E.10100@linux.intel.com>

On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 09:54 AM, Daniel Vetter wrote:
> >On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Use the fb modifier if it was specified over object tiling mode.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
> >>  1 file changed, 33 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index e22afbe..ca69da0 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >>  	.create_handle = intel_user_framebuffer_create_handle,
> >>  };
> >>
> >>+static unsigned int
> >>+intel_fb_modifier_to_tiling(u64 modifier)
> >>+{
> >>+	switch (modifier) {
> >>+	case I915_FORMAT_MOD_X_TILED:
> >>+		return I915_TILING_X;
> >>+	default:
> >>+	case I915_FORMAT_MOD_NONE:
> >>+		break;
> >>+	}
> >>+
> >>+	return I915_TILING_NONE;
> >>+}
> >>+
> >>  static int intel_framebuffer_init(struct drm_device *dev,
> >>  				  struct intel_framebuffer *intel_fb,
> >>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> >>@@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  {
> >>  	int aligned_height;
> >>  	int pitch_limit;
> >>+	unsigned int tiling_mode = obj->tiling_mode;
> >>  	int ret;
> >>
> >>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>
> >>-	if (obj->tiling_mode == I915_TILING_Y) {
> >>+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>+		tiling_mode =
> >>+			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
> >>+		if (tiling_mode != obj->tiling_mode &&
> >>+			obj->tiling_mode != I915_TILING_NONE) {
> >>+			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
> >>+					tiling_mode, obj->tiling_mode);
> >>+			return -EINVAL;
> >>+		}
> >>+	}
> >
> >Ah, here comes the magic. I think this might be simpler if we just use
> >->modifier (and fix it up if FB_MODIFIERS isn't set).
> >
> >Btw another reason for this split is that this way we have a clear
> >separation between the tiling modes supported generally (as fb modifiers)
> >and the tiling modes supported by fences. It might therefore make sense to
> >rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
> >->fence_tiling_mode). To make it really clear that it's just about the
> >global gtt fences and nothing more.
> 
> I don't really like using ->modifier directly in tiling patch since it is an
> bag of unrelated stuff, not only a superset. Unrelated especially, but not
> only, from the point of view of call sites / users.
> 
> Therefore I see some design elegance in extracting the tiling, or any other
> logical group of modifiers before hand.
> 
> At the very least would call something like intel_fb_modifier_to_tiling(),
> but, it is very ugly to have a dynamic cost at every call site. Which is
> another reason why I preferred to extract the data before hand.

The reason is that the current tiling_mode enum is userspace ABI, and
it's just for how to fence global gtt mappings. That's the point of
splitting the fb modifiers out like in this rfc.

So if you add your fancy new tiling mode you can't do that, since you
can't extend the tiling_mode enum. Adding another enum also seems a bit
too much when we already have fb_modifiers.

And if fb_modifiers get too complicated we can add helper functions which
normalize stuff, e.g. extract just the base tiling mode and remove other
things (like compression mode or whatever it's going to be).
-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