diff mbox series

[-fixes,3/3] drm/i915: Account for scale factor when calculating initial phase

Message ID 20181114114925.6818-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [-fixes,1/3] drm/i915: Move programming plane scaler to its own function. | expand

Commit Message

Ville Syrjälä Nov. 14, 2018, 11:49 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To get the initial phase correct we need to account for the scale
factor as well. I forgot this initially and was mostly looking at
heavily upscaled content where the minor difference between -0.5
and the proper initial phase was not readily apparent.

And let's toss in a comment that tries to explain the formula
a little bit.

v2: The initial phase upper limit is 1.5, not 24.0!

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 0a59952b24e2 ("drm/i915: Configure SKL+ scaler initial phase correctly")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20181029181820.21956-1-ville.syrjala@linux.intel.com
Tested-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> #irc
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> #irc
(cherry picked from commit e7a278a329dd8aa2c70c564849f164cb5673689c)
---
 drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 20 +++++++++----
 3 files changed, 57 insertions(+), 10 deletions(-)

Comments

Ville Syrjälä Nov. 14, 2018, 1:28 p.m. UTC | #1 
On Wed, Nov 14, 2018 at 01:49:25PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To get the initial phase correct we need to account for the scale
> factor as well. I forgot this initially and was mostly looking at
> heavily upscaled content where the minor difference between -0.5
> and the proper initial phase was not readily apparent.
> 
> And let's toss in a comment that tries to explain the formula
> a little bit.
> 
> v2: The initial phase upper limit is 1.5, not 24.0!
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 0a59952b24e2 ("drm/i915: Configure SKL+ scaler initial phase correctly")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20181029181820.21956-1-ville.syrjala@linux.intel.com
> Tested-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> #irc
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> #irc
> (cherry picked from commit e7a278a329dd8aa2c70c564849f164cb5673689c)
> ---
>  drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  | 20 +++++++++----
>  3 files changed, 57 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 23d8008a93bb..636738c04eb2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4850,8 +4850,31 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>   * chroma samples for both of the luma samples, and thus we don't
>   * actually get the expected MPEG2 chroma siting convention :(
>   * The same behaviour is observed on pre-SKL platforms as well.
> + *
> + * Theory behind the formula (note that we ignore sub-pixel
> + * source coordinates):
> + * s = source sample position
> + * d = destination sample position
> + *
> + * Downscaling 4:1:
> + * -0.5
> + * | 0.0
> + * | |     1.5 (initial phase)
> + * | |     |
> + * v v     v
> + * | s | s | s | s |
> + * |       d       |
> + *
> + * Upscaling 1:4:
> + * -0.5
> + * | -0.375 (initial phase)
> + * | |     0.0
> + * | |     |
> + * v v     v
> + * |       s       |
> + * | d | d | d | d |
>   */
> -u16 skl_scaler_calc_phase(int sub, bool chroma_cosited)
> +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_cosited)
>  {
>  	int phase = -0x8000;
>  	u16 trip = 0;
> @@ -4859,6 +4882,15 @@ u16 skl_scaler_calc_phase(int sub, bool chroma_cosited)
>  	if (chroma_cosited)
>  		phase += (sub - 1) * 0x8000 / sub;
>  
> +	phase += scale / (2 * sub);
> +
> +	/*
> +	 * Hardware initial phase limited to [-0.5:1.5].
> +	 * Since the max hardware scale factor is 3.0, we
> +	 * should never actually excdeed 1.0 here.
> +	 */
> +	WARN_ON(phase < -0x8000 || phase > 0x18000);
> +
>  	if (phase < 0)
>  		phase = 0x10000 + phase;
>  	else
> @@ -5067,13 +5099,20 @@ static void skylake_pfit_enable(struct intel_crtc *crtc)
>  
>  	if (crtc->config->pch_pfit.enabled) {
>  		u16 uv_rgb_hphase, uv_rgb_vphase;
> +		int pfit_w, pfit_h, hscale, vscale;
>  		int id;
>  
>  		if (WARN_ON(crtc->config->scaler_state.scaler_id < 0))
>  			return;
>  
> -		uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> -		uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> +		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> +		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> +
> +		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> +		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;

Bah. This doesn't build. I'll fix and resend, and make doubly sure to
build test this time. Sorry for the noise.

> +
> +		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> +		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
>  
>  		id = scaler_state->scaler_id;
>  		I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f8dc84b2d2d3..8b298e5f012d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1646,7 +1646,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
>  				  struct intel_crtc_state *crtc_state);
>  
> -u16 skl_scaler_calc_phase(int sub, bool chroma_center);
> +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center);
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
>  int skl_max_scale(const struct intel_crtc_state *crtc_state,
>  		  u32 pixel_format);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fa7eaace5f92..d3090a7537bb 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -318,22 +318,30 @@ skl_program_scaler(struct intel_plane *plane,
>  	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
>  	u16 y_hphase, uv_rgb_hphase;
>  	u16 y_vphase, uv_rgb_vphase;
> +	int hscale, vscale;
> +
> +	hscale = drm_rect_calc_hscale(&plane_state->base.src,
> +				      &plane_state->base.dst,
> +				      0, INT_MAX);
> +	vscale = drm_rect_calc_vscale(&plane_state->base.src,
> +				      &plane_state->base.dst,
> +				      0, INT_MAX);
>  
>  	/* TODO: handle sub-pixel coordinates */
>  	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> -		y_hphase = skl_scaler_calc_phase(1, false);
> -		y_vphase = skl_scaler_calc_phase(1, false);
> +		y_hphase = skl_scaler_calc_phase(1, hscale, false);
> +		y_vphase = skl_scaler_calc_phase(1, vscale, false);
>  
>  		/* MPEG2 chroma siting convention */
> -		uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> -		uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> +		uv_rgb_hphase = skl_scaler_calc_phase(2, hscale, true);
> +		uv_rgb_vphase = skl_scaler_calc_phase(2, vscale, false);
>  	} else {
>  		/* not used */
>  		y_hphase = 0;
>  		y_vphase = 0;
>  
> -		uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> -		uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> +		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> +		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
>  	}
>  
>  	I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> -- 
> 2.18.1
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 23d8008a93bb..636738c04eb2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4850,8 +4850,31 @@  static void cpt_verify_modeset(struct drm_device *dev, int pipe)
  * chroma samples for both of the luma samples, and thus we don't
  * actually get the expected MPEG2 chroma siting convention :(
  * The same behaviour is observed on pre-SKL platforms as well.
+ *
+ * Theory behind the formula (note that we ignore sub-pixel
+ * source coordinates):
+ * s = source sample position
+ * d = destination sample position
+ *
+ * Downscaling 4:1:
+ * -0.5
+ * | 0.0
+ * | |     1.5 (initial phase)
+ * | |     |
+ * v v     v
+ * | s | s | s | s |
+ * |       d       |
+ *
+ * Upscaling 1:4:
+ * -0.5
+ * | -0.375 (initial phase)
+ * | |     0.0
+ * | |     |
+ * v v     v
+ * |       s       |
+ * | d | d | d | d |
  */
-u16 skl_scaler_calc_phase(int sub, bool chroma_cosited)
+u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_cosited)
 {
 	int phase = -0x8000;
 	u16 trip = 0;
@@ -4859,6 +4882,15 @@  u16 skl_scaler_calc_phase(int sub, bool chroma_cosited)
 	if (chroma_cosited)
 		phase += (sub - 1) * 0x8000 / sub;
 
+	phase += scale / (2 * sub);
+
+	/*
+	 * Hardware initial phase limited to [-0.5:1.5].
+	 * Since the max hardware scale factor is 3.0, we
+	 * should never actually excdeed 1.0 here.
+	 */
+	WARN_ON(phase < -0x8000 || phase > 0x18000);
+
 	if (phase < 0)
 		phase = 0x10000 + phase;
 	else
@@ -5067,13 +5099,20 @@  static void skylake_pfit_enable(struct intel_crtc *crtc)
 
 	if (crtc->config->pch_pfit.enabled) {
 		u16 uv_rgb_hphase, uv_rgb_vphase;
+		int pfit_w, pfit_h, hscale, vscale;
 		int id;
 
 		if (WARN_ON(crtc->config->scaler_state.scaler_id < 0))
 			return;
 
-		uv_rgb_hphase = skl_scaler_calc_phase(1, false);
-		uv_rgb_vphase = skl_scaler_calc_phase(1, false);
+		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
+		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
+
+		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
+		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
+
+		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
+		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
 
 		id = scaler_state->scaler_id;
 		I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f8dc84b2d2d3..8b298e5f012d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1646,7 +1646,7 @@  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
 				  struct intel_crtc_state *crtc_state);
 
-u16 skl_scaler_calc_phase(int sub, bool chroma_center);
+u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center);
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
 int skl_max_scale(const struct intel_crtc_state *crtc_state,
 		  u32 pixel_format);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fa7eaace5f92..d3090a7537bb 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -318,22 +318,30 @@  skl_program_scaler(struct intel_plane *plane,
 	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
 	u16 y_hphase, uv_rgb_hphase;
 	u16 y_vphase, uv_rgb_vphase;
+	int hscale, vscale;
+
+	hscale = drm_rect_calc_hscale(&plane_state->base.src,
+				      &plane_state->base.dst,
+				      0, INT_MAX);
+	vscale = drm_rect_calc_vscale(&plane_state->base.src,
+				      &plane_state->base.dst,
+				      0, INT_MAX);
 
 	/* TODO: handle sub-pixel coordinates */
 	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
-		y_hphase = skl_scaler_calc_phase(1, false);
-		y_vphase = skl_scaler_calc_phase(1, false);
+		y_hphase = skl_scaler_calc_phase(1, hscale, false);
+		y_vphase = skl_scaler_calc_phase(1, vscale, false);
 
 		/* MPEG2 chroma siting convention */
-		uv_rgb_hphase = skl_scaler_calc_phase(2, true);
-		uv_rgb_vphase = skl_scaler_calc_phase(2, false);
+		uv_rgb_hphase = skl_scaler_calc_phase(2, hscale, true);
+		uv_rgb_vphase = skl_scaler_calc_phase(2, vscale, false);
 	} else {
 		/* not used */
 		y_hphase = 0;
 		y_vphase = 0;
 
-		uv_rgb_hphase = skl_scaler_calc_phase(1, false);
-		uv_rgb_vphase = skl_scaler_calc_phase(1, false);
+		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
+		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
 	}
 
 	I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),