> -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: Tuesday, December 21, 2021 1:03 AM > To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx> > Subject: Re: [PATCH] drm/i915/display: Move cdclk checks to > atomic check > > On Fri, 17 Dec 2021, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> wrote: > > Checking cdclk conditions during atomic check and preparing for commit > > phase so we can have atomic commit as simple as possible. Add the > > specific steps to be taken during cdclk changes, prepare for > > squashing, crawling and modeset scenarios. > > Rename functions intel_cdclk_can_squash() and > > intel_cdclk_can_crawl() since they no longer simply check if squashing > > and crawling can be performed. > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 130 +++++++++++------- > > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +- > > .../drm/i915/display/intel_display_power.c | 2 +- > > drivers/gpu/drm/i915/i915_drv.h | 13 ++ > > 4 files changed, 96 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 249f81a80eb7..4a5ddc202c05 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1698,12 +1698,15 @@ static void bxt_set_cdclk(struct > drm_i915_private *dev_priv, > > const struct intel_cdclk_config *cdclk_config, > > enum pipe pipe) > > { > > + struct cdclk_steps *cdclk_steps = dev_priv->cdclk.steps; > > int cdclk = cdclk_config->cdclk; > > int vco = cdclk_config->vco; > > + u32 squash_ctl = 0; > > u32 val; > > u16 waveform; > > int clock; > > int ret; > > + int i; > > > > /* Inform power controller of upcoming frequency change. */ > > if (DISPLAY_VER(dev_priv) >= 11) > > @@ -1727,40 +1730,43 @@ 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) > > + for (i = 0; i < CDCLK_ACTIONS; i++) { > > + switch (cdclk_steps[i].action) { > > + case CDCLK_MODESET: > > + if (DISPLAY_VER(dev_priv) >= 11) { > > + if (dev_priv->cdclk.hw.vco != 0 && > > + dev_priv->cdclk.hw.vco != vco) > > + icl_cdclk_pll_disable(dev_priv); > > + > > + if (dev_priv->cdclk.hw.vco != vco) > > + icl_cdclk_pll_enable(dev_priv, vco); > > + } else { > > + if (dev_priv->cdclk.hw.vco != 0 && > > + dev_priv->cdclk.hw.vco != vco) > > + bxt_de_pll_disable(dev_priv); > > + > > + if (dev_priv->cdclk.hw.vco != vco) > > + bxt_de_pll_enable(dev_priv, vco); > > + } > > + clock = cdclk; > > + break; > > + case CDCLK_CRAWL: > > adlp_cdclk_pll_crawl(dev_priv, vco); > > - } else if (DISPLAY_VER(dev_priv) >= 11) { > > - if (dev_priv->cdclk.hw.vco != 0 && > > - dev_priv->cdclk.hw.vco != vco) > > - icl_cdclk_pll_disable(dev_priv); > > - > > - if (dev_priv->cdclk.hw.vco != vco) > > - icl_cdclk_pll_enable(dev_priv, vco); > > - } else { > > - if (dev_priv->cdclk.hw.vco != 0 && > > - dev_priv->cdclk.hw.vco != vco) > > - bxt_de_pll_disable(dev_priv); > > - > > - if (dev_priv->cdclk.hw.vco != vco) > > - 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) > > + clock = cdclk; > > + break; > > + case CDCLK_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); > > + CDCLK_SQUASH_WINDOW_SIZE(0xf) > | waveform; > > + intel_de_write(dev_priv, CDCLK_SQUASH_CTL, > squash_ctl); > > + break; > > + case CDCLK_NOOP: > > + break; > > + default: > > + MISSING_CASE(cdclk_steps[i].action); > > + break; > > + } > > } > > > > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | @@ -1950,10 > > +1956,11 @@ void intel_cdclk_uninit_hw(struct drm_i915_private *i915) > > skl_cdclk_uninit_hw(i915); > > } > > > > -static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv, > > +static bool intel_cdclk_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)) > > @@ -1966,16 +1973,22 @@ 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 = CDCLK_CRAWL; > > + cdclk_transition[0].cdclk = b->cdclk; > > + cdclk_transition[1].action = CDCLK_NOOP; > > + cdclk_transition[1].cdclk = b->cdclk; > > + > > return a->vco != 0 && b->vco != 0 && > > a->vco != b->vco && > > a_div == b_div && > > a->ref == b->ref; > > } > > > > -static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > > +static bool intel_cdclk_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 @@ > > -1985,6 +1998,11 @@ static bool intel_cdclk_can_squash(struct > drm_i915_private *dev_priv, > > if (!has_cdclk_squasher(dev_priv)) > > return false; > > > > + cdclk_transition[0].action = CDCLK_SQUASH; > > + cdclk_transition[0].cdclk = b->cdclk; > > + cdclk_transition[1].action = CDCLK_NOOP; > > + cdclk_transition[1].cdclk = b->cdclk; > > + > > return a->cdclk != b->cdclk && > > a->vco != 0 && > > a->vco == b->vco && > > @@ -2001,9 +2019,19 @@ static bool intel_cdclk_can_squash(struct > drm_i915_private *dev_priv, > > * True if changing between the two CDCLK configurations > > * requires all pipes to be off, false if not. > > */ > > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > > +bool intel_cdclk_needs_modeset(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; > > + > > + if (a->cdclk != b->cdclk || a->vco != b->vco || a->ref != b->ref) { > > + cdclk_transition[0].action = CDCLK_MODESET; > > + cdclk_transition[0].cdclk = b->cdclk; > > + cdclk_transition[1].action = CDCLK_NOOP; > > + cdclk_transition[1].cdclk = b->cdclk; > > + } > > + > > return a->cdclk != b->cdclk || > > a->vco != b->vco || > > a->ref != b->ref; > > @@ -2051,10 +2079,11 @@ static bool intel_cdclk_can_cd2x_update(struct > drm_i915_private *dev_priv, > > * Returns: > > * True if the CDCLK configurations don't match, false if they do. > > */ > > -static bool intel_cdclk_changed(const struct intel_cdclk_config *a, > > +static bool intel_cdclk_changed(struct drm_i915_private *dev_priv, > > + const struct intel_cdclk_config *a, > > const struct intel_cdclk_config *b) { > > - return intel_cdclk_needs_modeset(a, b) || > > + return intel_cdclk_needs_modeset(dev_priv, a, b) || > > a->voltage_level != b->voltage_level; } > > > > @@ -2082,7 +2111,7 @@ static void intel_set_cdclk(struct > > drm_i915_private *dev_priv, { > > struct intel_encoder *encoder; > > > > - if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config)) > > + if (!intel_cdclk_changed(dev_priv, &dev_priv->cdclk.hw, > > +cdclk_config)) > > return; > > > > if (drm_WARN_ON_ONCE(&dev_priv->drm, > > !dev_priv->cdclk_funcs->set_cdclk)) > > @@ -2129,7 +2158,7 @@ static void intel_set_cdclk(struct > drm_i915_private *dev_priv, > > intel_audio_cdclk_change_post(dev_priv); > > > > if (drm_WARN(&dev_priv->drm, > > - intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config), > > + intel_cdclk_changed(dev_priv, &dev_priv->cdclk.hw, > > +cdclk_config), > > "cdclk state doesn't match!\n")) { > > intel_dump_cdclk_config(&dev_priv->cdclk.hw, "[hw > state]"); > > intel_dump_cdclk_config(cdclk_config, "[sw state]"); @@ - > 2153,7 > > +2182,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state > *state) > > intel_atomic_get_new_cdclk_state(state); > > enum pipe pipe = new_cdclk_state->pipe; > > > > - if (!intel_cdclk_changed(&old_cdclk_state->actual, > > + if (!intel_cdclk_changed(dev_priv, &old_cdclk_state->actual, > > &new_cdclk_state->actual)) > > return; > > > > @@ -2182,7 +2211,7 @@ intel_set_cdclk_post_plane_update(struct > intel_atomic_state *state) > > intel_atomic_get_new_cdclk_state(state); > > enum pipe pipe = new_cdclk_state->pipe; > > > > - if (!intel_cdclk_changed(&old_cdclk_state->actual, > > + if (!intel_cdclk_changed(dev_priv, &old_cdclk_state->actual, > > &new_cdclk_state->actual)) > > return; > > > > @@ -2736,7 +2765,7 @@ int intel_modeset_calc_cdclk(struct > intel_atomic_state *state) > > if (ret) > > return ret; > > > > - if (intel_cdclk_changed(&old_cdclk_state->actual, > > + if (intel_cdclk_changed(dev_priv, &old_cdclk_state->actual, > > &new_cdclk_state->actual)) { > > /* > > * Also serialize commits across all crtcs @@ -2747,7 +2776,7 > @@ > > int intel_modeset_calc_cdclk(struct intel_atomic_state *state) > > return ret; > > } else if (old_cdclk_state->active_pipes != new_cdclk_state- > >active_pipes || > > old_cdclk_state->force_min_cdclk != new_cdclk_state- > >force_min_cdclk || > > - intel_cdclk_changed(&old_cdclk_state->logical, > > + intel_cdclk_changed(dev_priv, &old_cdclk_state->logical, > > &new_cdclk_state->logical)) { > > ret = intel_atomic_lock_global_state(&new_cdclk_state- > >base); > > if (ret) > > @@ -2774,14 +2803,14 @@ int intel_modeset_calc_cdclk(struct > intel_atomic_state *state) > > pipe = INVALID_PIPE; > > } > > > > - if (intel_cdclk_can_squash(dev_priv, > > - &old_cdclk_state->actual, > > - &new_cdclk_state->actual)) { > > + if (intel_cdclk_squash(dev_priv, > > + &old_cdclk_state->actual, > > + &new_cdclk_state->actual)) { > > drm_dbg_kms(&dev_priv->drm, > > "Can change cdclk via squasher\n"); > > - } else if (intel_cdclk_can_crawl(dev_priv, > > - &old_cdclk_state->actual, > > - &new_cdclk_state->actual)) { > > + } else if (intel_cdclk_crawl(dev_priv, > > + &old_cdclk_state->actual, > > + &new_cdclk_state->actual)) { > > drm_dbg_kms(&dev_priv->drm, > > "Can change cdclk via crawl\n"); > > } else if (pipe != INVALID_PIPE) { > > @@ -2790,7 +2819,8 @@ int intel_modeset_calc_cdclk(struct > intel_atomic_state *state) > > drm_dbg_kms(&dev_priv->drm, > > "Can change cdclk cd2x divider with pipe %c > active\n", > > pipe_name(pipe)); > > - } else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual, > > + } else if (intel_cdclk_needs_modeset(dev_priv, > > + &old_cdclk_state->actual, > > &new_cdclk_state->actual)) { > > /* All pipes must be switched off while we change the cdclk. > */ > > ret = intel_modeset_all_pipes(state); diff --git > > a/drivers/gpu/drm/i915/display/intel_cdclk.h > > b/drivers/gpu/drm/i915/display/intel_cdclk.h > > index 71dd84740ae3..cc52ef029bb4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > @@ -58,7 +58,8 @@ void intel_init_cdclk_hooks(struct drm_i915_private > > *dev_priv); void intel_update_max_cdclk(struct drm_i915_private > > *dev_priv); void intel_update_cdclk(struct drm_i915_private > > *dev_priv); > > u32 intel_read_rawclk(struct drm_i915_private *dev_priv); -bool > > intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > > +bool intel_cdclk_needs_modeset(struct drm_i915_private *dev_priv, > > + const struct intel_cdclk_config *a, > > const struct intel_cdclk_config *b); void > > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state); > > void intel_set_cdclk_post_plane_update(struct intel_atomic_state > > *state); diff --git > > a/drivers/gpu/drm/i915/display/intel_display_power.c > > b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 05babdcf5f2e..5c6671296019 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -1297,7 +1297,7 @@ static void gen9_disable_dc_states(struct > drm_i915_private *dev_priv) > > intel_cdclk_get_cdclk(dev_priv, &cdclk_config); > > /* Can't read out voltage_level so can't use intel_cdclk_changed() */ > > drm_WARN_ON(&dev_priv->drm, > > - intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, > > + intel_cdclk_needs_modeset(dev_priv, &dev_priv- > >cdclk.hw, > > &cdclk_config)); > > > > gen9_assert_dbuf_enabled(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index ed48179bacd8..2d2f97138fe2 > > 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -119,6 +119,14 @@ > > > > struct drm_i915_gem_object; > > > > +enum cdclk_actions { > > + CDCLK_MODESET = 0, > > + CDCLK_SQUASH, > > + CDCLK_CRAWL, > > + CDCLK_NOOP, > > + CDCLK_ACTIONS > > +}; > > + > > /* Threshold == 5 for long IRQs, 50 for short */ #define > > HPD_STORM_DEFAULT_THRESHOLD 50 > > > > @@ -769,6 +777,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]; > > Is this modeset state recorded directly in struct drm_i915_private instead of > actual modeset state objects? This response got lost in the rest of the emails that I received during vacation. Apologies for the late response. Yeah the cdclk action or the state to be taken is recorded here. Maybe moving this to cdclk_state is better? Anusha > BR, > Jani. > > > } cdclk; > > > > struct { > > -- > Jani Nikula, Intel Open Source Graphics Center