[Intel-gfx] [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Daniel Vetter]

On Fri, Feb 27, 2015 at 09:45:27AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/26/2015 04:44 PM, Daniel Vetter wrote:
> >On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>By this patch all underlying bits have been implemented and this
> >>patch actually enables the feature.
> >>
> >>v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Reviewed-by: Damien Lespiau <damien.lespiau at intel.com> (v1)
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
> >>  1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 1d50934..3232ddd 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>
> >>  	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>+		/* Passed in modifier sanity checking. */
> >>+		switch (mode_cmd->modifier[0]) {
> >>+		case DRM_FORMAT_MOD_NONE:
> >>+		case I915_FORMAT_MOD_X_TILED:
> >>+		case I915_FORMAT_MOD_Y_TILED:
> >>+		case I915_FORMAT_MOD_Yf_TILED:
> >>+			break;
> >>+		default:
> >>+			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> >>+				  mode_cmd->modifier[0]);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>  		/* Enforce that fb modifier and tiling mode match, but only for
> >>  		 * X-tiled. This is needed for FBC. */
> >>  		if (!!(obj->tiling_mode == I915_TILING_X) !=
> >>@@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> >>  			return -EINVAL;
> >>  		}
> >>+
> >>+		if (INTEL_INFO(dev)->gen < 9 &&
> >>+		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>+		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>+			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> >>+				  mode_cmd->modifier[0]);
> >>+			return -EINVAL;
> >>+		}
> >>  	} else {
> >>  		if (obj->tiling_mode == I915_TILING_X)
> >>  			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> >>@@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  		}
> >>  	}
> >>
> >>-	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> >>-		DRM_DEBUG("hardware does not support tiling Y\n");
> >>-		return -EINVAL;
> >>-	}
> >
> >My idea was actually to put the switch here (reduces one indent level) and
> >put the per-gen stuff into the switch statement too. I.e.
> >
> >     	/* Passed in modifier sanity checking. */
> >     	switch (mode_cmd->modifier[0]) {
> >     	case I915_FORMAT_MOD_Y_TILED:
> >     	case I915_FORMAT_MOD_Yf_TILED:
> >		if (INTEL_INFO(dev)->gen < 9) {
> >			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> >				  mode_cmd->modifier[0]);
> >			return -EINVAL;
> >
> >		}
> >     	case DRM_FORMAT_MOD_NONE:
> >     	case I915_FORMAT_MOD_X_TILED:
> >     		break;
> >     	default:
> >     		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> >     			  mode_cmd->modifier[0]);
> >     		return -EINVAL;
> >     	}
> >
> >That gives us a natural place for extensions later on if we need to add
> >more special cases.
> 
> Yes I agree this patch being only on v2 was way disproportionate compared to
> some others from this series. ;)

Well I've obviously failed to explain properly in my review to v1 to avoid
a v3 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch