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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 03, 2015 at 10:41:49AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 08:17 PM, Daniel Vetter wrote:
> >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.
> 
> So you want to carefully lay out our fb modifiers so we can get our data out
> easily rather than extract the data at fb creation and have no such
> constraints?

Not sure it'll be that bad really, often it should look like this I expect
(totally made up example):

	switch(fb->modifier) {
		case MOD_NONE:
			break;
		case MOD_X:
			plane_mode |= TILE_X
			break;
		case MOD_FANCY_TILING_WITH_COMPRESSION:
			plane_mode |= COMPRESSED;
		case MOD_FANCY_TILING:
			plane_mode |= TILE_FANCY;
			break;
		default:
			/* this plane doesn't support any other
			 * combination */
			MISSING_CASE(fb->modifier);
	}

So not even sure we'd need to split stuff up that badly and make it all
maskable. In any case 56 bits gives us a lot of places for submasks, I
don't think that's a severe restriction really.

> Or in other words you want to reserve some bits for tiling and define a mask
> straight away, did I get that right? Not like this:
> 
> 1.
> 
> intel_fb creation
>    -> parse fb modifier and extract private attributes for future use
>       -> tiling_mode
>       -> compression_mode (eg.)
>       -> swizzling_mode (eg.)
> 
> tiling_mode use sites
>    -> just use intel_fb->tiling_mode
> 
> But like this:
> 
> 2.
> 
> intel_fb creation
>    -> do nothing

s/do nothing/fixup fb modifier for legacy userspace not using it/

> call sites
>    -> extract tiling_mode from fb modifier
> 
> * Note this still needs to map to same "enum" space as today at least in
> places which program the hardware.
> 
> You want option 2, correct?

Yes, I think that overall has less opportunities for accidental bugs.
-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