Re: [Intel-gfx] [PATCH 02/17] drm/i915: Move linetime wms into the crtc state

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Mon, 2020-01-20 at 19:47 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The linetime watermarks really have very little in common with the
> plane watermarks. It looks to be cleaner to simply track them in
> the crtc_state and program them from the normal modeset/fastset
> paths.
> 
> The only dark cloud comes from the fact that the register is
> still supposedly single buffered. So in theory it might still
> need some form of two stage programming. Note that even though
> HSW/BDWhave two stage programming we never computed any special
> intermediate values for the linetime watermarks, and on SKL+
> we don't even have the two stage stuff plugged in since everything
> else is double buffered. So let's assume it's all fine and
> continue doing what we've been doing.
> 
> Actually on HSW/BDW the value should not even change without
> a full modeset since it doesn't account for pfit downscaling.
> Thus only fastboot might be affected. But on SKL+ the pfit
> scaling factor is take into consideration so the value may
> change during any fastset.
> 
> As a bonus we'll plug this thing into the state
> checker/dump now.
> 
> v2: Rebase due to bigjoiner prep
> v2: Only compute ips linetime for IPS capable pipes.
>     Bspec says the register values is ignored for other
>     pipes, but in fact it can't even be written so the
>     state checker becomes unhappy if we don't compute
>     it as zero.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  95 +++++++++++++-
>  .../drm/i915/display/intel_display_types.h    |   7 +-
>  drivers/gpu/drm/i915/i915_drv.h               |   1 -
>  drivers/gpu/drm/i915/intel_pm.c               | 117 +---------------
> --
>  4 files changed, 98 insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 76c17341df2b..8dcb86c51aaa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6885,6 +6885,16 @@ static void icl_pipe_mbus_enable(struct
> intel_crtc *crtc)
>  	I915_WRITE(PIPE_MBUS_DBOX_CTL(pipe), val);
>  }
>  
> +static void hsw_set_linetime_wm(const struct intel_crtc_state
> *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	I915_WRITE(WM_LINETIME(crtc->pipe),
> +		   HSW_LINETIME(crtc_state->linetime) |
> +		   HSW_IPS_LINETIME(crtc_state->ips_linetime));
> +}
> +
>  static void hsw_set_frame_start_delay(const struct intel_crtc_state
> *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -6969,6 +6979,8 @@ static void hsw_crtc_enable(struct
> intel_atomic_state *state,
>  	if (INTEL_GEN(dev_priv) < 9)
>  		intel_disable_primary_plane(new_crtc_state);
>  
> +	hsw_set_linetime_wm(new_crtc_state);
> +
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		icl_set_pipe_chicken(crtc);
>  
> @@ -10947,6 +10959,7 @@ static bool hsw_get_pipe_config(struct
> intel_crtc *crtc,
>  	enum intel_display_power_domain power_domain;
>  	u64 power_domain_mask;
>  	bool active;
> +	u32 tmp;
>  
>  	pipe_config->master_transcoder = INVALID_TRANSCODER;
>  
> @@ -11010,7 +11023,7 @@ static bool hsw_get_pipe_config(struct
> intel_crtc *crtc,
>  	pipe_config->csc_mode = I915_READ(PIPE_CSC_MODE(crtc->pipe));
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
> -		u32 tmp = I915_READ(SKL_BOTTOM_COLOR(crtc->pipe));
> +		tmp = I915_READ(SKL_BOTTOM_COLOR(crtc->pipe));
>  
>  		if (tmp & SKL_BOTTOM_COLOR_GAMMA_ENABLE)
>  			pipe_config->gamma_enable = true;
> @@ -11023,6 +11036,12 @@ static bool hsw_get_pipe_config(struct
> intel_crtc *crtc,
>  
>  	intel_color_get_config(pipe_config);
>  
> +	tmp = I915_READ(WM_LINETIME(crtc->pipe));
> +	pipe_config->linetime = REG_FIELD_GET(HSW_LINETIME_MASK, tmp);

Shouldn't we have also gen >= 9 check here? If let's say
hsw_get_pipe_config was called for non-gen 9, non-hsw and non-bdw case.

I see that hsw_get_pipe_config hook is used also for any platform which
evaluates HAS_DDI check to true in intel_init_display_hooks.

At least I see that everywhere else there is a check for >= gen 9,
when you set pipe_config->linetime. 

Stan

> +	if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> +		pipe_config->ips_linetime =
> +			REG_FIELD_GET(HSW_IPS_LINETIME_MASK, tmp);
> +
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>  	WARN_ON(power_domain_mask & BIT_ULL(power_domain));
>  
> @@ -12508,6 +12527,53 @@ static int
> icl_compute_port_sync_crtc_state(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +static u16 hsw_linetime_wm(const struct intel_crtc_state
> *crtc_state)
> +{
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->hw.adjusted_mode;
> +
> +	if (!crtc_state->hw.enable)
> +		return 0;
> +
> +	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> +				 adjusted_mode->crtc_clock);
> +}
> +
> +static u16 hsw_ips_linetime_wm(const struct intel_crtc_state
> *crtc_state)
> +{
> +	const struct intel_atomic_state *state =
> +		to_intel_atomic_state(crtc_state->uapi.state);
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->hw.adjusted_mode;
> +
> +	if (!crtc_state->hw.enable)
> +		return 0;
> +
> +	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> +				 state->cdclk.logical.cdclk);
> +}
> +
> +static u16 skl_linetime_wm(const struct intel_crtc_state
> *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->hw.adjusted_mode;
> +	u16 linetime_wm;
> +
> +	if (!crtc_state->hw.enable)
> +		return 0;
> +
> +	linetime_wm = DIV_ROUND_UP(adjusted_mode->crtc_htotal * 1000 *
> 8,
> +				   crtc_state->pixel_rate);
> +
> +	/* Display WA #1135: BXT:ALL GLK:ALL */
> +	if (IS_GEN9_LP(dev_priv) && dev_priv->ipc_enabled)
> +		linetime_wm /= 2;
> +
> +	return linetime_wm;
> +}
> +
>  static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc)
>  {
> @@ -12579,6 +12645,14 @@ static int intel_crtc_atomic_check(struct
> intel_atomic_state *state,
>  	if (HAS_IPS(dev_priv))
>  		crtc_state->ips_enabled =
> hsw_compute_ips_config(crtc_state);
>  
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		crtc_state->linetime = skl_linetime_wm(crtc_state);
> +	} else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) {
> +		crtc_state->linetime = hsw_linetime_wm(crtc_state);
> +		if (hsw_crtc_supports_ips(crtc))
> +			crtc_state->ips_linetime =
> hsw_ips_linetime_wm(crtc_state);
> +	}
> +
>  	return ret;
>  }
>  
> @@ -12868,6 +12942,9 @@ static void intel_dump_pipe_config(const
> struct intel_crtc_state *pipe_config,
>  		      pipe_config->pipe_src_w, pipe_config->pipe_src_h,
>  		      pipe_config->pixel_rate);
>  
> +	DRM_DEBUG_KMS("linetime: %d, ips linetime: %d\n",
> +		      pipe_config->linetime, pipe_config-
> >ips_linetime);
> +
>  	if (INTEL_GEN(dev_priv) >= 9)
>  		DRM_DEBUG_KMS("num_scalers: %d, scaler_users: 0x%x,
> scaler_id: %d\n",
>  			      crtc->num_scalers,
> @@ -13638,10 +13715,12 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
>  		PIPE_CONF_CHECK_BOOL(gamma_enable);
>  		PIPE_CONF_CHECK_BOOL(csc_enable);
>  
> +		PIPE_CONF_CHECK_I(linetime);
> +		PIPE_CONF_CHECK_I(ips_linetime);
> +
>  		bp_gamma =
> intel_color_get_gamma_bit_precision(pipe_config);
>  		if (bp_gamma)
>  			PIPE_CONF_CHECK_COLOR_LUT(gamma_mode,
> hw.gamma_lut, bp_gamma);
> -
>  	}
>  
>  	PIPE_CONF_CHECK_BOOL(double_wide);
> @@ -14798,6 +14877,18 @@ static void intel_pipe_fastset(const struct
> intel_crtc_state *old_crtc_state,
>  			ilk_pfit_disable(old_crtc_state);
>  	}
>  
> +	/*
> +	 * The register is supposedly single buffered so perhaps
> +	 * not 100% correct to do this here. But SKL+ calculate
> +	 * this based on the adjust pixel rate so pfit changes do
> +	 * affect it and so it must be updated for fastsets.
> +	 * HSW/BDW only really need this here for fastboot, after
> +	 * that the value should not change without a full modeset.
> +	 */
> +	if (INTEL_GEN(dev_priv) >= 9 ||
> +	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> +		hsw_set_linetime_wm(new_crtc_state);
> +
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		icl_set_pipe_chicken(crtc);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index bfe85e180e16..2d8491590501 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -662,8 +662,6 @@ struct intel_crtc_scaler_state {
>  
>  struct intel_pipe_wm {
>  	struct intel_wm_level wm[5];
> -	u16 linetime;
> -	u16 ips_linetime;
>  	bool fbc_wm_enabled;
>  	bool pipe_enabled;
>  	bool sprites_enabled;
> @@ -679,7 +677,6 @@ struct skl_plane_wm {
>  
>  struct skl_pipe_wm {
>  	struct skl_plane_wm planes[I915_MAX_PLANES];
> -	u32 linetime;
>  };
>  
>  enum vlv_wm_level {
> @@ -1050,6 +1047,10 @@ struct intel_crtc_state {
>  		struct drm_dsc_config config;
>  	} dsc;
>  
> +	/* HSW+ linetime watermarks */
> +	u16 linetime;
> +	u16 ips_linetime;
> +
>  	/* Forward Error correction State */
>  	bool fec_enable;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 077af22b8340..5bd40184ddee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -744,7 +744,6 @@ struct ilk_wm_values {
>  	u32 wm_pipe[3];
>  	u32 wm_lp[3];
>  	u32 wm_lp_spr[3];
> -	u32 wm_linetime[3];
>  	bool enable_fbc_wm;
>  	enum intel_ddb_partitioning partitioning;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index a4b66ee1e3d8..edd62367006d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2810,34 +2810,6 @@ static void ilk_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  	result->enable = true;
>  }
>  
> -static u32
> -hsw_linetime_wm(const struct intel_crtc_state *crtc_state)
> -{
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->hw.adjusted_mode;
> -
> -	if (!crtc_state->hw.active)
> -		return 0;
> -
> -	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> -				 adjusted_mode->crtc_clock);
> -}
> -
> -static u32
> -hsw_ips_linetime_wm(const struct intel_crtc_state *crtc_state)
> -{
> -	const struct intel_atomic_state *state =
> -		to_intel_atomic_state(crtc_state->uapi.state);
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->hw.adjusted_mode;
> -
> -	if (!crtc_state->hw.active)
> -		return 0;
> -
> -	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> -				 state->cdclk.logical.cdclk);
> -}
> -
>  static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
>  				  u16 wm[8])
>  {
> @@ -3178,11 +3150,6 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc_state *crtc_state)
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, crtc_state,
>  			     pristate, sprstate, curstate, &pipe_wm-
> >wm[0]);
>  
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		pipe_wm->linetime = hsw_linetime_wm(crtc_state);
> -		pipe_wm->ips_linetime =
> hsw_ips_linetime_wm(crtc_state);
> -	}
> -
>  	if (!ilk_validate_pipe_wm(dev_priv, pipe_wm))
>  		return -EINVAL;
>  
> @@ -3433,9 +3400,6 @@ static void ilk_compute_wm_results(struct
> drm_i915_private *dev_priv,
>  
>  		if (WARN_ON(!r->enable))
>  			continue;
> -		results->wm_linetime[pipe] =
> -			HSW_LINETIME(pipe_wm->linetime) |
> -			HSW_IPS_LINETIME(pipe_wm->ips_linetime);
>  
>  		results->wm_pipe[pipe] =
>  			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> @@ -3475,7 +3439,6 @@ ilk_find_best_result(struct drm_i915_private
> *dev_priv,
>  
>  /* dirty bits used to track which watermarks need changes */
>  #define WM_DIRTY_PIPE(pipe) (1 << (pipe))
> -#define WM_DIRTY_LINETIME(pipe) (1 << (8 + (pipe)))
>  #define WM_DIRTY_LP(wm_lp) (1 << (15 + (wm_lp)))
>  #define WM_DIRTY_LP_ALL (WM_DIRTY_LP(1) | WM_DIRTY_LP(2) |
> WM_DIRTY_LP(3))
>  #define WM_DIRTY_FBC (1 << 24)
> @@ -3490,12 +3453,6 @@ static unsigned int
> ilk_compute_wm_dirty(struct drm_i915_private *dev_priv,
>  	int wm_lp;
>  
>  	for_each_pipe(dev_priv, pipe) {
> -		if (old->wm_linetime[pipe] != new->wm_linetime[pipe]) {
> -			dirty |= WM_DIRTY_LINETIME(pipe);
> -			/* Must disable LP1+ watermarks too */
> -			dirty |= WM_DIRTY_LP_ALL;
> -		}
> -
>  		if (old->wm_pipe[pipe] != new->wm_pipe[pipe]) {
>  			dirty |= WM_DIRTY_PIPE(pipe);
>  			/* Must disable LP1+ watermarks too */
> @@ -3587,13 +3544,6 @@ static void ilk_write_wm_values(struct
> drm_i915_private *dev_priv,
>  	if (dirty & WM_DIRTY_PIPE(PIPE_C))
>  		I915_WRITE(WM0_PIPEC_IVB, results->wm_pipe[2]);
>  
> -	if (dirty & WM_DIRTY_LINETIME(PIPE_A))
> -		I915_WRITE(WM_LINETIME(PIPE_A), results-
> >wm_linetime[0]);
> -	if (dirty & WM_DIRTY_LINETIME(PIPE_B))
> -		I915_WRITE(WM_LINETIME(PIPE_B), results-
> >wm_linetime[1]);
> -	if (dirty & WM_DIRTY_LINETIME(PIPE_C))
> -		I915_WRITE(WM_LINETIME(PIPE_C), results-
> >wm_linetime[2]);
> -
>  	if (dirty & WM_DIRTY_DDB) {
>  		if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  			val = I915_READ(WM_MISC);
> @@ -4847,24 +4797,6 @@ skl_compute_wm_levels(const struct
> intel_crtc_state *crtc_state,
>  	}
>  }
>  
> -static u32
> -skl_compute_linetime_wm(const struct intel_crtc_state *crtc_state)
> -{
> -	struct drm_atomic_state *state = crtc_state->uapi.state;
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	uint_fixed_16_16_t linetime_us;
> -	u32 linetime_wm;
> -
> -	linetime_us = intel_get_linetime_us(crtc_state);
> -	linetime_wm = fixed16_to_u32_round_up(mul_u32_fixed16(8,
> linetime_us));
> -
> -	/* Display WA #1135: BXT:ALL GLK:ALL */
> -	if (IS_GEN9_LP(dev_priv) && dev_priv->ipc_enabled)
> -		linetime_wm /= 2;
> -
> -	return linetime_wm;
> -}
> -
>  static void skl_compute_transition_wm(const struct intel_crtc_state
> *crtc_state,
>  				      const struct skl_wm_params *wp,
>  				      struct skl_plane_wm *wm)
> @@ -5052,8 +4984,6 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *crtc_state)
>  			return ret;
>  	}
>  
> -	pipe_wm->linetime = skl_compute_linetime_wm(crtc_state);
> -
>  	return 0;
>  }
>  
> @@ -5178,7 +5108,7 @@ static bool skl_pipe_wm_equals(struct
> intel_crtc *crtc,
>  			return false;
>  	}
>  
> -	return wm1->linetime == wm2->linetime;
> +	return true;
>  }
>  
>  static inline bool skl_ddb_entries_overlap(const struct
> skl_ddb_entry *a,
> @@ -5559,40 +5489,6 @@ skl_compute_wm(struct intel_atomic_state
> *state)
>  	return 0;
>  }
>  
> -static void skl_atomic_update_crtc_wm(struct intel_atomic_state
> *state,
> -				      struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	const struct intel_crtc_state *crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	const struct skl_pipe_wm *pipe_wm = &crtc_state-
> >wm.skl.optimal;
> -	enum pipe pipe = crtc->pipe;
> -
> -	if ((state->wm_results.dirty_pipes & BIT(crtc->pipe)) == 0)
> -		return;
> -
> -	I915_WRITE(WM_LINETIME(pipe), HSW_LINETIME(pipe_wm->linetime));
> -}
> -
> -static void skl_initial_wm(struct intel_atomic_state *state,
> -			   struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	const struct intel_crtc_state *crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct skl_ddb_values *results = &state->wm_results;
> -
> -	if ((results->dirty_pipes & BIT(crtc->pipe)) == 0)
> -		return;
> -
> -	mutex_lock(&dev_priv->wm.wm_mutex);
> -
> -	if (crtc_state->uapi.active_changed)
> -		skl_atomic_update_crtc_wm(state, crtc);
> -
> -	mutex_unlock(&dev_priv->wm.wm_mutex);
> -}
> -
>  static void ilk_compute_wm_config(struct drm_i915_private *dev_priv,
>  				  struct intel_wm_config *config)
>  {
> @@ -5715,9 +5611,6 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc
> *crtc,
>  
>  	if (!crtc->active)
>  		return;
> -
> -	val = I915_READ(WM_LINETIME(pipe));
> -	out->linetime = REG_FIELD_GET(HSW_LINETIME_MASK, val);
>  }
>  
>  void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
> @@ -5758,8 +5651,6 @@ static void ilk_pipe_wm_get_hw_state(struct
> intel_crtc *crtc)
>  	};
>  
>  	hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]);
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		hw->wm_linetime[pipe] = I915_READ(WM_LINETIME(pipe));
>  
>  	memset(active, 0, sizeof(*active));
>  
> @@ -5778,10 +5669,6 @@ static void ilk_pipe_wm_get_hw_state(struct
> intel_crtc *crtc)
>  		active->wm[0].pri_val = (tmp & WM0_PIPE_PLANE_MASK) >>
> WM0_PIPE_PLANE_SHIFT;
>  		active->wm[0].spr_val = (tmp & WM0_PIPE_SPRITE_MASK) >>
> WM0_PIPE_SPRITE_SHIFT;
>  		active->wm[0].cur_val = tmp & WM0_PIPE_CURSOR_MASK;
> -		active->linetime = REG_FIELD_GET(HSW_LINETIME_MASK,
> -						 hw-
> >wm_linetime[pipe]);
> -		active->ips_linetime =
> REG_FIELD_GET(HSW_IPS_LINETIME_MASK,
> -						     hw-
> >wm_linetime[pipe]);
>  	} else {
>  		int level, max_level = ilk_wm_max_level(dev_priv);
>  
> @@ -7264,8 +7151,6 @@ void intel_init_pm(struct drm_i915_private
> *dev_priv)
>  	/* For FIFO watermark updates */
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		skl_setup_wm_latency(dev_priv);
> -		dev_priv->display.initial_watermarks = skl_initial_wm;
> -		dev_priv->display.atomic_update_watermarks =
> skl_atomic_update_crtc_wm;
>  		dev_priv->display.compute_global_watermarks =
> skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
>  		ilk_setup_wm_latency(dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx