[PATCH v2 1/3] drm/i915/cdclk: Fix CDCLK programming order when pipes are active

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, March 27, 2024 3:40 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [PATCH v2 1/3] drm/i915/cdclk: Fix CDCLK programming order when
> pipes are active
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Currently we always reprogram CDCLK from the
> intel_set_cdclk_pre_plane_update() when using squahs/crawl.

Nitpick: Typo in squash

Change Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar at intel.com>

> The code only works correctly for the cd2x update or full modeset cases, and it
> was simply never updated to deal with squash/crawl.
> 
> If the CDCLK frequency is increasing we must reprogram it before we do anything
> else that might depend on the new higher frequency, and conversely we must not
> decrease the frequency until everything that might still depend on the old higher
> frequency has been dealt with.
> 
> Since cdclk_state->pipe is only relevant when doing a cd2x update we can't use it
> to determine the correct sequence during squash/crawl. To that end introduce
> cdclk_state->disable_pipes which simply indicates that we must perform the
> update while the pipes are disable (ie. during
> intel_set_cdclk_pre_plane_update()). Otherwise we use the same old vs. new
> CDCLK frequency comparsiong as for cd2x updates.
> 
> The only remaining problem case is when the voltage_level needs to increase due
> to a DDI port, but the CDCLK frequency is decreasing (and not all pipes are being
> disabled). The current approach will not bump the voltage level up until after the
> port has already been enabled, which is too late.
> But we'll take care of that case separately.
> 
> v2: Don't break the "must disable pipes case"
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
> drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfc..619529dba095 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2600,7 +2600,6 @@ intel_set_cdclk_pre_plane_update(struct
> intel_atomic_state *state)
>  		intel_atomic_get_old_cdclk_state(state);
>  	const struct intel_cdclk_state *new_cdclk_state =
>  		intel_atomic_get_new_cdclk_state(state);
> -	enum pipe pipe = new_cdclk_state->pipe;
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2609,11 +2608,12 @@ intel_set_cdclk_pre_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_pre_notify(state);
> 
> -	if (pipe == INVALID_PIPE ||
> +	if (new_cdclk_state->disable_pipes ||
>  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>  		drm_WARN_ON(&i915->drm, !new_cdclk_state-
> >base.changed);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -2632,7 +2632,6 @@ intel_set_cdclk_post_plane_update(struct
> intel_atomic_state *state)
>  		intel_atomic_get_old_cdclk_state(state);
>  	const struct intel_cdclk_state *new_cdclk_state =
>  		intel_atomic_get_new_cdclk_state(state);
> -	enum pipe pipe = new_cdclk_state->pipe;
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2641,11 +2640,12 @@ intel_set_cdclk_post_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_post_notify(state);
> 
> -	if (pipe != INVALID_PIPE &&
> +	if (!new_cdclk_state->disable_pipes &&
>  	    old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>  		drm_WARN_ON(&i915->drm, !new_cdclk_state-
> >base.changed);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -3124,6 +3124,7 @@ static struct intel_global_state
> *intel_cdclk_duplicate_state(struct intel_globa
>  		return NULL;
> 
>  	cdclk_state->pipe = INVALID_PIPE;
> +	cdclk_state->disable_pipes = false;
> 
>  	return &cdclk_state->base;
>  }
> @@ -3316,6 +3317,8 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
>  		if (ret)
>  			return ret;
> 
> +		new_cdclk_state->disable_pipes = true;
> +
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Modeset required for cdclk change\n");
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index bc8f86e292d8..2843fc091086 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -53,6 +53,9 @@ struct intel_cdclk_state {
> 
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	/* update cdclk with pipes disabled */
> +	bool disable_pipes;
>  };
> 
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> --
> 2.43.2