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.lankho...@linux.intel.com>
> > Cc: Azhar Shaikh <azhar.sha...@intel.com>
> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to