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 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@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.

One idea might be to pass just a pair of flags to set_cdclk()
whether to skip the pre/post steps.

-- 
Ville Syrjälä
Intel