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

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

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
> 

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