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

[Srivatsa, Anusha]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, September 14, 2022 2:43 PM
> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to
> intel_cdclk_state
> 
> On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote:
> > 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 at intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at 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.
> 
> One idea might be to pass just a pair of flags to set_cdclk() whether to skip
> the pre/post steps.

This is all considering that the new struct cdclk_step is embedded in cdclk_config and not cdclk_state. I am not understanding why cdclk-state is not accessible from bxt_set_cdclk. 
What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. 

Anusha 
> --
> Ville Syrjälä
> Intel