Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

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

On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> This is a prep patch for what the rest of the series does.
> 
> Add existing actions that change cdclk - squash, crawl, modeset to
> intel_cdclk_state so we have access to the cdclk values
> that are in transition.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index b535cf6a7d9e..43835688ee02 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -15,6 +15,14 @@ struct drm_i915_private;
>  struct intel_atomic_state;
>  struct intel_crtc_state;
>  
> +enum cdclk_actions {
> +	INTEL_CDCLK_MODESET = 0,
> +	INTEL_CDCLK_SQUASH,
> +	INTEL_CDCLK_CRAWL,
> +	INTEL_CDCLK_NOOP,
> +	MAX_CDCLK_ACTIONS
> +};

This whole actions thing feels overly complicated to me.
I think we should only need something like this:

if (new.squash > old.squash) {
	mid.vco = old.vco;
	mid.squash = new.squash;
} else {
	mid.vco = new.vco;
	mid.squash = old.squash;
}
/*
 * bunch of asserts here to make sure
 * the mid state looks sane.
 */
set_cdclk(mid);
set_cdclk(new);

And perhaps the current set_cdclk needs to get chunked up
into smaller pieces so we don't do all the pre/post stuff
more than once needlessly.

> +
>  struct intel_cdclk_config {
>  	unsigned int cdclk, vco, ref, bypass;
>  	u8 voltage_level;
> @@ -51,6 +59,11 @@ struct intel_cdclk_state {
>  
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	struct cdclk_step {
> +		enum cdclk_actions action;
> +		u32 cdclk;
> +	} steps[MAX_CDCLK_ACTIONS];
>  };
>  
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel