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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2017-02-28 às 18:57 -0800, Dhinakaran Pandiyan escreveu:
> 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;

Here we're just looking at the CRTCs in the state. Notice that
max_pixel_rate caches the values for all the CRTCs so we can account
for all of them instead of just the ones that are in the current atomic
commit.

What if some other CRTC not in the current state raised the minimum
clock to 432/316? Don't we end up risking not noticing it and going
with a lower clock here? Imagine two very-low-res monitors with audio
enabled. First we do the modeset on the audio monitor, then we enable
the other monitor. Won't the second modeset end up wrongly reducing the
clock below 432/316?

In other words: why doesn't min_cdclk need to do the same caching
scheme that max_cdclk needs?

>  
> -	/* 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);

Can you please clarify why it's not possible to simply add a new check
for GLK in the old function? That would have been simpler.

If we still want to go with this approach, I'd suggest splitting your
commit in two separate commits: one reworking the current code (and
addressing the issue I pointed above), and another adding the GLK
stuff.


> +
> +		/* 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);
>  
>  	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);
>  	}
>  
> @@ -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