[PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Dhinakaran Pandiyan]

According to BSpec, cdclk has to be not less than 432 MHz with DP audio
enabled, port width x4, and link rate HBR2 (5.4 GHz)

Having a lower cdclk triggers pipe underruns, which then lead to displays
continuously cycling off and on. This is essential for DP MST audio as the
link is trained at HBR2 and 4 lanes by default.

This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cfcb03f..6a05183 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
 	return 0;
 }
 
+static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
+{
+
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	/* 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."
+	 */
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc_state);
+
+		return (intel_crtc_has_dp_encoder(pipe_config) &&
+			pipe_config->has_audio &&
+			pipe_config->port_clock == 540000 &&
+			pipe_config->lane_count == 4);
+	}
+
+	return false;
+}
+
 static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	int max_pixclk = ilk_max_pixel_rate(state);
+	int cdclk, min_cdclk = 0;
 	struct intel_atomic_state *intel_state =
 		to_intel_atomic_state(state);
 
-	intel_state->cdclk = intel_state->dev_cdclk =
-		bxt_calc_cdclk(max_pixclk);
+	if (cdclk_min_for_dp_audio(state))
+		min_cdclk = bxt_calc_cdclk(432000);
+
+	cdclk = bxt_calc_cdclk(max_pixclk);
+	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
 
 	if (!intel_state->active_crtcs)
 		intel_state->dev_cdclk = bxt_calc_cdclk(0);
@@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
-	int cdclk;
+	int cdclk, min_cdclk = 0;
+
+	if (cdclk_min_for_dp_audio(state))
+		min_cdclk = broadwell_calc_cdclk(432000);
 
 	/*
 	 * FIXME should also account for plane ratio
@@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
 	 */
 	cdclk = broadwell_calc_cdclk(max_pixclk);
 
+	cdclk =	max(min_cdclk, cdclk);
+
 	if (cdclk > dev_priv->max_cdclk_freq) {
 		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
 			      cdclk, dev_priv->max_cdclk_freq);
@@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	const int max_pixclk = ilk_max_pixel_rate(state);
 	int vco = intel_state->cdclk_pll_vco;
-	int cdclk;
+	int cdclk, min_cdclk = 0;
+
+	if (cdclk_min_for_dp_audio(state))
+		min_cdclk = skl_calc_cdclk(432000, vco);
 
 	/*
 	 * FIXME should also account for plane ratio
@@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	 */
 	cdclk = skl_calc_cdclk(max_pixclk, vco);
 
+	cdclk = max(min_cdclk, cdclk);
+
 	/*
 	 * FIXME move the cdclk caclulation to
 	 * compute_config() so we can fail gracegully.
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Paulo Zanoni]

Em Qui, 2016-10-13 às 11:04 -0700, Dhinakaran Pandiyan escreveu:
> According to BSpec, cdclk has to be not less than 432 MHz with DP
> audio
> enabled, port width x4, and link rate HBR2 (5.4 GHz)

This is just for pre-production hardware, and we don't implement
workarounds for pre-prod.


A quick research seems to indicate that our CDCLK must be at least
twice the frequency of the audio BCLK. Maybe this is what we need to
investigate/check?

Anyway, it looks like we're getting closer to the final bug fix.

> 
> Having a lower cdclk triggers pipe underruns, which then lead to
> displays
> continuously cycling off and on. This is essential for DP MST audio
> as the
> link is trained at HBR2 and 4 lanes by default.
> 
> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907

Please use the "Bugzilla:" tag since there are some scripts and robots
that parse it and do stuff with it.


> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47
> +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index cfcb03f..6a05183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,43 @@ static int
> valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> +{
> +
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	/* 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."
> +	 */
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> +			pipe_config->has_audio &&
> +			pipe_config->port_clock == 540000 &&
> +			pipe_config->lane_count == 4);
> +	}
> +
> +	return false;
> +}
> +
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk, min_cdclk = 0;
>  	struct intel_atomic_state *intel_state =
>  		to_intel_atomic_state(state);
>  
> -	intel_state->cdclk = intel_state->dev_cdclk =
> -		bxt_calc_cdclk(max_pixclk);
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = bxt_calc_cdclk(432000);
> +
> +	cdclk = bxt_calc_cdclk(max_pixclk);
> +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk,
> cdclk);
>  
>  	if (!intel_state->active_crtcs)
>  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> @@ -10374,7 +10403,10 @@ static int
> broadwell_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 = ilk_max_pixel_rate(state);
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = broadwell_calc_cdclk(432000);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10382,6 +10414,8 @@ static int
> broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = broadwell_calc_cdclk(max_pixclk);
>  
> +	cdclk =	max(min_cdclk, cdclk);
> +
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> (%d kHz)\n",
>  			      cdclk, dev_priv->max_cdclk_freq);
> @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	const int max_pixclk = ilk_max_pixel_rate(state);
>  	int vco = intel_state->cdclk_pll_vco;
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = skl_calc_cdclk(432000, vco);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(max_pixclk, vco);
>  
> +	cdclk = max(min_cdclk, cdclk);
> +
>  	/*
>  	 * FIXME move the cdclk caclulation to
>  	 * compute_config() so we can fail gracegully.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Jim Bride]

On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> enabled, port width x4, and link rate HBR2 (5.4 GHz)
> 
> Having a lower cdclk triggers pipe underruns, which then lead to displays
> continuously cycling off and on. This is essential for DP MST audio as the
> link is trained at HBR2 and 4 lanes by default.
> 
> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cfcb03f..6a05183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> +{
> +
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	/* 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."
> +	 */

It's probably worth mentioning that this limitation really only applies for
SKL (at least that I can see) and BXT A (which I don't think we even care
about anymore since it's not a production part.)  Were we seeing underruns
on non-SKL platforms that increasing CDCLK fixed?   In any event, I'd rather
see this only implemented for SKL (see BDW code below) unless we've empirically
noticed a similar problem on BDW (or the B-Spec says something about this and I
missed it.)

Jim


> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> +			pipe_config->has_audio &&
> +			pipe_config->port_clock == 540000 &&
> +			pipe_config->lane_count == 4);
> +	}
> +
> +	return false;
> +}
> +
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk, min_cdclk = 0;
>  	struct intel_atomic_state *intel_state =
>  		to_intel_atomic_state(state);
>  
> -	intel_state->cdclk = intel_state->dev_cdclk =
> -		bxt_calc_cdclk(max_pixclk);
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = bxt_calc_cdclk(432000);
> +
> +	cdclk = bxt_calc_cdclk(max_pixclk);
> +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
>  
>  	if (!intel_state->active_crtcs)
>  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = broadwell_calc_cdclk(432000);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = broadwell_calc_cdclk(max_pixclk);
>  
> +	cdclk =	max(min_cdclk, cdclk);
> +
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
>  			      cdclk, dev_priv->max_cdclk_freq);
> @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	const int max_pixclk = ilk_max_pixel_rate(state);
>  	int vco = intel_state->cdclk_pll_vco;
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = skl_calc_cdclk(432000, vco);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(max_pixclk, vco);
>  
> +	cdclk = max(min_cdclk, cdclk);
> +
>  	/*
>  	 * FIXME move the cdclk caclulation to
>  	 * compute_config() so we can fail gracegully.
> -- 
> 2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Ville Syrjälä]

On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> enabled, port width x4, and link rate HBR2 (5.4 GHz)
> 
> Having a lower cdclk triggers pipe underruns, which then lead to displays
> continuously cycling off and on. This is essential for DP MST audio as the
> link is trained at HBR2 and 4 lanes by default.
> 
> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cfcb03f..6a05183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> +{
> +
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	/* 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."
> +	 */
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> +			pipe_config->has_audio &&
> +			pipe_config->port_clock == 540000 &&
> +			pipe_config->lane_count == 4);
> +	}

That's not good enough on account of crtcs not part of the state
potentially needing the workaround as well. However, since we only do
this when there's a modeset, I think we'll be covered by the
connection_mutex, and so we should be able to peek at the current state
of all crtcs without grabbing the corresponding crtc locks.

So I think we'd be OK with something like:

WARN_ON(!locked(connection_mutex));

for_each_intel_crtc() {
	/*
	 * Peeking at the current state is safe since
	 * we can only get here while holding the
	 * connection_mutex.
	 */
	crtc_state = intel_get_existing_crtc_state();
	if (!crtc_state)
		crtc_state = to_intel_crtc_state(crtc->base.state);

	...
}

The other option would be to track the min cdclk for each pipe in the
top level state I suppose. We already do that for the pixel rate
actually so that we can calculate the cdclk to begin with. Hmm. Maybe
we should just switch to tracking the min cdclk per pipe instead of the
pixel rate. Or did we need to the pixel rate itself for something else,
Maarten?

Or we could perhaps replace the pixel rate/pixclk tracking with the peek
approach entirely. Not quite sure. Would have to read the entire thing
through.

> +
> +	return false;
> +}
> +
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk, min_cdclk = 0;
>  	struct intel_atomic_state *intel_state =
>  		to_intel_atomic_state(state);
>  
> -	intel_state->cdclk = intel_state->dev_cdclk =
> -		bxt_calc_cdclk(max_pixclk);
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = bxt_calc_cdclk(432000);
> +
> +	cdclk = bxt_calc_cdclk(max_pixclk);
> +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
>  
>  	if (!intel_state->active_crtcs)
>  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = broadwell_calc_cdclk(432000);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = broadwell_calc_cdclk(max_pixclk);
>  
> +	cdclk =	max(min_cdclk, cdclk);
> +
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
>  			      cdclk, dev_priv->max_cdclk_freq);
> @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	const int max_pixclk = ilk_max_pixel_rate(state);
>  	int vco = intel_state->cdclk_pll_vco;
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = skl_calc_cdclk(432000, vco);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(max_pixclk, vco);
>  
> +	cdclk = max(min_cdclk, cdclk);
> +
>  	/*
>  	 * FIXME move the cdclk caclulation to
>  	 * compute_config() so we can fail gracegully.
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Patchwork]

== Series Details ==

Series: drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2
URL   : https://patchwork.freedesktop.org/series/13745/
State : success

== Summary ==

Series 13745v1 drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2
https://patchwork.freedesktop.org/api/1.0/series/13745/revisions/1/mbox/

Test drv_module_reload_basic:
                skip       -> PASS       (fi-skl-6770hq)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (fi-ilk-650)
Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-hsw-4770)
                skip       -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:212  dwarn:2   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:246  pass:210  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:184  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:221  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:246  pass:230  dwarn:1   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2709/

dbcf6fbb541e70fac7db669631958eab2e4e0d9c drm-intel-nightly: 2016y-10m-13d-15h-31m-19s UTC integration manifest
36d0d8d drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Yang, Libin]

Tested-by: Libin Yang <libin.yang@intel.com>

Regards,
Libin


> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Dhinakaran Pandiyan
> Sent: Friday, October 14, 2016 2:04 AM
> To: intel-gfx@freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Kp, Jeeja <jeeja.kp@intel.com>;
> Libin Yang <libin.yang@linux.intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>
> Subject: [Intel-gfx] [PATCH] drm/i915/dp: Increase cdclk when DP audio is
> enabled with 4 lanes and HBR2
> 
> According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> enabled, port width x4, and link rate HBR2 (5.4 GHz)
> 
> Having a lower cdclk triggers pipe underruns, which then lead to displays
> continuously cycling off and on. This is essential for DP MST audio as the link
> is trained at HBR2 and 4 lanes by default.
> 
> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47
> +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index cfcb03f..6a05183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	return 0;
>  }
> 
> +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state) {
> +
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	/* 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."
> +	 */
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> +			pipe_config->has_audio &&
> +			pipe_config->port_clock == 540000 &&
> +			pipe_config->lane_count == 4);
> +	}
> +
> +	return false;
> +}
> +
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)  {
>  	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk, min_cdclk = 0;
>  	struct intel_atomic_state *intel_state =
>  		to_intel_atomic_state(state);
> 
> -	intel_state->cdclk = intel_state->dev_cdclk =
> -		bxt_calc_cdclk(max_pixclk);
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = bxt_calc_cdclk(432000);
> +
> +	cdclk = bxt_calc_cdclk(max_pixclk);
> +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> 
>  	if (!intel_state->active_crtcs)
>  		intel_state->dev_cdclk = bxt_calc_cdclk(0); @@ -10374,7
> +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = broadwell_calc_cdclk(432000);
> 
>  	/*
>  	 * FIXME should also account for plane ratio @@ -10382,6 +10414,8
> @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = broadwell_calc_cdclk(max_pixclk);
> 
> +	cdclk =	max(min_cdclk, cdclk);
> +
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> (%d kHz)\n",
>  			      cdclk, dev_priv->max_cdclk_freq); @@ -10411,7
> +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state
> *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	const int max_pixclk = ilk_max_pixel_rate(state);
>  	int vco = intel_state->cdclk_pll_vco;
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = skl_calc_cdclk(432000, vco);
> 
>  	/*
>  	 * FIXME should also account for plane ratio @@ -10419,6 +10456,8
> @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> 
> +	cdclk = max(min_cdclk, cdclk);
> +
>  	/*
>  	 * FIXME move the cdclk caclulation to
>  	 * compute_config() so we can fail gracegully.
> --
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Jani Nikula]

On Thu, 13 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> enabled, port width x4, and link rate HBR2 (5.4 GHz)
>
> Having a lower cdclk triggers pipe underruns, which then lead to displays
> continuously cycling off and on. This is essential for DP MST audio as the
> link is trained at HBR2 and 4 lanes by default.
>
> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cfcb03f..6a05183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)

One drive-by comment, this name for the function would suggest it
returns the minimum cdclk for dp audio. As-is, the function name is
confusing.

BR,
Jani.

> +{
> +
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	/* 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."
> +	 */
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> +			pipe_config->has_audio &&
> +			pipe_config->port_clock == 540000 &&
> +			pipe_config->lane_count == 4);
> +	}
> +
> +	return false;
> +}
> +
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk, min_cdclk = 0;
>  	struct intel_atomic_state *intel_state =
>  		to_intel_atomic_state(state);
>  
> -	intel_state->cdclk = intel_state->dev_cdclk =
> -		bxt_calc_cdclk(max_pixclk);
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = bxt_calc_cdclk(432000);
> +
> +	cdclk = bxt_calc_cdclk(max_pixclk);
> +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
>  
>  	if (!intel_state->active_crtcs)
>  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = broadwell_calc_cdclk(432000);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = broadwell_calc_cdclk(max_pixclk);
>  
> +	cdclk =	max(min_cdclk, cdclk);
> +
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
>  			      cdclk, dev_priv->max_cdclk_freq);
> @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	const int max_pixclk = ilk_max_pixel_rate(state);
>  	int vco = intel_state->cdclk_pll_vco;
> -	int cdclk;
> +	int cdclk, min_cdclk = 0;
> +
> +	if (cdclk_min_for_dp_audio(state))
> +		min_cdclk = skl_calc_cdclk(432000, vco);
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(max_pixclk, vco);
>  
> +	cdclk = max(min_cdclk, cdclk);
> +
>  	/*
>  	 * FIXME move the cdclk caclulation to
>  	 * compute_config() so we can fail gracegully.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Thu, 2016-10-13 at 15:28 -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 11:04 -0700, Dhinakaran Pandiyan escreveu:
> > According to BSpec, cdclk has to be not less than 432 MHz with DP
> > audio
> > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> 
> This is just for pre-production hardware, and we don't implement
> workarounds for pre-prod.
> 
> 

I try to get a confirmation that is applicable to production HW too.

> A quick research seems to indicate that our CDCLK must be at least
> twice the frequency of the audio BCLK. Maybe this is what we need to
> investigate/check?
> 

From what I see in the description for AUD_FREQ_CNTRL, BCLK is either
48MHz or 96MHz. But we see this issue even with cdclk set to 337.5 MHz.

> Anyway, it looks like we're getting closer to the final bug fix.
> 
> > 
> > Having a lower cdclk triggers pipe underruns, which then lead to
> > displays
> > continuously cycling off and on. This is essential for DP MST audio
> > as the
> > link is trained at HBR2 and 4 lanes by default.
> > 
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> 
> Please use the "Bugzilla:" tag since there are some scripts and robots
> that parse it and do stuff with it.
> 
> 

Will do. 

> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 47
> > +++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int
> > valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > +{
> > +
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	/* 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."
> > +	 */
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +
> > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > +			pipe_config->has_audio &&
> > +			pipe_config->port_clock == 540000 &&
> > +			pipe_config->lane_count == 4);
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> > +	int cdclk, min_cdclk = 0;
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> >  
> > -	intel_state->cdclk = intel_state->dev_cdclk =
> > -		bxt_calc_cdclk(max_pixclk);
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = bxt_calc_cdclk(432000);
> > +
> > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk,
> > cdclk);
> >  
> >  	if (!intel_state->active_crtcs)
> >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > @@ -10374,7 +10403,10 @@ static int
> > broadwell_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 = ilk_max_pixel_rate(state);
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = broadwell_calc_cdclk(432000);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10382,6 +10414,8 @@ static int
> > broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> >  
> > +	cdclk =	max(min_cdclk, cdclk);
> > +
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> >  			      cdclk, dev_priv->max_cdclk_freq);
> > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	const int max_pixclk = ilk_max_pixel_rate(state);
> >  	int vco = intel_state->cdclk_pll_vco;
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = skl_calc_cdclk(432000, vco);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> >  
> > +	cdclk = max(min_cdclk, cdclk);
> > +
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> >  	 * compute_config() so we can fail gracegully.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Thu, 2016-10-13 at 11:30 -0700, Jim Bride wrote:
> On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > 
> > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > continuously cycling off and on. This is essential for DP MST audio as the
> > link is trained at HBR2 and 4 lanes by default.
> > 
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > +{
> > +
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	/* 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."
> > +	 */
> 
> It's probably worth mentioning that this limitation really only applies for
> SKL (at least that I can see) and BXT A (which I don't think we even care
> about anymore since it's not a production part.)  Were we seeing underruns
> on non-SKL platforms that increasing CDCLK fixed?   In any event, I'd rather
> see this only implemented for SKL (see BDW code below) unless we've empirically
> noticed a similar problem on BDW (or the B-Spec says something about this and I
> missed it.)
> 
> Jim
> 
> 
Yes, Libin saw the problem when testing a BDW NUC. And, the patch is
applicable to BDW according to the BSpec. I will try to get a
confirmation on whether this is needed BXT production parts.

> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +
> > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > +			pipe_config->has_audio &&
> > +			pipe_config->port_clock == 540000 &&
> > +			pipe_config->lane_count == 4);
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> > +	int cdclk, min_cdclk = 0;
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> >  
> > -	intel_state->cdclk = intel_state->dev_cdclk =
> > -		bxt_calc_cdclk(max_pixclk);
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = bxt_calc_cdclk(432000);
> > +
> > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> >  
> >  	if (!intel_state->active_crtcs)
> >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = broadwell_calc_cdclk(432000);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> >  
> > +	cdclk =	max(min_cdclk, cdclk);
> > +
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> >  			      cdclk, dev_priv->max_cdclk_freq);
> > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	const int max_pixclk = ilk_max_pixel_rate(state);
> >  	int vco = intel_state->cdclk_pll_vco;
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = skl_calc_cdclk(432000, vco);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> >  
> > +	cdclk = max(min_cdclk, cdclk);
> > +
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> >  	 * compute_config() so we can fail gracegully.
> > -- 
> > 2.7.4
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Fri, 2016-10-14 at 11:40 +0300, Jani Nikula wrote:
> On Thu, 13 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> >
> > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > continuously cycling off and on. This is essential for DP MST audio as the
> > link is trained at HBR2 and 4 lanes by default.
> >
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> 
> One drive-by comment, this name for the function would suggest it
> returns the minimum cdclk for dp audio. As-is, the function name is
> confusing.
> 

Agreed, I was running out of ideas and spent way too much time thinking
about the function name :) Any suggestions?

> BR,
> Jani.
> 
> > +{
> > +
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	/* 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."
> > +	 */
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +
> > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > +			pipe_config->has_audio &&
> > +			pipe_config->port_clock == 540000 &&
> > +			pipe_config->lane_count == 4);
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> > +	int cdclk, min_cdclk = 0;
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> >  
> > -	intel_state->cdclk = intel_state->dev_cdclk =
> > -		bxt_calc_cdclk(max_pixclk);
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = bxt_calc_cdclk(432000);
> > +
> > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> >  
> >  	if (!intel_state->active_crtcs)
> >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = broadwell_calc_cdclk(432000);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> >  
> > +	cdclk =	max(min_cdclk, cdclk);
> > +
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> >  			      cdclk, dev_priv->max_cdclk_freq);
> > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	const int max_pixclk = ilk_max_pixel_rate(state);
> >  	int vco = intel_state->cdclk_pll_vco;
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = skl_calc_cdclk(432000, vco);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> >  
> > +	cdclk = max(min_cdclk, cdclk);
> > +
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> >  	 * compute_config() so we can fail gracegully.
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Thu, 2016-10-13 at 21:44 +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > 
> > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > continuously cycling off and on. This is essential for DP MST audio as the
> > link is trained at HBR2 and 4 lanes by default.
> > 
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > +{
> > +
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	/* 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."
> > +	 */
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +
> > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > +			pipe_config->has_audio &&
> > +			pipe_config->port_clock == 540000 &&
> > +			pipe_config->lane_count == 4);
> > +	}
> 
> That's not good enough on account of crtcs not part of the state
> potentially needing the workaround as well. However, since we only do
> this when there's a modeset, I think we'll be covered by the
> connection_mutex, and so we should be able to peek at the current state
> of all crtcs without grabbing the corresponding crtc locks.
> 

Please correct me if I am wrong. Won't the first modeset that has all
the conditions met (DP + HBR2 + 4 lanes + audio) include the crtc
driving the display which triggered the modeset?

Since, the new cdclk freq that will be set is common for all the crtcs,
we don't need the workaround for crtcs that are not in state. 

> So I think we'd be OK with something like:
> 
> WARN_ON(!locked(connection_mutex));
> 
> for_each_intel_crtc() {
> 	/*
> 	 * Peeking at the current state is safe since
> 	 * we can only get here while holding the
> 	 * connection_mutex.
> 	 */
> 	crtc_state = intel_get_existing_crtc_state();
> 	if (!crtc_state)
> 		crtc_state = to_intel_crtc_state(crtc->base.state);
> 
> 	...
> }
> 
> The other option would be to track the min cdclk for each pipe in the
> top level state I suppose. We already do that for the pixel rate
> actually so that we can calculate the cdclk to begin with. Hmm. Maybe
> we should just switch to tracking the min cdclk per pipe instead of the
> pixel rate. Or did we need to the pixel rate itself for something else,
> Maarten?
> 
> Or we could perhaps replace the pixel rate/pixclk tracking with the peek
> approach entirely. Not quite sure. Would have to read the entire thing
> through.
> 

I thought of this, but the work around applies for only three platforms
(potentially just two) as of now. Does it warrant a driver wide change?
I have to check if mincdclk is useful elsewhere.

> > +
> > +	return false;
> > +}
> > +
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> > +	int cdclk, min_cdclk = 0;
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> >  
> > -	intel_state->cdclk = intel_state->dev_cdclk =
> > -		bxt_calc_cdclk(max_pixclk);
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = bxt_calc_cdclk(432000);
> > +
> > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> >  
> >  	if (!intel_state->active_crtcs)
> >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = broadwell_calc_cdclk(432000);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> >  
> > +	cdclk =	max(min_cdclk, cdclk);
> > +
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> >  			      cdclk, dev_priv->max_cdclk_freq);
> > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	const int max_pixclk = ilk_max_pixel_rate(state);
> >  	int vco = intel_state->cdclk_pll_vco;
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = skl_calc_cdclk(432000, vco);
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> >  
> > +	cdclk = max(min_cdclk, cdclk);
> > +
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> >  	 * compute_config() so we can fail gracegully.
> > -- 
> > 2.7.4
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Fri, 2016-10-14 at 03:09 +0000, Yang, Libin wrote:
> Tested-by: Libin Yang <libin.yang@intel.com>
> 
> Regards,
> Libin
> 
> 

Thanks Libin. Can you confirm the max. BCLK frequency we set for the
audio controller?


-DK
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > Dhinakaran Pandiyan
> > Sent: Friday, October 14, 2016 2:04 AM
> > To: intel-gfx@freedesktop.org
> > Cc: Nikula, Jani <jani.nikula@intel.com>; Kp, Jeeja <jeeja.kp@intel.com>;
> > Libin Yang <libin.yang@linux.intel.com>; Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com>
> > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Increase cdclk when DP audio is
> > enabled with 4 lanes and HBR2
> > 
> > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > 
> > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > continuously cycling off and on. This is essential for DP MST audio as the link
> > is trained at HBR2 and 4 lanes by default.
> > 
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 47
> > +++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	return 0;
> >  }
> > 
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state) {
> > +
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	/* 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."
> > +	 */
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +
> > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > +			pipe_config->has_audio &&
> > +			pipe_config->port_clock == 540000 &&
> > +			pipe_config->lane_count == 4);
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)  {
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> > +	int cdclk, min_cdclk = 0;
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> > 
> > -	intel_state->cdclk = intel_state->dev_cdclk =
> > -		bxt_calc_cdclk(max_pixclk);
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = bxt_calc_cdclk(432000);
> > +
> > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> > 
> >  	if (!intel_state->active_crtcs)
> >  		intel_state->dev_cdclk = bxt_calc_cdclk(0); @@ -10374,7
> > +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = broadwell_calc_cdclk(432000);
> > 
> >  	/*
> >  	 * FIXME should also account for plane ratio @@ -10382,6 +10414,8
> > @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > 
> > +	cdclk =	max(min_cdclk, cdclk);
> > +
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> >  			      cdclk, dev_priv->max_cdclk_freq); @@ -10411,7
> > +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state
> > *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	const int max_pixclk = ilk_max_pixel_rate(state);
> >  	int vco = intel_state->cdclk_pll_vco;
> > -	int cdclk;
> > +	int cdclk, min_cdclk = 0;
> > +
> > +	if (cdclk_min_for_dp_audio(state))
> > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > 
> >  	/*
> >  	 * FIXME should also account for plane ratio @@ -10419,6 +10456,8
> > @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > 
> > +	cdclk = max(min_cdclk, cdclk);
> > +
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> >  	 * compute_config() so we can fail gracegully.
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Yang, Libin]

> -----Original Message-----
> From: Pandiyan, Dhinakaran
> Sent: Saturday, October 15, 2016 4:36 AM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: intel-gfx@freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; Kp, Jeeja
> <jeeja.kp@intel.com>; libin.yang@linux.intel.com
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dp: Increase cdclk when DP audio is
> enabled with 4 lanes and HBR2
> 
> On Fri, 2016-10-14 at 03:09 +0000, Yang, Libin wrote:
> > Tested-by: Libin Yang <libin.yang@intel.com>
> >
> > Regards,
> > Libin
> >
> >
> 
> Thanks Libin. Can you confirm the max. BCLK frequency we set for the audio
> controller?

Currently, we don't support HBR audio. We only test 48KHz, 44.1KHz and 32KHz.

For the BDW/HSW, audio requires that gfx driver should notify audio driver 
that cdclk is changed. Currently , eld notify will notify the audio the change
of cdclk.

I will ask our QA to do a full test on 48KHz, 44.1KHz and 32KHz.

Regards,
Libin

> 
> 
> -DK
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> > > Behalf Of Dhinakaran Pandiyan
> > > Sent: Friday, October 14, 2016 2:04 AM
> > > To: intel-gfx@freedesktop.org
> > > Cc: Nikula, Jani <jani.nikula@intel.com>; Kp, Jeeja
> > > <jeeja.kp@intel.com>; Libin Yang <libin.yang@linux.intel.com>;
> > > Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> > > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Increase cdclk when DP
> > > audio is enabled with 4 lanes and HBR2
> > >
> > > According to BSpec, cdclk has to be not less than 432 MHz with DP
> > > audio enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > >
> > > Having a lower cdclk triggers pipe underruns, which then lead to
> > > displays continuously cycling off and on. This is essential for DP
> > > MST audio as the link is trained at HBR2 and 4 lanes by default.
> > >
> > > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > >
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 47
> > > +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index cfcb03f..6a05183 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6603,14 +6603,43 @@ static int
> > > valleyview_modeset_calc_cdclk(struct
> > > drm_atomic_state *state)
> > >  	return 0;
> > >  }
> > >
> > > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > > +{
> > > +
> > > +	struct drm_crtc_state *crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > > +
> > > +	/* 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."
> > > +	 */
> > > +
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		struct intel_crtc_state *pipe_config =
> > > +			to_intel_crtc_state(crtc_state);
> > > +
> > > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > > +			pipe_config->has_audio &&
> > > +			pipe_config->port_clock == 540000 &&
> > > +			pipe_config->lane_count == 4);
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)  {
> > >  	int max_pixclk = ilk_max_pixel_rate(state);
> > > +	int cdclk, min_cdclk = 0;
> > >  	struct intel_atomic_state *intel_state =
> > >  		to_intel_atomic_state(state);
> > >
> > > -	intel_state->cdclk = intel_state->dev_cdclk =
> > > -		bxt_calc_cdclk(max_pixclk);
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = bxt_calc_cdclk(432000);
> > > +
> > > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk,
> > > +cdclk);
> > >
> > >  	if (!intel_state->active_crtcs)
> > >  		intel_state->dev_cdclk = bxt_calc_cdclk(0); @@ -10374,7
> > > +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = broadwell_calc_cdclk(432000);
> > >
> > >  	/*
> > >  	 * FIXME should also account for plane ratio @@ -10382,6 +10414,8
> > > @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state
> *state)
> > >  	 */
> > >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > >
> > > +	cdclk =	max(min_cdclk, cdclk);
> > > +
> > >  	if (cdclk > dev_priv->max_cdclk_freq) {
> > >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> (%d kHz)\n",
> > >  			      cdclk, dev_priv->max_cdclk_freq); @@ -10411,7
> > > +10445,10 @@ static int skl_modeset_calc_cdclk(struct
> > > +drm_atomic_state
> > > *state)
> > >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > >  	const int max_pixclk = ilk_max_pixel_rate(state);
> > >  	int vco = intel_state->cdclk_pll_vco;
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > >
> > >  	/*
> > >  	 * FIXME should also account for plane ratio @@ -10419,6 +10456,8
> > > @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	 */
> > >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > >
> > > +	cdclk = max(min_cdclk, cdclk);
> > > +
> > >  	/*
> > >  	 * FIXME move the cdclk caclulation to
> > >  	 * compute_config() so we can fail gracegully.
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Daniel Vetter]

On Fri, Oct 14, 2016 at 07:27:39PM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-10-13 at 11:30 -0700, Jim Bride wrote:
> > On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > > 
> > > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > > continuously cycling off and on. This is essential for DP MST audio as the
> > > link is trained at HBR2 and 4 lanes by default.
> > > 
> > > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index cfcb03f..6a05183 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > > +{
> > > +
> > > +	struct drm_crtc_state *crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > > +
> > > +	/* 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."
> > > +	 */
> > 
> > It's probably worth mentioning that this limitation really only applies for
> > SKL (at least that I can see) and BXT A (which I don't think we even care
> > about anymore since it's not a production part.)  Were we seeing underruns
> > on non-SKL platforms that increasing CDCLK fixed?   In any event, I'd rather
> > see this only implemented for SKL (see BDW code below) unless we've empirically
> > noticed a similar problem on BDW (or the B-Spec says something about this and I
> > missed it.)
> > 
> > Jim
> > 
> > 
> Yes, Libin saw the problem when testing a BDW NUC. And, the patch is
> applicable to BDW according to the BSpec. I will try to get a
> confirmation on whether this is needed BXT production parts.

Please also check docs for hsw, that's where dp mst support started.
-Daniel

> 
> > > +
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		struct intel_crtc_state *pipe_config =
> > > +			to_intel_crtc_state(crtc_state);
> > > +
> > > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > > +			pipe_config->has_audio &&
> > > +			pipe_config->port_clock == 540000 &&
> > > +			pipe_config->lane_count == 4);
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  {
> > >  	int max_pixclk = ilk_max_pixel_rate(state);
> > > +	int cdclk, min_cdclk = 0;
> > >  	struct intel_atomic_state *intel_state =
> > >  		to_intel_atomic_state(state);
> > >  
> > > -	intel_state->cdclk = intel_state->dev_cdclk =
> > > -		bxt_calc_cdclk(max_pixclk);
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = bxt_calc_cdclk(432000);
> > > +
> > > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> > >  
> > >  	if (!intel_state->active_crtcs)
> > >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = broadwell_calc_cdclk(432000);
> > >  
> > >  	/*
> > >  	 * FIXME should also account for plane ratio
> > > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	 */
> > >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > >  
> > > +	cdclk =	max(min_cdclk, cdclk);
> > > +
> > >  	if (cdclk > dev_priv->max_cdclk_freq) {
> > >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > >  			      cdclk, dev_priv->max_cdclk_freq);
> > > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > >  	const int max_pixclk = ilk_max_pixel_rate(state);
> > >  	int vco = intel_state->cdclk_pll_vco;
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > >  
> > >  	/*
> > >  	 * FIXME should also account for plane ratio
> > > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	 */
> > >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > >  
> > > +	cdclk = max(min_cdclk, cdclk);
> > > +
> > >  	/*
> > >  	 * FIXME move the cdclk caclulation to
> > >  	 * compute_config() so we can fail gracegully.
> > > -- 
> > > 2.7.4
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Ville Syrjälä]

On Fri, Oct 14, 2016 at 08:33:37PM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-10-13 at 21:44 +0300, Ville Syrjälä wrote:
> > On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > > 
> > > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > > continuously cycling off and on. This is essential for DP MST audio as the
> > > link is trained at HBR2 and 4 lanes by default.
> > > 
> > > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index cfcb03f..6a05183 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > > +{
> > > +
> > > +	struct drm_crtc_state *crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > > +
> > > +	/* 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."
> > > +	 */
> > > +
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		struct intel_crtc_state *pipe_config =
> > > +			to_intel_crtc_state(crtc_state);
> > > +
> > > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > > +			pipe_config->has_audio &&
> > > +			pipe_config->port_clock == 540000 &&
> > > +			pipe_config->lane_count == 4);
> > > +	}
> > 
> > That's not good enough on account of crtcs not part of the state
> > potentially needing the workaround as well. However, since we only do
> > this when there's a modeset, I think we'll be covered by the
> > connection_mutex, and so we should be able to peek at the current state
> > of all crtcs without grabbing the corresponding crtc locks.
> > 
> 
> Please correct me if I am wrong. Won't the first modeset that has all
> the conditions met (DP + HBR2 + 4 lanes + audio) include the crtc
> driving the display which triggered the modeset?
> 
> Since, the new cdclk freq that will be set is common for all the crtcs,
> we don't need the workaround for crtcs that are not in state. 

There can be another modeset afterwards that doesn't need the w/a and
that would then end up reducing cdclk below the required frequency.

> 
> > So I think we'd be OK with something like:
> > 
> > WARN_ON(!locked(connection_mutex));
> > 
> > for_each_intel_crtc() {
> > 	/*
> > 	 * Peeking at the current state is safe since
> > 	 * we can only get here while holding the
> > 	 * connection_mutex.
> > 	 */
> > 	crtc_state = intel_get_existing_crtc_state();
> > 	if (!crtc_state)
> > 		crtc_state = to_intel_crtc_state(crtc->base.state);
> > 
> > 	...
> > }
> > 
> > The other option would be to track the min cdclk for each pipe in the
> > top level state I suppose. We already do that for the pixel rate
> > actually so that we can calculate the cdclk to begin with. Hmm. Maybe
> > we should just switch to tracking the min cdclk per pipe instead of the
> > pixel rate. Or did we need to the pixel rate itself for something else,
> > Maarten?
> > 
> > Or we could perhaps replace the pixel rate/pixclk tracking with the peek
> > approach entirely. Not quite sure. Would have to read the entire thing
> > through.
> > 
> 
> I thought of this, but the work around applies for only three platforms
> (potentially just two) as of now. Does it warrant a driver wide change?
> I have to check if mincdclk is useful elsewhere.

We need to do one of these options. No way around it if we need this
w/a. Though I guess it's a little bit of an open question at the moment
since on SKL it only supposedly applies up to D stepping which we don't
care about. On BDW it seems to be for everything though.

> 
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  {
> > >  	int max_pixclk = ilk_max_pixel_rate(state);
> > > +	int cdclk, min_cdclk = 0;
> > >  	struct intel_atomic_state *intel_state =
> > >  		to_intel_atomic_state(state);
> > >  
> > > -	intel_state->cdclk = intel_state->dev_cdclk =
> > > -		bxt_calc_cdclk(max_pixclk);
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = bxt_calc_cdclk(432000);
> > > +
> > > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> > >  
> > >  	if (!intel_state->active_crtcs)
> > >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = broadwell_calc_cdclk(432000);
> > >  
> > >  	/*
> > >  	 * FIXME should also account for plane ratio
> > > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	 */
> > >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > >  
> > > +	cdclk =	max(min_cdclk, cdclk);
> > > +
> > >  	if (cdclk > dev_priv->max_cdclk_freq) {
> > >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > >  			      cdclk, dev_priv->max_cdclk_freq);
> > > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > >  	const int max_pixclk = ilk_max_pixel_rate(state);
> > >  	int vco = intel_state->cdclk_pll_vco;
> > > -	int cdclk;
> > > +	int cdclk, min_cdclk = 0;
> > > +
> > > +	if (cdclk_min_for_dp_audio(state))
> > > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > >  
> > >  	/*
> > >  	 * FIXME should also account for plane ratio
> > > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	 */
> > >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > >  
> > > +	cdclk = max(min_cdclk, cdclk);
> > > +
> > >  	/*
> > >  	 * FIXME move the cdclk caclulation to
> > >  	 * compute_config() so we can fail gracegully.
> > > -- 
> > > 2.7.4
> > 
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 08:55:51AM +0200, Daniel Vetter wrote:
> On Fri, Oct 14, 2016 at 07:27:39PM +0000, Pandiyan, Dhinakaran wrote:
> > On Thu, 2016-10-13 at 11:30 -0700, Jim Bride wrote:
> > > On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > > > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > > > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > > > 
> > > > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > > > continuously cycling off and on. This is essential for DP MST audio as the
> > > > link is trained at HBR2 and 4 lanes by default.
> > > > 
> > > > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index cfcb03f..6a05183 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > > > +{
> > > > +
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	struct drm_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	/* 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."
> > > > +	 */
> > > 
> > > It's probably worth mentioning that this limitation really only applies for
> > > SKL (at least that I can see) and BXT A (which I don't think we even care
> > > about anymore since it's not a production part.)  Were we seeing underruns
> > > on non-SKL platforms that increasing CDCLK fixed?   In any event, I'd rather
> > > see this only implemented for SKL (see BDW code below) unless we've empirically
> > > noticed a similar problem on BDW (or the B-Spec says something about this and I
> > > missed it.)
> > > 
> > > Jim
> > > 
> > > 
> > Yes, Libin saw the problem when testing a BDW NUC. And, the patch is
> > applicable to BDW according to the BSpec. I will try to get a
> > confirmation on whether this is needed BXT production parts.
> 
> Please also check docs for hsw, that's where dp mst support started.

s/mst/hbr2/, otherwise we've veering off topic.

Not applicable to HSW, at least directly, since we don't support changing
cdclk there. Only HSW-ULX has cdclk below this limit, and that can't do
HBR2 anyway. But it's best to double check anyway, in case there are
other related limits on HSW.

> -Daniel
> 
> > 
> > > > +
> > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > +		struct intel_crtc_state *pipe_config =
> > > > +			to_intel_crtc_state(crtc_state);
> > > > +
> > > > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > > > +			pipe_config->has_audio &&
> > > > +			pipe_config->port_clock == 540000 &&
> > > > +			pipe_config->lane_count == 4);
> > > > +	}
> > > > +
> > > > +	return false;
> > > > +}
> > > > +
> > > >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  {
> > > >  	int max_pixclk = ilk_max_pixel_rate(state);
> > > > +	int cdclk, min_cdclk = 0;
> > > >  	struct intel_atomic_state *intel_state =
> > > >  		to_intel_atomic_state(state);
> > > >  
> > > > -	intel_state->cdclk = intel_state->dev_cdclk =
> > > > -		bxt_calc_cdclk(max_pixclk);
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = bxt_calc_cdclk(432000);
> > > > +
> > > > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > > > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> > > >  
> > > >  	if (!intel_state->active_crtcs)
> > > >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > > > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > > > -	int cdclk;
> > > > +	int cdclk, min_cdclk = 0;
> > > > +
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = broadwell_calc_cdclk(432000);
> > > >  
> > > >  	/*
> > > >  	 * FIXME should also account for plane ratio
> > > > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	 */
> > > >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > > >  
> > > > +	cdclk =	max(min_cdclk, cdclk);
> > > > +
> > > >  	if (cdclk > dev_priv->max_cdclk_freq) {
> > > >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > >  			      cdclk, dev_priv->max_cdclk_freq);
> > > > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > >  	const int max_pixclk = ilk_max_pixel_rate(state);
> > > >  	int vco = intel_state->cdclk_pll_vco;
> > > > -	int cdclk;
> > > > +	int cdclk, min_cdclk = 0;
> > > > +
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > > >  
> > > >  	/*
> > > >  	 * FIXME should also account for plane ratio
> > > > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	 */
> > > >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > > >  
> > > > +	cdclk = max(min_cdclk, cdclk);
> > > > +
> > > >  	/*
> > > >  	 * FIXME move the cdclk caclulation to
> > > >  	 * compute_config() so we can fail gracegully.
> > > > -- 
> > > > 2.7.4
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Increase cdclk when DP audio is enabled with 4 lanes and HBR2

[Pandiyan, Dhinakaran]

On Mon, 2016-10-17 at 11:33 +0300, Ville Syrjälä wrote:
> On Fri, Oct 14, 2016 at 08:33:37PM +0000, Pandiyan, Dhinakaran wrote:
> > On Thu, 2016-10-13 at 21:44 +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 13, 2016 at 11:04:19AM -0700, Dhinakaran Pandiyan wrote:
> > > > According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > > > enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > > > 
> > > > Having a lower cdclk triggers pipe underruns, which then lead to displays
> > > > continuously cycling off and on. This is essential for DP MST audio as the
> > > > link is trained at HBR2 and 4 lanes by default.
> > > > 
> > > > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index cfcb03f..6a05183 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > > > +{
> > > > +
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	struct drm_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	/* 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."
> > > > +	 */
> > > > +
> > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > +		struct intel_crtc_state *pipe_config =
> > > > +			to_intel_crtc_state(crtc_state);
> > > > +
> > > > +		return (intel_crtc_has_dp_encoder(pipe_config) &&
> > > > +			pipe_config->has_audio &&
> > > > +			pipe_config->port_clock == 540000 &&
> > > > +			pipe_config->lane_count == 4);
> > > > +	}
> > > 
> > > That's not good enough on account of crtcs not part of the state
> > > potentially needing the workaround as well. However, since we only do
> > > this when there's a modeset, I think we'll be covered by the
> > > connection_mutex, and so we should be able to peek at the current state
> > > of all crtcs without grabbing the corresponding crtc locks.
> > > 
> > 
> > Please correct me if I am wrong. Won't the first modeset that has all
> > the conditions met (DP + HBR2 + 4 lanes + audio) include the crtc
> > driving the display which triggered the modeset?
> > 
> > Since, the new cdclk freq that will be set is common for all the crtcs,
> > we don't need the workaround for crtcs that are not in state. 
> 
> There can be another modeset afterwards that doesn't need the w/a and
> that would then end up reducing cdclk below the required frequency.
> 

Got it, thanks for the explanation.


> > 
> > > So I think we'd be OK with something like:
> > > 
> > > WARN_ON(!locked(connection_mutex));
> > > 
> > > for_each_intel_crtc() {
> > > 	/*
> > > 	 * Peeking at the current state is safe since
> > > 	 * we can only get here while holding the
> > > 	 * connection_mutex.
> > > 	 */
> > > 	crtc_state = intel_get_existing_crtc_state();
> > > 	if (!crtc_state)
> > > 		crtc_state = to_intel_crtc_state(crtc->base.state);
> > > 
> > > 	...
> > > }
> > > 
> > > The other option would be to track the min cdclk for each pipe in the
> > > top level state I suppose. We already do that for the pixel rate
> > > actually so that we can calculate the cdclk to begin with. Hmm. Maybe
> > > we should just switch to tracking the min cdclk per pipe instead of the
> > > pixel rate. Or did we need to the pixel rate itself for something else,
> > > Maarten?
> > > 
> > > Or we could perhaps replace the pixel rate/pixclk tracking with the peek
> > > approach entirely. Not quite sure. Would have to read the entire thing
> > > through.
> > > 
> > 
> > I thought of this, but the work around applies for only three platforms
> > (potentially just two) as of now. Does it warrant a driver wide change?
> > I have to check if mincdclk is useful elsewhere.
> 
> We need to do one of these options. No way around it if we need this
> w/a. Though I guess it's a little bit of an open question at the moment
> since on SKL it only supposedly applies up to D stepping which we don't
> care about. On BDW it seems to be for everything though.
> 

Yeah, you are right. 

I have a different workaround for SKL that does not involve cdclk.
Setting CHICKEN_TRANS_x bit 13 fixes the underruns. However, for BDW we
still have to set cdclk minimum to 432 MHz.

Tracking the min cdclk per pipe sounds cleaner than peeking at crtc
states that are not part of the modeset. 


> > 
> > > > +
> > > > +	return false;
> > > > +}
> > > > +
> > > >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  {
> > > >  	int max_pixclk = ilk_max_pixel_rate(state);
> > > > +	int cdclk, min_cdclk = 0;
> > > >  	struct intel_atomic_state *intel_state =
> > > >  		to_intel_atomic_state(state);
> > > >  
> > > > -	intel_state->cdclk = intel_state->dev_cdclk =
> > > > -		bxt_calc_cdclk(max_pixclk);
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = bxt_calc_cdclk(432000);
> > > > +
> > > > +	cdclk = bxt_calc_cdclk(max_pixclk);
> > > > +	intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> > > >  
> > > >  	if (!intel_state->active_crtcs)
> > > >  		intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > > > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > > > -	int cdclk;
> > > > +	int cdclk, min_cdclk = 0;
> > > > +
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = broadwell_calc_cdclk(432000);
> > > >  
> > > >  	/*
> > > >  	 * FIXME should also account for plane ratio
> > > > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	 */
> > > >  	cdclk = broadwell_calc_cdclk(max_pixclk);
> > > >  
> > > > +	cdclk =	max(min_cdclk, cdclk);
> > > > +
> > > >  	if (cdclk > dev_priv->max_cdclk_freq) {
> > > >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > >  			      cdclk, dev_priv->max_cdclk_freq);
> > > > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > >  	const int max_pixclk = ilk_max_pixel_rate(state);
> > > >  	int vco = intel_state->cdclk_pll_vco;
> > > > -	int cdclk;
> > > > +	int cdclk, min_cdclk = 0;
> > > > +
> > > > +	if (cdclk_min_for_dp_audio(state))
> > > > +		min_cdclk = skl_calc_cdclk(432000, vco);
> > > >  
> > > >  	/*
> > > >  	 * FIXME should also account for plane ratio
> > > > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	 */
> > > >  	cdclk = skl_calc_cdclk(max_pixclk, vco);
> > > >  
> > > > +	cdclk = max(min_cdclk, cdclk);
> > > > +
> > > >  	/*
> > > >  	 * FIXME move the cdclk caclulation to
> > > >  	 * compute_config() so we can fail gracegully.
> > > > -- 
> > > > 2.7.4
> > > 
> > 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx