GNOME Bugzilla – Bug 705670
Add a DBus API for display configuration under X
Last modified: 2013-08-17 22:57:18 UTC
This bug is about implementing the proposed DBus display configuration API in the master branch, using XRandR as the backend. Patches are above master, not wip/wayland. Wayland stuff is in bug 705513 (or wip/wayland-display really)
Created attachment 251157 [details] [review] Rework and consolidate monitor handling in MetaScreen Consolidate all places that deal with output configuration in MetaScreen, which gets it either from XRandR or from a dummy static configuration. We still need to read the Xinerama config, even when running xwayland, because we need the indices for _NET_WM_FULLSCREEN_MONITORS, but now we do it only when needed.
Created attachment 251158 [details] [review] Split monitor handling into an helper object Create a new singleton object, MetaMonitorManager, which deals with reading the XRandR configuration and in the future applying the new one. This is required because xwayland will not bind the xserver interface until he has seen the current wl_outputs, so we can't wait until MetaScreen is built to expose them.
Created attachment 251159 [details] [review] Introduce a new DBus interface for display configuration This new interface will be used by the control center and possibly the settings daemon to configure the screens. It is designed to resemble a simplified XRandR, while still exposing all the quirks of the hardware, so that the panel can limit the user choices appropriately. To do so, MetaMonitorMode needs to track CRTCs, outputs and modes, so the low level objects have been decoupled from the high-level MetaMonitorInfo, which is used by core and API and offers a simplified view of HW, that hides away the details of what is cloned and how. This is still not efficient as it should be, because on every HW change we drop all data structures and rebuild them from scratch (which is not expensive because there aren't many of them, but at least in the XRandR path it involves a few sync X calls)
Created attachment 251160 [details] [review] Add the write side of the DBus protocol too This is just in the documentation for now, to attract wider feedback before we start looking at how to implement this for real.
Created attachment 251161 [details] [review] Extend the DBus XRandR protocol to expose cloning restriction Turns out that even if two outputs say that they can be controlled by a given CRTC, you can't configure them in the same CRTC unless they are marked as "possible clones" one of the other. This can further restrict the configuration options, so we need to expose this limitation in the DBus API.
Created attachment 251162 [details] [review] DisplayConfig: add the write side of the API Implement ApplyConfiguration in terms of XRandR calls. Error checking is done before actually committing the configuration. If mutter is using one of the other monitor config backends, an error is reported and nothing happens.
Created attachment 251163 [details] [review] Reverse handling of XRandR events between Screen and MonitorManager Now MonitorManager does its own handling of XRandR events, which means we no longer handle ConfigureNotify on the root window. MetaScreen reacts to MonitorManager::monitor-changed and updates its internal state, including the new size. This paves the way for doing display configuration using only the dummy backend, which would allow testing wl_output interfaces.
Created attachment 251164 [details] [review] DisplayConfig: make the dummy backend writable Add a number of dummy outputs and modes to the dummy backend, and implement the writing bits. The only visible effect is that you can change the screen size, which resizes the output window.
Created attachment 251165 [details] [review] default plugin: add a random color background on each monitor Instead of a full white background, make one with a random color. This way the different "monitors" are visible and it's easier to debug the DBus API.
Created attachment 251166 [details] [review] MonitorManager: fix handling of output transform Read the current transform from XRandR, and expose the transforms that are really supported on the bus. The dummy backend now advertises all transforms, since it doesn't actually apply them.
Created attachment 251167 [details] [review] MonitorManager: inherit directly from DisplayConfig instead of handling signals This way we can handle properties too.
Created attachment 251168 [details] [review] MonitorManager: add support for DPMS levels To the XRandR and dummy backend (and as usual the dummy backend has no effect)
Created attachment 251169 [details] [review] MonitorManager: add support for persistent monitor configurations Add a new object, MetaMonitorConfig, that takes care of converting between the logical configurations stored in monitors.xml and the HW resources exposed by MonitorManager. This commit includes loading and saving of configurations, but still missing is the actual CRTC assignments and a default configuration when none is found in the file.
Created attachment 251170 [details] [review] MonitorConfig: add CRTC assignment Ripped off libgnome-desktop, trimming the parts that checked that the configuration was plausible, as that should be done in gnome-control-center before asking mutter for a change.
Created attachment 251171 [details] [review] MonitorConfig: add support for default configurations Activate the presentation bit on new hotplugged monitors, while making a fully extended setup when running for the first time.
Created attachment 251172 [details] [review] MonitorManager: store the presentation mode bit in XRandR Use a private output property to store if the output is in presentation mode or not, so that this information is not lost after the configuration read back from the server.
Created attachment 251173 [details] [review] MonitorManager: further extend the dummy backend The default configuration is extended, which is only possible if there are as many CRTCs as outputs, so make sure that's true. Also, add more and bigger modes, so that different sizes will be chosen for the three outputs. A nice side effect of this is that with a real 1920x1080 + 1600x900 layout, if you disable the VGA you get a stage that matches the screen size, which triggers the legacy fullscreen path in the outside mutter.
Created attachment 251174 [details] [review] MonitorManager: add support for backlight GnomeRR needs that too. The backlight is exported as a normalized 0-100 value, or -1 if not supported. Clamping to HW limits is handled by the backend. Changing backlight uses a different method call, to avoid recomputing the full display configuration every time the user presses the backlight keys.
Created attachment 251175 [details] [review] MonitorManager: ignore configuration changes that disable all outputs If we compute a screen size of 0 in either direction, we crash later on, as it is invalid for clutter, cogl and X.
Created attachment 251176 [details] [review] MonitorManager: add gamma support Add GetCrtcGamma() and SetCrtcGamma(), that wrap the similarly named XRandR API. These are used by GnomeRR inside the color plugin of the control center (and may go away if the color plugin decides to do something different under wayland)
Created attachment 251177 [details] [review] MonitorConfig: handle changes in the laptop lid This way we don't need to track the current and previous configuration in gnome-settings-daemon, when we already do so in mutter.
Created attachment 251178 [details] [review] MetaPlugin: add a UI hook for confirming display changes We want to show a dialog when a display change happens from the control center. To do so, add a new vfunc to MetaPlugin and call it when a configuration change is requested via DBus.
Created attachment 251179 [details] [review] MonitorManager: split the XRandR parts in a subclass Instead of keeping a forest of if backend else ..., use a subclass and virtual functions to discriminate between XRandR and the dummy backend (which lives in the parent class togheter with the common code)
Created attachment 251180 [details] [review] MonitorManager: add EDID properties to the output DBus description Add "edid-file", if we have one (in the KMS case, where we can point people to the right sysfs file), or "edid" with inline data. These are needed by colord to build the default ICC profile for uncalibrated displays.
*** Bug 705513 has been marked as a duplicate of this bug. ***
Review of attachment 251157 [details] [review]: ::: src/core/screen-private.h @@ +51,3 @@ + int width_mm; + int height_mm; + CoglSubpixelOrder subpixel_order; What is that one for? You only seem to set it to COGL_SUBPIXEL_ORDER_UNKNOWN but never do anything else with it. If we don't need it just remove it. @@ +64,3 @@ + + /* The primary or first output for this crtc, 0 if we can't figure out. + This is a XID when using XRandR, otherwise a KMS id (not implemented) */ Well I'd just say 0 if we can't figure out otherwise it is an implementation detail of the backend (XID, KMS id, no meaning in the dummy backend). ::: src/core/screen.c @@ +352,3 @@ +static gboolean +has_dummy_output (void) What's the point of this? It always returns FALSE so could as well not exist? (and seems like it does not in your branch). @@ +506,2 @@ + resources = XRRGetScreenResourcesCurrent (screen->display->xdisplay, screen->xroot); + if (!resources) When / why would this happen? Can we really continue with just the dummy values?
Review of attachment 251158 [details] [review]: Partial can you please reattach the current branch? ::: src/core/display.c @@ +518,3 @@ if (meta_is_syncing ()) XSynchronize (xdisplay, True); + Unrelated. ::: src/core/monitor-private.h @@ +54,3 @@ + int width_mm; + int height_mm; + CoglSubpixelOrder subpixel_order; Again not needed / used -> kill it. ::: src/core/monitor.c @@ +28,3 @@ +#include "config.h" + +#include <clutter/clutter.h> Why? @@ +225,3 @@ + */ +static gboolean +has_dummy_output (void) Hmm again? @@ +239,3 @@ +{ + if (has_dummy_output ()) + return make_dummy_monitor_config (manager); It never gets there.
Created attachment 251870 [details] [review] Rework and consolidate monitor handling in MetaScreen Consolidate all places that deal with output configuration in MetaScreen, which gets it either from XRandR or from a dummy static configuration. We still need to read the Xinerama config, even when running xwayland, because we need the indices for _NET_WM_FULLSCREEN_MONITORS, but now we do it only when needed.
Created attachment 251871 [details] [review] Split monitor handling into an helper object Create a new singleton object, MetaMonitorManager, which deals with reading the XRandR configuration and in the future applying the new one. This is required because xwayland will not bind the xserver interface until he has seen the current wl_outputs, so we can't wait until MetaScreen is built to expose them.
Created attachment 251872 [details] [review] Introduce a new DBus interface for display configuration This new interface will be used by the control center and possibly the settings daemon to configure the screens. It is designed to resemble a simplified XRandR, while still exposing all the quirks of the hardware, so that the panel can limit the user choices appropriately. To do so, MetaMonitorMode needs to track CRTCs, outputs and modes, so the low level objects have been decoupled from the high-level MetaMonitorInfo, which is used by core and API and offers a simplified view of HW, that hides away the details of what is cloned and how. This is still not efficient as it should be, because on every HW change we drop all data structures and rebuild them from scratch (which is not expensive because there aren't many of them, but at least in the XRandR path it involves a few sync X calls)
Created attachment 251873 [details] [review] Add the write side of the DBus protocol too This is just in the documentation for now, to attract wider feedback before we start looking at how to implement this for real.
Created attachment 251874 [details] [review] Extend the DBus XRandR protocol to expose cloning restriction Turns out that even if two outputs say that they can be controlled by a given CRTC, you can't configure them in the same CRTC unless they are marked as "possible clones" one of the other. This can further restrict the configuration options, so we need to expose this limitation in the DBus API.
Created attachment 251875 [details] [review] DisplayConfig: add the write side of the API Implement ApplyConfiguration in terms of XRandR calls. Error checking is done before actually committing the configuration. If mutter is using one of the other monitor config backends, an error is reported and nothing happens.
Created attachment 251876 [details] [review] Reverse handling of XRandR events between Screen and MonitorManager Now MonitorManager does its own handling of XRandR events, which means we no longer handle ConfigureNotify on the root window. MetaScreen reacts to MonitorManager::monitor-changed and updates its internal state, including the new size. This paves the way for doing display configuration using only the dummy backend, which would allow testing wl_output interfaces.
Created attachment 251877 [details] [review] DisplayConfig: make the dummy backend writable Add a number of dummy outputs and modes to the dummy backend, and implement the writing bits. The only visible effect is that you can change the screen size, which resizes the output window.
Created attachment 251878 [details] [review] default plugin: add a random color background on each monitor Instead of a full white background, make one with a random color. This way the different "monitors" are visible and it's easier to debug the DBus API.
Created attachment 251879 [details] [review] MonitorManager: fix handling of output transform Read the current transform from XRandR, and expose the transforms that are really supported on the bus. The dummy backend now advertises all transforms, since it doesn't actually apply them.
Created attachment 251880 [details] [review] MonitorManager: inherit directly from DisplayConfig instead of handling signals This way we can handle properties too.
Created attachment 251881 [details] [review] MonitorManager: add support for DPMS levels To the XRandR and dummy backend (and as usual the dummy backend has no effect)
Created attachment 251882 [details] [review] MonitorManager: add support for persistent monitor configurations Add a new object, MetaMonitorConfig, that takes care of converting between the logical configurations stored in monitors.xml and the HW resources exposed by MonitorManager. This commit includes loading and saving of configurations, but still missing is the actual CRTC assignments and a default configuration when none is found in the file.
Created attachment 251883 [details] [review] MonitorConfig: add CRTC assignment Ripped off libgnome-desktop, trimming the parts that checked that the configuration was plausible, as that should be done in gnome-control-center before asking mutter for a change.
Created attachment 251884 [details] [review] MonitorConfig: add support for default configurations Activate the presentation bit on new hotplugged monitors, while making a fully extended setup when running for the first time.
Created attachment 251885 [details] [review] MonitorManager: store the presentation mode bit in XRandR Use a private output property to store if the output is in presentation mode or not, so that this information is not lost after the configuration read back from the server.
Created attachment 251886 [details] [review] MonitorManager: further extend the dummy backend The default configuration is extended, which is only possible if there are as many CRTCs as outputs, so make sure that's true. Also, add more and bigger modes, so that different sizes will be chosen for the three outputs. A nice side effect of this is that with a real 1920x1080 + 1600x900 layout, if you disable the VGA you get a stage that matches the screen size, which triggers the legacy fullscreen path in the outside mutter.
Created attachment 251887 [details] [review] MonitorManager: add support for backlight GnomeRR needs that too. The backlight is exported as a normalized 0-100 value, or -1 if not supported. Clamping to HW limits is handled by the backend. Changing backlight uses a different method call, to avoid recomputing the full display configuration every time the user presses the backlight keys.
Created attachment 251888 [details] [review] MonitorManager: ignore configuration changes that disable all outputs If we compute a screen size of 0 in either direction, we crash later on, as it is invalid for clutter, cogl and X.
Created attachment 251889 [details] [review] MonitorManager: add gamma support Add GetCrtcGamma() and SetCrtcGamma(), that wrap the similarly named XRandR API. These are used by GnomeRR inside the color plugin of the control center (and may go away if the color plugin decides to do something different under wayland)
Created attachment 251890 [details] [review] MonitorConfig: handle changes in the laptop lid This way we don't need to track the current and previous configuration in gnome-settings-daemon, when we already do so in mutter.
Created attachment 251891 [details] [review] MetaPlugin: add a UI hook for confirming display changes We want to show a dialog when a display change happens from the control center. To do so, add a new vfunc to MetaPlugin and call it when a configuration change is requested via DBus.
Created attachment 251892 [details] [review] MonitorManager: split the XRandR parts in a subclass Instead of keeping a forest of if backend else ..., use a subclass and virtual functions to discriminate between XRandR and the dummy backend (which lives in the parent class togheter with the common code)
Created attachment 251893 [details] [review] MonitorManager: add EDID properties to the output DBus description Add "edid-file", if we have one (in the KMS case, where we can point people to the right sysfs file), or "edid" with inline data. These are needed by colord to build the default ICC profile for uncalibrated displays.
Created attachment 251894 [details] [review] MonitorXrandr: use XIDs, not API ids, in the warning More useful to debug mutter.
Created attachment 251895 [details] [review] MonitorXrandr: implement correct EDID parsing To provide valid values for the vendor, product and serial fields we need to read the EDID and parse it. Parser kindly provided by gnome-desktop.
Created attachment 251896 [details] [review] Monitor: restore correct display name handling Now that we have the right values from the EDID, we can load the PNP database and find the proper vendor name, to show in the control center UI.
Created attachment 251897 [details] [review] MonitorXrandr: resize the framebuffer prior to setting the CRTC configuration Otherwise X11 will trim the new configuration and disable outputs outside the screen.
Review of attachment 251870 [details] [review]: Same things still apply modulo the dummy thing.
(In reply to comment #26) > Review of attachment 251157 [details] [review]: > > ::: src/core/screen-private.h > @@ +51,3 @@ > + int width_mm; > + int height_mm; > + CoglSubpixelOrder subpixel_order; > > What is that one for? You only seem to set it to COGL_SUBPIXEL_ORDER_UNKNOWN > but never do anything else with it. > If we don't need it just remove it. It's needed in the wayland branch (exposed to wayland clients). I have a patch to set it sensibly in the X11 path too, I don't know how useful that would be... > ::: src/core/screen.c > @@ +352,3 @@ > > +static gboolean > +has_dummy_output (void) > > What's the point of this? It always returns FALSE so could as well not exist? > (and seems like it does not in your branch). It has more use in the wayland branch (in that case, it returns true when running as a non-kms wayland server). I left it here to make rebasing/merging easier. > @@ +506,2 @@ > + resources = XRRGetScreenResourcesCurrent (screen->display->xdisplay, > screen->xroot); > + if (!resources) > > When / why would this happen? Can we really continue with just the dummy > values? Good question, I don't know when the call can fail, but the value was checked in the previous xrandr mutter code as well as gnomerr. (In reply to comment #27) > ::: src/core/monitor.c > @@ +28,3 @@ > +#include "config.h" > + > +#include <clutter/clutter.h> > > Why? Uhm... for the cogl configuration backend? (That is later removed...) > @@ +239,3 @@ > +{ > + if (has_dummy_output ()) > + return make_dummy_monitor_config (manager); > > It never gets there. Yeah, but it reduces the diff with the wayland branch a little.
Review of attachment 251871 [details] [review]: ::: src/core/display.c @@ +518,3 @@ if (meta_is_syncing ()) XSynchronize (xdisplay, True); + Unrelated. ::: src/core/monitor.c @@ +222,3 @@ + * + * Returns TRUE if the only available monitor is the dummy one + * backing the ClutterStage window. That's not what the code does. It always returns false. If it is going to be made useful later add a comment.
Review of attachment 251872 [details] [review]: ::: src/Makefile.am @@ +335,3 @@ + --c-namespace MetaDBus \ + --generate-c-code meta-dbus-xrandr \ + xrandr.xml Not sure the "xrandr" makes sense if it is an abstraction for different backends but otoh it is based in the xrandr API ... ::: src/core/monitor.c @@ +227,3 @@ + meta_output->output_id = resources->outputs[i]; + meta_output->name = g_strdup (output->name); + meta_output->vendor = g_strdup ("unknown"); Add a comment that we don't have EDID parsing yet. @@ +514,3 @@ } +static const double known_diagonals[] = { Rationale for this? Or was this like that in gnome-rr as well? @@ +702,1 @@ static MetaMonitorManager *global_manager; global_monitor_manager? Don't know why but "global_manager" sounds like "manages globals" to me which just sounds weird.
Review of attachment 251871 [details] [review]: (make splinter happy by typing something here) ::: src/core/display.c @@ +518,3 @@ if (meta_is_syncing ()) XSynchronize (xdisplay, True); + Unrelated. ::: src/core/monitor.c @@ +222,3 @@ + * + * Returns TRUE if the only available monitor is the dummy one + * backing the ClutterStage window. That's not what the code does. It always returns false. If it is going to be made useful later add a comment.
Crap blame splinter for not submitting / giving any feedback.
Review of attachment 251873 [details] [review]: ::: src/xrandr.xml @@ +138,3 @@ + + @serial must match the serial from the last GetResources() call, + or org.freedesktop.DBus.AccessDenied will be generated. Sounds racy what if some client just grabs the information and while another one wants to change the configuration? @@ +139,3 @@ + @serial must match the serial from the last GetResources() call, + or org.freedesktop.DBus.AccessDenied will be generated. + (XXX: a better error maybe?) InvalidArgs ? The serial is invalid because it got invalidated by the newer GetResources call? dunno. @@ +143,3 @@ + If @persistent is true, mutter will attempt to replicate this + configuration the next time this HW layout appears. + (XXX: or is this gnome-settings-daemon role?) Good question.
Review of attachment 251874 [details] [review]: OK.
Review of attachment 251875 [details] [review]: ::: src/core/monitor.c @@ +180,3 @@ + + manager->max_screen_width = manager->modes[0].width; + manager->max_screen_height = manager->modes[0].height; As said on IRC we should limit that to the maximum texture size for now (can be done in a later patch though). @@ +961,3 @@ + guint output_id; + + if (crtc_id < 0 || crtc_id >= manager->n_crtcs) The first condition cannot happen crtc_id is unsigned. @@ +1005,3 @@ + MetaOutput *output; + + if (output_id < 0 || output_id >= manager->n_outputs) output_id is unsigned as well. @@ +1048,3 @@ + while (g_variant_iter_loop (&output_iter, "(ua{sv})", &output_id, NULL)) + { + if (output_id < 0 || output_id >= manager->n_outputs) Again first condition cannot happen.
Review of attachment 251876 [details] [review]: OK. ::: src/core/display.c @@ +2832,3 @@ &event->xconfigure); } + Unrelated whitespace change.
Review of attachment 251877 [details] [review]: OK.
Created attachment 251916 [details] [review] MonitorXrandr: expose the subpixel order from X11 Might be inaccurate in the face of rotation. Cogl has some code to account for that, but a) it's not clear which value we should expose in the wayland protocol b) the Cogl code is obviously wrong in that it just discards the X value c) the subpixel order is only used by wayland, so in the end... why bother?
Created attachment 251917 [details] [review] MonitorXrandr: follow the right order in applying the new configuration First disable CRTCs that should be off in the new configuration, then resize the framebuffer, then enable the new CRTCs. If we don't do that, and we're making the screen smaller, X complains with a BadMatch.
Review of attachment 251878 [details] [review]: OK. ::: src/compositor/plugins/default.c @@ +30,1 @@ +#include <stdlib.h> Why?
Review of attachment 251879 [details] [review]: ::: src/core/monitor-private.h @@ +49,3 @@ + +#ifndef HAVE_WAYLAND +enum wl_output_transform { Hmm thinking about this some more .. do we really want to mix wayland constants into the other backends? Wouldn't it make more sense to have a meta_output_transform ? Or is this API supposed to be implemented by someone else other then mutter?
Review of attachment 251880 [details] [review]: OK.
Review of attachment 251881 [details] [review]: OK.
Review of attachment 251882 [details] [review]: OK.
Review of attachment 251883 [details] [review]: OK.
Review of attachment 251884 [details] [review]: ::: src/core/monitor-config.c @@ +852,3 @@ + * best resolution + * + * Input assertions: there is at least one output Why no g_return_if_fail or g_assert then?
Review of attachment 251886 [details] [review]: OK
Review of attachment 251887 [details] [review]: OK, note: did not test on a device that actually has back light control. ::: src/core/monitor.c @@ +437,3 @@ + + if (actual_type != XA_INTEGER || actual_format != 32 || + nitems < 1) nitems is unsigned so that is impossible. @@ +1799,3 @@ + } + + if (output_id < 0 || output_id >= manager->n_outputs) You are doing a "guint < 0" again.
Review of attachment 251888 [details] [review]: OK.
Review of attachment 251889 [details] [review]: OK, needs fixes for the unsigned < 0 checks. ::: src/core/monitor.c @@ +1913,3 @@ + } + + if (crtc_id < 0 || crtc_id >= manager->n_crtcs) Again that can't be true. @@ +2000,3 @@ + } + + if (crtc_id < 0 || crtc_id >= manager->n_crtcs) Same here.
Review of attachment 251890 [details] [review]: OK, but note did test on a desktop so could not test this as well.
Review of attachment 251891 [details] [review]: Do we need a gnome-shell change for this?
Review of attachment 251892 [details] [review]: Hmm .. I'd rather have an abstract class and have the dummy backend subclass it as well.
Review of attachment 251893 [details] [review]: OK.
Review of attachment 251894 [details] [review]: Makes sense, maybe squash into the other patch?
Review of attachment 251895 [details] [review]: Files are missing. Forgot to git-add ?
Review of attachment 251896 [details] [review]: OK.
Review of attachment 251897 [details] [review]: OK. ::: src/core/monitor-xrandr.c @@ +48,3 @@ + * http://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xsettings/gsd-xsettings-manager.c + * for the reasoning */ +#define DPI_FALLBACK 96.0 But but the internets told me that this is wrong ;)
Review of attachment 251916 [details] [review]: "why bother?" ... why add code that serves no real purpose then?
Review of attachment 251917 [details] [review]: OK.
(In reply to comment #78) > Review of attachment 251879 [details] [review]: > > ::: src/core/monitor-private.h > @@ +49,3 @@ > + > +#ifndef HAVE_WAYLAND > +enum wl_output_transform { > > Hmm thinking about this some more .. do we really want to mix wayland constants > into the other backends? Wouldn't it make more sense to have a > meta_output_transform ? Or is this API supposed to be implemented by someone > else other then mutter? Well, it's either that or XRandR constants - I don't want to have another enum just to translate back and forth. XRandR Rotation is slightly more complicate to understand (it's bit mask of a rotation flag and one reflection flag), so I chose wl_output_transform. I thought of having a MetaOutputTransform that was bit-compatible with wl_output_transform, but what's the point? The enum is always there anyway - gtk is compiled with wayland support these days. (In reply to comment #83) > Review of attachment 251884 [details] [review]: > > ::: src/core/monitor-config.c > @@ +852,3 @@ > + * best resolution > + * > + * Input assertions: there is at least one output > > Why no g_return_if_fail or g_assert then? Because if the assertion is violated, it returns NULL and crashes immediately after, so it's equivalent to a g_assert (except it's segv rather than abrt...) (In reply to comment #85) > Review of attachment 251887 [details] [review]: > > OK, note: did not test on a device that actually has back light control. > > ::: src/core/monitor.c > @@ +437,3 @@ > + > + if (actual_type != XA_INTEGER || actual_format != 32 || > + nitems < 1) > > nitems is unsigned so that is impossible. I believe 0 is a valid value for unsigned :) > ::: src/core/monitor.c > @@ +222,3 @@ > + * > + * Returns TRUE if the only available monitor is the dummy one > + * backing the ClutterStage window. > > That's not what the code does. It always returns false. If it is going to be > made useful later add a comment. Well, not later, just in a different branch. (Also, if I remove it, suddenly the dummy backend becomes unused code and triggers warnings, but I really don't want to remove it or postpone the addition to the patch adding the env var) (In reply to comment #70) > Review of attachment 251873 [details] [review]: > > ::: src/xrandr.xml > @@ +138,3 @@ > + > + @serial must match the serial from the last GetResources() call, > + or org.freedesktop.DBus.AccessDenied will be generated. > > Sounds racy what if some client just grabs the information and while another > one wants to change the configuration? The serial is only incremented when something happens upstream (X emitted an event), so if a client calls GetResources() while another calls ApplyConfiguration(), the Apply calls succeeds, but the client's information immediately becomes stale. The querying client will at some point get XRRScreenNotify or wl_output_geometry, and then call GetResources(). If that doesn't happen, and it calls ApplyConfiguration too, it will fail, which is safe. In practice, there are only two clients of the API: gnome-settings-daemon and gnome-control-center, so the risk of races is just theoretical. > @@ +139,3 @@ > + @serial must match the serial from the last GetResources() call, > + or org.freedesktop.DBus.AccessDenied will be generated. > + (XXX: a better error maybe?) > > InvalidArgs ? The serial is invalid because it got invalidated by the newer > GetResources call? dunno. I wanted to reserve InvalidArgs for errors that can be prevented by good client side coding. > @@ +143,3 @@ > + If @persistent is true, mutter will attempt to replicate this > + configuration the next time this HW layout appears. > + (XXX: or is this gnome-settings-daemon role?) > > Good question. Well, it's answered by later patches - g-s-d's xrandr plugin will die, but 3.10 or 3.12. (In reply to comment #89) > Review of attachment 251891 [details] [review]: > > Do we need a gnome-shell change for this? Definitely. And I need to write it... (In reply to comment #90) > Review of attachment 251892 [details] [review]: > > Hmm .. I'd rather have an abstract class and have the dummy backend subclass it > as well. What would it buy us, beyond more GObject boilerplate in the header file and more casts?
(In reply to comment #60) > Review of attachment 251872 [details] [review]: > > ::: src/Makefile.am > @@ +335,3 @@ > + --c-namespace MetaDBus \ > + --generate-c-code meta-dbus-xrandr \ > + xrandr.xml > > Not sure the "xrandr" makes sense if it is an abstraction for different > backends but otoh it is based in the xrandr API ... Yeah, originally the DBus API was called org.gnome.Mutter.XRandR, but gdbus-codegen would generate a terrible name (xrand_r), so I switched to DisplayConfig. This is "XRandR for wayland" after all. > @@ +514,3 @@ > } > > +static const double known_diagonals[] = { > > Rationale for this? Or was this like that in gnome-rr as well? Copy pasted for gnome-desktop.
Created attachment 252023 [details] [review] Rework and consolidate monitor handling in MetaScreen Consolidate all places that deal with output configuration in MetaScreen, which gets it either from XRandR or from a dummy static configuration. We still need to read the Xinerama config, even when running xwayland, because we need the indices for _NET_WM_FULLSCREEN_MONITORS, but now we do it only when needed.
Created attachment 252024 [details] [review] Introduce a new DBus interface for display configuration This new interface will be used by the control center and possibly the settings daemon to configure the screens. It is designed to resemble a simplified XRandR, while still exposing all the quirks of the hardware, so that the panel can limit the user choices appropriately. To do so, MetaMonitorMode needs to track CRTCs, outputs and modes, so the low level objects have been decoupled from the high-level MetaMonitorInfo, which is used by core and API and offers a simplified view of HW, that hides away the details of what is cloned and how. This is still not efficient as it should be, because on every HW change we drop all data structures and rebuild them from scratch (which is not expensive because there aren't many of them, but at least in the XRandR path it involves a few sync X calls)
Created attachment 252025 [details] [review] Add the write side of the DBus protocol too This is just in the documentation for now, to attract wider feedback before we start looking at how to implement this for real.
Created attachment 252026 [details] [review] Extend the DBus XRandR protocol to expose cloning restriction Turns out that even if two outputs say that they can be controlled by a given CRTC, you can't configure them in the same CRTC unless they are marked as "possible clones" one of the other. This can further restrict the configuration options, so we need to expose this limitation in the DBus API.
Created attachment 252027 [details] [review] DisplayConfig: add the write side of the API Implement ApplyConfiguration in terms of XRandR calls. Error checking is done before actually committing the configuration. If mutter is using one of the other monitor config backends, an error is reported and nothing happens.
Created attachment 252028 [details] [review] Reverse handling of XRandR events between Screen and MonitorManager Now MonitorManager does its own handling of XRandR events, which means we no longer handle ConfigureNotify on the root window. MetaScreen reacts to MonitorManager::monitor-changed and updates its internal state, including the new size. This paves the way for doing display configuration using only the dummy backend, which would allow testing wl_output interfaces.
Created attachment 252029 [details] [review] DisplayConfig: make the dummy backend writable Add a number of dummy outputs and modes to the dummy backend, and implement the writing bits. The only visible effect is that you can change the screen size, which resizes the output window.
Created attachment 252030 [details] [review] default plugin: add a random color background on each monitor Instead of a full white background, make one with a random color. This way the different "monitors" are visible and it's easier to debug the DBus API.
Created attachment 252031 [details] [review] MonitorManager: fix handling of output transform Read the current transform from XRandR, and expose the transforms that are really supported on the bus. The dummy backend now advertises all transforms, since it doesn't actually apply them.
Created attachment 252032 [details] [review] MonitorManager: inherit directly from DisplayConfig instead of handling signals This way we can handle properties too.
Created attachment 252033 [details] [review] MonitorManager: add support for DPMS levels To the XRandR and dummy backend (and as usual the dummy backend has no effect)
Created attachment 252034 [details] [review] MonitorManager: add support for persistent monitor configurations Add a new object, MetaMonitorConfig, that takes care of converting between the logical configurations stored in monitors.xml and the HW resources exposed by MonitorManager. This commit includes loading and saving of configurations, but still missing is the actual CRTC assignments and a default configuration when none is found in the file.
Created attachment 252035 [details] [review] MonitorConfig: add CRTC assignment Ripped off libgnome-desktop, trimming the parts that checked that the configuration was plausible, as that should be done in gnome-control-center before asking mutter for a change.
Created attachment 252036 [details] [review] MonitorConfig: add support for default configurations Activate the presentation bit on new hotplugged monitors, while making a fully extended setup when running for the first time.
Created attachment 252037 [details] [review] MonitorManager: store the presentation mode bit in XRandR Use a private output property to store if the output is in presentation mode or not, so that this information is not lost after the configuration read back from the server.
Created attachment 252038 [details] [review] MonitorManager: further extend the dummy backend The default configuration is extended, which is only possible if there are as many CRTCs as outputs, so make sure that's true. Also, add more and bigger modes, so that different sizes will be chosen for the three outputs. A nice side effect of this is that with a real 1920x1080 + 1600x900 layout, if you disable the VGA you get a stage that matches the screen size, which triggers the legacy fullscreen path in the outside mutter.
Created attachment 252039 [details] [review] MonitorManager: add support for backlight GnomeRR needs that too. The backlight is exported as a normalized 0-100 value, or -1 if not supported. Clamping to HW limits is handled by the backend. Changing backlight uses a different method call, to avoid recomputing the full display configuration every time the user presses the backlight keys.
Created attachment 252040 [details] [review] MonitorManager: ignore configuration changes that disable all outputs If we compute a screen size of 0 in either direction, we crash later on, as it is invalid for clutter, cogl and X.
Created attachment 252041 [details] [review] MonitorManager: add gamma support Add GetCrtcGamma() and SetCrtcGamma(), that wrap the similarly named XRandR API. These are used by GnomeRR inside the color plugin of the control center (and may go away if the color plugin decides to do something different under wayland)
Created attachment 252042 [details] [review] MonitorConfig: handle changes in the laptop lid This way we don't need to track the current and previous configuration in gnome-settings-daemon, when we already do so in mutter.
Created attachment 252043 [details] [review] MetaPlugin: add a UI hook for confirming display changes We want to show a dialog when a display change happens from the control center. To do so, add a new vfunc to MetaPlugin and call it when a configuration change is requested via DBus.
Created attachment 252044 [details] [review] MonitorManager: split the XRandR parts in a subclass Instead of keeping a forest of if backend else ..., use a subclass and virtual functions to discriminate between XRandR and the dummy backend (which lives in the parent class togheter with the common code)
Created attachment 252045 [details] [review] MonitorManager: add EDID properties to the output DBus description Add "edid-file", if we have one (in the KMS case, where we can point people to the right sysfs file), or "edid" with inline data. These are needed by colord to build the default ICC profile for uncalibrated displays.
Created attachment 252046 [details] [review] MonitorXrandr: implement correct EDID parsing To provide valid values for the vendor, product and serial fields we need to read the EDID and parse it. Parser kindly provided by gnome-desktop.
Created attachment 252047 [details] [review] Monitor: restore correct display name handling Now that we have the right values from the EDID, we can load the PNP database and find the proper vendor name, to show in the control center UI.
Created attachment 252048 [details] [review] MonitorXrandr: resize the framebuffer prior to setting the CRTC configuration Otherwise X11 will trim the new configuration and disable outputs outside the screen.
Created attachment 252049 [details] [review] MonitorXrandr: follow the right order in applying the new configuration First disable CRTCs that should be off in the new configuration, then resize the framebuffer, then enable the new CRTCs. If we don't do that, and we're making the screen smaller, X complains with a BadMatch.
Note: there are some changes in this stack related to transform handling (namely, we needed to swap width/height in some cases and we weren't doing it) Also, this is a lot more tested now. I'll restore acn status based on the previous review.
(In reply to comment #98) > (In reply to comment #78) > > Review of attachment 251879 [details] [review] [details]: > > > > ::: src/core/monitor-private.h > > @@ +49,3 @@ > > + > > +#ifndef HAVE_WAYLAND > > +enum wl_output_transform { > > > > Hmm thinking about this some more .. do we really want to mix wayland constants > > into the other backends? Wouldn't it make more sense to have a > > meta_output_transform ? Or is this API supposed to be implemented by someone > > else other then mutter? > > Well, it's either that or XRandR constants - I don't want to have another enum > just to translate back and forth. > XRandR Rotation is slightly more complicate to understand (it's bit mask of a > rotation flag and one reflection flag), so I chose wl_output_transform. > I thought of having a MetaOutputTransform that was bit-compatible with > wl_output_transform, but what's the point? The enum is always there anyway - > gtk is compiled with wayland support these days. OK. > (In reply to comment #83) > > Review of attachment 251884 [details] [review] [details]: > > > > ::: src/core/monitor-config.c > > @@ +852,3 @@ > > + * best resolution > > + * > > + * Input assertions: there is at least one output > > > > Why no g_return_if_fail or g_assert then? > > Because if the assertion is violated, it returns NULL and crashes immediately > after, so it's equivalent to a g_assert (except it's segv rather than abrt...) Not really the same. Don't segfault abort(). It is not only "cleaner" but is more obvious why it crashed. And it is not that much of extra code ;) > (In reply to comment #85) > > Review of attachment 251887 [details] [review] [details]: > > > > OK, note: did not test on a device that actually has back light control. > > > > ::: src/core/monitor.c > > @@ +437,3 @@ > > + > > + if (actual_type != XA_INTEGER || actual_format != 32 || > > + nitems < 1) > > > > nitems is unsigned so that is impossible. > > I believe 0 is a valid value for unsigned :) Oops ... apparently I stopped reading after the "<" ;) > > ::: src/core/monitor.c > > @@ +222,3 @@ > > + * > > + * Returns TRUE if the only available monitor is the dummy one > > + * backing the ClutterStage window. > > > > That's not what the code does. It always returns false. If it is going to be > > made useful later add a comment. > > Well, not later, just in a different branch. > (Also, if I remove it, suddenly the dummy backend becomes unused code and > triggers warnings, but I really don't want to remove it or postpone the > addition to the patch adding the env var) OK, adding a comment still wouldn't hurt. > (In reply to comment #70) > > Review of attachment 251873 [details] [review] [details]: > > > > ::: src/xrandr.xml > > @@ +138,3 @@ > > + > > + @serial must match the serial from the last GetResources() call, > > + or org.freedesktop.DBus.AccessDenied will be generated. > > > > Sounds racy what if some client just grabs the information and while another > > one wants to change the configuration? > > The serial is only incremented when something happens upstream (X emitted an > event), so if a client calls GetResources() while another calls > ApplyConfiguration(), the Apply calls succeeds, but the client's information > immediately becomes stale. > The querying client will at some point get XRRScreenNotify or > wl_output_geometry, and then call GetResources(). If that doesn't happen, and > it calls ApplyConfiguration too, it will fail, which is safe. > In practice, there are only two clients of the API: gnome-settings-daemon and > gnome-control-center, so the risk of races is just theoretical. OK, fair enough. > > @@ +139,3 @@ > > + @serial must match the serial from the last GetResources() call, > > + or org.freedesktop.DBus.AccessDenied will be generated. > > + (XXX: a better error maybe?) > > > > InvalidArgs ? The serial is invalid because it got invalidated by the newer > > GetResources call? dunno. > > I wanted to reserve InvalidArgs for errors that can be prevented by good client > side coding. OK. > > @@ +143,3 @@ > > + If @persistent is true, mutter will attempt to replicate this > > + configuration the next time this HW layout appears. > > + (XXX: or is this gnome-settings-daemon role?) > > > > Good question. > > Well, it's answered by later patches - g-s-d's xrandr plugin will die, but 3.10 > or 3.12. "but 3.10 or 3.12" what? > (In reply to comment #89) > > Review of attachment 251891 [details] [review] [details]: > > > > Do we need a gnome-shell change for this? > > Definitely. And I need to write it... OK > (In reply to comment #90) > > Review of attachment 251892 [details] [review] [details]: > > > > Hmm .. I'd rather have an abstract class and have the dummy backend subclass it > > as well. > > What would it buy us, beyond more GObject boilerplate in the header file and > more casts? A "cleaner design" but you have a point ...
Created attachment 252056 [details] [review] MonitorXrandr: update the internal data structures after applying We were relying on the XRandR events from the X server to update the configuration, but we were calling meta_monitor_config_update_current() immediately after, so the MonitorConfig would be updated with the old configuration (and we would save that to disk!)
Review of attachment 252023 [details] [review]: OK, with the comment added. ::: src/core/monitor.c @@ +225,3 @@ + */ +static gboolean +has_dummy_output (void) As I said in the other comment please add a comment why we have a function that always returns FALSE.
Review of attachment 252024 [details] [review]: OK.
Review of attachment 252025 [details] [review]: OK.
Review of attachment 252027 [details] [review]: OK, with var renamed. ::: src/core/monitor-private.h @@ +97,3 @@ + + /* Used when changing configuration */ + gboolean dirty; s/dirty/is_dirty/ (yeah somehow forgot to note that in the previous review).
Review of attachment 252031 [details] [review]: OK.
Review of attachment 252036 [details] [review]: OK.
Review of attachment 252037 [details] [review]: OK.
Review of attachment 252039 [details] [review]: OK, with style fixes (still didn't test I assume you did). ::: src/core/monitor.c @@ +414,3 @@ + int hw_value) +{ + return round((double)(hw_value - output->backlight_min) / Missing whitespace. @@ +1787,3 @@ + int hw_value; + + hw_value = round((double)value / 100.0 * output->backlight_max + output->backlight_min); Missing whitespace.
Review of attachment 252041 [details] [review]: OK.
Review of attachment 252042 [details] [review]: OK, with style fixed (still didn't test on a laptop though). ::: src/core/monitor-config.c @@ +1268,3 @@ +{ + if (self->previous) + { Remove for one line if.
Review of attachment 252044 [details] [review]: OK. ::: src/core/monitor-private.h @@ +215,3 @@ + /* XXX: this structure is very badly + packed, but I like the logical organization + of fields */ Just kill this comment.
Review of attachment 252046 [details] [review]: Broken coding style please fix. ::: src/core/edid-parse.c @@ +78,3 @@ + switch (edid[0x10]) + { + case 0x00: Broken indentation. @@ +93,3 @@ + + if (is_model_year) + { Same here .. I know it is copy and pasted but still should match our coding style. ::: src/core/monitor-xrandr.c @@ +264,3 @@ + } + else + { Remove braces.
Review of attachment 252047 [details] [review]: OK.
Review of attachment 252049 [details] [review]: OK.
Review of attachment 252056 [details] [review]: OK.
Pushed with the suggested changes. Now let's wait for the regressions! :) Attachment 252023 [details] pushed as 214f312 - Rework and consolidate monitor handling in MetaScreen Attachment 252024 [details] pushed as 3bb33d3 - Introduce a new DBus interface for display configuration Attachment 252025 [details] pushed as 7e1d100 - Add the write side of the DBus protocol too Attachment 252026 [details] pushed as dc242e4 - Extend the DBus XRandR protocol to expose cloning restriction Attachment 252027 [details] pushed as 57d0837 - DisplayConfig: add the write side of the API Attachment 252028 [details] pushed as bf40409 - Reverse handling of XRandR events between Screen and MonitorManager Attachment 252029 [details] pushed as c354e7e - DisplayConfig: make the dummy backend writable Attachment 252030 [details] pushed as fc67c70 - default plugin: add a random color background on each monitor Attachment 252031 [details] pushed as 522542c - MonitorManager: fix handling of output transform Attachment 252032 [details] pushed as dbd8d4d - MonitorManager: inherit directly from DisplayConfig instead of handling signals Attachment 252033 [details] pushed as e039add - MonitorManager: add support for DPMS levels Attachment 252034 [details] pushed as 8f46212 - MonitorManager: add support for persistent monitor configurations Attachment 252035 [details] pushed as d0529b7 - MonitorConfig: add CRTC assignment Attachment 252036 [details] pushed as 764c472 - MonitorConfig: add support for default configurations Attachment 252037 [details] pushed as 5c27a91 - MonitorManager: store the presentation mode bit in XRandR Attachment 252038 [details] pushed as 849050b - MonitorManager: further extend the dummy backend Attachment 252039 [details] pushed as 8b52782 - MonitorManager: add support for backlight Attachment 252040 [details] pushed as cd20f1b - MonitorManager: ignore configuration changes that disable all outputs Attachment 252041 [details] pushed as 3b61b85 - MonitorManager: add gamma support Attachment 252042 [details] pushed as bbbcd8c - MonitorConfig: handle changes in the laptop lid Attachment 252043 [details] pushed as 5086626 - MetaPlugin: add a UI hook for confirming display changes Attachment 252044 [details] pushed as 46de0ed - MonitorManager: split the XRandR parts in a subclass Attachment 252045 [details] pushed as 5707743 - MonitorManager: add EDID properties to the output DBus description Attachment 252046 [details] pushed as 6946784 - MonitorXrandr: implement correct EDID parsing Attachment 252047 [details] pushed as 3bb5086 - Monitor: restore correct display name handling Attachment 252048 [details] pushed as 0986b66 - MonitorXrandr: resize the framebuffer prior to setting the CRTC configuration Attachment 252049 [details] pushed as 3528b06 - MonitorXrandr: follow the right order in applying the new configuration Attachment 252056 [details] pushed as 3112794 - MonitorXrandr: update the internal data structures after applying