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