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 21:17:42 +0100	[thread overview]
Message-ID: <20150202201742.GD14009@phenom.ffwll.local> (raw)
In-Reply-To: <54CFB43C.3000806@linux.intel.com>

On Mon, Feb 02, 2015 at 05:30:36PM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 05:15 PM, Daniel Vetter wrote:
> >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
> 
> Why not? It is not changing the ABI since obj->tiling_mode stays exactly the
> same as it is today.
> 
> Do you worry about leaking new data out in i915_drm.h, under the
> I915_TILING_* #defines? I don't see that we have to change that at all.

I prefer to keep enums for different types of values separate to avoid
confusion.

> >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).
> 
> So you are strongly for "looking into a bag of stuff" to see if anything
> interesting is there on every call site?
> 
> Helper functions in my view only marginally help there - they make the code
> neater but design is conceptually still untidy. And you add pointless
> processing on every call site.
> 
> I just don't see what is the problem with extracting the interesting data
> "from the bag" at fb init time. If you tried to make some synchronization
> argument in the other reply I don't get it.

So afaik at most we'll get a few more bits for compression, perhaps
swizzling (although that's dead on gen8+), whatelse. If we lay out the
defines in the intel vendor modifier space we can get at that by simple
masking. Also, kms operations are done at about 60fps rate, so computation
overhead is totally irrelevant (well as long as we just waste a few
cycles).

The synchronization argument is that any kind of duplicated data will get
out of sync sooner or later in my experience. We can't smash everything
into obj->tiling with the addfb2.5 abi (and because they're also for
different things), but we can avoid duplicating information between
fb->modifier and intel_fb->tiling_mode by not having the second.

Yes that means we need to fix up ->modifier (which your patches dont do).
But sooner or later someone will look at ->modifier and not ->tiling_mode
(because hey it worked on new userspace) and then *boom* we have a nice
confusing regression report from someone. Or someone looks at
obj->tiling_mode instead of intel_fb->tiling_mode (hey it works, because
they're using the same enum values) until the newfangled tiling thing
shows up.

> fb->modifier[0] should be, in my opinion, viewed as immutable. And it lives
> at the base class level while in intel_frambuffer sub-class it should be
> just fine to "parse" that into directly usable data stored at the sub-class
> level.

Fully agreed on immutable, but that doesn't exclude computing an
appropriate value at fb init time.
-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