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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 14, 2015 at 07:29:00PM +0300, 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
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
>  drivers/gpu/drm/i915/intel_sprite.c  | 15 +++++----
>  3 files changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 75be66b..bd55d06 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2451,13 +2451,50 @@ 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. */
> +/*
> + * Return the tile dimensions in pixel units matching
> + * the specified rotation angle.
> + */
> +static void intel_rotate_tile_dims(unsigned int *tile_width,
> +				   unsigned int *tile_height,
> +				   unsigned int *pitch,
> +				   unsigned int cpp,
> +				   unsigned int rotation)

A rotation enum would be nice, so that we can employ sparse to check it.
That'd work since sparse treats enums as bitfields, but we'd need to
add names for the BIT(DRM_ROTATION_*) variants. Just an aside.

> +{
> +	if (intel_rotation_90_or_270(rotation)) {
> +		WARN_ON(*pitch % *tile_height);
> +
> +		/* pixel units please */
> +		*tile_width /= cpp;

Ok, something dawns on me now about tile_width ... it's in bytes, I
somehow thought it's pixels. And this function doing a behind-the-scenes
conversions from bytes to pixels is a bit tricky.

Should we have separate tile_pitch and tile_width to unconfuse this?
Generally foo_width in the modeset code is mostly pixels ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on this one (and I
retract my earlier review on tile_width), but I'd like something clearer
here in the end ...

Cheers, Daniel

> +
> +		/*
> +		 * Coordinate space is rotated, orient
> +		 * our tile dimensions the same way
> +		 */
> +		swap(*tile_width, *tile_height);
> +	} else {
> +		WARN_ON(*pitch % *tile_width);
> +
> +		/* pixel units please */
> +		*tile_width /= cpp;
> +		*pitch /= cpp;
> +	}
> +}
> +
> +/*
> + * 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.
> + */
>  unsigned long intel_compute_page_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;
> @@ -2467,13 +2504,16 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv,
>  		tile_width = intel_tile_width(dev_priv, fb_modifier, cpp);
>  		tile_height = tile_size / tile_width;
>  
> +		intel_rotate_tile_dims(&tile_width, &tile_height,
> +				       &pitch, cpp, rotation);
> +
>  		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 / tile_width) + tiles) * tile_size;
>  	} else {
>  		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
>  		unsigned int offset;
> @@ -2685,6 +2725,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	bool visible = to_intel_plane_state(primary->state)->visible;
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
> +	unsigned int rotation;
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg = DSPCNTR(plane);
> @@ -2704,6 +2745,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	if (WARN_ON(obj == NULL))
>  		return;
>  
> +	rotation = crtc->primary->state->rotation;
>  	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
> @@ -2768,7 +2810,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  		intel_crtc->dspaddr_offset =
>  			intel_compute_page_offset(dev_priv, &x, &y,
>  						  fb->modifier[0], pixel_size,
> -						  fb->pitches[0]);
> +						  fb->pitches[0], rotation);
>  		linear_offset -= intel_crtc->dspaddr_offset;
>  	} else {
>  		intel_crtc->dspaddr_offset = linear_offset;
> @@ -2814,6 +2856,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	bool visible = to_intel_plane_state(primary->state)->visible;
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
> +	unsigned int rotation;
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg = DSPCNTR(plane);
> @@ -2830,6 +2873,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	if (WARN_ON(obj == NULL))
>  		return;
>  
> +	rotation = crtc->primary->state->rotation;
>  	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
> @@ -2872,9 +2916,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	intel_crtc->dspaddr_offset =
>  		intel_compute_page_offset(dev_priv, &x, &y,
>  					  fb->modifier[0], pixel_size,
> -					  fb->pitches[0]);
> +					  fb->pitches[0], rotation);
>  	linear_offset -= intel_crtc->dspaddr_offset;
> -	if (crtc->primary->state->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 a12ac95..ed47ca3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1139,7 +1139,8 @@ unsigned long intel_compute_page_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 6614adb..8eaebce 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -354,6 +354,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  	u32 sprctl;
> +	unsigned int rotation = dplane->state->rotation;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	const struct drm_intel_sprite_colorkey *key =
> @@ -422,10 +423,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	linear_offset = y * fb->pitches[0] + x * pixel_size;
>  	sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y,
>  						   fb->modifier[0], pixel_size,
> -						   fb->pitches[0]);
> +						   fb->pitches[0], rotation);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SP_ROTATE_180;
>  
>  		x += src_w;
> @@ -491,6 +492,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	enum pipe pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
> +	unsigned int rotation = plane->state->rotation;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	const struct drm_intel_sprite_colorkey *key =
> @@ -554,10 +556,10 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	linear_offset = y * fb->pitches[0] + x * pixel_size;
>  	sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y,
>  						   fb->modifier[0], pixel_size,
> -						   fb->pitches[0]);
> +						   fb->pitches[0], rotation);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SPRITE_ROTATE_180;
>  
>  		/* HSW and BDW does this automagically in hardware */
> @@ -631,6 +633,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int pipe = intel_plane->pipe;
> +	unsigned int rotation = plane->state->rotation;
>  	unsigned long dvssurf_offset, linear_offset;
>  	u32 dvscntr, dvsscale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> @@ -691,10 +694,10 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	linear_offset = y * fb->pitches[0] + x * pixel_size;
>  	dvssurf_offset = intel_compute_page_offset(dev_priv, &x, &y,
>  						   fb->modifier[0], pixel_size,
> -						   fb->pitches[0]);
> +						   fb->pitches[0], rotation);
>  	linear_offset -= dvssurf_offset;
>  
> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (rotation == BIT(DRM_ROTATE_180)) {
>  		dvscntr |= DVS_ROTATE_180;
>  
>  		x += src_w;
> -- 
> 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