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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 21, 2015 at 03:09:55PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 21, 2015 at 12:15:42PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 14, 2015 at 07:28:55PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > >  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.
> 
> Yeah, can do. And we definitely need it if we want to use this for gem
> stuff. Although we need to change HAS_128_BYTE_Y_TILING() to include
> gen2. Currently that define actually means "has 128x32 Y tiling". Or
> we make intel_tile_width() check for 'IS_GEN2 || HAS_128_BYTE_Y_TILING'.

Oops, yeah.

> > 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.
> 
> 32x32 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.
> 
> 32x16 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.
> 
> 16x16 for the full page

Yeah I can't compute. But the real trouble was that I thought this was in
pixels, but it's in bytes ;-) I clarified that later on and r-b with these
numbers here.
-Daniel

> 
> > 
> > Otherwise looks good to me, Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux