[Intel-gfx] [PATCH] drm/i915: Fix DisplayPort Hotplug
Palmer Dabbelt
palmer at dabbelt.com
Thu Feb 16 02:58:13 UTC 2017
On Tue, Feb 14, 2017 at 11:00 AM, Ville Syrjälä
<ville.syrjala at 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 at 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 at 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 at 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 at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmesg-001-boot.gz
Type: application/x-gzip
Size: 37508 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20170215/2bb4e22f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmesg-002-xset_dpms_force_off.gz
Type: application/x-gzip
Size: 42443 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20170215/2bb4e22f/attachment-0003.bin>
More information about the Intel-gfx
mailing list