[Intel-gfx] [PATCH 2/8] drm/i915: Use literal representation of cdclk tables
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Sep 10 12:56:16 UTC 2019
On Sat, Sep 07, 2019 at 09:05:00PM -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.
>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 318 ++++++++-------------
> drivers/gpu/drm/i915/display/intel_cdclk.h | 8 +
> drivers/gpu/drm/i915/i915_drv.h | 4 +
> 3 files changed, 125 insertions(+), 205 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index e07de3b84cec..ca64143b6d7e 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1161,28 +1161,96 @@ 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[] = {
> + { 144000, 8, 60 },
> + { 288000, 4, 60 },
> + { 384000, 3, 60 },
> + { 576000, 2, 60 },
> + { 624000, 2, 65 },
Named initializers might make this number soup digestible.
> +};
> +
> +static const struct intel_cdclk_vals glk_cdclk_table[] = {
> + { 79200, 8, 33 },
> + { 158400, 4, 33 },
> + { 316800, 2, 33 },
> +};
> +
> +static const struct intel_cdclk_vals cnl_cdclk_table[] = {
> + { 168000, 4, 35, 28 },
> + { 336000, 2, 35, 28 },
> + { 528000, 2, 55, 44 },
> +};
> +
> +static const struct intel_cdclk_vals icl_cdclk_table[] = {
> + { 172800, 2, 18, 0, 9 },
> + { 180000, 2, 0, 15, 0 },
> + { 192000, 2, 20, 16, 10 },
> + { 307200, 2, 32, 0, 16 },
> + { 312000, 2, 0, 26, 0 },
> + { 324000, 4, 0, 54, 0 },
> + { 326400, 4, 68, 0, 34 },
> + { 552000, 2, 0, 46, 0 },
> + { 556800, 2, 58, 0, 29 },
> + { 648000, 2, 0, 54, 0 },
> + { 652800, 2, 68, 0, 34 },
> +};
> +
> +static int calc_cdclk(struct drm_i915_private *dev_priv,
> + int min_cdclk)
> +{
> + const struct intel_cdclk_vals *table = dev_priv->cdclk.table;
> + unsigned int ref = dev_priv->cdclk.hw.ref;
> + int best_cdclk = 0;
> + int i;
> +
> + if (WARN_ON(ref != 19200 && ref != 24000 && ref != 38400))
> + ref = 19200;
Feels a bit like dead code.
> +
> + for (i = 0; i < dev_priv->cdclk.table_size; i++) {
> + if ((ref == 19200 && table[i].ratio_19 == 0) ||
> + (ref == 24000 && table[i].ratio_24 == 0) ||
> + (ref == 38400 && table[i].ratio_38 == 0))
> + continue;
> +
> + best_cdclk = table[i].cdclk;
> + if (table[i].cdclk >= min_cdclk)
> + return best_cdclk;
> + }
> +
> + WARN(1, "Cannot satisfy minimum cdclk %d\n", min_cdclk);
> + return best_cdclk;
Same for the best_cdclk thing. I'd probably just MISSING_CASE()
and return 0, or something.
> }
>
> -static int glk_calc_cdclk(int min_cdclk)
> +static int calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
We still use these things for bxt+ only, so would be nice if
the names kept reflecting that fact.
Though I suppose we should be able to extend this more
declarative approach backwards to older platforms as well.
> {
> - 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 ratio, best_ratio, i;
> +
> + if (cdclk == dev_priv->cdclk.hw.bypass)
> + return 0;
> +
> + for (i = 0; i < dev_priv->cdclk.table_size; i++) {
> + if (dev_priv->cdclk.hw.ref == 19200)
> + ratio = table[i].ratio_19;
> + else if (dev_priv->cdclk.hw.ref == 24000)
> + ratio = table[i].ratio_24;
> + else
> + ratio = table[i].ratio_38;
> +
> + if (ratio == 0)
> + continue;
> + else
> + best_ratio = ratio;
> +
> + if (table[i].cdclk == cdclk ||
> + WARN_ON(table[i].cdclk > cdclk))
> + return dev_priv->cdclk.hw.ref * ratio;
> + }
> +
> + WARN(1, "cdclk %d not valid for refclk %d\n",
> + cdclk, dev_priv->cdclk.hw.ref);
> +
> + return dev_priv->cdclk.hw.ref * best_ratio;
> }
>
> static u8 bxt_calc_voltage_level(int cdclk)
> @@ -1220,52 +1288,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 +1598,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 = calc_cdclk(dev_priv, 0);
> + cdclk_state.vco = 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 +1616,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 +1725,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 +1767,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,9 +1795,9 @@ 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);
> + sanitized_state.cdclk = calc_cdclk(dev_priv, 0);
> + sanitized_state.vco = calc_cdclk_pll_vco(dev_priv,
> + sanitized_state.cdclk);
> if (IS_ELKHARTLAKE(dev_priv))
> sanitized_state.voltage_level =
> ehl_calc_voltage_level(sanitized_state.cdclk);
> @@ -1923,8 +1836,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 = calc_cdclk(dev_priv, 0);
> + cdclk_state.vco = 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 +2339,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 = calc_cdclk(dev_priv, min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2440,13 +2348,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 = calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2468,8 +2371,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 = calc_cdclk(dev_priv, min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2478,8 +2381,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 = calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2495,15 +2398,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 = calc_cdclk(dev_priv, min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> @@ -2517,8 +2419,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 = calc_cdclk(dev_priv, state->cdclk.force_min_cdclk);
> + vco = calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.actual.vco = vco;
> state->cdclk.actual.cdclk = cdclk;
> @@ -2754,12 +2656,18 @@ 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;
> + dev_priv->cdclk.table_size = ARRAY_SIZE(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;
> + dev_priv->cdclk.table_size = ARRAY_SIZE(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;
> + dev_priv->cdclk.table_size = ARRAY_SIZE(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;
> @@ -2774,7 +2682,7 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
> }
>
> - else if (IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 10)
> + if (IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 10)
> dev_priv->display.get_cdclk = bxt_get_cdclk;
> else if (IS_GEN9_BC(dev_priv))
> dev_priv->display.get_cdclk = skl_get_cdclk;
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 4d6f7f5f8930..80dc17a07aa2 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -15,6 +15,14 @@ struct intel_atomic_state;
> struct intel_cdclk_state;
> struct intel_crtc_state;
>
> +struct intel_cdclk_vals {
> + int cdclk;
> + int divider; /* CD2X divider * 2 */
> + int ratio_19;
> + int ratio_24;
> + int ratio_38;
I think a bunch of these can easily fit smaller types.
A more general approach might be just
struct ... {
cdclk
ref
divider
ratio
};
Ie. add a unique entry for each cdclk+ref combo. that way we don't have
to encode the set of possible ref clocks here at all, and thus can use
this still when the set of ref clocks changes on some future platform.
> +};
> +
> 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 db7480831e52..a4d249193dc8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1420,6 +1420,10 @@ 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 table_size;
Or we terminate the tables with {} ?
> +
> int force_min_cdclk;
> } cdclk;
>
> --
> 2.20.1
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list