[Intel-gfx] [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Fri Mar 3 12:14:44 UTC 2017
On Tue, 2017-02-28 at 18:57 -0800, Dhinakaran Pandiyan wrote:
> Implement GLK cdclk restriction for DP audio, similar to what's implemented
> for BDW and other GEN9 platforms. The cdclk restriction has been
> refactored out of max. pixel clock computation as the 1:1 relationship
> between pixel clock and cdclk frequency does not hold for GLK.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++--------------
> 1 file changed, 52 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..8fc0f72 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1069,11 +1069,11 @@ static int bxt_calc_cdclk(int max_pixclk)
> return 144000;
> }
>
> -static int glk_calc_cdclk(int max_pixclk)
> +static int glk_calc_cdclk(int max_pixclk, int min_cdclk)
> {
> - if (max_pixclk > 2 * 158400)
> + if (max_pixclk > 2 * 158400 || min_cdclk > 158400)
> return 316800;
> - else if (max_pixclk > 2 * 79200)
> + else if (max_pixclk > 2 * 79200 || min_cdclk > 79200)
> return 158400;
> else
> return 79200;
> @@ -1367,7 +1367,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv)
> * 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.cdclk = glk_calc_cdclk(0, 0);
> cdclk_state.vco = glk_de_pll_vco(dev_priv, cdclk_state.cdclk);
> } else {
> cdclk_state.cdclk = bxt_calc_cdclk(0);
> @@ -1432,28 +1432,37 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> }
>
> -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> - int pixel_rate)
> +static int intel_min_cdclk(struct drm_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv =
> - to_i915(crtc_state->base.crtc->dev);
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> + int min_cdclk = 0;
>
> - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + for_each_crtc_in_state(state, crtc, cstate, i) {
> + struct intel_crtc_state *crtc_state;
>
> - /* BSpec says "Do not use DisplayPort with CDCLK less than
> - * 432 MHz, audio enabled, port width x4, and link rate
> - * HBR2 (5.4 GHz), or else there may be audio corruption or
> - * screen corruption."
> - */
> - if (intel_crtc_has_dp_encoder(crtc_state) &&
> - crtc_state->has_audio &&
> - crtc_state->port_clock >= 540000 &&
> - crtc_state->lane_count == 4)
> - pixel_rate = max(432000, pixel_rate);
> + crtc_state = to_intel_crtc_state(cstate);
> +
> + /* According to BSpec, "Do not use DisplayPort with CDCLK less
> + * than 432 MHz, audio enabled, port width x4, and link rate
> + * HBR2 (5.4 GHz), or else there may be audio corruption or
> + * screen corruption." for BDW and GEN9. The cdclk restriction
> + * for GLK is at 316.8 MHz
> + */
> + if (intel_crtc_has_dp_encoder(crtc_state) &&
> + crtc_state->has_audio &&
> + crtc_state->port_clock >= 540000 &&
> + crtc_state->lane_count == 4) {
> + if (IS_GEMINILAKE(dev_priv))
> + min_cdclk = 316800;
> + else if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> + min_cdclk = 432000;
> + }
> + }
>
> - return pixel_rate;
> + return min_cdclk;
> }
>
> /* compute the max rate for new configuration */
> @@ -1481,10 +1490,9 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>
> pixel_rate = crtc_state->pixel_rate;
>
> - if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> - pixel_rate =
> - bdw_adjust_min_pipe_pixel_rate(crtc_state,
> - pixel_rate);
> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> + pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>
> intel_state->min_pixclk[i] = pixel_rate;
> }
> @@ -1531,13 +1539,17 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk;
>
> /*
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = bdw_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bdw_calc_cdclk(min_cdclk);
> + else
> + cdclk = bdw_calc_cdclk(max_pixclk);
Looks like intel_min_cdclk() returns a valid cdclk for the platform, so wouldn't
cdclk = *_calc_cdclk(max_pixclk);
if (cdclk < min_cdclk)
cdclk = min_cdclk;
be simpler?
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1564,6 +1576,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> const int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> vco = intel_state->cdclk.logical.vco;
> @@ -1574,7 +1587,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = skl_calc_cdclk(max_pixclk, vco);
> + if (min_cdclk > max_pixclk)
> + cdclk = skl_calc_cdclk(min_cdclk, vco);
> + else
> + cdclk = skl_calc_cdclk(max_pixclk, vco);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1604,13 +1620,18 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> int max_pixclk = intel_max_pixel_rate(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(max_pixclk);
> + cdclk = glk_calc_cdclk(max_pixclk, min_cdclk);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> - cdclk = bxt_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bxt_calc_cdclk(min_cdclk);
> + else
> + cdclk = bxt_calc_cdclk(max_pixclk);
> +
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
With the idea above, this would become
if (IS_GEMINILAKE(dev_priv))
cdclk = glk_calc_cdclk(max_pixclk);
else
cdclk = bxt_calc_cdclk(max_pixclk);
if (cdclk < min_cdclk)
cdclk = min_cdclk;
if (IS_GEMINILAKE(dev_priv))
vco = glk_de_pll_vco(dev_priv, cdclk);
else
vco = bxt_de_pll_vco(dev_priv, cdclk);
Not great, but maybe still better than changing the signature of
glk_calc_cdclk()?
Also, this conflicts with Paulo's cdclk clean up [1].
[1] https://patchwork.freedesktop.org/series/19974/
Ander
>
> @@ -1625,7 +1646,7 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> if (!intel_state->active_crtcs) {
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(0);
> + cdclk = glk_calc_cdclk(0, 0);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> cdclk = bxt_calc_cdclk(0);
More information about the Intel-gfx
mailing list