Re: [PATCH] drm/i915/dp: Fix the channel equalization failure condition during Link Training

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Aug 24, 2017 at 10:15:42AM -0700, Manasi Navare wrote:
> On Thu, Aug 24, 2017 at 03:33:27PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 17, 2017 at 08:03:04PM -0700, Manasi Navare wrote:
> > > In the channel EQ retry loop, we break from the loop in case
> > > of failure to get link status or failure in clock recovery or
> > > failure to update link training. In these cases channel_eq_status
> > > is still false even though the retry loop has not reached max retries.
> > > So we need to consider this as a failure condition.
> > 
> > This just prints a debug message, and each break in the loop also
> > prints some kind of debug/error message. So to me this just seems to
> > make it harder to see why things failed because we now point the finger
> > at channel EQ even when the actual error was something else.
> >
> 
> Thanks for your feedback.
> The idea is to have one common place for failure so that
> failure handling like in case of PSR can be done only
> in that one place.
> The debug prints for each failure case would still give the
> exact reason for failure followed by the end result debug print
> saying that "Channel EQ failed" since any of those failures will
> result in Ch EQ fail.
> 
> Manasi
>

Ville, we would need this common exit point for the Channel EQ failures
especially in case where we set the train_set_valid to false in case of
any Channel EQ failures like in the patch:
"drm/i915/edp: Be less aggressive about changing link config on eDP"
So we need to move all the failure handling to the common place like I do
here in this patch if (tries == 5 || !intel_dp->channel_eq_status)
So I am not just adding the DEBUG print but also adding this condition
that if tries ==5 or if channel_eq_status is still false.
This is the place where, train_set_valid gets set to false.

Manasi

> > > 
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 05907fa..3fef219 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -294,9 +294,9 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > >  	}
> > >  
> > >  	/* Try 5 times, else fail and try at lower BW */
> > > -	if (tries == 5) {
> > > +	if (tries == 5 || !intel_dp->channel_eq_status) {
> > >  		intel_dp_dump_link_status(link_status);
> > > -		DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
> > > +		DRM_DEBUG_KMS("Channel equalization failed\n");
> > >  	}
> > >  
> > >  	intel_dp_set_idle_link_train(intel_dp);
> > > -- 
> > > 2.1.4
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx