drm/i915: Tune down link train error messages due to IO failure

Submitted by Imre Deak on May 23, 2019, 1:08 p.m.

Details

Message ID 20190523130836.6488-1-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Tune down link train error messages due to IO failure" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak May 23, 2019, 1:08 p.m.
An IO failure happens if the sink is unplugged. This scenario shouldn't
be logged with error level, since it is not a user visible problem.
Converting the corresponding error messages to be debug messages also
ensures that we don't fail some CI test incorrectly that tests the
unplugging while a mode is still active on the given connector (like the
kms_chamelium tests imititating such an unplug during being suspended).

Leave those link training error messages that are due to an actual
transfer rate error (and not an IO error) as-is, as that kind of problem
is related to the user's configuration (cable, mode, monitor) and so it
is really user visible.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 9b1fccea966b..392ebeb89ce8 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -169,7 +169,7 @@  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	if (!intel_dp_reset_link_train(intel_dp,
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to enable link training\n");
+		DRM_DEBUG_KMS("failed to enable link training\n");
 		return false;
 	}
 
@@ -193,7 +193,7 @@  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		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");
+			DRM_DEBUG_KMS("failed to get link status\n");
 			return false;
 		}
 
@@ -217,7 +217,7 @@  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		/* Update training set as requested by target */
 		intel_get_adjust_train(intel_dp, link_status);
 		if (!intel_dp_update_link_train(intel_dp)) {
-			DRM_ERROR("failed to update link training\n");
+			DRM_DEBUG_KMS("failed to update link training\n");
 			return false;
 		}
 
@@ -295,7 +295,7 @@  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	/* channel equalization */
 	if (!intel_dp_set_link_train(intel_dp,
 				     training_pattern)) {
-		DRM_ERROR("failed to start channel equalization\n");
+		DRM_DEBUG_KMS("failed to start channel equalization\n");
 		return false;
 	}
 
@@ -303,7 +303,7 @@  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 
 		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");
+			DRM_DEBUG_KMS("failed to get link status\n");
 			break;
 		}
 
@@ -327,7 +327,7 @@  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		/* Update training set as requested by target */
 		intel_get_adjust_train(intel_dp, link_status);
 		if (!intel_dp_update_link_train(intel_dp)) {
-			DRM_ERROR("failed to update link training\n");
+			DRM_DEBUG_KMS("failed to update link training\n");
 			break;
 		}
 	}

Comments

On 23/05/2019 16:08, Imre Deak wrote:
> An IO failure happens if the sink is unplugged. This scenario shouldn't
> be logged with error level, since it is not a user visible problem.
> Converting the corresponding error messages to be debug messages also
> ensures that we don't fail some CI test incorrectly that tests the
> unplugging while a mode is still active on the given connector (like the
> kms_chamelium tests imititating such an unplug during being suspended).
> 
> Leave those link training error messages that are due to an actual
> transfer rate error (and not an IO error) as-is, as that kind of problem
> is related to the user's configuration (cable, mode, monitor) and so it
> is really user visible.

Are there some scenarios that would be warranting a DRM_ERROR? If so,
then we should decide on the verbosity based on wether the connector is
connected or disconnected, don't you think?

Martin
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 9b1fccea966b..392ebeb89ce8 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -169,7 +169,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  	if (!intel_dp_reset_link_train(intel_dp,
>  				       DP_TRAINING_PATTERN_1 |
>  				       DP_LINK_SCRAMBLING_DISABLE)) {
> -		DRM_ERROR("failed to enable link training\n");
> +		DRM_DEBUG_KMS("failed to enable link training\n");
>  		return false;
>  	}
>  
> @@ -193,7 +193,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  		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");
> +			DRM_DEBUG_KMS("failed to get link status\n");
>  			return false;
>  		}
>  
> @@ -217,7 +217,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  		/* Update training set as requested by target */
>  		intel_get_adjust_train(intel_dp, link_status);
>  		if (!intel_dp_update_link_train(intel_dp)) {
> -			DRM_ERROR("failed to update link training\n");
> +			DRM_DEBUG_KMS("failed to update link training\n");
>  			return false;
>  		}
>  
> @@ -295,7 +295,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  	/* channel equalization */
>  	if (!intel_dp_set_link_train(intel_dp,
>  				     training_pattern)) {
> -		DRM_ERROR("failed to start channel equalization\n");
> +		DRM_DEBUG_KMS("failed to start channel equalization\n");
>  		return false;
>  	}
>  
> @@ -303,7 +303,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  
>  		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");
> +			DRM_DEBUG_KMS("failed to get link status\n");
>  			break;
>  		}
>  
> @@ -327,7 +327,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  		/* Update training set as requested by target */
>  		intel_get_adjust_train(intel_dp, link_status);
>  		if (!intel_dp_update_link_train(intel_dp)) {
> -			DRM_ERROR("failed to update link training\n");
> +			DRM_DEBUG_KMS("failed to update link training\n");
>  			break;
>  		}
>  	}
>
On Fri, May 24, 2019 at 07:27:54PM +0300, Martin Peres wrote:
> On 23/05/2019 16:08, Imre Deak wrote:
> > An IO failure happens if the sink is unplugged. This scenario shouldn't
> > be logged with error level, since it is not a user visible problem.
> > Converting the corresponding error messages to be debug messages also
> > ensures that we don't fail some CI test incorrectly that tests the
> > unplugging while a mode is still active on the given connector (like the
> > kms_chamelium tests imititating such an unplug during being suspended).
> > 
> > Leave those link training error messages that are due to an actual
> > transfer rate error (and not an IO error) as-is, as that kind of problem
> > is related to the user's configuration (cable, mode, monitor) and so it
> > is really user visible.
> 
> Are there some scenarios that would be warranting a DRM_ERROR? If so,
> then we should decide on the verbosity based on wether the connector is
> connected or disconnected, don't you think?

I don't think we should depend on the correctness of setting the message
level correctly based on the live state, since the live state itself is
flaky/unreliable based on the timing of the unplug vs. the settling of
the live state flag. For instance we could see the AUX IO failing
already but live state still indicating that the sink is plugged, so
we'd print again an error instead of the correct tuned-down note.

> 
> Martin
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 9b1fccea966b..392ebeb89ce8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -169,7 +169,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  	if (!intel_dp_reset_link_train(intel_dp,
> >  				       DP_TRAINING_PATTERN_1 |
> >  				       DP_LINK_SCRAMBLING_DISABLE)) {
> > -		DRM_ERROR("failed to enable link training\n");
> > +		DRM_DEBUG_KMS("failed to enable link training\n");
> >  		return false;
> >  	}
> >  
> > @@ -193,7 +193,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  		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");
> > +			DRM_DEBUG_KMS("failed to get link status\n");
> >  			return false;
> >  		}
> >  
> > @@ -217,7 +217,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  		/* Update training set as requested by target */
> >  		intel_get_adjust_train(intel_dp, link_status);
> >  		if (!intel_dp_update_link_train(intel_dp)) {
> > -			DRM_ERROR("failed to update link training\n");
> > +			DRM_DEBUG_KMS("failed to update link training\n");
> >  			return false;
> >  		}
> >  
> > @@ -295,7 +295,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >  	/* channel equalization */
> >  	if (!intel_dp_set_link_train(intel_dp,
> >  				     training_pattern)) {
> > -		DRM_ERROR("failed to start channel equalization\n");
> > +		DRM_DEBUG_KMS("failed to start channel equalization\n");
> >  		return false;
> >  	}
> >  
> > @@ -303,7 +303,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >  
> >  		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");
> > +			DRM_DEBUG_KMS("failed to get link status\n");
> >  			break;
> >  		}
> >  
> > @@ -327,7 +327,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >  		/* Update training set as requested by target */
> >  		intel_get_adjust_train(intel_dp, link_status);
> >  		if (!intel_dp_update_link_train(intel_dp)) {
> > -			DRM_ERROR("failed to update link training\n");
> > +			DRM_DEBUG_KMS("failed to update link training\n");
> >  			break;
> >  		}
> >  	}
> > 
>
On Fri, May 24, 2019 at 07:49:07PM +0300, Imre Deak wrote:
> On Fri, May 24, 2019 at 07:27:54PM +0300, Martin Peres wrote:
> > On 23/05/2019 16:08, Imre Deak wrote:
> > > An IO failure happens if the sink is unplugged. This scenario shouldn't
> > > be logged with error level, since it is not a user visible problem.
> > > Converting the corresponding error messages to be debug messages also
> > > ensures that we don't fail some CI test incorrectly that tests the
> > > unplugging while a mode is still active on the given connector (like the
> > > kms_chamelium tests imititating such an unplug during being suspended).
> > > 
> > > Leave those link training error messages that are due to an actual
> > > transfer rate error (and not an IO error) as-is, as that kind of problem
> > > is related to the user's configuration (cable, mode, monitor) and so it
> > > is really user visible.
> > 
> > Are there some scenarios that would be warranting a DRM_ERROR? If so,
> > then we should decide on the verbosity based on wether the connector is
> > connected or disconnected, don't you think?
> 
> I don't think we should depend on the correctness of setting the message
> level correctly based on the live state, since the live state itself is
> flaky/unreliable based on the timing of the unplug vs. the settling of
> the live state flag. For instance we could see the AUX IO failing
> already but live state still indicating that the sink is plugged, so
> we'd print again an error instead of the correct tuned-down note.
>

But in case of AUX failures which we see a lot when AUX power
is accidently off or eDP is still not ON or sink is unplugged while
the link training was in process, doesnt it make sense to
call it as a DRM_ERROR and not just a DEBUG_KMS.
For Eg: the CI tests failed due to AUX timeouts should be the actual failures
which would be filtered now if we tune it down to DEBUG_KMS.

Manasi
 
> > 
> > Martin
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 9b1fccea966b..392ebeb89ce8 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -169,7 +169,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > >  	if (!intel_dp_reset_link_train(intel_dp,
> > >  				       DP_TRAINING_PATTERN_1 |
> > >  				       DP_LINK_SCRAMBLING_DISABLE)) {
> > > -		DRM_ERROR("failed to enable link training\n");
> > > +		DRM_DEBUG_KMS("failed to enable link training\n");
> > >  		return false;
> > >  	}
> > >  
> > > @@ -193,7 +193,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > >  		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");
> > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > >  			return false;
> > >  		}
> > >  
> > > @@ -217,7 +217,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > >  		/* Update training set as requested by target */
> > >  		intel_get_adjust_train(intel_dp, link_status);
> > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > -			DRM_ERROR("failed to update link training\n");
> > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > >  			return false;
> > >  		}
> > >  
> > > @@ -295,7 +295,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > >  	/* channel equalization */
> > >  	if (!intel_dp_set_link_train(intel_dp,
> > >  				     training_pattern)) {
> > > -		DRM_ERROR("failed to start channel equalization\n");
> > > +		DRM_DEBUG_KMS("failed to start channel equalization\n");
> > >  		return false;
> > >  	}
> > >  
> > > @@ -303,7 +303,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > >  
> > >  		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");
> > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > >  			break;
> > >  		}
> > >  
> > > @@ -327,7 +327,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > >  		/* Update training set as requested by target */
> > >  		intel_get_adjust_train(intel_dp, link_status);
> > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > -			DRM_ERROR("failed to update link training\n");
> > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > >  			break;
> > >  		}
> > >  	}
> > > 
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 24, 2019 at 11:32:36AM -0700, Manasi Navare wrote:
> On Fri, May 24, 2019 at 07:49:07PM +0300, Imre Deak wrote:
> > On Fri, May 24, 2019 at 07:27:54PM +0300, Martin Peres wrote:
> > > On 23/05/2019 16:08, Imre Deak wrote:
> > > > An IO failure happens if the sink is unplugged. This scenario shouldn't
> > > > be logged with error level, since it is not a user visible problem.
> > > > Converting the corresponding error messages to be debug messages also
> > > > ensures that we don't fail some CI test incorrectly that tests the
> > > > unplugging while a mode is still active on the given connector (like the
> > > > kms_chamelium tests imititating such an unplug during being suspended).
> > > > 
> > > > Leave those link training error messages that are due to an actual
> > > > transfer rate error (and not an IO error) as-is, as that kind of problem
> > > > is related to the user's configuration (cable, mode, monitor) and so it
> > > > is really user visible.
> > > 
> > > Are there some scenarios that would be warranting a DRM_ERROR? If so,
> > > then we should decide on the verbosity based on wether the connector is
> > > connected or disconnected, don't you think?
> > 
> > I don't think we should depend on the correctness of setting the message
> > level correctly based on the live state, since the live state itself is
> > flaky/unreliable based on the timing of the unplug vs. the settling of
> > the live state flag. For instance we could see the AUX IO failing
> > already but live state still indicating that the sink is plugged, so
> > we'd print again an error instead of the correct tuned-down note.
> 
> But in case of AUX failures which we see a lot when AUX power
> is accidently off or eDP is still not ON or sink is unplugged while
> the link training was in process, doesnt it make sense to
> call it as a DRM_ERROR and not just a DEBUG_KMS.

Logging such AUX fails as DEBUG makes sense, since it will be up to
a developer to look through the logs if needed; for instance if it leads
to failing the actual link training logged with DRM_ERROR or failing the
detection when you expect the sink to be there.

Otoh, it's a bad thing to log such AUX fails as DRM_ERROR as will result
in failing tests incorrectly when the sink is disconnected and the AUX
fails happen as expected.

> For Eg: the CI tests failed due to AUX timeouts should be the actual failures
> which would be filtered now if we tune it down to DEBUG_KMS.

These will lead to actually failing the detection/link training, which
you can depend on failing the tests instead of depending on error
messages in dmesg.

> 
> Manasi
>  
> > > 
> > > Martin
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > index 9b1fccea966b..392ebeb89ce8 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > @@ -169,7 +169,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  	if (!intel_dp_reset_link_train(intel_dp,
> > > >  				       DP_TRAINING_PATTERN_1 |
> > > >  				       DP_LINK_SCRAMBLING_DISABLE)) {
> > > > -		DRM_ERROR("failed to enable link training\n");
> > > > +		DRM_DEBUG_KMS("failed to enable link training\n");
> > > >  		return false;
> > > >  	}
> > > >  
> > > > @@ -193,7 +193,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  		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");
> > > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > > >  			return false;
> > > >  		}
> > > >  
> > > > @@ -217,7 +217,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  		/* Update training set as requested by target */
> > > >  		intel_get_adjust_train(intel_dp, link_status);
> > > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > > -			DRM_ERROR("failed to update link training\n");
> > > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > > >  			return false;
> > > >  		}
> > > >  
> > > > @@ -295,7 +295,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > >  	/* channel equalization */
> > > >  	if (!intel_dp_set_link_train(intel_dp,
> > > >  				     training_pattern)) {
> > > > -		DRM_ERROR("failed to start channel equalization\n");
> > > > +		DRM_DEBUG_KMS("failed to start channel equalization\n");
> > > >  		return false;
> > > >  	}
> > > >  
> > > > @@ -303,7 +303,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > >  
> > > >  		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");
> > > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > > >  			break;
> > > >  		}
> > > >  
> > > > @@ -327,7 +327,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > >  		/* Update training set as requested by target */
> > > >  		intel_get_adjust_train(intel_dp, link_status);
> > > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > > -			DRM_ERROR("failed to update link training\n");
> > > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > > >  			break;
> > > >  		}
> > > >  	}
> > > > 
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 24, 2019 at 10:04:10PM +0300, Imre Deak wrote:
> On Fri, May 24, 2019 at 11:32:36AM -0700, Manasi Navare wrote:
> > On Fri, May 24, 2019 at 07:49:07PM +0300, Imre Deak wrote:
> > > On Fri, May 24, 2019 at 07:27:54PM +0300, Martin Peres wrote:
> > > > On 23/05/2019 16:08, Imre Deak wrote:
> > > > > An IO failure happens if the sink is unplugged. This scenario shouldn't
> > > > > be logged with error level, since it is not a user visible problem.
> > > > > Converting the corresponding error messages to be debug messages also
> > > > > ensures that we don't fail some CI test incorrectly that tests the
> > > > > unplugging while a mode is still active on the given connector (like the
> > > > > kms_chamelium tests imititating such an unplug during being suspended).
> > > > > 
> > > > > Leave those link training error messages that are due to an actual
> > > > > transfer rate error (and not an IO error) as-is, as that kind of problem
> > > > > is related to the user's configuration (cable, mode, monitor) and so it
> > > > > is really user visible.
> > > > 
> > > > Are there some scenarios that would be warranting a DRM_ERROR? If so,
> > > > then we should decide on the verbosity based on wether the connector is
> > > > connected or disconnected, don't you think?
> > > 
> > > I don't think we should depend on the correctness of setting the message
> > > level correctly based on the live state, since the live state itself is
> > > flaky/unreliable based on the timing of the unplug vs. the settling of
> > > the live state flag. For instance we could see the AUX IO failing
> > > already but live state still indicating that the sink is plugged, so
> > > we'd print again an error instead of the correct tuned-down note.
> > 
> > But in case of AUX failures which we see a lot when AUX power
> > is accidently off or eDP is still not ON or sink is unplugged while
> > the link training was in process, doesnt it make sense to
> > call it as a DRM_ERROR and not just a DEBUG_KMS.
> 
> Logging such AUX fails as DEBUG makes sense, since it will be up to
> a developer to look through the logs if needed; for instance if it leads
> to failing the actual link training logged with DRM_ERROR or failing the
> detection when you expect the sink to be there.
> 
> Otoh, it's a bad thing to log such AUX fails as DRM_ERROR as will result
> in failing tests incorrectly when the sink is disconnected and the AUX
> fails happen as expected.
> 
> > For Eg: the CI tests failed due to AUX timeouts should be the actual failures
> > which would be filtered now if we tune it down to DEBUG_KMS.
> 
> These will lead to actually failing the detection/link training, which
> you can depend on failing the tests instead of depending on error
> messages in dmesg.

Okay makes sense. So basically rely on the final result of training only
to decide if its a real failure or not.
Got it.
Looks good to me in that case.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> 
> > 
> > Manasi
> >  
> > > > 
> > > > Martin
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > > index 9b1fccea966b..392ebeb89ce8 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > > @@ -169,7 +169,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > >  	if (!intel_dp_reset_link_train(intel_dp,
> > > > >  				       DP_TRAINING_PATTERN_1 |
> > > > >  				       DP_LINK_SCRAMBLING_DISABLE)) {
> > > > > -		DRM_ERROR("failed to enable link training\n");
> > > > > +		DRM_DEBUG_KMS("failed to enable link training\n");
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > @@ -193,7 +193,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > >  		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");
> > > > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > > > >  			return false;
> > > > >  		}
> > > > >  
> > > > > @@ -217,7 +217,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > >  		/* Update training set as requested by target */
> > > > >  		intel_get_adjust_train(intel_dp, link_status);
> > > > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > > > -			DRM_ERROR("failed to update link training\n");
> > > > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > > > >  			return false;
> > > > >  		}
> > > > >  
> > > > > @@ -295,7 +295,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > > >  	/* channel equalization */
> > > > >  	if (!intel_dp_set_link_train(intel_dp,
> > > > >  				     training_pattern)) {
> > > > > -		DRM_ERROR("failed to start channel equalization\n");
> > > > > +		DRM_DEBUG_KMS("failed to start channel equalization\n");
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > @@ -303,7 +303,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > > >  
> > > > >  		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");
> > > > > +			DRM_DEBUG_KMS("failed to get link status\n");
> > > > >  			break;
> > > > >  		}
> > > > >  
> > > > > @@ -327,7 +327,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > > >  		/* Update training set as requested by target */
> > > > >  		intel_get_adjust_train(intel_dp, link_status);
> > > > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > > > > -			DRM_ERROR("failed to update link training\n");
> > > > > +			DRM_DEBUG_KMS("failed to update link training\n");
> > > > >  			break;
> > > > >  		}
> > > > >  	}
> > > > > 
> > > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx