[v26,2/9] drm/i915: Use bw state for per crtc SAGV evaluation

Commit Message

Stanislav Lisovskiy April 23, 2020, 7:58 a.m. UTC

Future platforms require per-crtc SAGV evaluation
and serializing global state when those are changed
from different commits.

v2: - Add has_sagv check to intel_crtc_can_enable_sagv
      so that it sets bit in reject mask.
    - Use bw_state in intel_pre/post_plane_enable_sagv
      instead of atomic state

v3: - Fixed rebase conflict, now using
      intel_atomic_crtc_state_for_each_plane_state in
      order to call it from atomic check
v4: - Use fb modifier from plane state

v5: - Make intel_has_sagv static again(Ville)
    - Removed unnecessary NULL assignments(Ville)
    - Removed unnecessary SAGV debug(Ville)
    - Call intel_compute_sagv_mask only for modesets(Ville)
    - Serialize global state only if sagv results change, but
      not mask itself(Ville)

v6: - use lock global state instead of serialize(Ville)

Signed-off-by: Stanislav Lisovskiy <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: James Ausmus <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
 drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
 drivers/gpu/drm/i915/intel_pm.h         |   3 +-
 3 files changed, 93 insertions(+), 29 deletions(-)

Comments

Ville Syrjala April 30, 2020, 9:09 a.m. UTC | #1

On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> Future platforms require per-crtc SAGV evaluation
> and serializing global state when those are changed
> from different commits.
> 
> v2: - Add has_sagv check to intel_crtc_can_enable_sagv
>       so that it sets bit in reject mask.
>     - Use bw_state in intel_pre/post_plane_enable_sagv
>       instead of atomic state
> 
> v3: - Fixed rebase conflict, now using
>       intel_atomic_crtc_state_for_each_plane_state in
>       order to call it from atomic check
> v4: - Use fb modifier from plane state
> 
> v5: - Make intel_has_sagv static again(Ville)
>     - Removed unnecessary NULL assignments(Ville)
>     - Removed unnecessary SAGV debug(Ville)
>     - Call intel_compute_sagv_mask only for modesets(Ville)
>     - Serialize global state only if sagv results change, but
>       not mask itself(Ville)
> 
> v6: - use lock global state instead of serialize(Ville)

What I meant is that we need both. Serialize if sagv state is going to
change, otherwise lock if the mask changes.

> 
> Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: James Ausmus <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
>  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
>  3 files changed, 93 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index ac004d6f4276..d6df91058223 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -18,6 +18,12 @@ struct intel_crtc_state;
>  struct intel_bw_state {
>  	struct intel_global_state base;
>  
> +	/*
> +	 * Contains a bit mask, used to determine, whether correspondent
> +	 * pipe allows SAGV or not.
> +	 */
> +	u8 pipe_sagv_reject;
> +
>  	unsigned int data_rate[I915_MAX_PIPES];
>  	u8 num_active_planes[I915_MAX_PIPES];
>  };
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 338a82577b76..7e15cf3368ad 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -43,6 +43,7 @@
>  #include "i915_fixed.h"
>  #include "i915_irq.h"
>  #include "i915_trace.h"
> +#include "display/intel_bw.h"
>  #include "intel_pm.h"
>  #include "intel_sideband.h"
>  #include "../../../platform/x86/intel_ips.h"
> @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
>  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	const struct intel_bw_state *new_bw_state;
>  
> -	if (!intel_can_enable_sagv(state))
> +	/*
> +	 * Just return if we can't control SAGV or don't have it.
> +	 * This is different from situation when we have SAGV but just can't
> +	 * afford it due to DBuf limitation - in case if SAGV is completely
> +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> +	 * as it will throw an error. So have to check it here.
> +	 */
> +	if (!intel_has_sagv(dev_priv))
> +		return;
> +
> +	new_bw_state = intel_atomic_get_new_bw_state(state);
> +	if (!new_bw_state)
> +		return;
> +
> +	if (!intel_can_enable_sagv(new_bw_state))
>  		intel_disable_sagv(dev_priv);
>  }
>  
>  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	const struct intel_bw_state *new_bw_state;
>  
> -	if (intel_can_enable_sagv(state))
> +	/*
> +	 * Just return if we can't control SAGV or don't have it.
> +	 * This is different from situation when we have SAGV but just can't
> +	 * afford it due to DBuf limitation - in case if SAGV is completely
> +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> +	 * as it will throw an error. So have to check it here.
> +	 */
> +	if (!intel_has_sagv(dev_priv))
> +		return;
> +
> +	new_bw_state = intel_atomic_get_new_bw_state(state);
> +	if (!new_bw_state)
> +		return;
> +
> +	if (intel_can_enable_sagv(new_bw_state))
>  		intel_enable_sagv(dev_priv);
>  }
>  
>  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_plane *plane;
> +	const struct intel_plane_state *plane_state;
>  	int level, latency;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return false;
> +
>  	if (!crtc_state->hw.active)
>  		return true;
>  
> +	/*
> +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> +	 * more then one pipe enabled
> +	 */
> +	if (hweight8(state->active_pipes) > 1)
> +		return false;
> +
>  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
>  		return false;
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
>  		const struct skl_plane_wm *wm =
>  			&crtc_state->wm.skl.optimal.planes[plane->id];
>  
> @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
>  		latency = dev_priv->wm.skl_latency[level];
>  
>  		if (skl_needs_memory_bw_wa(dev_priv) &&
> -		    plane->base.state->fb->modifier ==
> +		    plane_state->uapi.fb->modifier ==
>  		    I915_FORMAT_MOD_X_TILED)
>  			latency += 15;
>  
> @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
>  	return true;
>  }
>  
> -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	return bw_state->pipe_sagv_reject == 0;
> +}
> +
> +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> +{
> +	int ret;
>  	struct intel_crtc *crtc;
> -	const struct intel_crtc_state *crtc_state;
> -	enum pipe pipe;
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_bw_state *new_bw_state = NULL;
> +	const struct intel_bw_state *old_bw_state = NULL;
> +	int i;
>  
> -	if (!intel_has_sagv(dev_priv))
> -		return false;
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		new_bw_state = intel_atomic_get_bw_state(state);
> +		if (IS_ERR(new_bw_state))
> +			return PTR_ERR(new_bw_state);
>  
> -	/*
> -	 * If there are no active CRTCs, no additional checks need be performed
> -	 */
> -	if (hweight8(state->active_pipes) == 0)
> -		return true;
> +		old_bw_state = intel_atomic_get_old_bw_state(state);
>  
> -	/*
> -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> -	 * more then one pipe enabled
> -	 */
> -	if (hweight8(state->active_pipes) > 1)
> -		return false;
> +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> +		else
> +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> +	}
>  
> -	/* Since we're now guaranteed to only have one active CRTC... */
> -	pipe = ffs(state->active_pipes) - 1;
> -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> -	crtc_state = to_intel_crtc_state(crtc->base.state);
> +	if (!new_bw_state)
> +		return 0;
>  
> -	return intel_crtc_can_enable_sagv(crtc_state);
> +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  /*
> @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> +	if (state->modeset) {
> +		ret = intel_compute_sagv_mask(state);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * skl_compute_ddb() will have adjusted the final watermarks
>  	 * based on how much ddb is available. Now we can actually
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 9a6036ab0f90..fd1dc422e6c5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -9,6 +9,7 @@
>  #include <linux/types.h>
>  
>  #include "i915_reg.h"
> +#include "display/intel_bw.h"
>  
>  struct drm_device;
>  struct drm_i915_private;
> @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
>  int intel_enable_sagv(struct drm_i915_private *dev_priv);
>  int intel_disable_sagv(struct drm_i915_private *dev_priv);
>  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> -- 
> 2.24.1.485.gad05a3d8e5

Stanislav Lisovskiy April 30, 2020, 9:13 a.m. UTC | #2

On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > Future platforms require per-crtc SAGV evaluation
> > and serializing global state when those are changed
> > from different commits.
> > 
> > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> >       so that it sets bit in reject mask.
> >     - Use bw_state in intel_pre/post_plane_enable_sagv
> >       instead of atomic state
> > 
> > v3: - Fixed rebase conflict, now using
> >       intel_atomic_crtc_state_for_each_plane_state in
> >       order to call it from atomic check
> > v4: - Use fb modifier from plane state
> > 
> > v5: - Make intel_has_sagv static again(Ville)
> >     - Removed unnecessary NULL assignments(Ville)
> >     - Removed unnecessary SAGV debug(Ville)
> >     - Call intel_compute_sagv_mask only for modesets(Ville)
> >     - Serialize global state only if sagv results change, but
> >       not mask itself(Ville)
> > 
> > v6: - use lock global state instead of serialize(Ville)
> 
> What I meant is that we need both. Serialize if sagv state is going to
> change, otherwise lock if the mask changes.

As I understand whenever we modify global state but not a real hw, we do
only global state locking - pipe sagv mask is not actually a hw, but just
a virtual thing. It affects the QGV points we enable and if it happens to
affect those in a way that those change - we'll any way have serialize
called from intel_bw.c. Thus shouldn't be an issue.

I can change it anyway of course.

Stan

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: James Ausmus <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> >  3 files changed, 93 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > index ac004d6f4276..d6df91058223 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> >  struct intel_bw_state {
> >  	struct intel_global_state base;
> >  
> > +	/*
> > +	 * Contains a bit mask, used to determine, whether correspondent
> > +	 * pipe allows SAGV or not.
> > +	 */
> > +	u8 pipe_sagv_reject;
> > +
> >  	unsigned int data_rate[I915_MAX_PIPES];
> >  	u8 num_active_planes[I915_MAX_PIPES];
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 338a82577b76..7e15cf3368ad 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -43,6 +43,7 @@
> >  #include "i915_fixed.h"
> >  #include "i915_irq.h"
> >  #include "i915_trace.h"
> > +#include "display/intel_bw.h"
> >  #include "intel_pm.h"
> >  #include "intel_sideband.h"
> >  #include "../../../platform/x86/intel_ips.h"
> > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	const struct intel_bw_state *new_bw_state;
> >  
> > -	if (!intel_can_enable_sagv(state))
> > +	/*
> > +	 * Just return if we can't control SAGV or don't have it.
> > +	 * This is different from situation when we have SAGV but just can't
> > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > +	 * as it will throw an error. So have to check it here.
> > +	 */
> > +	if (!intel_has_sagv(dev_priv))
> > +		return;
> > +
> > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > +	if (!new_bw_state)
> > +		return;
> > +
> > +	if (!intel_can_enable_sagv(new_bw_state))
> >  		intel_disable_sagv(dev_priv);
> >  }
> >  
> >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	const struct intel_bw_state *new_bw_state;
> >  
> > -	if (intel_can_enable_sagv(state))
> > +	/*
> > +	 * Just return if we can't control SAGV or don't have it.
> > +	 * This is different from situation when we have SAGV but just can't
> > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > +	 * as it will throw an error. So have to check it here.
> > +	 */
> > +	if (!intel_has_sagv(dev_priv))
> > +		return;
> > +
> > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > +	if (!new_bw_state)
> > +		return;
> > +
> > +	if (intel_can_enable_sagv(new_bw_state))
> >  		intel_enable_sagv(dev_priv);
> >  }
> >  
> >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> >  {
> > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	struct intel_plane *plane;
> > +	const struct intel_plane_state *plane_state;
> >  	int level, latency;
> >  
> > +	if (!intel_has_sagv(dev_priv))
> > +		return false;
> > +
> >  	if (!crtc_state->hw.active)
> >  		return true;
> >  
> > +	/*
> > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > +	 * more then one pipe enabled
> > +	 */
> > +	if (hweight8(state->active_pipes) > 1)
> > +		return false;
> > +
> >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> >  		return false;
> >  
> > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> >  		const struct skl_plane_wm *wm =
> >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> >  
> > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> >  		latency = dev_priv->wm.skl_latency[level];
> >  
> >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > -		    plane->base.state->fb->modifier ==
> > +		    plane_state->uapi.fb->modifier ==
> >  		    I915_FORMAT_MOD_X_TILED)
> >  			latency += 15;
> >  
> > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> >  	return true;
> >  }
> >  
> > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	return bw_state->pipe_sagv_reject == 0;
> > +}
> > +
> > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > +{
> > +	int ret;
> >  	struct intel_crtc *crtc;
> > -	const struct intel_crtc_state *crtc_state;
> > -	enum pipe pipe;
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_bw_state *new_bw_state = NULL;
> > +	const struct intel_bw_state *old_bw_state = NULL;
> > +	int i;
> >  
> > -	if (!intel_has_sagv(dev_priv))
> > -		return false;
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		new_bw_state = intel_atomic_get_bw_state(state);
> > +		if (IS_ERR(new_bw_state))
> > +			return PTR_ERR(new_bw_state);
> >  
> > -	/*
> > -	 * If there are no active CRTCs, no additional checks need be performed
> > -	 */
> > -	if (hweight8(state->active_pipes) == 0)
> > -		return true;
> > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> >  
> > -	/*
> > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > -	 * more then one pipe enabled
> > -	 */
> > -	if (hweight8(state->active_pipes) > 1)
> > -		return false;
> > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > +		else
> > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > +	}
> >  
> > -	/* Since we're now guaranteed to only have one active CRTC... */
> > -	pipe = ffs(state->active_pipes) - 1;
> > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > +	if (!new_bw_state)
> > +		return 0;
> >  
> > -	return intel_crtc_can_enable_sagv(crtc_state);
> > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (state->modeset) {
> > +		ret = intel_compute_sagv_mask(state);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	/*
> >  	 * skl_compute_ddb() will have adjusted the final watermarks
> >  	 * based on how much ddb is available. Now we can actually
> > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 9a6036ab0f90..fd1dc422e6c5 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/types.h>
> >  
> >  #include "i915_reg.h"
> > +#include "display/intel_bw.h"
> >  
> >  struct drm_device;
> >  struct drm_i915_private;
> > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala April 30, 2020, 9:25 a.m. UTC | #3

On Thu, Apr 30, 2020 at 12:13:35PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > > Future platforms require per-crtc SAGV evaluation
> > > and serializing global state when those are changed
> > > from different commits.
> > > 
> > > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > >       so that it sets bit in reject mask.
> > >     - Use bw_state in intel_pre/post_plane_enable_sagv
> > >       instead of atomic state
> > > 
> > > v3: - Fixed rebase conflict, now using
> > >       intel_atomic_crtc_state_for_each_plane_state in
> > >       order to call it from atomic check
> > > v4: - Use fb modifier from plane state
> > > 
> > > v5: - Make intel_has_sagv static again(Ville)
> > >     - Removed unnecessary NULL assignments(Ville)
> > >     - Removed unnecessary SAGV debug(Ville)
> > >     - Call intel_compute_sagv_mask only for modesets(Ville)
> > >     - Serialize global state only if sagv results change, but
> > >       not mask itself(Ville)
> > > 
> > > v6: - use lock global state instead of serialize(Ville)
> > 
> > What I meant is that we need both. Serialize if sagv state is going to
> > change, otherwise lock if the mask changes.
> 
> As I understand whenever we modify global state but not a real hw, we do
> only global state locking - pipe sagv mask is not actually a hw, but just
> a virtual thing. It affects the QGV points we enable and if it happens to
> affect those in a way that those change - we'll any way have serialize
> called from intel_bw.c. Thus shouldn't be an issue.

I don't like the code to rely on magic happening elsewhere. IMO
it just makes it hard to reason about the logic when you have
constantly remind youself what may or may not happen some other
piece of code. Also we don't even have qgv points on all the
platforms, so presumably we may not even excute that other
piece of code always?

> 
> I can change it anyway of course.
> 
> Stan
> 
> > 
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: James Ausmus <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> > >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> > >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> > >  3 files changed, 93 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > index ac004d6f4276..d6df91058223 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > >  struct intel_bw_state {
> > >  	struct intel_global_state base;
> > >  
> > > +	/*
> > > +	 * Contains a bit mask, used to determine, whether correspondent
> > > +	 * pipe allows SAGV or not.
> > > +	 */
> > > +	u8 pipe_sagv_reject;
> > > +
> > >  	unsigned int data_rate[I915_MAX_PIPES];
> > >  	u8 num_active_planes[I915_MAX_PIPES];
> > >  };
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 338a82577b76..7e15cf3368ad 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -43,6 +43,7 @@
> > >  #include "i915_fixed.h"
> > >  #include "i915_irq.h"
> > >  #include "i915_trace.h"
> > > +#include "display/intel_bw.h"
> > >  #include "intel_pm.h"
> > >  #include "intel_sideband.h"
> > >  #include "../../../platform/x86/intel_ips.h"
> > > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	const struct intel_bw_state *new_bw_state;
> > >  
> > > -	if (!intel_can_enable_sagv(state))
> > > +	/*
> > > +	 * Just return if we can't control SAGV or don't have it.
> > > +	 * This is different from situation when we have SAGV but just can't
> > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > +	 * as it will throw an error. So have to check it here.
> > > +	 */
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return;
> > > +
> > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > +	if (!new_bw_state)
> > > +		return;
> > > +
> > > +	if (!intel_can_enable_sagv(new_bw_state))
> > >  		intel_disable_sagv(dev_priv);
> > >  }
> > >  
> > >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	const struct intel_bw_state *new_bw_state;
> > >  
> > > -	if (intel_can_enable_sagv(state))
> > > +	/*
> > > +	 * Just return if we can't control SAGV or don't have it.
> > > +	 * This is different from situation when we have SAGV but just can't
> > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > +	 * as it will throw an error. So have to check it here.
> > > +	 */
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return;
> > > +
> > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > +	if (!new_bw_state)
> > > +		return;
> > > +
> > > +	if (intel_can_enable_sagv(new_bw_state))
> > >  		intel_enable_sagv(dev_priv);
> > >  }
> > >  
> > >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > >  {
> > > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	struct intel_plane *plane;
> > > +	const struct intel_plane_state *plane_state;
> > >  	int level, latency;
> > >  
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return false;
> > > +
> > >  	if (!crtc_state->hw.active)
> > >  		return true;
> > >  
> > > +	/*
> > > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > +	 * more then one pipe enabled
> > > +	 */
> > > +	if (hweight8(state->active_pipes) > 1)
> > > +		return false;
> > > +
> > >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > >  		return false;
> > >  
> > > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > >  		const struct skl_plane_wm *wm =
> > >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> > >  
> > > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > >  		latency = dev_priv->wm.skl_latency[level];
> > >  
> > >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > > -		    plane->base.state->fb->modifier ==
> > > +		    plane_state->uapi.fb->modifier ==
> > >  		    I915_FORMAT_MOD_X_TILED)
> > >  			latency += 15;
> > >  
> > > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > >  	return true;
> > >  }
> > >  
> > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > >  {
> > > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	return bw_state->pipe_sagv_reject == 0;
> > > +}
> > > +
> > > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > +{
> > > +	int ret;
> > >  	struct intel_crtc *crtc;
> > > -	const struct intel_crtc_state *crtc_state;
> > > -	enum pipe pipe;
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_bw_state *new_bw_state = NULL;
> > > +	const struct intel_bw_state *old_bw_state = NULL;
> > > +	int i;
> > >  
> > > -	if (!intel_has_sagv(dev_priv))
> > > -		return false;
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		new_bw_state = intel_atomic_get_bw_state(state);
> > > +		if (IS_ERR(new_bw_state))
> > > +			return PTR_ERR(new_bw_state);
> > >  
> > > -	/*
> > > -	 * If there are no active CRTCs, no additional checks need be performed
> > > -	 */
> > > -	if (hweight8(state->active_pipes) == 0)
> > > -		return true;
> > > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> > >  
> > > -	/*
> > > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > -	 * more then one pipe enabled
> > > -	 */
> > > -	if (hweight8(state->active_pipes) > 1)
> > > -		return false;
> > > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > +		else
> > > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > +	}
> > >  
> > > -	/* Since we're now guaranteed to only have one active CRTC... */
> > > -	pipe = ffs(state->active_pipes) - 1;
> > > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > +	if (!new_bw_state)
> > > +		return 0;
> > >  
> > > -	return intel_crtc_can_enable_sagv(crtc_state);
> > > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (state->modeset) {
> > > +		ret = intel_compute_sagv_mask(state);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >  	/*
> > >  	 * skl_compute_ddb() will have adjusted the final watermarks
> > >  	 * based on how much ddb is available. Now we can actually
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > index 9a6036ab0f90..fd1dc422e6c5 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > @@ -9,6 +9,7 @@
> > >  #include <linux/types.h>
> > >  
> > >  #include "i915_reg.h"
> > > +#include "display/intel_bw.h"
> > >  
> > >  struct drm_device;
> > >  struct drm_i915_private;
> > > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > -- 
> > > 2.24.1.485.gad05a3d8e5
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Stanislav Lisovskiy April 30, 2020, 9:52 a.m. UTC | #4

On Thu, Apr 30, 2020 at 12:25:38PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 30, 2020 at 12:13:35PM +0300, Lisovskiy, Stanislav wrote:
> > On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> > > On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > > > Future platforms require per-crtc SAGV evaluation
> > > > and serializing global state when those are changed
> > > > from different commits.
> > > > 
> > > > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > > >       so that it sets bit in reject mask.
> > > >     - Use bw_state in intel_pre/post_plane_enable_sagv
> > > >       instead of atomic state
> > > > 
> > > > v3: - Fixed rebase conflict, now using
> > > >       intel_atomic_crtc_state_for_each_plane_state in
> > > >       order to call it from atomic check
> > > > v4: - Use fb modifier from plane state
> > > > 
> > > > v5: - Make intel_has_sagv static again(Ville)
> > > >     - Removed unnecessary NULL assignments(Ville)
> > > >     - Removed unnecessary SAGV debug(Ville)
> > > >     - Call intel_compute_sagv_mask only for modesets(Ville)
> > > >     - Serialize global state only if sagv results change, but
> > > >       not mask itself(Ville)
> > > > 
> > > > v6: - use lock global state instead of serialize(Ville)
> > > 
> > > What I meant is that we need both. Serialize if sagv state is going to
> > > change, otherwise lock if the mask changes.
> > 
> > As I understand whenever we modify global state but not a real hw, we do
> > only global state locking - pipe sagv mask is not actually a hw, but just
> > a virtual thing. It affects the QGV points we enable and if it happens to
> > affect those in a way that those change - we'll any way have serialize
> > called from intel_bw.c. Thus shouldn't be an issue.
> 
> I don't like the code to rely on magic happening elsewhere. IMO
> it just makes it hard to reason about the logic when you have
> constantly remind youself what may or may not happen some other
> piece of code. Also we don't even have qgv points on all the
> platforms, so presumably we may not even excute that other
> piece of code always?

Agree, sounds reasonable. Would be cool may be to unite both serialize
and global state locking, under some helper function, so that same
code snippet is not copy-paste all over the place.

like intel_lock_or_serialize_state(state, bool global_state_changed, bool hw_state_changed) 

Stan

> 
> > 
> > I can change it anyway of course.
> > 
> > Stan
> > 
> > > 
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: James Ausmus <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> > > >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> > > >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> > > >  3 files changed, 93 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > index ac004d6f4276..d6df91058223 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > > >  struct intel_bw_state {
> > > >  	struct intel_global_state base;
> > > >  
> > > > +	/*
> > > > +	 * Contains a bit mask, used to determine, whether correspondent
> > > > +	 * pipe allows SAGV or not.
> > > > +	 */
> > > > +	u8 pipe_sagv_reject;
> > > > +
> > > >  	unsigned int data_rate[I915_MAX_PIPES];
> > > >  	u8 num_active_planes[I915_MAX_PIPES];
> > > >  };
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 338a82577b76..7e15cf3368ad 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -43,6 +43,7 @@
> > > >  #include "i915_fixed.h"
> > > >  #include "i915_irq.h"
> > > >  #include "i915_trace.h"
> > > > +#include "display/intel_bw.h"
> > > >  #include "intel_pm.h"
> > > >  #include "intel_sideband.h"
> > > >  #include "../../../platform/x86/intel_ips.h"
> > > > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	const struct intel_bw_state *new_bw_state;
> > > >  
> > > > -	if (!intel_can_enable_sagv(state))
> > > > +	/*
> > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > +	 * This is different from situation when we have SAGV but just can't
> > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > +	 * as it will throw an error. So have to check it here.
> > > > +	 */
> > > > +	if (!intel_has_sagv(dev_priv))
> > > > +		return;
> > > > +
> > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > +	if (!new_bw_state)
> > > > +		return;
> > > > +
> > > > +	if (!intel_can_enable_sagv(new_bw_state))
> > > >  		intel_disable_sagv(dev_priv);
> > > >  }
> > > >  
> > > >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	const struct intel_bw_state *new_bw_state;
> > > >  
> > > > -	if (intel_can_enable_sagv(state))
> > > > +	/*
> > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > +	 * This is different from situation when we have SAGV but just can't
> > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > +	 * as it will throw an error. So have to check it here.
> > > > +	 */
> > > > +	if (!intel_has_sagv(dev_priv))
> > > > +		return;
> > > > +
> > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > +	if (!new_bw_state)
> > > > +		return;
> > > > +
> > > > +	if (intel_can_enable_sagv(new_bw_state))
> > > >  		intel_enable_sagv(dev_priv);
> > > >  }
> > > >  
> > > >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > >  	struct intel_plane *plane;
> > > > +	const struct intel_plane_state *plane_state;
> > > >  	int level, latency;
> > > >  
> > > > +	if (!intel_has_sagv(dev_priv))
> > > > +		return false;
> > > > +
> > > >  	if (!crtc_state->hw.active)
> > > >  		return true;
> > > >  
> > > > +	/*
> > > > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > +	 * more then one pipe enabled
> > > > +	 */
> > > > +	if (hweight8(state->active_pipes) > 1)
> > > > +		return false;
> > > > +
> > > >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > > >  		return false;
> > > >  
> > > > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > > >  		const struct skl_plane_wm *wm =
> > > >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> > > >  
> > > > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > >  		latency = dev_priv->wm.skl_latency[level];
> > > >  
> > > >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > > > -		    plane->base.state->fb->modifier ==
> > > > +		    plane_state->uapi.fb->modifier ==
> > > >  		    I915_FORMAT_MOD_X_TILED)
> > > >  			latency += 15;
> > > >  
> > > > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > >  	return true;
> > > >  }
> > > >  
> > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > > >  {
> > > > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	return bw_state->pipe_sagv_reject == 0;
> > > > +}
> > > > +
> > > > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > +{
> > > > +	int ret;
> > > >  	struct intel_crtc *crtc;
> > > > -	const struct intel_crtc_state *crtc_state;
> > > > -	enum pipe pipe;
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_bw_state *new_bw_state = NULL;
> > > > +	const struct intel_bw_state *old_bw_state = NULL;
> > > > +	int i;
> > > >  
> > > > -	if (!intel_has_sagv(dev_priv))
> > > > -		return false;
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		new_bw_state = intel_atomic_get_bw_state(state);
> > > > +		if (IS_ERR(new_bw_state))
> > > > +			return PTR_ERR(new_bw_state);
> > > >  
> > > > -	/*
> > > > -	 * If there are no active CRTCs, no additional checks need be performed
> > > > -	 */
> > > > -	if (hweight8(state->active_pipes) == 0)
> > > > -		return true;
> > > > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> > > >  
> > > > -	/*
> > > > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > -	 * more then one pipe enabled
> > > > -	 */
> > > > -	if (hweight8(state->active_pipes) > 1)
> > > > -		return false;
> > > > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > > > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > > +		else
> > > > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > > +	}
> > > >  
> > > > -	/* Since we're now guaranteed to only have one active CRTC... */
> > > > -	pipe = ffs(state->active_pipes) - 1;
> > > > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > +	if (!new_bw_state)
> > > > +		return 0;
> > > >  
> > > > -	return intel_crtc_can_enable_sagv(crtc_state);
> > > > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > > > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  /*
> > > > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > +	if (state->modeset) {
> > > > +		ret = intel_compute_sagv_mask(state);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * skl_compute_ddb() will have adjusted the final watermarks
> > > >  	 * based on how much ddb is available. Now we can actually
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > > index 9a6036ab0f90..fd1dc422e6c5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > > @@ -9,6 +9,7 @@
> > > >  #include <linux/types.h>
> > > >  
> > > >  #include "i915_reg.h"
> > > > +#include "display/intel_bw.h"
> > > >  
> > > >  struct drm_device;
> > > >  struct drm_i915_private;
> > > > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > > -- 
> > > > 2.24.1.485.gad05a3d8e5
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala April 30, 2020, 10:08 a.m. UTC | #5

On Thu, Apr 30, 2020 at 12:52:42PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Apr 30, 2020 at 12:25:38PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 30, 2020 at 12:13:35PM +0300, Lisovskiy, Stanislav wrote:
> > > On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > > > > Future platforms require per-crtc SAGV evaluation
> > > > > and serializing global state when those are changed
> > > > > from different commits.
> > > > > 
> > > > > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > > > >       so that it sets bit in reject mask.
> > > > >     - Use bw_state in intel_pre/post_plane_enable_sagv
> > > > >       instead of atomic state
> > > > > 
> > > > > v3: - Fixed rebase conflict, now using
> > > > >       intel_atomic_crtc_state_for_each_plane_state in
> > > > >       order to call it from atomic check
> > > > > v4: - Use fb modifier from plane state
> > > > > 
> > > > > v5: - Make intel_has_sagv static again(Ville)
> > > > >     - Removed unnecessary NULL assignments(Ville)
> > > > >     - Removed unnecessary SAGV debug(Ville)
> > > > >     - Call intel_compute_sagv_mask only for modesets(Ville)
> > > > >     - Serialize global state only if sagv results change, but
> > > > >       not mask itself(Ville)
> > > > > 
> > > > > v6: - use lock global state instead of serialize(Ville)
> > > > 
> > > > What I meant is that we need both. Serialize if sagv state is going to
> > > > change, otherwise lock if the mask changes.
> > > 
> > > As I understand whenever we modify global state but not a real hw, we do
> > > only global state locking - pipe sagv mask is not actually a hw, but just
> > > a virtual thing. It affects the QGV points we enable and if it happens to
> > > affect those in a way that those change - we'll any way have serialize
> > > called from intel_bw.c. Thus shouldn't be an issue.
> > 
> > I don't like the code to rely on magic happening elsewhere. IMO
> > it just makes it hard to reason about the logic when you have
> > constantly remind youself what may or may not happen some other
> > piece of code. Also we don't even have qgv points on all the
> > platforms, so presumably we may not even excute that other
> > piece of code always?
> 
> Agree, sounds reasonable. Would be cool may be to unite both serialize
> and global state locking, under some helper function, so that same
> code snippet is not copy-paste all over the place.
> 
> like intel_lock_or_serialize_state(state, bool global_state_changed, bool hw_state_changed) 

intel_lock_or_serialize_state(state,
			      a != b,
			      x != y);

doesn't really make the intent clear at all. So not really in favor of a
function that takes two booleans.

> 
> Stan
> 
> > 
> > > 
> > > I can change it anyway of course.
> > > 
> > > Stan
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > Cc: James Ausmus <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> > > > >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> > > > >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> > > > >  3 files changed, 93 insertions(+), 29 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > index ac004d6f4276..d6df91058223 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > > > >  struct intel_bw_state {
> > > > >  	struct intel_global_state base;
> > > > >  
> > > > > +	/*
> > > > > +	 * Contains a bit mask, used to determine, whether correspondent
> > > > > +	 * pipe allows SAGV or not.
> > > > > +	 */
> > > > > +	u8 pipe_sagv_reject;
> > > > > +
> > > > >  	unsigned int data_rate[I915_MAX_PIPES];
> > > > >  	u8 num_active_planes[I915_MAX_PIPES];
> > > > >  };
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 338a82577b76..7e15cf3368ad 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -43,6 +43,7 @@
> > > > >  #include "i915_fixed.h"
> > > > >  #include "i915_irq.h"
> > > > >  #include "i915_trace.h"
> > > > > +#include "display/intel_bw.h"
> > > > >  #include "intel_pm.h"
> > > > >  #include "intel_sideband.h"
> > > > >  #include "../../../platform/x86/intel_ips.h"
> > > > > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	const struct intel_bw_state *new_bw_state;
> > > > >  
> > > > > -	if (!intel_can_enable_sagv(state))
> > > > > +	/*
> > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > +	 * as it will throw an error. So have to check it here.
> > > > > +	 */
> > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > +		return;
> > > > > +
> > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > +	if (!new_bw_state)
> > > > > +		return;
> > > > > +
> > > > > +	if (!intel_can_enable_sagv(new_bw_state))
> > > > >  		intel_disable_sagv(dev_priv);
> > > > >  }
> > > > >  
> > > > >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	const struct intel_bw_state *new_bw_state;
> > > > >  
> > > > > -	if (intel_can_enable_sagv(state))
> > > > > +	/*
> > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > +	 * as it will throw an error. So have to check it here.
> > > > > +	 */
> > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > +		return;
> > > > > +
> > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > +	if (!new_bw_state)
> > > > > +		return;
> > > > > +
> > > > > +	if (intel_can_enable_sagv(new_bw_state))
> > > > >  		intel_enable_sagv(dev_priv);
> > > > >  }
> > > > >  
> > > > >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > > > >  {
> > > > > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > > > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > >  	struct intel_plane *plane;
> > > > > +	const struct intel_plane_state *plane_state;
> > > > >  	int level, latency;
> > > > >  
> > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > +		return false;
> > > > > +
> > > > >  	if (!crtc_state->hw.active)
> > > > >  		return true;
> > > > >  
> > > > > +	/*
> > > > > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > +	 * more then one pipe enabled
> > > > > +	 */
> > > > > +	if (hweight8(state->active_pipes) > 1)
> > > > > +		return false;
> > > > > +
> > > > >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > > > >  		return false;
> > > > >  
> > > > > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > > > >  		const struct skl_plane_wm *wm =
> > > > >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> > > > >  
> > > > > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > >  		latency = dev_priv->wm.skl_latency[level];
> > > > >  
> > > > >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > > > > -		    plane->base.state->fb->modifier ==
> > > > > +		    plane_state->uapi.fb->modifier ==
> > > > >  		    I915_FORMAT_MOD_X_TILED)
> > > > >  			latency += 15;
> > > > >  
> > > > > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > > > >  {
> > > > > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	return bw_state->pipe_sagv_reject == 0;
> > > > > +}
> > > > > +
> > > > > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > > +{
> > > > > +	int ret;
> > > > >  	struct intel_crtc *crtc;
> > > > > -	const struct intel_crtc_state *crtc_state;
> > > > > -	enum pipe pipe;
> > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > +	struct intel_bw_state *new_bw_state = NULL;
> > > > > +	const struct intel_bw_state *old_bw_state = NULL;
> > > > > +	int i;
> > > > >  
> > > > > -	if (!intel_has_sagv(dev_priv))
> > > > > -		return false;
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > +					 new_crtc_state, i) {
> > > > > +		new_bw_state = intel_atomic_get_bw_state(state);
> > > > > +		if (IS_ERR(new_bw_state))
> > > > > +			return PTR_ERR(new_bw_state);
> > > > >  
> > > > > -	/*
> > > > > -	 * If there are no active CRTCs, no additional checks need be performed
> > > > > -	 */
> > > > > -	if (hweight8(state->active_pipes) == 0)
> > > > > -		return true;
> > > > > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> > > > >  
> > > > > -	/*
> > > > > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > -	 * more then one pipe enabled
> > > > > -	 */
> > > > > -	if (hweight8(state->active_pipes) > 1)
> > > > > -		return false;
> > > > > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > > > > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > > > +		else
> > > > > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > > > +	}
> > > > >  
> > > > > -	/* Since we're now guaranteed to only have one active CRTC... */
> > > > > -	pipe = ffs(state->active_pipes) - 1;
> > > > > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > > > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > > +	if (!new_bw_state)
> > > > > +		return 0;
> > > > >  
> > > > > -	return intel_crtc_can_enable_sagv(crtc_state);
> > > > > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > > > > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > +	if (state->modeset) {
> > > > > +		ret = intel_compute_sagv_mask(state);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > >  	/*
> > > > >  	 * skl_compute_ddb() will have adjusted the final watermarks
> > > > >  	 * based on how much ddb is available. Now we can actually
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > > > index 9a6036ab0f90..fd1dc422e6c5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > > > @@ -9,6 +9,7 @@
> > > > >  #include <linux/types.h>
> > > > >  
> > > > >  #include "i915_reg.h"
> > > > > +#include "display/intel_bw.h"
> > > > >  
> > > > >  struct drm_device;
> > > > >  struct drm_i915_private;
> > > > > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > > > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > > > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > > > -- 
> > > > > 2.24.1.485.gad05a3d8e5
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Stanislav Lisovskiy April 30, 2020, 10:14 a.m. UTC | #6

On Thu, Apr 30, 2020 at 01:08:20PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 30, 2020 at 12:52:42PM +0300, Lisovskiy, Stanislav wrote:
> > On Thu, Apr 30, 2020 at 12:25:38PM +0300, Ville Syrjälä wrote:
> > > On Thu, Apr 30, 2020 at 12:13:35PM +0300, Lisovskiy, Stanislav wrote:
> > > > On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> > > > > On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > > > > > Future platforms require per-crtc SAGV evaluation
> > > > > > and serializing global state when those are changed
> > > > > > from different commits.
> > > > > > 
> > > > > > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > > > > >       so that it sets bit in reject mask.
> > > > > >     - Use bw_state in intel_pre/post_plane_enable_sagv
> > > > > >       instead of atomic state
> > > > > > 
> > > > > > v3: - Fixed rebase conflict, now using
> > > > > >       intel_atomic_crtc_state_for_each_plane_state in
> > > > > >       order to call it from atomic check
> > > > > > v4: - Use fb modifier from plane state
> > > > > > 
> > > > > > v5: - Make intel_has_sagv static again(Ville)
> > > > > >     - Removed unnecessary NULL assignments(Ville)
> > > > > >     - Removed unnecessary SAGV debug(Ville)
> > > > > >     - Call intel_compute_sagv_mask only for modesets(Ville)
> > > > > >     - Serialize global state only if sagv results change, but
> > > > > >       not mask itself(Ville)
> > > > > > 
> > > > > > v6: - use lock global state instead of serialize(Ville)
> > > > > 
> > > > > What I meant is that we need both. Serialize if sagv state is going to
> > > > > change, otherwise lock if the mask changes.
> > > > 
> > > > As I understand whenever we modify global state but not a real hw, we do
> > > > only global state locking - pipe sagv mask is not actually a hw, but just
> > > > a virtual thing. It affects the QGV points we enable and if it happens to
> > > > affect those in a way that those change - we'll any way have serialize
> > > > called from intel_bw.c. Thus shouldn't be an issue.
> > > 
> > > I don't like the code to rely on magic happening elsewhere. IMO
> > > it just makes it hard to reason about the logic when you have
> > > constantly remind youself what may or may not happen some other
> > > piece of code. Also we don't even have qgv points on all the
> > > platforms, so presumably we may not even excute that other
> > > piece of code always?
> > 
> > Agree, sounds reasonable. Would be cool may be to unite both serialize
> > and global state locking, under some helper function, so that same
> > code snippet is not copy-paste all over the place.
> > 
> > like intel_lock_or_serialize_state(state, bool global_state_changed, bool hw_state_changed) 
> 
> intel_lock_or_serialize_state(state,
> 			      a != b,
> 			      x != y);
> 
> doesn't really make the intent clear at all. So not really in favor of a
> function that takes two booleans.

bool global_state_changed = new_sagv_pipe_mask != old_sagv_pipe_mask;
bool hw_state_changed = new_can_enable_sagv != old_can_enable_sagv;

intel_lock_or_serialize(state, global_state_changed, hw_state_changed);

I think together with proper comment this looks pretty clear and also
eliminates the need in duplicating conditions all over the place.

Just a proposal though.

Stan

> 
> > 
> > Stan
> > 
> > > 
> > > > 
> > > > I can change it anyway of course.
> > > > 
> > > > Stan
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > Cc: James Ausmus <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> > > > > >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> > > > > >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> > > > > >  3 files changed, 93 insertions(+), 29 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > index ac004d6f4276..d6df91058223 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > > > > >  struct intel_bw_state {
> > > > > >  	struct intel_global_state base;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * Contains a bit mask, used to determine, whether correspondent
> > > > > > +	 * pipe allows SAGV or not.
> > > > > > +	 */
> > > > > > +	u8 pipe_sagv_reject;
> > > > > > +
> > > > > >  	unsigned int data_rate[I915_MAX_PIPES];
> > > > > >  	u8 num_active_planes[I915_MAX_PIPES];
> > > > > >  };
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 338a82577b76..7e15cf3368ad 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -43,6 +43,7 @@
> > > > > >  #include "i915_fixed.h"
> > > > > >  #include "i915_irq.h"
> > > > > >  #include "i915_trace.h"
> > > > > > +#include "display/intel_bw.h"
> > > > > >  #include "intel_pm.h"
> > > > > >  #include "intel_sideband.h"
> > > > > >  #include "../../../platform/x86/intel_ips.h"
> > > > > > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +	const struct intel_bw_state *new_bw_state;
> > > > > >  
> > > > > > -	if (!intel_can_enable_sagv(state))
> > > > > > +	/*
> > > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > > +	 * as it will throw an error. So have to check it here.
> > > > > > +	 */
> > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > +		return;
> > > > > > +
> > > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > > +	if (!new_bw_state)
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (!intel_can_enable_sagv(new_bw_state))
> > > > > >  		intel_disable_sagv(dev_priv);
> > > > > >  }
> > > > > >  
> > > > > >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +	const struct intel_bw_state *new_bw_state;
> > > > > >  
> > > > > > -	if (intel_can_enable_sagv(state))
> > > > > > +	/*
> > > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > > +	 * as it will throw an error. So have to check it here.
> > > > > > +	 */
> > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > +		return;
> > > > > > +
> > > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > > +	if (!new_bw_state)
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (intel_can_enable_sagv(new_bw_state))
> > > > > >  		intel_enable_sagv(dev_priv);
> > > > > >  }
> > > > > >  
> > > > > >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > > > > >  {
> > > > > > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > > > > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > > > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > > >  	struct intel_plane *plane;
> > > > > > +	const struct intel_plane_state *plane_state;
> > > > > >  	int level, latency;
> > > > > >  
> > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > +		return false;
> > > > > > +
> > > > > >  	if (!crtc_state->hw.active)
> > > > > >  		return true;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > > +	 * more then one pipe enabled
> > > > > > +	 */
> > > > > > +	if (hweight8(state->active_pipes) > 1)
> > > > > > +		return false;
> > > > > > +
> > > > > >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > > > > >  		return false;
> > > > > >  
> > > > > > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > > > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > > > > >  		const struct skl_plane_wm *wm =
> > > > > >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> > > > > >  
> > > > > > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > > >  		latency = dev_priv->wm.skl_latency[level];
> > > > > >  
> > > > > >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > > > > > -		    plane->base.state->fb->modifier ==
> > > > > > +		    plane_state->uapi.fb->modifier ==
> > > > > >  		    I915_FORMAT_MOD_X_TILED)
> > > > > >  			latency += 15;
> > > > > >  
> > > > > > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > > >  	return true;
> > > > > >  }
> > > > > >  
> > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > > > > >  {
> > > > > > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +	return bw_state->pipe_sagv_reject == 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > > > +{
> > > > > > +	int ret;
> > > > > >  	struct intel_crtc *crtc;
> > > > > > -	const struct intel_crtc_state *crtc_state;
> > > > > > -	enum pipe pipe;
> > > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > > +	struct intel_bw_state *new_bw_state = NULL;
> > > > > > +	const struct intel_bw_state *old_bw_state = NULL;
> > > > > > +	int i;
> > > > > >  
> > > > > > -	if (!intel_has_sagv(dev_priv))
> > > > > > -		return false;
> > > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > > +					 new_crtc_state, i) {
> > > > > > +		new_bw_state = intel_atomic_get_bw_state(state);
> > > > > > +		if (IS_ERR(new_bw_state))
> > > > > > +			return PTR_ERR(new_bw_state);
> > > > > >  
> > > > > > -	/*
> > > > > > -	 * If there are no active CRTCs, no additional checks need be performed
> > > > > > -	 */
> > > > > > -	if (hweight8(state->active_pipes) == 0)
> > > > > > -		return true;
> > > > > > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> > > > > >  
> > > > > > -	/*
> > > > > > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > > -	 * more then one pipe enabled
> > > > > > -	 */
> > > > > > -	if (hweight8(state->active_pipes) > 1)
> > > > > > -		return false;
> > > > > > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > > > > > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > > > > +		else
> > > > > > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > > > > +	}
> > > > > >  
> > > > > > -	/* Since we're now guaranteed to only have one active CRTC... */
> > > > > > -	pipe = ffs(state->active_pipes) - 1;
> > > > > > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > > > > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > > > +	if (!new_bw_state)
> > > > > > +		return 0;
> > > > > >  
> > > > > > -	return intel_crtc_can_enable_sagv(crtc_state);
> > > > > > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > > > > > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > > >  
> > > > > > +	if (state->modeset) {
> > > > > > +		ret = intel_compute_sagv_mask(state);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > >  	/*
> > > > > >  	 * skl_compute_ddb() will have adjusted the final watermarks
> > > > > >  	 * based on how much ddb is available. Now we can actually
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > > > > index 9a6036ab0f90..fd1dc422e6c5 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > > > > @@ -9,6 +9,7 @@
> > > > > >  #include <linux/types.h>
> > > > > >  
> > > > > >  #include "i915_reg.h"
> > > > > > +#include "display/intel_bw.h"
> > > > > >  
> > > > > >  struct drm_device;
> > > > > >  struct drm_i915_private;
> > > > > > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > > > > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > > > > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > > > > -- 
> > > > > > 2.24.1.485.gad05a3d8e5
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala April 30, 2020, 10:37 a.m. UTC | #7

On Thu, Apr 30, 2020 at 01:14:57PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Apr 30, 2020 at 01:08:20PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 30, 2020 at 12:52:42PM +0300, Lisovskiy, Stanislav wrote:
> > > On Thu, Apr 30, 2020 at 12:25:38PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Apr 30, 2020 at 12:13:35PM +0300, Lisovskiy, Stanislav wrote:
> > > > > On Thu, Apr 30, 2020 at 12:09:22PM +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Apr 23, 2020 at 10:58:55AM +0300, Stanislav Lisovskiy wrote:
> > > > > > > Future platforms require per-crtc SAGV evaluation
> > > > > > > and serializing global state when those are changed
> > > > > > > from different commits.
> > > > > > > 
> > > > > > > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > > > > > >       so that it sets bit in reject mask.
> > > > > > >     - Use bw_state in intel_pre/post_plane_enable_sagv
> > > > > > >       instead of atomic state
> > > > > > > 
> > > > > > > v3: - Fixed rebase conflict, now using
> > > > > > >       intel_atomic_crtc_state_for_each_plane_state in
> > > > > > >       order to call it from atomic check
> > > > > > > v4: - Use fb modifier from plane state
> > > > > > > 
> > > > > > > v5: - Make intel_has_sagv static again(Ville)
> > > > > > >     - Removed unnecessary NULL assignments(Ville)
> > > > > > >     - Removed unnecessary SAGV debug(Ville)
> > > > > > >     - Call intel_compute_sagv_mask only for modesets(Ville)
> > > > > > >     - Serialize global state only if sagv results change, but
> > > > > > >       not mask itself(Ville)
> > > > > > > 
> > > > > > > v6: - use lock global state instead of serialize(Ville)
> > > > > > 
> > > > > > What I meant is that we need both. Serialize if sagv state is going to
> > > > > > change, otherwise lock if the mask changes.
> > > > > 
> > > > > As I understand whenever we modify global state but not a real hw, we do
> > > > > only global state locking - pipe sagv mask is not actually a hw, but just
> > > > > a virtual thing. It affects the QGV points we enable and if it happens to
> > > > > affect those in a way that those change - we'll any way have serialize
> > > > > called from intel_bw.c. Thus shouldn't be an issue.
> > > > 
> > > > I don't like the code to rely on magic happening elsewhere. IMO
> > > > it just makes it hard to reason about the logic when you have
> > > > constantly remind youself what may or may not happen some other
> > > > piece of code. Also we don't even have qgv points on all the
> > > > platforms, so presumably we may not even excute that other
> > > > piece of code always?
> > > 
> > > Agree, sounds reasonable. Would be cool may be to unite both serialize
> > > and global state locking, under some helper function, so that same
> > > code snippet is not copy-paste all over the place.
> > > 
> > > like intel_lock_or_serialize_state(state, bool global_state_changed, bool hw_state_changed) 
> > 
> > intel_lock_or_serialize_state(state,
> > 			      a != b,
> > 			      x != y);
> > 
> > doesn't really make the intent clear at all. So not really in favor of a
> > function that takes two booleans.
> 
> bool global_state_changed = new_sagv_pipe_mask != old_sagv_pipe_mask;
> bool hw_state_changed = new_can_enable_sagv != old_can_enable_sagv;

If we consistently make those functions instead of
variables then I could probably buy it.

> 
> intel_lock_or_serialize(state, global_state_changed, hw_state_changed);
> 
> I think together with proper comment this looks pretty clear and also
> eliminates the need in duplicating conditions all over the place.

If we have to add a comment I'd say the plan has already
failed. Good code needs no comments.

> 
> Just a proposal though.
> 
> Stan
> 
> > 
> > > 
> > > Stan
> > > 
> > > > 
> > > > > 
> > > > > I can change it anyway of course.
> > > > > 
> > > > > Stan
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Stanislav Lisovskiy <[e-mail]>
> > > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > > Cc: James Ausmus <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/display/intel_bw.h |   6 ++
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c         | 113 ++++++++++++++++++------
> > > > > > >  drivers/gpu/drm/i915/intel_pm.h         |   3 +-
> > > > > > >  3 files changed, 93 insertions(+), 29 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > > index ac004d6f4276..d6df91058223 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > > > > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > > > > > >  struct intel_bw_state {
> > > > > > >  	struct intel_global_state base;
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * Contains a bit mask, used to determine, whether correspondent
> > > > > > > +	 * pipe allows SAGV or not.
> > > > > > > +	 */
> > > > > > > +	u8 pipe_sagv_reject;
> > > > > > > +
> > > > > > >  	unsigned int data_rate[I915_MAX_PIPES];
> > > > > > >  	u8 num_active_planes[I915_MAX_PIPES];
> > > > > > >  };
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > index 338a82577b76..7e15cf3368ad 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > @@ -43,6 +43,7 @@
> > > > > > >  #include "i915_fixed.h"
> > > > > > >  #include "i915_irq.h"
> > > > > > >  #include "i915_trace.h"
> > > > > > > +#include "display/intel_bw.h"
> > > > > > >  #include "intel_pm.h"
> > > > > > >  #include "intel_sideband.h"
> > > > > > >  #include "../../../platform/x86/intel_ips.h"
> > > > > > > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > > > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +	const struct intel_bw_state *new_bw_state;
> > > > > > >  
> > > > > > > -	if (!intel_can_enable_sagv(state))
> > > > > > > +	/*
> > > > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > > > +	 * as it will throw an error. So have to check it here.
> > > > > > > +	 */
> > > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > > > +	if (!new_bw_state)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (!intel_can_enable_sagv(new_bw_state))
> > > > > > >  		intel_disable_sagv(dev_priv);
> > > > > > >  }
> > > > > > >  
> > > > > > >  void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +	const struct intel_bw_state *new_bw_state;
> > > > > > >  
> > > > > > > -	if (intel_can_enable_sagv(state))
> > > > > > > +	/*
> > > > > > > +	 * Just return if we can't control SAGV or don't have it.
> > > > > > > +	 * This is different from situation when we have SAGV but just can't
> > > > > > > +	 * afford it due to DBuf limitation - in case if SAGV is completely
> > > > > > > +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> > > > > > > +	 * as it will throw an error. So have to check it here.
> > > > > > > +	 */
> > > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > > > > > +	if (!new_bw_state)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (intel_can_enable_sagv(new_bw_state))
> > > > > > >  		intel_enable_sagv(dev_priv);
> > > > > > >  }
> > > > > > >  
> > > > > > >  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > > > > > >  {
> > > > > > > -	struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > > > > > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > > > > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > > > >  	struct intel_plane *plane;
> > > > > > > +	const struct intel_plane_state *plane_state;
> > > > > > >  	int level, latency;
> > > > > > >  
> > > > > > > +	if (!intel_has_sagv(dev_priv))
> > > > > > > +		return false;
> > > > > > > +
> > > > > > >  	if (!crtc_state->hw.active)
> > > > > > >  		return true;
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > > > +	 * more then one pipe enabled
> > > > > > > +	 */
> > > > > > > +	if (hweight8(state->active_pipes) > 1)
> > > > > > > +		return false;
> > > > > > > +
> > > > > > >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > > > > > >  		return false;
> > > > > > >  
> > > > > > > -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > > > > +	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > > > > > >  		const struct skl_plane_wm *wm =
> > > > > > >  			&crtc_state->wm.skl.optimal.planes[plane->id];
> > > > > > >  
> > > > > > > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > > > >  		latency = dev_priv->wm.skl_latency[level];
> > > > > > >  
> > > > > > >  		if (skl_needs_memory_bw_wa(dev_priv) &&
> > > > > > > -		    plane->base.state->fb->modifier ==
> > > > > > > +		    plane_state->uapi.fb->modifier ==
> > > > > > >  		    I915_FORMAT_MOD_X_TILED)
> > > > > > >  			latency += 15;
> > > > > > >  
> > > > > > > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > > > > > >  	return true;
> > > > > > >  }
> > > > > > >  
> > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > > > > > >  {
> > > > > > > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +	return bw_state->pipe_sagv_reject == 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > >  	struct intel_crtc *crtc;
> > > > > > > -	const struct intel_crtc_state *crtc_state;
> > > > > > > -	enum pipe pipe;
> > > > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > > > +	struct intel_bw_state *new_bw_state = NULL;
> > > > > > > +	const struct intel_bw_state *old_bw_state = NULL;
> > > > > > > +	int i;
> > > > > > >  
> > > > > > > -	if (!intel_has_sagv(dev_priv))
> > > > > > > -		return false;
> > > > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > > > +					 new_crtc_state, i) {
> > > > > > > +		new_bw_state = intel_atomic_get_bw_state(state);
> > > > > > > +		if (IS_ERR(new_bw_state))
> > > > > > > +			return PTR_ERR(new_bw_state);
> > > > > > >  
> > > > > > > -	/*
> > > > > > > -	 * If there are no active CRTCs, no additional checks need be performed
> > > > > > > -	 */
> > > > > > > -	if (hweight8(state->active_pipes) == 0)
> > > > > > > -		return true;
> > > > > > > +		old_bw_state = intel_atomic_get_old_bw_state(state);
> > > > > > >  
> > > > > > > -	/*
> > > > > > > -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > > > > -	 * more then one pipe enabled
> > > > > > > -	 */
> > > > > > > -	if (hweight8(state->active_pipes) > 1)
> > > > > > > -		return false;
> > > > > > > +		if (intel_crtc_can_enable_sagv(new_crtc_state))
> > > > > > > +			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > > > > > +		else
> > > > > > > +			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > > > > > +	}
> > > > > > >  
> > > > > > > -	/* Since we're now guaranteed to only have one active CRTC... */
> > > > > > > -	pipe = ffs(state->active_pipes) - 1;
> > > > > > > -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > > > > > -	crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > > > > +	if (!new_bw_state)
> > > > > > > +		return 0;
> > > > > > >  
> > > > > > > -	return intel_crtc_can_enable_sagv(crtc_state);
> > > > > > > +	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
> > > > > > > +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > >  /*
> > > > > > > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > > > > >  	if (ret)
> > > > > > >  		return ret;
> > > > > > >  
> > > > > > > +	if (state->modeset) {
> > > > > > > +		ret = intel_compute_sagv_mask(state);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/*
> > > > > > >  	 * skl_compute_ddb() will have adjusted the final watermarks
> > > > > > >  	 * based on how much ddb is available. Now we can actually
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > > > > > index 9a6036ab0f90..fd1dc422e6c5 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > > > > > @@ -9,6 +9,7 @@
> > > > > > >  #include <linux/types.h>
> > > > > > >  
> > > > > > >  #include "i915_reg.h"
> > > > > > > +#include "display/intel_bw.h"
> > > > > > >  
> > > > > > >  struct drm_device;
> > > > > > >  struct drm_i915_private;
> > > > > > > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
> > > > > > > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > > > > > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > > > > > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > > > > > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > > > > > -- 
> > > > > > > 2.24.1.485.gad05a3d8e5
> > > > > > 
> > > > > > -- 
> > > > > > Ville Syrjälä
> > > > > > Intel
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index ac004d6f4276..d6df91058223 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -18,6 +18,12 @@  struct intel_crtc_state;
 struct intel_bw_state {
 	struct intel_global_state base;
 
+	/*
+	 * Contains a bit mask, used to determine, whether correspondent
+	 * pipe allows SAGV or not.
+	 */
+	u8 pipe_sagv_reject;
+
 	unsigned int data_rate[I915_MAX_PIPES];
 	u8 num_active_planes[I915_MAX_PIPES];
 };
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 338a82577b76..7e15cf3368ad 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -43,6 +43,7 @@ 
 #include "i915_fixed.h"
 #include "i915_irq.h"
 #include "i915_trace.h"
+#include "display/intel_bw.h"
 #include "intel_pm.h"
 #include "intel_sideband.h"
 #include "../../../platform/x86/intel_ips.h"
@@ -3760,34 +3761,75 @@  intel_disable_sagv(struct drm_i915_private *dev_priv)
 void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_bw_state *new_bw_state;
 
-	if (!intel_can_enable_sagv(state))
+	/*
+	 * Just return if we can't control SAGV or don't have it.
+	 * This is different from situation when we have SAGV but just can't
+	 * afford it due to DBuf limitation - in case if SAGV is completely
+	 * disabled in a BIOS, we are not even allowed to send a PCode request,
+	 * as it will throw an error. So have to check it here.
+	 */
+	if (!intel_has_sagv(dev_priv))
+		return;
+
+	new_bw_state = intel_atomic_get_new_bw_state(state);
+	if (!new_bw_state)
+		return;
+
+	if (!intel_can_enable_sagv(new_bw_state))
 		intel_disable_sagv(dev_priv);
 }
 
 void intel_sagv_post_plane_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_bw_state *new_bw_state;
 
-	if (intel_can_enable_sagv(state))
+	/*
+	 * Just return if we can't control SAGV or don't have it.
+	 * This is different from situation when we have SAGV but just can't
+	 * afford it due to DBuf limitation - in case if SAGV is completely
+	 * disabled in a BIOS, we are not even allowed to send a PCode request,
+	 * as it will throw an error. So have to check it here.
+	 */
+	if (!intel_has_sagv(dev_priv))
+		return;
+
+	new_bw_state = intel_atomic_get_new_bw_state(state);
+	if (!new_bw_state)
+		return;
+
+	if (intel_can_enable_sagv(new_bw_state))
 		intel_enable_sagv(dev_priv);
 }
 
 static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc_state->uapi.crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_plane *plane;
+	const struct intel_plane_state *plane_state;
 	int level, latency;
 
+	if (!intel_has_sagv(dev_priv))
+		return false;
+
 	if (!crtc_state->hw.active)
 		return true;
 
+	/*
+	 * SKL+ workaround: bspec recommends we disable SAGV when we have
+	 * more then one pipe enabled
+	 */
+	if (hweight8(state->active_pipes) > 1)
+		return false;
+
 	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
 		return false;
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
 		const struct skl_plane_wm *wm =
 			&crtc_state->wm.skl.optimal.planes[plane->id];
 
@@ -3803,7 +3845,7 @@  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
 		latency = dev_priv->wm.skl_latency[level];
 
 		if (skl_needs_memory_bw_wa(dev_priv) &&
-		    plane->base.state->fb->modifier ==
+		    plane_state->uapi.fb->modifier ==
 		    I915_FORMAT_MOD_X_TILED)
 			latency += 15;
 
@@ -3819,35 +3861,44 @@  static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
 	return true;
 }
 
-bool intel_can_enable_sagv(struct intel_atomic_state *state)
+bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
 {
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	return bw_state->pipe_sagv_reject == 0;
+}
+
+static int intel_compute_sagv_mask(struct intel_atomic_state *state)
+{
+	int ret;
 	struct intel_crtc *crtc;
-	const struct intel_crtc_state *crtc_state;
-	enum pipe pipe;
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_bw_state *new_bw_state = NULL;
+	const struct intel_bw_state *old_bw_state = NULL;
+	int i;
 
-	if (!intel_has_sagv(dev_priv))
-		return false;
+	for_each_new_intel_crtc_in_state(state, crtc,
+					 new_crtc_state, i) {
+		new_bw_state = intel_atomic_get_bw_state(state);
+		if (IS_ERR(new_bw_state))
+			return PTR_ERR(new_bw_state);
 
-	/*
-	 * If there are no active CRTCs, no additional checks need be performed
-	 */
-	if (hweight8(state->active_pipes) == 0)
-		return true;
+		old_bw_state = intel_atomic_get_old_bw_state(state);
 
-	/*
-	 * SKL+ workaround: bspec recommends we disable SAGV when we have
-	 * more then one pipe enabled
-	 */
-	if (hweight8(state->active_pipes) > 1)
-		return false;
+		if (intel_crtc_can_enable_sagv(new_crtc_state))
+			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
+		else
+			new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
+	}
 
-	/* Since we're now guaranteed to only have one active CRTC... */
-	pipe = ffs(state->active_pipes) - 1;
-	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
-	crtc_state = to_intel_crtc_state(crtc->base.state);
+	if (!new_bw_state)
+		return 0;
 
-	return intel_crtc_can_enable_sagv(crtc_state);
+	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
+		ret = intel_atomic_lock_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 /*
@@ -5860,6 +5911,12 @@  skl_compute_wm(struct intel_atomic_state *state)
 	if (ret)
 		return ret;
 
+	if (state->modeset) {
+		ret = intel_compute_sagv_mask(state);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * skl_compute_ddb() will have adjusted the final watermarks
 	 * based on how much ddb is available. Now we can actually
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 9a6036ab0f90..fd1dc422e6c5 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -9,6 +9,7 @@ 
 #include <linux/types.h>
 
 #include "i915_reg.h"
+#include "display/intel_bw.h"
 
 struct drm_device;
 struct drm_i915_private;
@@ -41,7 +42,7 @@  void skl_pipe_wm_get_hw_state(struct intel_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 intel_atomic_state *state);
+bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
 int intel_enable_sagv(struct drm_i915_private *dev_priv);
 int intel_disable_sagv(struct drm_i915_private *dev_priv);
 void intel_sagv_pre_plane_update(struct intel_atomic_state *state);