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

From: Daniel Vetter <daniel@ffwll.ch>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation
Date: Thu, 26 Feb 2015 17:44:10 +0100	[thread overview]
Message-ID: <20150226164410.GQ24485@phenom.ffwll.local> (raw)
In-Reply-To: <1424882844-19444-9-git-send-email-tvrtko.ursulin@linux.intel.com>

On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@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.
-Daniel

> -
>  	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
>  						     mode_cmd->pixel_format);
>  	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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