On Wed, Dec 14, 2022 at 09:15:07PM +0200, Srivatsa, Anusha wrote:
>
>
> > -----Original Message-----
> > From: Lisovskiy, Stanislav <stanislav.lisovskiy@xxxxxxxxx>
> > Sent: Wednesday, December 14, 2022 2:31 AM
> > To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani <jani.nikula@xxxxxxxxx>
> > Subject: Re: [PATCH 1/1] drm/i915: Implement workaround for
> > CDCLK PLL disable/enable
> >
> > On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> > > > Of Stanislav Lisovskiy
> > > > Sent: Thursday, November 24, 2022 2:36 AM
> > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> > > > Subject: [PATCH 1/1] drm/i915: Implement workaround for
> > > > CDCLK PLL disable/enable
> > > >
> > > > It was reported that we might get a hung and loss of register access
> > > > in some cases when CDCLK PLL is disabled and then enabled, while
> > > > squashing is enabled.
> > > > As a workaround it was proposed by HW team that SW should disable
> > > > squashing when CDCLK PLL is being reenabled.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> > > > 1 file changed, 12 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 0c107a38f9d0..e338f288c9ac 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1801,6 +1801,13 @@ static bool
> > > > cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private
> > *i91
> > > > return true;
> > > > }
> > > >
> > > > +static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
> > {
> > > > + return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > > > + && dev_priv->display.cdclk.hw.vco > 0
> > > > + && HAS_CDCLK_SQUASH(dev_priv));
> > > Redundant check. If it is MTL or DG2, then it will have
> > HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of > 0.
> > The commit message says the hang can be observed when moving from 0 to
> > > 0 vco.
> > >
> > > Anusha
> >
> > Idea was that we probably should bind more to the feature rather than
> > platform, I agree checking both "IS_DG2" and if platform has squashing is
> > redundant, because then we would have to add each new platform
> > manually, so I would leave HAS_CDCLK_SQUASH and then at some point
> > just cut off using some INTEL_GEN or other check all the new platforms
> > where this is fixed in HW.
> >
> > Regarding vco, the icl_cdclk_pll_update func works as follows:
> >
> > if (i915->display.cdclk.hw.vco != 0 &&
> > i915->display.cdclk.hw.vco != vco)
> > icl_cdclk_pll_disable(i915);
> >
> > if (i915->display.cdclk.hw.vco != vco)
> > icl_cdclk_pll_enable(i915, vco);
> >
> > 1) if vco changes from zero value(i915->display.cdclk.hw.vco) to non-zero
> > value(vco), that means
> > currently squashing is anyway disabled(if vco == 0, cdclk is set to "bypass"
> > and squash waveform
> > is 0), so the W/A is not needed.
> >
> > 2) if vco changes from non-zero value in i915->display.cdclk.hw.vco to zero
> > value(vco), we are not
> > going to call icl_cdclk_pll_enable anyway so W/A is also not needed.
> >
> > 3) if vco changes from some non-zero value in i915->display.cdclk.hw.vco to
> > other non-zero value(vco),
> > which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw vco!=0
> > and vco!=0) and then
> > icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
> > So that disabling enabling in between is what we are interested and
> > where we should make sure
> > squashing is disabled.
> > BTW I have another question - is this code even correct? Shouldn't we
> > avoid disabling/enabling PLL if we have
> > squashing/crawling? As I understood the whole point of having
> > swuashing/crawling is avoiding swithing the PLL
> > on and off.
> >
> Squashing works when we don't need to change the PLL ratio. We fix the PLL ratio to say 307 (this can change from platform to platform). Then squashing can be used to vary frequencies below this. So we set squasher to ffff it will mean highest. We can use squasher to change frequency with squash waveform, max being ffff and any value lower will take lower frequencies.
> Cdclk Crawling is used when the PLL has to be changed. Eg higher than 307 then we need to update PLL. In that case we first program squashing to ffff(this will take to 307) n then use crawling to go up to the desired frequency.
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)) {
if (dev_priv->display.cdclk.hw.vco != vco)
adlp_cdclk_pll_crawl(dev_priv, vco);
} else if (DISPLAY_VER(dev_priv) >= 11) {
if (pll_enable_wa_needed(dev_priv))
dg2_cdclk_squash_program(dev_priv, 0);
icl_cdclk_pll_update(dev_priv, vco);
} else
bxt_cdclk_pll_update(dev_priv, vco);
I think that condition above will trigger CDCLK crawl whenever vco is not ~0, CDCLK crawl is supported
and both hw.vco and vco are > 0 and vco has to be changed.
In bxt_set_cdclk which calls _bxt_set_cdclk we calculate the midpoint however I don't see
how _bxt_set_cdclk is going to distinguish between crawling and squashing.
I can see that squash waveform will be returned as 0, if CDCLK is >= 307 MHz for MTL for example,
or if CDCLK is equal to bypass, however the only way to skip crawling here seems to either have
vco == ~0(probably after hw init) or vco == 0, but if vco == 0 we are going to then call
icl_cdclk_pll_update which might disable pll, if current hw.vco is not 0.
So what am I missing here?
Stan
>
> Anusha
> > Stan
> >
> >
> > > > +}
> > > > +
> > > > static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > const struct intel_cdclk_config *cdclk_config,
> > > > enum pipe pipe)
> > > > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > > > drm_i915_private *dev_priv,
> > > > !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > > > if (dev_priv->display.cdclk.hw.vco != vco)
> > > > adlp_cdclk_pll_crawl(dev_priv, vco);
> > > > - } else if (DISPLAY_VER(dev_priv) >= 11)
> > > > + } else if (DISPLAY_VER(dev_priv) >= 11) {
> > > > + if (pll_enable_wa_needed(dev_priv))
> > > > + dg2_cdclk_squash_program(dev_priv, 0);
> > > > +
> > > > icl_cdclk_pll_update(dev_priv, vco);
> > > > - else
> > > > + } else
> > > > bxt_cdclk_pll_update(dev_priv, vco);
> > > >
> > > > waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > > > --
> > > > 2.37.3
> > >
