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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 08 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

Thanks. Sent v3, but realized I'm still missing the intra-hop stuff.



-- 
Jani Nikula, Intel Open Source Graphics Center