Re: [PATCH 03/22] drm/i915: Factor out intel_tile_width()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 14, 2015 at 07:28:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Pull the tile width calculations from intel_fb_stride_alignment() into a
> new function intel_tile_width().
> 
> Also take the opportunity to pass aroun dev_priv instead of dev to
> intel_fb_stride_alignment().
> 
> v2: Reorder argumnents to be more consistent with other functions
>     Change intel_fb_stride_alignment() to accept dev_priv instead of dev
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
>  3 files changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6add8d1..c31fe47 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2214,6 +2214,41 @@ static bool need_vtd_wa(struct drm_device *dev)
>  	return false;
>  }
>  
> +static unsigned int intel_tile_width(const struct drm_i915_private *dev_priv,
> +				     uint64_t fb_modifier, unsigned int cpp)
> +{
> +	switch (fb_modifier) {
> +	case DRM_FORMAT_MOD_NONE:
> +		return cpp;
> +	case I915_FORMAT_MOD_X_TILED:
> +		return IS_GEN2(dev_priv) ? 128 : 512;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		/* No need to check for old gens and Y tiling since this is
> +		 * about the display engine and those will be blocked before
> +		 * we get here.
> +		 */
> +		return 128;

Imo just drop the comment and change this to

		return HAS_128_BYTE_Y_TILING(dev) 128 : 512;

Just to avoid wtf moments. It's less code than the comment too.

Another one: Enforcing pot alignment for gen2/3 is fairly hapzardous - we
do it accidentally by requiring that for X-tiled the underlying obj is
also X-tiled and that strides match. Comment for this would be good here.

> +	case I915_FORMAT_MOD_Yf_TILED:
> +		switch (cpp) {
> +		case 1:
> +			return 64;
> +		case 2:
> +		case 4:
> +			return 128;

With 4 cpp we have 4x4 pixels for the 64b cacheline. So 8x8 for the 256b
block with 1:1 aspect ratio, hence 64x64 pixel tile for the full page.

> +		case 8:

With 8 cpp we have 2x4 pixels for the 64b cacheline. So 8x4 for the 256b
block with 2:1 aspect ratio, hence 64x32 pixel tile for the full page.

> +		case 16:
> +			return 256;

With 16 cpp we have 1x4 pixels for the 64b cacheline. So 4x4 for the 256b
block with 1:1 aspect ratio, hence 32x32 pixel tile for the full page.

Otherwise looks good to me, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
once we close the above.

Cheers, Daniel

> +		default:
> +			MISSING_CASE(cpp);
> +			return cpp;
> +		}
> +		break;
> +	default:
> +		MISSING_CASE(fb_modifier);
> +		return cpp;
> +	}
> +}
> +
>  unsigned int
>  intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
>  		  uint64_t fb_format_modifier, unsigned int plane)
> @@ -2895,37 +2930,20 @@ 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 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> +			      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 (INTEL_INFO(dev)->gen == 2)
> -			return 128;
> -		return 512;
> -	case I915_FORMAT_MOD_Y_TILED:
> -		/* No need to check for old gens and Y tiling since this is
> -		 * about the display engine and those will be blocked before
> -		 * we get here.
> -		 */
> -		return 128;
> -	case I915_FORMAT_MOD_Yf_TILED:
> -		if (bits_per_pixel == 8)
> -			return 64;
> -		else
> -			return 128;
> -	default:
> -		MISSING_CASE(fb_modifier);
> +	if (fb_modifier == DRM_FORMAT_MOD_NONE) {
>  		return 64;
> +	} else {
> +		unsigned int cpp = drm_format_plane_cpp(pixel_format, 0);
> +
> +		return intel_tile_width(dev_priv, fb_modifier, cpp);
>  	}
>  }
>  
> @@ -3106,7 +3124,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>  	obj = intel_fb_obj(fb);
> -	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> +	stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>  					       fb->pixel_format);
>  	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>  
> @@ -9066,7 +9084,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	fb->width = ((val >> 0) & 0x1fff) + 1;
>  
>  	val = I915_READ(PLANE_STRIDE(pipe, 0));
> -	stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> +	stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>  						fb->pixel_format);
>  	fb->pitches[0] = (val & 0x3ff) * stride_mult;
>  
> @@ -11137,7 +11155,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
>  	 * linear buffers or in number of tiles for tiled buffers.
>  	 */
>  	stride = fb->pitches[0] /
> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> +		 intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>  					   fb->pixel_format);
>  
>  	/*
> @@ -14214,6 +14232,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
>  				  struct drm_i915_gem_object *obj)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned int aligned_height;
>  	int ret;
>  	u32 pitch_limit, stride_alignment;
> @@ -14255,7 +14274,8 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> -	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> +	stride_alignment = intel_fb_stride_alignment(dev_priv,
> +						     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",
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1152566..98a8d31 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1017,8 +1017,8 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
>  				   uint64_t fb_format_modifier);
>  void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
>  			enum fb_op_origin origin);
> -u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> -			      uint32_t pixel_format);
> +u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> +			      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 90e27c8..3d96871 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -215,7 +215,7 @@ 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],
> +	stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>  					       fb->pixel_format);
>  
>  	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> -- 
> 2.4.9
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx