Re: [PATCH v2 5/8] drm/i915/gen11: Program the scalers correctly for planar formats, v3.

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

On Thu, Oct 18, 2018 at 01:51:31PM +0200, Maarten Lankhorst wrote:
> The first 3 planes (primary, sprite 0 and 1) have a dedicated chroma
> upsampler to upscale YUV420 to YUV444 and the scaler should only be
> used for upscaling. Because of this we shouldn't program the scalers
> in planar mode if NV12 and the chroma upsampler are used. Instead
> program the scalers like on normal planes.
> 
> Sprite 2 and 3 have no dedicated scaler, and need to program the
> selected Y plane in the scaler mode.
> 
> Changes since v1:
> - Make the comment less confusing.
> Changes since v2:
> - Fix checkpatch warning (Matt)
> - gen10- -> Pre-gen11 (Ville)
> - PS_SCALER_MODE_PACKED -> PS_SCALER_MODE_NORMAL. (Matt)
> - Add comment about scaler mode in intel_atomic_setup_scaler(). (Matt)
> - Rename need_scaling to need_scaler. (Matt)
> - Move the crtc need_scaling check to skl_update_scaler_crtc().
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I like it.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++-
>  drivers/gpu/drm/i915/intel_atomic.c  | 16 +++++++++---
>  drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
>  5 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 81f1c601987d..8a81e7c2fe25 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6819,7 +6819,7 @@ enum {
>  #define SKL_PS_SCALER_MODE_HQ  (1 << 28)
>  #define SKL_PS_SCALER_MODE_NV12 (2 << 28)
>  #define PS_SCALER_MODE_PLANAR (1 << 29)
> -#define PS_SCALER_MODE_PACKED (0 << 29)
> +#define PS_SCALER_MODE_NORMAL (0 << 29)
>  #define PS_PLANE_SEL_MASK  (7 << 25)
>  #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
>  #define PS_FILTER_MASK         (3 << 23)
> @@ -6836,6 +6836,8 @@ enum {
>  #define PS_VADAPT_MODE_LEAST_ADAPT (0 << 5)
>  #define PS_VADAPT_MODE_MOD_ADAPT   (1 << 5)
>  #define PS_VADAPT_MODE_MOST_ADAPT  (3 << 5)
> +#define PS_PLANE_Y_SEL_MASK  (7 << 5)
> +#define PS_PLANE_Y_SEL(plane) (((plane) + 1) << 5)
>  
>  #define _PS_PWR_GATE_1A     0x68160
>  #define _PS_PWR_GATE_2A     0x68260
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 20bfc89c652c..7238ab615548 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -233,13 +233,23 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  	    plane_state->base.fb->format->is_yuv &&
>  	    plane_state->base.fb->format->num_planes > 1) {
>  		if (INTEL_GEN(dev_priv) == 9 &&
> -		    !IS_GEMINILAKE(dev_priv))
> +		    !IS_GEMINILAKE(dev_priv)) {
>  			mode = SKL_PS_SCALER_MODE_NV12;
> -		else
> +		} else if (icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
> +			/*
> +			 * On gen11+'s HDR planes we only use the scaler for
> +			 * scaling. They have a dedicated chroma upsampler, so
> +			 * we don't need the scaler to upsample the UV plane.
> +			 */
> +			mode = PS_SCALER_MODE_NORMAL;
> +		} else {
>  			mode = PS_SCALER_MODE_PLANAR;
>  
> +			if (plane_state->linked_plane)
> +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> +		}
>  	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
> -		mode = PS_SCALER_MODE_PACKED;
> +		mode = PS_SCALER_MODE_NORMAL;
>  	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
>  		/*
>  		 * when only 1 scaler is in use on a pipe with 2 scalers
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cbb3fb1d5ad4..5997097177e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4836,8 +4836,7 @@ static int
>  skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  		  unsigned int scaler_user, int *scaler_id,
>  		  int src_w, int src_h, int dst_w, int dst_h,
> -		  bool plane_scaler_check,
> -		  uint32_t pixel_format)
> +		  const struct drm_format_info *format, bool need_scaler)
>  {
>  	struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> @@ -4846,22 +4845,14 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->base.adjusted_mode;
> -	int need_scaling;
>  
>  	/*
>  	 * 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_scaler_check)
> -		if (pixel_format == DRM_FORMAT_NV12)
> -			need_scaling = true;
> -
> -	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 &&
> -	    scaler_user == SKL_CRTC_INDEX)
> -		need_scaling = true;
> +	if (src_w != dst_w || src_h != dst_h)
> +		need_scaler = true;
>  
>  	/*
>  	 * Scaling/fitting not supported in IF-ID mode in GEN9+
> @@ -4870,7 +4861,7 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 * for NV12.
>  	 */
>  	if (INTEL_GEN(dev_priv) >= 9 && crtc_state->base.enable &&
> -	    need_scaling && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +	    need_scaler && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>  		DRM_DEBUG_KMS("Pipe/Plane scaling not supported with IF-ID mode\n");
>  		return -EINVAL;
>  	}
> @@ -4885,7 +4876,7 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 * update to free the scaler is done in plane/panel-fit programming.
>  	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
>  	 */
> -	if (force_detach || !need_scaling) {
> +	if (force_detach || !need_scaler) {
>  		if (*scaler_id >= 0) {
>  			scaler_state->scaler_users &= ~(1 << scaler_user);
>  			scaler_state->scalers[*scaler_id].in_use = 0;
> @@ -4899,7 +4890,7 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  		return 0;
>  	}
>  
> -	if (plane_scaler_check && pixel_format == DRM_FORMAT_NV12 &&
> +	if (format && format->format == DRM_FORMAT_NV12 &&
>  	    (src_h < SKL_MIN_YUV_420_SRC_H || src_w < SKL_MIN_YUV_420_SRC_W)) {
>  		DRM_DEBUG_KMS("NV12: src dimensions not met\n");
>  		return -EINVAL;
> @@ -4942,12 +4933,16 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  {
>  	const struct drm_display_mode *adjusted_mode = &state->base.adjusted_mode;
> +	bool need_scaler = false;
> +
> +	if (state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +		need_scaler = true;
>  
>  	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
>  				 &state->scaler_state.scaler_id,
>  				 state->pipe_src_w, state->pipe_src_h,
>  				 adjusted_mode->crtc_hdisplay,
> -				 adjusted_mode->crtc_vdisplay, false, 0);
> +				 adjusted_mode->crtc_vdisplay, NULL, need_scaler);
>  }
>  
>  /**
> @@ -4962,13 +4957,17 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  				   struct intel_plane_state *plane_state)
>  {
> -
>  	struct intel_plane *intel_plane =
>  		to_intel_plane(plane_state->base.plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  	int ret;
> -
>  	bool force_detach = !fb || !plane_state->base.visible;
> +	bool need_scaler = false;
> +
> +	/* Pre-gen11 and SDR planes always need a scaler for planar formats. */
> +	if (!icl_is_hdr_plane(intel_plane) &&
> +	    fb && fb->format->format == DRM_FORMAT_NV12)
> +		need_scaler = true;
>  
>  	ret = skl_update_scaler(crtc_state, force_detach,
>  				drm_plane_index(&intel_plane->base),
> @@ -4977,7 +4976,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  				drm_rect_height(&plane_state->base.src) >> 16,
>  				drm_rect_width(&plane_state->base.dst),
>  				drm_rect_height(&plane_state->base.dst),
> -				fb ? true : false, fb ? fb->format->format : 0);
> +				fb ? fb->format : NULL, need_scaler);
>  
>  	if (ret || plane_state->scaler_id < 0)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 272de906a001..f1d196bcdb66 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2211,6 +2211,14 @@ static inline bool icl_is_nv12_y_plane(enum plane_id id)
>  	return false;
>  }
>  
> +static inline bool icl_is_hdr_plane(struct intel_plane *plane)
> +{
> +	if (INTEL_GEN(to_i915(plane->base.dev)) < 11)
> +		return false;
> +
> +	return plane->id < PLANE_SPRITE2;
> +}
> +
>  /* intel_tv.c */
>  void intel_tv_init(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 7cd59eee5cad..a2a4328107b6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -332,7 +332,8 @@ skl_program_scaler(struct drm_i915_private *dev_priv,
>  	crtc_h--;
>  
>  	/* TODO: handle sub-pixel coordinates */
> -	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> +	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12 &&
> +	    !icl_is_hdr_plane(plane)) {
>  		y_hphase = skl_scaler_calc_phase(1, false);
>  		y_vphase = skl_scaler_calc_phase(1, false);
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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