> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Monday, November 14, 2022 2:16 PM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/3] drm/i915/display: Do both crawl and squash when
> changing cdclk
>
> On Mon, Nov 14, 2022 at 12:57:16PM -0800, Anusha Srivatsa wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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)
> >
> > v7: (MattR)
> > - s/cdclk_crawl_and_squash/cdclk_compute_crawl_squash_midpoint
> > - Cleanup Pcode checks in bxt_set_cdclk()
> > - Correct unsigned/signed checks
> >
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 163
> > ++++++++++++++++-----
> > 1 file changed, 124 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 25d01271dc09..4db7103fe5d6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1727,37 +1727,74 @@ static bool cdclk_pll_is_unknown(unsigned int
> vco)
> > return vco == ~0;
> > }
> >
> > -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_compute_crawl_and_squash_midpoint(struct
> drm_i915_private *i915,
> > + 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 ||
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
> > + return false;
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* Populate the mid_cdclk_config accordingly.
>
> Nit: kernel coding style says the "/*" needs to be on its own line.
>
> > + * - If moving to a higher cdclk, the desired action is squashing.
> > + * The mid cdclk config should have the new (squash) waveform.
> > + * - If moving to a lower cdclk, the desired action is crawling.
> > + * The mid cdclk config should have the new vco.
> > + */
> > +
> > + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> > + mid_cdclk_config->vco = old_cdclk_config->vco;
> > + mid_waveform = new_waveform;
> > + } else {
> > + mid_cdclk_config->vco = new_cdclk_config->vco;
> > + mid_waveform = old_waveform;
> > + }
> > +
> > + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > + mid_cdclk_config->vco, size
> * div);
> > +
> > + /* make sure the mid clock came out sane */
> > +
> > + drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk <
> > + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > + drm_WARN_ON(&i915->drm, mid_cdclk_config->cdclk >
> > + i915->display.cdclk.max_cdclk_freq);
> > + drm_WARN_ON(&i915->drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> > + mid_waveform);
> > +
> > + return true;
> > +}
> > +
> > +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > + const struct intel_cdclk_config *cdclk_config,
> > + enum pipe pipe)
> > {
> > int cdclk = cdclk_config->cdclk;
> > int vco = cdclk_config->vco;
> > u32 val;
> > u16 waveform;
> > int clock;
> > - int ret;
> > -
> > - /* Inform power controller of upcoming frequency change. */
> > - if (DISPLAY_VER(dev_priv) >= 11)
> > - ret = skl_pcode_request(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > - SKL_CDCLK_PREPARE_FOR_CHANGE,
> > - SKL_CDCLK_READY_FOR_CHANGE,
> > - SKL_CDCLK_READY_FOR_CHANGE, 3);
> > - else
> > - /*
> > - * BSpec requires us to wait up to 150usec, but that leads to
> > - * timeouts; the 2ms used here is based on experiment.
> > - */
> > - ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > -
> HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - 0x80000000, 150, 2);
> > - if (ret) {
> > - drm_err(&dev_priv->drm,
> > - "Failed to inform PCU about cdclk change (err %d,
> freq %d)\n",
> > - ret, cdclk);
> > - return;
> > - }
> >
> > if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco
> > 0 && vco > 0 &&
> > !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) { @@
> > -1793,30 +1830,53 @@ static void bxt_set_cdclk(struct drm_i915_private
> > *dev_priv,
> >
> > if (pipe != INVALID_PIPE)
> >
> intel_crtc_wait_for_next_vblank(intel_crtc_for_pipe(dev_priv,
> > pipe));
> > +}
> >
> > - if (DISPLAY_VER(dev_priv) >= 11) {
> > - ret = snb_pcode_write(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > - cdclk_config->voltage_level);
> > +static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > + const struct intel_cdclk_config *cdclk_config,
> > + enum pipe pipe)
> > +{
> > + struct intel_cdclk_config mid_cdclk_config;
> > + int cdclk = cdclk_config->cdclk;
> > + int ret;
>
> You should initialize ret to 0 here since you don't set it in the new branch of
> the if/else ladder below.
>
> > +
> > + /* Inform power controller of upcoming frequency change. */
> > + if (DISPLAY_VER(dev_priv) >= 14) {
> > + /* DISPLAY14+ do not follow the PUnit mailbox
> communication,
>
> "Display versions 14 and above" or "Xe_LPD+ and beyond"
>
> Also, this is another case where "/*" should be on its own line.
>
> > + * skip this step.
> > + */
> > + } else if (DISPLAY_VER(dev_priv) >= 11) {
> > + ret = skl_pcode_request(&dev_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> > + SKL_CDCLK_PREPARE_FOR_CHANGE,
> > + SKL_CDCLK_READY_FOR_CHANGE,
> > + SKL_CDCLK_READY_FOR_CHANGE, 3);
> > } else {
> > /*
> > - * The timeout isn't specified, the 2ms used here is based on
> > - * experiment.
> > - * FIXME: Waiting for the request completion could be
> delayed
> > - * until the next PCODE request based on BSpec.
> > + * BSpec requires us to wait up to 150usec, but that leads to
> > + * timeouts; the 2ms used here is based on experiment.
> > */
> > ret = snb_pcode_write_timeout(&dev_priv->uncore,
> >
> HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - cdclk_config->voltage_level,
> > - 150, 2);
> > + 0x80000000, 150, 2);
>
> The switch from cdclk_config->voltage_level to a magic number
> (0x80000000) on old platforms doesn't seem to be related to the rest of this
> patch. Ditto for the comment update about 150usec not being enough.
> Were those intended for a different patch?
Well, initially both squash and crawl support for MTl was the intention. The change came to be a part of this patch because MTL doesn't go through the Punit mailbox communication like previous platforms and hence the churn. Thinking out loud if changing the commit message makes more sense or a separate patch. A separate patch would just remove make sure MTL does not touch the bits of code Punit code. And the next patch would be the actual squash_crawl-mid_cdclk_config patch.
>
> > }
> > -
> > +
>
> Stray whitespace
>
> > if (ret) {
> > drm_err(&dev_priv->drm,
> > - "PCode CDCLK freq set failed, (err %d, freq %d)\n",
> > + "Failed to inform PCU about cdclk change (err %d,
> freq %d)\n",
>
> Error message change seems unrelated to the rest of this patch since it's not
> possible to get here on MTL (at least once you fix the uninitialized 'ret' noted
> above).
>
> > ret, cdclk);
> > return;
> > }
> >
> > + if (cdclk_compute_crawl_and_squash_midpoint(dev_priv,
> > + &dev_priv-
> >display.cdclk.hw,
> > + cdclk_config,
> > + &mid_cdclk_config)) {
> > + _bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
> > + _bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > + } else {
> > + _bxt_set_cdclk(dev_priv, cdclk_config, pipe);
> > + }
> > +
> > intel_update_cdclk(dev_priv);
> >
> > if (DISPLAY_VER(dev_priv) >= 11)
> > @@ -1965,6 +2025,26 @@ void intel_cdclk_uninit_hw(struct
> drm_i915_private *i915)
> > skl_cdclk_uninit_hw(i915);
> > }
> >
> > +static bool intel_cdclk_can_crawl_and_squash(struct drm_i915_private
> *i915,
> > + const struct intel_cdclk_config *a,
> > + const struct intel_cdclk_config *b) {
> > + u16 old_waveform;
> > + u16 new_waveform;
> > +
> > + if (a->vco == 0 || b->vco == 0)
> > + return false;
> > +
>
> Do we also need to return false here if cdclk_pll_is_unknown() for either a or
> b?
>
The above check should suffice. If it indeed is ~0, the bxt_set_cdclk() will now make sure driver des not take crawl path.
Anusha
>
> Matt
>
> > + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > + return false;
> > +
> > + old_waveform = cdclk_squash_waveform(i915, a->cdclk);
> > + new_waveform = cdclk_squash_waveform(i915, b->cdclk);
> > +
> > + return a->vco != b->vco &&
> > + old_waveform != new_waveform; }
> > +
> > static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
> > const struct intel_cdclk_config *a,
> > const struct intel_cdclk_config *b) @@ -
> 2771,9 +2851,14 @@ int
> > intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> > pipe = INVALID_PIPE;
> > }
> >
> > - if (intel_cdclk_can_squash(dev_priv,
> > - &old_cdclk_state->actual,
> > - &new_cdclk_state->actual)) {
> > + if (intel_cdclk_can_crawl_and_squash(dev_priv,
> > + &old_cdclk_state->actual,
> > + &new_cdclk_state->actual)) {
> > + drm_dbg_kms(&dev_priv->drm,
> > + "Can change cdclk via crawling and squashing\n");
> > + } else if (intel_cdclk_can_squash(dev_priv,
> > + &old_cdclk_state->actual,
> > + &new_cdclk_state->actual)) {
> > drm_dbg_kms(&dev_priv->drm,
> > "Can change cdclk via squashing\n");
> > } else if (intel_cdclk_can_crawl(dev_priv,
> > --
> > 2.25.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
