[RFC] misc DP fixes/changes

[RFC] misc DP fixes/changes

[Jesse Barnes]

These are still being tested by our conformance testing team, but at
least some of them are necessary to bring back DP links after a hotplug
event (specifically 5/8 and 6/8).  The others should help us better
handle configuration changes at runtime.

Thanks,
Jesse

[PATCH 1/8] drm/i915/dp: retry link status read 3 times on failure

[Jesse Barnes]

Especially after a hotplug or power status change, the sink may not
reply immediately to a link status query.  So retry 3 times per the spec
to really make sure nothing is there.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 391b55f..1829ecc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 static bool
 intel_dp_get_link_status(struct intel_dp *intel_dp)
 {
-	int ret;
+	int ret, i;
+
+	/* Must try AUX reads for this at least 3 times */
+	for (i = 0; i < 3; i++) {
+		ret = intel_dp_aux_native_read(intel_dp,
+					       DP_LANE0_1_STATUS,
+					       intel_dp->link_status,
+					       DP_LINK_STATUS_SIZE);
+		if (ret == DP_LINK_STATUS_SIZE)
+			break;
+		msleep(1);
+	}
 
-	ret = intel_dp_aux_native_read(intel_dp,
-				       DP_LANE0_1_STATUS,
-				       intel_dp->link_status, DP_LINK_STATUS_SIZE);
 	if (ret != DP_LINK_STATUS_SIZE)
 		return false;
+
 	return true;
 }
 
-- 
1.7.4.1

[PATCH 2/8] drm/i915/dp: use DP DPCD defines when looking at DPCD values

[Jesse Barnes]

Makes it easier to search for DP related constants.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1829ecc..1132ecd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -138,8 +138,8 @@ intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
 	int max_lane_count = 4;
 
-	if (intel_dp->dpcd[0] >= 0x11) {
-		max_lane_count = intel_dp->dpcd[2] & 0x1f;
+	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
+		max_lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & 0x1f;
 		switch (max_lane_count) {
 		case 1: case 2: case 4:
 			break;
@@ -153,7 +153,7 @@ intel_dp_max_lane_count(struct intel_dp *intel_dp)
 static int
 intel_dp_max_link_bw(struct intel_dp *intel_dp)
 {
-	int max_link_bw = intel_dp->dpcd[1];
+	int max_link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
 
 	switch (max_link_bw) {
 	case DP_LINK_BW_1_62:
@@ -774,7 +774,8 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	/*
 	 * Check for DPCD version > 1.1 and enhanced framing support
 	 */
-	if (intel_dp->dpcd[0] >= 0x11 && (intel_dp->dpcd[2] & DP_ENHANCED_FRAME_CAP)) {
+	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
+	    (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
 		intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 		intel_dp->DP |= DP_ENHANCED_FRAMING;
 	}
@@ -1556,7 +1557,7 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
 				     0x000, intel_dp->dpcd,
 				     sizeof (intel_dp->dpcd))
 	    == sizeof(intel_dp->dpcd)) {
-		if (intel_dp->dpcd[0] != 0)
+		if (intel_dp->dpcd[DP_DPCD_REV] != 0)
 			status = connector_status_connected;
 	}
 	DRM_DEBUG_KMS("DPCD: %hx%hx%hx%hx\n", intel_dp->dpcd[0],
@@ -1595,7 +1596,7 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	if (intel_dp_aux_native_read(intel_dp, 0x000, intel_dp->dpcd,
 				     sizeof (intel_dp->dpcd)) == sizeof (intel_dp->dpcd))
 	{
-		if (intel_dp->dpcd[0] != 0)
+		if (intel_dp->dpcd[DP_DPCD_REV] != 0)
 			status = connector_status_connected;
 	}
 
@@ -1963,8 +1964,9 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 					       sizeof(intel_dp->dpcd));
 		ironlake_edp_panel_vdd_off(intel_dp);
 		if (ret == sizeof(intel_dp->dpcd)) {
-			if (intel_dp->dpcd[0] >= 0x11)
-				dev_priv->no_aux_handshake = intel_dp->dpcd[3] &
+			if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
+				dev_priv->no_aux_handshake =
+					intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
 					DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
 		} else {
 			/* if this fails, presume the device is a ghost */
-- 
1.7.4.1

[PATCH 3/8] drm/i915/dp: read more receiver capability bits on hotplug

[Jesse Barnes]

When a hotplug event is received, we need to check the receiver cap bits
in case they've changed (as they might with a hub or chain config).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1132ecd..dcef7fd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1525,6 +1525,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 static void
 intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
+	int ret;
+
 	if (!intel_dp->base.base.crtc)
 		return;
 
@@ -1533,6 +1535,15 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Try to read receiver status if the link appears to be up */
+	ret = intel_dp_aux_native_read(intel_dp,
+				       0x000, intel_dp->dpcd,
+				       sizeof (intel_dp->dpcd));
+	if (ret != sizeof(intel_dp->dpcd)) {
+		intel_dp_link_down(intel_dp);
+		return;
+	}
+
 	if (!intel_channel_eq_ok(intel_dp)) {
 		intel_dp_start_link_train(intel_dp);
 		intel_dp_complete_link_train(intel_dp);
-- 
1.7.4.1

[PATCH 4/8] drm/i915/dp: try to read receiver capabilities 3 times when detecting

[Jesse Barnes]

If ->detect is called too soon after a hot plug event, the sink may not
be ready yet.  So try up to 3 times with 1ms sleeps in between tries to
get the data (spec dictates that receivers must be ready to respond within
1ms and that sources should try 3 times).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dcef7fd..480e8d3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1554,6 +1554,7 @@ static enum drm_connector_status
 ironlake_dp_detect(struct intel_dp *intel_dp)
 {
 	enum drm_connector_status status;
+	int ret, i;
 
 	/* Can't disconnect eDP, but you can close the lid... */
 	if (is_edp(intel_dp)) {
@@ -1564,12 +1565,16 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
 	}
 
 	status = connector_status_disconnected;
-	if (intel_dp_aux_native_read(intel_dp,
-				     0x000, intel_dp->dpcd,
-				     sizeof (intel_dp->dpcd))
-	    == sizeof(intel_dp->dpcd)) {
-		if (intel_dp->dpcd[DP_DPCD_REV] != 0)
+	for (i = 0; i < 3; i++) {
+		ret = intel_dp_aux_native_read(intel_dp,
+					       0x000, intel_dp->dpcd,
+					       sizeof (intel_dp->dpcd));
+		if (ret == sizeof(intel_dp->dpcd) &&
+		    intel_dp->dpcd[DP_DPCD_REV] != 0) {
 			status = connector_status_connected;
+			break;
+		}
+		msleep(1);
 	}
 	DRM_DEBUG_KMS("DPCD: %hx%hx%hx%hx\n", intel_dp->dpcd[0],
 		      intel_dp->dpcd[1], intel_dp->dpcd[2], intel_dp->dpcd[3]);
-- 
1.7.4.1

[PATCH 5/8] drm/i915/dp: set DP DPMS mode to "on" in ->commit

[Jesse Barnes]

A regular mode set can be considered a "DPMS on" state as far as
receiver detection goes.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 480e8d3..f41aec3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -978,6 +978,7 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp))
 		ironlake_edp_backlight_on(dev);
+	intel_dp->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
 static void
-- 
1.7.4.1

[PATCH 6/8] drm/i915/dp: clear DP encoder CRTC if the receiver disappears

[Jesse Barnes]

If the receiver goes away, drop any associated CRTC.  This will force a
full mode set on any subsequent setcrtc call, which is what we need if
the receiver is gone and the link is down.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f41aec3..d7a8d24 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1533,7 +1533,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	if (!intel_dp_get_link_status(intel_dp)) {
 		intel_dp_link_down(intel_dp);
-		return;
+		goto out_gone;
 	}
 
 	/* Try to read receiver status if the link appears to be up */
@@ -1542,13 +1542,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 				       sizeof (intel_dp->dpcd));
 	if (ret != sizeof(intel_dp->dpcd)) {
 		intel_dp_link_down(intel_dp);
-		return;
+		goto out_gone;
 	}
 
 	if (!intel_channel_eq_ok(intel_dp)) {
 		intel_dp_start_link_train(intel_dp);
 		intel_dp_complete_link_train(intel_dp);
 	}
+
+	return;
+
+out_gone:
+	DRM_DEBUG_DRIVER("sink gone, clearing crtc for DP\n");
+	intel_dp->base.base.crtc = NULL;
 }
 
 static enum drm_connector_status
-- 
1.7.4.1

[PATCH 7/8] drm/i915/dp: rename dpms_mode to receiver_configured

[Jesse Barnes]

Make its usage a little more clear.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d7a8d24..ac6334d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -50,7 +50,7 @@ struct intel_dp {
 	bool has_audio;
 	int force_audio;
 	uint32_t color_range;
-	int dpms_mode;
+	bool receiver_configured; /* set if we *think* there's a receiver */
 	uint8_t link_bw;
 	uint8_t lane_count;
 	uint8_t dpcd[4];
@@ -978,7 +978,7 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp))
 		ironlake_edp_backlight_on(dev);
-	intel_dp->dpms_mode = DRM_MODE_DPMS_ON;
+	intel_dp->receiver_configured = true;
 }
 
 static void
@@ -997,6 +997,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 			ironlake_edp_panel_off(dev);
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
+		intel_dp->receiver_configured = false;
 	} else {
 		if (is_edp(intel_dp))
 			ironlake_edp_panel_vdd_on(intel_dp);
@@ -1010,8 +1011,8 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		}
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_on(dev);
+		intel_dp->receiver_configured = true;
 	}
-	intel_dp->dpms_mode = mode;
 }
 
 /*
@@ -1823,7 +1824,7 @@ intel_dp_hot_plug(struct intel_encoder *intel_encoder)
 {
 	struct intel_dp *intel_dp = container_of(intel_encoder, struct intel_dp, base);
 
-	if (intel_dp->dpms_mode == DRM_MODE_DPMS_ON)
+	if (intel_dp->receiver_configured)
 		intel_dp_check_link_status(intel_dp);
 }
 
@@ -1892,7 +1893,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		return;
 
 	intel_dp->output_reg = output_reg;
-	intel_dp->dpms_mode = -1;
+	intel_dp->receiver_configured = false;
 
 	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
-- 
1.7.4.1

[PATCH 8/8] drm/i915/dp: clear receiver_configured when link training fails

[Jesse Barnes]

When link training or detection fails, set the receiver_configured flag
to false to reflect the current state.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ac6334d..e3dd40f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1383,6 +1383,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		if (cr_tries > 5) {
 			DRM_ERROR("failed to train DP, aborting\n");
 			intel_dp_link_down(intel_dp);
+			intel_dp->receiver_configured = false;
 			break;
 		}
 
@@ -1556,6 +1557,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 out_gone:
 	DRM_DEBUG_DRIVER("sink gone, clearing crtc for DP\n");
 	intel_dp->base.base.crtc = NULL;
+	intel_dp->receiver_configured = false;
 }
 
 static enum drm_connector_status
-- 
1.7.4.1

Re: [RFC] misc DP fixes/changes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 496 bytes --]

On Fri,  1 Jul 2011 15:22:50 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> These are still being tested by our conformance testing team, but at
> least some of them are necessary to bring back DP links after a hotplug
> event (specifically 5/8 and 6/8).  The others should help us better
> handle configuration changes at runtime.

This series fixes a ton of random hot-plug weirdness on my X220.

Tested-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 1/8] drm/i915/dp: retry link status read 3 times on failure

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1513 bytes --]

On Fri,  1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Especially after a hotplug or power status change, the sink may not
> reply immediately to a link status query.  So retry 3 times per the spec
> to really make sure nothing is there.

There's no 'false' return path here. I think you want:
> 
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 391b55f..1829ecc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
>  static bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp)
>  {
> -	int ret;
> +	int ret, i;
> +
> +	/* Must try AUX reads for this at least 3 times */
> +	for (i = 0; i < 3; i++) {
> +		ret = intel_dp_aux_native_read(intel_dp,
> +					       DP_LANE0_1_STATUS,
> +					       intel_dp->link_status,
> +					       DP_LINK_STATUS_SIZE);
> +		if (ret == DP_LINK_STATUS_SIZE)
> +			break;

                        return true;
> +		msleep(1);
> +	}
>  
> -	ret = intel_dp_aux_native_read(intel_dp,
> -				       DP_LANE0_1_STATUS,
> -				       intel_dp->link_status, DP_LINK_STATUS_SIZE);
>  	if (ret != DP_LINK_STATUS_SIZE)
>  		return false;
> +
>  	return true;

        return false;
        
>  }

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 2/8] drm/i915/dp: use DP DPCD defines when looking at DPCD values

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 283 bytes --]

On Fri,  1 Jul 2011 15:22:52 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Makes it easier to search for DP related constants.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 3/8] drm/i915/dp: read more receiver capability bits on hotplug

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 431 bytes --]

On Fri,  1 Jul 2011 15:22:53 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> When a hotplug event is received, we need to check the receiver cap bits
> in case they've changed (as they might with a hub or chain config).

Please create a common function to read the dpcd values; this code
exists four times in the driver. That function should retry 3 times,
like ironlake_dp_detect.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 4/8] drm/i915/dp: try to read receiver capabilities 3 times when detecting

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 523 bytes --]

On Fri,  1 Jul 2011 15:22:54 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> If ->detect is called too soon after a hot plug event, the sink may not
> be ready yet.  So try up to 3 times with 1ms sleeps in between tries to
> get the data (spec dictates that receivers must be ready to respond within
> 1ms and that sources should try 3 times).

See previous patch comment -- the dpcd read should be stuffed into a
separate function and shared between the four current users.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 5/8] drm/i915/dp: set DP DPMS mode to "on" in ->commit

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 341 bytes --]

On Fri,  1 Jul 2011 15:22:55 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> A regular mode set can be considered a "DPMS on" state as far as
> receiver detection goes.

There are three patches affecting the tracking of the DP receiver
connected status. Please merge them into a single patch.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 1/8] drm/i915/dp: retry link status read 3 times on failure

[Jesse Barnes]

On Fri, 01 Jul 2011 16:41:06 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Fri,  1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > Especially after a hotplug or power status change, the sink may not
> > reply immediately to a link status query.  So retry 3 times per the spec
> > to really make sure nothing is there.
> 
> There's no 'false' return path here. I think you want:
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   17 +++++++++++++----
> >  1 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 391b55f..1829ecc 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
> >  static bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp)
> >  {
> > -	int ret;
> > +	int ret, i;
> > +
> > +	/* Must try AUX reads for this at least 3 times */
> > +	for (i = 0; i < 3; i++) {
> > +		ret = intel_dp_aux_native_read(intel_dp,
> > +					       DP_LANE0_1_STATUS,
> > +					       intel_dp->link_status,
> > +					       DP_LINK_STATUS_SIZE);
> > +		if (ret == DP_LINK_STATUS_SIZE)
> > +			break;
> 
>                         return true;
> > +		msleep(1);
> > +	}
> >  
> > -	ret = intel_dp_aux_native_read(intel_dp,
> > -				       DP_LANE0_1_STATUS,
> > -				       intel_dp->link_status, DP_LINK_STATUS_SIZE);
> >  	if (ret != DP_LINK_STATUS_SIZE)
> >  		return false;

I think we'd hit the "return false" above if the loop failed.  But
simply returning true from the loop and false otherwise is clearer and
fewer lines.  Will fix.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915/dp: clear DP encoder CRTC if the receiver disappears

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 528 bytes --]

On Fri,  1 Jul 2011 15:22:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> If the receiver goes away, drop any associated CRTC.  This will force a
> full mode set on any subsequent setcrtc call, which is what we need if
> the receiver is gone and the link is down.

This doesn't look like a good solution to me -- we're smashing the
connection between the DP output and the selected CRTC. If you plug the
cable back in, won't this mess things up when we try to retrain again?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 7/8] drm/i915/dp: rename dpms_mode to receiver_configured

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 261 bytes --]

On Fri,  1 Jul 2011 15:22:57 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Make its usage a little more clear.

See previous comment -- this patch should be merged with the other
two receiver configured patches.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 8/8] drm/i915/dp: clear receiver_configured when link training fails

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 318 bytes --]

On Fri,  1 Jul 2011 15:22:58 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> When link training or detection fails, set the receiver_configured flag
> to false to reflect the current state.

I think this should get merged with the other two patches affecting this logic.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 6/8] drm/i915/dp: clear DP encoder CRTC if the receiver disappears

[Jesse Barnes]

On Fri, 01 Jul 2011 16:48:27 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Fri,  1 Jul 2011 15:22:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > If the receiver goes away, drop any associated CRTC.  This will force a
> > full mode set on any subsequent setcrtc call, which is what we need if
> > the receiver is gone and the link is down.
> 
> This doesn't look like a good solution to me -- we're smashing the
> connection between the DP output and the selected CRTC. If you plug the
> cable back in, won't this mess things up when we try to retrain again?

That depends on what behavior we want.  With the previous fixes, if you
unplug, we'll get a hotplug event, fail to detect a link, and tear down
the receiver.  With the old code you'd get bad behavior unless you had
hit a DPMS path earlier.

A subsequent hotplug will be ignored as far as link training goes, since
we don't have a receiver configured.

In both cases, we'll emit hotplug events to userspace, which is free to
react (or not) however it wants.

So I think we'd need to leave the receiver_configured bit set even if
the hot plug re-train failed, and just try it again on the next hot
plug (assuming we want to preserve user configs across hot plug
events and not just let userspace handle the hotplug events).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915/dp: clear DP encoder CRTC if the receiver disappears

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1628 bytes --]

On Fri, 1 Jul 2011 16:59:48 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> That depends on what behavior we want.  With the previous fixes, if you
> unplug, we'll get a hotplug event, fail to detect a link, and tear down
> the receiver.  With the old code you'd get bad behavior unless you had
> hit a DPMS path earlier.

Yeah, the old code was clearly wrong; this looks like something that
needs fixing, I just don't know what the right fix is.

> A subsequent hotplug will be ignored as far as link training goes, since
> we don't have a receiver configured.

Do we need separate variables for 'should be configured' and 'actually is
configured' then? We need to know if there is a configuration we should
be selecting, and then we need to know whether the display is actually
using that configuration.

> In both cases, we'll emit hotplug events to userspace, which is free to
> react (or not) however it wants.

Let's assume (for now) that user space doesn't do anything. That seems
like a good first step to get right; we should assume that if user space
sets some mode then it should work correctly.

> So I think we'd need to leave the receiver_configured bit set even if
> the hot plug re-train failed, and just try it again on the next hot
> plug (assuming we want to preserve user configs across hot plug
> events and not just let userspace handle the hotplug events).

I don't think we want to lose the user config -- in the absence of any
user-driven changes, we want to reset the mode exactly as it was when
the monitor is plugged back in.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 1/8] drm/i915/dp: retry link status read 3 times on failure

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 567 bytes --]

On Fri,  1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Especially after a hotplug or power status change, the sink may not
> reply immediately to a link status query.  So retry 3 times per the spec
> to really make sure nothing is there.

One thing we've been trying to do increasingly on the 3D side is cite
chapter and verse in comments when we're doing something "per the spec"
(which spec here?).  Clarifies whether it was some crazy workaround
copying what the BIOS did at one point vs. something that we really are
supposed to do.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 1/8] drm/i915/dp: retry link status read 3 times on failure

[Jesse Barnes]

On Tue, 05 Jul 2011 21:27:35 -0700
Eric Anholt <eric@anholt.net> wrote:

> On Fri,  1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Especially after a hotplug or power status change, the sink may not
> > reply immediately to a link status query.  So retry 3 times per the spec
> > to really make sure nothing is there.
> 
> One thing we've been trying to do increasingly on the 3D side is cite
> chapter and verse in comments when we're doing something "per the spec"
> (which spec here?).  Clarifies whether it was some crazy workaround
> copying what the BIOS did at one point vs. something that we really are
> supposed to do.

Yeah good point, I'll point at the DP spec section stuff here when I
re-post.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center