[PATCH 00/17] drm/i915: Fix vlv/chv panel power sequencer

[PATCH 00/17] drm/i915: Fix vlv/chv panel power sequencer

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

After weeks or months of beating on the hardware I finally managed
to figure out how to kick the vlv/chv power sequencer in a reasonably
light way after changing the pipe<->port mapping.

Contrary to my earlier comments we don't actually need to kick it
when dealing with regular DP ports. I'm not sure if my previous BSW
was just a bit funky or if I just overlooked something because I clearly
remember having to do that. Anyway now it seems enough to kick eDP
ports only and with the refined kick procedure (which doesn't involve
panel power on+off cycles anymore) it should be a reasonably fast
operation too. The power sequencer on one pipe can control any eDP
port even if that pipe otherwise is driving another non-eDP port.

I've tested this this on BYT eDP+DP, BSW eDP+DP, ILK eDP+DP, IVB DP
and HSW eDP and everything appears to working perfectly now. I've
been beating on it for a few days now trying all kinds combinations
of pipes and ports and swapping them around in various ways.

I also simulated a dual eDP system on the BYT and BSW machines by
making the code pretend that the external DP port is also eDP. That
way I could test that the power sequencer code really would work
in dual eDP machines. Obviously I was unable to test that VDD would
actually get enabled for both ports since one of them was DP but
at least the registers pretend that is, and the power sequencer
didn't cause the port to gets stuck at any point, and the AUX stuff
worked every single time on the actual eDP port so at least there
VDD got applied appropriately.

So I'm quite confident that this vlv/chv power seqeuencer problem
is now solved. Well, until someone breaks it that is. I'm not sure
we can do any kind of satisfactory tests for this stuff since it
depends on eDP and some of trickier interaction even requires dual
eDP. Also some of the failures can manifest as something as benign
as failed OUI reads which doesn't even flag an error in dmesg. So
I don't have particularly good ideas on how we can prevent regressions
to this code.

Ville Syrjälä (17):
  drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv
  drm/i915: Warn if stealing power sequencer from an active eDP port
  drm/i915: Remove high level intel_edp_vdd_{on,off}() from hpd/detect
  drm/i915: Store power sequencer delays in intel_dp
  drm/i915: Don't initialize power seqeuencer delays more than once
  drm/i915: Split power sequencer panel on/off functions to locked and
    unlocked variants
  drm/i915: Hold the pps mutex across the whole panel power enable
    sequence
  drm/i915: Wait for PHY port ready before link training on VLV/CHV
  drm/i915: Fix eDP link training when switching pipes on VLV/CHV
  drm/i915: Kick the power sequencer before AUX transactions
  drm/i915: Make sure DPLL is enabled when kicking the power sequencer
    on VLV/CHV
  drm/i915: Don't kick the power seqeuncer just to check if we have
    vdd/panel power
  drm/i915: Clear PPS port select when giving up the power sequencer
  drm/i915: Warn if stealing non pipe A/B power sequencer
  drm/i915: Steal power sequencer in vlv_power_sequencer_pipe()
  drm/i915: Improve VDD/PPS debugs
  drm/i915: Warn if panel power is already on when enabling it

 drivers/gpu/drm/i915/intel_display.c |  84 +++++----
 drivers/gpu/drm/i915/intel_dp.c      | 347 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  16 ++
 3 files changed, 304 insertions(+), 143 deletions(-)

-- 
2.0.4

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

[PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Only ports B and C have the power sequencer and backlight controls,
so complain if we ever try to register an eDP connector on some other
port.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4455009..de919e9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	if (type == DRM_MODE_CONNECTOR_eDP)
 		intel_encoder->type = INTEL_OUTPUT_EDP;
 
+	/* eDP only on port B and/or C on vlv/chv */
+	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
+		    port != PORT_B && port != PORT_C))
+		return false;
+
 	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
 			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
 			port_name(port));
-- 
2.0.4

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

[PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

eDP ports need the power seqeuncer whenever the port is active. Warn if
we accidentally steal the power sequener from an active eDP port. This
should not happen unless there's a bug somewhere else, but it's best to
scream loudly if it happens to help with debugging.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index de919e9..7ac5c47 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
+		WARN(encoder->connectors_active,
+		     "stealing pipe %c power sequencer from active eDP port %c\n",
+		     pipe_name(pipe), port_name(port));
+
 		/* make sure vdd is off before we steal it */
 		edp_panel_vdd_off_sync(intel_dp);
 
-- 
2.0.4

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

[PATCH 03/17] drm/i915: Remove high level intel_edp_vdd_{on, off}() from hpd/detect

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

want_panel_vdd is a bool so it can't cope with interleaving on/off calls
from multiple threads. If we want to make that possible we'd need to
convert want_panel_vdd into a proper ref count. But an easier fix is to
remove the high level vdd on/off calls from detect/hpd code paths and
just rely on the delayed vdd off to avoid needless vdd on<->off ping
pong.

After this change only the encoder enable/disable paths use the high
level functions, which is fine since both the on and off low level edp
vdd calls from intel_dp_aux_ch() happen without dropping pps_mutex in
between and so want_panel_vdd can't change in between.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7ac5c47..13c3a9b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3857,8 +3857,6 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	intel_edp_panel_vdd_on(intel_dp);
-
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
@@ -3866,8 +3864,6 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
-
-	intel_edp_panel_vdd_off(intel_dp, false);
 }
 
 static bool
@@ -3881,7 +3877,6 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
 		return false;
 
-	intel_edp_panel_vdd_on(intel_dp);
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
 		if (buf[0] & DP_MST_CAP) {
 			DRM_DEBUG_KMS("Sink is MST capable\n");
@@ -3891,7 +3886,6 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 			intel_dp->is_mst = false;
 		}
 	}
-	intel_edp_panel_vdd_off(intel_dp, false);
 
 	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
 	return intel_dp->is_mst;
@@ -5109,9 +5103,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	intel_edp_panel_vdd_sanitize(intel_encoder);
 
 	/* Cache DPCD and EDID for edp. */
-	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);
-	intel_edp_panel_vdd_off(intel_dp, false);
 
 	if (has_dpcd) {
 		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
-- 
2.0.4

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

[PATCH 04/17] drm/i915: Store power sequencer delays in intel_dp

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The power seqeuncer delays are fixed for a given panel, so we can keep
them around once computed.

Not that on VLV/CHV we still re-compute them every time we initialize
the power seqeuncer registers, but that will change soon enough.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 52 +++++++++++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 13c3a9b..7a10464 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -283,12 +283,10 @@ intel_hrawclk(struct drm_device *dev)
 
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out);
+				    struct intel_dp *intel_dp);
 static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *out);
+					      struct intel_dp *intel_dp);
 
 static void pps_lock(struct intel_dp *intel_dp)
 {
@@ -330,7 +328,6 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
 	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
-	struct edp_power_seq power_seq;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
@@ -368,9 +365,8 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 		      port_name(intel_dig_port->port));
 
 	/* init power sequencer on this pipe and port */
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
+	intel_dp_init_panel_power_sequencer(dev, intel_dp);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 
 	return intel_dp->pps_pipe;
 }
@@ -425,7 +421,6 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edp_power_seq power_seq;
 	enum port port = intel_dig_port->port;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
@@ -453,9 +448,8 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("initial power sequencer for port %c: pipe %c\n",
 		      port_name(port), pipe_name(intel_dp->pps_pipe));
 
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
+	intel_dp_init_panel_power_sequencer(dev, intel_dp);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 }
 
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv)
@@ -2624,7 +2618,6 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-	struct edp_power_seq power_seq;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
@@ -2652,9 +2645,8 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 		      pipe_name(intel_dp->pps_pipe), port_name(intel_dig_port->port));
 
 	/* init power sequencer on this pipe and port */
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
+	intel_dp_init_panel_power_sequencer(dev, intel_dp);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 }
 
 static void vlv_pre_enable_dp(struct intel_encoder *encoder)
@@ -4754,11 +4746,11 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
 
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out)
+				    struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edp_power_seq cur, vbt, spec, final;
+	struct edp_power_seq cur, vbt, spec,
+		*final = &intel_dp->pps_delays;
 	u32 pp_on, pp_off, pp_div, pp;
 	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
 
@@ -4825,7 +4817,7 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 
 	/* Use the max of the register settings and vbt. If both are
 	 * unset, fall back to the spec limits. */
-#define assign_final(field)	final.field = (max(cur.field, vbt.field) == 0 ? \
+#define assign_final(field)	final->field = (max(cur.field, vbt.field) == 0 ? \
 				       spec.field : \
 				       max(cur.field, vbt.field))
 	assign_final(t1_t3);
@@ -4835,7 +4827,7 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 	assign_final(t11_t12);
 #undef assign_final
 
-#define get_delay(field)	(DIV_ROUND_UP(final.field, 10))
+#define get_delay(field)	(DIV_ROUND_UP(final->field, 10))
 	intel_dp->panel_power_up_delay = get_delay(t1_t3);
 	intel_dp->backlight_on_delay = get_delay(t8);
 	intel_dp->backlight_off_delay = get_delay(t9);
@@ -4849,21 +4841,18 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 
 	DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n",
 		      intel_dp->backlight_on_delay, intel_dp->backlight_off_delay);
-
-	if (out)
-		*out = final;
 }
 
 static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *seq)
+					      struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp_on, pp_off, pp_div, port_sel = 0;
 	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
 	int pp_on_reg, pp_off_reg, pp_div_reg;
 	enum port port = dp_to_dig_port(intel_dp)->port;
+	const struct edp_power_seq *seq = &intel_dp->pps_delays;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
@@ -5081,8 +5070,7 @@ void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder)
 }
 
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
-				     struct intel_connector *intel_connector,
-				     struct edp_power_seq *power_seq)
+				     struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -5118,7 +5106,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* We now know it's not a ghost, init power sequence regs. */
 	pps_lock(intel_dp);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 	pps_unlock(intel_dp);
 
 	mutex_lock(&dev->mode_config.mutex);
@@ -5179,7 +5167,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
-	struct edp_power_seq power_seq = { 0 };
 	int type;
 
 	intel_dp->pps_pipe = INVALID_PIPE;
@@ -5269,8 +5256,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			vlv_initial_power_sequencer_setup(intel_dp);
 		} else {
 			intel_dp_init_panel_power_timestamps(intel_dp);
-			intel_dp_init_panel_power_sequencer(dev, intel_dp,
-							    &power_seq);
+			intel_dp_init_panel_power_sequencer(dev, intel_dp);
 		}
 		pps_unlock(intel_dp);
 	}
@@ -5285,7 +5271,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		}
 	}
 
-	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
+	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
 		drm_dp_aux_unregister(&intel_dp->aux);
 		if (is_edp(intel_dp)) {
 			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5ab813c..f7ba1fc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -590,6 +590,7 @@ struct intel_dp {
 	 * this port. Only relevant on VLV/CHV.
 	 */
 	enum pipe pps_pipe;
+	struct edp_power_seq pps_delays;
 
 	bool use_tps3;
 	bool can_mst; /* this port supports mst */
-- 
2.0.4

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

[PATCH 05/17] drm/i915: Don't initialize power seqeuencer delays more than once

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since we read the current power seqeuncer delays from the registers
(as well as looking at the vbt and spec values) we may end up
corrupting delays we already initialized when we switch to another
pipe and the power seqeuncer there has different values currently
in the registers.

So make sure we only initialize the delays once even if
intel_dp_init_panel_power_sequencer() gets called multiple times.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7a10464..9a1295d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4756,6 +4756,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	/* already initialized? */
+	if (final->t11_t12 != 0)
+		return;
+
 	if (HAS_PCH_SPLIT(dev)) {
 		pp_ctrl_reg = PCH_PP_CONTROL;
 		pp_on_reg = PCH_PP_ON_DELAYS;
-- 
2.0.4

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

[PATCH 06/17] drm/i915: Split power sequencer panel on/off functions to locked and unlocked variants

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We'll be needing to the call the power seqeuencer functions while
already holding pps_mutex, so split the locking out to small wrapper
functions.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9a1295d..2168568 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1555,23 +1555,23 @@ static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	pps_unlock(intel_dp);
 }
 
-void intel_edp_panel_on(struct intel_dp *intel_dp)
+static void edp_panel_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 	u32 pp_ctrl_reg;
 
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
 	if (!is_edp(intel_dp))
 		return;
 
 	DRM_DEBUG_KMS("Turn eDP power on\n");
 
-	pps_lock(intel_dp);
-
 	if (edp_have_panel_power(intel_dp)) {
 		DRM_DEBUG_KMS("eDP power already on\n");
-		goto out;
+		return;
 	}
 
 	wait_panel_power_cycle(intel_dp);
@@ -1600,12 +1600,20 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
 		I915_WRITE(pp_ctrl_reg, pp);
 		POSTING_READ(pp_ctrl_reg);
 	}
+}
 
- out:
+void intel_edp_panel_on(struct intel_dp *intel_dp)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	pps_lock(intel_dp);
+	edp_panel_on(intel_dp);
 	pps_unlock(intel_dp);
 }
 
-void intel_edp_panel_off(struct intel_dp *intel_dp)
+
+static void edp_panel_off(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -1615,13 +1623,13 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	u32 pp;
 	u32 pp_ctrl_reg;
 
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
 	if (!is_edp(intel_dp))
 		return;
 
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
-	pps_lock(intel_dp);
-
 	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
 
 	pp = ironlake_get_pp_control(intel_dp);
@@ -1643,7 +1651,15 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	/* We got a reference when we enabled the VDD. */
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_put(dev_priv, power_domain);
+}
 
+void intel_edp_panel_off(struct intel_dp *intel_dp)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	pps_lock(intel_dp);
+	edp_panel_off(intel_dp);
 	pps_unlock(intel_dp);
 }
 
-- 
2.0.4

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

[PATCH 07/17] drm/i915: Hold the pps mutex across the whole panel power enable sequence

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Just grab the pps_mutex once and do all the pps panel startup operations
while holding the mutex instead of grabbing the mutex separately for
each individual step.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 47 ++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2168568..4c8f169 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -113,6 +113,7 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
 static void intel_dp_link_down(struct intel_dp *intel_dp);
 static bool edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
+static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
 
 int
 intel_dp_max_link_bw(struct intel_dp *intel_dp)
@@ -1539,22 +1540,6 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 		edp_panel_vdd_schedule_off(intel_dp);
 }
 
-/*
- * Must be paired with intel_edp_panel_vdd_on().
- * Nested calls to these functions are not allowed since
- * we drop the lock. Caller must use some higher level
- * locking to prevent nested calls from other threads.
- */
-static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
-{
-	if (!is_edp(intel_dp))
-		return;
-
-	pps_lock(intel_dp);
-	edp_panel_vdd_off(intel_dp, sync);
-	pps_unlock(intel_dp);
-}
-
 static void edp_panel_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -2552,10 +2537,19 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	if (WARN_ON(dp_reg & DP_PORT_EN))
 		return;
 
+	pps_lock(intel_dp);
+
+	if (IS_VALLEYVIEW(dev))
+		vlv_init_panel_power_sequencer(intel_dp);
+
 	intel_dp_enable_port(intel_dp);
-	intel_edp_panel_vdd_on(intel_dp);
-	intel_edp_panel_on(intel_dp);
-	intel_edp_panel_vdd_off(intel_dp, true);
+
+	edp_panel_vdd_on(intel_dp);
+	edp_panel_on(intel_dp);
+	edp_panel_vdd_off(intel_dp, true);
+
+	pps_unlock(intel_dp);
+
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_start_link_train(intel_dp);
 	intel_dp_complete_link_train(intel_dp);
@@ -2637,6 +2631,9 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	if (!is_edp(intel_dp))
+		return;
+
 	if (intel_dp->pps_pipe == crtc->pipe)
 		return;
 
@@ -2691,12 +2688,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
 
 	mutex_unlock(&dev_priv->dpio_lock);
 
-	if (is_edp(intel_dp)) {
-		pps_lock(intel_dp);
-		vlv_init_panel_power_sequencer(intel_dp);
-		pps_unlock(intel_dp);
-	}
-
 	intel_enable_dp(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport);
@@ -2791,12 +2782,6 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
 
 	mutex_unlock(&dev_priv->dpio_lock);
 
-	if (is_edp(intel_dp)) {
-		pps_lock(intel_dp);
-		vlv_init_panel_power_sequencer(intel_dp);
-		pps_unlock(intel_dp);
-	}
-
 	intel_enable_dp(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport);
-- 
2.0.4

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

[PATCH 08/17] drm/i915: Wait for PHY port ready before link training on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no point in checking if the data lanes came out of reset after
link training. If the data lanes aren't ready link training will fail
anyway.

Suggested-by: Todd Previte <tprevite@gmail.com>
Cc: Todd Previte <tprevite@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
If Todd has a better patch with better description we can use that one
instead of my version. But at least I think the spot where I put the
vlv_wait_port_ready() is the right one. We could perhaps skip the link
training attempt entirely if the port is already stuck.

 drivers/gpu/drm/i915/intel_dp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4c8f169..6f568b4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2550,6 +2550,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 
 	pps_unlock(intel_dp);
 
+	if (IS_VALLEYVIEW(dev))
+		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
+
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_start_link_train(intel_dp);
 	intel_dp_complete_link_train(intel_dp);
@@ -2689,8 +2692,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
 	mutex_unlock(&dev_priv->dpio_lock);
 
 	intel_enable_dp(encoder);
-
-	vlv_wait_port_ready(dev_priv, dport);
 }
 
 static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
@@ -2783,8 +2784,6 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
 	mutex_unlock(&dev_priv->dpio_lock);
 
 	intel_enable_dp(encoder);
-
-	vlv_wait_port_ready(dev_priv, dport);
 }
 
 static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
-- 
2.0.4

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

[PATCH 09/17] drm/i915: Fix eDP link training when switching pipes on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When switching from one pipe to another, the power sequencer of the new
pipe seems to need a bit of kicking to lock into the port. Even the vdd
force bit doesn't work before the power sequencer has been sufficiently
kicked, so this must be done before any AUX transactions are attempted.

After extensive experimentation I've determined that it's sufficient
to first write the port register with the correct values except the
port must remain disabled, then we can do a second write to enable the
port, after which the power sequencer is operational and allows the port
to start up properly.

Contrary to my earlier theories we don't need to enable the port with
the idle pattern, so let's just use training pattern 1 as that's what
other platforms use here.

v2: Refine the kick procedure

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6f568b4..81a740b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2517,14 +2517,23 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	intel_dp->DP |= DP_PORT_EN;
-
 	/* enable with pattern 1 (as per spec) */
 	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
 				 DP_TRAINING_PATTERN_1);
 
 	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
+
+	/*
+	 * Magic for VLV/CHV. We _must_ first set up the register
+	 * without actually enabling the port, and then do another
+	 * write to enable the port. Otherwise link training will
+	 * fail when the power sequencer is freshly used for this port.
+	 */
+	intel_dp->DP |= DP_PORT_EN;
+
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
+	POSTING_READ(intel_dp->output_reg);
 }
 
 static void intel_enable_dp(struct intel_encoder *encoder)
-- 
2.0.4

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

[PATCH 10/17] drm/i915: Kick the power sequencer before AUX transactions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we pick a new power sequencer for the port but we're not doing a
full modeset, the power sequencer may have locked on to another port (or
no port). So kick it a bit to make sure it controls the port we want.

Again just like when we attempt to actually enable the DP port, we
must first write the port register with the approriate value except
the enable bit, and then we must enable the port to make the power
sequencer happy. In this case since we don't want the port actually
enabled we just toggle it on and immediately back off. Going forward
the power sequencer will keep working on that specific port until again
moved to another port.

v2: Refine the kick procedure

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81a740b..dcb87c8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -321,6 +321,52 @@ static void pps_unlock(struct intel_dp *intel_dp)
 	intel_display_power_put(dev_priv, power_domain);
 }
 
+static void
+vlv_power_sequencer_kick(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = intel_dp->pps_pipe;
+	uint32_t DP;
+
+	if (WARN(I915_READ(intel_dp->output_reg) & DP_PORT_EN,
+		 "skipping pipe %c power seqeuncer kick due to port %c being active\n",
+		 pipe_name(pipe), port_name(intel_dig_port->port)))
+		return;
+
+	DRM_DEBUG_KMS("kicking pipe %c power sequencer for port %c\n",
+		      pipe_name(pipe), port_name(intel_dig_port->port));
+
+	/* Preserve the BIOS-computed detected bit. This is
+	 * supposed to be read-only.
+	 */
+	DP = I915_READ(intel_dp->output_reg) & DP_DETECTED;
+	DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
+	DP |= DP_PORT_WIDTH(1);
+	DP |= DP_LINK_TRAIN_PAT_1;
+
+	if (IS_CHERRYVIEW(dev))
+		DP |= DP_PIPE_SELECT_CHV(pipe);
+	else if (pipe == PIPE_B)
+		DP |= DP_PIPEB_SELECT;
+
+	/*
+	 * Similar magic as in intel_dp_enable_port().
+	 * We _must_ do this port enable + disable trick
+	 * to make this power seqeuencer lock onto the port.
+	 * Otherwise even VDD force bit won't work.
+	 */
+	I915_WRITE(intel_dp->output_reg, DP);
+	POSTING_READ(intel_dp->output_reg);
+
+	I915_WRITE(intel_dp->output_reg, DP | DP_PORT_EN);
+	POSTING_READ(intel_dp->output_reg);
+
+	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
+	POSTING_READ(intel_dp->output_reg);
+}
+
 static enum pipe
 vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 {
@@ -369,6 +415,12 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	intel_dp_init_panel_power_sequencer(dev, intel_dp);
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 
+	/*
+	 * Even vdd force doesn't work until we've made
+	 * the power sequencer lock in on the port.
+	 */
+	vlv_power_sequencer_kick(intel_dp);
+
 	return intel_dp->pps_pipe;
 }
 
-- 
2.0.4

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

[PATCH 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The power seqeuencer kick procedure requires the DPLL to be running
in order to complete succesfully. In case the DPLL isn't currently
running when we need to kick the power seqeuncer enable it
temporarily. This can happen eg. during ->detect() when the pipe is
not already active.

To avoid needlessly duplicating the DPLL programming re-use the already
existing functions by passing a temporary pipe config to them instead
of having them consult the current pipe config at crtc->config.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 84 +++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_dp.c      | 37 ++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     | 15 +++++++
 3 files changed, 96 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d901961..7e70b85 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -94,8 +94,6 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 static void ironlake_set_pipeconf(struct drm_crtc *crtc);
 static void haswell_set_pipeconf(struct drm_crtc *crtc);
 static void intel_set_pipe_csc(struct drm_crtc *crtc);
-static void vlv_prepare_pll(struct intel_crtc *crtc);
-static void chv_prepare_pll(struct intel_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -1484,12 +1482,13 @@ static void intel_init_dpio(struct drm_device *dev)
 	}
 }
 
-static void vlv_enable_pll(struct intel_crtc *crtc)
+void vlv_enable_pll(struct intel_crtc *crtc,
+		    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int reg = DPLL(crtc->pipe);
-	u32 dpll = crtc->config.dpll_hw_state.dpll;
+	u32 dpll = pipe_config->dpll_hw_state.dpll;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 
@@ -1507,7 +1506,7 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	if (wait_for(((I915_READ(reg) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("DPLL %d failed to lock\n", crtc->pipe);
 
-	I915_WRITE(DPLL_MD(crtc->pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(crtc->pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(crtc->pipe));
 
 	/* We do this three times for luck */
@@ -1522,7 +1521,8 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	udelay(150); /* wait for warmup */
 }
 
-static void chv_enable_pll(struct intel_crtc *crtc)
+void chv_enable_pll(struct intel_crtc *crtc,
+		    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1547,14 +1547,14 @@ static void chv_enable_pll(struct intel_crtc *crtc)
 	udelay(1);
 
 	/* Enable PLL */
-	I915_WRITE(DPLL(pipe), crtc->config.dpll_hw_state.dpll);
+	I915_WRITE(DPLL(pipe), pipe_config->dpll_hw_state.dpll);
 
 	/* Check PLL is locked */
 	if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("PLL %d failed to lock\n", pipe);
 
 	/* not sure when this should be written */
-	I915_WRITE(DPLL_MD(pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(pipe));
 
 	mutex_unlock(&dev_priv->dpio_lock);
@@ -1666,7 +1666,7 @@ static void i9xx_disable_pll(struct intel_crtc *crtc)
 	POSTING_READ(DPLL(pipe));
 }
 
-static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
 	u32 val = 0;
 
@@ -1684,7 +1684,7 @@ static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 }
 
-static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
 	enum dpio_channel port = vlv_pipe_to_channel(pipe);
 	u32 val;
@@ -4844,9 +4844,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_prepare_pll(intel_crtc);
+			chv_prepare_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_prepare_pll(intel_crtc);
+			vlv_prepare_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	if (intel_crtc->config.has_dp_encoder)
@@ -4873,9 +4873,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_enable_pll(intel_crtc);
+			chv_enable_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_enable_pll(intel_crtc);
+			vlv_enable_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -5756,7 +5756,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc)
 						   &crtc->config.dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc)
+void vlv_update_pll(struct intel_crtc *crtc,
+		    struct intel_crtc_config *pipe_config)
 {
 	u32 dpll, dpll_md;
 
@@ -5771,14 +5772,15 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	if (crtc->pipe == PIPE_B)
 		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	pipe_config->dpll_hw_state.dpll = dpll;
 
-	dpll_md = (crtc->config.pixel_multiplier - 1)
+	dpll_md = (pipe_config->pixel_multiplier - 1)
 		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-	crtc->config.dpll_hw_state.dpll_md = dpll_md;
+	pipe_config->dpll_hw_state.dpll_md = dpll_md;
 }
 
-static void vlv_prepare_pll(struct intel_crtc *crtc)
+void vlv_prepare_pll(struct intel_crtc *crtc,
+		     const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5789,11 +5791,11 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 
 	mutex_lock(&dev_priv->dpio_lock);
 
-	bestn = crtc->config.dpll.n;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/* See eDP HDMI DPIO driver vbios notes doc */
 
@@ -5830,7 +5832,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW3(pipe), mdiv);
 
 	/* Set HBR and RBR LPF coefficients */
-	if (crtc->config.port_clock == 162000 ||
+	if (pipe_config->port_clock == 162000 ||
 	    intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_ANALOG) ||
 	    intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
 		vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe),
@@ -5869,19 +5871,21 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc)
+void chv_update_pll(struct intel_crtc *crtc,
+		    struct intel_crtc_config *pipe_config)
 {
-	crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
+	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
 		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
 		DPLL_VCO_ENABLE;
 	if (crtc->pipe != PIPE_A)
-		crtc->config.dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	crtc->config.dpll_hw_state.dpll_md =
-		(crtc->config.pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
+	pipe_config->dpll_hw_state.dpll_md =
+		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
 }
 
-static void chv_prepare_pll(struct intel_crtc *crtc)
+void chv_prepare_pll(struct intel_crtc *crtc,
+		     const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5892,18 +5896,18 @@ static void chv_prepare_pll(struct intel_crtc *crtc)
 	u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
 	int refclk;
 
-	bestn = crtc->config.dpll.n;
-	bestm2_frac = crtc->config.dpll.m2 & 0x3fffff;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2 >> 22;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm2_frac = pipe_config->dpll.m2 & 0x3fffff;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2 >> 22;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/*
 	 * Enable Refclk and SSC
 	 */
 	I915_WRITE(dpll_reg,
-		   crtc->config.dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
+		   pipe_config->dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
 
 	mutex_lock(&dev_priv->dpio_lock);
 
@@ -6331,9 +6335,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	} else if (IS_CHERRYVIEW(dev)) {
-		chv_update_pll(intel_crtc);
+		chv_update_pll(intel_crtc, &intel_crtc->config);
 	} else if (IS_VALLEYVIEW(dev)) {
-		vlv_update_pll(intel_crtc);
+		vlv_update_pll(intel_crtc, &intel_crtc->config);
 	} else {
 		i9xx_update_pll(intel_crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dcb87c8..74cf827 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -328,6 +328,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe = intel_dp->pps_pipe;
+	bool pll_enabled;
 	uint32_t DP;
 
 	if (WARN(I915_READ(intel_dp->output_reg) & DP_PORT_EN,
@@ -351,6 +352,35 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else if (pipe == PIPE_B)
 		DP |= DP_PIPEB_SELECT;
 
+	pll_enabled = I915_READ(DPLL(pipe)) & DPLL_VCO_ENABLE;
+
+	/*
+	 * The DPLL for the pipe must be enabled for this to work.
+	 * So enable temporarily it if it's not already enabled.
+	 */
+	if (!pll_enabled) {
+		struct intel_crtc *crtc =
+			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+		struct intel_crtc_config pipe_config = {
+			.pixel_multiplier = 1,
+			.dpll = IS_CHERRYVIEW(dev) ?
+				chv_dpll[0].dpll : vlv_dpll[0].dpll,
+			.port_clock = 162000,
+			.clock_set = true,
+			.has_dp_encoder = true,
+		};
+
+		if (IS_CHERRYVIEW(dev)) {
+			chv_update_pll(crtc, &pipe_config);
+			chv_prepare_pll(crtc, &pipe_config);
+			chv_enable_pll(crtc, &pipe_config);
+		} else {
+			vlv_update_pll(crtc, &pipe_config);
+			vlv_prepare_pll(crtc, &pipe_config);
+			vlv_enable_pll(crtc, &pipe_config);
+		}
+	}
+
 	/*
 	 * Similar magic as in intel_dp_enable_port().
 	 * We _must_ do this port enable + disable trick
@@ -365,6 +395,13 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 
 	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
 	POSTING_READ(intel_dp->output_reg);
+
+	if (!pll_enabled) {
+		if (IS_CHERRYVIEW(dev))
+			chv_disable_pll(dev_priv, pipe);
+		else
+			vlv_disable_pll(dev_priv, pipe);
+	}
 }
 
 static enum pipe
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f7ba1fc..69c8c5f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -911,6 +911,21 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
 void intel_put_shared_dpll(struct intel_crtc *crtc);
 
+void vlv_update_pll(struct intel_crtc *crtc,
+		    struct intel_crtc_config *pipe_config);
+void chv_update_pll(struct intel_crtc *crtc,
+		    struct intel_crtc_config *pipe_config);
+void vlv_prepare_pll(struct intel_crtc *crtc,
+		     const struct intel_crtc_config *pipe_config);
+void chv_prepare_pll(struct intel_crtc *crtc,
+		     const struct intel_crtc_config *pipe_config);
+void vlv_enable_pll(struct intel_crtc *crtc,
+		    const struct intel_crtc_config *pipe_config);
+void chv_enable_pll(struct intel_crtc *crtc,
+		    const struct intel_crtc_config *pipe_config);
+void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
+void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
+
 /* modesetting asserts */
 void assert_panel_unlocked(struct drm_i915_private *dev_priv,
 			   enum pipe pipe);
-- 
2.0.4

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

[PATCH 12/17] drm/i915: Don't kick the power seqeuncer just to check if we have vdd/panel power

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If there's no power sequencer assigned to the port currently we can't
very well have vdd or panel power enabled either. If we would try to
check that from the pps registers we'd need to pick a power seqeuncer
and kick it. So let's skip the register read and the kick.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 74cf827..c9a1600 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -634,6 +634,10 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	if (IS_VALLEYVIEW(dev) &&
+	    intel_dp->pps_pipe == INVALID_PIPE)
+		return false;
+
 	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
 }
 
@@ -644,6 +648,10 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	if (IS_VALLEYVIEW(dev) &&
+	    intel_dp->pps_pipe == INVALID_PIPE)
+		return false;
+
 	return I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
 }
 
-- 
2.0.4

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

[PATCH 13/17] drm/i915: Clear PPS port select when giving up the power sequencer

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VLV gets confused if two power sequencers have the same port selected.
It would seem the port doesn't start up properly in the is case and
vlv_wait_port_ready() will fail as will the link training. Clearing the
port select in the PP_ON_DELAYS register fixes this problem.

CHV doesn't seem to need this, but it doesn't seem to hurt either so
let's just do it for both to keep the code between the platforms as
uniform as possible.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c9a1600..103468c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2694,6 +2694,32 @@ static void g4x_pre_enable_dp(struct intel_encoder *encoder)
 	}
 }
 
+static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = intel_dig_port->base.base.dev->dev_private;
+	enum pipe pipe = intel_dp->pps_pipe;
+	int pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
+
+	edp_panel_vdd_off_sync(intel_dp);
+
+	/*
+	 * VLV seems to get confused when multiple power seqeuencers
+	 * have the same port selected (even if only one has power/vdd
+	 * enabled). The failure manifests as vlv_wait_port_ready() failing
+	 * CHV on the other hand doesn't seem to mind having the same port
+	 * selected in multiple power seqeuencers, but let's clear the
+	 * port select always when logically disconnecting a power sequencer
+	 * from a port.
+	 */
+	DRM_DEBUG_KMS("detaching pipe %c power sequencer from port %c\n",
+		      pipe_name(pipe), port_name(intel_dig_port->port));
+	I915_WRITE(pp_on_reg, 0);
+	POSTING_READ(pp_on_reg);
+
+	intel_dp->pps_pipe = INVALID_PIPE;
+}
+
 static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe)
 {
@@ -2724,9 +2750,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		     pipe_name(pipe), port_name(port));
 
 		/* make sure vdd is off before we steal it */
-		edp_panel_vdd_off_sync(intel_dp);
-
-		intel_dp->pps_pipe = INVALID_PIPE;
+		vlv_detach_power_sequencer(intel_dp);
 	}
 }
 
@@ -2752,7 +2776,7 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 	 * we still have control of it.
 	 */
 	if (intel_dp->pps_pipe != INVALID_PIPE)
-		edp_panel_vdd_off_sync(intel_dp);
+		vlv_detach_power_sequencer(intel_dp);
 
 	/*
 	 * We may be stealing the power
-- 
2.0.4

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

[PATCH 14/17] drm/i915: Warn if stealing non pipe A/B power sequencer

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no power sequencer on pipe C on VLV/CHV so scream a bit if we
try to steal one from pipes other than A and B.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 103468c..74a6514 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2728,6 +2728,9 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
 			    base.head) {
 		struct intel_dp *intel_dp;
-- 
2.0.4

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

[PATCH 15/17] drm/i915: Steal power sequencer in vlv_power_sequencer_pipe()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In case we fumble something and end up picking an already used power
seqeuencer in vlv_power_sequencer_pipe() at least try to steal it
gracefully. In theory this should never happen though.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 74a6514..b58c94f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -114,6 +114,8 @@ static void intel_dp_link_down(struct intel_dp *intel_dp);
 static bool edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
+static void vlv_steal_power_sequencer(struct drm_device *dev,
+				      enum pipe pipe);
 
 int
 intel_dp_max_link_bw(struct intel_dp *intel_dp)
@@ -412,9 +414,13 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
 	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
+	enum pipe pipe;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
+	/* We should never land here with regular DP ports */
+	WARN_ON(!is_edp(intel_dp));
+
 	if (intel_dp->pps_pipe != INVALID_PIPE)
 		return intel_dp->pps_pipe;
 
@@ -440,9 +446,12 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	 * are two power sequencers and up to two eDP ports.
 	 */
 	if (WARN_ON(pipes == 0))
-		return PIPE_A;
+		pipe = PIPE_A;
+	else
+		pipe = ffs(pipes) - 1;
 
-	intel_dp->pps_pipe = ffs(pipes) - 1;
+	vlv_steal_power_sequencer(dev, pipe);
+	intel_dp->pps_pipe = pipe;
 
 	DRM_DEBUG_KMS("picked pipe %c power sequencer for port %c\n",
 		      pipe_name(intel_dp->pps_pipe),
-- 
2.0.4

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

[PATCH 16/17] drm/i915: Improve VDD/PPS debugs

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Print the port name in the VDD/PPS debugs messages.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b58c94f..c8b58e4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1510,7 +1510,8 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	DRM_DEBUG_KMS("Turning eDP VDD on\n");
+	DRM_DEBUG_KMS("Turning eDP port %c VDD on\n",
+		      port_name(intel_dig_port->port));
 
 	if (!edp_have_panel_power(intel_dp))
 		wait_panel_power_cycle(intel_dp);
@@ -1529,7 +1530,8 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	 * If the panel wasn't on, delay before accessing aux channel
 	 */
 	if (!edp_have_panel_power(intel_dp)) {
-		DRM_DEBUG_KMS("eDP was not running\n");
+		DRM_DEBUG_KMS("eDP port %c panel power wasn't enabled\n",
+			      port_name(intel_dig_port->port));
 		msleep(intel_dp->panel_power_up_delay);
 	}
 
@@ -1554,7 +1556,8 @@ void intel_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	vdd = edp_panel_vdd_on(intel_dp);
 	pps_unlock(intel_dp);
 
-	WARN(!vdd, "eDP VDD already requested on\n");
+	WARN(!vdd, "eDP port %c VDD already requested on\n",
+	     port_name(dp_to_dig_port(intel_dp)->port));
 }
 
 static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
@@ -1575,7 +1578,8 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	if (!edp_have_panel_vdd(intel_dp))
 		return;
 
-	DRM_DEBUG_KMS("Turning eDP VDD off\n");
+	DRM_DEBUG_KMS("Turning eDP port %c VDD off\n",
+		      port_name(intel_dig_port->port));
 
 	pp = ironlake_get_pp_control(intel_dp);
 	pp &= ~EDP_FORCE_VDD;
@@ -1636,7 +1640,8 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	if (!is_edp(intel_dp))
 		return;
 
-	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
+	WARN(!intel_dp->want_panel_vdd, "eDP port %c VDD not forced on",
+	     port_name(dp_to_dig_port(intel_dp)->port));
 
 	intel_dp->want_panel_vdd = false;
 
@@ -1658,7 +1663,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return;
 
-	DRM_DEBUG_KMS("Turn eDP power on\n");
+	DRM_DEBUG_KMS("Turn eDP port %c panel power on\n",
+		      port_name(dp_to_dig_port(intel_dp)->port));
 
 	if (edp_have_panel_power(intel_dp)) {
 		DRM_DEBUG_KMS("eDP power already on\n");
@@ -1719,9 +1725,11 @@ static void edp_panel_off(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return;
 
-	DRM_DEBUG_KMS("Turn eDP power off\n");
+	DRM_DEBUG_KMS("Turn eDP port %c panel power off\n",
+		      port_name(dp_to_dig_port(intel_dp)->port));
 
-	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
+	WARN(!intel_dp->want_panel_vdd, "Need eDP port %c VDD to turn off panel\n",
+	     port_name(dp_to_dig_port(intel_dp)->port));
 
 	pp = ironlake_get_pp_control(intel_dp);
 	/* We need to switch off panel power _and_ force vdd, for otherwise some
-- 
2.0.4

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

[PATCH 17/17] drm/i915: Warn if panel power is already on when enabling it

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We should never enable the panel power twice. That would indicate a bug
somewhere else as we would need to enable the port twice without
disabling it in between. Also print the port name.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c8b58e4..51f4748 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1666,10 +1666,10 @@ static void edp_panel_on(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("Turn eDP port %c panel power on\n",
 		      port_name(dp_to_dig_port(intel_dp)->port));
 
-	if (edp_have_panel_power(intel_dp)) {
-		DRM_DEBUG_KMS("eDP power already on\n");
+	if (WARN(edp_have_panel_power(intel_dp),
+		 "eDP port %c panel power already on\n",
+		 port_name(dp_to_dig_port(intel_dp)->port)))
 		return;
-	}
 
 	wait_panel_power_cycle(intel_dp);
 
-- 
2.0.4

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

Re: [PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

[Jani Nikula]

On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Only ports B and C have the power sequencer and backlight controls,
> so complain if we ever try to register an eDP connector on some other
> port.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4455009..de919e9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	if (type == DRM_MODE_CONNECTOR_eDP)
>  		intel_encoder->type = INTEL_OUTPUT_EDP;
>  
> +	/* eDP only on port B and/or C on vlv/chv */
> +	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
> +		    port != PORT_B && port != PORT_C))
> +		return false;

Sidetracking about WARNs here for a sec.

One takeaway from working on bugs for a month is that when you read a
lot (I mean really a lot) of dmesgs for various kernel versions, and you
see all those warning backtraces from WARN_ON in the logs, it gets
really tedious to find the line emitting the warning in the source. You
see the function and the line number, but maybe those have changed and
maybe the warn was different a few kernel releases ago.

The WARNs with a descriptive message, on the other hand, are really
quite helpful both while reading the backtrace and while reading the
source. Kind of self-documenting. I'd like to discourage the use of
WARN_ON in favor of WARN all around.

So here you have a WARN_ON with a comment that could become the WARN
message almost verbatim. Please consider making the change.

BR,
Jani.



> +
>  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
>  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
>  			port_name(port));
> -- 
> 2.0.4
>
> _______________________________________________
> 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

Re: [PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

[Ville Syrjälä]

On Fri, Oct 17, 2014 at 12:47:31PM +0300, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Only ports B and C have the power sequencer and backlight controls,
> > so complain if we ever try to register an eDP connector on some other
> > port.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4455009..de919e9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  	if (type == DRM_MODE_CONNECTOR_eDP)
> >  		intel_encoder->type = INTEL_OUTPUT_EDP;
> >  
> > +	/* eDP only on port B and/or C on vlv/chv */
> > +	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
> > +		    port != PORT_B && port != PORT_C))
> > +		return false;
> 
> Sidetracking about WARNs here for a sec.
> 
> One takeaway from working on bugs for a month is that when you read a
> lot (I mean really a lot) of dmesgs for various kernel versions, and you
> see all those warning backtraces from WARN_ON in the logs, it gets
> really tedious to find the line emitting the warning in the source. You
> see the function and the line number, but maybe those have changed and
> maybe the warn was different a few kernel releases ago.
> 
> The WARNs with a descriptive message, on the other hand, are really
> quite helpful both while reading the backtrace and while reading the
> source. Kind of self-documenting. I'd like to discourage the use of
> WARN_ON in favor of WARN all around.
> 
> So here you have a WARN_ON with a comment that could become the WARN
> message almost verbatim. Please consider making the change.

Yeah I've come to the same conclusion myself and yet I somehow always
skip the message when adding one initially. I need to tech myself better
habits, or someone needs to kill WARN_ON() entirely :P

I'm not actually sure this WARN here is all that useful. Might be better
to sprinkle a few more of the pipe==A|B checks in some of the lower
level PPS funcs like I did in patch 14/17. Although that could use the
message too. Also maybe WARN_ONCE() would be better for those so that we
don't bog down the entire machine.

> 
> BR,
> Jani.
> 
> 
> 
> > +
> >  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
> >  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> >  			port_name(port));
> > -- 
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

[Mika Kuoppala]

Jani Nikula <jani.nikula@linux.intel.com> writes:

> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Only ports B and C have the power sequencer and backlight controls,
>> so complain if we ever try to register an eDP connector on some other
>> port.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 4455009..de919e9 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>  	if (type == DRM_MODE_CONNECTOR_eDP)
>>  		intel_encoder->type = INTEL_OUTPUT_EDP;
>>  
>> +	/* eDP only on port B and/or C on vlv/chv */
>> +	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
>> +		    port != PORT_B && port != PORT_C))
>> +		return false;
>
> Sidetracking about WARNs here for a sec.
>
> One takeaway from working on bugs for a month is that when you read a
> lot (I mean really a lot) of dmesgs for various kernel versions, and you
> see all those warning backtraces from WARN_ON in the logs, it gets
> really tedious to find the line emitting the warning in the source. You
> see the function and the line number, but maybe those have changed and
> maybe the warn was different a few kernel releases ago.
>
> The WARNs with a descriptive message, on the other hand, are really
> quite helpful both while reading the backtrace and while reading the
> source. Kind of self-documenting. I'd like to discourage the use of
> WARN_ON in favor of WARN all around.
>

Is

diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index ac6232b..9b82d90 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -57,6 +57,11 @@
 #define DRIVER_DESC            "Intel Graphics"
 #define DRIVER_DATE            "20141003"
 
+#ifdef WARN_ON
+#undef WARN_ON
+#define WARN_ON(x)     WARN(x, "WARN_ON(" #x ")")
+#endif
+

too big of a hammer?

adds ~16k to i915.ko.

--Mika

> So here you have a WARN_ON with a comment that could become the WARN
> message almost verbatim. Please consider making the change.
>
> BR,
> Jani.
>
>
>
>> +
>>  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
>>  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
>>  			port_name(port));
>> -- 
>> 2.0.4
>>
>> _______________________________________________
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

[Daniel Vetter]

On Fri, Oct 17, 2014 at 08:26:13PM +0300, Mika Kuoppala wrote:
> Jani Nikula <jani.nikula@linux.intel.com> writes:
> 
> > On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Only ports B and C have the power sequencer and backlight controls,
> >> so complain if we ever try to register an eDP connector on some other
> >> port.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 4455009..de919e9 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >>  	if (type == DRM_MODE_CONNECTOR_eDP)
> >>  		intel_encoder->type = INTEL_OUTPUT_EDP;
> >>  
> >> +	/* eDP only on port B and/or C on vlv/chv */
> >> +	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
> >> +		    port != PORT_B && port != PORT_C))
> >> +		return false;
> >
> > Sidetracking about WARNs here for a sec.
> >
> > One takeaway from working on bugs for a month is that when you read a
> > lot (I mean really a lot) of dmesgs for various kernel versions, and you
> > see all those warning backtraces from WARN_ON in the logs, it gets
> > really tedious to find the line emitting the warning in the source. You
> > see the function and the line number, but maybe those have changed and
> > maybe the warn was different a few kernel releases ago.
> >
> > The WARNs with a descriptive message, on the other hand, are really
> > quite helpful both while reading the backtrace and while reading the
> > source. Kind of self-documenting. I'd like to discourage the use of
> > WARN_ON in favor of WARN all around.
> >
> 
> Is
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index ac6232b..9b82d90 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -57,6 +57,11 @@
>  #define DRIVER_DESC            "Intel Graphics"
>  #define DRIVER_DATE            "20141003"
>  
> +#ifdef WARN_ON
> +#undef WARN_ON
> +#define WARN_ON(x)     WARN(x, "WARN_ON(" #x ")")
> +#endif
> +
> 
> too big of a hammer?

Imo this is awesome!
> 
> adds ~16k to i915.ko.

And for all those concerned about since we need options to cut away _all_
the debug strings. Well _all_ the printk output even. Wasting a bit more
space for easier debug is imo more than worth it.

So please make this a proper patch and let's get this in. For hilarity
maybe cc: lkml and ask get_maintainers who might be interested in a
generic version ... But I think we want the i915 one in asap.

Cheers, Daniel
> 
> --Mika
> 
> > So here you have a WARN_ON with a comment that could become the WARN
> > message almost verbatim. Please consider making the change.
> >
> > BR,
> > Jani.
> >
> >
> >
> >> +
> >>  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
> >>  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> >>  			port_name(port));
> >> -- 
> >> 2.0.4
> >>
> >> _______________________________________________
> >> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 08/17] drm/i915: Wait for PHY port ready before link training on VLV/CHV

[Todd Previte]

On 10/16/2014 11:27 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There's no point in checking if the data lanes came out of reset after
> link training. If the data lanes aren't ready link training will fail
> anyway.
>
> Suggested-by: Todd Previte <tprevite@gmail.com>
> Cc: Todd Previte <tprevite@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> If Todd has a better patch with better description we can use that one
> instead of my version. But at least I think the spot where I put the
> vlv_wait_port_ready() is the right one. We could perhaps skip the link
> training attempt entirely if the port is already stuck.
>
>   drivers/gpu/drm/i915/intel_dp.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4c8f169..6f568b4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2550,6 +2550,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>   
>   	pps_unlock(intel_dp);
>   
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
> +
>   	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>   	intel_dp_start_link_train(intel_dp);
>   	intel_dp_complete_link_train(intel_dp);
> @@ -2689,8 +2692,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>   	mutex_unlock(&dev_priv->dpio_lock);
>   
>   	intel_enable_dp(encoder);
> -
> -	vlv_wait_port_ready(dev_priv, dport);
>   }
>   
>   static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
> @@ -2783,8 +2784,6 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>   	mutex_unlock(&dev_priv->dpio_lock);
>   
>   	intel_enable_dp(encoder);
> -
> -	vlv_wait_port_ready(dev_priv, dport);
>   }
>   
>   static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)

We should definitely skip link training if the PHYs are down. There's 
going to be a WARN when wait_port_ready() fails so we'll be well aware 
that something went wrong.  Spamming dmesg with errors/WARNs from trying 
to train the link after that is really counterproductive, since we 
already know there's no way link training could succeed.

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

Re: [PATCH 05/17] drm/i915: Don't initialize power seqeuencer delays more than once

[Imre Deak]

On Thu, 2014-10-16 at 21:27 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since we read the current power seqeuncer delays from the registers
> (as well as looking at the vbt and spec values) we may end up
> corrupting delays we already initialized when we switch to another
> pipe and the power seqeuncer there has different values currently
> in the registers.
> 
> So make sure we only initialize the delays once even if
> intel_dp_init_panel_power_sequencer() gets called multiple times.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7a10464..9a1295d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4756,6 +4756,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> +	/* already initialized? */
> +	if (final->t11_t12 != 0)
> +		return;
> +

I wonder if some other place depends on the PP_CONTROL unlocking done
here. At least intel_dp_init_panel_power_sequencer_registers() doesn't
do the unlocking when writing to other PP regs. Maybe the locking
mechanism has an effect only while power sequencing is active, so it
wouldn't matter but the comment in this function suggests that we need
to unlock as a first step. The VLV spec is unclear if unlocking is
needed.


>  	if (HAS_PCH_SPLIT(dev)) {
>  		pp_ctrl_reg = PCH_PP_CONTROL;
>  		pp_on_reg = PCH_PP_ON_DELAYS;


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

Re: [PATCH 05/17] drm/i915: Don't initialize power seqeuencer delays more than once

[Ville Syrjälä]

On Mon, Oct 27, 2014 at 04:43:07PM +0200, Imre Deak wrote:
> On Thu, 2014-10-16 at 21:27 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Since we read the current power seqeuncer delays from the registers
> > (as well as looking at the vbt and spec values) we may end up
> > corrupting delays we already initialized when we switch to another
> > pipe and the power seqeuncer there has different values currently
> > in the registers.
> > 
> > So make sure we only initialize the delays once even if
> > intel_dp_init_panel_power_sequencer() gets called multiple times.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 7a10464..9a1295d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4756,6 +4756,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > +	/* already initialized? */
> > +	if (final->t11_t12 != 0)
> > +		return;
> > +
> 
> I wonder if some other place depends on the PP_CONTROL unlocking done
> here. At least intel_dp_init_panel_power_sequencer_registers() doesn't
> do the unlocking when writing to other PP regs. Maybe the locking
> mechanism has an effect only while power sequencing is active, so it
> wouldn't matter but the comment in this function suggests that we need
> to unlock as a first step. The VLV spec is unclear if unlocking is
> needed.

Have I mentioned recently how much I _hate_ all of these hardware
lockout mechanisms? They just get in the way of doing stuff.

Anyway the spec has this to say about the PP_ON bit:
"If this bit is not a zero, it activates the register write protect
 and writes to those registers will be ignored unless the write
 protect key value is set in the panel sequencing control register."

So I think we should be safe.

> 
> 
> >  	if (HAS_PCH_SPLIT(dev)) {
> >  		pp_ctrl_reg = PCH_PP_CONTROL;
> >  		pp_on_reg = PCH_PP_ON_DELAYS;
> 

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

Re: [PATCH 12/17] drm/i915: Don't kick the power seqeuncer just to check if we have vdd/panel power

[Imre Deak]

On Thu, 2014-10-16 at 21:29 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If there's no power sequencer assigned to the port currently we can't
> very well have vdd or panel power enabled either. If we would try to
> check that from the pps registers we'd need to pick a power seqeuncer
> and kick it. So let's skip the register read and the kick.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 74cf827..c9a1600 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -634,6 +634,10 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> +	if (IS_VALLEYVIEW(dev) &&
> +	    intel_dp->pps_pipe == INVALID_PIPE)
> +		return false;
> +
>  	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>  }
>  
> @@ -644,6 +648,10 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> +	if (IS_VALLEYVIEW(dev) &&
> +	    intel_dp->pps_pipe == INVALID_PIPE)
> +		return false;
> +

During resume this makes intel_edp_panel_vdd_sanitize() think VDD is
off, though it could be left on by the BIOS. But AFAICS the above makes
also sure that VDD refcounting won't get broken even in this case. The
only issue I see is that VDD will be enabled when it was already on,
which has some overhead, but I don't think that's a priority for now.

>  	return I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
>  }
>  

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

Re: [PATCH 00/17] drm/i915: Fix vlv/chv panel power sequencer

[Imre Deak]

On Thu, 2014-10-16 at 21:27 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> After weeks or months of beating on the hardware I finally managed
> to figure out how to kick the vlv/chv power sequencer in a reasonably
> light way after changing the pipe<->port mapping.
> 
> Contrary to my earlier comments we don't actually need to kick it
> when dealing with regular DP ports. I'm not sure if my previous BSW
> was just a bit funky or if I just overlooked something because I clearly
> remember having to do that. Anyway now it seems enough to kick eDP
> ports only and with the refined kick procedure (which doesn't involve
> panel power on+off cycles anymore) it should be a reasonably fast
> operation too. The power sequencer on one pipe can control any eDP
> port even if that pipe otherwise is driving another non-eDP port.
> 
> I've tested this this on BYT eDP+DP, BSW eDP+DP, ILK eDP+DP, IVB DP
> and HSW eDP and everything appears to working perfectly now. I've
> been beating on it for a few days now trying all kinds combinations
> of pipes and ports and swapping them around in various ways.
> 
> I also simulated a dual eDP system on the BYT and BSW machines by
> making the code pretend that the external DP port is also eDP. That
> way I could test that the power sequencer code really would work
> in dual eDP machines. Obviously I was unable to test that VDD would
> actually get enabled for both ports since one of them was DP but
> at least the registers pretend that is, and the power sequencer
> didn't cause the port to gets stuck at any point, and the AUX stuff
> worked every single time on the actual eDP port so at least there
> VDD got applied appropriately.
> 
> So I'm quite confident that this vlv/chv power seqeuencer problem
> is now solved. Well, until someone breaks it that is. I'm not sure
> we can do any kind of satisfactory tests for this stuff since it
> depends on eDP and some of trickier interaction even requires dual
> eDP. Also some of the failures can manifest as something as benign
> as failed OUI reads which doesn't even flag an error in dmesg. So
> I don't have particularly good ideas on how we can prevent regressions
> to this code.
> 
> Ville Syrjälä (17):
>   drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv
>   drm/i915: Warn if stealing power sequencer from an active eDP port
>   drm/i915: Remove high level intel_edp_vdd_{on,off}() from hpd/detect
>   drm/i915: Store power sequencer delays in intel_dp
>   drm/i915: Don't initialize power seqeuencer delays more than once
>   drm/i915: Split power sequencer panel on/off functions to locked and
>     unlocked variants
>   drm/i915: Hold the pps mutex across the whole panel power enable
>     sequence
>   drm/i915: Wait for PHY port ready before link training on VLV/CHV
>   drm/i915: Fix eDP link training when switching pipes on VLV/CHV
>   drm/i915: Kick the power sequencer before AUX transactions
>   drm/i915: Make sure DPLL is enabled when kicking the power sequencer
>     on VLV/CHV
>   drm/i915: Don't kick the power seqeuncer just to check if we have
>     vdd/panel power
>   drm/i915: Clear PPS port select when giving up the power sequencer
>   drm/i915: Warn if stealing non pipe A/B power sequencer
>   drm/i915: Steal power sequencer in vlv_power_sequencer_pipe()
>   drm/i915: Improve VDD/PPS debugs
>   drm/i915: Warn if panel power is already on when enabling it
> 
>  drivers/gpu/drm/i915/intel_display.c |  84 +++++----
>  drivers/gpu/drm/i915/intel_dp.c      | 347 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |  16 ++
>  3 files changed, 304 insertions(+), 143 deletions(-)

Looks good to me, I guess it could fix a bunch of VLV modeset related
bugs we have open. I tested this on my BYT-M, eDP/DP with basic xrandr
on/off sequences and igt/kms_flip and it works nicely, so on the entire
series:
Reviewed-by: Imre Deak <imre.deak@intel.com>


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

Re: [PATCH 12/17] drm/i915: Don't kick the power seqeuncer just to check if we have vdd/panel power

[Ville Syrjälä]

On Mon, Oct 27, 2014 at 07:10:08PM +0200, Imre Deak wrote:
> On Thu, 2014-10-16 at 21:29 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If there's no power sequencer assigned to the port currently we can't
> > very well have vdd or panel power enabled either. If we would try to
> > check that from the pps registers we'd need to pick a power seqeuncer
> > and kick it. So let's skip the register read and the kick.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 74cf827..c9a1600 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -634,6 +634,10 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > +	if (IS_VALLEYVIEW(dev) &&
> > +	    intel_dp->pps_pipe == INVALID_PIPE)
> > +		return false;
> > +
> >  	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >  }
> >  
> > @@ -644,6 +648,10 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > +	if (IS_VALLEYVIEW(dev) &&
> > +	    intel_dp->pps_pipe == INVALID_PIPE)
> > +		return false;
> > +
> 
> During resume this makes intel_edp_panel_vdd_sanitize() think VDD is
> off, though it could be left on by the BIOS. But AFAICS the above makes
> also sure that VDD refcounting won't get broken even in this case. The
> only issue I see is that VDD will be enabled when it was already on,
> which has some overhead, but I don't think that's a priority for now.

Hmm. Yeah, so we should basically do the vlv_initial_power_sequencer_setup()
stuff (or at least some of it) on resume as well. Otherwise the hw and
sw state can get out of sync. I'll see about cooking up a patch for
that...

Oh that makes me think of another issue. What if someone has set
disable_power_wells=0 and then suspends the machine? I think currently
we'd end up leaving the power wells enabled which doesn't sound very
nice, and also it would interfere with the pps_pipe reset handling.
Should we have some kind of "force power wells off" step during suspend?

> 
> >  	return I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
> >  }
> >  

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

Re: [PATCH 12/17] drm/i915: Don't kick the power seqeuncer just to check if we have vdd/panel power

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:03:10AM +0200, Ville Syrjälä wrote:
> Oh that makes me think of another issue. What if someone has set
> disable_power_wells=0 and then suspends the machine? I think currently
> we'd end up leaving the power wells enabled which doesn't sound very
> nice, and also it would interfere with the pps_pipe reset handling.
> Should we have some kind of "force power wells off" step during suspend?

I don't think so. If at all we should probably taint the kernel if anyone
touches that option. dinq hasn't yet rolled forward to -rc1 yet so we
can't apply that patch yet.
-Daniel
> 
> > 
> > >  	return I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
> > >  }
> > >  
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[Daniel Vetter]

On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> eDP ports need the power seqeuncer whenever the port is active. Warn if
> we accidentally steal the power sequener from an active eDP port. This
> should not happen unless there's a bug somewhere else, but it's best to
> scream loudly if it happens to help with debugging.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index de919e9..7ac5c47 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
>  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
>  			      pipe_name(pipe), port_name(port));
>  
> +		WARN(encoder->connectors_active,

This only checks for the pipe actually running, i.e. dpms on. But I guess
we actually want to check for logically enabled (i.e. no matter the dpms
state) to make sure we can always transition from dpms off to on again.
That should check for encoder->base.crtc instead.

I'll punt on this patch until this is clarified.
-Daniel

> +		     "stealing pipe %c power sequencer from active eDP port %c\n",
> +		     pipe_name(pipe), port_name(port));
> +
>  		/* make sure vdd is off before we steal it */
>  		edp_panel_vdd_off_sync(intel_dp);
>  
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/17] drm/i915: Don't initialize power seqeuencer delays more than once

[Daniel Vetter]

On Mon, Oct 27, 2014 at 04:55:16PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 27, 2014 at 04:43:07PM +0200, Imre Deak wrote:
> > On Thu, 2014-10-16 at 21:27 +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Since we read the current power seqeuncer delays from the registers
> > > (as well as looking at the vbt and spec values) we may end up
> > > corrupting delays we already initialized when we switch to another
> > > pipe and the power seqeuncer there has different values currently
> > > in the registers.
> > > 
> > > So make sure we only initialize the delays once even if
> > > intel_dp_init_panel_power_sequencer() gets called multiple times.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 7a10464..9a1295d 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4756,6 +4756,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> > >  
> > >  	lockdep_assert_held(&dev_priv->pps_mutex);
> > >  
> > > +	/* already initialized? */
> > > +	if (final->t11_t12 != 0)
> > > +		return;
> > > +
> > 
> > I wonder if some other place depends on the PP_CONTROL unlocking done
> > here. At least intel_dp_init_panel_power_sequencer_registers() doesn't
> > do the unlocking when writing to other PP regs. Maybe the locking
> > mechanism has an effect only while power sequencing is active, so it
> > wouldn't matter but the comment in this function suggests that we need
> > to unlock as a first step. The VLV spec is unclear if unlocking is
> > needed.
> 
> Have I mentioned recently how much I _hate_ all of these hardware
> lockout mechanisms? They just get in the way of doing stuff.
> 
> Anyway the spec has this to say about the PP_ON bit:
> "If this bit is not a zero, it activates the register write protect
>  and writes to those registers will be ignored unless the write
>  protect key value is set in the panel sequencing control register."

I've added this quote to the patch when applying, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[Ville Syrjälä]

On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote:
> On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > eDP ports need the power seqeuncer whenever the port is active. Warn if
> > we accidentally steal the power sequener from an active eDP port. This
> > should not happen unless there's a bug somewhere else, but it's best to
> > scream loudly if it happens to help with debugging.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index de919e9..7ac5c47 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
> >  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
> >  			      pipe_name(pipe), port_name(port));
> >  
> > +		WARN(encoder->connectors_active,
> 
> This only checks for the pipe actually running, i.e. dpms on. But I guess
> we actually want to check for logically enabled (i.e. no matter the dpms
> state) to make sure we can always transition from dpms off to on again.
> That should check for encoder->base.crtc instead.

We can allow stealing the power sequencer whenever the port is not
running. When the port gets re-enabled the power seqeuencer gets
stolen back.

> 
> I'll punt on this patch until this is clarified.
> -Daniel
> 
> > +		     "stealing pipe %c power sequencer from active eDP port %c\n",
> > +		     pipe_name(pipe), port_name(port));
> > +
> >  		/* make sure vdd is off before we steal it */
> >  		edp_panel_vdd_off_sync(intel_dp);
> >  
> > -- 
> > 2.0.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH 08/17] drm/i915: Wait for PHY port ready before link training on VLV/CHV

[Daniel Vetter]

On Wed, Oct 22, 2014 at 08:10:40AM -0700, Todd Previte wrote:
> 
> On 10/16/2014 11:27 AM, ville.syrjala@linux.intel.com wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >There's no point in checking if the data lanes came out of reset after
> >link training. If the data lanes aren't ready link training will fail
> >anyway.
> >
> >Suggested-by: Todd Previte <tprevite@gmail.com>
> >Cc: Todd Previte <tprevite@gmail.com>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> >If Todd has a better patch with better description we can use that one
> >instead of my version. But at least I think the spot where I put the
> >vlv_wait_port_ready() is the right one. We could perhaps skip the link
> >training attempt entirely if the port is already stuck.
> >
> >  drivers/gpu/drm/i915/intel_dp.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >index 4c8f169..6f568b4 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -2550,6 +2550,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  	pps_unlock(intel_dp);
> >+	if (IS_VALLEYVIEW(dev))
> >+		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
> >+
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >  	intel_dp_start_link_train(intel_dp);
> >  	intel_dp_complete_link_train(intel_dp);
> >@@ -2689,8 +2692,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
> >  	mutex_unlock(&dev_priv->dpio_lock);
> >  	intel_enable_dp(encoder);
> >-
> >-	vlv_wait_port_ready(dev_priv, dport);
> >  }
> >  static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
> >@@ -2783,8 +2784,6 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
> >  	mutex_unlock(&dev_priv->dpio_lock);
> >  	intel_enable_dp(encoder);
> >-
> >-	vlv_wait_port_ready(dev_priv, dport);
> >  }
> >  static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
> 
> We should definitely skip link training if the PHYs are down. There's going
> to be a WARN when wait_port_ready() fails so we'll be well aware that
> something went wrong.  Spamming dmesg with errors/WARNs from trying to train
> the link after that is really counterproductive, since we already know
> there's no way link training could succeed.

I'm taking this as an ack for Ville's patch, especially since the
discussion around your patch to avoid link-training at other places is
still ongoing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

[Daniel Vetter]

On Thu, Oct 16, 2014 at 09:29:45PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The power seqeuencer kick procedure requires the DPLL to be running
> in order to complete succesfully. In case the DPLL isn't currently
> running when we need to kick the power seqeuncer enable it
> temporarily. This can happen eg. during ->detect() when the pipe is
> not already active.
> 
> To avoid needlessly duplicating the DPLL programming re-use the already
> existing functions by passing a temporary pipe config to them instead
> of having them consult the current pipe config at crtc->config.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

[snip]

> +	/*
> +	 * The DPLL for the pipe must be enabled for this to work.
> +	 * So enable temporarily it if it's not already enabled.
> +	 */
> +	if (!pll_enabled) {
> +		struct intel_crtc *crtc =
> +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +		struct intel_crtc_config pipe_config = {
> +			.pixel_multiplier = 1,
> +			.dpll = IS_CHERRYVIEW(dev) ?
> +				chv_dpll[0].dpll : vlv_dpll[0].dpll,
> +			.port_clock = 162000,
> +			.clock_set = true,
> +			.has_dp_encoder = true,
> +		};
> +
> +		if (IS_CHERRYVIEW(dev)) {
> +			chv_update_pll(crtc, &pipe_config);
> +			chv_prepare_pll(crtc, &pipe_config);
> +			chv_enable_pll(crtc, &pipe_config);
> +		} else {
> +			vlv_update_pll(crtc, &pipe_config);
> +			vlv_prepare_pll(crtc, &pipe_config);
> +			vlv_enable_pll(crtc, &pipe_config);

I'm not terribly happy with the massive list of non-static platform
functions this exposes. And it's also fairly leaky since the minimal pipe
config depends upon what exactly the vlv/chv pll functions need.

Could we instead have a wrapper function for both the enable and disable
which sits right next to the vlv/chv dpll code in intel_display.c? E.g.
valleyview_force_pll_on/off(dev, pipe) or something like that.
-Daniel

> +		}
> +	}
> +
>  	/*
>  	 * Similar magic as in intel_dp_enable_port().
>  	 * We _must_ do this port enable + disable trick
> @@ -365,6 +395,13 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  
>  	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
>  	POSTING_READ(intel_dp->output_reg);
> +
> +	if (!pll_enabled) {
> +		if (IS_CHERRYVIEW(dev))
> +			chv_disable_pll(dev_priv, pipe);
> +		else
> +			vlv_disable_pll(dev_priv, pipe);
> +	}
>  }
>  
>  static enum pipe
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f7ba1fc..69c8c5f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -911,6 +911,21 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
>  void intel_put_shared_dpll(struct intel_crtc *crtc);
>  
> +void vlv_update_pll(struct intel_crtc *crtc,
> +		    struct intel_crtc_config *pipe_config);
> +void chv_update_pll(struct intel_crtc *crtc,
> +		    struct intel_crtc_config *pipe_config);
> +void vlv_prepare_pll(struct intel_crtc *crtc,
> +		     const struct intel_crtc_config *pipe_config);
> +void chv_prepare_pll(struct intel_crtc *crtc,
> +		     const struct intel_crtc_config *pipe_config);
> +void vlv_enable_pll(struct intel_crtc *crtc,
> +		    const struct intel_crtc_config *pipe_config);
> +void chv_enable_pll(struct intel_crtc *crtc,
> +		    const struct intel_crtc_config *pipe_config);
> +void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> +void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> +
>  /* modesetting asserts */
>  void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe);
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Don't kick the power seqeuncer just to check if we have vdd/panel power

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:03:10AM +0200, Ville Syrjälä wrote:
> On Mon, Oct 27, 2014 at 07:10:08PM +0200, Imre Deak wrote:
> > On Thu, 2014-10-16 at 21:29 +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If there's no power sequencer assigned to the port currently we can't
> > > very well have vdd or panel power enabled either. If we would try to
> > > check that from the pps registers we'd need to pick a power seqeuncer
> > > and kick it. So let's skip the register read and the kick.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 74cf827..c9a1600 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -634,6 +634,10 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> > >  
> > >  	lockdep_assert_held(&dev_priv->pps_mutex);
> > >  
> > > +	if (IS_VALLEYVIEW(dev) &&
> > > +	    intel_dp->pps_pipe == INVALID_PIPE)
> > > +		return false;
> > > +
> > >  	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> > >  }
> > >  
> > > @@ -644,6 +648,10 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> > >  
> > >  	lockdep_assert_held(&dev_priv->pps_mutex);
> > >  
> > > +	if (IS_VALLEYVIEW(dev) &&
> > > +	    intel_dp->pps_pipe == INVALID_PIPE)
> > > +		return false;
> > > +
> > 
> > During resume this makes intel_edp_panel_vdd_sanitize() think VDD is
> > off, though it could be left on by the BIOS. But AFAICS the above makes
> > also sure that VDD refcounting won't get broken even in this case. The
> > only issue I see is that VDD will be enabled when it was already on,
> > which has some overhead, but I don't think that's a priority for now.
> 
> Hmm. Yeah, so we should basically do the vlv_initial_power_sequencer_setup()
> stuff (or at least some of it) on resume as well. Otherwise the hw and
> sw state can get out of sync. I'll see about cooking up a patch for
> that...

Yeah, pretty much same thought here. We even have a special callback for
this stuff with encoder->reset!
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

[Ville Syrjälä]

On Tue, Oct 28, 2014 at 09:22:12AM +0100, Daniel Vetter wrote:
> On Thu, Oct 16, 2014 at 09:29:45PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The power seqeuencer kick procedure requires the DPLL to be running
> > in order to complete succesfully. In case the DPLL isn't currently
> > running when we need to kick the power seqeuncer enable it
> > temporarily. This can happen eg. during ->detect() when the pipe is
> > not already active.
> > 
> > To avoid needlessly duplicating the DPLL programming re-use the already
> > existing functions by passing a temporary pipe config to them instead
> > of having them consult the current pipe config at crtc->config.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> [snip]
> 
> > +	/*
> > +	 * The DPLL for the pipe must be enabled for this to work.
> > +	 * So enable temporarily it if it's not already enabled.
> > +	 */
> > +	if (!pll_enabled) {
> > +		struct intel_crtc *crtc =
> > +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > +		struct intel_crtc_config pipe_config = {
> > +			.pixel_multiplier = 1,
> > +			.dpll = IS_CHERRYVIEW(dev) ?
> > +				chv_dpll[0].dpll : vlv_dpll[0].dpll,
> > +			.port_clock = 162000,
> > +			.clock_set = true,
> > +			.has_dp_encoder = true,
> > +		};
> > +
> > +		if (IS_CHERRYVIEW(dev)) {
> > +			chv_update_pll(crtc, &pipe_config);
> > +			chv_prepare_pll(crtc, &pipe_config);
> > +			chv_enable_pll(crtc, &pipe_config);
> > +		} else {
> > +			vlv_update_pll(crtc, &pipe_config);
> > +			vlv_prepare_pll(crtc, &pipe_config);
> > +			vlv_enable_pll(crtc, &pipe_config);
> 
> I'm not terribly happy with the massive list of non-static platform
> functions this exposes. And it's also fairly leaky since the minimal pipe
> config depends upon what exactly the vlv/chv pll functions need.
> 
> Could we instead have a wrapper function for both the enable and disable
> which sits right next to the vlv/chv dpll code in intel_display.c? E.g.
> valleyview_force_pll_on/off(dev, pipe) or something like that.

Sure. I'll respin this with that idea.

> -Daniel
> 
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * Similar magic as in intel_dp_enable_port().
> >  	 * We _must_ do this port enable + disable trick
> > @@ -365,6 +395,13 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
> >  
> >  	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
> >  	POSTING_READ(intel_dp->output_reg);
> > +
> > +	if (!pll_enabled) {
> > +		if (IS_CHERRYVIEW(dev))
> > +			chv_disable_pll(dev_priv, pipe);
> > +		else
> > +			vlv_disable_pll(dev_priv, pipe);
> > +	}
> >  }
> >  
> >  static enum pipe
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f7ba1fc..69c8c5f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -911,6 +911,21 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
> >  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
> >  void intel_put_shared_dpll(struct intel_crtc *crtc);
> >  
> > +void vlv_update_pll(struct intel_crtc *crtc,
> > +		    struct intel_crtc_config *pipe_config);
> > +void chv_update_pll(struct intel_crtc *crtc,
> > +		    struct intel_crtc_config *pipe_config);
> > +void vlv_prepare_pll(struct intel_crtc *crtc,
> > +		     const struct intel_crtc_config *pipe_config);
> > +void chv_prepare_pll(struct intel_crtc *crtc,
> > +		     const struct intel_crtc_config *pipe_config);
> > +void vlv_enable_pll(struct intel_crtc *crtc,
> > +		    const struct intel_crtc_config *pipe_config);
> > +void chv_enable_pll(struct intel_crtc *crtc,
> > +		    const struct intel_crtc_config *pipe_config);
> > +void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> > +void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> > +
> >  /* modesetting asserts */
> >  void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> >  			   enum pipe pipe);
> > -- 
> > 2.0.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH 15/17] drm/i915: Steal power sequencer in vlv_power_sequencer_pipe()

[Daniel Vetter]

On Thu, Oct 16, 2014 at 09:29:59PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> In case we fumble something and end up picking an already used power
> seqeuencer in vlv_power_sequencer_pipe() at least try to steal it
> gracefully. In theory this should never happen though.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Just an aside: I'm not terribly in favour of code "just in case" - it
tends to bitrot even faster than the code we actually use. But this
doesn't look too onerous, so I've merged it.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 74a6514..b58c94f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -114,6 +114,8 @@ static void intel_dp_link_down(struct intel_dp *intel_dp);
>  static bool edp_panel_vdd_on(struct intel_dp *intel_dp);
>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>  static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> +static void vlv_steal_power_sequencer(struct drm_device *dev,
> +				      enum pipe pipe);
>  
>  int
>  intel_dp_max_link_bw(struct intel_dp *intel_dp)
> @@ -412,9 +414,13 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *encoder;
>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
> +	enum pipe pipe;
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> +	/* We should never land here with regular DP ports */
> +	WARN_ON(!is_edp(intel_dp));
> +
>  	if (intel_dp->pps_pipe != INVALID_PIPE)
>  		return intel_dp->pps_pipe;
>  
> @@ -440,9 +446,12 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
>  	 * are two power sequencers and up to two eDP ports.
>  	 */
>  	if (WARN_ON(pipes == 0))
> -		return PIPE_A;
> +		pipe = PIPE_A;
> +	else
> +		pipe = ffs(pipes) - 1;
>  
> -	intel_dp->pps_pipe = ffs(pipes) - 1;
> +	vlv_steal_power_sequencer(dev, pipe);
> +	intel_dp->pps_pipe = pipe;
>  
>  	DRM_DEBUG_KMS("picked pipe %c power sequencer for port %c\n",
>  		      pipe_name(intel_dp->pps_pipe),
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/17] drm/i915: Fix vlv/chv panel power sequencer

[Daniel Vetter]

On Mon, Oct 27, 2014 at 07:56:50PM +0200, Imre Deak wrote:
> On Thu, 2014-10-16 at 21:27 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > After weeks or months of beating on the hardware I finally managed
> > to figure out how to kick the vlv/chv power sequencer in a reasonably
> > light way after changing the pipe<->port mapping.
> > 
> > Contrary to my earlier comments we don't actually need to kick it
> > when dealing with regular DP ports. I'm not sure if my previous BSW
> > was just a bit funky or if I just overlooked something because I clearly
> > remember having to do that. Anyway now it seems enough to kick eDP
> > ports only and with the refined kick procedure (which doesn't involve
> > panel power on+off cycles anymore) it should be a reasonably fast
> > operation too. The power sequencer on one pipe can control any eDP
> > port even if that pipe otherwise is driving another non-eDP port.
> > 
> > I've tested this this on BYT eDP+DP, BSW eDP+DP, ILK eDP+DP, IVB DP
> > and HSW eDP and everything appears to working perfectly now. I've
> > been beating on it for a few days now trying all kinds combinations
> > of pipes and ports and swapping them around in various ways.
> > 
> > I also simulated a dual eDP system on the BYT and BSW machines by
> > making the code pretend that the external DP port is also eDP. That
> > way I could test that the power sequencer code really would work
> > in dual eDP machines. Obviously I was unable to test that VDD would
> > actually get enabled for both ports since one of them was DP but
> > at least the registers pretend that is, and the power sequencer
> > didn't cause the port to gets stuck at any point, and the AUX stuff
> > worked every single time on the actual eDP port so at least there
> > VDD got applied appropriately.
> > 
> > So I'm quite confident that this vlv/chv power seqeuencer problem
> > is now solved. Well, until someone breaks it that is. I'm not sure
> > we can do any kind of satisfactory tests for this stuff since it
> > depends on eDP and some of trickier interaction even requires dual
> > eDP. Also some of the failures can manifest as something as benign
> > as failed OUI reads which doesn't even flag an error in dmesg. So
> > I don't have particularly good ideas on how we can prevent regressions
> > to this code.
> > 
> > Ville Syrjälä (17):
> >   drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv
> >   drm/i915: Warn if stealing power sequencer from an active eDP port
> >   drm/i915: Remove high level intel_edp_vdd_{on,off}() from hpd/detect
> >   drm/i915: Store power sequencer delays in intel_dp
> >   drm/i915: Don't initialize power seqeuencer delays more than once
> >   drm/i915: Split power sequencer panel on/off functions to locked and
> >     unlocked variants
> >   drm/i915: Hold the pps mutex across the whole panel power enable
> >     sequence
> >   drm/i915: Wait for PHY port ready before link training on VLV/CHV
> >   drm/i915: Fix eDP link training when switching pipes on VLV/CHV
> >   drm/i915: Kick the power sequencer before AUX transactions
> >   drm/i915: Make sure DPLL is enabled when kicking the power sequencer
> >     on VLV/CHV
> >   drm/i915: Don't kick the power seqeuncer just to check if we have
> >     vdd/panel power
> >   drm/i915: Clear PPS port select when giving up the power sequencer
> >   drm/i915: Warn if stealing non pipe A/B power sequencer
> >   drm/i915: Steal power sequencer in vlv_power_sequencer_pipe()
> >   drm/i915: Improve VDD/PPS debugs
> >   drm/i915: Warn if panel power is already on when enabling it
> > 
> >  drivers/gpu/drm/i915/intel_display.c |  84 +++++----
> >  drivers/gpu/drm/i915/intel_dp.c      | 347 ++++++++++++++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_drv.h     |  16 ++
> >  3 files changed, 304 insertions(+), 143 deletions(-)
> 
> Looks good to me, I guess it could fix a bunch of VLV modeset related
> bugs we have open. I tested this on my BYT-M, eDP/DP with basic xrandr
> on/off sequences and igt/kms_flip and it works nicely, so on the entire
> series:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Ok, pulled in the entire series except for two patches where I want to
wait for Ville's reply first.

Thanks for patches&review,
Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote:
> On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote:
> > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > eDP ports need the power seqeuncer whenever the port is active. Warn if
> > > we accidentally steal the power sequener from an active eDP port. This
> > > should not happen unless there's a bug somewhere else, but it's best to
> > > scream loudly if it happens to help with debugging.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index de919e9..7ac5c47 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
> > >  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
> > >  			      pipe_name(pipe), port_name(port));
> > >  
> > > +		WARN(encoder->connectors_active,
> > 
> > This only checks for the pipe actually running, i.e. dpms on. But I guess
> > we actually want to check for logically enabled (i.e. no matter the dpms
> > state) to make sure we can always transition from dpms off to on again.
> > That should check for encoder->base.crtc instead.
> 
> We can allow stealing the power sequencer whenever the port is not
> running. When the port gets re-enabled the power seqeuencer gets
> stolen back.

Hm, but how does this work if we run out of power sequencers? Generally
dpms on isn't allowed to fail, so if there's any shared resources we need
to keep them reserved.

Or am I super-dense again?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The power seqeuencer kick procedure requires the DPLL to be running
in order to complete succesfully. In case the DPLL isn't currently
running when we need to kick the power seqeuncer enable it
temporarily. This can happen eg. during ->detect() when the pipe is
not already active.

To avoid needlessly duplicating the DPLL programming re-use the already
existing functions by passing a temporary pipe config to them instead
of having them consult the current pipe config at crtc->config.

v2: Introduce vlv_force_pll_{on,off}() (Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dp.c      |  14 ++++
 drivers/gpu/drm/i915/intel_drv.h     |   4 ++
 3 files changed, 112 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d901961..1fc9ecd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -94,8 +94,10 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 static void ironlake_set_pipeconf(struct drm_crtc *crtc);
 static void haswell_set_pipeconf(struct drm_crtc *crtc);
 static void intel_set_pipe_csc(struct drm_crtc *crtc);
-static void vlv_prepare_pll(struct intel_crtc *crtc);
-static void chv_prepare_pll(struct intel_crtc *crtc);
+static void vlv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config);
+static void chv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -1484,12 +1486,13 @@ static void intel_init_dpio(struct drm_device *dev)
 	}
 }
 
-static void vlv_enable_pll(struct intel_crtc *crtc)
+static void vlv_enable_pll(struct intel_crtc *crtc,
+			   const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int reg = DPLL(crtc->pipe);
-	u32 dpll = crtc->config.dpll_hw_state.dpll;
+	u32 dpll = pipe_config->dpll_hw_state.dpll;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 
@@ -1507,7 +1510,7 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	if (wait_for(((I915_READ(reg) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("DPLL %d failed to lock\n", crtc->pipe);
 
-	I915_WRITE(DPLL_MD(crtc->pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(crtc->pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(crtc->pipe));
 
 	/* We do this three times for luck */
@@ -1522,7 +1525,8 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	udelay(150); /* wait for warmup */
 }
 
-static void chv_enable_pll(struct intel_crtc *crtc)
+static void chv_enable_pll(struct intel_crtc *crtc,
+			   const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1547,14 +1551,14 @@ static void chv_enable_pll(struct intel_crtc *crtc)
 	udelay(1);
 
 	/* Enable PLL */
-	I915_WRITE(DPLL(pipe), crtc->config.dpll_hw_state.dpll);
+	I915_WRITE(DPLL(pipe), pipe_config->dpll_hw_state.dpll);
 
 	/* Check PLL is locked */
 	if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("PLL %d failed to lock\n", pipe);
 
 	/* not sure when this should be written */
-	I915_WRITE(DPLL_MD(pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(pipe));
 
 	mutex_unlock(&dev_priv->dpio_lock);
@@ -4844,9 +4848,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_prepare_pll(intel_crtc);
+			chv_prepare_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_prepare_pll(intel_crtc);
+			vlv_prepare_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	if (intel_crtc->config.has_dp_encoder)
@@ -4873,9 +4877,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_enable_pll(intel_crtc);
+			chv_enable_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_enable_pll(intel_crtc);
+			vlv_enable_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -5756,7 +5760,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc)
 						   &crtc->config.dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc)
+static void vlv_update_pll(struct intel_crtc *crtc,
+			   struct intel_crtc_config *pipe_config)
 {
 	u32 dpll, dpll_md;
 
@@ -5771,14 +5776,15 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	if (crtc->pipe == PIPE_B)
 		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	pipe_config->dpll_hw_state.dpll = dpll;
 
-	dpll_md = (crtc->config.pixel_multiplier - 1)
+	dpll_md = (pipe_config->pixel_multiplier - 1)
 		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-	crtc->config.dpll_hw_state.dpll_md = dpll_md;
+	pipe_config->dpll_hw_state.dpll_md = dpll_md;
 }
 
-static void vlv_prepare_pll(struct intel_crtc *crtc)
+static void vlv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5789,11 +5795,11 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 
 	mutex_lock(&dev_priv->dpio_lock);
 
-	bestn = crtc->config.dpll.n;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/* See eDP HDMI DPIO driver vbios notes doc */
 
@@ -5830,7 +5836,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW3(pipe), mdiv);
 
 	/* Set HBR and RBR LPF coefficients */
-	if (crtc->config.port_clock == 162000 ||
+	if (pipe_config->port_clock == 162000 ||
 	    intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_ANALOG) ||
 	    intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
 		vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe),
@@ -5869,19 +5875,21 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc)
+static void chv_update_pll(struct intel_crtc *crtc,
+			   struct intel_crtc_config *pipe_config)
 {
-	crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
+	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
 		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
 		DPLL_VCO_ENABLE;
 	if (crtc->pipe != PIPE_A)
-		crtc->config.dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	crtc->config.dpll_hw_state.dpll_md =
-		(crtc->config.pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
+	pipe_config->dpll_hw_state.dpll_md =
+		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
 }
 
-static void chv_prepare_pll(struct intel_crtc *crtc)
+static void chv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5892,18 +5900,18 @@ static void chv_prepare_pll(struct intel_crtc *crtc)
 	u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
 	int refclk;
 
-	bestn = crtc->config.dpll.n;
-	bestm2_frac = crtc->config.dpll.m2 & 0x3fffff;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2 >> 22;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm2_frac = pipe_config->dpll.m2 & 0x3fffff;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2 >> 22;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/*
 	 * Enable Refclk and SSC
 	 */
 	I915_WRITE(dpll_reg,
-		   crtc->config.dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
+		   pipe_config->dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
 
 	mutex_lock(&dev_priv->dpio_lock);
 
@@ -5951,6 +5959,54 @@ static void chv_prepare_pll(struct intel_crtc *crtc)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
+/**
+ * vlv_force_pll_on - forcibly enable just the PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to enable
+ * @dpll: PLL configuration
+ *
+ * Enable the PLL for @pipe using the supplied @dpll config. To be used
+ * in cases where we need the PLL enabled even when @pipe is not going to
+ * be enabled.
+ */
+void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
+		      const struct dpll *dpll)
+{
+	struct intel_crtc *crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	struct intel_crtc_config pipe_config = {
+		.pixel_multiplier = 1,
+		.dpll = *dpll,
+	};
+
+	if (IS_CHERRYVIEW(dev)) {
+		chv_update_pll(crtc, &pipe_config);
+		chv_prepare_pll(crtc, &pipe_config);
+		chv_enable_pll(crtc, &pipe_config);
+	} else {
+		vlv_update_pll(crtc, &pipe_config);
+		vlv_prepare_pll(crtc, &pipe_config);
+		vlv_enable_pll(crtc, &pipe_config);
+	}
+
+}
+
+/**
+ * vlv_force_pll_off - forcibly disable just the PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to disable
+ *
+ * Disable the PLL for @pipe. To be used in cases where we need
+ * the PLL enabled even when @pipe is not going to be enabled.
+ */
+void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
+{
+	if (IS_CHERRYVIEW(dev))
+		chv_disable_pll(to_i915(dev), pipe);
+	else
+		vlv_disable_pll(to_i915(dev), pipe);
+}
+
 static void i9xx_update_pll(struct intel_crtc *crtc,
 			    intel_clock_t *reduced_clock,
 			    int num_connectors)
@@ -6331,9 +6387,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	} else if (IS_CHERRYVIEW(dev)) {
-		chv_update_pll(intel_crtc);
+		chv_update_pll(intel_crtc, &intel_crtc->config);
 	} else if (IS_VALLEYVIEW(dev)) {
-		vlv_update_pll(intel_crtc);
+		vlv_update_pll(intel_crtc, &intel_crtc->config);
 	} else {
 		i9xx_update_pll(intel_crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dcb87c8..0a71a74 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -328,6 +328,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe = intel_dp->pps_pipe;
+	bool pll_enabled;
 	uint32_t DP;
 
 	if (WARN(I915_READ(intel_dp->output_reg) & DP_PORT_EN,
@@ -351,6 +352,16 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else if (pipe == PIPE_B)
 		DP |= DP_PIPEB_SELECT;
 
+	pll_enabled = I915_READ(DPLL(pipe)) & DPLL_VCO_ENABLE;
+
+	/*
+	 * The DPLL for the pipe must be enabled for this to work.
+	 * So enable temporarily it if it's not already enabled.
+	 */
+	if (!pll_enabled)
+		vlv_force_pll_on(dev, pipe, IS_CHERRYVIEW(dev) ?
+				 &chv_dpll[0].dpll : &vlv_dpll[0].dpll);
+
 	/*
 	 * Similar magic as in intel_dp_enable_port().
 	 * We _must_ do this port enable + disable trick
@@ -365,6 +376,9 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 
 	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
 	POSTING_READ(intel_dp->output_reg);
+
+	if (!pll_enabled)
+		vlv_force_pll_off(dev, pipe);
 }
 
 static enum pipe
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f7ba1fc..88252e4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -911,6 +911,10 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
 void intel_put_shared_dpll(struct intel_crtc *crtc);
 
+void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
+		      const struct dpll *dpll);
+void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe);
+
 /* modesetting asserts */
 void assert_panel_unlocked(struct drm_i915_private *dev_priv,
 			   enum pipe pipe);
-- 
2.0.4

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

Re: [PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[Ville Syrjälä]

On Tue, Oct 28, 2014 at 09:34:40AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote:
> > On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote:
> > > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > eDP ports need the power seqeuncer whenever the port is active. Warn if
> > > > we accidentally steal the power sequener from an active eDP port. This
> > > > should not happen unless there's a bug somewhere else, but it's best to
> > > > scream loudly if it happens to help with debugging.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index de919e9..7ac5c47 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
> > > >  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
> > > >  			      pipe_name(pipe), port_name(port));
> > > >  
> > > > +		WARN(encoder->connectors_active,
> > > 
> > > This only checks for the pipe actually running, i.e. dpms on. But I guess
> > > we actually want to check for logically enabled (i.e. no matter the dpms
> > > state) to make sure we can always transition from dpms off to on again.
> > > That should check for encoder->base.crtc instead.
> > 
> > We can allow stealing the power sequencer whenever the port is not
> > running. When the port gets re-enabled the power seqeuencer gets
> > stolen back.
> 
> Hm, but how does this work if we run out of power sequencers? Generally
> dpms on isn't allowed to fail, so if there's any shared resources we need
> to keep them reserved.
> 
> Or am I super-dense again?

Currently it shouldn't fail. We have two power sequencers and at most two
eDP ports. If that should change we need to rethink this code.

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

Re: [PATCH 02/17] drm/i915: Warn if stealing power sequencer from an active eDP port

[Daniel Vetter]

On Tue, Oct 28, 2014 at 11:07:27AM +0200, Ville Syrjälä wrote:
> On Tue, Oct 28, 2014 at 09:34:40AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote:
> > > On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote:
> > > > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > eDP ports need the power seqeuncer whenever the port is active. Warn if
> > > > > we accidentally steal the power sequener from an active eDP port. This
> > > > > should not happen unless there's a bug somewhere else, but it's best to
> > > > > scream loudly if it happens to help with debugging.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index de919e9..7ac5c47 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
> > > > >  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
> > > > >  			      pipe_name(pipe), port_name(port));
> > > > >  
> > > > > +		WARN(encoder->connectors_active,
> > > > 
> > > > This only checks for the pipe actually running, i.e. dpms on. But I guess
> > > > we actually want to check for logically enabled (i.e. no matter the dpms
> > > > state) to make sure we can always transition from dpms off to on again.
> > > > That should check for encoder->base.crtc instead.
> > > 
> > > We can allow stealing the power sequencer whenever the port is not
> > > running. When the port gets re-enabled the power seqeuencer gets
> > > stolen back.
> > 
> > Hm, but how does this work if we run out of power sequencers? Generally
> > dpms on isn't allowed to fail, so if there's any shared resources we need
> > to keep them reserved.
> > 
> > Or am I super-dense again?
> 
> Currently it shouldn't fail. We have two power sequencers and at most two
> eDP ports. If that should change we need to rethink this code.

Ok I'm convinced. Added a summary of our discussion here and merged your
patch.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The power seqeuencer kick procedure requires the DPLL to be running
in order to complete successfully. In case the DPLL isn't currently
running when we need to kick the power seqeuncer enable it
temporarily. This can happen eg. during ->detect() when the pipe is
not already active.

To avoid needlessly duplicating the DPLL programming re-use the already
existing functions by passing a temporary pipe config to them instead
of having them consult the current pipe config at crtc->config.

v2: Introduce vlv_force_pll_{on,off}() (Daniel)
v3: Rebase due to drm_crtc vs. intel_crtc changes
    Fix a typo in commit msg (checkpatch)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dp.c      |  14 ++++
 drivers/gpu/drm/i915/intel_drv.h     |   4 ++
 3 files changed, 112 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0ceaf30..3527567 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -94,8 +94,10 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 static void ironlake_set_pipeconf(struct drm_crtc *crtc);
 static void haswell_set_pipeconf(struct drm_crtc *crtc);
 static void intel_set_pipe_csc(struct drm_crtc *crtc);
-static void vlv_prepare_pll(struct intel_crtc *crtc);
-static void chv_prepare_pll(struct intel_crtc *crtc);
+static void vlv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config);
+static void chv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -1484,12 +1486,13 @@ static void intel_init_dpio(struct drm_device *dev)
 	}
 }
 
-static void vlv_enable_pll(struct intel_crtc *crtc)
+static void vlv_enable_pll(struct intel_crtc *crtc,
+			   const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int reg = DPLL(crtc->pipe);
-	u32 dpll = crtc->config.dpll_hw_state.dpll;
+	u32 dpll = pipe_config->dpll_hw_state.dpll;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 
@@ -1507,7 +1510,7 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	if (wait_for(((I915_READ(reg) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("DPLL %d failed to lock\n", crtc->pipe);
 
-	I915_WRITE(DPLL_MD(crtc->pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(crtc->pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(crtc->pipe));
 
 	/* We do this three times for luck */
@@ -1522,7 +1525,8 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	udelay(150); /* wait for warmup */
 }
 
-static void chv_enable_pll(struct intel_crtc *crtc)
+static void chv_enable_pll(struct intel_crtc *crtc,
+			   const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1547,14 +1551,14 @@ static void chv_enable_pll(struct intel_crtc *crtc)
 	udelay(1);
 
 	/* Enable PLL */
-	I915_WRITE(DPLL(pipe), crtc->config.dpll_hw_state.dpll);
+	I915_WRITE(DPLL(pipe), pipe_config->dpll_hw_state.dpll);
 
 	/* Check PLL is locked */
 	if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("PLL %d failed to lock\n", pipe);
 
 	/* not sure when this should be written */
-	I915_WRITE(DPLL_MD(pipe), crtc->config.dpll_hw_state.dpll_md);
+	I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
 	POSTING_READ(DPLL_MD(pipe));
 
 	mutex_unlock(&dev_priv->dpio_lock);
@@ -4848,9 +4852,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_prepare_pll(intel_crtc);
+			chv_prepare_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_prepare_pll(intel_crtc);
+			vlv_prepare_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	if (intel_crtc->config.has_dp_encoder)
@@ -4877,9 +4881,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	if (!is_dsi) {
 		if (IS_CHERRYVIEW(dev))
-			chv_enable_pll(intel_crtc);
+			chv_enable_pll(intel_crtc, &intel_crtc->config);
 		else
-			vlv_enable_pll(intel_crtc);
+			vlv_enable_pll(intel_crtc, &intel_crtc->config);
 	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -5760,7 +5764,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc)
 						   &crtc->config.dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc)
+static void vlv_update_pll(struct intel_crtc *crtc,
+			   struct intel_crtc_config *pipe_config)
 {
 	u32 dpll, dpll_md;
 
@@ -5775,14 +5780,15 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	if (crtc->pipe == PIPE_B)
 		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	pipe_config->dpll_hw_state.dpll = dpll;
 
-	dpll_md = (crtc->config.pixel_multiplier - 1)
+	dpll_md = (pipe_config->pixel_multiplier - 1)
 		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-	crtc->config.dpll_hw_state.dpll_md = dpll_md;
+	pipe_config->dpll_hw_state.dpll_md = dpll_md;
 }
 
-static void vlv_prepare_pll(struct intel_crtc *crtc)
+static void vlv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5793,11 +5799,11 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 
 	mutex_lock(&dev_priv->dpio_lock);
 
-	bestn = crtc->config.dpll.n;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/* See eDP HDMI DPIO driver vbios notes doc */
 
@@ -5834,7 +5840,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW3(pipe), mdiv);
 
 	/* Set HBR and RBR LPF coefficients */
-	if (crtc->config.port_clock == 162000 ||
+	if (pipe_config->port_clock == 162000 ||
 	    intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG) ||
 	    intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
 		vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe),
@@ -5873,19 +5879,21 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc)
+static void chv_update_pll(struct intel_crtc *crtc,
+			   struct intel_crtc_config *pipe_config)
 {
-	crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
+	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
 		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
 		DPLL_VCO_ENABLE;
 	if (crtc->pipe != PIPE_A)
-		crtc->config.dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	crtc->config.dpll_hw_state.dpll_md =
-		(crtc->config.pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
+	pipe_config->dpll_hw_state.dpll_md =
+		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
 }
 
-static void chv_prepare_pll(struct intel_crtc *crtc)
+static void chv_prepare_pll(struct intel_crtc *crtc,
+			    const struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5896,18 +5904,18 @@ static void chv_prepare_pll(struct intel_crtc *crtc)
 	u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
 	int refclk;
 
-	bestn = crtc->config.dpll.n;
-	bestm2_frac = crtc->config.dpll.m2 & 0x3fffff;
-	bestm1 = crtc->config.dpll.m1;
-	bestm2 = crtc->config.dpll.m2 >> 22;
-	bestp1 = crtc->config.dpll.p1;
-	bestp2 = crtc->config.dpll.p2;
+	bestn = pipe_config->dpll.n;
+	bestm2_frac = pipe_config->dpll.m2 & 0x3fffff;
+	bestm1 = pipe_config->dpll.m1;
+	bestm2 = pipe_config->dpll.m2 >> 22;
+	bestp1 = pipe_config->dpll.p1;
+	bestp2 = pipe_config->dpll.p2;
 
 	/*
 	 * Enable Refclk and SSC
 	 */
 	I915_WRITE(dpll_reg,
-		   crtc->config.dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
+		   pipe_config->dpll_hw_state.dpll & ~DPLL_VCO_ENABLE);
 
 	mutex_lock(&dev_priv->dpio_lock);
 
@@ -5955,6 +5963,54 @@ static void chv_prepare_pll(struct intel_crtc *crtc)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
+/**
+ * vlv_force_pll_on - forcibly enable just the PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to enable
+ * @dpll: PLL configuration
+ *
+ * Enable the PLL for @pipe using the supplied @dpll config. To be used
+ * in cases where we need the PLL enabled even when @pipe is not going to
+ * be enabled.
+ */
+void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
+		      const struct dpll *dpll)
+{
+	struct intel_crtc *crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	struct intel_crtc_config pipe_config = {
+		.pixel_multiplier = 1,
+		.dpll = *dpll,
+	};
+
+	if (IS_CHERRYVIEW(dev)) {
+		chv_update_pll(crtc, &pipe_config);
+		chv_prepare_pll(crtc, &pipe_config);
+		chv_enable_pll(crtc, &pipe_config);
+	} else {
+		vlv_update_pll(crtc, &pipe_config);
+		vlv_prepare_pll(crtc, &pipe_config);
+		vlv_enable_pll(crtc, &pipe_config);
+	}
+
+}
+
+/**
+ * vlv_force_pll_off - forcibly disable just the PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to disable
+ *
+ * Disable the PLL for @pipe. To be used in cases where we need
+ * the PLL enabled even when @pipe is not going to be enabled.
+ */
+void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
+{
+	if (IS_CHERRYVIEW(dev))
+		chv_disable_pll(to_i915(dev), pipe);
+	else
+		vlv_disable_pll(to_i915(dev), pipe);
+}
+
 static void i9xx_update_pll(struct intel_crtc *crtc,
 			    intel_clock_t *reduced_clock,
 			    int num_connectors)
@@ -6336,9 +6392,9 @@ static int i9xx_crtc_mode_set(struct intel_crtc *crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	} else if (IS_CHERRYVIEW(dev)) {
-		chv_update_pll(crtc);
+		chv_update_pll(crtc, &crtc->config);
 	} else if (IS_VALLEYVIEW(dev)) {
-		vlv_update_pll(crtc);
+		vlv_update_pll(crtc, &crtc->config);
 	} else {
 		i9xx_update_pll(crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 680f030..82e47da 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -330,6 +330,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe = intel_dp->pps_pipe;
+	bool pll_enabled;
 	uint32_t DP;
 
 	if (WARN(I915_READ(intel_dp->output_reg) & DP_PORT_EN,
@@ -353,6 +354,16 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else if (pipe == PIPE_B)
 		DP |= DP_PIPEB_SELECT;
 
+	pll_enabled = I915_READ(DPLL(pipe)) & DPLL_VCO_ENABLE;
+
+	/*
+	 * The DPLL for the pipe must be enabled for this to work.
+	 * So enable temporarily it if it's not already enabled.
+	 */
+	if (!pll_enabled)
+		vlv_force_pll_on(dev, pipe, IS_CHERRYVIEW(dev) ?
+				 &chv_dpll[0].dpll : &vlv_dpll[0].dpll);
+
 	/*
 	 * Similar magic as in intel_dp_enable_port().
 	 * We _must_ do this port enable + disable trick
@@ -367,6 +378,9 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 
 	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
 	POSTING_READ(intel_dp->output_reg);
+
+	if (!pll_enabled)
+		vlv_force_pll_off(dev, pipe);
 }
 
 static enum pipe
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b323c9b..d53ac23 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -918,6 +918,10 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
 void intel_put_shared_dpll(struct intel_crtc *crtc);
 
+void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
+		      const struct dpll *dpll);
+void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe);
+
 /* modesetting asserts */
 void assert_panel_unlocked(struct drm_i915_private *dev_priv,
 			   enum pipe pipe);
-- 
2.0.4

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

Re: [PATCH 08/17] drm/i915: Wait for PHY port ready before link training on VLV/CHV

[Todd Previte]

On 10/28/2014 1:15 AM, Daniel Vetter wrote:
> On Wed, Oct 22, 2014 at 08:10:40AM -0700, Todd Previte wrote:
>> On 10/16/2014 11:27 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> There's no point in checking if the data lanes came out of reset after
>>> link training. If the data lanes aren't ready link training will fail
>>> anyway.
>>>
>>> Suggested-by: Todd Previte <tprevite@gmail.com>
>>> Cc: Todd Previte <tprevite@gmail.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>> If Todd has a better patch with better description we can use that one
>>> instead of my version. But at least I think the spot where I put the
>>> vlv_wait_port_ready() is the right one. We could perhaps skip the link
>>> training attempt entirely if the port is already stuck.
>>>
>>>   drivers/gpu/drm/i915/intel_dp.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 4c8f169..6f568b4 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2550,6 +2550,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>   	pps_unlock(intel_dp);
>>> +	if (IS_VALLEYVIEW(dev))
>>> +		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
>>> +
>>>   	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>   	intel_dp_start_link_train(intel_dp);
>>>   	intel_dp_complete_link_train(intel_dp);
>>> @@ -2689,8 +2692,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>>>   	mutex_unlock(&dev_priv->dpio_lock);
>>>   	intel_enable_dp(encoder);
>>> -
>>> -	vlv_wait_port_ready(dev_priv, dport);
>>>   }
>>>   static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>>> @@ -2783,8 +2784,6 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>>>   	mutex_unlock(&dev_priv->dpio_lock);
>>>   	intel_enable_dp(encoder);
>>> -
>>> -	vlv_wait_port_ready(dev_priv, dport);
>>>   }
>>>   static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
>> We should definitely skip link training if the PHYs are down. There's going
>> to be a WARN when wait_port_ready() fails so we'll be well aware that
>> something went wrong.  Spamming dmesg with errors/WARNs from trying to train
>> the link after that is really counterproductive, since we already know
>> there's no way link training could succeed.
> I'm taking this as an ack for Ville's patch, especially since the
> discussion around your patch to avoid link-training at other places is
> still ongoing.
> -Daniel
Sounds good. Once the avoiding link training part is resolved, it's a 
minor follow-up patch to implement it.

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