On Wed, Dec 14, 2022 at 09:15:07PM +0200, Srivatsa, Anusha wrote:
>
>
> > -----Original Message-----
> > From: Lisovskiy, Stanislav <[email protected]>
> > Sent: Wednesday, December 14, 2022 2:31 AM
> > To: Srivatsa, Anusha <[email protected]>
> > Cc: [email protected]; Nikula, Jani <[email protected]>
> > Subject: Re: [Intel-gfx] [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 <[email protected]> On Behalf
> > > > Of Stanislav Lisovskiy
> > > > Sent: Thursday, November 24, 2022 2:36 AM
> > > > To: [email protected]
> > > > Cc: Nikula, Jani <[email protected]>
> > > > Subject: [Intel-gfx] [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 <[email protected]>
> > > > ---
> > > > 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
> > >
