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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 26, 2024 at 10:31:26PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we always reprogram CDCLK from the
> intel_set_cdclk_post_plane_update() when using squahs/crawl.
> 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.
> 
> So let us only consider the relationship of the old and
> new CDCLK frequencies when determining where to do
> the reprogramming, regarless of whether all pipes might
> be off or not at the time.
> 
> If the CDCLK freqiency remains unchanges we may still have to
> do the reprogramming to change the voltage_level. Currently
> we do that from intel_set_cdclk_pre_plane_update() which
> probably is the right choice most of the time. The only
> counterexample is when the voltage_level needs to increase
> due to a DDI port, but the CDCLK frequency is decreasing.
> 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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfc..49e2f09a796a 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,11 @@ 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 ||
> -	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +	if (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 +2631,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 +2639,11 @@ 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 &&
> -	    old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> +	if (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);
>  	}

And naturally this will not work for the full modeset case.
We must do the cdclk programming in the pre hook in that
case. I suppose we'll need to actually track whether we're
going to do a cd2x/squash/crawl or a full modeset...

-- 
Ville Syrjälä
Intel