On Thu, Feb 22, 2018 at 11:28:58AM -0800, Rodrigo Vivi wrote:
> According to Spec "Requirement before plane enabling or
> configuration change: Disable SAGV if any enabled plane will not
> be able to enable watermarks for memory latency >= SAGV block
> time, or any transcoder is interlaced. Else, enable SAGV."
>
> Currently we are only enabling and disabling SAGV on full
> modeset. If we end up changing plane configurations and
> sagv remains enabled when latency is higher than sagv block
> time the machine can hang.
>
> Also we are computing the latency values in different places
> and following different conditions/rules.
>
> So let's move the can_enable_sagv check to be inside
> skl_compute_wm and disable and enable on {pre,post} plane
> updates.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Azhar Shaikh <azhar.shaikh@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 18 +++++-----
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pm.c | 64 +++++++++++++-----------------------
> 3 files changed, 32 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 65c8487be7c7..008254579be1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_atomic_state *old_state = old_crtc_state->base.state;
> struct intel_crtc_state *pipe_config =
> intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
> @@ -5120,6 +5122,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> !old_primary_state->base.visible))
> intel_post_enable_primary(&crtc->base, pipe_config);
> }
> +
> + if (pipe_config->sagv)
> + intel_enable_sagv(dev_priv);
Unfortunately a single pipe can't make a unilateral decision to
enable sagv. The other pipes must be disabled first. And we can't use
state->active_crtcs for this in non-modeset commits since that's only
valid during a modeset.
cxsr has the same problem pretty much. Currently that's handled somewhat
hackily from foo_merge_wm() to disable cxsr when multiple pipes are
active. But I think a better idea might be to maintain a bitmask of
pipes allowing cxsr in dev_priv. And then have enable/disable hooks
that get called for each pipe from the commit path to manipulate the
bitmask and based on what the bitmask ends up looking either enable or
disable cxsr. That idea could be used for sagv as well I suppose.
> }
>
> static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> @@ -5205,6 +5210,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> pipe_config);
> else if (pipe_config->update_wm_pre)
> intel_update_watermarks(crtc);
> +
> + if (!pipe_config->sagv)
> + intel_disable_sagv(dev_priv);
> }
>
> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -12366,13 +12374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>
> intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
>
> - /*
> - * SKL workaround: bspec recommends we disable the SAGV when we
> - * have more then one pipe enabled
> - */
> - if (!intel_can_enable_sagv(state))
> - intel_disable_sagv(dev_priv);
> -
> intel_modeset_verify_disabled(dev, state);
> }
>
> @@ -12431,9 +12432,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> if (intel_state->modeset)
> intel_verify_planes(intel_state);
>
> - if (intel_state->modeset && intel_can_enable_sagv(state))
> - intel_enable_sagv(dev_priv);
> -
> drm_atomic_helper_commit_hw_done(state);
>
> if (intel_state->modeset) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1535bfb7ea40..4c10a8a94d73 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -712,6 +712,7 @@ struct intel_crtc_state {
> bool update_pipe; /* can a fast modeset be performed? */
> bool disable_cxsr;
> bool update_wm_pre, update_wm_post; /* watermarks are updated */
> + bool sagv; /* Disable SAGV on any latency higher than its block time */
> bool fb_changed; /* fb on any of the planes is changed */
> bool fifo_changed; /* FIFO split is changed */
>
> @@ -2001,7 +2002,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> struct skl_pipe_wm *out);
> void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> -bool intel_can_enable_sagv(struct drm_atomic_state *state);
> +bool intel_can_enable_sagv(struct intel_atomic_state *state, u32 latency);
> int intel_enable_sagv(struct drm_i915_private *dev_priv);
> int intel_disable_sagv(struct drm_i915_private *dev_priv);
> bool skl_wm_level_equals(const struct skl_wm_level *l1,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 21dac6ebc202..731b3808a62e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3681,16 +3681,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> -bool intel_can_enable_sagv(struct drm_atomic_state *state)
> +bool intel_can_enable_sagv(struct intel_atomic_state *intel_state, u32 latency)
> {
> - struct drm_device *dev = state->dev;
> + struct drm_device *dev = intel_state->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct intel_crtc *crtc;
> - struct intel_plane *plane;
> struct intel_crtc_state *cstate;
> enum pipe pipe;
> - int level, latency;
> int sagv_block_time_us;
>
> if (!intel_has_sagv(dev_priv))
> @@ -3703,6 +3700,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> else
> sagv_block_time_us = 10;
>
> + if (latency < sagv_block_time_us)
> + return false;
> +
> /*
> * SKL+ workaround: bspec recommends we disable the SAGV when we have
> * more then one pipe enabled
> @@ -3722,35 +3722,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> return false;
>
> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - struct skl_plane_wm *wm =
> - &cstate->wm.skl.optimal.planes[plane->id];
> -
> - /* Skip this plane if it's not enabled */
> - if (!wm->wm[0].plane_en)
> - continue;
> -
> - /* Find the highest enabled wm level for this plane */
> - for (level = ilk_wm_max_level(dev_priv);
> - !wm->wm[level].plane_en; --level)
> - { }
> -
> - latency = dev_priv->wm.skl_latency[level];
> -
> - if (skl_needs_memory_bw_wa(intel_state) &&
> - plane->base.state->fb->modifier ==
> - I915_FORMAT_MOD_X_TILED)
> - latency += 15;
> -
> - /*
> - * If any of the planes on this pipe don't enable wm levels that
> - * incur memory latencies higher than sagv_block_time_us we
> - * can't enable the SAGV.
> - */
> - if (latency < sagv_block_time_us)
> - return false;
> - }
> -
> return true;
> }
>
> @@ -4501,7 +4472,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> const struct skl_wm_params *wp,
> uint16_t *out_blocks, /* out */
> uint8_t *out_lines, /* out */
> - bool *enabled /* out */)
> + bool *enabled, /* out */
> + bool *sagv /* out */)
> {
> const struct drm_plane_state *pstate = &intel_pstate->base;
> uint32_t latency = dev_priv->wm.skl_latency[level];
> @@ -4528,6 +4500,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> if (apply_memory_bw_wa && wp->x_tiled)
> latency += 15;
>
> + *sagv = intel_can_enable_sagv(state, latency);
> +
> method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> wp->cpp, latency, wp->dbuf_block_size);
> method2 = skl_wm_method2(wp->plane_pixel_rate,
> @@ -4629,7 +4603,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> const struct skl_wm_params *wm_params,
> - struct skl_plane_wm *wm)
> + struct skl_plane_wm *wm,
> + bool *sagv)
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> struct drm_plane *plane = intel_pstate->base.plane;
> @@ -4655,7 +4630,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> wm_params,
> &result->plane_res_b,
> &result->plane_res_l,
> - &result->plane_en);
> + &result->plane_en,
> + sagv);
> if (ret)
> return ret;
> }
> @@ -4743,7 +4719,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>
> static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> struct skl_ddb_allocation *ddb,
> - struct skl_pipe_wm *pipe_wm)
> + struct skl_pipe_wm *pipe_wm,
> + bool *sagv)
> {
> struct drm_device *dev = cstate->base.crtc->dev;
> struct drm_crtc_state *crtc_state = &cstate->base;
> @@ -4777,7 +4754,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> return ret;
>
> ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> - intel_pstate, &wm_params, wm);
> + intel_pstate, &wm_params, wm, sagv);
> if (ret)
> return ret;
> skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> @@ -4899,12 +4876,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> const struct skl_pipe_wm *old_pipe_wm,
> struct skl_pipe_wm *pipe_wm, /* out */
> struct skl_ddb_allocation *ddb, /* out */
> - bool *changed /* out */)
> + bool *changed /* out */,
> + bool *sagv /* out */)
> {
> struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
> int ret;
>
> - ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
> + ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
> if (ret)
> return ret;
>
> @@ -5099,6 +5077,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct skl_pipe_wm *pipe_wm;
> bool changed = false;
> + bool sagv = true;
> int ret, i;
>
> /*
> @@ -5147,7 +5126,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>
> pipe_wm = &intel_cstate->wm.skl.optimal;
> ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> - &results->ddb, &changed);
> + &results->ddb, &changed, &sagv);
> if (ret)
> return ret;
>
> @@ -5159,6 +5138,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> continue;
>
> intel_cstate->update_wm_pre = true;
> + intel_cstate->sagv &= sagv;
> }
>
> skl_print_wm_changes(state);
> --
> 2.13.6
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
