| Message ID | 20240520185822.3725844-13-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dp_mst: Enable link training fallback | expand |
On Mon, May 20, 2024 at 09:58:10PM +0300, Imre Deak wrote: > Simplify things by retraining a DP link if a bad link is detected in the > connector detect handler from the encoder's check link state work, > similarly to how this is done after a modeset link training failure. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index ff4ed6bb520d8..70b00e5ae7ad7 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5863,11 +5863,8 @@ intel_dp_detect(struct drm_connector *connector, > * Some external monitors do not signal loss of link synchronization > * with an IRQ_HPD, so force a link status check. > */ > - if (!intel_dp_is_edp(intel_dp)) { > - ret = intel_dp_retrain_link(encoder, ctx); > - if (ret) > - return ret; > - } > + if (!intel_dp_is_edp(intel_dp)) > + intel_dp_check_link_state(intel_dp); I would like to see this hack nuked entirely. But that could be a followup. > > /* > * Clearing NACK and defer counts to get their exact values > -- > 2.43.3
On Thu, May 23, 2024 at 06:08:40PM +0300, Ville Syrjälä wrote: > On Mon, May 20, 2024 at 09:58:10PM +0300, Imre Deak wrote: > > Simplify things by retraining a DP link if a bad link is detected in the > > connector detect handler from the encoder's check link state work, > > similarly to how this is done after a modeset link training failure. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index ff4ed6bb520d8..70b00e5ae7ad7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -5863,11 +5863,8 @@ intel_dp_detect(struct drm_connector *connector, > > * Some external monitors do not signal loss of link synchronization > > * with an IRQ_HPD, so force a link status check. > > */ > > - if (!intel_dp_is_edp(intel_dp)) { > > - ret = intel_dp_retrain_link(encoder, ctx); > > - if (ret) > > - return ret; > > - } > > + if (!intel_dp_is_edp(intel_dp)) > > + intel_dp_check_link_state(intel_dp); > > I would like to see this hack nuked entirely. But that > could be a followup. Okay. This tries to keep the current behavior, but can add a note that the above workaround can be removed after the link state is checked after modesets. I also wondered about the link state check in the hotplug handler. If that's only a way to defer doing this from the HPD IRQ handler - which is now changed by patch 13 - that could be also removed eventually? > > > > > /* > > * Clearing NACK and defer counts to get their exact values > > -- > > 2.43.3 > > -- > Ville Syrjälä > Intel
On Thu, May 23, 2024 at 06:29:21PM +0300, Imre Deak wrote: > On Thu, May 23, 2024 at 06:08:40PM +0300, Ville Syrjälä wrote: > > On Mon, May 20, 2024 at 09:58:10PM +0300, Imre Deak wrote: > > > Simplify things by retraining a DP link if a bad link is detected in the > > > connector detect handler from the encoder's check link state work, > > > similarly to how this is done after a modeset link training failure. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++----- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index ff4ed6bb520d8..70b00e5ae7ad7 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -5863,11 +5863,8 @@ intel_dp_detect(struct drm_connector *connector, > > > * Some external monitors do not signal loss of link synchronization > > > * with an IRQ_HPD, so force a link status check. > > > */ > > > - if (!intel_dp_is_edp(intel_dp)) { > > > - ret = intel_dp_retrain_link(encoder, ctx); > > > - if (ret) > > > - return ret; > > > - } > > > + if (!intel_dp_is_edp(intel_dp)) > > > + intel_dp_check_link_state(intel_dp); > > > > I would like to see this hack nuked entirely. But that > > could be a followup. > > Okay. This tries to keep the current behavior, but can add a note that > the above workaround can be removed after the link state is checked > after modesets. > > I also wondered about the link state check in the hotplug handler. If > that's only a way to defer doing this from the HPD IRQ handler - which > is now changed by patch 13 - that could be also removed eventually? Not sure which one you want to removed exactly. I presume there are still at least these cases we need to handle: - long HDP dropped and came back without any userspace initiated modeset in between -> kick off a retrain from the long HPD handler - short HPD indicated some link badness -> kick off a retrain from the short HDP handler - link dropped on its own soon after modeset without any HPD for some reason -> kick off a retrain from the post modeset link check And the one we should remove: - something weird happened to the link and no one noticed, and for some random reason userspace just happens to do a getconnector() which ends up randomly fixing things Did I miss anything else?
On Thu, May 23, 2024 at 06:43:40PM +0300, Ville Syrjälä wrote: > On Thu, May 23, 2024 at 06:29:21PM +0300, Imre Deak wrote: > > On Thu, May 23, 2024 at 06:08:40PM +0300, Ville Syrjälä wrote: > > > On Mon, May 20, 2024 at 09:58:10PM +0300, Imre Deak wrote: > > > > Simplify things by retraining a DP link if a bad link is detected in the > > > > connector detect handler from the encoder's check link state work, > > > > similarly to how this is done after a modeset link training failure. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++----- > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index ff4ed6bb520d8..70b00e5ae7ad7 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -5863,11 +5863,8 @@ intel_dp_detect(struct drm_connector *connector, > > > > * Some external monitors do not signal loss of link synchronization > > > > * with an IRQ_HPD, so force a link status check. > > > > */ > > > > - if (!intel_dp_is_edp(intel_dp)) { > > > > - ret = intel_dp_retrain_link(encoder, ctx); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > + if (!intel_dp_is_edp(intel_dp)) > > > > + intel_dp_check_link_state(intel_dp); > > > > > > I would like to see this hack nuked entirely. But that > > > could be a followup. > > > > Okay. This tries to keep the current behavior, but can add a note that > > the above workaround can be removed after the link state is checked > > after modesets. > > > > I also wondered about the link state check in the hotplug handler. If > > that's only a way to defer doing this from the HPD IRQ handler - which > > is now changed by patch 13 - that could be also removed eventually? > > Not sure which one you want to removed exactly. I presume there > are still at least these cases we need to handle: > - long HDP dropped and came back without any userspace > initiated modeset in between > -> kick off a retrain from the long HPD handler I meant this one, but didn't think of the case where the link can be actually retrained after a long HPD. I guess with a full modeset it works, so should continue doing that. > - short HPD indicated some link badness > -> kick off a retrain from the short HDP handler > - link dropped on its own soon after modeset without > any HPD for some reason > -> kick off a retrain from the post modeset link check > > And the one we should remove: > - something weird happened to the link and no one noticed, > and for some random reason userspace just happens to do > a getconnector() which ends up randomly fixing things Yes, this is clear. > Did I miss anything else? > > -- > Ville Syrjälä > Intel
A bad link in MST is not retrained. Please also consider MST.
The issue ticket is https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10902.
if (intel_dp->is_mst) {
/*
* If we are in MST mode then this connector
* won't appear connected or have anything
* with EDID on it
*/
status = connector_status_disconnected;
goto out;
}
/*
* Some external monitors do not signal loss of link synchronization
* with an IRQ_HPD, so force a link status check.
*/
if (!intel_dp_is_edp(intel_dp)) {
ret = intel_dp_retrain_link(encoder, ctx);
if (ret)
return ret;
}
On Mon, May 27, 2024 at 01:14:32PM +0800, gareth.yu@intel.com wrote: Hi, > A bad link in MST is not retrained. Please also consider MST. > The issue ticket is https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10902. > > if (intel_dp->is_mst) { > /* > * If we are in MST mode then this connector > * won't appear connected or have anything > * with EDID on it > */ > status = connector_status_disconnected; > goto out; > } > > /* > * Some external monitors do not signal loss of link synchronization > * with an IRQ_HPD, so force a link status check. > */ > if (!intel_dp_is_edp(intel_dp)) { > ret = intel_dp_retrain_link(encoder, ctx); > if (ret) > return ret; > } this is not the proper place to retrain the link, the plan is to remove the above. Could you give a try to the patchset and follow up with a dmesg log on the ticket? Thanks, Imre
Hi Max,
Please provide the test results.
Hi Imre,
Meanwhile, my question here is the link status is not checked in MST mode according to the current flow. The changes below
for MST are same as https://patchwork.freedesktop.org/patch/591953/?series=132685&rev=6. Please check it.
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0923a5adc14b..bf008a70304f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5927,6 +5927,13 @@ intel_dp_detect(struct drm_connector *connector,
intel_dp_print_rates(intel_dp);
+ /*
+ * Some external monitors do not signal loss of link synchronization
+ * with an IRQ_HPD, so force a link status check.
+ */
+ if (!intel_dp_is_edp(intel_dp))
+ intel_dp_check_link_state(intel_dp);
+
if (intel_dp->is_mst) {
/*
* If we are in MST mode then this connector
@@ -5937,13 +5944,6 @@ intel_dp_detect(struct drm_connector *connector,
goto out;
}
- /*
- * Some external monitors do not signal loss of link synchronization
- * with an IRQ_HPD, so force a link status check.
- */
- if (!intel_dp_is_edp(intel_dp))
- intel_dp_check_link_state(intel_dp);
-
/*
* Clearing NACK and defer counts to get their exact values
* while reading EDID which are required by Compliance tests
Thanks,
Gareth
> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Monday, May 27, 2024 7:30 PM
> To: Yu, Gareth <gareth.yu@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Ville Syrjälä <ville.syrjala@linux.intel.com>
> Subject: Re: [PATCH v2 12/21] drm/i915/dp: Use check link state work in the
> detect handler
>
> On Mon, May 27, 2024 at 01:14:32PM +0800, gareth.yu@intel.com wrote:
> Hi,
>
> > A bad link in MST is not retrained. Please also consider MST.
> > The issue ticket is https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/10902.
> >
> > if (intel_dp->is_mst) {
> > /*
> > * If we are in MST mode then this connector
> > * won't appear connected or have anything
> > * with EDID on it
> > */
> > status = connector_status_disconnected;
> > goto out;
> > }
> >
> > /*
> > * Some external monitors do not signal loss of link synchronization
> > * with an IRQ_HPD, so force a link status check.
> > */
> > if (!intel_dp_is_edp(intel_dp)) {
> > ret = intel_dp_retrain_link(encoder, ctx);
> > if (ret)
> > return ret;
> > }
>
> this is not the proper place to retrain the link, the plan is to remove the above.
> Could you give a try to the patchset and follow up with a dmesg log on the
> ticket?
>
> Thanks,
> Imre
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index ff4ed6bb520d8..70b00e5ae7ad7 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5863,11 +5863,8 @@ intel_dp_detect(struct drm_connector *connector, * Some external monitors do not signal loss of link synchronization * with an IRQ_HPD, so force a link status check. */ - if (!intel_dp_is_edp(intel_dp)) { - ret = intel_dp_retrain_link(encoder, ctx); - if (ret) - return ret; - } + if (!intel_dp_is_edp(intel_dp)) + intel_dp_check_link_state(intel_dp); /* * Clearing NACK and defer counts to get their exact values
Simplify things by retraining a DP link if a bad link is detected in the connector detect handler from the encoder's check link state work, similarly to how this is done after a modeset link training failure. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)