[PATCH] drm/i915: Fix DisplayPort Hotplug

[PATCH] drm/i915: Fix DisplayPort Hotplug

[Palmer Dabbelt]

DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
attached to an ASUS PB287Q).  I believe the problem is that long pulses
are no longer triggering intel_dp_check_link_status.  I bisected the
problem down to (7d23e3c37 "drm/i915: Cleaning up intel_dp_hpd_pulse")
and it appears the only material change there was to remove one of those
calls.

This commit adds the check_link_status call back in, which causes hotplug to
work again for me.  I test this via a "xset dpms force off".  Note that this is
very similar to <https://bugs.freedesktop.org/show_bug.cgi?id=89453>, but it's
back again.

I've tested this against 4.9, and it applies against the current head.

See <https://bugs.freedesktop.org/show_bug.cgi?id=99766>.

Here's the commit that triggered the regression:

commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Date:   Wed Mar 30 18:05:23 2016 +0530

    drm/i915: Cleaning up intel_dp_hpd_pulse

    Current DP detection has DPCD operations split across
    intel_dp_hpd_pulse and intel_dp_detect which contains
    duplicates as well. Also intel_dp_detect is called
    during modes enumeration as well which will result
    in multiple dpcd operations. So this patch tries
    to solve both these by bringing all DPCD operations
    in one single function and make intel_dp_detect
    use existing values instead of repeating same steps.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0b8e8eb..32ca4be 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4831,6 +4831,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		      long_hpd ? "long" : "short");
 
 	if (long_hpd) {
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
 		intel_dp->detect_done = false;
 		return IRQ_NONE;
 	}
-- 
2.10.2

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

✗ Fi.CI.BAT: warning for drm/i915: Fix DisplayPort Hotplug

[Patchwork]

== Series Details ==

Series: drm/i915: Fix DisplayPort Hotplug
URL   : https://patchwork.freedesktop.org/series/19601/
State : warning

== Summary ==

Series 19601v1 drm/i915: Fix DisplayPort Hotplug
https://patchwork.freedesktop.org/api/1.0/series/19601/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-wb-ro-before-default:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (fi-skl-6260u) FDO#99803
Test gem_ringfill:
        Subgroup basic-default-fd:
                pass       -> DMESG-WARN (fi-ivb-3770)
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (fi-hsw-4770)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-hsw-4770)

FDO#99803 https://bugs.freedesktop.org/show_bug.cgi?id=99803

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:234  dwarn:2   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:233  dwarn:1   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:221  dwarn:3   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

6f8a1e0f2ca1011aa723712ca059b0b3432c3a88 drm-tip: 2017y-02m-13d-21h-03m-32s UTC integration manifest
4b84996 drm/i915: Fix DisplayPort Hotplug

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3795/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: Fix DisplayPort Hotplug

[Saarinen, Jani]

HI, 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Patchwork
> Sent: Tuesday, February 14, 2017 10:22 AM
> To: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] ✗ Fi.CI.BAT: warning for drm/i915: Fix DisplayPort Hotplug
> 
> == Series Details ==
> 
> Series: drm/i915: Fix DisplayPort Hotplug
> URL   : https://patchwork.freedesktop.org/series/19601/
> State : warning
> 
> == Summary ==
> 
> Series 19601v1 drm/i915: Fix DisplayPort Hotplug
> https://patchwork.freedesktop.org/api/1.0/series/19601/revisions/1/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-wb-ro-before-default:
>                 pass       -> DMESG-WARN (fi-snb-2520m)
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 incomplete -> PASS       (fi-skl-6260u) FDO#99803
> Test gem_ringfill:
>         Subgroup basic-default-fd:
>                 pass       -> DMESG-WARN (fi-ivb-3770)
>         Subgroup basic-default-hang:
>                 pass       -> DMESG-WARN (fi-hsw-4770)
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (fi-snb-2520m)
> Test kms_force_connector_basic:
>         Subgroup force-load-detect:
>                 pass       -> DMESG-WARN (fi-snb-2520m)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-b:
>                 incomplete -> PASS       (fi-hsw-4770)
All warnings are related to 
[drm:intel_dp_check_link_status [i915]] *ERROR* Failed to get link status


> FDO#99803 https://bugs.freedesktop.org/show_bug.cgi?id=99803
> 
> fi-hsw-4770      total:252  pass:234  dwarn:2   dfail:0   fail:0   skip:16
> fi-ivb-3770      total:252  pass:233  dwarn:1   dfail:0   fail:0   skip:18
> fi-snb-2520m     total:252  pass:221  dwarn:3   dfail:0   fail:0   skip:28
> 
> 6f8a1e0f2ca1011aa723712ca059b0b3432c3a88 drm-tip: 2017y-02m-13d-21h-
> 03m-32s UTC integration manifest
> 4b84996 drm/i915: Fix DisplayPort Hotplug
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3795/


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo


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

Re: [PATCH] drm/i915: Fix DisplayPort Hotplug

[Ville Syrjälä]

On Fri, Feb 10, 2017 at 02:44:20PM -0800, Palmer Dabbelt wrote:
> DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
> attached to an ASUS PB287Q).  I believe the problem is that long pulses
> are no longer triggering intel_dp_check_link_status.

That shouldn't itsefl cause problems with hotplugs. It could cause
problems if you hotplug displays without a proper randr client running
in which case the link is left running when you unplug the displays and
then gets retrained when you plug a display back in.

That doesn't explain why intel_dp_check_link_status() wouldn't get
called though. It should be called via intel_dp_detect() -> 
intel_dp_long_pulse().

> I bisected the
> problem down to (7d23e3c37 "drm/i915: Cleaning up intel_dp_hpd_pulse")
> and it appears the only material change there was to remove one of those
> calls.
> 
> This commit adds the check_link_status call back in, which causes hotplug to
> work again for me.  I test this via a "xset dpms force off".  Note that this is
> very similar to <https://bugs.freedesktop.org/show_bug.cgi?id=89453>, but it's
> back again.
> 
> I've tested this against 4.9, and it applies against the current head.
> 
> See <https://bugs.freedesktop.org/show_bug.cgi?id=99766>.
> 
> Here's the commit that triggered the regression:
> 
> commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
> Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> Date:   Wed Mar 30 18:05:23 2016 +0530
> 
>     drm/i915: Cleaning up intel_dp_hpd_pulse
> 
>     Current DP detection has DPCD operations split across
>     intel_dp_hpd_pulse and intel_dp_detect which contains
>     duplicates as well. Also intel_dp_detect is called
>     during modes enumeration as well which will result
>     in multiple dpcd operations. So this patch tries
>     to solve both these by bringing all DPCD operations
>     in one single function and make intel_dp_detect
>     use existing values instead of repeating same steps.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0b8e8eb..32ca4be 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4831,6 +4831,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		      long_hpd ? "long" : "short");
>  
>  	if (long_hpd) {
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
>  		intel_dp->detect_done = false;
>  		return IRQ_NONE;
>  	}
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix DisplayPort Hotplug

[Palmer Dabbelt]

On Tue, 14 Feb 2017 07:01:54 PST (-0800), ville.syrjala@linux.intel.com wrote:
> On Fri, Feb 10, 2017 at 02:44:20PM -0800, Palmer Dabbelt wrote:
>> DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
>> attached to an ASUS PB287Q).  I believe the problem is that long pulses
>> are no longer triggering intel_dp_check_link_status.
>
> That shouldn't itsefl cause problems with hotplugs. It could cause
> problems if you hotplug displays without a proper randr client running
> in which case the link is left running when you unplug the displays and
> then gets retrained when you plug a display back in.

Maybe it's a problem with my setup, as I don't think I have any randr client
running: I just run xrandr via my xinitrc and via some scripts linked from udev
for DRM hotplug events for when I move the laptop around.  Is it required that
I have more stuff running in order to make DPMS events work?  IIRC my setup has
looked like this for years now, but if it's broken because it's wacky then I
can change it.

Regardless, I think something is still broken here.  I don't need to actaully
unplug anything for this to break, if I just run "xset dpms force off" to turn
the screen off and then wake it back up my eDP laptop display comes back fine
but my DP monitor doesn't.  I'd expect eDP and DP to behave the same here.
IIRC the link doesn't come back up even if I twiddle some xrandr stuff, but I
might getting this confused with the previous similar bug.  If you want I can
boot back into the broken kernel and run some experiments, but it'd be great if
you could suggest some for me.

> That doesn't explain why intel_dp_check_link_status() wouldn't get
> called though. It should be called via intel_dp_detect() ->
> intel_dp_long_pulse().

Sorry, I don't actually know anything about DRM so I'm not sure what's going
on.  I just generated this patch by looking at the bug from last time my DP
monitor stopped working, looking at the last few things that touched that
function, and then adding back in the thing that looks like it would make link
detection work again :).

The last time I did this I got it wrong as well -- looking at
<https://bugs.freedesktop.org/show_bug.cgi?id=89453> I see that I suggested

  diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
  index d714a4b5711e..274bd293d9e9 100644
  --- a/drivers/gpu/drm/i915/intel_dp.c
  +++ b/drivers/gpu/drm/i915/intel_dp.c
  @@ -4625,7 +4625,9 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = {
   void
   intel_dp_hot_plug(struct intel_encoder *intel_encoder)
   {
  -	return;
  +       struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
  +
  +       intel_dp_check_link_status(intel_dp);
   }

 enum irqreturn

but the correct solution (d14e7b6d1 "drm/i915: Check DP link status on long hpd
too") was very different

  diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
  index 3781cd3..94686cb 100644
  --- a/drivers/gpu/drm/i915/intel_dp.c
  +++ b/drivers/gpu/drm/i915/intel_dp.c
  @@ -4961,9 +4961,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)

                  intel_dp_probe_oui(intel_dp);

  -               if (!intel_dp_probe_mst(intel_dp))
  +               if (!intel_dp_probe_mst(intel_dp)) {
  +                       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 mst_fail;
  -
  +               }

my fix for this new bug (which might just be another workaround) was copied
from the correct fix from last time.

>
>> I bisected the
>> problem down to (7d23e3c37 "drm/i915: Cleaning up intel_dp_hpd_pulse")
>> and it appears the only material change there was to remove one of those
>> calls.
>>
>> This commit adds the check_link_status call back in, which causes hotplug to
>> work again for me.  I test this via a "xset dpms force off".  Note that this is
>> very similar to <https://bugs.freedesktop.org/show_bug.cgi?id=89453>, but it's
>> back again.
>>
>> I've tested this against 4.9, and it applies against the current head.
>>
>> See <https://bugs.freedesktop.org/show_bug.cgi?id=99766>.
>>
>> Here's the commit that triggered the regression:
>>
>> commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
>> Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> Date:   Wed Mar 30 18:05:23 2016 +0530
>>
>>     drm/i915: Cleaning up intel_dp_hpd_pulse
>>
>>     Current DP detection has DPCD operations split across
>>     intel_dp_hpd_pulse and intel_dp_detect which contains
>>     duplicates as well. Also intel_dp_detect is called
>>     during modes enumeration as well which will result
>>     in multiple dpcd operations. So this patch tries
>>     to solve both these by bringing all DPCD operations
>>     in one single function and make intel_dp_detect
>>     use existing values instead of repeating same steps.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 0b8e8eb..32ca4be 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4831,6 +4831,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>  		      long_hpd ? "long" : "short");
>>
>>  	if (long_hpd) {
>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +		intel_dp_check_link_status(intel_dp);
>> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>>  		intel_dp->detect_done = false;
>>  		return IRQ_NONE;
>>  	}
>> --
>> 2.10.2
>>
>> _______________________________________________
>> 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] drm/i915: Fix DisplayPort Hotplug

[Ville Syrjälä]

On Tue, Feb 14, 2017 at 10:48:10AM -0800, Palmer Dabbelt wrote:
> On Tue, 14 Feb 2017 07:01:54 PST (-0800), ville.syrjala@linux.intel.com wrote:
> > On Fri, Feb 10, 2017 at 02:44:20PM -0800, Palmer Dabbelt wrote:
> >> DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
> >> attached to an ASUS PB287Q).  I believe the problem is that long pulses
> >> are no longer triggering intel_dp_check_link_status.
> >
> > That shouldn't itsefl cause problems with hotplugs. It could cause
> > problems if you hotplug displays without a proper randr client running
> > in which case the link is left running when you unplug the displays and
> > then gets retrained when you plug a display back in.
> 
> Maybe it's a problem with my setup, as I don't think I have any randr client
> running: I just run xrandr via my xinitrc and via some scripts linked from udev
> for DRM hotplug events for when I move the laptop around.

That should more or less work. Well, depends on what you do in your
udev scripts. But that's pretty much what your typical xrandr clients
do, except the udev event gets first converted into a randr event by the
X server.

> Is it required that
> I have more stuff running in order to make DPMS events work?

Not sure what you mean by DPMS events. "xset dpms" just turns things
on/off, there are no events involved really.

> IIRC my setup has
> looked like this for years now, but if it's broken because it's wacky then I
> can change it.
> 
> Regardless, I think something is still broken here.  I don't need to actaully
> unplug anything for this to break, if I just run "xset dpms force off" to turn
> the screen off and then wake it back up my eDP laptop display comes back fine
> but my DP monitor doesn't.

That is definitely a bug somewhere. Can you boot your machine with
drm.debug=0xe passed to the kernel, then reproduce this dpms problem,
and then post the full dmesg. Maybe the log will show something
interesting.

> I'd expect eDP and DP to behave the same here.
> IIRC the link doesn't come back up even if I twiddle some xrandr stuff, but I
> might getting this confused with the previous similar bug.  If you want I can
> boot back into the broken kernel and run some experiments, but it'd be great if
> you could suggest some for me.
> 
> > That doesn't explain why intel_dp_check_link_status() wouldn't get
> > called though. It should be called via intel_dp_detect() ->
> > intel_dp_long_pulse().
> 
> Sorry, I don't actually know anything about DRM so I'm not sure what's going
> on.  I just generated this patch by looking at the bug from last time my DP
> monitor stopped working, looking at the last few things that touched that
> function, and then adding back in the thing that looks like it would make link
> detection work again :).

Well, it should already work. Somehting is hinky.

> 
> The last time I did this I got it wrong as well -- looking at
> <https://bugs.freedesktop.org/show_bug.cgi?id=89453> I see that I suggested
> 
>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>   index d714a4b5711e..274bd293d9e9 100644
>   --- a/drivers/gpu/drm/i915/intel_dp.c
>   +++ b/drivers/gpu/drm/i915/intel_dp.c
>   @@ -4625,7 +4625,9 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = {
>    void
>    intel_dp_hot_plug(struct intel_encoder *intel_encoder)
>    {
>   -	return;
>   +       struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
>   +
>   +       intel_dp_check_link_status(intel_dp);
>    }
> 
>  enum irqreturn
> 
> but the correct solution (d14e7b6d1 "drm/i915: Check DP link status on long hpd
> too") was very different
> 
>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>   index 3781cd3..94686cb 100644
>   --- a/drivers/gpu/drm/i915/intel_dp.c
>   +++ b/drivers/gpu/drm/i915/intel_dp.c
>   @@ -4961,9 +4961,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> 
>                   intel_dp_probe_oui(intel_dp);
> 
>   -               if (!intel_dp_probe_mst(intel_dp))
>   +               if (!intel_dp_probe_mst(intel_dp)) {
>   +                       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 mst_fail;
>   -
>   +               }
> 
> my fix for this new bug (which might just be another workaround) was copied
> from the correct fix from last time.
> 
> >
> >> I bisected the
> >> problem down to (7d23e3c37 "drm/i915: Cleaning up intel_dp_hpd_pulse")
> >> and it appears the only material change there was to remove one of those
> >> calls.
> >>
> >> This commit adds the check_link_status call back in, which causes hotplug to
> >> work again for me.  I test this via a "xset dpms force off".  Note that this is
> >> very similar to <https://bugs.freedesktop.org/show_bug.cgi?id=89453>, but it's
> >> back again.
> >>
> >> I've tested this against 4.9, and it applies against the current head.
> >>
> >> See <https://bugs.freedesktop.org/show_bug.cgi?id=99766>.
> >>
> >> Here's the commit that triggered the regression:
> >>
> >> commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
> >> Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> >> Date:   Wed Mar 30 18:05:23 2016 +0530
> >>
> >>     drm/i915: Cleaning up intel_dp_hpd_pulse
> >>
> >>     Current DP detection has DPCD operations split across
> >>     intel_dp_hpd_pulse and intel_dp_detect which contains
> >>     duplicates as well. Also intel_dp_detect is called
> >>     during modes enumeration as well which will result
> >>     in multiple dpcd operations. So this patch tries
> >>     to solve both these by bringing all DPCD operations
> >>     in one single function and make intel_dp_detect
> >>     use existing values instead of repeating same steps.
> >>
> >> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 0b8e8eb..32ca4be 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -4831,6 +4831,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >>  		      long_hpd ? "long" : "short");
> >>
> >>  	if (long_hpd) {
> >> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >> +		intel_dp_check_link_status(intel_dp);
> >> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >> +
> >>  		intel_dp->detect_done = false;
> >>  		return IRQ_NONE;
> >>  	}
> >> --
> >> 2.10.2
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix DisplayPort Hotplug

[Palmer Dabbelt]

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

On Tue, Feb 14, 2017 at 11:00 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 14, 2017 at 10:48:10AM -0800, Palmer Dabbelt wrote:
>> On Tue, 14 Feb 2017 07:01:54 PST (-0800), ville.syrjala@linux.intel.com wrote:
>> > On Fri, Feb 10, 2017 at 02:44:20PM -0800, Palmer Dabbelt wrote:
>> >> DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
>> >> attached to an ASUS PB287Q).  I believe the problem is that long pulses
>> >> are no longer triggering intel_dp_check_link_status.
>> >
>> > That shouldn't itsefl cause problems with hotplugs. It could cause
>> > problems if you hotplug displays without a proper randr client running
>> > in which case the link is left running when you unplug the displays and
>> > then gets retrained when you plug a display back in.
>>
>> Maybe it's a problem with my setup, as I don't think I have any randr client
>> running: I just run xrandr via my xinitrc and via some scripts linked from udev
>> for DRM hotplug events for when I move the laptop around.
>
> That should more or less work. Well, depends on what you do in your
> udev scripts. But that's pretty much what your typical xrandr clients
> do, except the udev event gets first converted into a randr event by the
> X server.
>
>> Is it required that
>> I have more stuff running in order to make DPMS events work?
>
> Not sure what you mean by DPMS events. "xset dpms" just turns things
> on/off, there are no events involved really.

I guess my "dpms event" I meant "the monitor going on and off".

>> IIRC my setup has
>> looked like this for years now, but if it's broken because it's wacky then I
>> can change it.
>>
>> Regardless, I think something is still broken here.  I don't need to actaully
>> unplug anything for this to break, if I just run "xset dpms force off" to turn
>> the screen off and then wake it back up my eDP laptop display comes back fine
>> but my DP monitor doesn't.
>
> That is definitely a bug somewhere. Can you boot your machine with
> drm.debug=0xe passed to the kernel, then reproduce this dpms problem,
> and then post the full dmesg. Maybe the log will show something
> interesting.

Attached.  There's two copies: one from after I boot and start X, and
the second from after I do a DPMS cycle:

  $ xset dpms force off
  $ sleep 10s # wait for the monitor to go to sleep
  # press ctrl to wake the monitor back up

One item of interest: when I ran this on my work monitor (a newer Dell
DisplayPort monitor) I didn't have any DPMS problems, but when I ran
it on my ASUS PB287Q the monitor doesn't come back from sleep.

>> I'd expect eDP and DP to behave the same here.
>> IIRC the link doesn't come back up even if I twiddle some xrandr stuff, but I
>> might getting this confused with the previous similar bug.  If you want I can
>> boot back into the broken kernel and run some experiments, but it'd be great if
>> you could suggest some for me.
>>
>> > That doesn't explain why intel_dp_check_link_status() wouldn't get
>> > called though. It should be called via intel_dp_detect() ->
>> > intel_dp_long_pulse().
>>
>> Sorry, I don't actually know anything about DRM so I'm not sure what's going
>> on.  I just generated this patch by looking at the bug from last time my DP
>> monitor stopped working, looking at the last few things that touched that
>> function, and then adding back in the thing that looks like it would make link
>> detection work again :).
>
> Well, it should already work. Somehting is hinky.
>
>>
>> The last time I did this I got it wrong as well -- looking at
>> <https://bugs.freedesktop.org/show_bug.cgi?id=89453> I see that I suggested
>>
>>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>   index d714a4b5711e..274bd293d9e9 100644
>>   --- a/drivers/gpu/drm/i915/intel_dp.c
>>   +++ b/drivers/gpu/drm/i915/intel_dp.c
>>   @@ -4625,7 +4625,9 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = {
>>    void
>>    intel_dp_hot_plug(struct intel_encoder *intel_encoder)
>>    {
>>   -   return;
>>   +       struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
>>   +
>>   +       intel_dp_check_link_status(intel_dp);
>>    }
>>
>>  enum irqreturn
>>
>> but the correct solution (d14e7b6d1 "drm/i915: Check DP link status on long hpd
>> too") was very different
>>
>>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>   index 3781cd3..94686cb 100644
>>   --- a/drivers/gpu/drm/i915/intel_dp.c
>>   +++ b/drivers/gpu/drm/i915/intel_dp.c
>>   @@ -4961,9 +4961,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>
>>                   intel_dp_probe_oui(intel_dp);
>>
>>   -               if (!intel_dp_probe_mst(intel_dp))
>>   +               if (!intel_dp_probe_mst(intel_dp)) {
>>   +                       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 mst_fail;
>>   -
>>   +               }
>>
>> my fix for this new bug (which might just be another workaround) was copied
>> from the correct fix from last time.
>>
>> >
>> >> I bisected the
>> >> problem down to (7d23e3c37 "drm/i915: Cleaning up intel_dp_hpd_pulse")
>> >> and it appears the only material change there was to remove one of those
>> >> calls.
>> >>
>> >> This commit adds the check_link_status call back in, which causes hotplug to
>> >> work again for me.  I test this via a "xset dpms force off".  Note that this is
>> >> very similar to <https://bugs.freedesktop.org/show_bug.cgi?id=89453>, but it's
>> >> back again.
>> >>
>> >> I've tested this against 4.9, and it applies against the current head.
>> >>
>> >> See <https://bugs.freedesktop.org/show_bug.cgi?id=99766>.
>> >>
>> >> Here's the commit that triggered the regression:
>> >>
>> >> commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
>> >> Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> >> Date:   Wed Mar 30 18:05:23 2016 +0530
>> >>
>> >>     drm/i915: Cleaning up intel_dp_hpd_pulse
>> >>
>> >>     Current DP detection has DPCD operations split across
>> >>     intel_dp_hpd_pulse and intel_dp_detect which contains
>> >>     duplicates as well. Also intel_dp_detect is called
>> >>     during modes enumeration as well which will result
>> >>     in multiple dpcd operations. So this patch tries
>> >>     to solve both these by bringing all DPCD operations
>> >>     in one single function and make intel_dp_detect
>> >>     use existing values instead of repeating same steps.
>> >>
>> >> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> >> index 0b8e8eb..32ca4be 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> @@ -4831,6 +4831,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>> >>                  long_hpd ? "long" : "short");
>> >>
>> >>    if (long_hpd) {
>> >> +          drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> >> +          intel_dp_check_link_status(intel_dp);
>> >> +          drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> >> +
>> >>            intel_dp->detect_done = false;
>> >>            return IRQ_NONE;
>> >>    }
>> >> --
>> >> 2.10.2
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC

[-- Attachment #2: dmesg-001-boot.gz --]
[-- Type: application/x-gzip, Size: 37508 bytes --]

[-- Attachment #3: dmesg-002-xset_dpms_force_off.gz --]
[-- Type: application/x-gzip, Size: 42443 bytes --]

[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/i915: Fix DisplayPort Hotplug

[Ville Syrjälä]

On Wed, Feb 15, 2017 at 06:58:13PM -0800, Palmer Dabbelt wrote:
> On Tue, Feb 14, 2017 at 11:00 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Feb 14, 2017 at 10:48:10AM -0800, Palmer Dabbelt wrote:
> >> On Tue, 14 Feb 2017 07:01:54 PST (-0800), ville.syrjala@linux.intel.com wrote:
> >> > On Fri, Feb 10, 2017 at 02:44:20PM -0800, Palmer Dabbelt wrote:
> >> >> DisplayPort no longer hotplugs on my machine (a 2014 MacBook Pro
> >> >> attached to an ASUS PB287Q).  I believe the problem is that long pulses
> >> >> are no longer triggering intel_dp_check_link_status.
> >> >
> >> > That shouldn't itsefl cause problems with hotplugs. It could cause
> >> > problems if you hotplug displays without a proper randr client running
> >> > in which case the link is left running when you unplug the displays and
> >> > then gets retrained when you plug a display back in.
> >>
> >> Maybe it's a problem with my setup, as I don't think I have any randr client
> >> running: I just run xrandr via my xinitrc and via some scripts linked from udev
> >> for DRM hotplug events for when I move the laptop around.
> >
> > That should more or less work. Well, depends on what you do in your
> > udev scripts. But that's pretty much what your typical xrandr clients
> > do, except the udev event gets first converted into a randr event by the
> > X server.
> >
> >> Is it required that
> >> I have more stuff running in order to make DPMS events work?
> >
> > Not sure what you mean by DPMS events. "xset dpms" just turns things
> > on/off, there are no events involved really.
> 
> I guess my "dpms event" I meant "the monitor going on and off".
> 
> >> IIRC my setup has
> >> looked like this for years now, but if it's broken because it's wacky then I
> >> can change it.
> >>
> >> Regardless, I think something is still broken here.  I don't need to actaully
> >> unplug anything for this to break, if I just run "xset dpms force off" to turn
> >> the screen off and then wake it back up my eDP laptop display comes back fine
> >> but my DP monitor doesn't.
> >
> > That is definitely a bug somewhere. Can you boot your machine with
> > drm.debug=0xe passed to the kernel, then reproduce this dpms problem,
> > and then post the full dmesg. Maybe the log will show something
> > interesting.
> 
> Attached.  There's two copies: one from after I boot and start X, and
> the second from after I do a DPMS cycle:
> 
>   $ xset dpms force off
>   $ sleep 10s # wait for the monitor to go to sleep
>   # press ctrl to wake the monitor back up
> 
> One item of interest: when I ran this on my work monitor (a newer Dell
> DisplayPort monitor) I didn't have any DPMS problems, but when I ran
> it on my ASUS PB287Q the monitor doesn't come back from sleep.

[   94.812227] [drm:intel_enable_pipe] enabling pipe B
[   94.812237] [drm:intel_audio_codec_enable] ELD on [CONNECTOR:45:DP-1], [ENCODER:44:DDI B]
[   94.812240] [drm:hsw_audio_codec_enable] Enable audio codec on pipe B, 36 bytes ELD
[   94.856356] [drm:verify_connector_state] [CONNECTOR:45:DP-1]
[   94.856365] [drm:intel_atomic_commit_tail] [CRTC:30:pipe B]
[   94.856377] [drm:verify_single_dpll_state.isra.113] LCPLL 2700
[   95.343061] [drm:intel_get_hpd_pins] hotplug event received, stat 0x00200000, dig 0x00101012, pins 0x00000020
[   95.343067] [drm:intel_hpd_irq_handler] digital hpd port B - long
[   95.343069] [drm:intel_hpd_irq_handler] Received HPD interrupt on PIN 5 - cnt: 0
[   95.343102] [drm:intel_dp_hpd_pulse] got hpd irq on port B - long
[   95.343108] [drm:i915_hotplug_work_func] running encoder hotplug functions
[   95.343112] [drm:i915_hotplug_work_func] Connector DP-1 (pin 5) received hotplug event.
[   95.343114] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
[   95.343130] [drm:i915_hotplug_work_func] [CONNECTOR:45:DP-1] status updated from connected to disconnected

OK so here turned on the pipe, and soon after the sink decided
to drop HPD for some reason.

[   96.545016] [drm:intel_get_hpd_pins] hotplug event received, stat 0x00200000, dig 0x00101012, pins 0x00000020
[   96.545023] [drm:intel_hpd_irq_handler] digital hpd port B - long
[   96.545026] [drm:intel_hpd_irq_handler] Received HPD interrupt on PIN 5 - cnt: 0
[   96.545059] [drm:intel_dp_hpd_pulse] got hpd irq on port B - long
[   96.545067] [drm:i915_hotplug_work_func] running encoder hotplug functions
[   96.545070] [drm:i915_hotplug_work_func] Connector DP-1 (pin 5) received hotplug event.
[   96.545076] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
[   96.546859] [drm:intel_dp_read_dpcd] DPCD: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 01
[   96.547619] [drm:intel_dp_detect] Display Port TPS3 support: source yes, sink yes
[   96.547622] [drm:intel_dp_print_rates] source rates: 162000, 270000, 540000
[   96.547624] [drm:intel_dp_print_rates] sink rates: 162000, 270000, 540000
[   96.547625] [drm:intel_dp_print_rates] common rates: 162000, 270000, 540000
[   96.549257] [drm:intel_dp_detect] Sink is not MST capable
[   96.552242] [drm:drm_dp_i2c_do_msg] native defer
[   96.553547] [drm:drm_dp_i2c_do_msg] native defer
[   96.554873] [drm:drm_dp_i2c_do_msg] native defer
[   96.556255] [drm:drm_dp_i2c_do_msg] native defer
[   96.557561] [drm:drm_dp_i2c_do_msg] native defer
[   96.558885] [drm:drm_dp_i2c_do_msg] native defer
[   96.560269] [drm:drm_dp_i2c_do_msg] native defer
[   96.561574] [drm:drm_dp_i2c_do_msg] native defer
[   96.564260] [drm:drm_dp_i2c_do_msg] native defer
[   96.565580] [drm:drm_dp_i2c_do_msg] native defer
[   96.566931] [drm:drm_dp_i2c_do_msg] native defer
[   96.568309] [drm:drm_dp_i2c_do_msg] native defer
[   96.569617] [drm:drm_dp_i2c_do_msg] native defer
[   96.570941] [drm:drm_dp_i2c_do_msg] native defer
[   96.572321] [drm:drm_dp_i2c_do_msg] native defer
[   96.573629] [drm:drm_dp_i2c_do_msg] native defer
[   96.574943] [drm:drm_detect_monitor_audio] Monitor has basic audio support
[   96.575694] [drm:i915_hotplug_work_func] [CONNECTOR:45:DP-1] status updated from disconnected to connected

And soon after it decided to reassert HPD again. I have no idea why
it's doing that. In theory your userspace scripts should have noticed
this and turned things off as soon as the HPD dropped, and then
reenabled things again. Not sure why that didn't happend, but in any
case we should still try to make sure you get a picture on your
screen even without any udev/xrandr magic. So what we need to do is
ignore the previous connector status when determining if we should
retrain the link. Patch is on the way...

I do wonder slighlty how this HPD ping pong would go over with userspace
that would actually react to the HPD dips. Would we end up in some
infinite on<->off loop or something? Hard to say.

PS. please don't drop people/ml from cc. This can be important information
    in case someone later has to revisit this code, so we want to have it
    all on record somewhere. I bounced your mail to the list now.

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

[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

[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

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

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

[Palmer Dabbelt]

[-- 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);

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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: [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
_______________________________________________
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

[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

[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: [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
_______________________________________________
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

[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: [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
_______________________________________________
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: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: [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
_______________________________________________
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

[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: [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 --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
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 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

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

[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

[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

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

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: [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

[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

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

[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

[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

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

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

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

✓ Fi.CI.BAT: success for drm/i915: Fix DisplayPort Hotplug (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix DisplayPort Hotplug (rev2)
URL   : https://patchwork.freedesktop.org/series/19601/
State : success

== Summary ==

Series 19601v2 drm/i915: Fix DisplayPort Hotplug
https://patchwork.freedesktop.org/api/1.0/series/19601/revisions/2/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

b29710e319e4b58d994065f5e776f6f66674d7c9 drm-tip: 2017y-02m-16d-15h-35m-40s UTC integration manifest
e8365fc drm/i915: Perform link quality check unconditionally during long pulse

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3861/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Fix DisplayPort Hotplug (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix DisplayPort Hotplug (rev3)
URL   : https://patchwork.freedesktop.org/series/19601/
State : success

== Summary ==

Series 19601v3 drm/i915: Fix DisplayPort Hotplug
https://patchwork.freedesktop.org/api/1.0/series/19601/revisions/3/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 455s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 572s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 539s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 588s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 503s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 497s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 442s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 517s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 488s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 484s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 506s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 600s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 499s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 545s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 549s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 412s

7748ac05e8cd90e45f039d434ac4e600e54fe6e9 drm-tip: 2017y-03m-13d-20h-07m-51s UTC integration manifest
6753232 drm/i915: Perform link quality check unconditionally during long pulse

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4158/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Fix DisplayPort Hotplug (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix DisplayPort Hotplug (rev4)
URL   : https://patchwork.freedesktop.org/series/19601/
State : success

== Summary ==

Series 19601v4 drm/i915: Fix DisplayPort Hotplug
https://patchwork.freedesktop.org/api/1.0/series/19601/revisions/4/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:431s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:423s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:584s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:505s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:558s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:479s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:409s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:430s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:491s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:456s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:571s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:465s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:503s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:399s

57188aaf3b96d08e4095755fda0e577b1e0d8c60 drm-tip: 2017y-04m-12d-16h-12m-10s UTC integration manifest
28b6530 drm/i915: Perform link quality check unconditionally during long pulse

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4498/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx