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 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.

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?


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center