Bug 91929 - Missing RandR events on hotplug
Summary: Missing RandR events on hotplug
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/intel (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Chris Wilson
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-09 03:43 UTC by Loïc Yhuel
Modified: 2015-10-01 09:03 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
/var/log/Xorg.0.log (31.60 KB, text/plain)
2015-09-09 10:00 UTC, Loïc Yhuel
no flags Details

Description Loïc Yhuel 2015-09-09 03:43:28 UTC
When investigating https://bugs.kde.org/show_bug.cgi?id=346455, I found I don't get all XRandR events on hotplug until someones calls RRGetInfo (either "randr", or XRRGetScreenResources in "xev -event randr"), or a VT switch.

By default when plugging the HDMI cable I only get :
 - RRScreenChangeNotify
 - XRROutputChangeNotifyEvent with RR_Disconnected
When using xev, or calling xrandr, I get :
 - a second RRScreenChangeNotify
 - XRROutputChangeNotifyEvent with RR_Connected

On unplug, it seems to be the same (not 100% reliable, sometimes the kernel doesn't generate any uevent) : I get "dummy" events (RRScreenChangeNotify + XRROutputChangeNotifyEvent with RR_Connected), and won't get the RR_Disconnected until someones calls RRGetInfo.
Comment 1 Chris Wilson 2015-09-09 07:31:03 UTC
Is your ddx compiled with udev support? Please always attach your Xorg.0.log.
Comment 2 Loïc Yhuel 2015-09-09 10:00:20 UTC
Created attachment 118164 [details]
/var/log/Xorg.0.log
Comment 3 Loïc Yhuel 2015-09-09 10:03:24 UTC
udev in enabled in the DDX, else I wouldn't get the first two events (unless someone was calling XRRGetScreenResources / XRRGetScreenInfo in a loop, which isn't the case).
Comment 4 Chris Wilson 2015-09-09 10:29:39 UTC
Oh completely misread what you said. I thought you meant you didn't see any events at all until xrandr (which is a side-effect of not having udev).

When we see a hotplug uevent, we iterate over all connectors looking for new outputs and comparing the connector->status on existing outputs, if the kernel reports that the output->status is different we send a RROutputChanged(output->randr_output, TRUE); and committed with RRTellChanged(screen).

Hmm, what we seem to be missing is setting the RROutputSetConnection().

commit f5aabb7bddc6fc5dc910a983d1291c9864f65f06
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Sep 9 11:18:15 2015 +0100

    sna: On hotplug events, update the output->status
    
    During the hotplug event, we query the current connector status and use
    that to trigger an output changed event to the clients. However, since
    we know the new status, we can set that on the RROutput immediately.
    Note the modelist is left unchanged, and will only be queried when the
    user requests it (though we may want to provide that in the hotplug
    notify as well).
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=91929#c2
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

should fix the fix issue, but you will still get the RRScreenChangeNotify following xrandr due to the full probe.
Comment 5 Loïc Yhuel 2015-09-09 18:14:18 UTC
The issue is fixed.

Sadly it generates many crashes in kscreen (even sometimes Qt).
Hotplug generates events, then kscreen changes the configuration which generates other events. So any change in the DDX or kscreen can result in new event sequences which weren't tested...

I'm a little surprised to see events like :
RRNotify event, serial 25, synthetic NO, window 0xd5,
    subtype XRRCrtcChangeNotifyEvent
    crtc 63, mode None, rotation RR_Rotate_0
    x 0, y 0, width 0, height 0
but it could be the result of :
[888172.618] (EE) intel(0): sna_mode_check: invalid state found on pipe 0, disabling CRTC:20
[888172.903] (EE) intel(0): sna_mode_check: invalid state found on pipe 1, disabling CRTC:24
which probably means kscreen did set an invalid configuration.
Comment 6 Chris Wilson 2015-09-10 08:15:23 UTC
Having KDE crash randomly is not fun either. Looking at mutter (at least, digging around in kwin is a bit trickier), it never seems to do a full reprobe by itself. So if the only source of the updated information is the hotplug uevent and sna_mode_discover(), we need to add the available modes there as well.
Comment 7 Loïc Yhuel 2015-09-10 08:52:04 UTC
Kscreen doesn't run in kwin, but in kded5 (decides which config to apply), and kscreen_backend_launcher (XRandR calls). These two parts communicate over DBus.

So you have XRandR events from hotplug, which generate DBus signals from the backend to the daemon, which calls the backend, which generates other events...


It doesn't use xcb_randr_get_screen_info (only for XRandR 1.1 backend).

It uses xcb_randr_get_screen_resources, but only once. After it uses xcb_randr_get_screen_resources_current (see https://quickgit.kde.org/?p=libkscreen.git&a=blob&h=fae1dcc971d5d3a16e95bfa745d6822c69fa2a55&hb=effd39e2ebe767868660b52a8758873e45e4c656&f=backends%2Fxrandr%2Fxrandr.cpp).
So it doesn't do a full reprobe (RRGetInfo).


On XRROutputChangeNotifyEvent with RR_Connected, it will call XRandROutput::update -> XRandROutput::init -> XRandROutput::updateModes, which uses xcb_randr_get_screen_resources_current.
Comment 8 Mark Kettenis 2015-09-26 12:18:11 UTC
Unfortunately commit 2c08d72393e4c8ddf5926571b087459aaa225cb1 that was made to resolve this bug introduced infinite recursion if polling udev for events fails (which is always the case on systems without udev).  The scenario is as follows:

1. The server calls RRGetInfo().
2. RRGetInfo() calls sna_randr_getinfo()
3. sna_randr_getinfo() calls sna_uevent_poll(), which returns false
4. sna_randr_getinfo() calls sna_mode_discover()
5. sna_mode_discover() calls RRGetInfo()

and we jump straight back to step 2, until we run out of stack space and crash

It's not obvious to me how to fix this.
Comment 9 Chris Wilson 2015-09-27 18:03:42 UTC
Hmm, nasty. So,

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 1b1a125..0a59d1d 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -4900,6 +4900,7 @@ void sna_mode_discover(struct sna *sna)
                                DBG(("%s: output %s (id=%d), changed state, reprobing\n",
                                     __FUNCTION__, output->name, sna_output->id));
                                sna_output->last_detect = 0;
+                               changed |= 4;
                        }
                        continue;
                }
@@ -4918,7 +4919,8 @@ void sna_mode_discover(struct sna *sna)
                changed |= 2;
        }
 
-       if (changed) {
+       /* Have the list of available outputs been updated? */
+       if (changed & 3) {
                DBG(("%s: outputs changed, broadcasting\n", __FUNCTION__));
 
                sna_mode_set_primary(sna);
@@ -4933,7 +4935,9 @@ void sna_mode_discover(struct sna *sna)
                xf86RandR12TellChanged(screen);
        }
 
-       RRGetInfo(screen, TRUE);
+       /* If anything has changed, refresh the RandR information */
+       if (changed)
+               RRGetInfo(screen, TRUE);
        RRTellChanged(screen);
 }
Comment 10 Jonathan Gray 2015-10-01 04:57:33 UTC
It appears that commit 679ee12079a7d2682d41506b81973c7c7d4fa1d8 has not resolved the problem on non udev systems.  Commenting out the call to RRGetInfo() in sna_mode_discover() as suggested by Mark lets X start here on OpenBSD.  So it would seem there is still an infinite recursion in RRGetInfo().
Comment 11 Chris Wilson 2015-10-01 09:03:42 UTC
commit 096ddef22d6c57198a424eef00845dc7302b0cfe
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Oct 1 09:47:59 2015 +0100

    sna: Indicate when we expect to call RRGetInfo during discovery

This time with some due diligence and booting with --disable-udev.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.