diff mbox series

[4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

Message ID 20180921173945.6276-5-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/gen11: Enable planar format support on gen11. | expand

Commit Message

Maarten Lankhorst Sept. 21, 2018, 5:39 p.m. UTC
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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
 5 files changed, 34 insertions(+), 15 deletions(-)

Comments

Ville Syrjälä Sept. 21, 2018, 6:45 p.m. UTC | #1
On Fri, Sep 21, 2018 at 07:39:42PM +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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
>  5 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e7e6ca7f9665..1b59d15aaf59 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6872,6 +6872,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..3c240ad0a8d3 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  		if (INTEL_GEN(dev_priv) == 9 &&
>  		    !IS_GEMINILAKE(dev_priv))
>  			mode = SKL_PS_SCALER_MODE_NV12;
> -		else
> +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
>  			mode = PS_SCALER_MODE_PLANAR;
>  
> +			if (plane_state->linked_plane)
> +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> +		} else
> +			mode = PS_SCALER_MODE_PACKED;
>  	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
>  		mode = PS_SCALER_MODE_PACKED;
>  	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19cd6bbb43c4..cea91235d498 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4833,8 +4833,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_scaling)
>  {
>  	struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> @@ -4843,18 +4842,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 (src_w != dst_w || src_h != dst_h)
> +		need_scaling = true;
>  
>  	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
> @@ -4895,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;
> @@ -4943,7 +4938,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  				 &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, 0);
>  }
>  
>  /**
> @@ -4958,13 +4953,22 @@ 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_scaling = false;
> +
> +	if (fb && fb->format->format == DRM_FORMAT_NV12) {
> +		/*
> +		 * Gen10- and sprite 2 and 3 always need the scaler.

This part of the comment is rather more confusing than helpful I think.
The rest seems clear enough so maybe just drop this sentence?

> +		 * On gen11 we use the chroma upsampler when available,
> +		 * and only use the scaler for normal scaling.
> +		 */
> +		if (!icl_is_hdr_plane(intel_plane))
> +			need_scaling = true;
> +	}
>  
>  	ret = skl_update_scaler(crtc_state, force_detach,
>  				drm_plane_index(&intel_plane->base),
> @@ -4973,7 +4977,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_scaling);
>  
>  	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 29c7a4bb484d..25be23414913 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2196,6 +2196,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 46c6336cb858..111d72a5d5a0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -310,7 +310,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.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Sept. 24, 2018, 8:39 a.m. UTC | #2
Op 21-09-18 om 20:45 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 07:39:42PM +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.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>>  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
>>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
>>  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
>>  5 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index e7e6ca7f9665..1b59d15aaf59 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6872,6 +6872,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..3c240ad0a8d3 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>>  		if (INTEL_GEN(dev_priv) == 9 &&
>>  		    !IS_GEMINILAKE(dev_priv))
>>  			mode = SKL_PS_SCALER_MODE_NV12;
>> -		else
>> +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
>>  			mode = PS_SCALER_MODE_PLANAR;
>>  
>> +			if (plane_state->linked_plane)
>> +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
>> +		} else
>> +			mode = PS_SCALER_MODE_PACKED;
>>  	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
>>  		mode = PS_SCALER_MODE_PACKED;
>>  	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 19cd6bbb43c4..cea91235d498 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4833,8 +4833,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_scaling)
>>  {
>>  	struct intel_crtc_scaler_state *scaler_state =
>>  		&crtc_state->scaler_state;
>> @@ -4843,18 +4842,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 (src_w != dst_w || src_h != dst_h)
>> +		need_scaling = true;
>>  
>>  	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>>  		need_scaling = true;
>> @@ -4895,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;
>> @@ -4943,7 +4938,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>>  				 &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, 0);
>>  }
>>  
>>  /**
>> @@ -4958,13 +4953,22 @@ 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_scaling = false;
>> +
>> +	if (fb && fb->format->format == DRM_FORMAT_NV12) {
>> +		/*
>> +		 * Gen10- and sprite 2 and 3 always need the scaler.
> This part of the comment is rather more confusing than helpful I think.
> The rest seems clear enough so maybe just drop this sentence?
r-b if changed to:
gen10- and non-hdr planes always need the scaler?
>> +		 * On gen11 we use the chroma upsampler when available,
>> +		 * and only use the scaler for normal scaling.
>> +		 */
>> +		if (!icl_is_hdr_plane(intel_plane))
>> +			need_scaling = true;
>> +	}
>>  
>>  	ret = skl_update_scaler(crtc_state, force_detach,
>>  				drm_plane_index(&intel_plane->base),
>> @@ -4973,7 +4977,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_scaling);
>>  
>>  	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 29c7a4bb484d..25be23414913 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2196,6 +2196,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 46c6336cb858..111d72a5d5a0 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -310,7 +310,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.18.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 24, 2018, 1:10 p.m. UTC | #3
On Mon, Sep 24, 2018 at 10:39:54AM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 20:45 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 07:39:42PM +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.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
> >>  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
> >>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
> >>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
> >>  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
> >>  5 files changed, 34 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index e7e6ca7f9665..1b59d15aaf59 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -6872,6 +6872,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..3c240ad0a8d3 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >>  		if (INTEL_GEN(dev_priv) == 9 &&
> >>  		    !IS_GEMINILAKE(dev_priv))
> >>  			mode = SKL_PS_SCALER_MODE_NV12;
> >> -		else
> >> +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
> >>  			mode = PS_SCALER_MODE_PLANAR;
> >>  
> >> +			if (plane_state->linked_plane)
> >> +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> >> +		} else
> >> +			mode = PS_SCALER_MODE_PACKED;
> >>  	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
> >>  		mode = PS_SCALER_MODE_PACKED;
> >>  	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 19cd6bbb43c4..cea91235d498 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4833,8 +4833,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_scaling)
> >>  {
> >>  	struct intel_crtc_scaler_state *scaler_state =
> >>  		&crtc_state->scaler_state;
> >> @@ -4843,18 +4842,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 (src_w != dst_w || src_h != dst_h)
> >> +		need_scaling = true;
> >>  
> >>  	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> >>  		need_scaling = true;
> >> @@ -4895,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;
> >> @@ -4943,7 +4938,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
> >>  				 &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, 0);
> >>  }
> >>  
> >>  /**
> >> @@ -4958,13 +4953,22 @@ 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_scaling = false;
> >> +
> >> +	if (fb && fb->format->format == DRM_FORMAT_NV12) {
> >> +		/*
> >> +		 * Gen10- and sprite 2 and 3 always need the scaler.
> > This part of the comment is rather more confusing than helpful I think.
> > The rest seems clear enough so maybe just drop this sentence?
> r-b if changed to:
> gen10- and non-hdr planes always need the scaler?

I don't know what gen10- even means. The usual notation is pre-foo.

> >> +		 * On gen11 we use the chroma upsampler when available,
> >> +		 * and only use the scaler for normal scaling.
> >> +		 */
> >> +		if (!icl_is_hdr_plane(intel_plane))
> >> +			need_scaling = true;
> >> +	}
> >>  
> >>  	ret = skl_update_scaler(crtc_state, force_detach,
> >>  				drm_plane_index(&intel_plane->base),
> >> @@ -4973,7 +4977,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_scaling);
> >>  
> >>  	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 29c7a4bb484d..25be23414913 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -2196,6 +2196,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 46c6336cb858..111d72a5d5a0 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -310,7 +310,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.18.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Matt Roper Sept. 27, 2018, 12:16 a.m. UTC | #4
On Fri, Sep 21, 2018 at 07:39:42PM +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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
>  5 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e7e6ca7f9665..1b59d15aaf59 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6872,6 +6872,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..3c240ad0a8d3 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  		if (INTEL_GEN(dev_priv) == 9 &&
>  		    !IS_GEMINILAKE(dev_priv))
>  			mode = SKL_PS_SCALER_MODE_NV12;
> -		else
> +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {

Minor kernel coding standard violation here; we need to make the entire
if/else block use braces once we add them to any branch.  Especially
here where we've got nested if/else already.

>  			mode = PS_SCALER_MODE_PLANAR;
>  
> +			if (plane_state->linked_plane)
> +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> +		} else
> +			mode = PS_SCALER_MODE_PACKED;

While this is correct, it looks really strange to have the code do
"if nv12...set mode=packed" -- maybe we should change this to definition
to _NORMAL rather than _PACKED; that's actually what the bspec calls
this bit on gen11 anyway.

It might also be worth adding a comment around here explaining how the
hardware is supposed to work since it's somewhat non-obvious:

  If NV12 on Planes 0-2: Uses chroma upsampler; scaler only used (in
  normal mode) if actual scaling is necessary
  NV12 on Planes 3-4: Scaler required (in planar mode) regardless of
  whether real scaling is necessary

>  	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
>  		mode = PS_SCALER_MODE_PACKED;
>  	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19cd6bbb43c4..cea91235d498 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4833,8 +4833,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_scaling)
>  {
>  	struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> @@ -4843,18 +4842,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 (src_w != dst_w || src_h != dst_h)
> +		need_scaling = true;
>  
>  	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
> @@ -4895,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;
> @@ -4943,7 +4938,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  				 &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, 0);

Make the final parameter here 'false' to make it clear that we're
passing a bool?

>  }
>  
>  /**
> @@ -4958,13 +4953,22 @@ 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_scaling = false;

Minor nitpick, but I'd call this (and the earlier parameter to
skl_update_scaler) either need_scaler or force_scaler to clarify that it
represents the need to reserve the hardware unit rather than making a
statement about src/dst size differences.  Even if we're on an
HDR-capable plane that doesn't automatically mandate scaler usage for
NV12, we do a separate check later to see whether we're actually doing
scaling and need to grab a scaler anyway.


Matt


> +
> +	if (fb && fb->format->format == DRM_FORMAT_NV12) {
> +		/*
> +		 * Gen10- and sprite 2 and 3 always need the scaler.
> +		 * On gen11 we use the chroma upsampler when available,
> +		 * and only use the scaler for normal scaling.
> +		 */
> +		if (!icl_is_hdr_plane(intel_plane))
> +			need_scaling = true;
> +	}
>  
>  	ret = skl_update_scaler(crtc_state, force_detach,
>  				drm_plane_index(&intel_plane->base),
> @@ -4973,7 +4977,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_scaling);
>  
>  	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 29c7a4bb484d..25be23414913 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2196,6 +2196,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 46c6336cb858..111d72a5d5a0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -310,7 +310,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.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 27, 2018, 1:08 p.m. UTC | #5
On Wed, Sep 26, 2018 at 05:16:40PM -0700, Matt Roper wrote:
> On Fri, Sep 21, 2018 at 07:39:42PM +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.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
> >  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
> >  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
> >  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
> >  5 files changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index e7e6ca7f9665..1b59d15aaf59 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6872,6 +6872,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..3c240ad0a8d3 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  		if (INTEL_GEN(dev_priv) == 9 &&
> >  		    !IS_GEMINILAKE(dev_priv))
> >  			mode = SKL_PS_SCALER_MODE_NV12;
> > -		else
> > +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
> 
> Minor kernel coding standard violation here; we need to make the entire
> if/else block use braces once we add them to any branch.  Especially
> here where we've got nested if/else already.
> 
> >  			mode = PS_SCALER_MODE_PLANAR;
> >  
> > +			if (plane_state->linked_plane)
> > +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> > +		} else
> > +			mode = PS_SCALER_MODE_PACKED;
> 
> While this is correct, it looks really strange to have the code do
> "if nv12...set mode=packed" -- maybe we should change this to definition
> to _NORMAL rather than _PACKED; that's actually what the bspec calls
> this bit on gen11 anyway.
> 
> It might also be worth adding a comment around here explaining how the
> hardware is supposed to work since it's somewhat non-obvious:
> 
>   If NV12 on Planes 0-2: Uses chroma upsampler; scaler only used (in
>   normal mode) if actual scaling is necessary
>   NV12 on Planes 3-4: Scaler required (in planar mode) regardless of
>   whether real scaling is necessary

Rather that talking about specific plane numbers I suggest we stick to
the HDR vs. SDR plane terminology. Makes things more future proof at
the very least.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e7e6ca7f9665..1b59d15aaf59 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6872,6 +6872,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..3c240ad0a8d3 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -235,9 +235,13 @@  static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
 		if (INTEL_GEN(dev_priv) == 9 &&
 		    !IS_GEMINILAKE(dev_priv))
 			mode = SKL_PS_SCALER_MODE_NV12;
-		else
+		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
 			mode = PS_SCALER_MODE_PLANAR;
 
+			if (plane_state->linked_plane)
+				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
+		} else
+			mode = PS_SCALER_MODE_PACKED;
 	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
 		mode = PS_SCALER_MODE_PACKED;
 	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 19cd6bbb43c4..cea91235d498 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4833,8 +4833,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_scaling)
 {
 	struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
@@ -4843,18 +4842,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 (src_w != dst_w || src_h != dst_h)
+		need_scaling = true;
 
 	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
 		need_scaling = true;
@@ -4895,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;
@@ -4943,7 +4938,7 @@  int skl_update_scaler_crtc(struct intel_crtc_state *state)
 				 &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, 0);
 }
 
 /**
@@ -4958,13 +4953,22 @@  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_scaling = false;
+
+	if (fb && fb->format->format == DRM_FORMAT_NV12) {
+		/*
+		 * Gen10- and sprite 2 and 3 always need the scaler.
+		 * On gen11 we use the chroma upsampler when available,
+		 * and only use the scaler for normal scaling.
+		 */
+		if (!icl_is_hdr_plane(intel_plane))
+			need_scaling = true;
+	}
 
 	ret = skl_update_scaler(crtc_state, force_detach,
 				drm_plane_index(&intel_plane->base),
@@ -4973,7 +4977,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_scaling);
 
 	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 29c7a4bb484d..25be23414913 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2196,6 +2196,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 46c6336cb858..111d72a5d5a0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -310,7 +310,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);