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

[PATCH 1/5] 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().
v2: Patch split and rewrote comment..

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 | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 436c22de33b6..cac1c7c6cbfd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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.
+		 * Some monitors do not signal loss of link synchronization
+		 * with an IRQ_HPD, so force a link status check.
 		 */
 		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 
-- 
2.14.1

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

[PATCH 2/5] 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. Limit 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 cac1c7c6cbfd..8bf9afa5683c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else {
+	}
+
+	if (!intel_dp_is_edp(intel_dp)) {
 		/*
 		 * Some monitors do not signal loss of link synchronization
 		 * with an IRQ_HPD, so force a link status check.
-- 
2.14.1

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

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

[Lyude Paul]

Did the patch this references get pushed? I saw it fly by and I thought we had
decided not to pull it in

On Tue, 2018-09-18 at 00:20 -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. Limit 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 cac1c7c6cbfd..8bf9afa5683c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else {
> +	}
> +
> +	if (!intel_dp_is_edp(intel_dp)) {
>  		/*
>  		 * Some monitors do not signal loss of link synchronization
>  		 * with an IRQ_HPD, so force a link status check.

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

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

[Rodrigo Vivi]

On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> Did the patch this references get pushed? I saw it fly by and I thought we had
> decided not to pull it in

ops, I created the confusion. Sorry.

What I decided that week was to not pull to drm-intel-fixes until Ville
or Jani acking. But it was already part of dinq, so moving to drm-next
and I didn't removed or reverted it.

So... But ville acked on irc and I pulled to -fixes on the following week
as well.

So yes, patch is there ;)

> 
> On Tue, 2018-09-18 at 00:20 -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. Limit 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 cac1c7c6cbfd..8bf9afa5683c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > -	} else {
> > +	}
> > +
> > +	if (!intel_dp_is_edp(intel_dp)) {
> >  		/*
> >  		 * Some monitors do not signal loss of link synchronization
> >  		 * with an IRQ_HPD, so force a link status check.
> 
> _______________________________________________
> 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/5] drm/i915/dp: Restrict link retrain workaround to external monitors

[Pandiyan, Dhinakaran]

On Tue, 2018-09-18 at 08:50 -0700, Rodrigo Vivi wrote:
> On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> > Did the patch this references get pushed? I saw it fly by and I
> > thought we had
> > decided not to pull it in
> 
> ops, I created the confusion. Sorry.
> 
> What I decided that week was to not pull to drm-intel-fixes until
> Ville
> or Jani acking. But it was already part of dinq, so moving to drm-
> next
> and I didn't removed or reverted it.

I would have preferred to limit this work around to the specific
monitor it was needed for. The next best thing is to at least limit it
to external monitors? The code adds a bunch of extra dpcd reads to
check link status and acquires locks whenever user spaces wants a
->detect() and most sinks do not need this.

Let me know what your thoughts are.

-DK


> 
> So... But ville acked on irc and I pulled to -fixes on the following
> week
> as well.
> 
> So yes, patch is there ;)
> 
> > 
> > On Tue, 2018-09-18 at 00:20 -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. Limit 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 cac1c7c6cbfd..8bf9afa5683c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector
> > > *connector,
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > -	} else {
> > > +	}
> > > +
> > > +	if (!intel_dp_is_edp(intel_dp)) {
> > >  		/*
> > >  		 * Some monitors do not signal loss of link
> > > synchronization
> > >  		 * with an IRQ_HPD, so force a link status
> > > check.
> > 
> > _______________________________________________
> > 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/5] drm/i915/dp: Restrict link retrain workaround to external monitors

[Lyude Paul]

SGTM

On Wed, 2018-09-19 at 17:58 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-09-18 at 08:50 -0700, Rodrigo Vivi wrote:
> > On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> > > Did the patch this references get pushed? I saw it fly by and I
> > > thought we had
> > > decided not to pull it in
> > 
> > ops, I created the confusion. Sorry.
> > 
> > What I decided that week was to not pull to drm-intel-fixes until
> > Ville
> > or Jani acking. But it was already part of dinq, so moving to drm-
> > next
> > and I didn't removed or reverted it.
> 
> I would have preferred to limit this work around to the specific
> monitor it was needed for. The next best thing is to at least limit it
> to external monitors? The code adds a bunch of extra dpcd reads to
> check link status and acquires locks whenever user spaces wants a
> ->detect() and most sinks do not need this.
> 
> Let me know what your thoughts are.
> 
> -DK
> 
> 
> > 
> > So... But ville acked on irc and I pulled to -fixes on the following
> > week
> > as well.
> > 
> > So yes, patch is there ;)
> > 
> > > 
> > > On Tue, 2018-09-18 at 00:20 -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. Limit 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 cac1c7c6cbfd..8bf9afa5683c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector
> > > > *connector,
> > > >  		 */
> > > >  		status = connector_status_disconnected;
> > > >  		goto out;
> > > > -	} else {
> > > > +	}
> > > > +
> > > > +	if (!intel_dp_is_edp(intel_dp)) {
> > > >  		/*
> > > >  		 * Some monitors do not signal loss of link
> > > > synchronization
> > > >  		 * with an IRQ_HPD, so force a link status
> > > > check.
> > > 
> > > _______________________________________________
> > > 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/5] drm/i915/dp: Remove intel_dp->detect_done flag

[Dhinakaran Pandiyan]

The intel_dp->detect_done flag is no more useful. Pull
intel_dp_long_pulse() into the lone caller,

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 63 +++++++++++++---------------------------
 drivers/gpu/drm/i915/intel_drv.h |  1 -
 2 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8bf9afa5683c..d06bf4303814 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5012,13 +5012,25 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 }
 
 static int
-intel_dp_long_pulse(struct intel_connector *connector,
-		    struct drm_modeset_acquire_ctx *ctx)
+intel_dp_detect(struct drm_connector *connector,
+		struct drm_modeset_acquire_ctx *ctx,
+		bool force)
 {
-	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
-	enum drm_connector_status status;
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	int status;
 	u8 sink_irq_vector = 0;
+	struct drm_crtc *crtc;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	crtc = connector->state->crtc;
+	if (crtc) {
+		status = drm_modeset_lock(&crtc->mutex, ctx);
+		if (status)
+			return status;
+	}
 
 	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
 
@@ -5093,9 +5105,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
 	intel_dp->aux.i2c_defer_count = 0;
 
 	intel_dp_set_edid(intel_dp);
-	if (intel_dp_is_edp(intel_dp) || connector->detect_edid)
+	if (intel_dp_is_edp(intel_dp) ||
+	    to_intel_connector(connector)->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -5120,37 +5132,6 @@ intel_dp_long_pulse(struct intel_connector *connector,
 	return status;
 }
 
-static int
-intel_dp_detect(struct drm_connector *connector,
-		struct drm_modeset_acquire_ctx *ctx,
-		bool force)
-{
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	int status = connector->status;
-
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-
-	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done) {
-		struct drm_crtc *crtc;
-		int ret;
-
-		crtc = connector->state->crtc;
-		if (crtc) {
-			ret = drm_modeset_lock(&crtc->mutex, ctx);
-			if (ret)
-				return ret;
-		}
-
-		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
-	}
-
-	intel_dp->detect_done = false;
-
-	return status;
-}
-
 static void
 intel_dp_force(struct drm_connector *connector)
 {
@@ -5643,7 +5624,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	if (long_hpd) {
 		intel_dp->reset_link_params = true;
-		intel_dp->detect_done = false;
 		return IRQ_NONE;
 	}
 
@@ -5660,7 +5640,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			intel_dp->is_mst = false;
 			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
 							intel_dp->is_mst);
-			intel_dp->detect_done = false;
 			goto put_power;
 		}
 	}
@@ -5673,10 +5652,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* Short pulse can signify loss of hdcp authentication */
 		intel_hdcp_check_link(intel_dp->attached_connector);
 
-		if (!handled) {
-			intel_dp->detect_done = false;
+		if (!handled)
 			goto put_power;
-		}
 	}
 
 	ret = IRQ_HANDLED;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf1c38728a59..c7206a4764e5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1070,7 +1070,6 @@ struct intel_dp {
 	bool link_mst;
 	bool link_trained;
 	bool has_audio;
-	bool detect_done;
 	bool reset_link_params;
 	enum aux_ch aux_ch;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
-- 
2.14.1

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

Re: [PATCH 3/5] drm/i915/dp: Remove intel_dp->detect_done flag

[Ville Syrjälä]

On Tue, Sep 18, 2018 at 12:20:07AM -0700, Dhinakaran Pandiyan wrote:
> The intel_dp->detect_done flag is no more useful. Pull
> intel_dp_long_pulse() into the lone caller,
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 63 +++++++++++++---------------------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  2 files changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8bf9afa5683c..d06bf4303814 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5012,13 +5012,25 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  }
>  
>  static int
> -intel_dp_long_pulse(struct intel_connector *connector,
> -		    struct drm_modeset_acquire_ctx *ctx)
> +intel_dp_detect(struct drm_connector *connector,
> +		struct drm_modeset_acquire_ctx *ctx,
> +		bool force)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> -	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> -	enum drm_connector_status status;
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	int status;
>  	u8 sink_irq_vector = 0;
> +	struct drm_crtc *crtc;
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	crtc = connector->state->crtc;
> +	if (crtc) {
> +		status = drm_modeset_lock(&crtc->mutex, ctx);

The 'status' name looks a bit weird here. I'm leaning towards keeping
the 'int ret' for this case.

Hmm. Actually, why are we even taking this lock here anyway? I would
imagine we only need it for the link retraining and 
intel_dp_retrain_link() already grabs it so we should be able to
just drop this code. Would allow page flips and detect to execute
in parallel again.

> +		if (status)
> +			return status;
> +	}
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> @@ -5093,9 +5105,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
>  	intel_dp->aux.i2c_defer_count = 0;
>  
>  	intel_dp_set_edid(intel_dp);
> -	if (intel_dp_is_edp(intel_dp) || connector->detect_edid)
> +	if (intel_dp_is_edp(intel_dp) ||
> +	    to_intel_connector(connector)->detect_edid)
>  		status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -5120,37 +5132,6 @@ intel_dp_long_pulse(struct intel_connector *connector,
>  	return status;
>  }
>  
> -static int
> -intel_dp_detect(struct drm_connector *connector,
> -		struct drm_modeset_acquire_ctx *ctx,
> -		bool force)
> -{
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	int status = connector->status;
> -
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> -
> -	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done) {
> -		struct drm_crtc *crtc;
> -		int ret;
> -
> -		crtc = connector->state->crtc;
> -		if (crtc) {
> -			ret = drm_modeset_lock(&crtc->mutex, ctx);
> -			if (ret)
> -				return ret;
> -		}
> -
> -		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
> -	}
> -
> -	intel_dp->detect_done = false;
> -
> -	return status;
> -}
> -
>  static void
>  intel_dp_force(struct drm_connector *connector)
>  {
> @@ -5643,7 +5624,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  
>  	if (long_hpd) {
>  		intel_dp->reset_link_params = true;
> -		intel_dp->detect_done = false;
>  		return IRQ_NONE;
>  	}
>  
> @@ -5660,7 +5640,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			intel_dp->is_mst = false;
>  			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
>  							intel_dp->is_mst);
> -			intel_dp->detect_done = false;
>  			goto put_power;
>  		}
>  	}
> @@ -5673,10 +5652,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		/* Short pulse can signify loss of hdcp authentication */
>  		intel_hdcp_check_link(intel_dp->attached_connector);
>  
> -		if (!handled) {
> -			intel_dp->detect_done = false;
> +		if (!handled)
>  			goto put_power;
> -		}
>  	}
>  
>  	ret = IRQ_HANDLED;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf1c38728a59..c7206a4764e5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1070,7 +1070,6 @@ struct intel_dp {
>  	bool link_mst;
>  	bool link_trained;
>  	bool has_audio;
> -	bool detect_done;
>  	bool reset_link_params;
>  	enum aux_ch aux_ch;
>  	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
> -- 
> 2.14.1

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

Re: [PATCH 3/5] drm/i915/dp: Remove intel_dp->detect_done flag

[Pandiyan, Dhinakaran]

On Wed, 2018-09-19 at 20:28 +0300, Ville Syrjälä wrote:
> On Tue, Sep 18, 2018 at 12:20:07AM -0700, Dhinakaran Pandiyan wrote:
> > The intel_dp->detect_done flag is no more useful. Pull
> > intel_dp_long_pulse() into the lone caller,
> > 
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 63 +++++++++++++---------------
> > ------------
> >  drivers/gpu/drm/i915/intel_drv.h |  1 -
> >  2 files changed, 20 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 8bf9afa5683c..d06bf4303814 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5012,13 +5012,25 @@ intel_dp_unset_edid(struct intel_dp
> > *intel_dp)
> >  }
> >  
> >  static int
> > -intel_dp_long_pulse(struct intel_connector *connector,
> > -		    struct drm_modeset_acquire_ctx *ctx)
> > +intel_dp_detect(struct drm_connector *connector,
> > +		struct drm_modeset_acquire_ctx *ctx,
> > +		bool force)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(connector-
> > >base.dev);
> > -	struct intel_dp *intel_dp = intel_attached_dp(&connector-
> > >base);
> > -	enum drm_connector_status status;
> > +	struct drm_i915_private *dev_priv = to_i915(connector-
> > >dev);
> > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > +	int status;
> >  	u8 sink_irq_vector = 0;
> > +	struct drm_crtc *crtc;
> > +
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.id, connector->name);
> > +
> > +	crtc = connector->state->crtc;
> > +	if (crtc) {
> > +		status = drm_modeset_lock(&crtc->mutex, ctx);
> 
> The 'status' name looks a bit weird here. I'm leaning towards keeping
> the 'int ret' for this case.
It does indeed, just wanted to reuse the local variable. Will change it
in the next version.

> 
> Hmm. Actually, why are we even taking this lock here anyway? I would
> imagine we only need it for the link retraining and 
> intel_dp_retrain_link() already grabs it so we should be able to
> just drop this code. Would allow page flips and detect to execute
> in parallel again.
> 
Looks like the code wasn't updated when intel_dp_retrain_link() started
grabbing the locks.

> > +		if (status)
> > +			return status;
> > +	}
> >  
> >  	WARN_ON(!drm_modeset_is_locked(&dev_priv-
> > >drm.mode_config.connection_mutex));
> >  
> > @@ -5093,9 +5105,9 @@ intel_dp_long_pulse(struct intel_connector
> > *connector,
> >  	intel_dp->aux.i2c_defer_count = 0;
> >  
> >  	intel_dp_set_edid(intel_dp);
> > -	if (intel_dp_is_edp(intel_dp) || connector->detect_edid)
> > +	if (intel_dp_is_edp(intel_dp) ||
> > +	    to_intel_connector(connector)->detect_edid)
> >  		status = connector_status_connected;
> > -	intel_dp->detect_done = true;
> >  
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > @@ -5120,37 +5132,6 @@ intel_dp_long_pulse(struct intel_connector
> > *connector,
> >  	return status;
> >  }
> >  
> > -static int
> > -intel_dp_detect(struct drm_connector *connector,
> > -		struct drm_modeset_acquire_ctx *ctx,
> > -		bool force)
> > -{
> > -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -	int status = connector->status;
> > -
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > -		      connector->base.id, connector->name);
> > -
> > -	/* If full detect is not performed yet, do a full detect
> > */
> > -	if (!intel_dp->detect_done) {
> > -		struct drm_crtc *crtc;
> > -		int ret;
> > -
> > -		crtc = connector->state->crtc;
> > -		if (crtc) {
> > -			ret = drm_modeset_lock(&crtc->mutex, ctx);
> > -			if (ret)
> > -				return ret;
> > -		}
> > -
> > -		status = intel_dp_long_pulse(intel_dp-
> > >attached_connector, ctx);
> > -	}
> > -
> > -	intel_dp->detect_done = false;
> > -
> > -	return status;
> > -}
> > -
> >  static void
> >  intel_dp_force(struct drm_connector *connector)
> >  {
> > @@ -5643,7 +5624,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  
> >  	if (long_hpd) {
> >  		intel_dp->reset_link_params = true;
> > -		intel_dp->detect_done = false;
> >  		return IRQ_NONE;
> >  	}
> >  
> > @@ -5660,7 +5640,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  			intel_dp->is_mst = false;
> >  			drm_dp_mst_topology_mgr_set_mst(&intel_dp-
> > >mst_mgr,
> >  							intel_dp-
> > >is_mst);
> > -			intel_dp->detect_done = false;
> >  			goto put_power;
> >  		}
> >  	}
> > @@ -5673,10 +5652,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  		/* Short pulse can signify loss of hdcp
> > authentication */
> >  		intel_hdcp_check_link(intel_dp-
> > >attached_connector);
> >  
> > -		if (!handled) {
> > -			intel_dp->detect_done = false;
> > +		if (!handled)
> >  			goto put_power;
> > -		}
> >  	}
> >  
> >  	ret = IRQ_HANDLED;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index bf1c38728a59..c7206a4764e5 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1070,7 +1070,6 @@ struct intel_dp {
> >  	bool link_mst;
> >  	bool link_trained;
> >  	bool has_audio;
> > -	bool detect_done;
> >  	bool reset_link_params;
> >  	enum aux_ch aux_ch;
> >  	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
> > -- 
> > 2.14.1
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915/dp: Fix duplication of DEVICE_SERVICE_IRQ handling

[Dhinakaran Pandiyan]

There are two copies of the same code called from long and short
pulse handlers.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 59 +++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d06bf4303814..123c2eafafab 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4038,13 +4038,6 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
 					intel_dp->is_mst);
 }
 
-static bool
-intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
-{
-	return drm_dp_dpcd_readb(&intel_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR,
-				 sink_irq_vector) == 1;
-}
-
 static bool
 intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
@@ -4432,6 +4425,26 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
 	return changed;
 }
 
+static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
+{
+	u8 val;
+
+	if (intel_dp->dpcd[DP_DPCD_REV] < 0x11)
+		return;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux,
+			      DP_DEVICE_SERVICE_IRQ_VECTOR, &val) != 1 || !val)
+		return;
+
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR, val);
+
+	if (val & DP_AUTOMATED_TEST_REQUEST)
+		intel_dp_handle_test_request(intel_dp);
+
+	if (val & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
+		DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4449,7 +4462,6 @@ static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
-	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
 
@@ -4472,20 +4484,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	/* Try to read the source of the interrupt */
-	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
-	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector) &&
-	    sink_irq_vector != 0) {
-		/* Clear interrupt source */
-		drm_dp_dpcd_writeb(&intel_dp->aux,
-				   DP_DEVICE_SERVICE_IRQ_VECTOR,
-				   sink_irq_vector);
-
-		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			intel_dp_handle_test_request(intel_dp);
-		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
-			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
-	}
+	intel_dp_check_service_irq(intel_dp);
 
 	/* Handle CEC interrupts, if any */
 	drm_dp_cec_irq(&intel_dp->aux);
@@ -5019,7 +5018,6 @@ intel_dp_detect(struct drm_connector *connector,
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int status;
-	u8 sink_irq_vector = 0;
 	struct drm_crtc *crtc;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
@@ -5109,20 +5107,7 @@ intel_dp_detect(struct drm_connector *connector,
 	    to_intel_connector(connector)->detect_edid)
 		status = connector_status_connected;
 
-	/* Try to read the source of the interrupt */
-	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
-	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector) &&
-	    sink_irq_vector != 0) {
-		/* Clear interrupt source */
-		drm_dp_dpcd_writeb(&intel_dp->aux,
-				   DP_DEVICE_SERVICE_IRQ_VECTOR,
-				   sink_irq_vector);
-
-		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			intel_dp_handle_test_request(intel_dp);
-		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
-			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
-	}
+	intel_dp_check_service_irq(intel_dp);
 
 out:
 	if (status != connector_status_connected && !intel_dp->is_mst)
-- 
2.14.1

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

[PATCH 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Dhinakaran Pandiyan]

This way all short pulse handlers except MST are in one place.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 123c2eafafab..03d2c6426016 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 		drm_kms_helper_hotplug_event(&dev_priv->drm);
 	}
 
+	/* Short pulse can signify loss of hdcp authentication */
+	intel_hdcp_check_link(intel_dp->attached_connector);
+
 	return true;
 }
 
@@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 		handled = intel_dp_short_pulse(intel_dp);
 
-		/* Short pulse can signify loss of hdcp authentication */
-		intel_hdcp_check_link(intel_dp->attached_connector);
-
 		if (!handled)
 			goto put_power;
 	}
-- 
2.14.1

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

Re: [PATCH 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Ville Syrjälä]

On Tue, Sep 18, 2018 at 12:20:09AM -0700, Dhinakaran Pandiyan wrote:
> This way all short pulse handlers except MST are in one place.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 123c2eafafab..03d2c6426016 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  		drm_kms_helper_hotplug_event(&dev_priv->drm);
>  	}
>  
> +	/* Short pulse can signify loss of hdcp authentication */
> +	intel_hdcp_check_link(intel_dp->attached_connector);
> +
>  	return true;
>  }
>  
> @@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  
>  		handled = intel_dp_short_pulse(intel_dp);
>  
> -		/* Short pulse can signify loss of hdcp authentication */
> -		intel_hdcp_check_link(intel_dp->attached_connector);
> -

This changes the behaviour to skip intel_hdcp_check_link() when
intel_dp_short_pulse() decides to fall back to full detect. Not sure if
that's OK or not.

>  		if (!handled)
>  			goto put_power;
>  	}
> -- 
> 2.14.1
> 
> _______________________________________________
> 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 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Pandiyan, Dhinakaran]

On Wed, 2018-09-19 at 20:30 +0300, Ville Syrjälä wrote:
> On Tue, Sep 18, 2018 at 12:20:09AM -0700, Dhinakaran Pandiyan wrote:
> > This way all short pulse handlers except MST are in one place.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 123c2eafafab..03d2c6426016 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp
> > *intel_dp)
> >  		drm_kms_helper_hotplug_event(&dev_priv->drm);
> >  	}
> >  
> > +	/* Short pulse can signify loss of hdcp authentication */
> > +	intel_hdcp_check_link(intel_dp->attached_connector);
> > +
> >  	return true;
> >  }
> >  
> > @@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  
> >  		handled = intel_dp_short_pulse(intel_dp);
> >  
> > -		/* Short pulse can signify loss of hdcp
> > authentication */
> > -		intel_hdcp_check_link(intel_dp-
> > >attached_connector);
> > -
> 
> This changes the behaviour to skip intel_hdcp_check_link() when
> intel_dp_short_pulse() decides to fall back to full detect. Not sure
> if
> that's OK or not.

I'm thinking we should move the decision to fallback to the very end of
intel_dp_short_pulse(). 

The benefit we get is to disable PSR if the sink complained of PSR
errors and then check if the hardware retrained the link correctly
after we disabled.


> 
> >  		if (!handled)
> >  			goto put_power;
> >  	}
> > -- 
> > 2.14.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 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Ville Syrjälä]

On Wed, Sep 19, 2018 at 05:49:33PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-09-19 at 20:30 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 18, 2018 at 12:20:09AM -0700, Dhinakaran Pandiyan wrote:
> > > This way all short pulse handlers except MST are in one place.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 123c2eafafab..03d2c6426016 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp
> > > *intel_dp)
> > >  		drm_kms_helper_hotplug_event(&dev_priv->drm);
> > >  	}
> > >  
> > > +	/* Short pulse can signify loss of hdcp authentication */
> > > +	intel_hdcp_check_link(intel_dp->attached_connector);
> > > +
> > >  	return true;
> > >  }
> > >  
> > > @@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port, bool long_hpd)
> > >  
> > >  		handled = intel_dp_short_pulse(intel_dp);
> > >  
> > > -		/* Short pulse can signify loss of hdcp
> > > authentication */
> > > -		intel_hdcp_check_link(intel_dp-
> > > >attached_connector);
> > > -
> > 
> > This changes the behaviour to skip intel_hdcp_check_link() when
> > intel_dp_short_pulse() decides to fall back to full detect. Not sure
> > if
> > that's OK or not.
> 
> I'm thinking we should move the decision to fallback to the very end of
> intel_dp_short_pulse(). 

Perhaps. The entire "how do we process hpd_irqs correctly?" thing
needs some real thought.

> 
> The benefit we get is to disable PSR if the sink complained of PSR
> errors and then check if the hardware retrained the link correctly
> after we disabled.
> 
> 
> > 
> > >  		if (!handled)
> > >  			goto put_power;
> > >  	}
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > 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 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Dhinakaran Pandiyan]

On Wed, 2018-09-19 at 21:05 +0300, Ville Syrjälä wrote:
> On Wed, Sep 19, 2018 at 05:49:33PM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-09-19 at 20:30 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 18, 2018 at 12:20:09AM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > This way all short pulse handlers except MST are in one place.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> > > > om>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 123c2eafafab..03d2c6426016 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp
> > > > *intel_dp)
> > > >  		drm_kms_helper_hotplug_event(&dev_priv->drm);
> > > >  	}
> > > >  
> > > > +	/* Short pulse can signify loss of hdcp authentication
> > > > */
> > > > +	intel_hdcp_check_link(intel_dp->attached_connector);
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct
> > > > intel_digital_port
> > > > *intel_dig_port, bool long_hpd)
> > > >  
> > > >  		handled = intel_dp_short_pulse(intel_dp);
> > > >  
> > > > -		/* Short pulse can signify loss of hdcp
> > > > authentication */
> > > > -		intel_hdcp_check_link(intel_dp-
> > > > > attached_connector);
> > > > 
> > > > -
> > > 
> > > This changes the behaviour to skip intel_hdcp_check_link() when
> > > intel_dp_short_pulse() decides to fall back to full detect. Not
> > > sure
> > > if
> > > that's OK or not.
> > 
> > I'm thinking we should move the decision to fallback to the very
> > end of
> > intel_dp_short_pulse(). 
> 
> Perhaps. The entire "how do we process hpd_irqs correctly?" thing
> needs some real thought.

One noteworthy information I remember from my last reading of the spec
is that the sink is required to generate a distinct IRQ_HPD to request
retraining. But then, if we get two interrupts in quick succession, a
queued work item will end up handling both. 

> 
> > 
> > The benefit we get is to disable PSR if the sink complained of PSR
> > errors and then check if the hardware retrained the link correctly
> > after we disabled.
> > 
> > 
> > > 
> > > >  		if (!handled)
> > > >  			goto put_power;
> > > >  	}
> > > > -- 
> > > > 2.14.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 5/5] drm/i915/dp: Move hdcp link check function into short pulse handler

[Pandiyan, Dhinakaran]

On Wed, 2018-09-19 at 21:05 +0300, Ville Syrjälä wrote:
> On Wed, Sep 19, 2018 at 05:49:33PM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-09-19 at 20:30 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 18, 2018 at 12:20:09AM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > This way all short pulse handlers except MST are in one place.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> > > > om>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 123c2eafafab..03d2c6426016 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4501,6 +4501,9 @@ intel_dp_short_pulse(struct intel_dp
> > > > *intel_dp)
> > > >  		drm_kms_helper_hotplug_event(&dev_priv->drm);
> > > >  	}
> > > >  
> > > > +	/* Short pulse can signify loss of hdcp authentication
> > > > */
> > > > +	intel_hdcp_check_link(intel_dp->attached_connector);
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -5634,9 +5637,6 @@ intel_dp_hpd_pulse(struct
> > > > intel_digital_port
> > > > *intel_dig_port, bool long_hpd)
> > > >  
> > > >  		handled = intel_dp_short_pulse(intel_dp);
> > > >  
> > > > -		/* Short pulse can signify loss of hdcp
> > > > authentication */
> > > > -		intel_hdcp_check_link(intel_dp-
> > > > > attached_connector);
> > > > 
> > > > -
> > > 
> > > This changes the behaviour to skip intel_hdcp_check_link() when
> > > intel_dp_short_pulse() decides to fall back to full detect. Not
> > > sure
> > > if
> > > that's OK or not.
> > 
> > I'm thinking we should move the decision to fallback to the very
> > end of
> > intel_dp_short_pulse(). 
> 
> Perhaps. The entire "how do we process hpd_irqs correctly?" thing
> needs some real thought.

One noteworthy information I remember from my last reading of the spec
is that the sink is required to generate a distinct IRQ_HPD to request
retraining. But then, if we get two interrupts in quick succession, a
queued work item will end up handling both. 

> 
> > 
> > The benefit we get is to disable PSR if the sink complained of PSR
> > errors and then check if the hardware retrained the link correctly
> > after we disabled.
> > 
> > 
> > > 
> > > >  		if (!handled)
> > > >  			goto put_power;
> > > >  	}
> > > > -- 
> > > > 2.14.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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

-:27: 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"")'
#27: 
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")

total: 1 errors, 1 warnings, 0 checks, 18 lines checked
2725094c4d5a 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
33bb993204ea drm/i915/dp: Remove intel_dp->detect_done flag
5872eb154e8c drm/i915/dp: Fix duplication of DEVICE_SERVICE_IRQ handling
c7ba164dc462 drm/i915/dp: Move hdcp link check function into short pulse handler

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4835 -> Patchwork_10209 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10209 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10209, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10209:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_fence@basic-busy-default:
      fi-bdw-samus:       NOTRUN -> DMESG-WARN

    


== Participating hosts (45 -> 41) ==

  Additional (1): fi-bdw-samus 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4835 -> Patchwork_10209

  CI_DRM_4835: 0875e7885717d3d812941e8e2db1dc99728be1ff @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4645: 03b90a39ed12a568c9da752466ea708d6348e110 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10209: c7ba164dc4626568faf4a1c4f2d51caebb2bc9d3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c7ba164dc462 drm/i915/dp: Move hdcp link check function into short pulse handler
5872eb154e8c drm/i915/dp: Fix duplication of DEVICE_SERVICE_IRQ handling
33bb993204ea drm/i915/dp: Remove intel_dp->detect_done flag
2725094c4d5a drm/i915/dp: Restrict link retrain workaround to external monitors
b03548d38052 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_10209/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Tue, Sep 18, 2018 at 12:20:05AM -0700, 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().

have you?!
I just see the comment change on this patch ;)


> v2: Patch split and rewrote comment..
> 
> 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 | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..cac1c7c6cbfd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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.
> +		 * Some monitors do not signal loss of link synchronization
> +		 * with an IRQ_HPD, so force a link status check.
>  		 */
>  		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  
> -- 
> 2.14.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 1/5] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Pandiyan, Dhinakaran]

On Tue, 2018-09-18 at 08:51 -0700, Rodrigo Vivi wrote:
> On Tue, Sep 18, 2018 at 12:20:05AM -0700, 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().
> 
> have you?!
> I just see the comment change on this patch ;)
> 
> 
> > v2: Patch split and rewrote comment..
I split the renaming part. I'll rewrite the commit message if you want
me to.



> > 
> > 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 | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 436c22de33b6..cac1c7c6cbfd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -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.
> > +		 * Some monitors do not signal loss of link
> > synchronization
> > +		 * with an IRQ_HPD, so force a link status check.
> >  		 */
> >  		struct intel_encoder *encoder =
> > &dp_to_dig_port(intel_dp)->base;
> >  
> > -- 
> > 2.14.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