diff mbox series

[02/17] drm/i915: Move linetime wms into the crtc state

Message ID 20200120174728.21095-3-ville.syrjala@linux.intel.com
State New, archived
Headers show
Series drm/i915: Global state rework | expand

Commit Message

Ville Syrjälä Jan. 20, 2020, 5:47 p.m. UTC
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(-)

Comments

Lisovskiy, Stanislav Jan. 29, 2020, 2:05 p.m. UTC | #1
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);
Ville Syrjälä Jan. 31, 2020, 3:07 p.m. UTC | #2
On Mon, Jan 20, 2020 at 07:47:12PM +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>

Extracted Stan's r-b from the trybot list (whoops) and pushed the lot:
https://lists.freedesktop.org/archives/intel-gfx-trybot/2020-January/086561.html

Thanks for the reviews.

As Imre pointed out there some further docs/function naming improvements
should probably be done to make the thing a bit less confusing. I'll
look at that as a followup.
diff mbox series

Patch

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);
+	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);