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@xxxxxxxxx> > >>>>>> > >>>>>>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@xxxxxxxxx> > >>>>>>--- > >>>>>> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx