Re: [Intel-gfx] [PATCH] drm/i915: Disable SAGV on pre plane update.

2018-02-22 Thread Rodrigo Vivi
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 
> > Cc: Azhar Shaikh 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Rodrigo Vivi 
> > ---
> >  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(>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, _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);
> > -
> > 

Re: [Intel-gfx] [PATCH] drm/i915: Disable SAGV on pre plane update.

2018-02-22 Thread Ville Syrjälä
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 
> Cc: Azhar Shaikh 
> Cc: Ville Syrjälä 
> Signed-off-by: Rodrigo Vivi 
> ---
>  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(>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, _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 

[Intel-gfx] [PATCH] drm/i915: Disable SAGV on pre plane update.

2018-02-22 Thread Rodrigo Vivi
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 
Cc: Azhar Shaikh 
Cc: Ville Syrjälä 
Signed-off-by: Rodrigo Vivi 
---
 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(>base, pipe_config);
}
+
+   if (pipe_config->sagv)
+   intel_enable_sagv(dev_priv);
 }
 
 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, _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
---