Re: [PATCH v2 06/20] drm/i915/cdclk: Extract dg2_power_well_count()

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

On Wed, 24 Sep 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Extract the code to determine the DG2 pipe power well count
> into a small helper. I'll have other uses for this later.
>
> TODO: need to move this power well stuff out from the cdclk code...
>
> v2: Don't lose the early return from intel_cdclk_pcode_pre_notify()
>     (kernel test robot)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 33 +++++++++++++---------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 05d9f488895e..f190cfb85a34 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2591,6 +2591,12 @@ static void intel_set_cdclk(struct intel_display *display,
>  	}
>  }
>  
> +static bool dg2_power_well_count(struct intel_display *display,
> +				 const struct intel_cdclk_state *cdclk_state)
> +{
> +	return display->platform.dg2 ? hweight8(cdclk_state->active_pipes) : 0;
> +}
> +
>  static void intel_cdclk_pcode_pre_notify(struct intel_atomic_state *state)
>  {
>  	struct intel_display *display = to_intel_display(state);
> @@ -2603,16 +2609,16 @@ static void intel_cdclk_pcode_pre_notify(struct intel_atomic_state *state)
>  
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual) &&
> -				 new_cdclk_state->active_pipes ==
> -				 old_cdclk_state->active_pipes)
> +	    dg2_power_well_count(display, old_cdclk_state) ==
> +	    dg2_power_well_count(display, old_cdclk_state))

Both have old_cdclk_state, making this always true.

Side note, perhaps the whole function should be renamed
dg2_cdclk_pcode_pre_notify(), because the additional dg2 check in
dg2_power_well_count() felt weird until I realized this is all dg2 only.

BR,
Jani.


>  		return;
>  
>  	/* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
>  	voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
>  
>  	change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> -	update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
> -			    hweight8(old_cdclk_state->active_pipes);
> +	update_pipe_count = dg2_power_well_count(display, new_cdclk_state) >
> +		dg2_power_well_count(display, old_cdclk_state);
>  
>  	/*
>  	 * According to "Sequence Before Frequency Change",
> @@ -2630,7 +2636,7 @@ static void intel_cdclk_pcode_pre_notify(struct intel_atomic_state *state)
>  	 * no action if it is decreasing, before the change
>  	 */
>  	if (update_pipe_count)
> -		num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> +		num_active_pipes = dg2_power_well_count(display, new_cdclk_state);
>  
>  	intel_pcode_notify(display, voltage_level, num_active_pipes, cdclk,
>  			   change_cdclk, update_pipe_count);
> @@ -2650,8 +2656,8 @@ static void intel_cdclk_pcode_post_notify(struct intel_atomic_state *state)
>  	voltage_level = new_cdclk_state->actual.voltage_level;
>  
>  	update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> -	update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
> -			    hweight8(old_cdclk_state->active_pipes);
> +	update_pipe_count = dg2_power_well_count(display, new_cdclk_state) <
> +		dg2_power_well_count(display, old_cdclk_state);
>  
>  	/*
>  	 * According to "Sequence After Frequency Change",
> @@ -2667,7 +2673,7 @@ static void intel_cdclk_pcode_post_notify(struct intel_atomic_state *state)
>  	 * no action if it is increasing, after the change
>  	 */
>  	if (update_pipe_count)
> -		num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> +		num_active_pipes = dg2_power_well_count(display, new_cdclk_state);
>  
>  	intel_pcode_notify(display, voltage_level, num_active_pipes, cdclk,
>  			   update_cdclk, update_pipe_count);
> @@ -3248,15 +3254,14 @@ static bool intel_cdclk_need_serialize(struct intel_display *display,
>  				       const struct intel_cdclk_state *old_cdclk_state,
>  				       const struct intel_cdclk_state *new_cdclk_state)
>  {
> -	bool power_well_cnt_changed = hweight8(old_cdclk_state->active_pipes) !=
> -				      hweight8(new_cdclk_state->active_pipes);
> -	bool cdclk_changed = intel_cdclk_changed(&old_cdclk_state->actual,
> -						 &new_cdclk_state->actual);
>  	/*
> -	 * We need to poke hw for gen >= 12, because we notify PCode if
> +	 * We need to poke hw for DG2, because we notify PCode if
>  	 * pipe power well count changes.
>  	 */
> -	return cdclk_changed || (display->platform.dg2 && power_well_cnt_changed);
> +	return intel_cdclk_changed(&old_cdclk_state->actual,
> +				   &new_cdclk_state->actual) ||
> +		dg2_power_well_count(display, old_cdclk_state) !=
> +		dg2_power_well_count(display, new_cdclk_state);
>  }
>  
>  int intel_modeset_calc_cdclk(struct intel_atomic_state *state)

-- 
Jani Nikula, Intel