| Message ID | 20240206104759.2079133-5-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DP link training failure fallback | expand |
On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > On link training failure retry link training with a lesser link > rate/lane count as specified in the DP spec. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index ed7620e7f763..29d785a4b904 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -2502,6 +2502,7 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state, > crtc_state->port_clock, > crtc_state->lane_count); > > +retry: > /* > * We only configure what the register value will be here. Actual > * enabling happens during link training farther down. > @@ -2586,7 +2587,14 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state, > * Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent) > * (timeout after 800 us) > */ > - intel_dp_start_link_train(intel_dp, crtc_state); > + if (!intel_dp_start_link_train(intel_dp, crtc_state)) { > + /* Link Training failed, retain */ > + intel_dp->link_trained = false; > + intel_dp_stop_link_train(intel_dp, crtc_state); > + encoder->post_disable(state, encoder, > + crtc_state, conn_state); > + goto retry; > + } As said, the retry needs to go via userspace. BR, Jani. > > /* 6.n Set DP_TP_CTL link training to Normal */ > if (!is_trans_port_sync_mode(crtc_state))
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Tuesday, February 13, 2024 11:45 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; > Shankar, Uma <uma.shankar@intel.com>; Murthy, Arun R > <arun.r.murthy@intel.com> > Subject: Re: [RFC 4/4] drm/i915/display/dp: On LT failure retry LT > > On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > > On link training failure retry link training with a lesser link > > rate/lane count as specified in the DP spec. > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index ed7620e7f763..29d785a4b904 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -2502,6 +2502,7 @@ static void mtl_ddi_pre_enable_dp(struct > intel_atomic_state *state, > > crtc_state->port_clock, > > crtc_state->lane_count); > > > > +retry: > > /* > > * We only configure what the register value will be here. Actual > > * enabling happens during link training farther down. > > @@ -2586,7 +2587,14 @@ static void mtl_ddi_pre_enable_dp(struct > intel_atomic_state *state, > > * Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent) > > * (timeout after 800 us) > > */ > > - intel_dp_start_link_train(intel_dp, crtc_state); > > + if (!intel_dp_start_link_train(intel_dp, crtc_state)) { > > + /* Link Training failed, retain */ > > + intel_dp->link_trained = false; > > + intel_dp_stop_link_train(intel_dp, crtc_state); > > + encoder->post_disable(state, encoder, > > + crtc_state, conn_state); > > + goto retry; > > + } > > As said, the retry needs to go via userspace. If within the supported mode range then also do we need to send uevent to user and should it come via userspace? The fallback mandates in DP2.1 spec does this fallback in a loop. The present fallback structure Struct dp_fallback { U32 link rate; U8 lane_count; U32 resolution; } In the same fallback code, the present mode will be verified to see if its less than or equal to the resolution in dp_fallback. If so proceed within the fallback loop else set the max link_rate/lane count values and sent uevent. Thanks and Regards, Arun R Murthy -------------------- > > BR, > Jani. > > > > > > /* 6.n Set DP_TP_CTL link training to Normal */ > > if (!is_trans_port_sync_mode(crtc_state)) > > -- > Jani Nikula, Intel
On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani <jani.nikula@intel.com> >> Sent: Tuesday, February 13, 2024 11:45 PM >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org >> Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; >> Shankar, Uma <uma.shankar@intel.com>; Murthy, Arun R >> <arun.r.murthy@intel.com> >> Subject: Re: [RFC 4/4] drm/i915/display/dp: On LT failure retry LT >> >> On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: >> > On link training failure retry link training with a lesser link >> > rate/lane count as specified in the DP spec. >> > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++++- >> > 1 file changed, 9 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c >> > b/drivers/gpu/drm/i915/display/intel_ddi.c >> > index ed7620e7f763..29d785a4b904 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> > @@ -2502,6 +2502,7 @@ static void mtl_ddi_pre_enable_dp(struct >> intel_atomic_state *state, >> > crtc_state->port_clock, >> > crtc_state->lane_count); >> > >> > +retry: >> > /* >> > * We only configure what the register value will be here. Actual >> > * enabling happens during link training farther down. >> > @@ -2586,7 +2587,14 @@ static void mtl_ddi_pre_enable_dp(struct >> intel_atomic_state *state, >> > * Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent) >> > * (timeout after 800 us) >> > */ >> > - intel_dp_start_link_train(intel_dp, crtc_state); >> > + if (!intel_dp_start_link_train(intel_dp, crtc_state)) { >> > + /* Link Training failed, retain */ >> > + intel_dp->link_trained = false; >> > + intel_dp_stop_link_train(intel_dp, crtc_state); >> > + encoder->post_disable(state, encoder, >> > + crtc_state, conn_state); >> > + goto retry; >> > + } >> >> As said, the retry needs to go via userspace. > > If within the supported mode range then also do we need to send uevent to user and should it come via userspace? > The fallback mandates in DP2.1 spec does this fallback in a loop. > > The present fallback structure > Struct dp_fallback { > U32 link rate; > U8 lane_count; > U32 resolution; > } > > In the same fallback code, the present mode will be verified to see if its less than or equal to the resolution in dp_fallback. If so proceed within the fallback loop else set the max link_rate/lane count values and sent uevent. I think I'll want *all* the link training fallbacks to go via userspace. Trying to sometimes do it in kernel is a premature optimization for a rare case, and it just complicates matters. We'll need the path via uevent and userspace retry anyway, for when the mode doesn't fit, so use it always. Let's not add multiple ways to do things, everything around this is already quite complicated. And as said, the uevent does give userspace some inkling that something fishy is going on, and could use that info to inform the user that a degraded experience may be expected. Again, adding a new stream to MST at a later time might not fit because of the reduced parameters, and it'll be surprising to the user if it used to work in the past (when full param link training succeeded). BR, Jani. > > Thanks and Regards, > Arun R Murthy > -------------------- >> >> BR, >> Jani. >> >> >> > >> > /* 6.n Set DP_TP_CTL link training to Normal */ >> > if (!is_trans_port_sync_mode(crtc_state)) >> >> -- >> Jani Nikula, Intel
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Wednesday, February 14, 2024 5:01 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; > Shankar, Uma <uma.shankar@intel.com> > Subject: RE: [RFC 4/4] drm/i915/display/dp: On LT failure retry LT > > On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: > >> -----Original Message----- > >> From: Nikula, Jani <jani.nikula@intel.com> > >> Sent: Tuesday, February 13, 2024 11:45 PM > >> To: Murthy, Arun R <arun.r.murthy@intel.com>; > >> intel-gfx@lists.freedesktop.org > >> Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville > >> <ville.syrjala@intel.com>; Shankar, Uma <uma.shankar@intel.com>; > >> Murthy, Arun R <arun.r.murthy@intel.com> > >> Subject: Re: [RFC 4/4] drm/i915/display/dp: On LT failure retry LT > >> > >> On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > >> > On link training failure retry link training with a lesser link > >> > rate/lane count as specified in the DP spec. > >> > > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++++- > >> > 1 file changed, 9 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > >> > b/drivers/gpu/drm/i915/display/intel_ddi.c > >> > index ed7620e7f763..29d785a4b904 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >> > @@ -2502,6 +2502,7 @@ static void mtl_ddi_pre_enable_dp(struct > >> intel_atomic_state *state, > >> > crtc_state->port_clock, > >> > crtc_state->lane_count); > >> > > >> > +retry: > >> > /* > >> > * We only configure what the register value will be here. Actual > >> > * enabling happens during link training farther down. > >> > @@ -2586,7 +2587,14 @@ static void mtl_ddi_pre_enable_dp(struct > >> intel_atomic_state *state, > >> > * Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent) > >> > * (timeout after 800 us) > >> > */ > >> > - intel_dp_start_link_train(intel_dp, crtc_state); > >> > + if (!intel_dp_start_link_train(intel_dp, crtc_state)) { > >> > + /* Link Training failed, retain */ > >> > + intel_dp->link_trained = false; > >> > + intel_dp_stop_link_train(intel_dp, crtc_state); > >> > + encoder->post_disable(state, encoder, > >> > + crtc_state, conn_state); > >> > + goto retry; > >> > + } > >> > >> As said, the retry needs to go via userspace. > > > > If within the supported mode range then also do we need to send uevent to > user and should it come via userspace? > > The fallback mandates in DP2.1 spec does this fallback in a loop. > > > > The present fallback structure > > Struct dp_fallback { > > U32 link rate; > > U8 lane_count; > > U32 resolution; > > } > > > > In the same fallback code, the present mode will be verified to see if its less > than or equal to the resolution in dp_fallback. If so proceed within the fallback > loop else set the max link_rate/lane count values and sent uevent. > > I think I'll want *all* the link training fallbacks to go via userspace. > Wouldn't this be an optimized way of handling the fallback values. Figure 3-52 of the DP2.1 spec also says to restart from the beginning of link training. > Trying to sometimes do it in kernel is a premature optimization for a rare case, > and it just complicates matters. We'll need the path via uevent and userspace > retry anyway, for when the mode doesn't fit, so use it always. Let's not add > multiple ways to do things, everything around this is already quite complicated. > When a mode change required due to limitation of the new fallback link rate, will use the existing path of sending uevent. I felt taking would approach would an optimal way of handling fallback. I am open to remove this optimization and take the uevent path always if *required*. > And as said, the uevent does give userspace some inkling that something fishy > is going on, and could use that info to inform the user that a degraded > experience may be expected. Again, adding a new stream to MST at a later time > might not fit because of the reduced parameters, and it'll be surprising to the > user if it used to work in the past (when full param link training succeeded). This FRC is not targeting to have fallback of MST streams. Its targeting only the link training for a particular stream only. As said above if no *optimization* is required and uevent path to be taken always for any fallback value I am open to remove this optimization and implement the way you suggest. Thanks and Regards, Arun R Murthy ------------------- > > BR, > Jani. > > > > > Thanks and Regards, > > Arun R Murthy > > -------------------- > >> > >> BR, > >> Jani. > >> > >> > >> > > >> > /* 6.n Set DP_TP_CTL link training to Normal */ > >> > if (!is_trans_port_sync_mode(crtc_state)) > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index ed7620e7f763..29d785a4b904 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -2502,6 +2502,7 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state, crtc_state->port_clock, crtc_state->lane_count); +retry: /* * We only configure what the register value will be here. Actual * enabling happens during link training farther down. @@ -2586,7 +2587,14 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state, * Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent) * (timeout after 800 us) */ - intel_dp_start_link_train(intel_dp, crtc_state); + if (!intel_dp_start_link_train(intel_dp, crtc_state)) { + /* Link Training failed, retain */ + intel_dp->link_trained = false; + intel_dp_stop_link_train(intel_dp, crtc_state); + encoder->post_disable(state, encoder, + crtc_state, conn_state); + goto retry; + } /* 6.n Set DP_TP_CTL link training to Normal */ if (!is_trans_port_sync_mode(crtc_state))
On link training failure retry link training with a lesser link rate/lane count as specified in the DP spec. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)