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.
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
>
