[Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk

[Srivatsa, Anusha]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, November 9, 2022 3:30 AM
> To: Roper, Matthew D <matthew.d.roper at intel.com>
> Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
> gfx at lists.freedesktop.org; Vivekanandan, Balasubramani
> <balasubramani.vivekanandan at intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
> 
> On Tue, Nov 08, 2022 at 04:24:30PM -0800, Matt Roper wrote:
> > On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > > > Sent: Tuesday, November 8, 2022 3:43 PM
> > > > To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> > > > Cc: intel-gfx at lists.freedesktop.org; Vivekanandan, Balasubramani
> > > > <balasubramani.vivekanandan at intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both
> > > > crawl and squash when changing cdclk
> > > >
> > > > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > >
> > > > > For MTL, changing cdclk from between certain frequencies has
> > > > > both squash and crawl. Use the current cdclk config and the
> > > > > new(desired) cdclk config to construtc a mid cdclk config.
> > > > > Set the cdclk twice:
> > > > > - Current cdclk -> mid cdclk
> > > > > - mid cdclk -> desired cdclk
> > > > >
> > > > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> > > > > change via modeset for platforms that support squash_crawl
> > > > > sequences(Ville)
> > > > >
> > > > > v3: Add checks for:
> > > > > - scenario where only slow clock is used and cdclk is actually 0
> > > > > (bringing up display).
> > > > > - PLLs are on before looking up the waveform.
> > > > > - Squash and crawl capability checks.(Ville)
> > > > >
> > > > > v4: Rebase
> > > > > - Move checks to be more consistent (Ville)
> > > > > - Add comments (Bala)
> > > > > v5:
> > > > > - Further small changes. Move checks around.
> > > > > - Make if-else better looking (Ville)
> > > > >
> > > > > v6: MTl should not follow PUnit mailbox communication as the
> > > > > rest of
> > > > > gen11+ platforms.(Anusha)
> > > > >
> > > > > Cc: Clint Taylor <Clinton.A.Taylor at intel.com>
> > > > > Cc: Balasubramani Vivekanandan
> > > > <balasubramani.vivekanandan at intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > > > +++++++++++++++++----
> > > > >  1 file changed, 133 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > index eada931cb1c8..d1e0763513be 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > @@ -1716,37 +1716,74 @@ static void
> > > > > dg2_cdclk_squash_program(struct
> > > > drm_i915_private *i915,
> > > > >  	intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);  }
> > > > >
> > > > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > > -			  const struct intel_cdclk_config *cdclk_config,
> > > > > -			  enum pipe pipe)
> > > > > +static int cdclk_squash_divider(u16 waveform) {
> > > > > +	return hweight16(waveform ?: 0xffff); }
> > > > > +
> > > > > +static bool cdclk_crawl_and_squash(struct drm_i915_private
> > > > > +*i915,
> > > >
> > > > Bikeshed:  maybe name this "cdclk_compute_crawl_squash_midpoint"
> > > > to help clarify that we're just computing stuff here and not
> > > > actually programming the hardware in this function?
> > > >
> > > > That naming would also help clarify why we're returning false if
> > > > we crawl but don't squash or vice versa (i.e., there's no midpoint in
> those cases).
> > >
> > > Makes sense.
> > >
> > > > > +				   const struct intel_cdclk_config
> > > > *old_cdclk_config,
> > > > > +				   const struct intel_cdclk_config
> > > > *new_cdclk_config,
> > > > > +				   struct intel_cdclk_config
> *mid_cdclk_config)
> > > > {
> > > > > +	u16 old_waveform, new_waveform, mid_waveform;
> > > > > +	int size = 16;
> > > > > +	int div = 2;
> > > > > +
> > > > > +	/* Return if both Squash and Crawl are not present */
> > > > > +	if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > > > +		return false;
> > > > > +
> > > > > +	old_waveform = cdclk_squash_waveform(i915,
> old_cdclk_config-
> > > > >cdclk);
> > > > > +	new_waveform = cdclk_squash_waveform(i915,
> new_cdclk_config-
> > > > >cdclk);
> > > > > +
> > > > > +	/* Return if Squash only or Crawl only is the desired action */
> > > > > +	if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0
> > > > > +||
> > > >
> > > > Isn't vco unsigned?  "== 0" should be fine here I think.
> > >
> > > You mean the new_cdclk_config->vco right?
> >
> > Both of them I think.  The vco field of intel_cdclk_config can't take
> > on negative values because it's defined as unsigned:
> >
> >         struct intel_cdclk_config {
> >                 unsigned int cdclk, vco, ref, bypass;
> >                 u8 voltage_level;
> >         };
> 
> Hmm. I guess I used the vco=-1 in sanitize() as a way to effectively write
> vco=~0, the point of which was force the PLL to be fully disabled first
> regardless of what its current state is.
> 
> But now that I look at that we might have an issue with platforms that
> support crawling. We wrote that code as if vco is signed so it's actually going
> to take the crawl path now that it looks like the PLL was prevsiously on.
> I think we need to add an explicit check to not do the crawl for the vco=~0/-1
> case...
@Ville Syrjälä  in bxt_sanitize_cdclk() do we need the scenario:
dev_priv->cdclk.hw.vco = -1; ?

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