Re: [PATCH] drm/i915: Tune down link train error messages due to IO failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

Manasi

> 
> > 
> > Manasi
> >  
> > > > 
> > > > Martin
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110390
> > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > > ---
> > > > >  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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux