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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 14, 2017 at 11:00 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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@xxxxxxxxx>
>> >> 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@xxxxxxxxxxx>
>> >> ---
>> >>  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@xxxxxxxxxxxxxxxxxxxxx
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC

Attachment: dmesg-001-boot.gz
Description: GNU Zip compressed data

Attachment: dmesg-002-xset_dpms_force_off.gz
Description: GNU Zip compressed data

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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux