Re: [PATCH 2/7] drm/i915/skl: Allow scanning out Y and Yf fbs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 23, 2015 at 03:55:56PM +0000, Tvrtko Ursulin wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> Skylake is able to scannout those tiling formats. We need to allow them
> in the ADDFB ioctl and tell the harware about it.
> 
> v2: Rebased for addfb2 interface. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> v4: Don't allow Y tiled fbs just yet. (Tvrtko Ursulin)
> v5: Check for stride alignment and max pitch. (Tvrtko Ursulin)
> v6: Simplify maximum pitch check. (Ville Syrjälä)
> v7: Drop the gen9 check since requirements are no different. (Ville Syrjälä)
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
>  3 files changed, 95 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e70748..a523d84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2728,6 +2728,34 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	POSTING_READ(reg);
>  }
>  
> +u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> +			      uint32_t pixel_format)
> +{
> +	u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> +
> +	/*
> +	 * The stride is either expressed as a multiple of 64 bytes
> +	 * chunks for linear buffers or in number of tiles for tiled
> +	 * buffers.
> +	 */
> +	switch (fb_modifier) {
> +	case DRM_FORMAT_MOD_NONE:
> +		return 64;
> +	case I915_FORMAT_MOD_X_TILED:

if (gen2)
	return 128;

> +		return 512;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		return 128;

In theory we could check gen2 and HAS_128_BYTE_Y_TILING() here, but
since old platforms didn't do Y tiled scanout anyway that's not really
needed. But maybe toss in a comment about that so that people don't
start wondering why this doesn't care about that stuff?

> +	case I915_FORMAT_MOD_Yf_TILED:
> +		if (bits_per_pixel == 8)
> +			return 64;
> +		else
> +			return 128;
> +	default:
> +		MISSING_CASE(fb_modifier);
> +		return 64;
> +	}
> +}
> +
>  static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					 struct drm_framebuffer *fb,
>  					 int x, int y)
> @@ -2735,10 +2763,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb;
>  	struct drm_i915_gem_object *obj;
>  	int pipe = intel_crtc->pipe;
> -	u32 plane_ctl, stride;
> +	u32 plane_ctl, stride_div;
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -2782,29 +2809,30 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		BUG();
>  	}
>  
> -	intel_fb = to_intel_framebuffer(fb);
> -	obj = intel_fb->obj;
> -
> -	/*
> -	 * The stride is either expressed as a multiple of 64 bytes chunks for
> -	 * linear buffers or in number of tiles for tiled buffers.
> -	 */
>  	switch (fb->modifier[0]) {
>  	case DRM_FORMAT_MOD_NONE:
> -		stride = fb->pitches[0] >> 6;
>  		break;
>  	case I915_FORMAT_MOD_X_TILED:
>  		plane_ctl |= PLANE_CTL_TILED_X;
> -		stride = fb->pitches[0] >> 9;
> +		break;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		plane_ctl |= PLANE_CTL_TILED_Y;
> +		break;
> +	case I915_FORMAT_MOD_Yf_TILED:
> +		plane_ctl |= PLANE_CTL_TILED_YF;
>  		break;
>  	default:
> -		BUG();
> +		MISSING_CASE(fb->modifier[0]);
>  	}
>  
>  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>  	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
>  
> +	obj = intel_fb_obj(fb);
> +	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> +					       fb->pixel_format);
> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  
>  	DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
> @@ -2817,7 +2845,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_SIZE(pipe, 0),
>  		   (intel_crtc->config->pipe_src_h - 1) << 16 |
>  		   (intel_crtc->config->pipe_src_w - 1));
> -	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> +	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>  	I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
>  
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -12657,14 +12685,43 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>  	.create_handle = intel_user_framebuffer_create_handle,
>  };
>  
> +static
> +u32 intel_fb_pitch_limit(struct drm_device *dev, uint64_t fb_modifier,
> +			 uint32_t pixel_format)
> +{
> +	u32 gen = INTEL_INFO(dev)->gen;
> +
> +	if (gen >= 9) {
> +		/* "The stride in bytes must not exceed the of the size of 8K
> +		 *  pixels and 32K bytes."
> +		 */
> +		 return min(8192*drm_format_plane_cpp(pixel_format, 0), 32768);
> +	} else if (gen >= 5 && !IS_VALLEYVIEW(dev)) {
> +		return 32*1024;
> +	} else if (gen >= 4) {
> +		if (fb_modifier == I915_FORMAT_MOD_X_TILED)
> +			return 16*1024;
> +		else
> +			return 32*1024;
> +	} else if (gen >= 3) {
> +		if (fb_modifier == I915_FORMAT_MOD_X_TILED)
> +			return 8*1024;
> +		else
> +			return 16*1024;
> +	} else {
> +		/* XXX DSPC is limited to 4k tiled */
> +		return 8*1024;
> +	}
> +}
> +
>  static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct intel_framebuffer *intel_fb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
>  				  struct drm_i915_gem_object *obj)
>  {
>  	int aligned_height;
> -	int pitch_limit;
>  	int ret;
> +	u32 pitch_limit, stride_alignment;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
> @@ -12690,31 +12747,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> -	if (mode_cmd->pitches[0] & 63) {
> -		DRM_DEBUG("pitch (%d) must be at least 64 byte aligned\n",
> -			  mode_cmd->pitches[0]);
> +	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> +						     mode_cmd->pixel_format);
> +	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> +		DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> +			  mode_cmd->pitches[0], stride_alignment);
>  		return -EINVAL;
>  	}
>  
> -	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
> -		pitch_limit = 32*1024;
> -	} else if (INTEL_INFO(dev)->gen >= 4) {
> -		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
> -			pitch_limit = 16*1024;
> -		else
> -			pitch_limit = 32*1024;
> -	} else if (INTEL_INFO(dev)->gen >= 3) {
> -		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
> -			pitch_limit = 8*1024;
> -		else
> -			pitch_limit = 16*1024;
> -	} else
> -		/* XXX DSPC is limited to 4k tiled */
> -		pitch_limit = 8*1024;
> -
> +	pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0],
> +					   mode_cmd->pixel_format);

We seem to alternate between "stride" and "pitch" somewhat randomly.
Should maybe stick to one or other :)

Anyways this looks pretty good to me. With the gen2 X tiled stride
alignment fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	if (mode_cmd->pitches[0] > pitch_limit) {
> -		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
> -			  mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED ?
> +		DRM_DEBUG("%s pitch (%u) must be at less than %d\n",
> +			  mode_cmd->modifier[0] != DRM_FORMAT_MOD_NONE ?
>  			  "tiled" : "linear",
>  			  mode_cmd->pitches[0], pitch_limit);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..399d2b2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -883,6 +883,8 @@ int intel_fb_align_height(struct drm_device *dev, int height,
>  			  uint64_t fb_format_modifier);
>  void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
>  
> +u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> +			      uint32_t pixel_format);
>  
>  /* intel_audio.c */
>  void intel_init_audio(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 5ae56ec..b7103bd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -189,7 +189,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	u32 plane_ctl, stride;
> +	u32 plane_ctl, stride_div;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
> @@ -247,15 +247,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  
>  	switch (fb->modifier[0]) {
>  	case DRM_FORMAT_MOD_NONE:
> -		stride = fb->pitches[0] >> 6;
>  		break;
>  	case I915_FORMAT_MOD_X_TILED:
>  		plane_ctl |= PLANE_CTL_TILED_X;
> -		stride = fb->pitches[0] >> 9;
> +		break;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		plane_ctl |= PLANE_CTL_TILED_Y;
> +		break;
> +	case I915_FORMAT_MOD_Yf_TILED:
> +		plane_ctl |= PLANE_CTL_TILED_YF;
>  		break;
>  	default:
> -		BUG();
> +		MISSING_CASE(fb->modifier[0]);
>  	}
> +
>  	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
>  
> @@ -266,6 +271,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  				       pixel_size, true,
>  				       src_w != crtc_w || src_h != crtc_h);
>  
> +	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> +					       fb->pixel_format);
> +
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -273,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	crtc_h--;
>  
>  	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> -	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> +	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
>  	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>  	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> -- 
> 2.3.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx