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

[Daniel Vetter <daniel@ffwll.ch>]

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

> +
> +	if (tiling_mode == I915_TILING_Y) {
>  		DRM_DEBUG("hardware does not support tiling Y\n");
>  		return -EINVAL;
>  	}
> @@ -12696,12 +12722,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
>  		pitch_limit = 32*1024;
>  	} else if (INTEL_INFO(dev)->gen >= 4) {
> -		if (obj->tiling_mode)
> +		if (tiling_mode)
>  			pitch_limit = 16*1024;
>  		else
>  			pitch_limit = 32*1024;
>  	} else if (INTEL_INFO(dev)->gen >= 3) {
> -		if (obj->tiling_mode)
> +		if (tiling_mode)
>  			pitch_limit = 8*1024;
>  		else
>  			pitch_limit = 16*1024;
> @@ -12711,12 +12737,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  
>  	if (mode_cmd->pitches[0] > pitch_limit) {
>  		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
> -			  obj->tiling_mode ? "tiled" : "linear",
> +			  tiling_mode ? "tiled" : "linear",
>  			  mode_cmd->pitches[0], pitch_limit);
>  		return -EINVAL;
>  	}
>  
> -	if (obj->tiling_mode != I915_TILING_NONE &&
> +	if (tiling_mode != I915_TILING_NONE && obj->stride &&
>  	    mode_cmd->pitches[0] != obj->stride) {
>  		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
>  			  mode_cmd->pitches[0], obj->stride);
> @@ -12771,7 +12797,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	aligned_height = intel_fb_align_height(dev, mode_cmd->height,
> -					       obj->tiling_mode);
> +					       tiling_mode);
>  	/* FIXME drm helper for size checks (especially planar formats)? */
>  	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
>  		return -EINVAL;
> @@ -12779,7 +12805,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
>  	intel_fb->obj = obj;
>  	intel_fb->obj->framebuffer_references++;
> -	intel_fb->tiling_mode = obj->tiling_mode;
> +	intel_fb->tiling_mode = tiling_mode;
>  
>  	ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
>  	if (ret) {
> -- 
> 2.2.2
> 
> _______________________________________________
> 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