[RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Maarten Lankhorst]

When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.

Right now this is not handled well in i915, and it calculates the crtc needs to
be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
active. Fix this by exposing SPLL as a shared pll that will not be returned
by intel_get_shared_dpll; you have to know it exists to use it. ;-)

Cc: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
RFC because I haven't tested it with VGA, but it seems to work according to fix
the problem mentioned above.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 81adf89b92f1..cacdac67d9ba 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -345,6 +345,8 @@ enum intel_dpll_id {
 	/* hsw/bdw */
 	DPLL_ID_WRPLL1 = 0,
 	DPLL_ID_WRPLL2 = 1,
+	DPLL_ID_SPLL = 2,
+
 	/* skl */
 	DPLL_ID_SKL_DPLL1 = 0,
 	DPLL_ID_SKL_DPLL2 = 1,
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43bef4a4..592d8fe9f991 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
 	pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder);
 }
 
-static void hsw_crt_pre_enable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n");
-	I915_WRITE(SPLL_CTL,
-		   SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC);
-	POSTING_READ(SPLL_CTL);
-	udelay(20);
-}
-
 /* Note: The caller is required to filter out dpms modes not supported by the
  * platform. */
 static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode)
@@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
 	intel_disable_crt(encoder);
 }
 
-static void hsw_crt_post_disable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t val;
-
-	DRM_DEBUG_KMS("Disabling SPLL\n");
-	val = I915_READ(SPLL_CTL);
-	WARN_ON(!(val & SPLL_PLL_ENABLE));
-	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
-	POSTING_READ(SPLL_CTL);
-}
-
 static void intel_enable_crt(struct intel_encoder *encoder)
 {
 	struct intel_crt *crt = intel_encoder_to_crt(encoder);
@@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
 	if (HAS_DDI(dev)) {
 		pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
 		pipe_config->port_clock = 135000 * 2;
+		pipe_config->dpll_hw_state.wrpll =
+			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
 	}
 
 	return true;
@@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev)
 	if (HAS_DDI(dev)) {
 		crt->base.get_config = hsw_crt_get_config;
 		crt->base.get_hw_state = intel_ddi_get_hw_state;
-		crt->base.pre_enable = hsw_crt_pre_enable;
-		crt->base.post_disable = hsw_crt_post_disable;
 	} else {
 		crt->base.get_config = intel_crt_get_config;
 		crt->base.get_hw_state = intel_crt_get_hw_state;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 61575f67a626..dabd903147fa 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 		}
 
 		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
+	} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) {
+		struct drm_atomic_state *state = crtc_state->base.state;
+		struct intel_shared_dpll_config *spll =
+			&intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL];
+
+		if (spll->crtc_mask &&
+		    WARN_ON(spll->hw_state.wrpll != crtc_state->dpll_hw_state.wrpll))
+			return false;
+
+		crtc_state->shared_dpll = DPLL_ID_SPLL;
+		spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll;
+		spll->crtc_mask |= 1 << intel_crtc->pipe;
 	}
 
 	return true;
@@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
-	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
-	POSTING_READ(WRPLL_CTL(pll->id));
+	uint32_t reg;
+
+	if (pll->id == DPLL_ID_SPLL)
+		reg = SPLL_CTL;
+	else
+		reg = WRPLL_CTL(pll->id);
+
+	I915_WRITE(reg, pll->config.hw_state.wrpll);
+	POSTING_READ(reg);
 	udelay(20);
 }
 
 static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
-	uint32_t val;
+	uint32_t reg, val;
 
-	val = I915_READ(WRPLL_CTL(pll->id));
-	I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE);
-	POSTING_READ(WRPLL_CTL(pll->id));
+	if (pll->id == DPLL_ID_SPLL)
+		reg = SPLL_CTL;
+	else
+		reg = WRPLL_CTL(pll->id);
+
+	val = I915_READ(reg);
+	I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE);
+	POSTING_READ(reg);
 }
 
 static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
@@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
-	val = I915_READ(WRPLL_CTL(pll->id));
-	hw_state->wrpll = val;
+	if (pll->id == DPLL_ID_SPLL)
+		val = I915_READ(SPLL_CTL);
+	else
+		val = I915_READ(WRPLL_CTL(pll->id));
 
+	hw_state->wrpll = val;
 	return val & WRPLL_PLL_ENABLE;
 }
 
 static const char * const hsw_ddi_pll_names[] = {
 	"WRPLL 1",
 	"WRPLL 2",
+	"SPLL"
 };
 
 static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
 {
 	int i;
 
-	dev_priv->num_shared_dpll = 2;
+	dev_priv->num_shared_dpll = 3;
 
+	/* SPLL is special, but needs to be initialized anyway.. */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		dev_priv->shared_dplls[i].id = i;
 		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca9278be49f7..582360d2fa08 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	struct intel_shared_dpll *pll;
 	struct intel_shared_dpll_config *shared_dpll;
 	enum intel_dpll_id i;
+	int max = dev_priv->num_shared_dpll;
 
 	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
 
@@ -4242,9 +4243,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
-	}
+	} else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
+		/* Do not consider SPLL */
+		max = 2;
 
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+	for (i = 0; i < max; i++) {
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
@@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
 	case PORT_CLK_SEL_WRPLL2:
 		pipe_config->shared_dpll = DPLL_ID_WRPLL2;
 		break;
+	case PORT_CLK_SEL_SPLL:
+		pipe_config->shared_dpll = DPLL_ID_SPLL;
 	}
 }
 

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

Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Daniel Vetter]

On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
> 
> Right now this is not handled well in i915, and it calculates the crtc needs to
> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
> active. Fix this by exposing SPLL as a shared pll that will not be returned
> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
> 
> Cc: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> RFC because I haven't tested it with VGA, but it seems to work according to fix
> the problem mentioned above.

lgtm and gets rid of some of the fdi vs. normal ddi special-casing, which
is nice (since that's a problem for bxt dsi too). I'll pick this up as
soon as you managed to run this on some hsw (since we inject a fake vga
screen for igt tests you don't even need a real screen).
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 81adf89b92f1..cacdac67d9ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -345,6 +345,8 @@ enum intel_dpll_id {
>  	/* hsw/bdw */
>  	DPLL_ID_WRPLL1 = 0,
>  	DPLL_ID_WRPLL2 = 1,
> +	DPLL_ID_SPLL = 2,
> +
>  	/* skl */
>  	DPLL_ID_SKL_DPLL1 = 0,
>  	DPLL_ID_SKL_DPLL2 = 1,
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43bef4a4..592d8fe9f991 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
>  	pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder);
>  }
>  
> -static void hsw_crt_pre_enable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n");
> -	I915_WRITE(SPLL_CTL,
> -		   SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC);
> -	POSTING_READ(SPLL_CTL);
> -	udelay(20);
> -}
> -
>  /* Note: The caller is required to filter out dpms modes not supported by the
>   * platform. */
>  static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode)
> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
>  	intel_disable_crt(encoder);
>  }
>  
> -static void hsw_crt_post_disable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t val;
> -
> -	DRM_DEBUG_KMS("Disabling SPLL\n");
> -	val = I915_READ(SPLL_CTL);
> -	WARN_ON(!(val & SPLL_PLL_ENABLE));
> -	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
> -	POSTING_READ(SPLL_CTL);
> -}
> -
>  static void intel_enable_crt(struct intel_encoder *encoder)
>  {
>  	struct intel_crt *crt = intel_encoder_to_crt(encoder);
> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>  	if (HAS_DDI(dev)) {
>  		pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>  		pipe_config->port_clock = 135000 * 2;
> +		pipe_config->dpll_hw_state.wrpll =
> +			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
>  	}
>  
>  	return true;
> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev)
>  	if (HAS_DDI(dev)) {
>  		crt->base.get_config = hsw_crt_get_config;
>  		crt->base.get_hw_state = intel_ddi_get_hw_state;
> -		crt->base.pre_enable = hsw_crt_pre_enable;
> -		crt->base.post_disable = hsw_crt_post_disable;
>  	} else {
>  		crt->base.get_config = intel_crt_get_config;
>  		crt->base.get_hw_state = intel_crt_get_hw_state;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 61575f67a626..dabd903147fa 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		}
>  
>  		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
> +	} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) {
> +		struct drm_atomic_state *state = crtc_state->base.state;
> +		struct intel_shared_dpll_config *spll =
> +			&intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL];
> +
> +		if (spll->crtc_mask &&
> +		    WARN_ON(spll->hw_state.wrpll != crtc_state->dpll_hw_state.wrpll))
> +			return false;
> +
> +		crtc_state->shared_dpll = DPLL_ID_SPLL;
> +		spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll;
> +		spll->crtc_mask |= 1 << intel_crtc->pipe;
>  	}
>  
>  	return true;
> @@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
> -	POSTING_READ(WRPLL_CTL(pll->id));
> +	uint32_t reg;
> +
> +	if (pll->id == DPLL_ID_SPLL)
> +		reg = SPLL_CTL;
> +	else
> +		reg = WRPLL_CTL(pll->id);
> +
> +	I915_WRITE(reg, pll->config.hw_state.wrpll);
> +	POSTING_READ(reg);
>  	udelay(20);
>  }
>  
>  static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
> -	uint32_t val;
> +	uint32_t reg, val;
>  
> -	val = I915_READ(WRPLL_CTL(pll->id));
> -	I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE);
> -	POSTING_READ(WRPLL_CTL(pll->id));
> +	if (pll->id == DPLL_ID_SPLL)
> +		reg = SPLL_CTL;
> +	else
> +		reg = WRPLL_CTL(pll->id);
> +
> +	val = I915_READ(reg);
> +	I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE);
> +	POSTING_READ(reg);
>  }
>  
>  static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
>  		return false;
>  
> -	val = I915_READ(WRPLL_CTL(pll->id));
> -	hw_state->wrpll = val;
> +	if (pll->id == DPLL_ID_SPLL)
> +		val = I915_READ(SPLL_CTL);
> +	else
> +		val = I915_READ(WRPLL_CTL(pll->id));
>  
> +	hw_state->wrpll = val;
>  	return val & WRPLL_PLL_ENABLE;
>  }
>  
>  static const char * const hsw_ddi_pll_names[] = {
>  	"WRPLL 1",
>  	"WRPLL 2",
> +	"SPLL"
>  };
>  
>  static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>  {
>  	int i;
>  
> -	dev_priv->num_shared_dpll = 2;
> +	dev_priv->num_shared_dpll = 3;
>  
> +	/* SPLL is special, but needs to be initialized anyway.. */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		dev_priv->shared_dplls[i].id = i;
>  		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ca9278be49f7..582360d2fa08 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  	struct intel_shared_dpll *pll;
>  	struct intel_shared_dpll_config *shared_dpll;
>  	enum intel_dpll_id i;
> +	int max = dev_priv->num_shared_dpll;
>  
>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
>  
> @@ -4242,9 +4243,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  		WARN_ON(shared_dpll[i].crtc_mask);
>  
>  		goto found;
> -	}
> +	} else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
> +		/* Do not consider SPLL */
> +		max = 2;
>  
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +	for (i = 0; i < max; i++) {
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> @@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>  	case PORT_CLK_SEL_WRPLL2:
>  		pipe_config->shared_dpll = DPLL_ID_WRPLL2;
>  		break;
> +	case PORT_CLK_SEL_SPLL:
> +		pipe_config->shared_dpll = DPLL_ID_SPLL;
>  	}
>  }
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Maarten Lankhorst]

Op 23-09-15 om 14:42 schreef Daniel Vetter:
> On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
>> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
>> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
>>
>> Right now this is not handled well in i915, and it calculates the crtc needs to
>> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
>> active. Fix this by exposing SPLL as a shared pll that will not be returned
>> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
>>
>> Cc: Emil Renner Berthing <kernel@esmil.dk>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> RFC because I haven't tested it with VGA, but it seems to work according to fix
>> the problem mentioned above.
> lgtm and gets rid of some of the fdi vs. normal ddi special-casing, which
> is nice (since that's a problem for bxt dsi too). I'll pick this up as
> soon as you managed to run this on some hsw (since we inject a fake vga
> screen for igt tests you don't even need a real screen).
>
Emil, do you have a haswell and if so can you test with intel-gpu-tools? kms_pipe_crc_basic and see if it works correctly.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Emil Renner Berthing]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On 23 September 2015 at 15:32, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 23-09-15 om 14:42 schreef Daniel Vetter:
>> On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
>>> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
>>> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
>>>
>>> Right now this is not handled well in i915, and it calculates the crtc needs to
>>> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
>>> active. Fix this by exposing SPLL as a shared pll that will not be returned
>>> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
>>>
>>> Cc: Emil Renner Berthing <kernel@esmil.dk>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>> RFC because I haven't tested it with VGA, but it seems to work according to fix
>>> the problem mentioned above.
>> lgtm and gets rid of some of the fdi vs. normal ddi special-casing, which
>> is nice (since that's a problem for bxt dsi too). I'll pick this up as
>> soon as you managed to run this on some hsw (since we inject a fake vga
>> screen for igt tests you don't even need a real screen).
>>
> Emil, do you have a haswell and if so can you test with intel-gpu-tools? kms_pipe_crc_basic and see if it works correctly.

Yeah, I just tested this on my Thinkpad X1 Carbon 2nd gen. with
00:02.0 VGA compatible controller: Intel Corporation Haswell-ULT
Integrated Graphics Controller (rev 0b).

It works fine.

--
Emil

[-- Attachment #2: kms_pipe_crc_basic.txt --]
[-- Type: text/plain, Size: 3350 bytes --]

IGT-Version: 1.12-ga167042 (x86_64) (Linux: 4.3.0.rc2-2-knud x86_64)
Subtest bad-pipe: SUCCESS (0.000s)
Subtest bad-source: SUCCESS (0.000s)
Subtest bad-nb-words-1: SUCCESS (0.000s)
Subtest bad-nb-words-3: SUCCESS (0.000s)
read-crc-pipe-A: Testing connector eDP-1 using pipe A
read-crc-pipe-A: Testing connector DP-1 using pipe A
read-crc-pipe-A: Testing connector HDMI-A-2 using pipe A
Subtest read-crc-pipe-A: SUCCESS (6.541s)
read-crc-pipe-A-frame-sequence: Testing connector eDP-1 using pipe A
read-crc-pipe-A-frame-sequence: Testing connector DP-1 using pipe A
read-crc-pipe-A-frame-sequence: Testing connector HDMI-A-2 using pipe A
Subtest read-crc-pipe-A-frame-sequence: SUCCESS (6.764s)
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Sep 23 15:29:01 2015
suspend-read-crc-pipe-A: Testing connector eDP-1 using pipe A
suspend-read-crc-pipe-A: Testing connector DP-1 using pipe A
suspend-read-crc-pipe-A: Testing connector HDMI-A-2 using pipe A
Subtest suspend-read-crc-pipe-A: SUCCESS (9.309s)
hang-read-crc-pipe-A: Testing connector eDP-1 using pipe A
hang-read-crc-pipe-A: Testing connector DP-1 using pipe A
hang-read-crc-pipe-A: Testing connector HDMI-A-2 using pipe A
Subtest hang-read-crc-pipe-A: SUCCESS (13.283s)
read-crc-pipe-B: Testing connector eDP-1 using pipe B
read-crc-pipe-B: Testing connector DP-1 using pipe B
read-crc-pipe-B: Testing connector HDMI-A-2 using pipe B
Subtest read-crc-pipe-B: SUCCESS (3.866s)
read-crc-pipe-B-frame-sequence: Testing connector eDP-1 using pipe B
read-crc-pipe-B-frame-sequence: Testing connector DP-1 using pipe B
read-crc-pipe-B-frame-sequence: Testing connector HDMI-A-2 using pipe B
Subtest read-crc-pipe-B-frame-sequence: SUCCESS (3.878s)
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Sep 23 15:30:02 2015
suspend-read-crc-pipe-B: Testing connector eDP-1 using pipe B
suspend-read-crc-pipe-B: Testing connector DP-1 using pipe B
suspend-read-crc-pipe-B: Testing connector HDMI-A-2 using pipe B
Subtest suspend-read-crc-pipe-B: SUCCESS (6.286s)
hang-read-crc-pipe-B: Testing connector eDP-1 using pipe B
hang-read-crc-pipe-B: Testing connector DP-1 using pipe B
hang-read-crc-pipe-B: Testing connector HDMI-A-2 using pipe B
Subtest hang-read-crc-pipe-B: SUCCESS (9.961s)
read-crc-pipe-C: Testing connector eDP-1 using pipe C
read-crc-pipe-C: Testing connector DP-1 using pipe C
read-crc-pipe-C: Testing connector HDMI-A-2 using pipe C
Subtest read-crc-pipe-C: SUCCESS (3.850s)
read-crc-pipe-C-frame-sequence: Testing connector eDP-1 using pipe C
read-crc-pipe-C-frame-sequence: Testing connector DP-1 using pipe C
read-crc-pipe-C-frame-sequence: Testing connector HDMI-A-2 using pipe C
Subtest read-crc-pipe-C-frame-sequence: SUCCESS (3.830s)
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Sep 23 15:30:56 2015
suspend-read-crc-pipe-C: Testing connector eDP-1 using pipe C
suspend-read-crc-pipe-C: Testing connector DP-1 using pipe C
suspend-read-crc-pipe-C: Testing connector HDMI-A-2 using pipe C
Subtest suspend-read-crc-pipe-C: SUCCESS (6.503s)
hang-read-crc-pipe-C: Testing connector eDP-1 using pipe C
hang-read-crc-pipe-C: Testing connector DP-1 using pipe C
hang-read-crc-pipe-C: Testing connector HDMI-A-2 using pipe C
Subtest hang-read-crc-pipe-C: SUCCESS (9.807s)

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Daniel Vetter]

On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
> 
> Right now this is not handled well in i915, and it calculates the crtc needs to
> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
> active. Fix this by exposing SPLL as a shared pll that will not be returned
> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
> 
> Cc: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> RFC because I haven't tested it with VGA, but it seems to work according to fix
> the problem mentioned above.
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 81adf89b92f1..cacdac67d9ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -345,6 +345,8 @@ enum intel_dpll_id {
>  	/* hsw/bdw */
>  	DPLL_ID_WRPLL1 = 0,
>  	DPLL_ID_WRPLL2 = 1,
> +	DPLL_ID_SPLL = 2,
> +
>  	/* skl */
>  	DPLL_ID_SKL_DPLL1 = 0,
>  	DPLL_ID_SKL_DPLL2 = 1,
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43bef4a4..592d8fe9f991 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
>  	pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder);
>  }
>  
> -static void hsw_crt_pre_enable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n");
> -	I915_WRITE(SPLL_CTL,
> -		   SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC);
> -	POSTING_READ(SPLL_CTL);
> -	udelay(20);
> -}
> -
>  /* Note: The caller is required to filter out dpms modes not supported by the
>   * platform. */
>  static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode)
> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
>  	intel_disable_crt(encoder);
>  }
>  
> -static void hsw_crt_post_disable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t val;
> -
> -	DRM_DEBUG_KMS("Disabling SPLL\n");
> -	val = I915_READ(SPLL_CTL);
> -	WARN_ON(!(val & SPLL_PLL_ENABLE));
> -	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
> -	POSTING_READ(SPLL_CTL);
> -}
> -
>  static void intel_enable_crt(struct intel_encoder *encoder)
>  {
>  	struct intel_crt *crt = intel_encoder_to_crt(encoder);
> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>  	if (HAS_DDI(dev)) {
>  		pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>  		pipe_config->port_clock = 135000 * 2;
> +		pipe_config->dpll_hw_state.wrpll =
> +			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
>  	}
>  
>  	return true;
> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev)
>  	if (HAS_DDI(dev)) {
>  		crt->base.get_config = hsw_crt_get_config;
>  		crt->base.get_hw_state = intel_ddi_get_hw_state;
> -		crt->base.pre_enable = hsw_crt_pre_enable;
> -		crt->base.post_disable = hsw_crt_post_disable;
>  	} else {
>  		crt->base.get_config = intel_crt_get_config;
>  		crt->base.get_hw_state = intel_crt_get_hw_state;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 61575f67a626..dabd903147fa 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		}
>  
>  		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
> +	} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) {
> +		struct drm_atomic_state *state = crtc_state->base.state;
> +		struct intel_shared_dpll_config *spll =
> +			&intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL];
> +
> +		if (spll->crtc_mask &&
> +		    WARN_ON(spll->hw_state.wrpll != crtc_state->dpll_hw_state.wrpll))
> +			return false;
> +
> +		crtc_state->shared_dpll = DPLL_ID_SPLL;
> +		spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll;
> +		spll->crtc_mask |= 1 << intel_crtc->pipe;
>  	}
>  
>  	return true;
> @@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
> -	POSTING_READ(WRPLL_CTL(pll->id));
> +	uint32_t reg;
> +
> +	if (pll->id == DPLL_ID_SPLL)
> +		reg = SPLL_CTL;
> +	else
> +		reg = WRPLL_CTL(pll->id);

This is a bit ugly, and reusing hw_sate.wrpll is also a bit fragile due to
the overlaps. I think it'd be better to add hw_state.spll (including hw
state cross-checking) and specialising the enable/disable/get_hw_state
functions.

With that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
> +	I915_WRITE(reg, pll->config.hw_state.wrpll);
> +	POSTING_READ(reg);
>  	udelay(20);
>  }
>  
>  static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
> -	uint32_t val;
> +	uint32_t reg, val;
>  
> -	val = I915_READ(WRPLL_CTL(pll->id));
> -	I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE);
> -	POSTING_READ(WRPLL_CTL(pll->id));
> +	if (pll->id == DPLL_ID_SPLL)
> +		reg = SPLL_CTL;
> +	else
> +		reg = WRPLL_CTL(pll->id);
> +
> +	val = I915_READ(reg);
> +	I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE);
> +	POSTING_READ(reg);
>  }
>  
>  static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
>  		return false;
>  
> -	val = I915_READ(WRPLL_CTL(pll->id));
> -	hw_state->wrpll = val;
> +	if (pll->id == DPLL_ID_SPLL)
> +		val = I915_READ(SPLL_CTL);
> +	else
> +		val = I915_READ(WRPLL_CTL(pll->id));
>  
> +	hw_state->wrpll = val;
>  	return val & WRPLL_PLL_ENABLE;
>  }
>  
>  static const char * const hsw_ddi_pll_names[] = {
>  	"WRPLL 1",
>  	"WRPLL 2",
> +	"SPLL"
>  };
>  
>  static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>  {
>  	int i;
>  
> -	dev_priv->num_shared_dpll = 2;
> +	dev_priv->num_shared_dpll = 3;
>  
> +	/* SPLL is special, but needs to be initialized anyway.. */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		dev_priv->shared_dplls[i].id = i;
>  		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ca9278be49f7..582360d2fa08 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  	struct intel_shared_dpll *pll;
>  	struct intel_shared_dpll_config *shared_dpll;
>  	enum intel_dpll_id i;
> +	int max = dev_priv->num_shared_dpll;
>  
>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
>  
> @@ -4242,9 +4243,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  		WARN_ON(shared_dpll[i].crtc_mask);
>  
>  		goto found;
> -	}
> +	} else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
> +		/* Do not consider SPLL */
> +		max = 2;
>  
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +	for (i = 0; i < max; i++) {
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> @@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>  	case PORT_CLK_SEL_WRPLL2:
>  		pipe_config->shared_dpll = DPLL_ID_WRPLL2;
>  		break;
> +	case PORT_CLK_SEL_SPLL:
> +		pipe_config->shared_dpll = DPLL_ID_SPLL;
>  	}
>  }
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Jani Nikula]

On Wed, 21 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
>> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
>> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
>> 
>> Right now this is not handled well in i915, and it calculates the crtc needs to
>> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
>> active. Fix this by exposing SPLL as a shared pll that will not be returned
>> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
>> 
>> Cc: Emil Renner Berthing <kernel@esmil.dk>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> RFC because I haven't tested it with VGA, but it seems to work according to fix
>> the problem mentioned above.
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 81adf89b92f1..cacdac67d9ba 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -345,6 +345,8 @@ enum intel_dpll_id {
>>  	/* hsw/bdw */
>>  	DPLL_ID_WRPLL1 = 0,
>>  	DPLL_ID_WRPLL2 = 1,
>> +	DPLL_ID_SPLL = 2,
>> +
>>  	/* skl */
>>  	DPLL_ID_SKL_DPLL1 = 0,
>>  	DPLL_ID_SKL_DPLL2 = 1,
>> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
>> index af5e43bef4a4..592d8fe9f991 100644
>> --- a/drivers/gpu/drm/i915/intel_crt.c
>> +++ b/drivers/gpu/drm/i915/intel_crt.c
>> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
>>  	pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder);
>>  }
>>  
>> -static void hsw_crt_pre_enable(struct intel_encoder *encoder)
>> -{
>> -	struct drm_device *dev = encoder->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -
>> -	WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n");
>> -	I915_WRITE(SPLL_CTL,
>> -		   SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC);
>> -	POSTING_READ(SPLL_CTL);
>> -	udelay(20);
>> -}
>> -
>>  /* Note: The caller is required to filter out dpms modes not supported by the
>>   * platform. */
>>  static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode)
>> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
>>  	intel_disable_crt(encoder);
>>  }
>>  
>> -static void hsw_crt_post_disable(struct intel_encoder *encoder)
>> -{
>> -	struct drm_device *dev = encoder->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	uint32_t val;
>> -
>> -	DRM_DEBUG_KMS("Disabling SPLL\n");
>> -	val = I915_READ(SPLL_CTL);
>> -	WARN_ON(!(val & SPLL_PLL_ENABLE));
>> -	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
>> -	POSTING_READ(SPLL_CTL);
>> -}
>> -
>>  static void intel_enable_crt(struct intel_encoder *encoder)
>>  {
>>  	struct intel_crt *crt = intel_encoder_to_crt(encoder);
>> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>>  	if (HAS_DDI(dev)) {
>>  		pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>>  		pipe_config->port_clock = 135000 * 2;
>> +		pipe_config->dpll_hw_state.wrpll =
>> +			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
>>  	}
>>  
>>  	return true;
>> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev)
>>  	if (HAS_DDI(dev)) {
>>  		crt->base.get_config = hsw_crt_get_config;
>>  		crt->base.get_hw_state = intel_ddi_get_hw_state;
>> -		crt->base.pre_enable = hsw_crt_pre_enable;
>> -		crt->base.post_disable = hsw_crt_post_disable;
>>  	} else {
>>  		crt->base.get_config = intel_crt_get_config;
>>  		crt->base.get_hw_state = intel_crt_get_hw_state;
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 61575f67a626..dabd903147fa 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  		}
>>  
>>  		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +	} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) {
>> +		struct drm_atomic_state *state = crtc_state->base.state;
>> +		struct intel_shared_dpll_config *spll =
>> +			&intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL];
>> +
>> +		if (spll->crtc_mask &&
>> +		    WARN_ON(spll->hw_state.wrpll != crtc_state->dpll_hw_state.wrpll))
>> +			return false;
>> +
>> +		crtc_state->shared_dpll = DPLL_ID_SPLL;
>> +		spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll;
>> +		spll->crtc_mask |= 1 << intel_crtc->pipe;
>>  	}
>>  
>>  	return true;
>> @@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>>  static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>  			       struct intel_shared_dpll *pll)
>>  {
>> -	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
>> -	POSTING_READ(WRPLL_CTL(pll->id));
>> +	uint32_t reg;
>> +
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		reg = SPLL_CTL;
>> +	else
>> +		reg = WRPLL_CTL(pll->id);
>
> This is a bit ugly, and reusing hw_sate.wrpll is also a bit fragile due to
> the overlaps. I think it'd be better to add hw_state.spll (including hw
> state cross-checking) and specialising the enable/disable/get_hw_state
> functions.
>
> With that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Maarten, please update the patch according to Daniel's comments, as this
fixes [1].

BR,
Jani.

[1] http://mid.gmane.org/5644DBE8.2090404@intel.com



>
>> +
>> +	I915_WRITE(reg, pll->config.hw_state.wrpll);
>> +	POSTING_READ(reg);
>>  	udelay(20);
>>  }
>>  
>>  static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
>>  				struct intel_shared_dpll *pll)
>>  {
>> -	uint32_t val;
>> +	uint32_t reg, val;
>>  
>> -	val = I915_READ(WRPLL_CTL(pll->id));
>> -	I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE);
>> -	POSTING_READ(WRPLL_CTL(pll->id));
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		reg = SPLL_CTL;
>> +	else
>> +		reg = WRPLL_CTL(pll->id);
>> +
>> +	val = I915_READ(reg);
>> +	I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE);
>> +	POSTING_READ(reg);
>>  }
>>  
>>  static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> @@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>  	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
>>  		return false;
>>  
>> -	val = I915_READ(WRPLL_CTL(pll->id));
>> -	hw_state->wrpll = val;
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		val = I915_READ(SPLL_CTL);
>> +	else
>> +		val = I915_READ(WRPLL_CTL(pll->id));
>>  
>> +	hw_state->wrpll = val;
>>  	return val & WRPLL_PLL_ENABLE;
>>  }
>>  
>>  static const char * const hsw_ddi_pll_names[] = {
>>  	"WRPLL 1",
>>  	"WRPLL 2",
>> +	"SPLL"
>>  };
>>  
>>  static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>>  {
>>  	int i;
>>  
>> -	dev_priv->num_shared_dpll = 2;
>> +	dev_priv->num_shared_dpll = 3;
>>  
>> +	/* SPLL is special, but needs to be initialized anyway.. */
>>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>  		dev_priv->shared_dplls[i].id = i;
>>  		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ca9278be49f7..582360d2fa08 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  	struct intel_shared_dpll *pll;
>>  	struct intel_shared_dpll_config *shared_dpll;
>>  	enum intel_dpll_id i;
>> +	int max = dev_priv->num_shared_dpll;
>>  
>>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
>>  
>> @@ -4242,9 +4243,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  		WARN_ON(shared_dpll[i].crtc_mask);
>>  
>>  		goto found;
>> -	}
>> +	} else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
>> +		/* Do not consider SPLL */
>> +		max = 2;
>>  
>> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +	for (i = 0; i < max; i++) {
>>  		pll = &dev_priv->shared_dplls[i];
>>  
>>  		/* Only want to check enabled timings first */
>> @@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>>  	case PORT_CLK_SEL_WRPLL2:
>>  		pipe_config->shared_dpll = DPLL_ID_WRPLL2;
>>  		break;
>> +	case PORT_CLK_SEL_SPLL:
>> +		pipe_config->shared_dpll = DPLL_ID_SPLL;
>>  	}
>>  }
>>  
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://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
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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