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 Wed, Sep 14, 2022 at 09:58:59PM +0000, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, September 14, 2022 2:43 PM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@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@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.
> 
> 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.

.set_cdclk() is lower level than that. It must be able to operate outside
atomic commits (eg. during display core init/uninit), and thus must be
able to do its work purely based on the passed in cdclk_config (which is
the lower level state structure, a pair of which are embedded inside the
atomic intel_cdclk_state).

> What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. 

We definitely don't want to hack around the core ideas
of how the atomic machinery works. That way leads
madness because no one will be able to understand how
anything works.

-- 
Ville Syrjälä
Intel