Re: [Intel-gfx] [PATCH v2 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 08, 2022 at 02:12:33PM +0200, Jani Nikula wrote:
> On Tue, 08 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Feb 08, 2022 at 11:17:22AM +0200, Jani Nikula wrote:
> >> On Fri, 04 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Thu, Feb 03, 2022 at 11:03:54AM +0200, Jani Nikula wrote:
> >> >> +
> >> >> +		if (timeout) {
> >> >> +			intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
> >> >> +			drm_err(&i915->drm,
> >> >> +				"[ENCODER:%d:%s] Lane channel eq timeout\n",
> >> >> +				encoder->base.base.id, encoder->base.name);
> >> >> +			return false;
> >> >> +		}
> >> >> +
> >> >> +		if (time_after(jiffies, deadline))
> >> >> +			timeout = true; /* try one last time after deadline */
> >> >
> >> > Is there a reason we can't do this just before drm_dp_dpcd_read_link_status()
> >> > so we don't have to pass the timeout status from one loop iteration to
> >> > the next?
> >> 
> >> The point is to check one last time after timeout has passed, like you
> >> suggested in previous review, and I agreed.
> >
> > Sure but why can't it be something more like?
> >
> > timeout = time_after();
> > read_status();
> > if (bad)
> > 	bail;
> > if (timeout)
> > 	bail;
> >
> > I think we have it more like that in wait_for()/etc.
> 
> I was going to fix this, but then realized the "one more time" really
> only makes sense if it includes updating the signal levels and training
> set and then checking the status. I don't think there's point in "one
> more time" only covering the status read.

Hmm. Yeah, I suppose that is true. We can't really know when the sink
updated the status so checking for the timeout just before that might
have the same issue as checking entirely after the status check.

> 
> I've got the loop set up such that the flow is natural when entering the
> loop i.e. I'd rather not have the adjust in the beginning with some if
> (try != 0) check.
> 
> Or am I missing something?

Nah. I guess it's best leave it the way you have it now.

-- 
Ville Syrjälä
Intel