GNOME Bugzilla – Bug 738630
Fixes and optimizations for monitor-manager
Last modified: 2014-12-29 06:39:20 UTC
Commit 1dbda68839a6c7a2746271ae308528ff8f86a889 introduced a bug where we were not triggering monitors-changed when turning off an output. While investigating, I came up with this handful of patches, please review. Note that the optimization would be valid even before the above mentioned commit introduced this bug since meta_monitor_config_match_current() is basically useless for the purpose that was intended here.
Created attachment 288680 [details] [review] monitor-manager: Remove needless code Nothing uses this. Signal handlers have access to the new monitor infos built by make_logical_config() .
Created attachment 288681 [details] [review] monitor-manager-xrandr: Re-work xrandr event handling In randr events, configTimestamp can be considerd the hotplug time, i.e. whenever the server notices hardware changes, this value will be updated. meta_monitor_config_match_current() only matches the number of outputs and if the output connector, vendor, product and serial match. This means that we can't use it to bypass doing any work here because it won't detect cases where we actually want to update ourselves like e.g. an output being turned off either by us or by another X client (e.g. xrandr). With all that in mind, we can simplify the code here and make it clearer.
Created attachment 288682 [details] [review] monitor-manager-kms: Don't try to match the outputs on hotplug meta_monitor_config_match_current() only matches the number of outputs and if the output connector, vendor, product and serial match. As such, this isn't needed because, unlike the xrandr backend, we only get called on real hotplug events and thus should always trigger the common hotplug code to (possibly) apply a new mode.
Created attachment 288683 [details] [review] monitor-manager-xrandr: Use CurrentTime when applying configurations This is what the xrandr CLI tool does and will allow us to do less work when we get RRScreenChangeNotify events.
Created attachment 288684 [details] [review] monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify The X server sends several RRScreenChangeNotify events in a burst when something happens which, currently, causes us to rebuild our view of the world as many times and notify the upper layers about it which causes a lot of bogus repeated work like rebuilding background actors. We can avoid this extra work by looking at the timestamp in the XRRScreenResources struct which is updated when an X client (including us!) last changed something and comparing it with the previous timestamp.
Created attachment 288685 [details] [review] monitor-config: Remove unused meta_monitor_config_match_current() This is no longer needed.
Review of attachment 288680 [details] [review]: OK.
Review of attachment 288681 [details] [review]: "considerd" Can you leave the match_current removal for a separate patch, and then squash in the native removal / function removal?
Review of attachment 288685 [details] [review]: To be squashed.
Review of attachment 288682 [details] [review]: To be squashed.
Review of attachment 288683 [details] [review]: OK.
Review of attachment 288684 [details] [review]: ::: src/backends/x11/meta-monitor-manager-xrandr.c @@ +1070,3 @@ + old_timestamp = manager_xrandr->resources->timestamp; + else + old_timestamp = 0; Are you sure about this one? 0 is a valid timestamp value, so it could theoretically show up in manager_xrandr->resources->timestamp. If you just want to rely on the timestamp being virtually always non-zero on startup, which is very close to where this method would first be called, then write a comment.
(In reply to comment #12) > Review of attachment 288684 [details] [review]: > > ::: src/backends/x11/meta-monitor-manager-xrandr.c > @@ +1070,3 @@ > + old_timestamp = manager_xrandr->resources->timestamp; > + else > + old_timestamp = 0; > > Are you sure about this one? 0 is a valid timestamp value, so it could > theoretically show up in manager_xrandr->resources->timestamp. > > If you just want to rely on the timestamp being virtually always non-zero on > startup, which is very close to where this method would first be called, then > write a comment. This is just to handle the theoretical case of manager_xrandr->resources being NULL and not crash. I know 0 is valid but I don't think it's a value that the server will ever send you in this struct and I used it because it will always compare less than whatever the server sends and thus we won't miss updating our view of the outputs in that case. I'll add a comment. Anyway ->resources won't ever be NULL in practice since we _read_current() in _contstructed()
(In reply to comment #13) > Anyway ->resources won't ever be NULL in practice since we _read_current() in > _contstructed() Then just remove the if test. If it crashes, we have more serious issues to worry about.
Created attachment 288705 [details] [review] monitor-manager-xrandr: Re-work xrandr event handling In randr events, configTimestamp can be considered the hotplug time, i.e. whenever the server notices hardware changes, this value will be updated. Having that in mind, we can re-work the logic to make it clearer. There are no semantic changes.
Created attachment 288706 [details] [review] monitor-manager: Don't try to match the outputs on hotplug meta_monitor_config_match_current() only matches the number of outputs and if the output connector, vendor, product and serial match. In the X backend, this means that we can't use it to bypass doing any work because it won't detect cases where we actually want to update ourselves like e.g. an output being turned off either by us or by another X client (e.g. xrandr). In the native backend, unlike the xrandr backend, we only get called on real hotplug events and thus should always trigger the common hotplug code to (possibly) apply a new mode so the check is pointless anyway.
Created attachment 288707 [details] [review] monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify -- removed the if
Review of attachment 288705 [details] [review]: ::: src/backends/x11/meta-monitor-manager-xrandr.c @@ +1080,2 @@ return TRUE; } if (hotplug) { /* This is a hotplug event, so go ahead and build a new configuration. */ meta_monitor_manager_on_hotplug (manager); } else { /* Something else changed -- tell the world about it. */ meta_monitor_manager_rebuild_derived (manager); }
Review of attachment 288706 [details] [review]: Perfect.
Review of attachment 288707 [details] [review]: Yep.
Pushed with the suggested change. Thanks Attachment 288680 [details] pushed as e1704ac - monitor-manager: Remove needless code Attachment 288683 [details] pushed as 016b8f5 - monitor-manager-xrandr: Use CurrentTime when applying configurations Attachment 288706 [details] pushed as b3821c4 - monitor-manager: Don't try to match the outputs on hotplug Attachment 288707 [details] pushed as 47e339b - monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify
Created attachment 292473 [details] [review] Revert "monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify" This reverts commit 47e339b46e80f01c2a4d020dbef7341e5a02b7eb. The approach that was used to reduce the amount of work we do on RR events to the necessary minimum is flawed. It assumes that, when the first event we see where the retrieved XRRScreenResources.timestamp is bigger than the previous, we already have all the data we need to rebuild our view of the world. That isn't true however, because the X server sends RRScreenChangeNotify events for every step of the configuration change, i.e. it lacks an atomic reconfiguration API. In particular, if the X screen size is one of the changes, when we rebuild our state and emit monitors-changed, the X screen size might still be the previous one and since we stop updating ourselves until another reconfiguration happens (noticed by looking at XRRScreenResources.timestamp) we end up with the wrong idea of the X screen size. -- This came up while fixing bug 740838 .
Created attachment 292474 [details] [review] monitor-manager-xrandr: Delay acting on RRScreenChangeNotify events The X server sends us several RRScreenChangeNotify events for every internal configuration change but we're only interested in the end result so, instead of mindlessly reacting to every event, let's give it a bit of time to settle and then act when we're likely to get the final configuration from XRRGetScreenResourcesCurrent() , which is btw, a blocking round trip. For slower hardware we might still do more than the necessary minimum amount of work but we should end up with the most up-to-date view of the world possible.
I investigated using serials to track the correct event that we want to process but I don't think it can be made to work reliably. Here's why: In _apply() we call several XRRSet* functions but not all of them will generate events and I can't think of a good way to find out if they will and save their serial. Take for instance the case in bug 740838, we're calling XRRSetCrtcConfig() with a configuration that doesn't change anything so the server doesn't send us an event for it. We could re-introduce the code that I deleted for that bug to decide whether to save the serial but I'm not sure that the checks that the server does are exactly the same that that code was doing. Also, the server conditions might change in the future and we could miss an event. The other problem with using serials is that if the configuration is changed by another client (e.g. xrandr or a game, etc.) then we have to process all events anyway since we don't have a serial to look for. So, my opinion at this point is to take the revert patch. And then take the timeout patch if we think the efficiency gain is worth it, otherwise we can just leave it as it always was and process all events.
Comment on attachment 292473 [details] [review] Revert "monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify" Attachment 292473 [details] pushed as 68542ae - Revert "monitor-manager-xrandr: Don't do extra work on RRScreenChangeNotify" OK, let's go with this for now. Man, what a horribly designed extension.