[PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently some DP sinks are a little nuts and cause HPD to drop
intermittently during modesets. This happens eg. on an ASUS PB287Q.
In oder to recover from this we can't really use the previous
connector status to determine if the link needs retraining, so let's
just ignore that piece of information and do the retrain
unconditionally. We do of course still check whether the link is
supposed to be running or not.

Cc: stable@vger.kernel.org
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 024798a9c016..37a746f7fbc3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else if (connector->status == connector_status_connected) {
+	} else {
 		/*
-		 * If display was connected already and is still connected
-		 * check links status, there has been known issues of
-		 * link loss triggerring long pulse!!!!
+		 * If display is now connected check links status,
+		 * there has been known issues of link loss triggerring
+		 * long pulse.
+		 *
+		 * Some sinks (eg. ASUS PB287Q) seem to perform some
+		 * weird HPD ping pong during modesets. So we can apparely
+		 * 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.
 		 */
 		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
-- 
2.10.2

Re: [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Palmer Dabbelt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On Thu, 16 Feb 2017 07:30:07 PST (-0800), ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Apparently some DP sinks are a little nuts and cause HPD to drop
> intermittently during modesets. This happens eg. on an ASUS PB287Q.
> In oder to recover from this we can't really use the previous
> connector status to determine if the link needs retraining, so let's
> just ignore that piece of information and do the retrain
> unconditionally. We do of course still check whether the link is
> supposed to be running or not.

With this patch on top of linux-4.9 I have DPMS on/off working again, but I
don't have the correct resolutions on my monitor.  I just see

DP1 connected 1024x768+2880+0 (normal left inverted right x axis y axis) 0mm x 0mm
   1024x768      60.00*
   800x600       60.32    56.25
   848x480       60.00
   640x480       59.94

>
> Cc: stable@vger.kernel.org
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 024798a9c016..37a746f7fbc3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> +	} else {
>  		/*
> -		 * If display was connected already and is still connected
> -		 * check links status, there has been known issues of
> -		 * link loss triggerring long pulse!!!!
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggerring
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparely
> +		 * 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.
>  		 */
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);

Re: [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Thu, Feb 16, 2017 at 07:39:29AM -0800, Palmer Dabbelt wrote:
> On Thu, 16 Feb 2017 07:30:07 PST (-0800), ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Apparently some DP sinks are a little nuts and cause HPD to drop
> > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > In oder to recover from this we can't really use the previous
> > connector status to determine if the link needs retraining, so let's
> > just ignore that piece of information and do the retrain
> > unconditionally. We do of course still check whether the link is
> > supposed to be running or not.
> 
> With this patch on top of linux-4.9 I have DPMS on/off working again, but I
> don't have the correct resolutions on my monitor.  I just see
> 
> DP1 connected 1024x768+2880+0 (normal left inverted right x axis y axis) 0mm x 0mm
>    1024x768      60.00*
>    800x600       60.32    56.25
>    848x480       60.00
>    640x480       59.94

Hmm. That would indicate that the EDID reads are also failing now.
I wonder if your monitor is just a lemon.

Anyways, there were no failed EDID reads visible in the logs you
provided. You should keep the drm.debug=0xe and rerun xrandr (do not
use the --current knob), and then we should check the dmesg again
to see what the kernel thinks the mode list should look like.

> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 024798a9c016..37a746f7fbc3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > -	} else if (connector->status == connector_status_connected) {
> > +	} else {
> >  		/*
> > -		 * If display was connected already and is still connected
> > -		 * check links status, there has been known issues of
> > -		 * link loss triggerring long pulse!!!!
> > +		 * If display is now connected check links status,
> > +		 * there has been known issues of link loss triggerring
> > +		 * long pulse.
> > +		 *
> > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > +		 * weird HPD ping pong during modesets. So we can apparely
> > +		 * 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.
> >  		 */
> >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >  		intel_dp_check_link_status(intel_dp);

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Manasi Navare]

On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Apparently some DP sinks are a little nuts and cause HPD to drop
> intermittently during modesets. This happens eg. on an ASUS PB287Q.
> In oder to recover from this we can't really use the previous
> connector status to determine if the link needs retraining, so let's
> just ignore that piece of information and do the retrain
> unconditionally. We do of course still check whether the link is
> supposed to be running or not.
> 
> Cc: stable@vger.kernel.org
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 024798a9c016..37a746f7fbc3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> +	} else {
>  		/*
> -		 * If display was connected already and is still connected
> -		 * check links status, there has been known issues of
> -		 * link loss triggerring long pulse!!!!
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggerring
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparely
> +		 * 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.
>  		 */
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);
> --

So here we basically just ignore the connector status and retrain irrespectively.
But that means even if we have newer values now for max link rate/lane count from
DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
should be set to 0 on HPD pulse, they would be set only during a modeset.

Regards
Manasi

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

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
> On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > Apparently some DP sinks are a little nuts and cause HPD to drop
> > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > In oder to recover from this we can't really use the previous
> > connector status to determine if the link needs retraining, so let's
> > just ignore that piece of information and do the retrain
> > unconditionally. We do of course still check whether the link is
> > supposed to be running or not.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 024798a9c016..37a746f7fbc3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > -	} else if (connector->status == connector_status_connected) {
> > +	} else {
> >  		/*
> > -		 * If display was connected already and is still connected
> > -		 * check links status, there has been known issues of
> > -		 * link loss triggerring long pulse!!!!
> > +		 * If display is now connected check links status,
> > +		 * there has been known issues of link loss triggerring
> > +		 * long pulse.
> > +		 *
> > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > +		 * weird HPD ping pong during modesets. So we can apparely
> > +		 * 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.
> >  		 */
> >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >  		intel_dp_check_link_status(intel_dp);
> > --
> 
> So here we basically just ignore the connector status and retrain irrespectively.

We ignore the _previous_ connector status.

> But that means even if we have newer values now for max link rate/lane count from
> DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
> and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
> should be set to 0 on HPD pulse, they would be set only during a modeset.

The DPCD has already been parsed by this time.

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Manasi Navare]

On Thu, Feb 16, 2017 at 07:18:57PM +0200, Ville Syrj�l� wrote:
> On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
> > On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > 
> > > Apparently some DP sinks are a little nuts and cause HPD to drop
> > > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > > In oder to recover from this we can't really use the previous
> > > connector status to determine if the link needs retraining, so let's
> > > just ignore that piece of information and do the retrain
> > > unconditionally. We do of course still check whether the link is
> > > supposed to be running or not.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 024798a9c016..37a746f7fbc3 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > -	} else if (connector->status == connector_status_connected) {
> > > +	} else {
> > >  		/*
> > > -		 * If display was connected already and is still connected
> > > -		 * check links status, there has been known issues of
> > > -		 * link loss triggerring long pulse!!!!
> > > +		 * If display is now connected check links status,
> > > +		 * there has been known issues of link loss triggerring
> > > +		 * long pulse.
> > > +		 *
> > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > +		 * weird HPD ping pong during modesets. So we can apparely
> > > +		 * 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.
> > >  		 */
> > >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >  		intel_dp_check_link_status(intel_dp);
> > > --
> > 
> > So here we basically just ignore the connector status and retrain irrespectively.
> 
> We ignore the _previous_ connector status.
> 
> > But that means even if we have newer values now for max link rate/lane count from
> > DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
> > and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
> > should be set to 0 on HPD pulse, they would be set only during a modeset.
> 
> The DPCD has already been parsed by this time.
> 
> -- 
> Ville Syrj�l�
> Intel OTC

Yes, we have parsed the DPCD but we dont write to intel_dp->link-rate and intel_dp->lane_count
until we do a modeset (these get written during pre_enable) and these values get used
during the retraining of the link. So at this point we will still use stale values.
Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
for retraining?

Regards
Manasi

Regards
Manasi

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Thu, Feb 16, 2017 at 09:24:09AM -0800, Manasi Navare wrote:
> On Thu, Feb 16, 2017 at 07:18:57PM +0200, Ville Syrj�l� wrote:
> > On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
> > > On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > 
> > > > Apparently some DP sinks are a little nuts and cause HPD to drop
> > > > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > > > In oder to recover from this we can't really use the previous
> > > > connector status to determine if the link needs retraining, so let's
> > > > just ignore that piece of information and do the retrain
> > > > unconditionally. We do of course still check whether the link is
> > > > supposed to be running or not.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 024798a9c016..37a746f7fbc3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > >  		 */
> > > >  		status = connector_status_disconnected;
> > > >  		goto out;
> > > > -	} else if (connector->status == connector_status_connected) {
> > > > +	} else {
> > > >  		/*
> > > > -		 * If display was connected already and is still connected
> > > > -		 * check links status, there has been known issues of
> > > > -		 * link loss triggerring long pulse!!!!
> > > > +		 * If display is now connected check links status,
> > > > +		 * there has been known issues of link loss triggerring
> > > > +		 * long pulse.
> > > > +		 *
> > > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > > +		 * weird HPD ping pong during modesets. So we can apparely
> > > > +		 * 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.
> > > >  		 */
> > > >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > >  		intel_dp_check_link_status(intel_dp);
> > > > --
> > > 
> > > So here we basically just ignore the connector status and retrain irrespectively.
> > 
> > We ignore the _previous_ connector status.
> > 
> > > But that means even if we have newer values now for max link rate/lane count from
> > > DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
> > > and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
> > > should be set to 0 on HPD pulse, they would be set only during a modeset.
> > 
> > The DPCD has already been parsed by this time.
> > 
> > -- 
> > Ville Syrj�l�
> > Intel OTC
> 
> Yes, we have parsed the DPCD but we dont write to intel_dp->link-rate and intel_dp->lane_count
> until we do a modeset (these get written during pre_enable) and these values get used
> during the retraining of the link. So at this point we will still use stale values.

They're not stale. They're exactly what we used when we set up the
mode that's still being drive out. This is just retraining the link
at the same parameters.

> Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
> for retraining?
> 
> Regards
> Manasi
> 
> Regards
> Manasi

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Palmer Dabbelt]

[-- Attachment #1: Type: text/plain, Size: 4609 bytes --]

On Thu, Feb 16, 2017 at 9:46 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Feb 16, 2017 at 09:24:09AM -0800, Manasi Navare wrote:
>> On Thu, Feb 16, 2017 at 07:18:57PM +0200, Ville Syrjälä wrote:
>> > On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
>> > > On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
>> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > >
>> > > > Apparently some DP sinks are a little nuts and cause HPD to drop
>> > > > intermittently during modesets. This happens eg. on an ASUS PB287Q.
>> > > > In oder to recover from this we can't really use the previous
>> > > > connector status to determine if the link needs retraining, so let's
>> > > > just ignore that piece of information and do the retrain
>> > > > unconditionally. We do of course still check whether the link is
>> > > > supposed to be running or not.
>> > > >
>> > > > Cc: stable@vger.kernel.org
>> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> > > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
>> > > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
>> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
>> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > > > index 024798a9c016..37a746f7fbc3 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>> > > >                  */
>> > > >                 status = connector_status_disconnected;
>> > > >                 goto out;
>> > > > -       } else if (connector->status == connector_status_connected) {
>> > > > +       } else {
>> > > >                 /*
>> > > > -                * If display was connected already and is still connected
>> > > > -                * check links status, there has been known issues of
>> > > > -                * link loss triggerring long pulse!!!!
>> > > > +                * If display is now connected check links status,
>> > > > +                * there has been known issues of link loss triggerring
>> > > > +                * long pulse.
>> > > > +                *
>> > > > +                * Some sinks (eg. ASUS PB287Q) seem to perform some
>> > > > +                * weird HPD ping pong during modesets. So we can apparely
>> > > > +                * 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.
>> > > >                  */
>> > > >                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> > > >                 intel_dp_check_link_status(intel_dp);
>> > > > --
>> > >
>> > > So here we basically just ignore the connector status and retrain irrespectively.
>> >
>> > We ignore the _previous_ connector status.
>> >
>> > > But that means even if we have newer values now for max link rate/lane count from
>> > > DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
>> > > and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
>> > > should be set to 0 on HPD pulse, they would be set only during a modeset.
>> >
>> > The DPCD has already been parsed by this time.
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>>
>> Yes, we have parsed the DPCD but we dont write to intel_dp->link-rate and intel_dp->lane_count
>> until we do a modeset (these get written during pre_enable) and these values get used
>> during the retraining of the link. So at this point we will still use stale values.
>
> They're not stale. They're exactly what we used when we set up the
> mode that's still being drive out. This is just retraining the link
> at the same parameters.
>
>> Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
>> for retraining?
>>
>> Regards
>> Manasi
>>
>> Regards
>> Manasi
>
> --
> Ville Syrjälä
> Intel OTC

Sorry, I forgot about this.  A log is attached of my machine booting
with the monitor attached, me detaching the monitor, and then plugging
it back in.

[-- Attachment #2: log.gz --]
[-- Type: application/x-gzip, Size: 35903 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Wed, Feb 22, 2017 at 08:00:38PM -0800, Palmer Dabbelt wrote:
> On Thu, Feb 16, 2017 at 9:46 AM, Ville Syrj�l�
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Feb 16, 2017 at 09:24:09AM -0800, Manasi Navare wrote:
> >> On Thu, Feb 16, 2017 at 07:18:57PM +0200, Ville Syrj�l� wrote:
> >> > On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
> >> > > On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala@linux.intel.com wrote:
> >> > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> > > >
> >> > > > Apparently some DP sinks are a little nuts and cause HPD to drop
> >> > > > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> >> > > > In oder to recover from this we can't really use the previous
> >> > > > connector status to determine if the link needs retraining, so let's
> >> > > > just ignore that piece of information and do the retrain
> >> > > > unconditionally. We do of course still check whether the link is
> >> > > > supposed to be running or not.
> >> > > >
> >> > > > Cc: stable@vger.kernel.org
> >> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >> > > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> > > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> >> > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> > > > ---
> >> > > >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> >> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> > > > index 024798a9c016..37a746f7fbc3 100644
> >> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > > > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >> > > >                  */
> >> > > >                 status = connector_status_disconnected;
> >> > > >                 goto out;
> >> > > > -       } else if (connector->status == connector_status_connected) {
> >> > > > +       } else {
> >> > > >                 /*
> >> > > > -                * If display was connected already and is still connected
> >> > > > -                * check links status, there has been known issues of
> >> > > > -                * link loss triggerring long pulse!!!!
> >> > > > +                * If display is now connected check links status,
> >> > > > +                * there has been known issues of link loss triggerring
> >> > > > +                * long pulse.
> >> > > > +                *
> >> > > > +                * Some sinks (eg. ASUS PB287Q) seem to perform some
> >> > > > +                * weird HPD ping pong during modesets. So we can apparely
> >> > > > +                * 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.
> >> > > >                  */
> >> > > >                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >> > > >                 intel_dp_check_link_status(intel_dp);
> >> > > > --
> >> > >
> >> > > So here we basically just ignore the connector status and retrain irrespectively.
> >> >
> >> > We ignore the _previous_ connector status.
> >> >
> >> > > But that means even if we have newer values now for max link rate/lane count from
> >> > > DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
> >> > > and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
> >> > > should be set to 0 on HPD pulse, they would be set only during a modeset.
> >> >
> >> > The DPCD has already been parsed by this time.
> >> >
> >> > --
> >> > Ville Syrj�l�
> >> > Intel OTC
> >>
> >> Yes, we have parsed the DPCD but we dont write to intel_dp->link-rate and intel_dp->lane_count
> >> until we do a modeset (these get written during pre_enable) and these values get used
> >> during the retraining of the link. So at this point we will still use stale values.
> >
> > They're not stale. They're exactly what we used when we set up the
> > mode that's still being drive out. This is just retraining the link
> > at the same parameters.
> >
> >> Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
> >> for retraining?
> >>
> >> Regards
> >> Manasi
> >>
> >> Regards
> >> Manasi
> >
> > --
> > Ville Syrj�l�
> > Intel OTC
> 
> Sorry, I forgot about this.  A log is attached of my machine booting
> with the monitor attached, me detaching the monitor, and then plugging
> it back in.

I was actually able to reproduce this yesterday with one particular
kernel. It looked like it wasn't even trying to do the EDID read. Not
sure why. And I wasn't able reproduce it anymore with the latest
drm-tip. I should probably try to figure out what changed...

-- 
Ville Syrj�l�
Intel OTC

[PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently some DP sinks are a little nuts and cause HPD to drop
intermittently during modesets. This happens eg. on an ASUS PB287Q.
In oder to recover from this we can't really use the previous
connector status to determine if the link needs retraining, so let's
just ignore that piece of information and do the retrain
unconditionally. We do of course still check whether the link is
supposed to be running or not.

To actually get read out the EDID and update things properly we
also need to nuke the goto out added by commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
why that was there. Perhaps to avoid an EDID read if the connector
status didn't appear to change, but that sort of thing is quite racy
and would have failed anyway if we failed to keep up with the
hotplugs (if we missed the HPD down in between two HPD ups). And
now that we take this codepath unconditionally we definitely need
to drop the goto as otherwise we would never do the EDID read.

v2: Drop the goto that made us skip EDID reads entirely. Doh!

Cc: stable@vger.kernel.org
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fd96a6cf7326..5c2f1b37b58f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4634,16 +4634,22 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else if (connector->status == connector_status_connected) {
+	} else {
 		/*
-		 * If display was connected already and is still connected
-		 * check links status, there has been known issues of
-		 * link loss triggerring long pulse!!!!
+		 * If display is now connected check links status,
+		 * there has been known issues of link loss triggerring
+		 * long pulse.
+		 *
+		 * Some sinks (eg. ASUS PB287Q) seem to perform some
+		 * weird HPD ping pong during modesets. So we can apparely
+		 * 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.
 		 */
 		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
 		drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		goto out;
 	}
 
 	/*
-- 
2.10.2

Re: [Intel-gfx] [PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse

[Chris Wilson]

On Mon, Mar 13, 2017 at 10:53:23PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Apparently some DP sinks are a little nuts and cause HPD to drop
> intermittently during modesets. This happens eg. on an ASUS PB287Q.
> In oder to recover from this we can't really use the previous
> connector status to determine if the link needs retraining, so let's
> just ignore that piece of information and do the retrain
> unconditionally. We do of course still check whether the link is
> supposed to be running or not.
> 
> To actually get read out the EDID and update things properly we
> also need to nuke the goto out added by commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
> why that was there. Perhaps to avoid an EDID read if the connector
> status didn't appear to change, but that sort of thing is quite racy
> and would have failed anyway if we failed to keep up with the
> hotplugs (if we missed the HPD down in between two HPD ups). And
> now that we take this codepath unconditionally we definitely need
> to drop the goto as otherwise we would never do the EDID read.
> 
> v2: Drop the goto that made us skip EDID reads entirely. Doh!
> 
> Cc: stable@vger.kernel.org
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fd96a6cf7326..5c2f1b37b58f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4634,16 +4634,22 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> +	} else {
>  		/*
> -		 * If display was connected already and is still connected
> -		 * check links status, there has been known issues of
> -		 * link loss triggerring long pulse!!!!
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggerring
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparely
s/apparely/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.

Makes sense as the retraining is only applied if the crtc was active
(i.e. the connector was previously connected and trained).

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse

[Manasi Navare]

On Mon, Mar 13, 2017 at 10:53:23PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Apparently some DP sinks are a little nuts and cause HPD to drop
> intermittently during modesets. This happens eg. on an ASUS PB287Q.
> In oder to recover from this we can't really use the previous
> connector status to determine if the link needs retraining, so let's
> just ignore that piece of information and do the retrain
> unconditionally. We do of course still check whether the link is
> supposed to be running or not.
> 
> To actually get read out the EDID and update things properly we
> also need to nuke the goto out added by commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
> why that was there. Perhaps to avoid an EDID read if the connector
> status didn't appear to change, but that sort of thing is quite racy
> and would have failed anyway if we failed to keep up with the
> hotplugs (if we missed the HPD down in between two HPD ups). And
> now that we take this codepath unconditionally we definitely need
> to drop the goto as otherwise we would never do the EDID read.
> 
> v2: Drop the goto that made us skip EDID reads entirely. Doh!
> 
> Cc: stable@vger.kernel.org
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fd96a6cf7326..5c2f1b37b58f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4634,16 +4634,22 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> +	} else {
>  		/*
> -		 * If display was connected already and is still connected
> -		 * check links status, there has been known issues of
> -		 * link loss triggerring long pulse!!!!
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggerring
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparely
> +		 * 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.
>  		 */
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);
>  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		goto out;
>  	}
>

Yes makes sense to me too since we have a check for crtc->active so
it will retrain only if the link is already up and running with an active crtc
and intel_dp->lane_count is not 0.
In case of link failures though when we send the hotplug uevent it will get through
this path and it will try to retrain which we dont want since the retraining
should happen only in the atomic_commit at the fallback rate, not here.
So I guess on link failure, I can set intel_dp->lane_count to 0 to invalidate that
and avoid retraining here.

Right?

Regards
Manasi  
>  	/*
> -- 
> 2.10.2
> 

Re: [PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 04:09:53PM -0700, Manasi Navare wrote:
> On Mon, Mar 13, 2017 at 10:53:23PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > Apparently some DP sinks are a little nuts and cause HPD to drop
> > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > In oder to recover from this we can't really use the previous
> > connector status to determine if the link needs retraining, so let's
> > just ignore that piece of information and do the retrain
> > unconditionally. We do of course still check whether the link is
> > supposed to be running or not.
> > 
> > To actually get read out the EDID and update things properly we
> > also need to nuke the goto out added by commit 7d23e3c37bb3
> > ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
> > why that was there. Perhaps to avoid an EDID read if the connector
> > status didn't appear to change, but that sort of thing is quite racy
> > and would have failed anyway if we failed to keep up with the
> > hotplugs (if we missed the HPD down in between two HPD ups). And
> > now that we take this codepath unconditionally we definitely need
> > to drop the goto as otherwise we would never do the EDID read.
> > 
> > v2: Drop the goto that made us skip EDID reads entirely. Doh!
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
> > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index fd96a6cf7326..5c2f1b37b58f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4634,16 +4634,22 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > -	} else if (connector->status == connector_status_connected) {
> > +	} else {
> >  		/*
> > -		 * If display was connected already and is still connected
> > -		 * check links status, there has been known issues of
> > -		 * link loss triggerring long pulse!!!!
> > +		 * If display is now connected check links status,
> > +		 * there has been known issues of link loss triggerring
> > +		 * long pulse.
> > +		 *
> > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > +		 * weird HPD ping pong during modesets. So we can apparely
> > +		 * 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.
> >  		 */
> >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >  		intel_dp_check_link_status(intel_dp);
> >  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > -		goto out;
> >  	}
> >
> 
> Yes makes sense to me too since we have a check for crtc->active so
> it will retrain only if the link is already up and running with an active crtc
> and intel_dp->lane_count is not 0.
> In case of link failures though when we send the hotplug uevent it will get through
> this path and it will try to retrain which we dont want since the retraining
> should happen only in the atomic_commit at the fallback rate, not here.
> So I guess on link failure, I can set intel_dp->lane_count to 0 to invalidate that
> and avoid retraining here.

I'm not sure we want to expand the link_count abuse here. It was only
added as a hack anyway. But the retraining code should perhaps check
whether the current link parameters are still in the set of
valid/untested parameters and skip the retraining if not.

-- 
Ville Syrj�l�
Intel OTC

[PATCH v3] drm/i915: Perform link quality check unconditionally during long pulse

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently some DP sinks are a little nuts and cause HPD to drop
intermittently during modesets. This happens eg. on an ASUS PB287Q.
In oder to recover from this we can't really use the previous
connector status to determine if the link needs retraining, so let's
just ignore that piece of information and do the retrain
unconditionally. We do of course still check whether the link is
supposed to be running or not.

To actually get read out the EDID and update things properly we
also need to nuke the goto out added by commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
why that was there. Perhaps to avoid an EDID read if the connector
status didn't appear to change, but that sort of thing is quite racy
and would have failed anyway if we failed to keep up with the
hotplugs (if we missed the HPD down in between two HPD ups). And
now that we take this codepath unconditionally we definitely need
to drop the goto as otherwise we would never do the EDID read.

v2: Drop the goto that made us skip EDID reads entirely. Doh!
v3: Rebase due to locking changes
    s/apparely/apparently/ in the comment (Chris)

Cc: stable@vger.kernel.org
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 16b7bf7af537..09601a22d3cc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4685,9 +4685,20 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else if (connector->status == connector_status_connected) {
+	} else {
+		/*
+		 * If display is now connected check links status,
+		 * there has been known issues of link loss triggerring
+		 * 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.
+		 */
 		intel_dp_check_link_status(intel_dp);
-		goto out;
 	}
 
 	/*
-- 
2.10.2

Re: [PATCH v3] drm/i915: Perform link quality check unconditionally during long pulse

[Ville Syrjälä]

On Wed, Apr 12, 2017 at 10:30:17PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Apparently some DP sinks are a little nuts and cause HPD to drop
> intermittently during modesets. This happens eg. on an ASUS PB287Q.
> In oder to recover from this we can't really use the previous
> connector status to determine if the link needs retraining, so let's
> just ignore that piece of information and do the retrain
> unconditionally. We do of course still check whether the link is
> supposed to be running or not.
> 
> To actually get read out the EDID and update things properly we
> also need to nuke the goto out added by commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
> why that was there. Perhaps to avoid an EDID read if the connector
> status didn't appear to change, but that sort of thing is quite racy
> and would have failed anyway if we failed to keep up with the
> hotplugs (if we missed the HPD down in between two HPD ups). And
> now that we take this codepath unconditionally we definitely need
> to drop the goto as otherwise we would never do the EDID read.
> 
> v2: Drop the goto that made us skip EDID reads entirely. Doh!
> v3: Rebase due to locking changes
>     s/apparely/apparently/ in the comment (Chris)
> 
> Cc: stable@vger.kernel.org
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And pushed to dinq. Thanks for the review.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 16b7bf7af537..09601a22d3cc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4685,9 +4685,20 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> +	} else {
> +		/*
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggerring
> +		 * 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.
> +		 */
>  		intel_dp_check_link_status(intel_dp);
> -		goto out;
>  	}
>  
>  	/*
> -- 
> 2.10.2

-- 
Ville Syrj�l�
Intel OTC