Re: [PATCH v3 08/18] drm/i915: Pass 90/270 vs. 0/180 rotation info for intel_gen4_compute_page_offset()

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

On Mon, Jan 25, 2016 at 06:30:17PM +0100, Daniel Vetter wrote:
> On Wed, Jan 20, 2016 at 09:05:29PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The page aligned surface address calculation needs to know which way
> > things are rotated. The contract now says that the caller must pass the
> > rotate x/y coordinates, as well as the tile_height aligned stride in
> > the tile_height direction. This will make it fairly simple to deal with
> > 90/270 degree rotation on SKL+ where we have to deal with the rotated
> > view into the GTT.
> > 
> > v2: Pass rotation instead of bool even thoughwe only care about 0/180 vs. 90/270
> > v3: Introduce intel_tile_dims(), and don't mix up different units so much
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2)
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 18 +++++-----
> >  3 files changed, 59 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cfd52ea68e34..bda3224021b2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2269,6 +2269,18 @@ unsigned int intel_tile_height(const struct drm_i915_private *dev_priv,
> >  			intel_tile_width(dev_priv, fb_modifier, cpp);
> >  }
> >  
> > +/* Return the tile dimensions in pixel units */
> > +static void intel_tile_dims(const struct drm_i915_private *dev_priv,
> > +			    unsigned int *tile_width,
> > +			    unsigned int *tile_height,
> > +			    uint64_t fb_modifier,
> > +			    unsigned int cpp)
> > +{
> > +	*tile_width = intel_tile_width(dev_priv, fb_modifier, cpp);
> > +	*tile_height = intel_tile_size(dev_priv) / *tile_width;
> > +	*tile_width /= cpp;
> 
> Seems like my suggestion to use tile_width in cpp wasn't that awesome, at
> least we still have a big confusion right here. Not sure what to do,
> especially since these kind of changes are super-painful to review. Could
> we perhaps go back to the old versions and untangle this mess afterwards?
> Iirc I've follow-up with r-b tags to all patches in the old series for
> that approach.

Hmm. What did I do differently in the old approach again? Other than not
having the confusion isolated to this one function. One step forward and
two back doens't sound productive. Plus I have some more patches on top
of this one already, so not pleasant at all.

Anyway, to avoid more confusion I suppose one thing that could be done
is this:
- intel_tile_width
+ intel_tile_width_bytes

intel_tile_dims()
{
	tile_width_bytes = intel_tile_width_bytes()
	*tile_height = tile_size / tile_width_bytes;
	*tile_width = tile_width_bytes / cpp;
}

> 
> No idea really what would be best here ...
> -Daniel
> 
> > +}
> > +
> >  unsigned int
> >  intel_fb_align_height(struct drm_device *dev, unsigned int height,
> >  		      uint32_t pixel_format, uint64_t fb_modifier)
> > @@ -2306,19 +2318,19 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >  	tile_size = intel_tile_size(dev_priv);
> >  
> >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > -	tile_width = intel_tile_width(dev_priv, fb->modifier[0], cpp);
> > -	tile_height = tile_size / tile_width;
> > +	intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > +			fb->modifier[0], cpp);
> >  
> > -	info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_width);
> > +	info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_width * cpp);
> >  	info->height_pages = DIV_ROUND_UP(fb->height, tile_height);
> >  	info->size = info->width_pages * info->height_pages * tile_size;
> >  
> >  	if (info->pixel_format == DRM_FORMAT_NV12) {
> >  		cpp = drm_format_plane_cpp(fb->pixel_format, 1);
> > -		tile_width = intel_tile_width(dev_priv, fb->modifier[1], cpp);
> > -		tile_height = tile_size / tile_width;
> > +		intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > +				fb->modifier[1], cpp);
> >  
> > -		info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_width);
> > +		info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_width * cpp);
> >  		info->height_pages_uv = DIV_ROUND_UP(fb->height / 2, tile_height);
> >  		info->size_uv = info->width_pages_uv * info->height_pages_uv * tile_size;
> >  	}
> > @@ -2446,29 +2458,43 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> >  	i915_gem_object_unpin_from_display_plane(obj, &view);
> >  }
> >  
> > -/* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
> > - * is assumed to be a power-of-two. */
> > +/*
> > + * Computes the linear offset to the base tile and adjusts
> > + * x, y. bytes per pixel is assumed to be a power-of-two.
> > + *
> > + * In the 90/270 rotated case, x and y are assumed
> > + * to be already rotated to match the rotated GTT view, and
> > + * pitch is the tile_height aligned framebuffer height.
> > + */
> >  u32 intel_compute_tile_offset(struct drm_i915_private *dev_priv,
> >  			      int *x, int *y,
> >  			      uint64_t fb_modifier,
> >  			      unsigned int cpp,
> > -			      unsigned int pitch)
> > +			      unsigned int pitch,
> > +			      unsigned int rotation)
> >  {
> >  	if (fb_modifier != DRM_FORMAT_MOD_NONE) {
> >  		unsigned int tile_size, tile_width, tile_height;
> > -		unsigned int tile_rows, tiles;
> > +		unsigned int tile_rows, tiles, pitch_tiles;
> >  
> >  		tile_size = intel_tile_size(dev_priv);
> > -		tile_width = intel_tile_width(dev_priv, fb_modifier, cpp);
> > -		tile_height = tile_size / tile_width;
> > +		intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > +				fb_modifier, cpp);
> > +
> > +		if (intel_rotation_90_or_270(rotation)) {
> > +			pitch_tiles = pitch / tile_height;
> > +			swap(tile_width, tile_height);
> > +		} else {
> > +			pitch_tiles = pitch / (tile_width * cpp);
> > +		}
> >  
> >  		tile_rows = *y / tile_height;
> >  		*y %= tile_height;
> >  
> > -		tiles = *x / (tile_width/cpp);
> > -		*x %= tile_width/cpp;
> > +		tiles = *x / tile_width;
> > +		*x %= tile_width;
> >  
> > -		return tile_rows * pitch * tile_height + tiles * tile_size;
> > +		return (tile_rows * pitch_tiles + tiles) * tile_size;
> >  	} else {
> >  		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
> >  		unsigned int offset;
> > @@ -2709,6 +2735,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> >  	u32 linear_offset;
> >  	u32 dspcntr;
> >  	i915_reg_t reg = DSPCNTR(plane);
> > +	unsigned int rotation = plane_state->base.rotation;
> >  	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	int x = plane_state->src.x1 >> 16;
> >  	int y = plane_state->src.y1 >> 16;
> > @@ -2775,13 +2802,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> >  		intel_crtc->dspaddr_offset =
> >  			intel_compute_tile_offset(dev_priv, &x, &y,
> >  						  fb->modifier[0], cpp,
> > -						  fb->pitches[0]);
> > +						  fb->pitches[0], rotation);
> >  		linear_offset -= intel_crtc->dspaddr_offset;
> >  	} else {
> >  		intel_crtc->dspaddr_offset = linear_offset;
> >  	}
> >  
> > -	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +	if (rotation == BIT(DRM_ROTATE_180)) {
> >  		dspcntr |= DISPPLANE_ROTATE_180;
> >  
> >  		x += (crtc_state->pipe_src_w - 1);
> > @@ -2839,6 +2866,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> >  	u32 linear_offset;
> >  	u32 dspcntr;
> >  	i915_reg_t reg = DSPCNTR(plane);
> > +	unsigned int rotation = plane_state->base.rotation;
> >  	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	int x = plane_state->src.x1 >> 16;
> >  	int y = plane_state->src.y1 >> 16;
> > @@ -2882,9 +2910,9 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> >  	intel_crtc->dspaddr_offset =
> >  		intel_compute_tile_offset(dev_priv, &x, &y,
> >  					  fb->modifier[0], cpp,
> > -					  fb->pitches[0]);
> > +					  fb->pitches[0], rotation);
> >  	linear_offset -= intel_crtc->dspaddr_offset;
> > -	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +	if (rotation == BIT(DRM_ROTATE_180)) {
> >  		dspcntr |= DISPPLANE_ROTATE_180;
> >  
> >  		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f620023ed134..223693dbfe7c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1176,7 +1176,8 @@ u32 intel_compute_tile_offset(struct drm_i915_private *dev_priv,
> >  			      int *x, int *y,
> >  			      uint64_t fb_modifier,
> >  			      unsigned int cpp,
> > -			      unsigned int pitch);
> > +			      unsigned int pitch,
> > +			      unsigned int rotation);
> >  void intel_prepare_reset(struct drm_device *dev);
> >  void intel_finish_reset(struct drm_device *dev);
> >  void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index a2582c455b36..7dc2b8b2a4ac 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -193,7 +193,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	u32 surf_addr;
> >  	u32 tile_height, plane_offset, plane_size;
> > -	unsigned int rotation;
> > +	unsigned int rotation = plane_state->base.rotation;
> >  	int x_offset, y_offset;
> >  	int crtc_x = plane_state->dst.x1;
> >  	int crtc_y = plane_state->dst.y1;
> > @@ -213,7 +213,6 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
> >  
> > -	rotation = plane_state->base.rotation;
> >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> >  
> >  	stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > @@ -351,6 +350,7 @@ vlv_update_plane(struct drm_plane *dplane,
> >  	int plane = intel_plane->plane;
> >  	u32 sprctl;
> >  	u32 sprsurf_offset, linear_offset;
> > +	unsigned int rotation = dplane->state->rotation;
> >  	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	int crtc_x = plane_state->dst.x1;
> > @@ -425,10 +425,10 @@ vlv_update_plane(struct drm_plane *dplane,
> >  	linear_offset = y * fb->pitches[0] + x * cpp;
> >  	sprsurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> >  						   fb->modifier[0], cpp,
> > -						   fb->pitches[0]);
> > +						   fb->pitches[0], rotation);
> >  	linear_offset -= sprsurf_offset;
> >  
> > -	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +	if (rotation == BIT(DRM_ROTATE_180)) {
> >  		sprctl |= SP_ROTATE_180;
> >  
> >  		x += src_w;
> > @@ -493,6 +493,7 @@ ivb_update_plane(struct drm_plane *plane,
> >  	enum pipe pipe = intel_plane->pipe;
> >  	u32 sprctl, sprscale = 0;
> >  	u32 sprsurf_offset, linear_offset;
> > +	unsigned int rotation = plane_state->base.rotation;
> >  	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	int crtc_x = plane_state->dst.x1;
> > @@ -558,10 +559,10 @@ ivb_update_plane(struct drm_plane *plane,
> >  	linear_offset = y * fb->pitches[0] + x * cpp;
> >  	sprsurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> >  						   fb->modifier[0], cpp,
> > -						   fb->pitches[0]);
> > +						   fb->pitches[0], rotation);
> >  	linear_offset -= sprsurf_offset;
> >  
> > -	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +	if (rotation == BIT(DRM_ROTATE_180)) {
> >  		sprctl |= SPRITE_ROTATE_180;
> >  
> >  		/* HSW and BDW does this automagically in hardware */
> > @@ -634,6 +635,7 @@ ilk_update_plane(struct drm_plane *plane,
> >  	int pipe = intel_plane->pipe;
> >  	u32 dvscntr, dvsscale;
> >  	u32 dvssurf_offset, linear_offset;
> > +	unsigned int rotation = plane_state->base.rotation;
> >  	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	int crtc_x = plane_state->dst.x1;
> > @@ -695,10 +697,10 @@ ilk_update_plane(struct drm_plane *plane,
> >  	linear_offset = y * fb->pitches[0] + x * cpp;
> >  	dvssurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> >  						   fb->modifier[0], cpp,
> > -						   fb->pitches[0]);
> > +						   fb->pitches[0], rotation);
> >  	linear_offset -= dvssurf_offset;
> >  
> > -	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +	if (rotation == BIT(DRM_ROTATE_180)) {
> >  		dvscntr |= DVS_ROTATE_180;
> >  
> >  		x += src_w;
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > 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

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