[PATCH 1/3] drm/i915: Fix scaling check for 90/270 degree plane rotation

[PATCH 1/3] drm/i915: Fix scaling check for 90/270 degree plane rotation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Starting from commit b63a16f6cd89 ("drm/i915: Compute display surface
offset in the plane check hook for SKL+") we've already rotated the src
coordinates by 270 degrees by the time we check if a scaler is needed
or not, so we must not account for the rotation a second time.
Previously we did these steps in the opposite order and hence the
scaler check had to deal with rotation itself. The double rotation
handling causes us to enable a scaler pretty much every time 90/270
degree plane rotation is requested, leading to fuzzier fonts and whatnot.

v2: s/unsigned/unsigned int/ to appease checkpatch

Cc: stable@vger.kernel.org
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81baa5a9780c..09c9995cafe6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4609,7 +4609,7 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 
 static int
 skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
-		  unsigned scaler_user, int *scaler_id, unsigned int rotation,
+		  unsigned int scaler_user, int *scaler_id,
 		  int src_w, int src_h, int dst_w, int dst_h)
 {
 	struct intel_crtc_scaler_state *scaler_state =
@@ -4618,9 +4618,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		to_intel_crtc(crtc_state->base.crtc);
 	int need_scaling;
 
-	need_scaling = drm_rotation_90_or_270(rotation) ?
-		(src_h != dst_w || src_w != dst_h):
-		(src_w != dst_w || src_h != dst_h);
+	/*
+	 * Src coordinates are already rotated by 270 degrees for
+	 * the 90/270 degree plane rotation cases (to match the
+	 * GTT mapping), hence no need to account for rotation here.
+	 */
+	need_scaling = src_w != dst_w || src_h != dst_h;
 
 	/*
 	 * if plane is being disabled or scaler is no more required or force detach
@@ -4682,7 +4685,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
 	const struct drm_display_mode *adjusted_mode = &state->base.adjusted_mode;
 
 	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
-		&state->scaler_state.scaler_id, DRM_ROTATE_0,
+		&state->scaler_state.scaler_id,
 		state->pipe_src_w, state->pipe_src_h,
 		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
 }
@@ -4711,7 +4714,6 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	ret = skl_update_scaler(crtc_state, force_detach,
 				drm_plane_index(&intel_plane->base),
 				&plane_state->scaler_id,
-				plane_state->base.rotation,
 				drm_rect_width(&plane_state->base.src) >> 16,
 				drm_rect_height(&plane_state->base.src) >> 16,
 				drm_rect_width(&plane_state->base.dst),
-- 
2.10.2

[PATCH 2/3] drm/i915: Fix SKL+ watermarks for 90/270 rotation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

skl_check_plane_surface() already rotates the clipped plane source
coordinates to match the scanout direction because that's the way
the GTT mapping is set up. Thus we no longer need to rotate the
coordinates in the watermark code.

For cursors we use the non-clipped coordinates which are not rotated
appropriately, but that doesn't actually matter since cursors don't
even support 90/270 degree rotation.

Cc: stable@vger.kernel.org
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 570bd603f401..6b1caf9ed3ca 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3373,20 +3373,26 @@ skl_plane_downscale_amount(const struct intel_crtc_state *cstate,
 
 	/* n.b., src is 16.16 fixed point, dst is whole integer */
 	if (plane->id == PLANE_CURSOR) {
+		/*
+		 * Cursors only support 0/180 degree rotation,
+		 * hence no need to account for rotation here.
+		 */
 		src_w = pstate->base.src_w;
 		src_h = pstate->base.src_h;
 		dst_w = pstate->base.crtc_w;
 		dst_h = pstate->base.crtc_h;
 	} else {
+		/*
+		 * Src coordinates are already rotated by 270 degrees for
+		 * the 90/270 degree plane rotation cases (to match the
+		 * GTT mapping), hence no need to account for rotation here.
+		 */
 		src_w = drm_rect_width(&pstate->base.src);
 		src_h = drm_rect_height(&pstate->base.src);
 		dst_w = drm_rect_width(&pstate->base.dst);
 		dst_h = drm_rect_height(&pstate->base.dst);
 	}
 
-	if (drm_rotation_90_or_270(pstate->base.rotation))
-		swap(dst_w, dst_h);
-
 	downscale_h = max(src_h / dst_h, (uint32_t)DRM_PLANE_HELPER_NO_SCALING);
 	downscale_w = max(src_w / dst_w, (uint32_t)DRM_PLANE_HELPER_NO_SCALING);
 
@@ -3417,12 +3423,14 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	if (y && format != DRM_FORMAT_NV12)
 		return 0;
 
+	/*
+	 * Src coordinates are already rotated by 270 degrees for
+	 * the 90/270 degree plane rotation cases (to match the
+	 * GTT mapping), hence no need to account for rotation here.
+	 */
 	width = drm_rect_width(&intel_pstate->base.src) >> 16;
 	height = drm_rect_height(&intel_pstate->base.src) >> 16;
 
-	if (drm_rotation_90_or_270(pstate->rotation))
-		swap(width, height);
-
 	/* for planar format */
 	if (format == DRM_FORMAT_NV12) {
 		if (y)  /* y-plane data rate */
@@ -3505,12 +3513,14 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
 	    fb->modifier != I915_FORMAT_MOD_Yf_TILED)
 		return 8;
 
+	/*
+	 * Src coordinates are already rotated by 270 degrees for
+	 * the 90/270 degree plane rotation cases (to match the
+	 * GTT mapping), hence no need to account for rotation here.
+	 */
 	src_w = drm_rect_width(&intel_pstate->base.src) >> 16;
 	src_h = drm_rect_height(&intel_pstate->base.src) >> 16;
 
-	if (drm_rotation_90_or_270(pstate->rotation))
-		swap(src_w, src_h);
-
 	/* Halve UV plane width and height for NV12 */
 	if (fb->format->format == DRM_FORMAT_NV12 && !y) {
 		src_w /= 2;
@@ -3794,13 +3804,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		width = intel_pstate->base.crtc_w;
 		height = intel_pstate->base.crtc_h;
 	} else {
+		/*
+		 * Src coordinates are already rotated by 270 degrees for
+		 * the 90/270 degree plane rotation cases (to match the
+		 * GTT mapping), hence no need to account for rotation here.
+		 */
 		width = drm_rect_width(&intel_pstate->base.src) >> 16;
 		height = drm_rect_height(&intel_pstate->base.src) >> 16;
 	}
 
-	if (drm_rotation_90_or_270(pstate->rotation))
-		swap(width, height);
-
 	cpp = fb->format->cpp[0];
 	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate, intel_pstate);
 
-- 
2.10.2

[PATCH 3/3] drm/i915: Fix 90/270 rotated coordinates for FBC

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The clipped src coordinates have already been rotated by 270 degrees for
when the plane rotation is 90/270 degrees, hence the FBC code should no
longer swap the width and height.

Cc: stable@vger.kernel.org
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index ded2add18b26..d93c58410bff 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -82,20 +82,10 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
 static void intel_fbc_get_plane_source_size(struct intel_fbc_state_cache *cache,
 					    int *width, int *height)
 {
-	int w, h;
-
-	if (drm_rotation_90_or_270(cache->plane.rotation)) {
-		w = cache->plane.src_h;
-		h = cache->plane.src_w;
-	} else {
-		w = cache->plane.src_w;
-		h = cache->plane.src_h;
-	}
-
 	if (width)
-		*width = w;
+		*width = cache->plane.src_w;
 	if (height)
-		*height = h;
+		*height = cache->plane.src_h;
 }
 
 static int intel_fbc_calculate_cfb_size(struct drm_i915_private *dev_priv,
@@ -746,6 +736,11 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 		cache->crtc.hsw_bdw_pixel_rate = crtc_state->pixel_rate;
 
 	cache->plane.rotation = plane_state->base.rotation;
+	/*
+	 * Src coordinates are already rotated by 270 degrees for
+	 * the 90/270 degree plane rotation cases (to match the
+	 * GTT mapping), hence no need to account for rotation here.
+	 */
 	cache->plane.src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	cache->plane.src_h = drm_rect_height(&plane_state->base.src) >> 16;
 	cache->plane.visible = plane_state->base.visible;
-- 
2.10.2

Re: [PATCH 3/3] drm/i915: Fix 90/270 rotated coordinates for FBC

[Paulo Zanoni]

Em Sex, 2017-03-31 às 21:00 +0300, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The clipped src coordinates have already been rotated by 270 degrees
> for
> when the plane rotation is 90/270 degrees, hence the FBC code should
> no
> longer swap the width and height.

I've never payed too much attention to rotation, but based on the
mentioned commits, what's said on the messages and my understanding of
the code, this looks sane, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

And in case someone suggests to just kill
intel_fbc_get_plane_source_size(), I'd like to point that "plane source
size" is wording used by our spec and there's a nice comment explaining
what exactly it's supposed to be, so I'd be in favor of keeping it.

Super bonus point if you end up writing some sort of rotation test for
kms_frontbuffer_tracking or kms_fbc_crc. The problem is that I'm not
entirely too sure about how much the current code structure for those
tests is ready to easily support such a test with minimal efforts.
Needs to be studied.

> 
> Cc: stable@vger.kernel.org
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the
> plane check hook for SKL+")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index ded2add18b26..d93c58410bff 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -82,20 +82,10 @@ static unsigned int
> get_crtc_fence_y_offset(struct intel_crtc *crtc)
>  static void intel_fbc_get_plane_source_size(struct
> intel_fbc_state_cache *cache,
>  					    int *width, int *height)
>  {
> -	int w, h;
> -
> -	if (drm_rotation_90_or_270(cache->plane.rotation)) {
> -		w = cache->plane.src_h;
> -		h = cache->plane.src_w;
> -	} else {
> -		w = cache->plane.src_w;
> -		h = cache->plane.src_h;
> -	}
> -
>  	if (width)
> -		*width = w;
> +		*width = cache->plane.src_w;
>  	if (height)
> -		*height = h;
> +		*height = cache->plane.src_h;
>  }
>  
>  static int intel_fbc_calculate_cfb_size(struct drm_i915_private
> *dev_priv,
> @@ -746,6 +736,11 @@ static void intel_fbc_update_state_cache(struct
> intel_crtc *crtc,
>  		cache->crtc.hsw_bdw_pixel_rate = crtc_state-
> >pixel_rate;
>  
>  	cache->plane.rotation = plane_state->base.rotation;
> +	/*
> +	 * Src coordinates are already rotated by 270 degrees for
> +	 * the 90/270 degree plane rotation cases (to match the
> +	 * GTT mapping), hence no need to account for rotation here.
> +	 */
>  	cache->plane.src_w = drm_rect_width(&plane_state->base.src)
> >> 16;
>  	cache->plane.src_h = drm_rect_height(&plane_state->base.src) 
> >> 16;
>  	cache->plane.visible = plane_state->base.visible;

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix 90/270 rotated coordinates for FBC

[Tvrtko Ursulin]

On 31/03/2017 19:00, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The clipped src coordinates have already been rotated by 270 degrees for
> when the plane rotation is 90/270 degrees, hence the FBC code should no
> longer swap the width and height.
>
> Cc: stable@vger.kernel.org
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index ded2add18b26..d93c58410bff 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -82,20 +82,10 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
>  static void intel_fbc_get_plane_source_size(struct intel_fbc_state_cache *cache,
>  					    int *width, int *height)
>  {
> -	int w, h;
> -
> -	if (drm_rotation_90_or_270(cache->plane.rotation)) {
> -		w = cache->plane.src_h;
> -		h = cache->plane.src_w;
> -	} else {
> -		w = cache->plane.src_w;
> -		h = cache->plane.src_h;
> -	}
> -
>  	if (width)
> -		*width = w;
> +		*width = cache->plane.src_w;
>  	if (height)
> -		*height = h;
> +		*height = cache->plane.src_h;
>  }
>
>  static int intel_fbc_calculate_cfb_size(struct drm_i915_private *dev_priv,
> @@ -746,6 +736,11 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  		cache->crtc.hsw_bdw_pixel_rate = crtc_state->pixel_rate;
>
>  	cache->plane.rotation = plane_state->base.rotation;
> +	/*
> +	 * Src coordinates are already rotated by 270 degrees for
> +	 * the 90/270 degree plane rotation cases (to match the
> +	 * GTT mapping), hence no need to account for rotation here.
> +	 */
>  	cache->plane.src_w = drm_rect_width(&plane_state->base.src) >> 16;
>  	cache->plane.src_h = drm_rect_height(&plane_state->base.src) >> 16;
>  	cache->plane.visible = plane_state->base.visible;
>

For the series:

Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko