diff mbox

[2/3] drm/i915: hide errors when probing for a reverse display port

Message ID 1438099409-25456-3-git-send-email-benjamin.tissoires@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Tissoires July 28, 2015, 4:03 p.m. UTC 
We check the polarity of the attached dp, so it is normal to fail.
Do not send errors to the users.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 74 ++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 16 deletions(-)

Comments

Chris Wilson July 28, 2015, 4:18 p.m. UTC | #1 
On Tue, Jul 28, 2015 at 12:03:28PM -0400, Benjamin Tissoires wrote:
> We check the polarity of the attached dp, so it is normal to fail.
> Do not send errors to the users.

if (probe) DRM_DEBUG else DRM_ERROR is fairly offensive.

It strikes me that you could make each of these functions report the
failure to the caller (have the caller even do so error handling!) and
as part of that have the caller report an error and so demote all of
these to DRM_DEBUG_KMS().

Who knows, actually doing some error handling may make monitor training
more reliable! Or we may even get carried away and report the failure
all the way back to userspace.
-Chris
Benjamin Tissoires July 29, 2015, 3:20 p.m. UTC | #2 
On Jul 28 2015 or thereabouts, Chris Wilson wrote:
> On Tue, Jul 28, 2015 at 12:03:28PM -0400, Benjamin Tissoires wrote:
> > We check the polarity of the attached dp, so it is normal to fail.
> > Do not send errors to the users.
> 
> if (probe) DRM_DEBUG else DRM_ERROR is fairly offensive.
> 
> It strikes me that you could make each of these functions report the
> failure to the caller (have the caller even do so error handling!) and
> as part of that have the caller report an error and so demote all of
> these to DRM_DEBUG_KMS().

Yes, sorry for that. I will change it to return an actual error code and
the error string if I still need to use these functions given Sivakumar
ideas. 

> 
> Who knows, actually doing some error handling may make monitor training
> more reliable! Or we may even get carried away and report the failure
> all the way back to userspace.

That would be a very good improvement indeed. But I can already tell you
that I will not do it by myself, I already have too much on my plate.
I'll do my share for this feature, but don't count on me for the whole
error handling rewrite :)

Cheers,
Benjamin
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2352d8..b740987 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3568,7 +3568,7 @@  static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 /* Enable corresponding port and start training pattern 1 */
 static bool
 _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
-			   uint32_t *DP)
+			   uint32_t *DP, bool probing)
 {
 	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
 	struct drm_device *dev = encoder->dev;
@@ -3576,6 +3576,7 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
 	uint8_t link_config[2];
+	char *error_str;
 
 	if (HAS_DDI(dev))
 		intel_ddi_prepare_link_retrain(encoder);
@@ -3600,7 +3601,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 	if (!intel_dp_reset_link_train(intel_dp, DP,
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to enable link training\n");
+		error_str = "failed to enable link training\n";
+		if (!probing)
+			DRM_ERROR(error_str);
+		else
+			DRM_DEBUG(error_str);
 		return false;
 	}
 
@@ -3612,7 +3617,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
-			DRM_ERROR("failed to get link status\n");
+			error_str = "failed to get link status\n";
+			if (!probing)
+				DRM_ERROR(error_str);
+			else
+				DRM_DEBUG(error_str);
 			break;
 		}
 
@@ -3632,7 +3641,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 			if (!intel_dp_reset_link_train(intel_dp, DP,
 						       DP_TRAINING_PATTERN_1 |
 						       DP_LINK_SCRAMBLING_DISABLE)) {
-				DRM_ERROR("failed to enable link training\n");
+				error_str = "failed to enable link training\n";
+				if (!probing)
+					DRM_ERROR(error_str);
+				else
+					DRM_DEBUG(error_str);
 				return false;
 			}
 			continue;
@@ -3645,7 +3658,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 		if (i == lane_count) {
 			++loop_tries;
 			if (loop_tries == 5) {
-				DRM_ERROR("too many full retries, give up\n");
+				error_str = "too many full retries, give up\n";
+				if (!probing)
+					DRM_ERROR(error_str);
+				else
+					DRM_DEBUG(error_str);
 				break;
 			}
 			intel_dp_reset_link_train(intel_dp, DP,
@@ -3659,7 +3676,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
 			++voltage_tries;
 			if (voltage_tries == 5) {
-				DRM_ERROR("too many voltage retries, give up\n");
+				error_str = "too many voltage retries, give up\n";
+				if (!probing)
+					DRM_ERROR(error_str);
+				else
+					DRM_DEBUG(error_str);
 				break;
 			}
 		} else
@@ -3668,7 +3689,11 @@  _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 
 		/* Update training set as requested by target */
 		if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
-			DRM_ERROR("failed to update link training\n");
+			error_str = "failed to update link training\n";
+			if (!probing)
+				DRM_ERROR(error_str);
+			else
+				DRM_DEBUG(error_str);
 			break;
 		}
 	}
@@ -3682,17 +3707,18 @@  intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
 	uint32_t DP = intel_dp->DP;
 
-	if (_intel_dp_start_link_train(intel_dp, intel_dp->lane_count, &DP))
+	if (_intel_dp_start_link_train(intel_dp, intel_dp->lane_count, &DP, false))
 		intel_dp->DP = DP;
 }
 
 static bool
 _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
-			      uint32_t *DP)
+			      uint32_t *DP, bool probing)
 {
 	bool channel_eq = false;
 	int tries, cr_tries;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
+	char *error_str;
 
 	/* Training Pattern 3 for HBR2 ot 1.2 devices that support it*/
 	if (intel_dp->link_bw == DP_LINK_BW_5_4 || intel_dp->use_tps3)
@@ -3702,7 +3728,11 @@  _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 	if (!intel_dp_set_link_train(intel_dp, DP,
 				     training_pattern |
 				     DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to start channel equalization\n");
+		error_str = "failed to start channel equalization\n";
+		if (!probing)
+			DRM_ERROR(error_str);
+		else
+			DRM_DEBUG(error_str);
 		return false;
 	}
 
@@ -3713,20 +3743,28 @@  _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 		uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 		if (cr_tries > 5) {
-			DRM_ERROR("failed to train DP, aborting\n");
+			error_str = "failed to train DP, aborting\n";
+			if (!probing)
+				DRM_ERROR(error_str);
+			else
+				DRM_DEBUG(error_str);
 			break;
 		}
 
 		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
-			DRM_ERROR("failed to get link status\n");
+			error_str = "failed to get link status\n";
+			if (!probing)
+				DRM_ERROR(error_str);
+			else
+				DRM_DEBUG(error_str);
 			break;
 		}
 
 		/* Make sure clock is still ok */
 		if (!drm_dp_clock_recovery_ok(link_status, lane_count)) {
 			intel_dp->train_set_valid = false;
-			_intel_dp_start_link_train(intel_dp, lane_count, DP);
+			_intel_dp_start_link_train(intel_dp, lane_count, DP, probing);
 			intel_dp_set_link_train(intel_dp, DP,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
@@ -3742,7 +3780,7 @@  _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 		/* Try 5 times, then try clock recovery if that fails */
 		if (tries > 5) {
 			intel_dp->train_set_valid = false;
-			_intel_dp_start_link_train(intel_dp);
+			_intel_dp_start_link_train(intel_dp, lane_count, DP, probing);
 			intel_dp_set_link_train(intel_dp, DP,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
@@ -3753,7 +3791,11 @@  _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
 
 		/* Update training set as requested by target */
 		if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
-			DRM_ERROR("failed to update link training\n");
+			error_str = "failed to update link training\n";
+			if (!probing)
+				DRM_ERROR(error_str);
+			else
+				DRM_DEBUG(error_str);
 			break;
 		}
 		++tries;
@@ -3774,7 +3816,7 @@  intel_dp_complete_link_train(struct intel_dp *intel_dp)
 {
 	uint32_t DP = intel_dp->DP;
 
-	if (_intel_dp_complete_link_train(intel_dp, intel_dp->lane_count, &DP))
+	if (_intel_dp_complete_link_train(intel_dp, intel_dp->lane_count, &DP, false))
 		intel_dp->DP = DP;
 }