Re: [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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@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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx