Re: [PATCH] drm/i915/cdclk: Add cdclk check to atomic check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Thursday, March 3, 2022 1:59 AM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re:  [PATCH] drm/i915/cdclk: Add cdclk check to atomic
> check
> 
> On Wed, 02 Mar 2022, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> wrote:
> > Checking cdclk conditions during atomic check and preparing for commit
> > phase so we can have atomic commit as simple as possible. Add the
> > specific steps to be taken during cdclk changes, prepare for
> > squashing, crawling and modeset scenarios.
> >
> > Rename functions intel_cdclk_can_squash() and
> > intel_cdclk_can_crawl() since they no longer simply check if squashing
> > and crawling can be performed.
> 
> IMO the patch is doing too much at once. There's adding parameters (can be
> done separately at first), there's renames (can be done separately
> afterwards), there's functional changes, all in one.
> 
> If you got a bisect result pointing at this commit as regressing, would you
> able to easily figure out what went wrong?

Agreed. Splitting would be good.

> Some comments inline too.
> 
> BR,
> Jani.
> 
> >
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c    | 169 +++++++++++-------
> >  drivers/gpu/drm/i915/display/intel_cdclk.h    |  16 +-
> >  .../drm/i915/display/intel_display_power.c    |   2 +-
> >  3 files changed, 123 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 8888fda8b701..04f3f77ef0a8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1700,12 +1700,23 @@ static void bxt_set_cdclk(struct
> drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_config *cdclk_config,
> >  			  enum pipe pipe)
> >  {
> > +	struct intel_atomic_state *state;
> > +	struct intel_cdclk_state *new_cdclk_state;
> > +	struct cdclk_steps *cdclk_steps;
> > +	struct intel_cdclk_state *cdclk_state;
> >  	int cdclk = cdclk_config->cdclk;
> >  	int vco = cdclk_config->vco;
> > +	u32 squash_ctl = 0;
> >  	u32 val;
> >  	u16 waveform;
> >  	int clock;
> >  	int ret;
> > +	int i;
> > +
> > +	cdclk_state =  to_intel_cdclk_state(dev_priv->cdclk.obj.state);
> > +	state = cdclk_state->base.state;
> > +	new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> > +	cdclk_steps = new_cdclk_state->steps;
> >
> >  	/* Inform power controller of upcoming frequency change. */
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > @@ -1728,40 +1739,43 @@ static void bxt_set_cdclk(struct
> drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >
> > -	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 &&
> vco > 0) {
> > -		if (dev_priv->cdclk.hw.vco != vco)
> > +	for (i = 0; i < CDCLK_ACTIONS; i++) {
> > +		switch (cdclk_steps[i].action) {
> > +		case CDCLK_MODESET:
> > +			if (DISPLAY_VER(dev_priv) >= 11) {
> > +				if (dev_priv->cdclk.hw.vco != 0 &&
> > +				    dev_priv->cdclk.hw.vco != vco)
> > +					icl_cdclk_pll_disable(dev_priv);
> > +
> > +				if (dev_priv->cdclk.hw.vco != vco)
> > +					icl_cdclk_pll_enable(dev_priv, vco);
> > +			} else {
> > +				if (dev_priv->cdclk.hw.vco != 0 &&
> > +				    dev_priv->cdclk.hw.vco != vco)
> > +					bxt_de_pll_disable(dev_priv);
> > +
> > +				if (dev_priv->cdclk.hw.vco != vco)
> > +					bxt_de_pll_enable(dev_priv, vco);
> > +			}
> > +			clock = cdclk;
> > +			break;
> > +		case CDCLK_CRAWL:
> >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > -	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > -		if (dev_priv->cdclk.hw.vco != 0 &&
> > -		    dev_priv->cdclk.hw.vco != vco)
> > -			icl_cdclk_pll_disable(dev_priv);
> > -
> > -		if (dev_priv->cdclk.hw.vco != vco)
> > -			icl_cdclk_pll_enable(dev_priv, vco);
> > -	} else {
> > -		if (dev_priv->cdclk.hw.vco != 0 &&
> > -		    dev_priv->cdclk.hw.vco != vco)
> > -			bxt_de_pll_disable(dev_priv);
> > -
> > -		if (dev_priv->cdclk.hw.vco != vco)
> > -			bxt_de_pll_enable(dev_priv, vco);
> > -	}
> > -
> > -	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > -
> > -	if (waveform)
> > -		clock = vco / 2;
> > -	else
> > -		clock = cdclk;
> > -
> > -	if (has_cdclk_squasher(dev_priv)) {
> > -		u32 squash_ctl = 0;
> > -
> > -		if (waveform)
> > +			clock = cdclk;
> > +			break;
> > +		case CDCLK_SQUASH:
> > +			waveform =  cdclk_squash_waveform(dev_priv,
> cdclk_steps[i].cdclk);
> > +			clock = vco / 2;
> >  			squash_ctl = CDCLK_SQUASH_ENABLE |
> >  				CDCLK_SQUASH_WINDOW_SIZE(0xf) |
> waveform;
> > -
> > -		intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
> > +			intel_de_write(dev_priv, CDCLK_SQUASH_CTL,
> squash_ctl);
> > +			break;
> > +		case CDCLK_NOOP:
> > +			break;
> > +		default:
> > +			MISSING_CASE(cdclk_steps[i].action);
> > +			break;
> > +		}
> >  	}
> >
> >  	val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | @@ -1951,11
> > +1965,12 @@ void intel_cdclk_uninit_hw(struct drm_i915_private *i915)
> >  		skl_cdclk_uninit_hw(i915);
> >  }
> >
> > -static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
> > -				  const struct intel_cdclk_config *a,
> > -				  const struct intel_cdclk_config *b)
> > +static bool intel_cdclk_crawl(struct drm_i915_private *dev_priv,
> > +			      const struct intel_cdclk_state *a,
> > +			      struct intel_cdclk_state *b)
> >  {
> >  	int a_div, b_div;
> > +	struct cdclk_steps *cdclk_transition = b->steps;
> >
> >  	if (!HAS_CDCLK_CRAWL(dev_priv))
> >  		return false;
> > @@ -1964,19 +1979,25 @@ static bool intel_cdclk_can_crawl(struct
> drm_i915_private *dev_priv,
> >  	 * The vco and cd2x divider will change independently
> >  	 * from each, so we disallow cd2x change when crawling.
> >  	 */
> > -	a_div = DIV_ROUND_CLOSEST(a->vco, a->cdclk);
> > -	b_div = DIV_ROUND_CLOSEST(b->vco, b->cdclk);
> > +	a_div = DIV_ROUND_CLOSEST(a->actual.vco, a->actual.cdclk);
> > +	b_div = DIV_ROUND_CLOSEST(b->actual.vco, b->actual.cdclk);
> >
> > -	return a->vco != 0 && b->vco != 0 &&
> > -		a->vco != b->vco &&
> > +	cdclk_transition[0].action = CDCLK_CRAWL;
> > +	cdclk_transition[0].cdclk = b->actual.cdclk;
> > +	cdclk_transition[1].action = CDCLK_NOOP;
> > +	cdclk_transition[1].cdclk = b->actual.cdclk;
> > +
> > +	return a->actual.vco != 0 && b->actual.vco != 0 &&
> > +		a->actual.vco != b->actual.vco &&
> >  		a_div == b_div &&
> > -		a->ref == b->ref;
> > +		a->actual.ref == b->actual.ref;
> >  }
> >
> > -static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> > -				   const struct intel_cdclk_config *a,
> > -				   const struct intel_cdclk_config *b)
> > +static bool intel_cdclk_squash(struct drm_i915_private *dev_priv,
> > +			       const struct intel_cdclk_state *a,
> > +			       struct intel_cdclk_state *b)
> >  {
> > +	struct cdclk_steps *cdclk_transition = b->steps;
> >  	/*
> >  	 * FIXME should store a bit more state in intel_cdclk_config
> >  	 * to differentiate squasher vs. cd2x divider properly. For @@
> > -1986,10 +2007,15 @@ static bool intel_cdclk_can_squash(struct
> drm_i915_private *dev_priv,
> >  	if (!has_cdclk_squasher(dev_priv))
> >  		return false;
> >
> > -	return a->cdclk != b->cdclk &&
> > -		a->vco != 0 &&
> > -		a->vco == b->vco &&
> > -		a->ref == b->ref;
> > +	cdclk_transition[0].action = CDCLK_SQUASH;
> > +	cdclk_transition[0].cdclk = b->actual.cdclk;
> > +	cdclk_transition[1].action = CDCLK_NOOP;
> > +	cdclk_transition[1].cdclk = b->actual.cdclk;
> > +
> > +	return a->actual.cdclk != b->actual.cdclk &&
> > +		a->actual.vco != 0 &&
> > +		a->actual.vco == b->actual.vco &&
> > +		a->actual.ref == b->actual.ref;
> >  }
> >
> >  /**
> > @@ -2002,9 +2028,27 @@ static bool intel_cdclk_can_squash(struct
> drm_i915_private *dev_priv,
> >   * True if changing between the two CDCLK configurations
> >   * requires all pipes to be off, false if not.
> >   */
> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> > +bool intel_cdclk_needs_modeset(struct drm_i915_private *i915,
> > +			       const struct intel_cdclk_config *a,
> >  			       const struct intel_cdclk_config *b)  {
> > +	struct intel_cdclk_state *new_cdclk_state;
> > +	struct cdclk_steps *cdclk_transition;
> > +	struct intel_cdclk_state *cdclk_state =  to_intel_cdclk_state(i915-
> >cdclk.obj.state);
> > +	struct intel_atomic_state *state = cdclk_state->base.state;
> > +
> > +	new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> > +	cdclk_transition = new_cdclk_state->steps;
> > +
> > +	if (a->cdclk != b->cdclk ||
> > +	    a->vco != b->vco ||
> > +	    a->ref != b->ref) {
> > +		cdclk_transition[0].action = CDCLK_MODESET;
> > +		cdclk_transition[0].cdclk = b->cdclk;
> > +		cdclk_transition[1].action = CDCLK_NOOP;
> > +		cdclk_transition[1].cdclk = b->cdclk;
> > +	}
> > +
> >  	return a->cdclk != b->cdclk ||
> >  		a->vco != b->vco ||
> >  		a->ref != b->ref;
> > @@ -2052,10 +2096,11 @@ static bool intel_cdclk_can_cd2x_update(struct
> drm_i915_private *dev_priv,
> >   * Returns:
> >   * True if the CDCLK configurations don't match, false if they do.
> >   */
> > -static bool intel_cdclk_changed(const struct intel_cdclk_config *a,
> > +static bool intel_cdclk_changed(struct drm_i915_private *i915,
> > +				const struct intel_cdclk_config *a,
> >  				const struct intel_cdclk_config *b)  {
> > -	return intel_cdclk_needs_modeset(a, b) ||
> > +	return intel_cdclk_needs_modeset(i915, a, b) ||
> >  		a->voltage_level != b->voltage_level;  }
> >
> > @@ -2084,7 +2129,7 @@ static void intel_set_cdclk(struct
> > drm_i915_private *dev_priv,  {
> >  	struct intel_encoder *encoder;
> >
> > -	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> > +	if (!intel_cdclk_changed(dev_priv, &dev_priv->cdclk.hw,
> > +cdclk_config))
> >  		return;
> >
> >  	if (drm_WARN_ON_ONCE(&dev_priv->drm,
> > !dev_priv->cdclk_funcs->set_cdclk))
> > @@ -2131,7 +2176,7 @@ static void intel_set_cdclk(struct
> drm_i915_private *dev_priv,
> >  	intel_audio_cdclk_change_post(dev_priv);
> >
> >  	if (drm_WARN(&dev_priv->drm,
> > -		     intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config),
> > +		     intel_cdclk_changed(dev_priv, &dev_priv->cdclk.hw,
> > +cdclk_config),
> >  		     "cdclk state doesn't match!\n")) {
> >  		intel_cdclk_dump_config(dev_priv, &dev_priv->cdclk.hw,
> "[hw state]");
> >  		intel_cdclk_dump_config(dev_priv, cdclk_config, "[sw
> state]"); @@
> > -2155,7 +2200,7 @@ intel_set_cdclk_pre_plane_update(struct
> intel_atomic_state *state)
> >  		intel_atomic_get_new_cdclk_state(state);
> >  	enum pipe pipe = new_cdclk_state->pipe;
> >
> > -	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > +	if (!intel_cdclk_changed(dev_priv, &old_cdclk_state->actual,
> >  				 &new_cdclk_state->actual))
> >  		return;
> >
> > @@ -2184,7 +2229,7 @@ intel_set_cdclk_post_plane_update(struct
> intel_atomic_state *state)
> >  		intel_atomic_get_new_cdclk_state(state);
> >  	enum pipe pipe = new_cdclk_state->pipe;
> >
> > -	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > +	if (!intel_cdclk_changed(dev_priv, &old_cdclk_state->actual,
> >  				 &new_cdclk_state->actual))
> >  		return;
> >
> > @@ -2738,7 +2783,7 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
> >  	if (ret)
> >  		return ret;
> >
> > -	if (intel_cdclk_changed(&old_cdclk_state->actual,
> > +	if (intel_cdclk_changed(dev_priv, &old_cdclk_state->actual,
> >  				&new_cdclk_state->actual)) {
> >  		/*
> >  		 * Also serialize commits across all crtcs @@ -2749,7 +2794,7
> @@
> > int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >  			return ret;
> >  	} else if (old_cdclk_state->active_pipes != new_cdclk_state-
> >active_pipes ||
> >  		   old_cdclk_state->force_min_cdclk != new_cdclk_state-
> >force_min_cdclk ||
> > -		   intel_cdclk_changed(&old_cdclk_state->logical,
> > +		   intel_cdclk_changed(dev_priv, &old_cdclk_state->logical,
> >  				       &new_cdclk_state->logical)) {
> >  		ret = intel_atomic_lock_global_state(&new_cdclk_state-
> >base);
> >  		if (ret)
> > @@ -2776,14 +2821,14 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
> >  			pipe = INVALID_PIPE;
> >  	}
> >
> > -	if (intel_cdclk_can_squash(dev_priv,
> > -				   &old_cdclk_state->actual,
> > -				   &new_cdclk_state->actual)) {
> > +	if (intel_cdclk_squash(dev_priv,
> > +			       old_cdclk_state,
> > +			       new_cdclk_state)) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "Can change cdclk via squasher\n");
> > -	} else if (intel_cdclk_can_crawl(dev_priv,
> > -					 &old_cdclk_state->actual,
> > -					 &new_cdclk_state->actual)) {
> > +	} else if (intel_cdclk_crawl(dev_priv,
> > +				     old_cdclk_state,
> > +				     new_cdclk_state)) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "Can change cdclk via crawl\n");
> >  	} else if (pipe != INVALID_PIPE) {
> > @@ -2792,7 +2837,7 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "Can change cdclk cd2x divider with pipe %c
> active\n",
> >  			    pipe_name(pipe));
> > -	} else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual,
> > +	} else if (intel_cdclk_needs_modeset(dev_priv,
> > +&old_cdclk_state->actual,
> >  					     &new_cdclk_state->actual)) {
> >  		/* All pipes must be switched off while we change the cdclk.
> */
> >  		ret = intel_modeset_all_pipes(state); diff --git
> > a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index df66f66fbad0..5008463958b1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > @@ -20,6 +20,14 @@ struct intel_cdclk_config {
> >  	u8 voltage_level;
> >  };
> >
> > +enum cdclk_actions {
> 
> Should it be a singular instead of a plural? Contrast:
> 
> 	enum cdclk_action x;
> 	enum cdclk_actions y;
> 
> It's only one action, right?
> 
> > +	CDCLK_MODESET = 0,
> > +	CDCLK_SQUASH,
> > +	CDCLK_CRAWL,
> > +	CDCLK_NOOP,
> > +	CDCLK_ACTIONS
> > +};
> > +
> 
> You need to prefix these with intel_cdclk_ or INTEL_CDCLK_.

Makes sense to me. Will make the change.

> >  struct intel_cdclk_state {
> >  	struct intel_global_state base;
> >
> > @@ -49,6 +57,11 @@ struct intel_cdclk_state {
> >
> >  	/* bitmask of active pipes */
> >  	u8 active_pipes;
> > +
> > +	struct cdclk_steps {
> 
> This doesn't need to have a name, really. And if it had a name, it would be
> one step, not steps. (The array is steps with plural.)

Hmmm, will change that.

> 
> > +		enum cdclk_actions action;
> > +		u32 cdclk;
> > +	} steps[CDCLK_ACTIONS];
> 
> It's a bit confusing to me that you have member action, but it's the same
> enum that defines the size of the steps, leading me to believe the index, as
> indexed by the enumerations, actually matters.
> 
> Should CDCLK_ACTIONS actually be named CDCLK_MAX_ACTIONS or
> something?
> Or COUNT?
As  I read the above comment I understood the confusion, yes renaming to CDCLK_MAX_ACTIONS is better.

Anusha 
> 
> 
> >  };
> >
> >  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state
> > *crtc_state); @@ -58,7 +71,8 @@ void intel_init_cdclk_hooks(struct
> > drm_i915_private *dev_priv);  void intel_update_max_cdclk(struct
> > drm_i915_private *dev_priv);  void intel_update_cdclk(struct
> > drm_i915_private *dev_priv);
> >  u32 intel_read_rawclk(struct drm_i915_private *dev_priv); -bool
> > intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> > +bool intel_cdclk_needs_modeset(struct drm_i915_private *i915,
> > +			       const struct intel_cdclk_config *a,
> >  			       const struct intel_cdclk_config *b);  void
> > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state
> > *state); diff --git
> > a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 3dc859032bac..417a56d54056 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -1118,7 +1118,7 @@ static void gen9_disable_dc_states(struct
> drm_i915_private *dev_priv)
> >  	intel_cdclk_get_cdclk(dev_priv, &cdclk_config);
> >  	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
> >  	drm_WARN_ON(&dev_priv->drm,
> > -		    intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > +		    intel_cdclk_needs_modeset(dev_priv, &dev_priv-
> >cdclk.hw,
> >  					      &cdclk_config));
> >
> >  	gen9_assert_dbuf_enabled(dev_priv);
> 
> --
> Jani Nikula, Intel Open Source Graphics Center




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux