| Message ID | 20180907062135.30054-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse() | expand |
On Thu, Sep 06, 2018 at 11:21:34PM -0700, Dhinakaran Pandiyan wrote: > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, > unconditionally during long pulse"")' applies a work around for monitors > that don't signal link loss. Apply this only for external displays as > eDP features like PSR when active will have the link turned off and the > driver ends up retraining the link seeeing that link is not synchronized. > > Cc: Lyude Paul <lyude@redhat.com> > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"") > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index befdaa076d8c..c59b2f78f41a 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct intel_connector *connector, > */ > status = connector_status_disconnected; > goto out; > - } else { > + } > + > + if (!intel_dp_is_edp(intel_dp)) { Hmm. How do things work if we get a short hpd from the sink while it's in PSR link off mode? Won't we try to retrain in that case as well? > /* > * Check link status in case the link went out of > * synchronization and the sink did not tell us. > -- > 2.17.1
On Fri, Sep 07, 2018 at 05:34:23PM +0300, Ville Syrjälä wrote: > On Thu, Sep 06, 2018 at 11:21:34PM -0700, Dhinakaran Pandiyan wrote: > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, > > unconditionally during long pulse"")' applies a work around for monitors > > that don't signal link loss. Apply this only for external displays as > > eDP features like PSR when active will have the link turned off and the > > driver ends up retraining the link seeeing that link is not synchronized. > > > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"") > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index befdaa076d8c..c59b2f78f41a 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct intel_connector *connector, > > */ > > status = connector_status_disconnected; > > goto out; > > - } else { > > + } > > + > > + if (!intel_dp_is_edp(intel_dp)) { Also what happens when the PSR is not active? We would still need to retrain even on eDP. eDP tends to get fixed a lot of times during link retraining. Manasi > > Hmm. How do things work if we get a short hpd from the sink while it's > in PSR link off mode? Won't we try to retrain in that case as well? > > > /* > > * Check link status in case the link went out of > > * synchronization and the sink did not tell us. > > -- > > 2.17.1 > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-09-07 at 09:25 -0700, Manasi Navare wrote: > On Fri, Sep 07, 2018 at 05:34:23PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 06, 2018 at 11:21:34PM -0700, Dhinakaran Pandiyan > > wrote: > > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality > > > check, > > > unconditionally during long pulse"")' applies a work around for > > > monitors > > > that don't signal link loss. Apply this only for external > > > displays as > > > eDP features like PSR when active will have the link turned off > > > and the > > > driver ends up retraining the link seeeing that link is not > > > synchronized. > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link > > > quality check, unconditionally during long pulse"") > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index befdaa076d8c..c59b2f78f41a 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct intel_connector > > > *connector, > > > */ > > > status = connector_status_disconnected; > > > goto out; > > > - } else { > > > + } > > > + > > > + if (!intel_dp_is_edp(intel_dp)) { > > Also what happens when the PSR is not active? We would still need to > retrain > even on eDP. > eDP tends to get fixed a lot of times during link retraining. > > Manasi > > > > > Hmm. How do things work if we get a short hpd from the sink while > > it's > > in PSR link off mode? Won't we try to retrain in that case as well? > > Looks like short pulse defers retraining to ->detect(), so with this change we end up not retraining. But, we do want to retrain when we get a short pulse for a bad link and the link is expected to be synchronized. > > > /* > > > * Check link status in case the link went out > > > of > > > * synchronization and the sink did not tell us. > > > -- > > > 2.17.1 > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 07, 2018 at 11:31:15AM -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-09-07 at 09:25 -0700, Manasi Navare wrote: > > On Fri, Sep 07, 2018 at 05:34:23PM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 06, 2018 at 11:21:34PM -0700, Dhinakaran Pandiyan > > > wrote: > > > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality > > > > check, > > > > unconditionally during long pulse"")' applies a work around for > > > > monitors > > > > that don't signal link loss. Apply this only for external > > > > displays as > > > > eDP features like PSR when active will have the link turned off > > > > and the > > > > driver ends up retraining the link seeeing that link is not > > > > synchronized. > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link > > > > quality check, unconditionally during long pulse"") > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index befdaa076d8c..c59b2f78f41a 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct intel_connector > > > > *connector, > > > > */ > > > > status = connector_status_disconnected; > > > > goto out; > > > > - } else { > > > > + } > > > > + > > > > + if (!intel_dp_is_edp(intel_dp)) { > > > > Also what happens when the PSR is not active? We would still need to > > retrain > > even on eDP. > > eDP tends to get fixed a lot of times during link retraining. > > > > Manasi > > > > > > > > Hmm. How do things work if we get a short hpd from the sink while > > > it's > > > in PSR link off mode? Won't we try to retrain in that case as well? > > > > Looks like short pulse defers retraining to ->detect(), so with this > change we end up not retraining. Except the ->hotplug() hook will still do the retraining I guess? It's a bit of a mess now with this getting handled in two places. > But, we do want to retrain when we get > a short pulse for a bad link and the link is expected to be > synchronized. > > > > > /* > > > > * Check link status in case the link went out > > > > of > > > > * synchronization and the sink did not tell us. > > > > -- > > > > 2.17.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-09-07 at 22:18 +0300, Ville Syrjälä wrote: > On Fri, Sep 07, 2018 at 11:31:15AM -0700, Dhinakaran Pandiyan wrote: > > On Fri, 2018-09-07 at 09:25 -0700, Manasi Navare wrote: > > > On Fri, Sep 07, 2018 at 05:34:23PM +0300, Ville Syrjälä wrote: > > > > On Thu, Sep 06, 2018 at 11:21:34PM -0700, Dhinakaran Pandiyan > > > > wrote: > > > > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link > > > > > quality > > > > > check, > > > > > unconditionally during long pulse"")' applies a work around > > > > > for > > > > > monitors > > > > > that don't signal link loss. Apply this only for external > > > > > displays as > > > > > eDP features like PSR when active will have the link turned > > > > > off > > > > > and the > > > > > driver ends up retraining the link seeeing that link is not > > > > > synchronized. > > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link > > > > > quality check, unconditionally during long pulse"") > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel > > > > > .com > > > > > > > > > > > > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index befdaa076d8c..c59b2f78f41a 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct > > > > > intel_connector > > > > > *connector, > > > > > */ > > > > > status = connector_status_disconnected; > > > > > goto out; > > > > > - } else { > > > > > + } > > > > > + > > > > > + if (!intel_dp_is_edp(intel_dp)) { > > > > > > Also what happens when the PSR is not active? We would still need > > > to > > > retrain > > > even on eDP. > > > eDP tends to get fixed a lot of times during link retraining. > > > > > > Manasi > > > > > > > > > > > Hmm. How do things work if we get a short hpd from the sink > > > > while > > > > it's > > > > in PSR link off mode? Won't we try to retrain in that case as > > > > well? > > > > > > > > Looks like short pulse defers retraining to ->detect(), so with > > this > > change we end up not retraining. > > Except the ->hotplug() hook will still do the retraining I guess? > It's a > bit of a mess now with this getting handled in two places. > Yeah, we'll have to fix shortpulse handling in a separate patch. Link standby v/s link off also determines how we should handle short pulse from the sink. A retrain is required if PSR put link in standby and then it went out of sync. > > But, we do want to retrain when we get > > a short pulse for a bad link and the link is expected to be > > synchronized. > > > > > > > /* > > > > > * Check link status in case the link went > > > > > out > > > > > of > > > > > * synchronization and the sink did not tell > > > > > us. > > > > > -- > > > > > 2.17.1 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index befdaa076d8c..c59b2f78f41a 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5072,7 +5072,9 @@ intel_dp_full_detect(struct intel_connector *connector, */ status = connector_status_disconnected; goto out; - } else { + } + + if (!intel_dp_is_edp(intel_dp)) { /* * Check link status in case the link went out of * synchronization and the sink did not tell us.
commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")' applies a work around for monitors that don't signal link loss. Apply this only for external displays as eDP features like PSR when active will have the link turned off and the driver ends up retraining the link seeeing that link is not synchronized. Cc: Lyude Paul <lyude@redhat.com> Cc: Jan-Marek Glogowski <glogow@fbihome.de> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"") Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)