> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Friday, December 10, 2021 4:17 PM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [RFC] drm/i915/display: Move cdclk checks to atomic
> check
>
> On Thu, 09 Dec 2021, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> wrote:
> > i915 has squashing for DG2 and crawling for ADLP.
> > Moving the checks to atomic check phase so at a later phase we know
> > how the cdclk changes.
>
> Just some high level comments:
>
> - Functions named intel_cdclk_can_foo() must *not* change the state,
> that's unexpected and surprising.
>
> - There's a bunch of state handling already for cdclk, please don't just
> dump new state in drm_i915_private, outside of the existing states. In
> particular, storing yet another copy of cdclk is suspicious.
>
> - Please don't add short stuff like CRAWL and SQUASH to our module
> namespace.
>
Will keep in mind for hopefully a better non-RFC version. Thanks for the feedback.
Anusha
> BR,
> Jani.
>
>
> >
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 49 +++++++++++++---------
> > drivers/gpu/drm/i915/i915_drv.h | 11 +++++
> > 2 files changed, 41 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 639a64733f61..9382dd24d889 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1707,9 +1707,27 @@ static void bxt_set_cdclk(struct
> drm_i915_private *dev_priv,
> > return;
> > }
> >
> > - if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 &&
> vco > 0) {
> > - if (dev_priv->cdclk.hw.vco != vco)
> > - adlp_cdclk_pll_crawl(dev_priv, vco);
> > + if (DISPLAY_VER(dev_priv) >= 12) {
> > + int i = 0;
> > + u32 squash_ctl = 0;
> > + struct cdclk_steps *cdclk_steps = dev_priv->cdclk.steps;
> > +
> > + for (i = 0; i < CDCLK_ACTIONS; i++) {
> > + switch (cdclk_steps[i].action) {
> > + case CRAWL:
> > + adlp_cdclk_pll_crawl(dev_priv, vco);
> > + break;
> > + case SQUASH:
> > + waveform =
> cdclk_squash_waveform(dev_priv, cdclk_steps[i].cdclk);
> > + clock = vco / 2;
> > + squash_ctl = CDCLK_SQUASH_ENABLE |
> > + CDCLK_SQUASH_WINDOW_SIZE(0xf) |
> waveform;
> > + intel_de_write(dev_priv,
> CDCLK_SQUASH_CTL, squash_ctl);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > } else if (DISPLAY_VER(dev_priv) >= 11) {
> > if (dev_priv->cdclk.hw.vco != 0 &&
> > dev_priv->cdclk.hw.vco != vco)
> > @@ -1726,22 +1744,7 @@ static void bxt_set_cdclk(struct
> drm_i915_private *dev_priv,
> > bxt_de_pll_enable(dev_priv, vco);
> > }
> >
> > - waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > -
> > - if (waveform)
> > - clock = vco / 2;
> > - else
> > - clock = cdclk;
> > -
> > - if (has_cdclk_squasher(dev_priv)) {
> > - u32 squash_ctl = 0;
> > -
> > - if (waveform)
> > - squash_ctl = CDCLK_SQUASH_ENABLE |
> > - CDCLK_SQUASH_WINDOW_SIZE(0xf) |
> waveform;
> > -
> > - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
> > - }
> > + clock = cdclk;
> >
> > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
> > bxt_cdclk_cd2x_pipe(dev_priv, pipe) | @@ -1934,6 +1937,7
> @@ static
> > bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
> > const struct intel_cdclk_config *a,
> > const struct intel_cdclk_config *b) {
> > + struct cdclk_steps *cdclk_transition = dev_priv->cdclk.steps;
> > int a_div, b_div;
> >
> > if (!HAS_CDCLK_CRAWL(dev_priv))
> > @@ -1946,6 +1950,9 @@ static bool intel_cdclk_can_crawl(struct
> drm_i915_private *dev_priv,
> > a_div = DIV_ROUND_CLOSEST(a->vco, a->cdclk);
> > b_div = DIV_ROUND_CLOSEST(b->vco, b->cdclk);
> >
> > + cdclk_transition[0].action = CRAWL;
> > + cdclk_transition[0].cdclk = b->cdclk;
> > +
> > return a->vco != 0 && b->vco != 0 &&
> > a->vco != b->vco &&
> > a_div == b_div &&
> > @@ -1956,6 +1963,7 @@ static bool intel_cdclk_can_squash(struct
> drm_i915_private *dev_priv,
> > const struct intel_cdclk_config *a,
> > const struct intel_cdclk_config *b) {
> > + struct cdclk_steps *cdclk_transition = dev_priv->cdclk.steps;
> > /*
> > * FIXME should store a bit more state in intel_cdclk_config
> > * to differentiate squasher vs. cd2x divider properly. For @@
> > -1965,6 +1973,9 @@ static bool intel_cdclk_can_squash(struct
> drm_i915_private *dev_priv,
> > if (!has_cdclk_squasher(dev_priv))
> > return false;
> >
> > + cdclk_transition[0].action = SQUASH;
> > + cdclk_transition[0].cdclk = b->cdclk;
> > +
> > return a->cdclk != b->cdclk &&
> > a->vco != 0 &&
> > a->vco == b->vco &&
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index ae7dc7862b5d..c03299253b81
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -117,6 +117,12 @@
> >
> > struct drm_i915_gem_object;
> >
> > +enum cdclk_actions {
> > + SQUASH = 0,
> > + CRAWL,
> > + CDCLK_ACTIONS
> > +};
> > +
> > /* Threshold == 5 for long IRQs, 50 for short */ #define
> > HPD_STORM_DEFAULT_THRESHOLD 50
> >
> > @@ -782,6 +788,11 @@ struct drm_i915_private {
> > const struct intel_cdclk_vals *table;
> >
> > struct intel_global_obj obj;
> > +
> > + struct cdclk_steps {
> > + enum cdclk_actions action;
> > + u32 cdclk;
> > + } steps[CDCLK_ACTIONS];
> > } cdclk;
> >
> > struct {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
