On Mon, 2020-01-20 at 19:47 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
