On Tue, Sep 10, 2019 at 08:42:46AM -0700, Matt Roper wrote:
> The bspec lays out legal cdclk frequencies, PLL ratios, and CD2X
> dividers in an easy-to-read table for most recent platforms. We've been
> translating the data from that table into platform-specific code logic,
> but it's easy to overlook an area we need to update when adding new
> cdclk values or enabling new platforms. Let's just add a form of the
> bspec table to the code and then adjust our functions to pull what they
> need directly out of the table.
>
> v2: Fix comparison when finding best cdclk.
>
> v3: Another logic fix for calc_cdclk.
>
> v4:
> - Use named initializers for cdclk tables. (Ville)
> - Include refclk as a field in the table instead of adding all three
> ratios for each entry. (Ville)
> - Terminate tables with an empty entry to avoid needing to store the
> table size. (Ville)
> - Don't try so hard to return reasonable values from our lookup
> functions if we get impossible inputs; just WARN and return 0.
> (Ville)
> - Keep a bxt_ prefix on the lookup functions since they're still only
> used on bxt+ for now. We can rename them later if we extend this
> table-based approach back to older platforms. (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 303 +++++++--------------
> drivers/gpu/drm/i915/display/intel_cdclk.h | 7 +
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> 3 files changed, 110 insertions(+), 203 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 01ed3262d91e..c8cf288b8e8e 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1161,28 +1161,88 @@ static void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> }
>
> -static int bxt_calc_cdclk(int min_cdclk)
> -{
> - if (min_cdclk > 576000)
> - return 624000;
> - else if (min_cdclk > 384000)
> - return 576000;
> - else if (min_cdclk > 288000)
> - return 384000;
> - else if (min_cdclk > 144000)
> - return 288000;
> - else
> - return 144000;
> +static const struct intel_cdclk_vals bxt_cdclk_table[] = {
> + { .refclk = 19200, .cdclk = 144000, .divider = 8, .ratio = 60 },
> + { .refclk = 19200, .cdclk = 288000, .divider = 4, .ratio = 60 },
> + { .refclk = 19200, .cdclk = 384000, .divider = 3, .ratio = 60 },
> + { .refclk = 19200, .cdclk = 576000, .divider = 2, .ratio = 60 },
> + { .refclk = 19200, .cdclk = 624000, .divider = 2, .ratio = 65 },
> + {}
> +};
If only we had constexpr so we could do
BUILD_BUG_ON(ref*ratio == cdclk*div) without having to hand roll
it for every table entry. Oh well.
> +
> +static const struct intel_cdclk_vals glk_cdclk_table[] = {
> + { .refclk = 19200, .cdclk = 79200, .divider = 8, .ratio = 33 },
> + { .refclk = 19200, .cdclk = 158400, .divider = 4, .ratio = 33 },
> + { .refclk = 19200, .cdclk = 316800, .divider = 2, .ratio = 33 },
> + {}
> +};
> +
> +static const struct intel_cdclk_vals cnl_cdclk_table[] = {
> + { .refclk = 19200, .cdclk = 168000, 4, 35 },
> + { .refclk = 19200, .cdclk = 336000, 2, 35 },
> + { .refclk = 19200, .cdclk = 528000, 2, 55 },
> +
> + { .refclk = 24000, .cdclk = 168000, 4, 35 },
> + { .refclk = 24000, .cdclk = 336000, 2, 35 },
> + { .refclk = 24000, .cdclk = 528000, 2, 55 },
The ratios looks wrong here. Also missing .divider= and .ratio=.
> + {}
> +};
> +
> +static const struct intel_cdclk_vals icl_cdclk_table[] = {
> + { .refclk = 19200, .cdclk = 172800, .divider = 2, .ratio = 18 },
> + { .refclk = 19200, .cdclk = 192000, .divider = 2, .ratio = 20 },
> + { .refclk = 19200, .cdclk = 307200, .divider = 2, .ratio = 32 },
> + { .refclk = 19200, .cdclk = 326400, .divider = 4, .ratio = 68 },
> + { .refclk = 19200, .cdclk = 556800, .divider = 2, .ratio = 58 },
> + { .refclk = 19200, .cdclk = 652800, .divider = 2, .ratio = 68 },
> +
> + { .refclk = 24000, .cdclk = 180000, .divider = 2, .ratio = 15 },
> + { .refclk = 24000, .cdclk = 192000, .divider = 2, .ratio = 16 },
> + { .refclk = 24000, .cdclk = 312000, .divider = 2, .ratio = 26 },
> + { .refclk = 24000, .cdclk = 324000, .divider = 4, .ratio = 54 },
> + { .refclk = 24000, .cdclk = 552000, .divider = 2, .ratio = 46 },
> + { .refclk = 24000, .cdclk = 648000, .divider = 2, .ratio = 54 },
> +
> + { .refclk = 38400, .cdclk = 172800, .divider = 2, .ratio = 9 },
> + { .refclk = 38400, .cdclk = 192000, .divider = 2, .ratio = 10 },
> + { .refclk = 38400, .cdclk = 307200, .divider = 2, .ratio = 16 },
> + { .refclk = 38400, .cdclk = 326400, .divider = 4, .ratio = 34 },
> + { .refclk = 38400, .cdclk = 556800, .divider = 2, .ratio = 29 },
> + { .refclk = 38400, .cdclk = 652800, .divider = 2, .ratio = 34 },
> + {}
> +};
Everything else seems to check out.
With the cnl tables fixed:
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> +
> +static int bxt_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> +{
> + const struct intel_cdclk_vals *table = dev_priv->cdclk.table;
> + int i;
> +
> + for (i = 0; table[i].refclk; i++)
> + if (table[i].refclk == dev_priv->cdclk.hw.ref &&
> + table[i].cdclk >= min_cdclk)
> + return table[i].cdclk;
> +
> + WARN(1, "Cannot satisfy minimum cdclk %d with refclk %u\n",
> + min_cdclk, dev_priv->cdclk.hw.ref);
> + return 0;
> }
>
> -static int glk_calc_cdclk(int min_cdclk)
> +static int bxt_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> {
> - if (min_cdclk > 158400)
> - return 316800;
> - else if (min_cdclk > 79200)
> - return 158400;
> - else
> - return 79200;
> + const struct intel_cdclk_vals *table = dev_priv->cdclk.table;
> + int i;
> +
> + if (cdclk == dev_priv->cdclk.hw.bypass)
> + return 0;
> +
> + for (i = 0; table[i].refclk; i++)
> + if (table[i].refclk == dev_priv->cdclk.hw.ref &&
> + table[i].cdclk == cdclk)
> + return dev_priv->cdclk.hw.ref * table[i].ratio;
> +
> + WARN(1, "cdclk %d not valid for refclk %u\n",
> + cdclk, dev_priv->cdclk.hw.ref);
> + return 0;
> }
>
> static u8 bxt_calc_voltage_level(int cdclk)
> @@ -1220,52 +1280,6 @@ static u8 ehl_calc_voltage_level(int cdclk)
> return 0;
> }
>
> -static int bxt_de_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> -{
> - int ratio;
> -
> - if (cdclk == dev_priv->cdclk.hw.bypass)
> - return 0;
> -
> - switch (cdclk) {
> - default:
> - MISSING_CASE(cdclk);
> - /* fall through */
> - case 144000:
> - case 288000:
> - case 384000:
> - case 576000:
> - ratio = 60;
> - break;
> - case 624000:
> - ratio = 65;
> - break;
> - }
> -
> - return dev_priv->cdclk.hw.ref * ratio;
> -}
> -
> -static int glk_de_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> -{
> - int ratio;
> -
> - if (cdclk == dev_priv->cdclk.hw.bypass)
> - return 0;
> -
> - switch (cdclk) {
> - default:
> - MISSING_CASE(cdclk);
> - /* fall through */
> - case 79200:
> - case 158400:
> - case 316800:
> - ratio = 33;
> - break;
> - }
> -
> - return dev_priv->cdclk.hw.ref * ratio;
> -}
> -
> static void cnl_readout_refclk(struct drm_i915_private *dev_priv,
> struct intel_cdclk_state *cdclk_state)
> {
> @@ -1576,13 +1590,8 @@ static void bxt_init_cdclk(struct drm_i915_private *dev_priv)
> * - The initial CDCLK needs to be read from VBT.
> * Need to make this change after VBT has changes for BXT.
> */
> - if (IS_GEMINILAKE(dev_priv)) {
> - cdclk_state.cdclk = glk_calc_cdclk(0);
> - cdclk_state.vco = glk_de_pll_vco(dev_priv, cdclk_state.cdclk);
> - } else {
> - cdclk_state.cdclk = bxt_calc_cdclk(0);
> - cdclk_state.vco = bxt_de_pll_vco(dev_priv, cdclk_state.cdclk);
> - }
> + cdclk_state.cdclk = bxt_calc_cdclk(dev_priv, 0);
> + cdclk_state.vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
>
> bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> @@ -1599,16 +1608,6 @@ static void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
> bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> }
>
> -static int cnl_calc_cdclk(int min_cdclk)
> -{
> - if (min_cdclk > 336000)
> - return 528000;
> - else if (min_cdclk > 168000)
> - return 336000;
> - else
> - return 168000;
> -}
> -
> static void cnl_cdclk_pll_disable(struct drm_i915_private *dev_priv)
> {
> u32 val;
> @@ -1718,29 +1717,6 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level;
> }
>
> -static int cnl_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> -{
> - int ratio;
> -
> - if (cdclk == dev_priv->cdclk.hw.bypass)
> - return 0;
> -
> - switch (cdclk) {
> - default:
> - MISSING_CASE(cdclk);
> - /* fall through */
> - case 168000:
> - case 336000:
> - ratio = dev_priv->cdclk.hw.ref == 19200 ? 35 : 28;
> - break;
> - case 528000:
> - ratio = dev_priv->cdclk.hw.ref == 19200 ? 55 : 44;
> - break;
> - }
> -
> - return dev_priv->cdclk.hw.ref * ratio;
> -}
> -
> static void cnl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> {
> u32 cdctl, expected;
> @@ -1783,77 +1759,6 @@ static void cnl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> dev_priv->cdclk.hw.vco = -1;
> }
>
> -static int icl_calc_cdclk(int min_cdclk, unsigned int ref)
> -{
> - static const int ranges_24[] = { 180000, 192000, 312000, 324000,
> - 552000, 648000 };
> - static const int ranges_19_38[] = { 172800, 192000, 307200, 326400,
> - 556800, 652800 };
> - const int *ranges;
> - int len, i;
> -
> - switch (ref) {
> - default:
> - MISSING_CASE(ref);
> - /* fall through */
> - case 24000:
> - ranges = ranges_24;
> - len = ARRAY_SIZE(ranges_24);
> - break;
> - case 19200:
> - case 38400:
> - ranges = ranges_19_38;
> - len = ARRAY_SIZE(ranges_19_38);
> - break;
> - }
> -
> - for (i = 0; i < len; i++) {
> - if (min_cdclk <= ranges[i])
> - return ranges[i];
> - }
> -
> - WARN_ON(min_cdclk > ranges[len - 1]);
> - return ranges[len - 1];
> -}
> -
> -static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> -{
> - int ratio;
> -
> - if (cdclk == dev_priv->cdclk.hw.bypass)
> - return 0;
> -
> - switch (cdclk) {
> - default:
> - MISSING_CASE(cdclk);
> - /* fall through */
> - case 172800:
> - case 307200:
> - case 326400:
> - case 556800:
> - case 652800:
> - WARN_ON(dev_priv->cdclk.hw.ref != 19200 &&
> - dev_priv->cdclk.hw.ref != 38400);
> - break;
> - case 180000:
> - case 312000:
> - case 324000:
> - case 552000:
> - case 648000:
> - WARN_ON(dev_priv->cdclk.hw.ref != 24000);
> - break;
> - case 192000:
> - WARN_ON(dev_priv->cdclk.hw.ref != 19200 &&
> - dev_priv->cdclk.hw.ref != 38400 &&
> - dev_priv->cdclk.hw.ref != 24000);
> - break;
> - }
> -
> - ratio = cdclk / (dev_priv->cdclk.hw.ref / 2);
> -
> - return dev_priv->cdclk.hw.ref * ratio;
> -}
> -
> static void icl_init_cdclk(struct drm_i915_private *dev_priv)
> {
> struct intel_cdclk_state sanitized_state;
> @@ -1882,8 +1787,8 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv)
> DRM_DEBUG_KMS("Sanitizing cdclk programmed by pre-os\n");
>
> sanitized_state.ref = dev_priv->cdclk.hw.ref;
> - sanitized_state.cdclk = icl_calc_cdclk(0, sanitized_state.ref);
> - sanitized_state.vco = icl_calc_cdclk_pll_vco(dev_priv,
> + sanitized_state.cdclk = bxt_calc_cdclk(dev_priv, 0);
> + sanitized_state.vco = bxt_calc_cdclk_pll_vco(dev_priv,
> sanitized_state.cdclk);
> if (IS_ELKHARTLAKE(dev_priv))
> sanitized_state.voltage_level =
> @@ -1923,8 +1828,8 @@ static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
>
> cdclk_state = dev_priv->cdclk.hw;
>
> - cdclk_state.cdclk = cnl_calc_cdclk(0);
> - cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> + cdclk_state.cdclk = bxt_calc_cdclk(dev_priv, 0);
> + cdclk_state.vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
>
> cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> @@ -2426,13 +2331,8 @@ static int bxt_modeset_calc_cdclk(struct intel_atomic_state *state)
> if (min_cdclk < 0)
> return min_cdclk;
>
> - if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(min_cdclk);
> - vco = glk_de_pll_vco(dev_priv, cdclk);
> - } else {
> - cdclk = bxt_calc_cdclk(min_cdclk);
> - vco = bxt_de_pll_vco(dev_priv, cdclk);
> - }
> + cdclk = bxt_calc_cdclk(dev_priv, min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2440,13 +2340,8 @@ static int bxt_modeset_calc_cdclk(struct intel_atomic_state *state)
> bxt_calc_voltage_level(cdclk);
>
> if (!state->active_pipes) {
> - if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(state->cdclk.force_min_cdclk);
> - vco = glk_de_pll_vco(dev_priv, cdclk);
> - } else {
> - cdclk = bxt_calc_cdclk(state->cdclk.force_min_cdclk);
> - vco = bxt_de_pll_vco(dev_priv, cdclk);
> - }
> + cdclk = bxt_calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2468,8 +2363,8 @@ static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
> if (min_cdclk < 0)
> return min_cdclk;
>
> - cdclk = cnl_calc_cdclk(min_cdclk);
> - vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> + cdclk = bxt_calc_cdclk(dev_priv, min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2478,8 +2373,8 @@ static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
> cnl_compute_min_voltage_level(state));
>
> if (!state->active_pipes) {
> - cdclk = cnl_calc_cdclk(state->cdclk.force_min_cdclk);
> - vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> + cdclk = bxt_calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2495,15 +2390,14 @@ static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
> static int icl_modeset_calc_cdclk(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> - unsigned int ref = state->cdclk.logical.ref;
> int min_cdclk, cdclk, vco;
>
> min_cdclk = intel_compute_min_cdclk(state);
> if (min_cdclk < 0)
> return min_cdclk;
>
> - cdclk = icl_calc_cdclk(min_cdclk, ref);
> - vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> + cdclk = bxt_calc_cdclk(dev_priv, min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2517,8 +2411,8 @@ static int icl_modeset_calc_cdclk(struct intel_atomic_state *state)
> cnl_compute_min_voltage_level(state));
>
> if (!state->active_pipes) {
> - cdclk = icl_calc_cdclk(state->cdclk.force_min_cdclk, ref);
> - vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> + cdclk = bxt_calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2754,12 +2648,15 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> if (INTEL_GEN(dev_priv) >= 11) {
> dev_priv->display.set_cdclk = cnl_set_cdclk;
> dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk;
> + dev_priv->cdclk.table = icl_cdclk_table;
> } else if (IS_CANNONLAKE(dev_priv)) {
> dev_priv->display.set_cdclk = cnl_set_cdclk;
> dev_priv->display.modeset_calc_cdclk = cnl_modeset_calc_cdclk;
> + dev_priv->cdclk.table = cnl_cdclk_table;
> } else if (IS_GEN9_LP(dev_priv)) {
> dev_priv->display.set_cdclk = bxt_set_cdclk;
> dev_priv->display.modeset_calc_cdclk = bxt_modeset_calc_cdclk;
> + dev_priv->cdclk.table = bxt_cdclk_table;
> } else if (IS_GEN9_BC(dev_priv)) {
> dev_priv->display.set_cdclk = skl_set_cdclk;
> dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 4d6f7f5f8930..ca6d651946b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -15,6 +15,13 @@ struct intel_atomic_state;
> struct intel_cdclk_state;
> struct intel_crtc_state;
>
> +struct intel_cdclk_vals {
> + u32 refclk;
> + u32 cdclk;
> + u8 divider; /* CD2X divider * 2 */
> + u8 ratio;
> +};
> +
> int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> void intel_cdclk_init(struct drm_i915_private *i915);
> void intel_cdclk_uninit(struct drm_i915_private *i915);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e289b4ffd34b..ff6aff2a4866 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1420,6 +1420,9 @@ struct drm_i915_private {
> /* The current hardware cdclk state */
> struct intel_cdclk_state hw;
>
> + /* cdclk, divider, and ratio table from bspec */
> + const struct intel_cdclk_vals *table;
> +
> int force_min_cdclk;
> } cdclk;
>
> --
> 2.20.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
