Re: [Intel-gfx] [RFC] drm/i915/display: Move cdclk checks to atomic check

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 09 Dec 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> i915 has squashing for DG2 and crawling for ADLP.
> Moving the checks to atomic check phase so
> at a later phase we know how the cdclk changes.

Just some high level comments:

- Functions named intel_cdclk_can_foo() must *not* change the state,
  that's unexpected and surprising.

- There's a bunch of state handling already for cdclk, please don't just
  dump new state in drm_i915_private, outside of the existing states. In
  particular, storing yet another copy of cdclk is suspicious.

- Please don't add short stuff like CRAWL and SQUASH to our module
  namespace.


BR,
Jani.


>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 49 +++++++++++++---------
>  drivers/gpu/drm/i915/i915_drv.h            | 11 +++++
>  2 files changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 639a64733f61..9382dd24d889 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1707,9 +1707,27 @@ 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)
> -			adlp_cdclk_pll_crawl(dev_priv, vco);
> +	if (DISPLAY_VER(dev_priv) >= 12) {
> +		int i = 0;
> +		u32 squash_ctl = 0;
> +		struct cdclk_steps *cdclk_steps = dev_priv->cdclk.steps;
> +
> +		for (i = 0; i < CDCLK_ACTIONS; i++) {
> +			switch (cdclk_steps[i].action) {
> +			case CRAWL:
> +				adlp_cdclk_pll_crawl(dev_priv, vco);
> +				break;
> +			case 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);
> +				break;
> +			default:
> +				break;
> +			}
> +		}
>  	} else if (DISPLAY_VER(dev_priv) >= 11) {
>  		if (dev_priv->cdclk.hw.vco != 0 &&
>  		    dev_priv->cdclk.hw.vco != vco)
> @@ -1726,22 +1744,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  			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)
> -			squash_ctl = CDCLK_SQUASH_ENABLE |
> -				CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
> -
> -		intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
> -	}
> +	clock = cdclk;
>  
>  	val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
>  		bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
> @@ -1934,6 +1937,7 @@ static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
>  				  const struct intel_cdclk_config *a,
>  				  const struct intel_cdclk_config *b)
>  {
> +	struct cdclk_steps *cdclk_transition = dev_priv->cdclk.steps;
>  	int a_div, b_div;
>  
>  	if (!HAS_CDCLK_CRAWL(dev_priv))
> @@ -1946,6 +1950,9 @@ static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
>  	a_div = DIV_ROUND_CLOSEST(a->vco, a->cdclk);
>  	b_div = DIV_ROUND_CLOSEST(b->vco, b->cdclk);
>  
> +	cdclk_transition[0].action = CRAWL;
> +	cdclk_transition[0].cdclk = b->cdclk;
> +
>  	return a->vco != 0 && b->vco != 0 &&
>  		a->vco != b->vco &&
>  		a_div == b_div &&
> @@ -1956,6 +1963,7 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>  				   const struct intel_cdclk_config *a,
>  				   const struct intel_cdclk_config *b)
>  {
> +	struct cdclk_steps *cdclk_transition = dev_priv->cdclk.steps;
>  	/*
>  	 * FIXME should store a bit more state in intel_cdclk_config
>  	 * to differentiate squasher vs. cd2x divider properly. For
> @@ -1965,6 +1973,9 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>  	if (!has_cdclk_squasher(dev_priv))
>  		return false;
>  
> +	cdclk_transition[0].action = SQUASH;
> +	cdclk_transition[0].cdclk = b->cdclk;
> +
>  	return a->cdclk != b->cdclk &&
>  		a->vco != 0 &&
>  		a->vco == b->vco &&
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ae7dc7862b5d..c03299253b81 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -117,6 +117,12 @@
>  
>  struct drm_i915_gem_object;
>  
> +enum cdclk_actions {
> +	SQUASH = 0,
> +	CRAWL,
> +	CDCLK_ACTIONS
> +};
> +
>  /* Threshold == 5 for long IRQs, 50 for short */
>  #define HPD_STORM_DEFAULT_THRESHOLD 50
>  
> @@ -782,6 +788,11 @@ struct drm_i915_private {
>  		const struct intel_cdclk_vals *table;
>  
>  		struct intel_global_obj obj;
> +
> +		struct cdclk_steps {
> +			enum cdclk_actions action;
> +			u32 cdclk;
> +		} steps[CDCLK_ACTIONS];
>  	} cdclk;
>  
>  	struct {

-- 
Jani Nikula, Intel Open Source Graphics Center