[PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Dhinakaran Pandiyan]

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(-)

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;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Dhinakaran Pandiyan]

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)) {
 		/*
 		 * Check link status in case the link went out of
 		 * synchronization and the sink did not tell us.
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Manasi Navare]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Dhinakaran Pandiyan]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/dp: Restrict link retrain workaround to external monitors

[Dhinakaran Pandiyan]

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
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/psr: Enable PSR1 on gen-9+ HW

[Dhinakaran Pandiyan]

We have new tests and fixes in place since the feature was last
disabled.

Try again for gen-9+ hardware and enable only PSR1 as a first step.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Jose Roberto de Souza <jose.souza@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
References: 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW")
References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."")
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b6838b525502..fc823f93a4dc 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug)
 static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 			       const struct intel_crtc_state *crtc_state)
 {
+	/* Disable PSR2 by default for all platforms */
+	if (i915_modparams.enable_psr == -1)
+		return false;
+
 	switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
 	case I915_PSR_DEBUG_FORCE_PSR1:
 		return false;
@@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
  * intel_psr_init - Init basic PSR work and mutex.
  * @dev_priv: i915 device private
  *
- * This function is  called only once at driver load to initialize basic
+ * This function is called only once at driver load to initialize basic
  * PSR stuff.
  */
 void intel_psr_init(struct drm_i915_private *dev_priv)
@@ -1065,19 +1069,14 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 	if (!dev_priv->psr.sink_support)
 		return;
 
-	if (i915_modparams.enable_psr == -1) {
-		i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
-
-		/* Per platform default: all disabled. */
-		i915_modparams.enable_psr = 0;
-	}
+	if (i915_modparams.enable_psr == -1)
+		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
+			i915_modparams.enable_psr = 0;
 
-	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		/* HSW and BDW require workarounds that we don't implement. */
 		dev_priv->psr.link_standby = false;
 	else
-		/* For new platforms let's respect VBT back again */
 		dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
 
 	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Enable PSR1 on gen-9+ HW

[Nathan Ciobanu]

On Thu, Sep 06, 2018 at 11:21:35PM -0700, Dhinakaran Pandiyan wrote:
> We have new tests and fixes in place since the feature was last
> disabled.
> 
> Try again for gen-9+ hardware and enable only PSR1 as a first step.
Since this patch explicitly disables PSR2 for all platforms maybe you 
can clarify that in the commit message or disable PSR2 in a separate
patch. It will help with productized kernels ;)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Jose Roberto de Souza <jose.souza@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> References: 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW")
> References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."")
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b6838b525502..fc823f93a4dc 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug)
>  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  			       const struct intel_crtc_state *crtc_state)
>  {
> +	/* Disable PSR2 by default for all platforms */
> +	if (i915_modparams.enable_psr == -1)
> +		return false;
> +
>  	switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
> @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @dev_priv: i915 device private
>   *
> - * This function is  called only once at driver load to initialize basic
> + * This function is called only once at driver load to initialize basic
>   * PSR stuff.
>   */
>  void intel_psr_init(struct drm_i915_private *dev_priv)
> @@ -1065,19 +1069,14 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	if (!dev_priv->psr.sink_support)
>  		return;
>  
> -	if (i915_modparams.enable_psr == -1) {
> -		i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
> -
> -		/* Per platform default: all disabled. */
> -		i915_modparams.enable_psr = 0;
> -	}
> +	if (i915_modparams.enable_psr == -1)
> +		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
> +			i915_modparams.enable_psr = 0;
>  
> -	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		/* HSW and BDW require workarounds that we don't implement. */
>  		dev_priv->psr.link_standby = false;
>  	else
> -		/* For new platforms let's respect VBT back again */
>  		dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
>  
>  	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> -- 
> 2.17.1
> 
> _______________________________________________
> 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: [PATCH 3/3] drm/i915/psr: Enable PSR1 on gen-9+ HW

[Dhinakaran Pandiyan]

On Fri, 2018-09-07 at 08:35 -0700, Nathan Ciobanu wrote:
> On Thu, Sep 06, 2018 at 11:21:35PM -0700, Dhinakaran Pandiyan wrote:
> > We have new tests and fixes in place since the feature was last
> > disabled.
> > 
> > Try again for gen-9+ hardware and enable only PSR1 as a first step.
> 
> Since this patch explicitly disables PSR2 for all platforms maybe
> you 
> can clarify that in the commit message or disable PSR2 in a separate
> patch. It will help with productized kernels ;)

It doesn't actually disable, since PSR2 was not enabled in the first
place. i915.enable_psr=1 will still enable PSR2 on a compatible panel.
i915.enable_psr=-1, the default option, will now enable PSR1.

Unless you are saying the patch is not doing what I'm claiming :)


> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Jose Roberto de Souza <jose.souza@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > References: 2ee7dc497e34 ("drm/i915: disable PSR by default on
> > HSW/BDW")
> > References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default
> > on Valleyview and Cherryview."")
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index b6838b525502..fc823f93a4dc 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug)
> >  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
> >  			       const struct intel_crtc_state
> > *crtc_state)
> >  {
> > +	/* Disable PSR2 by default for all platforms */
> > +	if (i915_modparams.enable_psr == -1)
> > +		return false;
> > +
> >  	switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> > @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >   * intel_psr_init - Init basic PSR work and mutex.
> >   * @dev_priv: i915 device private
> >   *
> > - * This function is  called only once at driver load to initialize
> > basic
> > + * This function is called only once at driver load to initialize
> > basic
> >   * PSR stuff.
> >   */
> >  void intel_psr_init(struct drm_i915_private *dev_priv)
> > @@ -1065,19 +1069,14 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  	if (!dev_priv->psr.sink_support)
> >  		return;
> >  
> > -	if (i915_modparams.enable_psr == -1) {
> > -		i915_modparams.enable_psr = dev_priv-
> > >vbt.psr.enable;
> > -
> > -		/* Per platform default: all disabled. */
> > -		i915_modparams.enable_psr = 0;
> > -	}
> > +	if (i915_modparams.enable_psr == -1)
> > +		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > >vbt.psr.enable)
> > +			i915_modparams.enable_psr = 0;
> >  
> > -	/* Set link_standby x link_off defaults */
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >  		/* HSW and BDW require workarounds that we don't
> > implement. */
> >  		dev_priv->psr.link_standby = false;
> >  	else
> > -		/* For new platforms let's respect VBT back again
> > */
> >  		dev_priv->psr.link_standby = dev_priv-
> > >vbt.psr.full_link;
> >  
> >  	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
URL   : https://patchwork.freedesktop.org/series/49321/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f2bc4d26da72 drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
-:26: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#26: 
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")

-:26: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")'
#26: 
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")

-:39: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#39: FILE: drivers/gpu/drm/i915/intel_dp.c:5016:
+intel_dp_full_detect(struct intel_connector *connector,
 		    struct drm_modeset_acquire_ctx *ctx)

total: 1 errors, 1 warnings, 1 checks, 35 lines checked
28d1e76ff662 drm/i915/dp: Restrict link retrain workaround to external monitors
-:20: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#20: 
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")

-:20: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")'
#20: 
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")

total: 1 errors, 1 warnings, 0 checks, 10 lines checked
32bfc3544e88 drm/i915/psr: Enable PSR1 on gen-9+ HW
-:19: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW")'
#19: 
References: 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW")

-:20: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#20: 
References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."")

-:20: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."")'
#20: 
References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."")

total: 2 errors, 1 warnings, 0 checks, 40 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
URL   : https://patchwork.freedesktop.org/series/49321/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4785 -> Patchwork_10122 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49321/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10122 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191) +1

    igt@kms_psr@primary_page_flip:
      fi-bdw-samus:       PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#107139, fdo#105128) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (53 -> 48) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-bxt-dsi fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4785 -> Patchwork_10122

  CI_DRM_4785: 7fddb79b0908d5c08efc93656fd10a761c9d14ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4634: 7d89cc39dde3b4881d85ace45d504cc098fa3684 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10122: 32bfc3544e88a513bde665139a0ee043a35d59f9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

32bfc3544e88 drm/i915/psr: Enable PSR1 on gen-9+ HW
28d1e76ff662 drm/i915/dp: Restrict link retrain workaround to external monitors
f2bc4d26da72 drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10122/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()
URL   : https://patchwork.freedesktop.org/series/49321/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4785_full -> Patchwork_10122_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10122_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume-context:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-shrfb-pgflip-blt:
      shard-glk:          PASS -> FAIL (fdo#103167)

    
    ==== Possible fixes ====

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4785 -> Patchwork_10122

  CI_DRM_4785: 7fddb79b0908d5c08efc93656fd10a761c9d14ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4634: 7d89cc39dde3b4881d85ace45d504cc098fa3684 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10122: 32bfc3544e88a513bde665139a0ee043a35d59f9 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10122/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Jani Nikula]

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;

-- 
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: [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Manasi Navare]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Dhinakaran Pandiyan]

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
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx