Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
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 > .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
> > > Cc: Ville Syrjälä
> > > Cc: Jani Nikula
> > > Cc: Rodrigo Vivi
> > > Cc: Jan-Marek Glogowski
> > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link
> > > quality check, unconditionally during long pulse"")
> > > Signed-off-by: Dhinakaran Pandiyan > > >
> > > ---
> > > 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 =
> > > _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
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
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
> > 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
> > > Cc: Ville Syrjälä
> > > Cc: Jani Nikula
> > > Cc: Rodrigo Vivi
> > > Cc: Jan-Marek Glogowski
> > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality
> > > check, unconditionally during long pulse"")
> > > Signed-off-by: Dhinakaran Pandiyan
> > > ---
> > > 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 = _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
On Fri, Sep 07, 2018 at 03:39:24PM +0300, Jani Nikula wrote:
> On Thu, 06 Sep 2018, Dhinakaran Pandiyan
> 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
> > Cc: Ville Syrjälä
> > Cc: Jani Nikula
> > Cc: Rodrigo Vivi
> > Cc: Jan-Marek Glogowski
> > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> > unconditionally during long pulse"")
> > Signed-off-by: Dhinakaran Pandiyan
> > ---
> > 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 = _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
Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
On Thu, 06 Sep 2018, Dhinakaran Pandiyan 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
> Cc: Ville Syrjälä
> Cc: Jani Nikula
> Cc: Rodrigo Vivi
> Cc: Jan-Marek Glogowski
> References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> unconditionally during long pulse"")
> Signed-off-by: Dhinakaran Pandiyan
> ---
> 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 = _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
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
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Rodrigo Vivi
Cc: Jan-Marek Glogowski
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
unconditionally during long pulse"")
Signed-off-by: Dhinakaran Pandiyan
---
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 = _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;
--
2.17.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
