diff mbox series

[1/2] drm/i915/display: Do both crawl and squash when changing cdclk

Message ID 20221104222642.815560-1-anusha.srivatsa@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/display: Do both crawl and squash when changing cdclk | expand

Commit Message

Srivatsa, Anusha Nov. 4, 2022, 10:26 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

For MTL, changing cdclk from between certain frequencies has
both squash and crawl. Use the current cdclk config and
the new(desired) cdclk config to construtc a mid cdclk config.
Set the cdclk twice:
- Current cdclk -> mid cdclk
- mid cdclk -> desired cdclk

v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
change via modeset for platforms that support squash_crawl sequences(Ville)

v3: Add checks for:
- scenario where only slow clock is used and
cdclk is actually 0 (bringing up display).
- PLLs are on before looking up the waveform.
- Squash and crawl capability checks.(Ville)

v4: Rebase
- Move checks to be more consistent (Ville)
- Add comments (Bala)
v5:
- Further small changes. Move checks around.
- Make if-else better looking (Ville)

v6: MTl should not follow PUnit mailbox communication as the rest of
gen11+ platforms.(Anusha)

Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 161 +++++++++++++++++----
 1 file changed, 133 insertions(+), 28 deletions(-)

Comments

Matt Roper Nov. 8, 2022, 11:42 p.m. UTC | #1 
On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> For MTL, changing cdclk from between certain frequencies has
> both squash and crawl. Use the current cdclk config and
> the new(desired) cdclk config to construtc a mid cdclk config.
> Set the cdclk twice:
> - Current cdclk -> mid cdclk
> - mid cdclk -> desired cdclk
> 
> v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> change via modeset for platforms that support squash_crawl sequences(Ville)
> 
> v3: Add checks for:
> - scenario where only slow clock is used and
> cdclk is actually 0 (bringing up display).
> - PLLs are on before looking up the waveform.
> - Squash and crawl capability checks.(Ville)
> 
> v4: Rebase
> - Move checks to be more consistent (Ville)
> - Add comments (Bala)
> v5:
> - Further small changes. Move checks around.
> - Make if-else better looking (Ville)
> 
> v6: MTl should not follow PUnit mailbox communication as the rest of
> gen11+ platforms.(Anusha)
> 
> Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 161 +++++++++++++++++----
>  1 file changed, 133 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index eada931cb1c8..d1e0763513be 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct drm_i915_private *i915,
>  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);
>  }
>  
> -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_config *cdclk_config,
> -			  enum pipe pipe)
> +static int cdclk_squash_divider(u16 waveform)
> +{
> +	return hweight16(waveform ?: 0xffff);
> +}
> +
> +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,

Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint" to help
clarify that we're just computing stuff here and not actually
programming the hardware in this function?

That naming would also help clarify why we're returning false if we
crawl but don't squash or vice versa (i.e., there's no midpoint in those
cases).

> +				   const struct intel_cdclk_config *old_cdclk_config,
> +				   const struct intel_cdclk_config *new_cdclk_config,
> +				   struct intel_cdclk_config *mid_cdclk_config)
> +{
> +	u16 old_waveform, new_waveform, mid_waveform;
> +	int size = 16;
> +	int div = 2;
> +
> +	/* Return if both Squash and Crawl are not present */
> +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> +		return false;
> +
> +	old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> +	new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
> +
> +	/* Return if Squash only or Crawl only is the desired action */
> +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||

Isn't vco unsigned?  "== 0" should be fine here I think.

> +	    old_cdclk_config->vco == new_cdclk_config->vco ||
> +	    old_waveform == new_waveform)
> +		return false;
> +
> +	*mid_cdclk_config = *new_cdclk_config;
> +
> +	/* Populate the mid_cdclk_config accordingly.
> +	 * - If moving to a higher cdclk, the desired action is squashing.
> +	 * The mid cdclk config should have the new (squash) waveform.
> +	 * - If moving to a lower cdclk, the desired action is crawling.
> +	 * The mid cdclk config should have the new vco.
> +	 */
> +
> +	if (cdclk_squash_divider(new_waveform) > cdclk_squash_divider(old_waveform)) {
> +		mid_cdclk_config->vco = old_cdclk_config->vco;
> +		mid_waveform = new_waveform;
> +	} else {
> +		mid_cdclk_config->vco = new_cdclk_config->vco;
> +		mid_waveform = old_waveform;
> +	}
> +
> +	mid_cdclk_config->cdclk = DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> +						    mid_cdclk_config->vco, size * div);
> +
> +	/* make sure the mid clock came out sane */
> +
> +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk <
> +		    min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk >
> +		    i915->display.cdclk.max_cdclk_freq);
> +	drm_WARN_ON(&i915->drm, cdclk_squash_waveform(i915, mid_cdclk_config->cdclk) !=
> +		    mid_waveform);
> +
> +	return true;
> +}
> +
> +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +			   const struct intel_cdclk_config *cdclk_config,
> +			   enum pipe pipe)
>  {
>  	int cdclk = cdclk_config->cdclk;
>  	int vco = cdclk_config->vco;
>  	u32 val;
>  	u16 waveform;
>  	int clock;
> -	int ret;
> -
> -	/* Inform power controller of upcoming frequency change. */
> -	if (DISPLAY_VER(dev_priv) >= 11)
> -		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> -					SKL_CDCLK_PREPARE_FOR_CHANGE,
> -					SKL_CDCLK_READY_FOR_CHANGE,
> -					SKL_CDCLK_READY_FOR_CHANGE, 3);
> -	else
> -		/*
> -		 * BSpec requires us to wait up to 150usec, but that leads to
> -		 * timeouts; the 2ms used here is based on experiment.
> -		 */
> -		ret = snb_pcode_write_timeout(&dev_priv->uncore,
> -					      HSW_PCODE_DE_WRITE_FREQ_REQ,
> -					      0x80000000, 150, 2);
> -	if (ret) {
> -		drm_err(&dev_priv->drm,
> -			"Failed to inform PCU about cdclk change (err %d, freq %d)\n",
> -			ret, cdclk);
> -		return;
> -	}
>  
>  	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 && vco > 0) {
>  		if (dev_priv->display.cdclk.hw.vco != vco)
> @@ -1781,6 +1818,49 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  
>  	if (pipe != INVALID_PIPE)
>  		intel_crtc_wait_for_next_vblank(intel_crtc_for_pipe(dev_priv, pipe));
> +}
> +
> +static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +			  const struct intel_cdclk_config *cdclk_config,
> +			  enum pipe pipe)
> +{
> +	struct intel_cdclk_config mid_cdclk_config;
> +	int cdclk = cdclk_config->cdclk;
> +	int ret;
> +
> +	/* Inform power controller of upcoming frequency change.
> +	 * MTL does not follow the PUnit mailbox communication, skip
> +	 * this for MTL. 
> +	 */
> +	if (!IS_METEORLAKE(dev_priv)) {

Is there a reason to believe that we'll go back to using pcode again on
future platforms?  If not, then it would be preferable to use a version
check here like

        if (DISPLAY_VER(dev_priv) >= 14)

since we usually assume future platforms will follow the newest
platform's behavior.

It also might be best to flatten this out rather than using nested if's.

        int ret = 0;

        if (display >= 14) {
                /* noop; Pcode not used for this */
        } else if (display >= 11) {
                pcode_request...
        } else {
                pcode_write_timeout...
        }


Matt

> +		if (DISPLAY_VER(dev_priv) >= 11)
> +			ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> +						SKL_CDCLK_PREPARE_FOR_CHANGE,
> +						SKL_CDCLK_READY_FOR_CHANGE,
> +						SKL_CDCLK_READY_FOR_CHANGE, 3);
> +		else
> +			/*
> +			 * BSpec requires us to wait up to 150usec, but that leads to
> +			 * timeouts; the 2ms used here is based on experiment.
> +			 */
> +			ret = snb_pcode_write_timeout(&dev_priv->uncore,
> +						      HSW_PCODE_DE_WRITE_FREQ_REQ,
> +						      0x80000000, 150, 2);
> +		if (ret) {
> +			drm_err(&dev_priv->drm,
> +				"Failed to inform PCU about cdclk change (err %d, freq %d)\n",
> +				ret, cdclk);
> +			return;
> +		}
> +	}
> +
> +	if (cdclk_crawl_and_squash(dev_priv, &dev_priv->display.cdclk.hw,
> +				   cdclk_config, &mid_cdclk_config)) {
> +		_bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
> +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> +	} else {
> +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> +	}
>  
>  	if (DISPLAY_VER(dev_priv) >= 11) {
>  		ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> @@ -1953,6 +2033,26 @@ void intel_cdclk_uninit_hw(struct drm_i915_private *i915)
>  		skl_cdclk_uninit_hw(i915);
>  }
>  
> +static bool intel_cdclk_can_crawl_and_squash(struct drm_i915_private *i915,
> +					     const struct intel_cdclk_config *a,
> +					     const struct intel_cdclk_config *b)
> +{
> +	u16 old_waveform;
> +	u16 new_waveform;
> +
> +	if (a->vco == 0 || b->vco == 0)
> +		return false;
> +
> +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> +		return false;
> +
> +	old_waveform = cdclk_squash_waveform(i915, a->cdclk);
> +	new_waveform = cdclk_squash_waveform(i915, b->cdclk);
> +
> +	return a->vco != b->vco &&
> +	       old_waveform != new_waveform;
> +}
> +
>  static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
>  				  const struct intel_cdclk_config *a,
>  				  const struct intel_cdclk_config *b)
> @@ -2759,9 +2859,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_can_crawl_and_squash(dev_priv,
> +					     &old_cdclk_state->actual,
> +					     &new_cdclk_state->actual)) {
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "Can change cdclk via crawling and squashing\n");
> +	} else if (intel_cdclk_can_squash(dev_priv,
> +					&old_cdclk_state->actual,
> +					&new_cdclk_state->actual)) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Can change cdclk via squashing\n");
>  	} else if (intel_cdclk_can_crawl(dev_priv,
> -- 
> 2.25.1
>
Srivatsa, Anusha Nov. 8, 2022, 11:56 p.m. UTC | #2 
> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: Tuesday, November 8, 2022 3:43 PM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> <balasubramani.vivekanandan@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
> 
> On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > For MTL, changing cdclk from between certain frequencies has both
> > squash and crawl. Use the current cdclk config and the new(desired)
> > cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and cdclk is actually 0
> > (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > v4: Rebase
> > - Move checks to be more consistent (Ville)
> > - Add comments (Bala)
> > v5:
> > - Further small changes. Move checks around.
> > - Make if-else better looking (Ville)
> >
> > v6: MTl should not follow PUnit mailbox communication as the rest of
> > gen11+ platforms.(Anusha)
> >
> > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > Cc: Balasubramani Vivekanandan
> <balasubramani.vivekanandan@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > +++++++++++++++++----
> >  1 file changed, 133 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index eada931cb1c8..d1e0763513be 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> drm_i915_private *i915,
> >  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);  }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_config *cdclk_config,
> > -			  enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform) {
> > +	return hweight16(waveform ?: 0xffff); }
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> 
> Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint" to
> help clarify that we're just computing stuff here and not actually
> programming the hardware in this function?
> 
> That naming would also help clarify why we're returning false if we crawl but
> don't squash or vice versa (i.e., there's no midpoint in those cases).

Makes sense.

> > +				   const struct intel_cdclk_config
> *old_cdclk_config,
> > +				   const struct intel_cdclk_config
> *new_cdclk_config,
> > +				   struct intel_cdclk_config *mid_cdclk_config)
> {
> > +	u16 old_waveform, new_waveform, mid_waveform;
> > +	int size = 16;
> > +	int div = 2;
> > +
> > +	/* Return if both Squash and Crawl are not present */
> > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > +		return false;
> > +
> > +	old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > +	new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> >cdclk);
> > +
> > +	/* Return if Squash only or Crawl only is the desired action */
> > +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> 
> Isn't vco unsigned?  "== 0" should be fine here I think.

You mean the new_cdclk_config->vco right?
 
> > +	    old_cdclk_config->vco == new_cdclk_config->vco ||
> > +	    old_waveform == new_waveform)
> > +		return false;
> > +
> > +	*mid_cdclk_config = *new_cdclk_config;
> > +
> > +	/* Populate the mid_cdclk_config accordingly.
> > +	 * - If moving to a higher cdclk, the desired action is squashing.
> > +	 * The mid cdclk config should have the new (squash) waveform.
> > +	 * - If moving to a lower cdclk, the desired action is crawling.
> > +	 * The mid cdclk config should have the new vco.
> > +	 */
> > +
> > +	if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> > +		mid_cdclk_config->vco = old_cdclk_config->vco;
> > +		mid_waveform = new_waveform;
> > +	} else {
> > +		mid_cdclk_config->vco = new_cdclk_config->vco;
> > +		mid_waveform = old_waveform;
> > +	}
> > +
> > +	mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > +						    mid_cdclk_config->vco, size
> * div);
> > +
> > +	/* make sure the mid clock came out sane */
> > +
> > +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk <
> > +		    min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk >
> > +		    i915->display.cdclk.max_cdclk_freq);
> > +	drm_WARN_ON(&i915->drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> > +		    mid_waveform);
> > +
> > +	return true;
> > +}
> > +
> > +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > +			   const struct intel_cdclk_config *cdclk_config,
> > +			   enum pipe pipe)
> >  {
> >  	int cdclk = cdclk_config->cdclk;
> >  	int vco = cdclk_config->vco;
> >  	u32 val;
> >  	u16 waveform;
> >  	int clock;
> > -	int ret;
> > -
> > -	/* Inform power controller of upcoming frequency change. */
> > -	if (DISPLAY_VER(dev_priv) >= 11)
> > -		ret = skl_pcode_request(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > -					SKL_CDCLK_PREPARE_FOR_CHANGE,
> > -					SKL_CDCLK_READY_FOR_CHANGE,
> > -					SKL_CDCLK_READY_FOR_CHANGE, 3);
> > -	else
> > -		/*
> > -		 * BSpec requires us to wait up to 150usec, but that leads to
> > -		 * timeouts; the 2ms used here is based on experiment.
> > -		 */
> > -		ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > -
> HSW_PCODE_DE_WRITE_FREQ_REQ,
> > -					      0x80000000, 150, 2);
> > -	if (ret) {
> > -		drm_err(&dev_priv->drm,
> > -			"Failed to inform PCU about cdclk change (err %d,
> freq %d)\n",
> > -			ret, cdclk);
> > -		return;
> > -	}
> >
> >  	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco
> > 0 && vco > 0) {
> >  		if (dev_priv->display.cdclk.hw.vco != vco) @@ -1781,6
> +1818,49 @@
> > static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >
> >  	if (pipe != INVALID_PIPE)
> >
> 	intel_crtc_wait_for_next_vblank(intel_crtc_for_pipe(dev_priv,
> > pipe));
> > +}
> > +
> > +static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > +			  const struct intel_cdclk_config *cdclk_config,
> > +			  enum pipe pipe)
> > +{
> > +	struct intel_cdclk_config mid_cdclk_config;
> > +	int cdclk = cdclk_config->cdclk;
> > +	int ret;
> > +
> > +	/* Inform power controller of upcoming frequency change.
> > +	 * MTL does not follow the PUnit mailbox communication, skip
> > +	 * this for MTL.
> > +	 */
> > +	if (!IS_METEORLAKE(dev_priv)) {
> 
> Is there a reason to believe that we'll go back to using pcode again on future
> platforms?  If not, then it would be preferable to use a version check here
> like
> 
>         if (DISPLAY_VER(dev_priv) >= 14)
> 
> since we usually assume future platforms will follow the newest platform's
> behavior.
> 
> It also might be best to flatten this out rather than using nested if's.
> 
>         int ret = 0;
> 
>         if (display >= 14) {
>                 /* noop; Pcode not used for this */
>         } else if (display >= 11) {
>                 pcode_request...
>         } else {
>                 pcode_write_timeout...
>         }
That's is definitely neater.
Does the rest of the patch look good?

Anusha 
> Matt
> 
> > +		if (DISPLAY_VER(dev_priv) >= 11)
> > +			ret = skl_pcode_request(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > +
> 	SKL_CDCLK_PREPARE_FOR_CHANGE,
> > +
> 	SKL_CDCLK_READY_FOR_CHANGE,
> > +
> 	SKL_CDCLK_READY_FOR_CHANGE, 3);
> > +		else
> > +			/*
> > +			 * BSpec requires us to wait up to 150usec, but that
> leads to
> > +			 * timeouts; the 2ms used here is based on
> experiment.
> > +			 */
> > +			ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > +
> HSW_PCODE_DE_WRITE_FREQ_REQ,
> > +						      0x80000000, 150, 2);
> > +		if (ret) {
> > +			drm_err(&dev_priv->drm,
> > +				"Failed to inform PCU about cdclk change (err
> %d, freq %d)\n",
> > +				ret, cdclk);
> > +			return;
> > +		}
> > +	}
> > +
> > +	if (cdclk_crawl_and_squash(dev_priv, &dev_priv->display.cdclk.hw,
> > +				   cdclk_config, &mid_cdclk_config)) {
> > +		_bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
> > +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > +	} else {
> > +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > +	}
> >
> >  	if (DISPLAY_VER(dev_priv) >= 11) {
> >  		ret = snb_pcode_write(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > @@ -1953,6 +2033,26 @@ void intel_cdclk_uninit_hw(struct
> drm_i915_private *i915)
> >  		skl_cdclk_uninit_hw(i915);
> >  }
> >
> > +static bool intel_cdclk_can_crawl_and_squash(struct drm_i915_private
> *i915,
> > +					     const struct intel_cdclk_config *a,
> > +					     const struct intel_cdclk_config *b) {
> > +	u16 old_waveform;
> > +	u16 new_waveform;
> > +
> > +	if (a->vco == 0 || b->vco == 0)
> > +		return false;
> > +
> > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > +		return false;
> > +
> > +	old_waveform = cdclk_squash_waveform(i915, a->cdclk);
> > +	new_waveform = cdclk_squash_waveform(i915, b->cdclk);
> > +
> > +	return a->vco != b->vco &&
> > +	       old_waveform != new_waveform; }
> > +
> >  static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
> >  				  const struct intel_cdclk_config *a,
> >  				  const struct intel_cdclk_config *b) @@ -
> 2759,9 +2859,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_can_crawl_and_squash(dev_priv,
> > +					     &old_cdclk_state->actual,
> > +					     &new_cdclk_state->actual)) {
> > +		drm_dbg_kms(&dev_priv->drm,
> > +			    "Can change cdclk via crawling and squashing\n");
> > +	} else if (intel_cdclk_can_squash(dev_priv,
> > +					&old_cdclk_state->actual,
> > +					&new_cdclk_state->actual)) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "Can change cdclk via squashing\n");
> >  	} else if (intel_cdclk_can_crawl(dev_priv,
> > --
> > 2.25.1
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
Matt Roper Nov. 9, 2022, 12:24 a.m. UTC | #3 
On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper@intel.com>
> > Sent: Tuesday, November 8, 2022 3:43 PM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> > <balasubramani.vivekanandan@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> > squash when changing cdclk
> > 
> > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > For MTL, changing cdclk from between certain frequencies has both
> > > squash and crawl. Use the current cdclk config and the new(desired)
> > > cdclk config to construtc a mid cdclk config.
> > > Set the cdclk twice:
> > > - Current cdclk -> mid cdclk
> > > - mid cdclk -> desired cdclk
> > >
> > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > > modeset for platforms that support squash_crawl sequences(Ville)
> > >
> > > v3: Add checks for:
> > > - scenario where only slow clock is used and cdclk is actually 0
> > > (bringing up display).
> > > - PLLs are on before looking up the waveform.
> > > - Squash and crawl capability checks.(Ville)
> > >
> > > v4: Rebase
> > > - Move checks to be more consistent (Ville)
> > > - Add comments (Bala)
> > > v5:
> > > - Further small changes. Move checks around.
> > > - Make if-else better looking (Ville)
> > >
> > > v6: MTl should not follow PUnit mailbox communication as the rest of
> > > gen11+ platforms.(Anusha)
> > >
> > > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > > Cc: Balasubramani Vivekanandan
> > <balasubramani.vivekanandan@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > +++++++++++++++++----
> > >  1 file changed, 133 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index eada931cb1c8..d1e0763513be 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> > drm_i915_private *i915,
> > >  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);  }
> > >
> > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_config *cdclk_config,
> > > -			  enum pipe pipe)
> > > +static int cdclk_squash_divider(u16 waveform) {
> > > +	return hweight16(waveform ?: 0xffff); }
> > > +
> > > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > 
> > Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint" to
> > help clarify that we're just computing stuff here and not actually
> > programming the hardware in this function?
> > 
> > That naming would also help clarify why we're returning false if we crawl but
> > don't squash or vice versa (i.e., there's no midpoint in those cases).
> 
> Makes sense.
> 
> > > +				   const struct intel_cdclk_config
> > *old_cdclk_config,
> > > +				   const struct intel_cdclk_config
> > *new_cdclk_config,
> > > +				   struct intel_cdclk_config *mid_cdclk_config)
> > {
> > > +	u16 old_waveform, new_waveform, mid_waveform;
> > > +	int size = 16;
> > > +	int div = 2;
> > > +
> > > +	/* Return if both Squash and Crawl are not present */
> > > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > +		return false;
> > > +
> > > +	old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> > >cdclk);
> > > +	new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> > >cdclk);
> > > +
> > > +	/* Return if Squash only or Crawl only is the desired action */
> > > +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > 
> > Isn't vco unsigned?  "== 0" should be fine here I think.
> 
> You mean the new_cdclk_config->vco right?

Both of them I think.  The vco field of intel_cdclk_config can't take on
negative values because it's defined as unsigned:

        struct intel_cdclk_config {
                unsigned int cdclk, vco, ref, bypass;
                u8 voltage_level;
        };
 
> > > +	    old_cdclk_config->vco == new_cdclk_config->vco ||
> > > +	    old_waveform == new_waveform)
> > > +		return false;
> > > +
> > > +	*mid_cdclk_config = *new_cdclk_config;
> > > +
> > > +	/* Populate the mid_cdclk_config accordingly.
> > > +	 * - If moving to a higher cdclk, the desired action is squashing.
> > > +	 * The mid cdclk config should have the new (squash) waveform.
> > > +	 * - If moving to a lower cdclk, the desired action is crawling.
> > > +	 * The mid cdclk config should have the new vco.
> > > +	 */
> > > +
> > > +	if (cdclk_squash_divider(new_waveform) >
> > cdclk_squash_divider(old_waveform)) {
> > > +		mid_cdclk_config->vco = old_cdclk_config->vco;
> > > +		mid_waveform = new_waveform;
> > > +	} else {
> > > +		mid_cdclk_config->vco = new_cdclk_config->vco;
> > > +		mid_waveform = old_waveform;
> > > +	}
> > > +
> > > +	mid_cdclk_config->cdclk =
> > DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > > +						    mid_cdclk_config->vco, size
> > * div);
> > > +
> > > +	/* make sure the mid clock came out sane */
> > > +
> > > +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk <
> > > +		    min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > > +	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk >
> > > +		    i915->display.cdclk.max_cdclk_freq);
> > > +	drm_WARN_ON(&i915->drm, cdclk_squash_waveform(i915,
> > mid_cdclk_config->cdclk) !=
> > > +		    mid_waveform);
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > +			   const struct intel_cdclk_config *cdclk_config,
> > > +			   enum pipe pipe)
> > >  {
> > >  	int cdclk = cdclk_config->cdclk;
> > >  	int vco = cdclk_config->vco;
> > >  	u32 val;
> > >  	u16 waveform;
> > >  	int clock;
> > > -	int ret;
> > > -
> > > -	/* Inform power controller of upcoming frequency change. */
> > > -	if (DISPLAY_VER(dev_priv) >= 11)
> > > -		ret = skl_pcode_request(&dev_priv->uncore,
> > SKL_PCODE_CDCLK_CONTROL,
> > > -					SKL_CDCLK_PREPARE_FOR_CHANGE,
> > > -					SKL_CDCLK_READY_FOR_CHANGE,
> > > -					SKL_CDCLK_READY_FOR_CHANGE, 3);
> > > -	else
> > > -		/*
> > > -		 * BSpec requires us to wait up to 150usec, but that leads to
> > > -		 * timeouts; the 2ms used here is based on experiment.
> > > -		 */
> > > -		ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > > -
> > HSW_PCODE_DE_WRITE_FREQ_REQ,
> > > -					      0x80000000, 150, 2);
> > > -	if (ret) {
> > > -		drm_err(&dev_priv->drm,
> > > -			"Failed to inform PCU about cdclk change (err %d,
> > freq %d)\n",
> > > -			ret, cdclk);
> > > -		return;
> > > -	}
> > >
> > >  	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco
> > > 0 && vco > 0) {
> > >  		if (dev_priv->display.cdclk.hw.vco != vco) @@ -1781,6
> > +1818,49 @@
> > > static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > >
> > >  	if (pipe != INVALID_PIPE)
> > >
> > 	intel_crtc_wait_for_next_vblank(intel_crtc_for_pipe(dev_priv,
> > > pipe));
> > > +}
> > > +
> > > +static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > +			  const struct intel_cdclk_config *cdclk_config,
> > > +			  enum pipe pipe)
> > > +{
> > > +	struct intel_cdclk_config mid_cdclk_config;
> > > +	int cdclk = cdclk_config->cdclk;
> > > +	int ret;
> > > +
> > > +	/* Inform power controller of upcoming frequency change.
> > > +	 * MTL does not follow the PUnit mailbox communication, skip
> > > +	 * this for MTL.
> > > +	 */
> > > +	if (!IS_METEORLAKE(dev_priv)) {
> > 
> > Is there a reason to believe that we'll go back to using pcode again on future
> > platforms?  If not, then it would be preferable to use a version check here
> > like
> > 
> >         if (DISPLAY_VER(dev_priv) >= 14)
> > 
> > since we usually assume future platforms will follow the newest platform's
> > behavior.
> > 
> > It also might be best to flatten this out rather than using nested if's.
> > 
> >         int ret = 0;
> > 
> >         if (display >= 14) {
> >                 /* noop; Pcode not used for this */
> >         } else if (display >= 11) {
> >                 pcode_request...
> >         } else {
> >                 pcode_write_timeout...
> >         }
> That's is definitely neater.
> Does the rest of the patch look good?

Yeah, aside from the few minor things I noted, the rest of this patch
looks good to me.


Matt

> 
> Anusha 
> > Matt
> > 
> > > +		if (DISPLAY_VER(dev_priv) >= 11)
> > > +			ret = skl_pcode_request(&dev_priv->uncore,
> > SKL_PCODE_CDCLK_CONTROL,
> > > +
> > 	SKL_CDCLK_PREPARE_FOR_CHANGE,
> > > +
> > 	SKL_CDCLK_READY_FOR_CHANGE,
> > > +
> > 	SKL_CDCLK_READY_FOR_CHANGE, 3);
> > > +		else
> > > +			/*
> > > +			 * BSpec requires us to wait up to 150usec, but that
> > leads to
> > > +			 * timeouts; the 2ms used here is based on
> > experiment.
> > > +			 */
> > > +			ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > > +
> > HSW_PCODE_DE_WRITE_FREQ_REQ,
> > > +						      0x80000000, 150, 2);
> > > +		if (ret) {
> > > +			drm_err(&dev_priv->drm,
> > > +				"Failed to inform PCU about cdclk change (err
> > %d, freq %d)\n",
> > > +				ret, cdclk);
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	if (cdclk_crawl_and_squash(dev_priv, &dev_priv->display.cdclk.hw,
> > > +				   cdclk_config, &mid_cdclk_config)) {
> > > +		_bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
> > > +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > > +	} else {
> > > +		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > > +	}
> > >
> > >  	if (DISPLAY_VER(dev_priv) >= 11) {
> > >  		ret = snb_pcode_write(&dev_priv->uncore,
> > SKL_PCODE_CDCLK_CONTROL,
> > > @@ -1953,6 +2033,26 @@ void intel_cdclk_uninit_hw(struct
> > drm_i915_private *i915)
> > >  		skl_cdclk_uninit_hw(i915);
> > >  }
> > >
> > > +static bool intel_cdclk_can_crawl_and_squash(struct drm_i915_private
> > *i915,
> > > +					     const struct intel_cdclk_config *a,
> > > +					     const struct intel_cdclk_config *b) {
> > > +	u16 old_waveform;
> > > +	u16 new_waveform;
> > > +
> > > +	if (a->vco == 0 || b->vco == 0)
> > > +		return false;
> > > +
> > > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > +		return false;
> > > +
> > > +	old_waveform = cdclk_squash_waveform(i915, a->cdclk);
> > > +	new_waveform = cdclk_squash_waveform(i915, b->cdclk);
> > > +
> > > +	return a->vco != b->vco &&
> > > +	       old_waveform != new_waveform; }
> > > +
> > >  static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
> > >  				  const struct intel_cdclk_config *a,
> > >  				  const struct intel_cdclk_config *b) @@ -
> > 2759,9 +2859,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_can_crawl_and_squash(dev_priv,
> > > +					     &old_cdclk_state->actual,
> > > +					     &new_cdclk_state->actual)) {
> > > +		drm_dbg_kms(&dev_priv->drm,
> > > +			    "Can change cdclk via crawling and squashing\n");
> > > +	} else if (intel_cdclk_can_squash(dev_priv,
> > > +					&old_cdclk_state->actual,
> > > +					&new_cdclk_state->actual)) {
> > >  		drm_dbg_kms(&dev_priv->drm,
> > >  			    "Can change cdclk via squashing\n");
> > >  	} else if (intel_cdclk_can_crawl(dev_priv,
> > > --
> > > 2.25.1
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
Ville Syrjälä Nov. 9, 2022, 11:29 a.m. UTC | #4 
On Tue, Nov 08, 2022 at 04:24:30PM -0800, Matt Roper wrote:
> On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Roper, Matthew D <matthew.d.roper@intel.com>
> > > Sent: Tuesday, November 8, 2022 3:43 PM
> > > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> > > <balasubramani.vivekanandan@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> > > squash when changing cdclk
> > > 
> > > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > For MTL, changing cdclk from between certain frequencies has both
> > > > squash and crawl. Use the current cdclk config and the new(desired)
> > > > cdclk config to construtc a mid cdclk config.
> > > > Set the cdclk twice:
> > > > - Current cdclk -> mid cdclk
> > > > - mid cdclk -> desired cdclk
> > > >
> > > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > > > modeset for platforms that support squash_crawl sequences(Ville)
> > > >
> > > > v3: Add checks for:
> > > > - scenario where only slow clock is used and cdclk is actually 0
> > > > (bringing up display).
> > > > - PLLs are on before looking up the waveform.
> > > > - Squash and crawl capability checks.(Ville)
> > > >
> > > > v4: Rebase
> > > > - Move checks to be more consistent (Ville)
> > > > - Add comments (Bala)
> > > > v5:
> > > > - Further small changes. Move checks around.
> > > > - Make if-else better looking (Ville)
> > > >
> > > > v6: MTl should not follow PUnit mailbox communication as the rest of
> > > > gen11+ platforms.(Anusha)
> > > >
> > > > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > > > Cc: Balasubramani Vivekanandan
> > > <balasubramani.vivekanandan@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > > +++++++++++++++++----
> > > >  1 file changed, 133 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index eada931cb1c8..d1e0763513be 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> > > drm_i915_private *i915,
> > > >  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);  }
> > > >
> > > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > -			  const struct intel_cdclk_config *cdclk_config,
> > > > -			  enum pipe pipe)
> > > > +static int cdclk_squash_divider(u16 waveform) {
> > > > +	return hweight16(waveform ?: 0xffff); }
> > > > +
> > > > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > > 
> > > Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint" to
> > > help clarify that we're just computing stuff here and not actually
> > > programming the hardware in this function?
> > > 
> > > That naming would also help clarify why we're returning false if we crawl but
> > > don't squash or vice versa (i.e., there's no midpoint in those cases).
> > 
> > Makes sense.
> > 
> > > > +				   const struct intel_cdclk_config
> > > *old_cdclk_config,
> > > > +				   const struct intel_cdclk_config
> > > *new_cdclk_config,
> > > > +				   struct intel_cdclk_config *mid_cdclk_config)
> > > {
> > > > +	u16 old_waveform, new_waveform, mid_waveform;
> > > > +	int size = 16;
> > > > +	int div = 2;
> > > > +
> > > > +	/* Return if both Squash and Crawl are not present */
> > > > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > > +		return false;
> > > > +
> > > > +	old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> > > >cdclk);
> > > > +	new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> > > >cdclk);
> > > > +
> > > > +	/* Return if Squash only or Crawl only is the desired action */
> > > > +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > > 
> > > Isn't vco unsigned?  "== 0" should be fine here I think.
> > 
> > You mean the new_cdclk_config->vco right?
> 
> Both of them I think.  The vco field of intel_cdclk_config can't take on
> negative values because it's defined as unsigned:
> 
>         struct intel_cdclk_config {
>                 unsigned int cdclk, vco, ref, bypass;
>                 u8 voltage_level;
>         };

Hmm. I guess I used the vco=-1 in sanitize() as a way to 
effectively write vco=~0, the point of which was force the
PLL to be fully disabled first regardless of what its
current state is.

But now that I look at that we might have an issue with
platforms that support crawling. We wrote that code as if
vco is signed so it's actually going to take the crawl
path now that it looks like the PLL was prevsiously on.
I think we need to add an explicit check to not do
the crawl for the vco=~0/-1 case...
Srivatsa, Anusha Nov. 9, 2022, 9:49 p.m. UTC | #5 
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, November 9, 2022 3:30 AM
> To: Roper, Matthew D <matthew.d.roper@intel.com>
> Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> <balasubramani.vivekanandan@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
> 
> On Tue, Nov 08, 2022 at 04:24:30PM -0800, Matt Roper wrote:
> > On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Roper, Matthew D <matthew.d.roper@intel.com>
> > > > Sent: Tuesday, November 8, 2022 3:43 PM
> > > > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> > > > <balasubramani.vivekanandan@intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both
> > > > crawl and squash when changing cdclk
> > > >
> > > > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > For MTL, changing cdclk from between certain frequencies has
> > > > > both squash and crawl. Use the current cdclk config and the
> > > > > new(desired) cdclk config to construtc a mid cdclk config.
> > > > > Set the cdclk twice:
> > > > > - Current cdclk -> mid cdclk
> > > > > - mid cdclk -> desired cdclk
> > > > >
> > > > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> > > > > change via modeset for platforms that support squash_crawl
> > > > > sequences(Ville)
> > > > >
> > > > > v3: Add checks for:
> > > > > - scenario where only slow clock is used and cdclk is actually 0
> > > > > (bringing up display).
> > > > > - PLLs are on before looking up the waveform.
> > > > > - Squash and crawl capability checks.(Ville)
> > > > >
> > > > > v4: Rebase
> > > > > - Move checks to be more consistent (Ville)
> > > > > - Add comments (Bala)
> > > > > v5:
> > > > > - Further small changes. Move checks around.
> > > > > - Make if-else better looking (Ville)
> > > > >
> > > > > v6: MTl should not follow PUnit mailbox communication as the
> > > > > rest of
> > > > > gen11+ platforms.(Anusha)
> > > > >
> > > > > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > > > > Cc: Balasubramani Vivekanandan
> > > > <balasubramani.vivekanandan@intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > > > +++++++++++++++++----
> > > > >  1 file changed, 133 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > index eada931cb1c8..d1e0763513be 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > @@ -1716,37 +1716,74 @@ static void
> > > > > dg2_cdclk_squash_program(struct
> > > > drm_i915_private *i915,
> > > > >  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);  }
> > > > >
> > > > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > > -			  const struct intel_cdclk_config *cdclk_config,
> > > > > -			  enum pipe pipe)
> > > > > +static int cdclk_squash_divider(u16 waveform) {
> > > > > +	return hweight16(waveform ?: 0xffff); }
> > > > > +
> > > > > +static bool cdclk_crawl_and_squash(struct drm_i915_private
> > > > > +*i915,
> > > >
> > > > Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint"
> > > > to help clarify that we're just computing stuff here and not
> > > > actually programming the hardware in this function?
> > > >
> > > > That naming would also help clarify why we're returning false if
> > > > we crawl but don't squash or vice versa (i.e., there's no midpoint in
> those cases).
> > >
> > > Makes sense.
> > >
> > > > > +				   const struct intel_cdclk_config
> > > > *old_cdclk_config,
> > > > > +				   const struct intel_cdclk_config
> > > > *new_cdclk_config,
> > > > > +				   struct intel_cdclk_config
> *mid_cdclk_config)
> > > > {
> > > > > +	u16 old_waveform, new_waveform, mid_waveform;
> > > > > +	int size = 16;
> > > > > +	int div = 2;
> > > > > +
> > > > > +	/* Return if both Squash and Crawl are not present */
> > > > > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > > > +		return false;
> > > > > +
> > > > > +	old_waveform = cdclk_squash_waveform(i915,
> old_cdclk_config-
> > > > >cdclk);
> > > > > +	new_waveform = cdclk_squash_waveform(i915,
> new_cdclk_config-
> > > > >cdclk);
> > > > > +
> > > > > +	/* Return if Squash only or Crawl only is the desired action */
> > > > > +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0
> > > > > +||
> > > >
> > > > Isn't vco unsigned?  "== 0" should be fine here I think.
> > >
> > > You mean the new_cdclk_config->vco right?
> >
> > Both of them I think.  The vco field of intel_cdclk_config can't take
> > on negative values because it's defined as unsigned:
> >
> >         struct intel_cdclk_config {
> >                 unsigned int cdclk, vco, ref, bypass;
> >                 u8 voltage_level;
> >         };
> 
> Hmm. I guess I used the vco=-1 in sanitize() as a way to effectively write
> vco=~0, the point of which was force the PLL to be fully disabled first
> regardless of what its current state is.
> 
> But now that I look at that we might have an issue with platforms that
> support crawling. We wrote that code as if vco is signed so it's actually going
> to take the crawl path now that it looks like the PLL was prevsiously on.
> I think we need to add an explicit check to not do the crawl for the vco=~0/-1
> case...
@Ville Syrjälä  in bxt_sanitize_cdclk() do we need the scenario:
dev_priv->cdclk.hw.vco = -1; ?

Anusha
> 
> --
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index eada931cb1c8..d1e0763513be 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1716,37 +1716,74 @@  static void dg2_cdclk_squash_program(struct drm_i915_private *i915,
 	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);
 }
 
-static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_config *cdclk_config,
-			  enum pipe pipe)
+static int cdclk_squash_divider(u16 waveform)
+{
+	return hweight16(waveform ?: 0xffff);
+}
+
+static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
+				   const struct intel_cdclk_config *old_cdclk_config,
+				   const struct intel_cdclk_config *new_cdclk_config,
+				   struct intel_cdclk_config *mid_cdclk_config)
+{
+	u16 old_waveform, new_waveform, mid_waveform;
+	int size = 16;
+	int div = 2;
+
+	/* Return if both Squash and Crawl are not present */
+	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
+		return false;
+
+	old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
+	new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
+
+	/* Return if Squash only or Crawl only is the desired action */
+	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
+	    old_cdclk_config->vco == new_cdclk_config->vco ||
+	    old_waveform == new_waveform)
+		return false;
+
+	*mid_cdclk_config = *new_cdclk_config;
+
+	/* Populate the mid_cdclk_config accordingly.
+	 * - If moving to a higher cdclk, the desired action is squashing.
+	 * The mid cdclk config should have the new (squash) waveform.
+	 * - If moving to a lower cdclk, the desired action is crawling.
+	 * The mid cdclk config should have the new vco.
+	 */
+
+	if (cdclk_squash_divider(new_waveform) > cdclk_squash_divider(old_waveform)) {
+		mid_cdclk_config->vco = old_cdclk_config->vco;
+		mid_waveform = new_waveform;
+	} else {
+		mid_cdclk_config->vco = new_cdclk_config->vco;
+		mid_waveform = old_waveform;
+	}
+
+	mid_cdclk_config->cdclk = DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
+						    mid_cdclk_config->vco, size * div);
+
+	/* make sure the mid clock came out sane */
+
+	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk <
+		    min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
+	drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk >
+		    i915->display.cdclk.max_cdclk_freq);
+	drm_WARN_ON(&i915->drm, cdclk_squash_waveform(i915, mid_cdclk_config->cdclk) !=
+		    mid_waveform);
+
+	return true;
+}
+
+static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
+			   const struct intel_cdclk_config *cdclk_config,
+			   enum pipe pipe)
 {
 	int cdclk = cdclk_config->cdclk;
 	int vco = cdclk_config->vco;
 	u32 val;
 	u16 waveform;
 	int clock;
-	int ret;
-
-	/* Inform power controller of upcoming frequency change. */
-	if (DISPLAY_VER(dev_priv) >= 11)
-		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
-					SKL_CDCLK_PREPARE_FOR_CHANGE,
-					SKL_CDCLK_READY_FOR_CHANGE,
-					SKL_CDCLK_READY_FOR_CHANGE, 3);
-	else
-		/*
-		 * BSpec requires us to wait up to 150usec, but that leads to
-		 * timeouts; the 2ms used here is based on experiment.
-		 */
-		ret = snb_pcode_write_timeout(&dev_priv->uncore,
-					      HSW_PCODE_DE_WRITE_FREQ_REQ,
-					      0x80000000, 150, 2);
-	if (ret) {
-		drm_err(&dev_priv->drm,
-			"Failed to inform PCU about cdclk change (err %d, freq %d)\n",
-			ret, cdclk);
-		return;
-	}
 
 	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 && vco > 0) {
 		if (dev_priv->display.cdclk.hw.vco != vco)
@@ -1781,6 +1818,49 @@  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 
 	if (pipe != INVALID_PIPE)
 		intel_crtc_wait_for_next_vblank(intel_crtc_for_pipe(dev_priv, pipe));
+}
+
+static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
+			  const struct intel_cdclk_config *cdclk_config,
+			  enum pipe pipe)
+{
+	struct intel_cdclk_config mid_cdclk_config;
+	int cdclk = cdclk_config->cdclk;
+	int ret;
+
+	/* Inform power controller of upcoming frequency change.
+	 * MTL does not follow the PUnit mailbox communication, skip
+	 * this for MTL. 
+	 */
+	if (!IS_METEORLAKE(dev_priv)) {
+		if (DISPLAY_VER(dev_priv) >= 11)
+			ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
+						SKL_CDCLK_PREPARE_FOR_CHANGE,
+						SKL_CDCLK_READY_FOR_CHANGE,
+						SKL_CDCLK_READY_FOR_CHANGE, 3);
+		else
+			/*
+			 * BSpec requires us to wait up to 150usec, but that leads to
+			 * timeouts; the 2ms used here is based on experiment.
+			 */
+			ret = snb_pcode_write_timeout(&dev_priv->uncore,
+						      HSW_PCODE_DE_WRITE_FREQ_REQ,
+						      0x80000000, 150, 2);
+		if (ret) {
+			drm_err(&dev_priv->drm,
+				"Failed to inform PCU about cdclk change (err %d, freq %d)\n",
+				ret, cdclk);
+			return;
+		}
+	}
+
+	if (cdclk_crawl_and_squash(dev_priv, &dev_priv->display.cdclk.hw,
+				   cdclk_config, &mid_cdclk_config)) {
+		_bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
+		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
+	} else {
+		_bxt_set_cdclk(dev_priv, cdclk_config, pipe);
+	}
 
 	if (DISPLAY_VER(dev_priv) >= 11) {
 		ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
@@ -1953,6 +2033,26 @@  void intel_cdclk_uninit_hw(struct drm_i915_private *i915)
 		skl_cdclk_uninit_hw(i915);
 }
 
+static bool intel_cdclk_can_crawl_and_squash(struct drm_i915_private *i915,
+					     const struct intel_cdclk_config *a,
+					     const struct intel_cdclk_config *b)
+{
+	u16 old_waveform;
+	u16 new_waveform;
+
+	if (a->vco == 0 || b->vco == 0)
+		return false;
+
+	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
+		return false;
+
+	old_waveform = cdclk_squash_waveform(i915, a->cdclk);
+	new_waveform = cdclk_squash_waveform(i915, b->cdclk);
+
+	return a->vco != b->vco &&
+	       old_waveform != new_waveform;
+}
+
 static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
 				  const struct intel_cdclk_config *a,
 				  const struct intel_cdclk_config *b)
@@ -2759,9 +2859,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_can_crawl_and_squash(dev_priv,
+					     &old_cdclk_state->actual,
+					     &new_cdclk_state->actual)) {
+		drm_dbg_kms(&dev_priv->drm,
+			    "Can change cdclk via crawling and squashing\n");
+	} else if (intel_cdclk_can_squash(dev_priv,
+					&old_cdclk_state->actual,
+					&new_cdclk_state->actual)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "Can change cdclk via squashing\n");
 	} else if (intel_cdclk_can_crawl(dev_priv,