On Thu, Feb 22, 2018 at 10:00:47PM +0200, Ville Syrjälä wrote:
> 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.
I wondered that...
> And we can't use
> state->active_crtcs for this in non-modeset commits since that's only
> valid during a modeset.
but I thought this was enough :(
>
> 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.
I considered some kind of bitmask for this case this morning, but
I thought that state->active_crtcs would take care of that so I ignored
and move forward.
> That idea could be used for sagv as well I suppose.
could it be the other way around?
>
> > }
> >
> > 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
