[PATCH 0/5] cpu edp fixes

[PATCH 0/5] cpu edp fixes

[Daniel Vetter]

Hi all!

Finally.

Now that the modeset-rework is merge I can finally dump the cpu edp fixes and
cleanups. Avoids my ivb edp from getting angry and leaving random pipes stuck in
the active state and the panel in the passive state.

Again, has been part of the modeset-rework branch since a longer time, so has
seen tons of testing.

Flames, comments & review highly welcome.

Cheers, Daniel

Daniel Vetter (5):
  drm/i915: add encoder->pre_enable/post_disable
  drm/i915: clean up the cpu edp pll special case
  drm/i915: robustify edp_pll_on/off
  drm/i915: rip out dp port enabling cludges^Wchecks
  drm/i915: disable the cpu edp port after the cpu pipe

 drivers/gpu/drm/i915/intel_display.c |   8 +++
 drivers/gpu/drm/i915/intel_dp.c      | 126 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 3 files changed, 69 insertions(+), 67 deletions(-)

-- 
1.7.11.2

[PATCH 1/5] drm/i915: add encoder->pre_enable/post_disable

[Daniel Vetter]

The cpu eDP encoder has some horrible hacks to set up the DP pll at
the right time. To be able to move them to the right place, add some
more encoder callbacks so that this can happen at the right time.

LVDS has some similar funky hacks, but that would require more work
(we need to move around the pll setup a bit). Hence for now only
wire these new callbacks up for ilk+ - we only have cpu eDP on these
platforms.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
 drivers/gpu/drm/i915/intel_drv.h     | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bff0936..1d31364 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3189,6 +3189,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		assert_fdi_rx_disabled(dev_priv, pipe);
 	}
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_enable)
+			encoder->pre_enable(encoder);
+
 	/* Enable panel fitting for LVDS */
 	if (dev_priv->pch_pf_size &&
 	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || HAS_eDP)) {
@@ -3258,6 +3262,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	I915_WRITE(PF_CTL(pipe), 0);
 	I915_WRITE(PF_WIN_SZ(pipe), 0);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->post_disable)
+			encoder->post_disable(encoder);
+
 	ironlake_fdi_disable(crtc);
 
 	intel_disable_transcoder(dev_priv, pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4f2b2d6..1306f05 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -153,7 +153,9 @@ struct intel_encoder {
 	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
 	void (*enable)(struct intel_encoder *);
+	void (*pre_enable)(struct intel_encoder *);
 	void (*disable)(struct intel_encoder *);
+	void (*post_disable)(struct intel_encoder *);
 	/* Read out the current hw state of this connector, returning true if
 	 * the encoder is active. If the encoder is enabled it also set the pipe
 	 * it is connected to in the pipe parameter. */
-- 
1.7.11.2

[PATCH 2/5] drm/i915: clean up the cpu edp pll special case

[Daniel Vetter]

By using the new pre_enabel/post_disable functions.

To ensure that we only frob the cpu edp pll while the pipe is off add
the relevant asserts. Thanks to the new output state staging, this is
now really easy.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 74 +++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c59710d..c72d4f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -808,9 +808,6 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	}
 }
 
-static void ironlake_edp_pll_on(struct drm_encoder *encoder);
-static void ironlake_edp_pll_off(struct drm_encoder *encoder);
-
 static void
 intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		  struct drm_display_mode *adjusted_mode)
@@ -821,14 +818,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	struct drm_crtc *crtc = intel_dp->base.base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	/* Turn on the eDP PLL if needed */
-	if (is_edp(intel_dp)) {
-		if (!is_pch_edp(intel_dp))
-			ironlake_edp_pll_on(encoder);
-		else
-			ironlake_edp_pll_off(encoder);
-	}
-
 	/*
 	 * There are four kinds of DP registers:
 	 *
@@ -1191,12 +1180,16 @@ static void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
 	msleep(intel_dp->backlight_off_delay);
 }
 
-static void ironlake_edp_pll_on(struct drm_encoder *encoder)
+static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_crtc *crtc = intel_dp->base.base.crtc;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpa_ctl;
 
+	assert_pipe_disabled(dev_priv,
+			     to_intel_crtc(crtc)->pipe);
+
 	DRM_DEBUG_KMS("\n");
 	dpa_ctl = I915_READ(DP_A);
 	dpa_ctl |= DP_PLL_ENABLE;
@@ -1205,12 +1198,16 @@ static void ironlake_edp_pll_on(struct drm_encoder *encoder)
 	udelay(200);
 }
 
-static void ironlake_edp_pll_off(struct drm_encoder *encoder)
+static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_crtc *crtc = intel_dp->base.base.crtc;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpa_ctl;
 
+	assert_pipe_disabled(dev_priv,
+			     to_intel_crtc(crtc)->pipe);
+
 	dpa_ctl = I915_READ(DP_A);
 	dpa_ctl &= ~DP_PLL_ENABLE;
 	I915_WRITE(DP_A, dpa_ctl);
@@ -1309,6 +1306,14 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 	intel_dp_link_down(intel_dp);
 }
 
+static void intel_post_disable_dp(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+
+	if (is_cpu_edp(intel_dp))
+		ironlake_edp_pll_off(intel_dp);
+}
+
 static void intel_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
@@ -1328,39 +1333,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	ironlake_edp_backlight_on(intel_dp);
 }
 
-static void
-intel_dp_dpms(struct drm_connector *connector, int mode)
+static void intel_pre_enable_dp(struct intel_encoder *encoder)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-
-	/* DP supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	if (!intel_dp->base.base.crtc) {
-		intel_dp->base.connectors_active = false;
-		return;
-	}
-
-	if (mode != DRM_MODE_DPMS_ON) {
-		intel_encoder_dpms(&intel_dp->base, mode);
-
-		if (is_cpu_edp(intel_dp))
-			ironlake_edp_pll_off(&intel_dp->base.base);
-	} else {
-		if (is_cpu_edp(intel_dp))
-			ironlake_edp_pll_on(&intel_dp->base.base);
-
-		intel_encoder_dpms(&intel_dp->base, mode);
-	}
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 
-	intel_modeset_check_state(connector->dev);
+	if (is_cpu_edp(intel_dp))
+		ironlake_edp_pll_on(intel_dp);
 }
 
 /*
@@ -2391,7 +2369,7 @@ static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = {
 };
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
-	.dpms = intel_dp_dpms,
+	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_set_property,
@@ -2522,7 +2500,9 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	drm_sysfs_connector_add(connector);
 
 	intel_encoder->enable = intel_enable_dp;
+	intel_encoder->pre_enable = intel_pre_enable_dp;
 	intel_encoder->disable = intel_disable_dp;
+	intel_encoder->post_disable = intel_post_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
 
-- 
1.7.11.2

[PATCH 3/5] drm/i915: robustify edp_pll_on/off

[Daniel Vetter]

With the previous patch to clean up where exactly these two functions
are getting called, this patch can tackle the enable/disable code
itself:

- WARN if the port enable bit is in the wrong state or if the edp pll
  bit is in the wrong state, just for paranoia's sake.
- Don't disable the edp pll harder in the modeset functions just for
  fun.
- Don't set the edp pll enable flag in intel_dp->DP in modeset, do
  that while changing the actual hw state. We do the same with the
  actual port enable bit, so this is a bit more consistent.
- Track the current DP register value when setting things up and add
  some comments how intel_dp->DP is used in the disable code.

v2: Be more careful with resetting intel_dp->DP - otherwise dpms
off->on will fail spectacularly, becuase we enable the eDP port when
we should only enable the eDP pll.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c72d4f3..7c746d1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		intel_dp->DP |= intel_crtc->pipe << 29;
 
 		/* don't miss out required setting for eDP */
-		intel_dp->DP |= DP_PLL_ENABLE;
 		if (adjusted_mode->clock < 200000)
 			intel_dp->DP |= DP_PLL_FREQ_160MHZ;
 		else
@@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 
 		if (is_cpu_edp(intel_dp)) {
 			/* don't miss out required setting for eDP */
-			intel_dp->DP |= DP_PLL_ENABLE;
 			if (adjusted_mode->clock < 200000)
 				intel_dp->DP |= DP_PLL_FREQ_160MHZ;
 			else
@@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("\n");
 	dpa_ctl = I915_READ(DP_A);
-	dpa_ctl |= DP_PLL_ENABLE;
-	I915_WRITE(DP_A, dpa_ctl);
+	WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
+	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
+
+	/* We don't adjust intel_dp->DP while tearing down the link, to
+	 * facilitate link retraining (e.g. after hotplug). Hence clear all
+	 * enable bits here to ensure that we don't enable too much. */
+	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
+	intel_dp->DP |= DP_PLL_ENABLE;
+	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(200);
 }
@@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 			     to_intel_crtc(crtc)->pipe);
 
 	dpa_ctl = I915_READ(DP_A);
+	WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
+	     "dp pll off, should be on\n");
+	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
+
+	/* We can't rely on the value tracked for the DP register in
+	 * intel_dp->DP because link_down must not change that (otherwise link
+	 * re-training will fail. */
 	dpa_ctl &= ~DP_PLL_ENABLE;
 	I915_WRITE(DP_A, dpa_ctl);
 	POSTING_READ(DP_A);
@@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("\n");
 
-	if (is_edp(intel_dp)) {
-		DP &= ~DP_PLL_ENABLE;
-		I915_WRITE(intel_dp->output_reg, DP);
-		POSTING_READ(intel_dp->output_reg);
-		udelay(100);
-	}
-
 	if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
 		DP &= ~DP_LINK_TRAIN_MASK_CPT;
 		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
@@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 
 	intel_dp->output_reg = output_reg;
 	intel_dp->port = port;
+	/* Preserve the current hw state. */
+	intel_dp->DP = I915_READ(intel_dp->output_reg);
 
 	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
-- 
1.7.11.2

[PATCH 4/5] drm/i915: rip out dp port enabling cludges^Wchecks

[Daniel Vetter]

These have been added because dp links are fiddle things and don't
like it when we try to re-train an enabled output (or disable a
disabled output harder). And because the crtc helper code is
ridiculously bad add tracking the modeset state.

But with the new code in place it is simply a bug to disable a disabled
encoder or to enable an enabled encoder again. Hence convert these to
WARNs (and bail out for safety), but flatten all conditionals in the
code itself.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7c746d1..07f9521 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1333,15 +1333,15 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
 
+	if (WARN_ON(dp_reg & DP_PORT_EN))
+		return;
+
 	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-	if (!(dp_reg & DP_PORT_EN)) {
-		intel_dp_start_link_train(intel_dp);
-		ironlake_edp_panel_on(intel_dp);
-		ironlake_edp_panel_vdd_off(intel_dp, true);
-		intel_dp_complete_link_train(intel_dp);
-	} else
-		ironlake_edp_panel_vdd_off(intel_dp, false);
+	intel_dp_start_link_train(intel_dp);
+	ironlake_edp_panel_on(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, true);
+	intel_dp_complete_link_train(intel_dp);
 	ironlake_edp_backlight_on(intel_dp);
 }
 
@@ -1913,7 +1913,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t DP = intel_dp->DP;
 
-	if ((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0)
+	if (WARN_ON((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0))
 		return;
 
 	DRM_DEBUG_KMS("\n");
-- 
1.7.11.2

[PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe

[Daniel Vetter]

See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This
applies to all platforms where we currently support eDP on, i.e. ilk,
snb & ivb.

Without this change we fail to light up the eDP port on previously
unused crtcs (likely because something is stuck on the old pipe), and
we also fail to properly disable the old pipe (i.e. bit 30 in the
PIPECONF register is stuck as set until the next reboot).

v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 07f9521..f28353d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 	ironlake_edp_backlight_off(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	ironlake_edp_panel_off(intel_dp);
-	intel_dp_link_down(intel_dp);
+
+	/* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
+	if (!is_cpu_edp(intel_dp))
+		intel_dp_link_down(intel_dp);
 }
 
 static void intel_post_disable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 
-	if (is_cpu_edp(intel_dp))
+	if (is_cpu_edp(intel_dp)) {
+		intel_dp_link_down(intel_dp);
 		ironlake_edp_pll_off(intel_dp);
+	}
 }
 
 static void intel_enable_dp(struct intel_encoder *encoder)
-- 
1.7.11.2

Re: [PATCH 1/5] drm/i915: add encoder->pre_enable/post_disable

[Paulo Zanoni]

Hi

2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The cpu eDP encoder has some horrible hacks to set up the DP pll at
> the right time. To be able to move them to the right place, add some
> more encoder callbacks so that this can happen at the right time.
>
> LVDS has some similar funky hacks, but that would require more work
> (we need to move around the pll setup a bit). Hence for now only
> wire these new callbacks up for ilk+ - we only have cpu eDP on these
> platforms.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I also plan to use these callbacks in the future.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Notice that this patch can not be applied to dinq as-is because of
small easy-to-solve conflicts.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h     | 2 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bff0936..1d31364 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3189,6 +3189,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>                 assert_fdi_rx_disabled(dev_priv, pipe);
>         }
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_enable)
> +                       encoder->pre_enable(encoder);
> +
>         /* Enable panel fitting for LVDS */
>         if (dev_priv->pch_pf_size &&
>             (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || HAS_eDP)) {
> @@ -3258,6 +3262,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>         I915_WRITE(PF_CTL(pipe), 0);
>         I915_WRITE(PF_WIN_SZ(pipe), 0);
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->post_disable)
> +                       encoder->post_disable(encoder);
> +
>         ironlake_fdi_disable(crtc);
>
>         intel_disable_transcoder(dev_priv, pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4f2b2d6..1306f05 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -153,7 +153,9 @@ struct intel_encoder {
>         bool connectors_active;
>         void (*hot_plug)(struct intel_encoder *);
>         void (*enable)(struct intel_encoder *);
> +       void (*pre_enable)(struct intel_encoder *);
>         void (*disable)(struct intel_encoder *);
> +       void (*post_disable)(struct intel_encoder *);
>         /* Read out the current hw state of this connector, returning true if
>          * the encoder is active. If the encoder is enabled it also set the pipe
>          * it is connected to in the pipe parameter. */
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/5] drm/i915: clean up the cpu edp pll special case

[Paulo Zanoni]

Hi

2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> By using the new pre_enabel/post_disable functions.

s/enabel/enable

>
> To ensure that we only frob the cpu edp pll while the pipe is off add
> the relevant asserts. Thanks to the new output state staging, this is
> now really easy.

This patch does more than it says. It creates the new pre/post
enable/disable functions, but it also replaces the dpms connector
function with the default intel_connector_dpms (because after removing
the edp enable/disable code from the dpms function, it will look
almost exactly like intel_connector_dpms). Ideally this should be 2
patches: first do the pre/post enable/disable dance, then switch the
specialized dpms with the generic one. The main reason to split this
in 2 patches is to make it easier to reviewers understand what's going
on, so they can review faster without trying to discover why you
switched the dpms function. But since you've already got a reviewer,
you should at least write about the dpms change in the commit message.

With the typo fixed and at least a small sentence on the commit
message explaining the replacement of intel_dp_dpms with
intel_connector_dpms:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 74 +++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c59710d..c72d4f3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -808,9 +808,6 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>         }
>  }
>
> -static void ironlake_edp_pll_on(struct drm_encoder *encoder);
> -static void ironlake_edp_pll_off(struct drm_encoder *encoder);
> -
>  static void
>  intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>                   struct drm_display_mode *adjusted_mode)
> @@ -821,14 +818,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>         struct drm_crtc *crtc = intel_dp->base.base.crtc;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> -       /* Turn on the eDP PLL if needed */
> -       if (is_edp(intel_dp)) {
> -               if (!is_pch_edp(intel_dp))
> -                       ironlake_edp_pll_on(encoder);
> -               else
> -                       ironlake_edp_pll_off(encoder);
> -       }
> -
>         /*
>          * There are four kinds of DP registers:
>          *
> @@ -1191,12 +1180,16 @@ static void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
>         msleep(intel_dp->backlight_off_delay);
>  }
>
> -static void ironlake_edp_pll_on(struct drm_encoder *encoder)
> +static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  {
> -       struct drm_device *dev = encoder->dev;
> +       struct drm_device *dev = intel_dp->base.base.dev;
> +       struct drm_crtc *crtc = intel_dp->base.base.crtc;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 dpa_ctl;
>
> +       assert_pipe_disabled(dev_priv,
> +                            to_intel_crtc(crtc)->pipe);
> +
>         DRM_DEBUG_KMS("\n");
>         dpa_ctl = I915_READ(DP_A);
>         dpa_ctl |= DP_PLL_ENABLE;
> @@ -1205,12 +1198,16 @@ static void ironlake_edp_pll_on(struct drm_encoder *encoder)
>         udelay(200);
>  }
>
> -static void ironlake_edp_pll_off(struct drm_encoder *encoder)
> +static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>  {
> -       struct drm_device *dev = encoder->dev;
> +       struct drm_device *dev = intel_dp->base.base.dev;
> +       struct drm_crtc *crtc = intel_dp->base.base.crtc;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 dpa_ctl;
>
> +       assert_pipe_disabled(dev_priv,
> +                            to_intel_crtc(crtc)->pipe);
> +
>         dpa_ctl = I915_READ(DP_A);
>         dpa_ctl &= ~DP_PLL_ENABLE;
>         I915_WRITE(DP_A, dpa_ctl);
> @@ -1309,6 +1306,14 @@ static void intel_disable_dp(struct intel_encoder *encoder)
>         intel_dp_link_down(intel_dp);
>  }
>
> +static void intel_post_disable_dp(struct intel_encoder *encoder)
> +{
> +       struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +       if (is_cpu_edp(intel_dp))
> +               ironlake_edp_pll_off(intel_dp);
> +}
> +
>  static void intel_enable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -1328,39 +1333,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>         ironlake_edp_backlight_on(intel_dp);
>  }
>
> -static void
> -intel_dp_dpms(struct drm_connector *connector, int mode)
> +static void intel_pre_enable_dp(struct intel_encoder *encoder)
>  {
> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> -
> -       /* DP supports only 2 dpms states. */
> -       if (mode != DRM_MODE_DPMS_ON)
> -               mode = DRM_MODE_DPMS_OFF;
> -
> -       if (mode == connector->dpms)
> -               return;
> -
> -       connector->dpms = mode;
> -
> -       /* Only need to change hw state when actually enabled */
> -       if (!intel_dp->base.base.crtc) {
> -               intel_dp->base.connectors_active = false;
> -               return;
> -       }
> -
> -       if (mode != DRM_MODE_DPMS_ON) {
> -               intel_encoder_dpms(&intel_dp->base, mode);
> -
> -               if (is_cpu_edp(intel_dp))
> -                       ironlake_edp_pll_off(&intel_dp->base.base);
> -       } else {
> -               if (is_cpu_edp(intel_dp))
> -                       ironlake_edp_pll_on(&intel_dp->base.base);
> -
> -               intel_encoder_dpms(&intel_dp->base, mode);
> -       }
> +       struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>
> -       intel_modeset_check_state(connector->dev);
> +       if (is_cpu_edp(intel_dp))
> +               ironlake_edp_pll_on(intel_dp);
>  }
>
>  /*
> @@ -2391,7 +2369,7 @@ static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = {
>  };
>
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> -       .dpms = intel_dp_dpms,
> +       .dpms = intel_connector_dpms,
>         .detect = intel_dp_detect,
>         .fill_modes = drm_helper_probe_single_connector_modes,
>         .set_property = intel_dp_set_property,
> @@ -2522,7 +2500,9 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>         drm_sysfs_connector_add(connector);
>
>         intel_encoder->enable = intel_enable_dp;
> +       intel_encoder->pre_enable = intel_pre_enable_dp;
>         intel_encoder->disable = intel_disable_dp;
> +       intel_encoder->post_disable = intel_post_disable_dp;
>         intel_encoder->get_hw_state = intel_dp_get_hw_state;
>         intel_connector->get_hw_state = intel_connector_get_hw_state;
>
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/5] drm/i915: clean up the cpu edp pll special case

[Daniel Vetter]

On Wed, Sep 12, 2012 at 06:00:58PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > By using the new pre_enabel/post_disable functions.
> 
> s/enabel/enable
> 
> >
> > To ensure that we only frob the cpu edp pll while the pipe is off add
> > the relevant asserts. Thanks to the new output state staging, this is
> > now really easy.
> 
> This patch does more than it says. It creates the new pre/post
> enable/disable functions, but it also replaces the dpms connector
> function with the default intel_connector_dpms (because after removing
> the edp enable/disable code from the dpms function, it will look
> almost exactly like intel_connector_dpms). Ideally this should be 2
> patches: first do the pre/post enable/disable dance, then switch the
> specialized dpms with the generic one. The main reason to split this
> in 2 patches is to make it easier to reviewers understand what's going
> on, so they can review faster without trying to discover why you
> switched the dpms function. But since you've already got a reviewer,
> you should at least write about the dpms change in the commit message.
> 
> With the typo fixed and at least a small sentence on the commit
> message explaining the replacement of intel_dp_dpms with
> intel_connector_dpms:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I've added a paragraph to explain why we can now simplify the dpms code,
too. Thanks for the review, patches 1&2 applied to dinq.
-Daniel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915: robustify edp_pll_on/off

[Paulo Zanoni]

Hi

2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> With the previous patch to clean up where exactly these two functions
> are getting called, this patch can tackle the enable/disable code
> itself:

This is the kind of patch that's hard to proof-read. Basically, IMHO,
the intel_dp->DP variable makes things harder to understand and
review. Sometimes we use intel_dp->DP directly, sometimes we do "int
DP = intel_dp->DP" and then just change the temporary DP (sometimes
not updating the original intel_dp->DP after changing the temporary
DP), sometimes we read/write directly from intel_dp->output_reg,
sometimes we pass "int DP" as a function argument instead of storing
the value inside intel_dp->DP. I have to admit that even after working
some time with this code I'm not exactly sure why intel_dp->DP is
really necessary. One of my plans was to try to write a patch that
removes the variable so I could at least maybe understand why it's
necessary (the plan was to read/write from/to intel_dp->output_reg
directly). You can do that if you want :)

Complaints aside, here are my comments:

>
> - WARN if the port enable bit is in the wrong state or if the edp pll
>   bit is in the wrong state, just for paranoia's sake.

Looks good.

> - Don't disable the edp pll harder in the modeset functions just for
>   fun.

Which part of the patch is this message about? Did you mean
intel_dp_link_down instead of modeset? I could not find anything on
our documentation saying that the PLL must be disabled when retraining
the link, so your patch looks correct, but this is the kind of thing
that we should really try to test somehow :)

> - Don't set the edp pll enable flag in intel_dp->DP in modeset, do
>   that while changing the actual hw state. We do the same with the
>   actual port enable bit, so this is a bit more consistent.

Looks good.

> - Track the current DP register value when setting things up and add
>   some comments how intel_dp->DP is used in the disable code.
>
> v2: Be more careful with resetting intel_dp->DP - otherwise dpms
> off->on will fail spectacularly, becuase we enable the eDP port when
> we should only enable the eDP pll.
>

These 2 chunks are the ones my comment above is about. I'll just trust
our beloved maintainer on these chunks :)
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c72d4f3..7c746d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>                 intel_dp->DP |= intel_crtc->pipe << 29;
>
>                 /* don't miss out required setting for eDP */
> -               intel_dp->DP |= DP_PLL_ENABLE;
>                 if (adjusted_mode->clock < 200000)
>                         intel_dp->DP |= DP_PLL_FREQ_160MHZ;
>                 else
> @@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>
>                 if (is_cpu_edp(intel_dp)) {
>                         /* don't miss out required setting for eDP */
> -                       intel_dp->DP |= DP_PLL_ENABLE;
>                         if (adjusted_mode->clock < 200000)
>                                 intel_dp->DP |= DP_PLL_FREQ_160MHZ;
>                         else
> @@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>
>         DRM_DEBUG_KMS("\n");
>         dpa_ctl = I915_READ(DP_A);
> -       dpa_ctl |= DP_PLL_ENABLE;
> -       I915_WRITE(DP_A, dpa_ctl);
> +       WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
> +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> +
> +       /* We don't adjust intel_dp->DP while tearing down the link, to
> +        * facilitate link retraining (e.g. after hotplug). Hence clear all
> +        * enable bits here to ensure that we don't enable too much. */
> +       intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> +       intel_dp->DP |= DP_PLL_ENABLE;
> +       I915_WRITE(DP_A, intel_dp->DP);
>         POSTING_READ(DP_A);
>         udelay(200);
>  }
> @@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>                              to_intel_crtc(crtc)->pipe);
>
>         dpa_ctl = I915_READ(DP_A);
> +       WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
> +            "dp pll off, should be on\n");
> +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> +
> +       /* We can't rely on the value tracked for the DP register in
> +        * intel_dp->DP because link_down must not change that (otherwise link
> +        * re-training will fail. */
>         dpa_ctl &= ~DP_PLL_ENABLE;
>         I915_WRITE(DP_A, dpa_ctl);
>         POSTING_READ(DP_A);
> @@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>
>         DRM_DEBUG_KMS("\n");
>
> -       if (is_edp(intel_dp)) {
> -               DP &= ~DP_PLL_ENABLE;
> -               I915_WRITE(intel_dp->output_reg, DP);
> -               POSTING_READ(intel_dp->output_reg);
> -               udelay(100);
> -       }
> -
>         if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
>                 DP &= ~DP_LINK_TRAIN_MASK_CPT;
>                 I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> @@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>
>         intel_dp->output_reg = output_reg;
>         intel_dp->port = port;
> +       /* Preserve the current hw state. */
> +       intel_dp->DP = I915_READ(intel_dp->output_reg);
>
>         intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
>         if (!intel_connector) {
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: rip out dp port enabling cludges^Wchecks

[Paulo Zanoni]

Hi

2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> These have been added because dp links are fiddle things and don't
> like it when we try to re-train an enabled output (or disable a
> disabled output harder). And because the crtc helper code is
> ridiculously bad add tracking the modeset state.
>
> But with the new code in place it is simply a bug to disable a disabled
> encoder or to enable an enabled encoder again. Hence convert these to
> WARNs (and bail out for safety), but flatten all conditionals in the
> code itself.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7c746d1..07f9521 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1333,15 +1333,15 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t dp_reg = I915_READ(intel_dp->output_reg);
>
> +       if (WARN_ON(dp_reg & DP_PORT_EN))
> +               return;
> +
>         ironlake_edp_panel_vdd_on(intel_dp);
>         intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> -       if (!(dp_reg & DP_PORT_EN)) {
> -               intel_dp_start_link_train(intel_dp);
> -               ironlake_edp_panel_on(intel_dp);
> -               ironlake_edp_panel_vdd_off(intel_dp, true);
> -               intel_dp_complete_link_train(intel_dp);
> -       } else
> -               ironlake_edp_panel_vdd_off(intel_dp, false);
> +       intel_dp_start_link_train(intel_dp);
> +       ironlake_edp_panel_on(intel_dp);
> +       ironlake_edp_panel_vdd_off(intel_dp, true);
> +       intel_dp_complete_link_train(intel_dp);
>         ironlake_edp_backlight_on(intel_dp);
>  }
>
> @@ -1913,7 +1913,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t DP = intel_dp->DP;
>
> -       if ((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0)
> +       if (WARN_ON((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0))
>                 return;
>
>         DRM_DEBUG_KMS("\n");
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe

[Paulo Zanoni]

Hi

2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This
> applies to all platforms where we currently support eDP on, i.e. ilk,
> snb & ivb.
>

Ok, so I looked at BSpec and the conclusion is: shouldn't we do this
for eDP _and_ DP instead of just eDP? The only things to disable
before the crtc are audio, the panel backlight, and then the panel
power.

Your patch looks correct, but maybe it could be even more correct by
including DP too? Yes, I can help testing.

> Without this change we fail to light up the eDP port on previously
> unused crtcs (likely because something is stuck on the old pipe), and
> we also fail to properly disable the old pipe (i.e. bit 30 in the
> PIPECONF register is stuck as set until the next reboot).
>
> v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 07f9521..f28353d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder)
>         ironlake_edp_backlight_off(intel_dp);
>         intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>         ironlake_edp_panel_off(intel_dp);
> -       intel_dp_link_down(intel_dp);
> +
> +       /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
> +       if (!is_cpu_edp(intel_dp))
> +               intel_dp_link_down(intel_dp);
>  }
>
>  static void intel_post_disable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>
> -       if (is_cpu_edp(intel_dp))
> +       if (is_cpu_edp(intel_dp)) {
> +               intel_dp_link_down(intel_dp);
>                 ironlake_edp_pll_off(intel_dp);
> +       }
>  }
>
>  static void intel_enable_dp(struct intel_encoder *encoder)
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe

[Daniel Vetter]

On Thu, Sep 13, 2012 at 04:11:20PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This
> > applies to all platforms where we currently support eDP on, i.e. ilk,
> > snb & ivb.
> >
> 
> Ok, so I looked at BSpec and the conclusion is: shouldn't we do this
> for eDP _and_ DP instead of just eDP? The only things to disable
> before the crtc are audio, the panel backlight, and then the panel
> power.
> 
> Your patch looks correct, but maybe it could be even more correct by
> including DP too? Yes, I can help testing.

The dp pll for cpu edp is special, since we set it in the DP_A register.
The pll for pch dp ports is just the regular pch pll iirc, and that is
handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was
definitely broken.

In any case, frobbing the pch dp sequence would be a separate patch imo.
So can I still have an r-b for this on here?
-Daniel

> 
> > Without this change we fail to light up the eDP port on previously
> > unused crtcs (likely because something is stuck on the old pipe), and
> > we also fail to properly disable the old pipe (i.e. bit 30 in the
> > PIPECONF register is stuck as set until the next reboot).
> >
> > v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 07f9521..f28353d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder)
> >         ironlake_edp_backlight_off(intel_dp);
> >         intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >         ironlake_edp_panel_off(intel_dp);
> > -       intel_dp_link_down(intel_dp);
> > +
> > +       /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
> > +       if (!is_cpu_edp(intel_dp))
> > +               intel_dp_link_down(intel_dp);
> >  }
> >
> >  static void intel_post_disable_dp(struct intel_encoder *encoder)
> >  {
> >         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >
> > -       if (is_cpu_edp(intel_dp))
> > +       if (is_cpu_edp(intel_dp)) {
> > +               intel_dp_link_down(intel_dp);
> >                 ironlake_edp_pll_off(intel_dp);
> > +       }
> >  }
> >
> >  static void intel_enable_dp(struct intel_encoder *encoder)
> > --
> > 1.7.11.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915: robustify edp_pll_on/off

[Daniel Vetter]

On Thu, Sep 13, 2012 at 02:46:19PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > With the previous patch to clean up where exactly these two functions
> > are getting called, this patch can tackle the enable/disable code
> > itself:
> 
> This is the kind of patch that's hard to proof-read. Basically, IMHO,
> the intel_dp->DP variable makes things harder to understand and
> review. Sometimes we use intel_dp->DP directly, sometimes we do "int
> DP = intel_dp->DP" and then just change the temporary DP (sometimes
> not updating the original intel_dp->DP after changing the temporary
> DP), sometimes we read/write directly from intel_dp->output_reg,
> sometimes we pass "int DP" as a function argument instead of storing
> the value inside intel_dp->DP. I have to admit that even after working
> some time with this code I'm not exactly sure why intel_dp->DP is
> really necessary. One of my plans was to try to write a patch that
> removes the variable so I could at least maybe understand why it's
> necessary (the plan was to read/write from/to intel_dp->output_reg
> directly). You can do that if you want :)
> 
> Complaints aside, here are my comments:
> 
> >
> > - WARN if the port enable bit is in the wrong state or if the edp pll
> >   bit is in the wrong state, just for paranoia's sake.
> 
> Looks good.
> 
> > - Don't disable the edp pll harder in the modeset functions just for
> >   fun.
> 
> Which part of the patch is this message about? Did you mean
> intel_dp_link_down instead of modeset? I could not find anything on
> our documentation saying that the PLL must be disabled when retraining
> the link, so your patch looks correct, but this is the kind of thing
> that we should really try to test somehow :)

See below.
> 
> > - Don't set the edp pll enable flag in intel_dp->DP in modeset, do
> >   that while changing the actual hw state. We do the same with the
> >   actual port enable bit, so this is a bit more consistent.
> 
> Looks good.
> 
> > - Track the current DP register value when setting things up and add
> >   some comments how intel_dp->DP is used in the disable code.
> >
> > v2: Be more careful with resetting intel_dp->DP - otherwise dpms
> > off->on will fail spectacularly, becuase we enable the eDP port when
> > we should only enable the eDP pll.
> >
> 
> These 2 chunks are the ones my comment above is about. I'll just trust
> our beloved maintainer on these chunks :)

Yeah, intel_dp->DP is hard to follow, and I have some more patches. After
all this has settled the rule is:
- intel_dp->DP is computed freshly in intel_dp_mode_set
- intel_dp->DP is update with the link training values in start_link_train
  so that complete_link_train writes the right registers. Same applies to
  edp_pll_on for similar reasons
- All the disable functions don't frob intel_dp->DP so that we can
  re-enable after dpms off without going through ->mode_set

So yeah, a bit a mess, but should work.
-Daniel

> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index c72d4f3..7c746d1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> >                 intel_dp->DP |= intel_crtc->pipe << 29;
> >
> >                 /* don't miss out required setting for eDP */
> > -               intel_dp->DP |= DP_PLL_ENABLE;
> >                 if (adjusted_mode->clock < 200000)
> >                         intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> >                 else
> > @@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> >
> >                 if (is_cpu_edp(intel_dp)) {
> >                         /* don't miss out required setting for eDP */
> > -                       intel_dp->DP |= DP_PLL_ENABLE;
> >                         if (adjusted_mode->clock < 200000)
> >                                 intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> >                         else
> > @@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> >
> >         DRM_DEBUG_KMS("\n");
> >         dpa_ctl = I915_READ(DP_A);
> > -       dpa_ctl |= DP_PLL_ENABLE;
> > -       I915_WRITE(DP_A, dpa_ctl);
> > +       WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
> > +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> > +
> > +       /* We don't adjust intel_dp->DP while tearing down the link, to
> > +        * facilitate link retraining (e.g. after hotplug). Hence clear all
> > +        * enable bits here to ensure that we don't enable too much. */
> > +       intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> > +       intel_dp->DP |= DP_PLL_ENABLE;
> > +       I915_WRITE(DP_A, intel_dp->DP);
> >         POSTING_READ(DP_A);
> >         udelay(200);
> >  }
> > @@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
> >                              to_intel_crtc(crtc)->pipe);
> >
> >         dpa_ctl = I915_READ(DP_A);
> > +       WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
> > +            "dp pll off, should be on\n");
> > +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> > +
> > +       /* We can't rely on the value tracked for the DP register in
> > +        * intel_dp->DP because link_down must not change that (otherwise link
> > +        * re-training will fail. */
> >         dpa_ctl &= ~DP_PLL_ENABLE;
> >         I915_WRITE(DP_A, dpa_ctl);
> >         POSTING_READ(DP_A);
> > @@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >
> >         DRM_DEBUG_KMS("\n");
> >
> > -       if (is_edp(intel_dp)) {
> > -               DP &= ~DP_PLL_ENABLE;
> > -               I915_WRITE(intel_dp->output_reg, DP);
> > -               POSTING_READ(intel_dp->output_reg);
> > -               udelay(100);
> > -       }
> > -

This junk here is the "disable cpu edp pll harder" part.

> >         if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
> >                 DP &= ~DP_LINK_TRAIN_MASK_CPT;
> >                 I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> > @@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >
> >         intel_dp->output_reg = output_reg;
> >         intel_dp->port = port;
> > +       /* Preserve the current hw state. */
> > +       intel_dp->DP = I915_READ(intel_dp->output_reg);
> >
> >         intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> >         if (!intel_connector) {
> > --
> > 1.7.11.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915: robustify edp_pll_on/off

[Daniel Vetter]

On Thu, Sep 13, 2012 at 02:46:19PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > With the previous patch to clean up where exactly these two functions
> > are getting called, this patch can tackle the enable/disable code
> > itself:
> 
> This is the kind of patch that's hard to proof-read. Basically, IMHO,
> the intel_dp->DP variable makes things harder to understand and
> review. Sometimes we use intel_dp->DP directly, sometimes we do "int
> DP = intel_dp->DP" and then just change the temporary DP (sometimes
> not updating the original intel_dp->DP after changing the temporary
> DP), sometimes we read/write directly from intel_dp->output_reg,
> sometimes we pass "int DP" as a function argument instead of storing
> the value inside intel_dp->DP. I have to admit that even after working
> some time with this code I'm not exactly sure why intel_dp->DP is
> really necessary. One of my plans was to try to write a patch that
> removes the variable so I could at least maybe understand why it's
> necessary (the plan was to read/write from/to intel_dp->output_reg
> directly). You can do that if you want :)

Oh, missed this one here a bit. Will blow up - the reason is that we need
to support re-training when the user unplugs, then replugs a dp screen.
Since that works perfectly fine with hdmi, dvi, vga it should work with
dp, too.

But on dp we need to retrain the link, which means disabling, then
re-enabling. Since that potentially clobbers the register, we keep the
value we've computed at ->mode_set time around so that we can reuse it for
the re-training. See my other mail for the exact semantics.

Imo once you've worked with the code long enough, and now that it'll lose
a lot of the ugly hacks and special cases, intel_dp->DP make sense.

Cheers, Daniel
> 
> Complaints aside, here are my comments:
> 
> >
> > - WARN if the port enable bit is in the wrong state or if the edp pll
> >   bit is in the wrong state, just for paranoia's sake.
> 
> Looks good.
> 
> > - Don't disable the edp pll harder in the modeset functions just for
> >   fun.
> 
> Which part of the patch is this message about? Did you mean
> intel_dp_link_down instead of modeset? I could not find anything on
> our documentation saying that the PLL must be disabled when retraining
> the link, so your patch looks correct, but this is the kind of thing
> that we should really try to test somehow :)
> 
> > - Don't set the edp pll enable flag in intel_dp->DP in modeset, do
> >   that while changing the actual hw state. We do the same with the
> >   actual port enable bit, so this is a bit more consistent.
> 
> Looks good.
> 
> > - Track the current DP register value when setting things up and add
> >   some comments how intel_dp->DP is used in the disable code.
> >
> > v2: Be more careful with resetting intel_dp->DP - otherwise dpms
> > off->on will fail spectacularly, becuase we enable the eDP port when
> > we should only enable the eDP pll.
> >
> 
> These 2 chunks are the ones my comment above is about. I'll just trust
> our beloved maintainer on these chunks :)
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index c72d4f3..7c746d1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> >                 intel_dp->DP |= intel_crtc->pipe << 29;
> >
> >                 /* don't miss out required setting for eDP */
> > -               intel_dp->DP |= DP_PLL_ENABLE;
> >                 if (adjusted_mode->clock < 200000)
> >                         intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> >                 else
> > @@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> >
> >                 if (is_cpu_edp(intel_dp)) {
> >                         /* don't miss out required setting for eDP */
> > -                       intel_dp->DP |= DP_PLL_ENABLE;
> >                         if (adjusted_mode->clock < 200000)
> >                                 intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> >                         else
> > @@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> >
> >         DRM_DEBUG_KMS("\n");
> >         dpa_ctl = I915_READ(DP_A);
> > -       dpa_ctl |= DP_PLL_ENABLE;
> > -       I915_WRITE(DP_A, dpa_ctl);
> > +       WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
> > +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> > +
> > +       /* We don't adjust intel_dp->DP while tearing down the link, to
> > +        * facilitate link retraining (e.g. after hotplug). Hence clear all
> > +        * enable bits here to ensure that we don't enable too much. */
> > +       intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> > +       intel_dp->DP |= DP_PLL_ENABLE;
> > +       I915_WRITE(DP_A, intel_dp->DP);
> >         POSTING_READ(DP_A);
> >         udelay(200);
> >  }
> > @@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
> >                              to_intel_crtc(crtc)->pipe);
> >
> >         dpa_ctl = I915_READ(DP_A);
> > +       WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
> > +            "dp pll off, should be on\n");
> > +       WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> > +
> > +       /* We can't rely on the value tracked for the DP register in
> > +        * intel_dp->DP because link_down must not change that (otherwise link
> > +        * re-training will fail. */
> >         dpa_ctl &= ~DP_PLL_ENABLE;
> >         I915_WRITE(DP_A, dpa_ctl);
> >         POSTING_READ(DP_A);
> > @@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >
> >         DRM_DEBUG_KMS("\n");
> >
> > -       if (is_edp(intel_dp)) {
> > -               DP &= ~DP_PLL_ENABLE;
> > -               I915_WRITE(intel_dp->output_reg, DP);
> > -               POSTING_READ(intel_dp->output_reg);
> > -               udelay(100);
> > -       }
> > -
> >         if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
> >                 DP &= ~DP_LINK_TRAIN_MASK_CPT;
> >                 I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> > @@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >
> >         intel_dp->output_reg = output_reg;
> >         intel_dp->port = port;
> > +       /* Preserve the current hw state. */
> > +       intel_dp->DP = I915_READ(intel_dp->output_reg);
> >
> >         intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> >         if (!intel_connector) {
> > --
> > 1.7.11.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe

[Paulo Zanoni]

2012/9/13 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Sep 13, 2012 at 04:11:20PM -0300, Paulo Zanoni wrote:
>> Hi
>>
>> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This
>> > applies to all platforms where we currently support eDP on, i.e. ilk,
>> > snb & ivb.
>> >
>>
>> Ok, so I looked at BSpec and the conclusion is: shouldn't we do this
>> for eDP _and_ DP instead of just eDP? The only things to disable
>> before the crtc are audio, the panel backlight, and then the panel
>> power.
>>
>> Your patch looks correct, but maybe it could be even more correct by
>> including DP too? Yes, I can help testing.
>
> The dp pll for cpu edp is special, since we set it in the DP_A register.
> The pll for pch dp ports is just the regular pch pll iirc, and that is
> handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was
> definitely broken.
>
> In any case, frobbing the pch dp sequence would be a separate patch imo.
> So can I still have an r-b for this on here?

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> -Daniel
>
>>
>> > Without this change we fail to light up the eDP port on previously
>> > unused crtcs (likely because something is stuck on the old pipe), and
>> > we also fail to properly disable the old pipe (i.e. bit 30 in the
>> > PIPECONF register is stuck as set until the next reboot).
>> >
>> > v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2.
>> >
>> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001
>> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 07f9521..f28353d 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder)
>> >         ironlake_edp_backlight_off(intel_dp);
>> >         intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>> >         ironlake_edp_panel_off(intel_dp);
>> > -       intel_dp_link_down(intel_dp);
>> > +
>> > +       /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
>> > +       if (!is_cpu_edp(intel_dp))
>> > +               intel_dp_link_down(intel_dp);
>> >  }
>> >
>> >  static void intel_post_disable_dp(struct intel_encoder *encoder)
>> >  {
>> >         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>> >
>> > -       if (is_cpu_edp(intel_dp))
>> > +       if (is_cpu_edp(intel_dp)) {
>> > +               intel_dp_link_down(intel_dp);
>> >                 ironlake_edp_pll_off(intel_dp);
>> > +       }
>> >  }
>> >
>> >  static void intel_enable_dp(struct intel_encoder *encoder)
>> > --
>> > 1.7.11.2
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe

[Daniel Vetter]

On Thu, Sep 13, 2012 at 04:43:57PM -0300, Paulo Zanoni wrote:
> 2012/9/13 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Sep 13, 2012 at 04:11:20PM -0300, Paulo Zanoni wrote:
> >> Hi
> >>
> >> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>:
> >> > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This
> >> > applies to all platforms where we currently support eDP on, i.e. ilk,
> >> > snb & ivb.
> >> >
> >>
> >> Ok, so I looked at BSpec and the conclusion is: shouldn't we do this
> >> for eDP _and_ DP instead of just eDP? The only things to disable
> >> before the crtc are audio, the panel backlight, and then the panel
> >> power.
> >>
> >> Your patch looks correct, but maybe it could be even more correct by
> >> including DP too? Yes, I can help testing.
> >
> > The dp pll for cpu edp is special, since we set it in the DP_A register.
> > The pll for pch dp ports is just the regular pch pll iirc, and that is
> > handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was
> > definitely broken.
> >
> > In any case, frobbing the pch dp sequence would be a separate patch imo.
> > So can I still have an r-b for this on here?
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I've slurped in the other 3 patches, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch