| Message ID | 20180907062135.30054-1-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, 06 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Comment claims link needs to be retrained because the connected sink raised > a long pulse to indicate link loss. If the sink did so, > intel_dp_hotplug() would have handled link retraining. Looking at the > logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: Re-apply > Perform link quality check, unconditionally during long pulse"")', the > issue is that the sink does not trigger an interrupt. We want the > ->detect() from user space to check link status and retrain. Ville's > review for the original patch also indicates the same root cause. > > I have also renamed long_pulse() to full_detect(). Since intel_dp_detect() doesn't actually do *anything* except skip detect sometimes, I think "full detect" is a misnomer. Arguably you could merge intel_dp_long_pulse() into its only caller, intel_dp_detect(), and do an early return on skipping detect. You have to look at the functions, and wonder if the current split is the right one. Please let others chime in too before making changes. BR, Jani. > > Cc: Lyude Paul <lyude@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > 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 | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 436c22de33b6..befdaa076d8c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > } > > static int > -intel_dp_long_pulse(struct intel_connector *connector, > +intel_dp_full_detect(struct intel_connector *connector, > struct drm_modeset_acquire_ctx *ctx) > { > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector *connector, > goto out; > } else { > /* > - * If display is now connected check links status, > - * there has been known issues of link loss triggering > - * long pulse. > - * > - * Some sinks (eg. ASUS PB287Q) seem to perform some > - * weird HPD ping pong during modesets. So we can apparently > - * end up with HPD going low during a modeset, and then > - * going back up soon after. And once that happens we must > - * retrain the link to get a picture. That's in case no > - * userspace component reacted to intermittent HPD dip. > + * Check link status in case the link went out of > + * synchronization and the sink did not tell us. > */ > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector *connector, > return ret; > } > > - status = intel_dp_long_pulse(intel_dp->attached_connector, ctx); > + status = intel_dp_full_detect(intel_dp->attached_connector, > + ctx); > } > > intel_dp->detect_done = false;
On Fri, Sep 07, 2018 at 03:39:24PM +0300, Jani Nikula wrote: > On Thu, 06 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > Comment claims link needs to be retrained because the connected sink raised > > a long pulse to indicate link loss. If the sink did so, > > intel_dp_hotplug() would have handled link retraining. Looking at the > > logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: Re-apply > > Perform link quality check, unconditionally during long pulse"")', the > > issue is that the sink does not trigger an interrupt. We want the > > ->detect() from user space to check link status and retrain. Ville's > > review for the original patch also indicates the same root cause. > > > > I have also renamed long_pulse() to full_detect(). > > Since intel_dp_detect() doesn't actually do *anything* except skip > detect sometimes, I think "full detect" is a misnomer. > > Arguably you could merge intel_dp_long_pulse() into its only caller, > intel_dp_detect(), and do an early return on skipping detect. You have > to look at the functions, and wonder if the current split is the right > one. That's pretty much what I suggested earlier. Although someone should double check to make sure the detect_done flag even makes sense these days. Maybe it does, maybe it doesn't, I don't recall. > > Please let others chime in too before making changes. > > BR, > Jani. > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > 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 | 17 +++++------------ > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 436c22de33b6..befdaa076d8c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > } > > > > static int > > -intel_dp_long_pulse(struct intel_connector *connector, > > +intel_dp_full_detect(struct intel_connector *connector, > > struct drm_modeset_acquire_ctx *ctx) > > { > > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector *connector, > > goto out; > > } else { > > /* > > - * If display is now connected check links status, > > - * there has been known issues of link loss triggering > > - * long pulse. > > - * > > - * Some sinks (eg. ASUS PB287Q) seem to perform some > > - * weird HPD ping pong during modesets. So we can apparently > > - * end up with HPD going low during a modeset, and then > > - * going back up soon after. And once that happens we must > > - * retrain the link to get a picture. That's in case no > > - * userspace component reacted to intermittent HPD dip. > > + * Check link status in case the link went out of > > + * synchronization and the sink did not tell us. > > */ > > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > > > @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector *connector, > > return ret; > > } > > > > - status = intel_dp_long_pulse(intel_dp->attached_connector, ctx); > > + status = intel_dp_full_detect(intel_dp->attached_connector, > > + ctx); > > } > > > > intel_dp->detect_done = false; > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, Sep 07, 2018 at 05:32:36PM +0300, Ville Syrjälä wrote: > On Fri, Sep 07, 2018 at 03:39:24PM +0300, Jani Nikula wrote: > > On Thu, 06 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > > Comment claims link needs to be retrained because the connected sink raised > > > a long pulse to indicate link loss. If the sink did so, > > > intel_dp_hotplug() would have handled link retraining. Looking at the > > > logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: Re-apply > > > Perform link quality check, unconditionally during long pulse"")', the > > > issue is that the sink does not trigger an interrupt. We want the > > > ->detect() from user space to check link status and retrain. Ville's > > > review for the original patch also indicates the same root cause. > > > > > > I have also renamed long_pulse() to full_detect(). > > > > Since intel_dp_detect() doesn't actually do *anything* except skip > > detect sometimes, I think "full detect" is a misnomer. > > > > Arguably you could merge intel_dp_long_pulse() into its only caller, > > intel_dp_detect(), and do an early return on skipping detect. You have > > to look at the functions, and wonder if the current split is the right > > one. > > That's pretty much what I suggested earlier. Although someone should > double check to make sure the detect_done flag even makes sense these > days. Maybe it does, maybe it doesn't, I don't recall. > Yes I remember digging into the detect_done flag earlier. So the history of this flag is that it was added when intel_dp_detect() was being called from multiple places like the hotplug and from userspace through drm_mode_get_connector(). However now that its called only from one place, I believe we do not need the detect_done flag. And in this case we can just have intel_dp_long_pulse get merged with intel_dp_detect() and just have all that under the _detect umbrella. Manasi > > > > Please let others chime in too before making changes. > > > > BR, > > Jani. > > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > > 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 | 17 +++++------------ > > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 436c22de33b6..befdaa076d8c 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > > } > > > > > > static int > > > -intel_dp_long_pulse(struct intel_connector *connector, > > > +intel_dp_full_detect(struct intel_connector *connector, > > > struct drm_modeset_acquire_ctx *ctx) > > > { > > > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > > @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector *connector, > > > goto out; > > > } else { > > > /* > > > - * If display is now connected check links status, > > > - * there has been known issues of link loss triggering > > > - * long pulse. > > > - * > > > - * Some sinks (eg. ASUS PB287Q) seem to perform some > > > - * weird HPD ping pong during modesets. So we can apparently > > > - * end up with HPD going low during a modeset, and then > > > - * going back up soon after. And once that happens we must > > > - * retrain the link to get a picture. That's in case no > > > - * userspace component reacted to intermittent HPD dip. > > > + * Check link status in case the link went out of > > > + * synchronization and the sink did not tell us. > > > */ > > > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > > > > > @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector *connector, > > > return ret; > > > } > > > > > > - status = intel_dp_long_pulse(intel_dp->attached_connector, ctx); > > > + status = intel_dp_full_detect(intel_dp->attached_connector, > > > + ctx); > > > } > > > > > > intel_dp->detect_done = false; > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > > -- > 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 17:32 +0300, Ville Syrjälä wrote: > On Fri, Sep 07, 2018 at 03:39:24PM +0300, Jani Nikula wrote: > > On Thu, 06 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel > > .com> wrote: > > > Comment claims link needs to be retrained because the connected > > > sink raised > > > a long pulse to indicate link loss. If the sink did so, > > > intel_dp_hotplug() would have handled link retraining. Looking at > > > the > > > logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: > > > Re-apply > > > Perform link quality check, unconditionally during long > > > pulse"")', the > > > issue is that the sink does not trigger an interrupt. We want the > > > ->detect() from user space to check link status and retrain. > > > Ville's > > > review for the original patch also indicates the same root cause. > > > > > > I have also renamed long_pulse() to full_detect(). > > > > Since intel_dp_detect() doesn't actually do *anything* except skip > > detect sometimes, I think "full detect" is a misnomer. > > > > Arguably you could merge intel_dp_long_pulse() into its only > > caller, > > intel_dp_detect(), and do an early return on skipping detect. You > > have > > to look at the functions, and wonder if the current split is the > > right > > one. > > That's pretty much what I suggested earlier. I had mixed feelings when I wrote it, but merging the functions does sound like a better idea now. > Although someone should > double check to make sure the detect_done flag even makes sense these > days. Maybe it does, maybe it doesn't, I don't recall. It doesn't afaict. The only place where detect_done gets set to true is followed by resetting of the flag to false. -DK > > > > > Please let others chime in too before making changes. > > > > BR, > > Jani. > > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de> > > > 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 | 17 +++++------------ > > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 436c22de33b6..befdaa076d8c 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp > > > *intel_dp) > > > } > > > > > > static int > > > -intel_dp_long_pulse(struct intel_connector *connector, > > > +intel_dp_full_detect(struct intel_connector *connector, > > > struct drm_modeset_acquire_ctx *ctx) > > > { > > > struct drm_i915_private *dev_priv = to_i915(connector- > > > >base.dev); > > > @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector > > > *connector, > > > goto out; > > > } else { > > > /* > > > - * If display is now connected check links > > > status, > > > - * there has been known issues of link loss > > > triggering > > > - * long pulse. > > > - * > > > - * Some sinks (eg. ASUS PB287Q) seem to perform > > > some > > > - * weird HPD ping pong during modesets. So we > > > can apparently > > > - * end up with HPD going low during a modeset, > > > and then > > > - * going back up soon after. And once that > > > happens we must > > > - * retrain the link to get a picture. That's in > > > case no > > > - * userspace component reacted to intermittent > > > HPD dip. > > > + * Check link status in case the link went out > > > of > > > + * synchronization and the sink did not tell us. > > > */ > > > struct intel_encoder *encoder = > > > &dp_to_dig_port(intel_dp)->base; > > > > > > @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector > > > *connector, > > > return ret; > > > } > > > > > > - status = intel_dp_long_pulse(intel_dp- > > > >attached_connector, ctx); > > > + status = intel_dp_full_detect(intel_dp- > > > >attached_connector, > > > + ctx); > > > } > > > > > > intel_dp->detect_done = false; > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 436c22de33b6..befdaa076d8c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) } static int -intel_dp_long_pulse(struct intel_connector *connector, +intel_dp_full_detect(struct intel_connector *connector, struct drm_modeset_acquire_ctx *ctx) { struct drm_i915_private *dev_priv = to_i915(connector->base.dev); @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector *connector, goto out; } else { /* - * If display is now connected check links status, - * there has been known issues of link loss triggering - * long pulse. - * - * Some sinks (eg. ASUS PB287Q) seem to perform some - * weird HPD ping pong during modesets. So we can apparently - * end up with HPD going low during a modeset, and then - * going back up soon after. And once that happens we must - * retrain the link to get a picture. That's in case no - * userspace component reacted to intermittent HPD dip. + * Check link status in case the link went out of + * synchronization and the sink did not tell us. */ struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector *connector, return ret; } - status = intel_dp_long_pulse(intel_dp->attached_connector, ctx); + status = intel_dp_full_detect(intel_dp->attached_connector, + ctx); } intel_dp->detect_done = false;
Comment claims link needs to be retrained because the connected sink raised a long pulse to indicate link loss. If the sink did so, intel_dp_hotplug() would have handled link retraining. Looking at the logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: Re-apply Perform link quality check, unconditionally during long pulse"")', the issue is that the sink does not trigger an interrupt. We want the ->detect() from user space to check link status and retrain. Ville's review for the original patch also indicates the same root cause. I have also renamed long_pulse() to full_detect(). Cc: Lyude Paul <lyude@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Jan-Marek Glogowski <glogow@fbihome.de> 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 | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)