[RFC 1/4] drm/i915/display/dp: Add DP fallback on LT

[Jani Nikula]

On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
>> >> And with the current code, DP_CAP_ANSI_128B132B does not decide
>> >> whether we use DP MST or not. So this will also cover 8b/10b fallback
>> >> for displays that support 128b/132b but have DP_MSTM_CAP == 0.
>> >
>> > Yes, the series doent depend on MST and SST and doest fallback from MST to
>> SST or viceversa.
>>
>> What I'm saying is, this changes the way 8b/10b link training fallback is
>> handled.
>>
> The first loop has a if condition for 128/132b and is executed only if its 128/132b and if not falls to the existing code. i.e 8/10b link training fallback sequence.

You check for sink 128b/132b capability. Please take the time to
consider what this means. I've tried to explain this a few times now.

>> First, it starts handling 8b/10b MST link training fallback.
>>
> As far as I see, at the entry of this function 128/132b is checked and link training fallback values for this obtained and if not link training fallback values for 8/10b is obtained. Have taken care as to not modify the existing 8/10b fallback.

Same as above.

>> Second, it changes the way 8b/10b *and* 128b/132b *and* SST *and* MST link
>> training fallback is handled for all displays that support 128b/132b channel
>> coding.
>>
> MST/SST configuration and then the link training happens. This link training by writing to dpcd registers is done over here by sending certain patterns. The fallback in this RFC is done only in this small link training sequence. On failure the handler doesn't return back instead retry from starting of link training is done. MST/SST configuration is not touched upon, if any required for this as part of fallback can be taken up in the next step.
> This RFC is aiming to achieve fallback for the link training sequence only.

To be clear, I don't want to change the way link training failure
fallback is handled in general. It should go via userspace. Please let's
just not go there, at all. Changing it does not help us right now, it
just adds another complication, and another code path to test.

But regardless of that, I don't think you rightly appreciate what
implications your changes actually have. The code does not do what you
claim it does. I don't know what else to say.

>> >> intel_dp_set_link_params() is supposed to be called in the DP encoder
>> >> (pre- )enable paths to set the link rates. If you do it here, the
>> >> subsequent enable will just overwrite whatever you did here.
>> > This is taken care of so as to not override and retain this fallback value.
>>
>> I don't understand.
>>
> With the existing code the driver sends uevent and a new modeset along with dp_init is done and the values will be overwritten. In this RFC we don't send uevent for all the fallback values instead re-iterate only the link training part without touching the dp enable sequence.

I any patch series, no matter what you're working on, each patch in the
series must stand on its own merits. Patches can't depend on something
that may or may not be done later in subsequent patches.

BR,
Jani.

-- 
Jani Nikula, Intel