GNOME Bugzilla – Bug 787477
monitor-manager (x11) problems
Last modified: 2021-07-05 13:51:55 UTC
1) Changing scaling-factor does nothing. Don't know if this is problem in mutter or gnome-settings-daemon... Wehn changing scaling-factor dbus-monitor shows that GetCurrentState is called: - monitors property, my both monitors have preferred scale set to 2 or 3 (I tried to use 0, 1, 2 and 3 as scaling factor), but supported scales array includes only scale 1. - logical monitors property, both monitor scale is 1 - layout-mode is 2 and global-scale-required is true If I restart mutter after I have set scaling-factor to 2, everything is still at scale 1. Then I can again set it to any value and then everything gets scaled to 2 no matter what scaling value is set now. As example I again set it to 3. dbus-monitor shows that monitors property is same as previously. preffered scale is 3 and supported scales are 1. but logical monitors now have scale 2 (value that was set before mutter restart) 2) meta_monitor_manager_xrandr_handle_xevent is using resources without checking if resources is not NULL. meta_monitor_manager_xrandr_read_current function clears resources. Code shows that XRRGetScreenResourcesCurrent can return NULL, but handle_xevent access timestamp assuming that resources is never NULL. 3) meta_monitor_config_store_constructed does not chain up to parent class. https://developer.gnome.org/gobject/stable/howto-gobject-destruction.html https://developer.gnome.org/gobject/stable/chapter-gobject.html These links tells that constructed, dispose and finalize always should chain up to parent class. 4) save_cancellable in meta-monitor-config-store is not freed in dispose. 5) meta_monitors_config_finalize also does not chain up. 6) About derive_logical_monitor_configs function. There are monitor_configs GList that is always NULL and two places where this list is freed. 7) meta_migrate_old_monitors_config use g_autoptr with hash table that are explicitly destroyed and not set to null. g_hash_table_unref is called after table is destroyed. Is that valid thing to do? 8) meta_monitor_finalize and meta_monitor_tiled_finalize also does not chain up.
meta_monitor_normal_generate_modes function sets preferred and/or current mode and then if adding this mode fails then this mode is freed. Now potentially preferred and/or current mode points to freed memory...
Created attachment 359491 [details] [review] backends: Chain up some GObjectClass vfuncs Various vfunc implementations was not chained up properly. This commit fixes that.
Created attachment 359492 [details] [review] monitor-config-migration: Remove unused variable Monitor configurations are appended directly into the logical monitor, and not into the temporary place holder; so remove the place holder.
Created attachment 359493 [details] [review] monitor-config-migration: Don't destroy autoptr:ed hash table It'll be destroyed automatically, so don't do it manually as well.
Created attachment 359494 [details] [review] monitor-config-store: Don't clear cancellable when cancelled The cancellable should only be cleared if we weren't cancelled, as if we were cancelled, the path cancelling have already cleared the cancellable.
Created attachment 359495 [details] [review] monitor-config-store: Don't leak when saving synchronously We currently only save synchronously when running the test suite, but should still not leak the generated config buffer. We also created the cancellable but never used it, so lets stop doing that too.
Created attachment 359496 [details] [review] monitor-config-store: Maybe force save configuration on tear down If there is a pending config file content replacement in progress on tear down, cancel it and save it synchronously to avoid any data loss.
Created attachment 359497 [details] [review] monitor-manager: Pass config to derive from when updating state When we update state, we might not have set the current config yet (for example if the Xrandr assignment didn't change), so pass the monitors config we should derive from instead of fetching it from the monitor config manager.
Created attachment 359498 [details] [review] monitor: Add scale 1 if no other supported scale was added We currently have a hard coded limit on logical monitor sizes, meant for filtering out monitor scales that would result in awkward desktop sizes. This has the side effect of also disqualifying scale 1 for resolutions that themself are lower than the mentioned limit. To avoid listing no supported scales, always add the fallback scale 1 if no other was added.
Thanks a lot for the feedback and review. Any chance you can check if the attached patches addresses the issue you mention in 1)? It seems a bug caused the scale used by X clients was always the scale from the previous configuration (if any) when no actual Xrandr state changed. "monitor-manager: Pass config to derive from when updating state" should fix that bug.
Applied all patches and now it does not build: https://paste.gnome.org/pmrwaqsla
Created attachment 359499 [details] [review] monitor-manager: Pass config to derive from when updating state When we update state, we might not have set the current config yet (for example if the Xrandr assignment didn't change), so pass the monitors config we should derive from instead of fetching it from the monitor config manager.
(In reply to Alberts Muktupāvels from comment #11) > Applied all patches and now it does not build: > https://paste.gnome.org/pmrwaqsla Oops. I had only git add -p:ed half the patch, sorry about that. Check the updated one.
Created attachment 359500 [details] [review] monitor-manager/xrandr: Handle failing to get resources better Warn louder if we don't get any resources, and also handle it later.
Now scaling-factor changing does nothing and even after restarting I can not get scale factor 2. Should not MetaMonitorManager connect to MetaSettings global-scaling-factor-changed signal and update monitors?
(In reply to Alberts Muktupāvels from comment #15) > Now scaling-factor changing does nothing and even after restarting I can not > get scale factor 2. Should not MetaMonitorManager connect to MetaSettings > global-scaling-factor-changed signal and update monitors? 'global-scaling-factor' is about the gsetting that overrides any computed scale. But connecting it to 'ui-scaling-factor' makes sense. BTW, how are you testing things? X11 clients gets their scaling factor from the 'gsd-xsettings' process (part of gnome-settings-daemon). gsd-xsettings will calculate that scale by looking at what mutter exposes. Could you also check if there is anything interesting in journalctl? Either way, I'm attaching a patch that also recalculates the ui-scaling-factor.
Created attachment 359501 [details] [review] settings: Update ui scaling factor when monitors changed
(In reply to Jonas Ådahl from comment #16) > (In reply to Alberts Muktupāvels from comment #15) > > Now scaling-factor changing does nothing and even after restarting I can not > > get scale factor 2. Should not MetaMonitorManager connect to MetaSettings > > global-scaling-factor-changed signal and update monitors? > > 'global-scaling-factor' is about the gsetting that overrides any computed > scale. But connecting it to 'ui-scaling-factor' makes sense. I am changing gsetting... > BTW, how are you testing things? X11 clients gets their scaling factor from > the 'gsd-xsettings' process (part of gnome-settings-daemon). gsd-xsettings > will calculate that scale by looking at what mutter exposes. - Applied patches - Restarted mutter when scaling-factor was default - 0 - Tried to change scaling-factor 0, 1, 2, 3 - Restarted mutter when scaling-factor was 2 - Tried to change scaling-factor 1, 3 dbus-monitor shows that gsd-xsettings calls GetCurrentState. Scale that I have set is only included as preferred scale, everything else there is set as 1. > Could you also check if there is anything interesting in journalctl? No. > Either way, I'm attaching a patch that also recalculates the > ui-scaling-factor. When GetCurrentState is called, mutter calculates supported scales. Maybe one problem is that supported scale does not include value I have set in gsettings?
Created attachment 359502 [details] [review] settings: Update ui scaling factor when monitors changed
https://paste.gnome.org/pfcl9mjnw This is what gsd-xsettings gets after changing scaling-factor.
Hmm, logical mode is 2 (physical)... https://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xsettings/gsd-xsettings-manager.c#n543 So this is bug in gsd? Mode is not logical so it returns always scale 1.
(In reply to Alberts Muktupāvels from comment #21) > Hmm, logical mode is 2 (physical)... Physical is correct; it's the only layout mode supported on X11. > > https://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xsettings/ > gsd-xsettings-manager.c#n543 > > So this is bug in gsd? Mode is not logical so it returns always scale 1. I suspect I know whats going on. Could you check what the 'overrides' setting in org.gnome.settings-daemon.plugins.xsettings contains? It might contain something about scaling factor; which effectively overrides what gsd-xsettings tells the world. Also note that the org.gnome.desktop.interface scaling-factor gsetting overrides what is calculated as the default scale of a monitor. When you configure your monitor via e.g. gnome-control-center, that scaling factor is overridden by what you configured with. Anyhow, I'll try to summarize: * org.gnome.desktop.interface::scaling-factor affects what is generated when no previous configuration is set (e.g. when no ~/.config/monitors.xml exists) * gsd-xsettings will pick up what mutter has been configured to do (i.e. if you configure mutter to use scale 2, gsd-xsettings will pick this up) * org.gnome.settings-daemon.plugins.xsettings.overrides may contain rules that overrides anything mutter told gsd-xsettings about * gsd-xsettings is responsible of notifying various X11 clients what scale to use (such as GTK+ 3 clients) * GNOME Shell gets the scaling factor directly from mutter (ui-scaling-factor in MetaSettings)
Comment on attachment 359502 [details] [review] settings: Update ui scaling factor when monitors changed Marking 'sittngs: Update ui scaling factor ..' as rejected; we already do that.
(In reply to Jonas Ådahl from comment #22) > (In reply to Alberts Muktupāvels from comment #21) > > Hmm, logical mode is 2 (physical)... > > Physical is correct; it's the only layout mode supported on X11. > > > > > https://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xsettings/ > > gsd-xsettings-manager.c#n543 > > > > So this is bug in gsd? Mode is not logical so it returns always scale 1. I misread code, I was thinking that if layout-mode is not logical then scale 1 is used. Ok so it is not problem in gsd. > I suspect I know whats going on. Could you check what the 'overrides' > setting in org.gnome.settings-daemon.plugins.xsettings contains? It might > contain something about scaling factor; which effectively overrides what > gsd-xsettings tells the world. It was set to empty value, but changed back to default so nothing is set, but this should not change anything. > Also note that the org.gnome.desktop.interface scaling-factor gsetting > overrides what is calculated as the default scale of a monitor. When you > configure your monitor via e.g. gnome-control-center, that scaling factor is > overridden by what you configured with. Well that is the problem. That gsetting does not override anything. I can change how many times I want it wont do anything. > Anyhow, I'll try to summarize: > > * org.gnome.desktop.interface::scaling-factor affects what is generated when > no previous configuration is set (e.g. when no ~/.config/monitors.xml exists) Ok, deleted this monitors file. And restarted mutter with scaling-factor set to 2. It still was unscaled... I had to do something that will trigger update in gsd-xsettings. Then everything was updated and scaled at 2, but still after that changing scaling-factor does nothing. > * gsd-xsettings will pick up what mutter has been configured to do (i.e. if > you configure mutter to use scale 2, gsd-xsettings will pick this up) gsd-xsettings re-calculates scale when it gets "monitors-changed" signal or gsettings value has changed. This part I think works correctly. The problem is that I am trying to configure it to use scale 2 with scaling-factor, but it does not change configuration. It use value that it has calculated when it started. > * org.gnome.settings-daemon.plugins.xsettings.overrides may contain rules > that overrides anything mutter told gsd-xsettings about I know that I could override scale using overrides, but it is empty / default and at least in this case it is not relevant. > * gsd-xsettings is responsible of notifying various X11 clients what scale > to use (such as GTK+ 3 clients) I know and that part works if mutter reports correct scale. > * GNOME Shell gets the scaling factor directly from mutter > (ui-scaling-factor in MetaSettings) In this case I am using gsd, gcc and mutter. What I think is that mutter should re-calculate monitor scales when org.gnome.desktop.interface::scaling-factor changes and emit monitors-changed signal so gsd-xsettings can do its work. Not related, but probably gsd-xsettings should not re-calculate scale when scaling-factor changes, but only when it gets monitors-changed signal. Simply it no longer use this value and probably does only unneeded work. In other words xsettings_callback function should not call xft_callback when scaling-factor changed.
Review of attachment 359492 [details] [review]: ::: src/backends/meta-monitor-config-migration.c @@ -995,3 @@ else { - g_list_free_full (monitor_configs, Should not this be `logical_monitor_config->monitor_configs`? @@ -1009,3 @@ if (!monitor_config) - { - g_list_free_full (monitor_configs, Same here.
Jonas, do you have any other comments about scaling-factor?
(In reply to Alberts Muktupāvels from comment #25) > Review of attachment 359492 [details] [review] [review]: > > ::: src/backends/meta-monitor-config-migration.c > @@ -995,3 @@ > else > { > - g_list_free_full (monitor_configs, > > Should not this be `logical_monitor_config->monitor_configs`? > It should rather free the the logical_monitor_configs, but good point. (In reply to Alberts Muktupāvels from comment #26) > Jonas, do you have any other comments about scaling-factor? Not too sure I think making the gsetting override any configured scale globally. Since it's there already we can't remove it, but in my opinion we should limit the "damage" it can cause. It changing could however trigger a monitors-change signal, so that if no configuration is set, the new configured scale can be used.
Review of attachment 359491 [details] [review]: ++
Review of attachment 359492 [details] [review]: marking needs-work for the missing g_list_free_full()s
Review of attachment 359493 [details] [review]: looks fine
Created attachment 360072 [details] [review] monitor-config-migration: Fix a minor leak when parsing fails -- noticed while reviewing
Review of attachment 359494 [details] [review]: ok (we don't strictly need to clear the cancellable in the callback since it would be cleared either if a new save request comes in or in dispose() )
Review of attachment 360072 [details] [review]: Yup
Review of attachment 359495 [details] [review]: the cancellable was already being used so that part of the commit message is wrong code looks fine ::: src/backends/meta-monitor-config-store.c @@ +1250,2 @@ static void +meta_monitor_config_store_force_save (MetaMonitorConfigStore *config_store) nit: I'd call it _save_sync() to make it clear it does sync IO
Review of attachment 359496 [details] [review]: makes sense
Review of attachment 359498 [details] [review]: oops, this one is my fault
Review of attachment 359499 [details] [review]: lgtm
Review of attachment 359500 [details] [review]: we also need non-null resources in xrandr_set_crtc_config() when applying a config so we should handle it there too
(In reply to Rui Matos from comment #34) > Review of attachment 359495 [details] [review] [review]: > > the cancellable was already being used so that part of the commit message is > wrong I'll rephrase it. What I meant was: We also created the cancellable when saving synchronously but never used it, so lets stop doing that too. > > code looks fine > > ::: src/backends/meta-monitor-config-store.c > @@ +1250,2 @@ > static void > +meta_monitor_config_store_force_save (MetaMonitorConfigStore *config_store) > > nit: I'd call it _save_sync() to make it clear it does sync IO Sure. (In reply to Rui Matos from comment #38) > Review of attachment 359500 [details] [review] [review]: > > we also need non-null resources in xrandr_set_crtc_config() when applying a > config so we should handle it there too I think that patch is incomplete still. We would be left with a bunch of invalid pointers to removed MetaOutput's and MetaCrtcs etc. I'll leave that patch be until some other time.
Attachment 359491 [details] pushed as 70e0fd0 - backends: Chain up some GObjectClass vfuncs Attachment 359493 [details] pushed as 96572fb - monitor-config-migration: Don't destroy autoptr:ed hash table Attachment 359494 [details] pushed as 5ed954e - monitor-config-store: Don't clear cancellable when cancelled Attachment 359495 [details] pushed as 7a1393b - monitor-config-store: Don't leak when saving synchronously Attachment 359496 [details] pushed as 8b022a5 - monitor-config-store: Maybe force save configuration on tear down Attachment 359498 [details] pushed as 67ce049 - monitor: Add scale 1 if no other supported scale was added Attachment 359499 [details] pushed as 22cdc8f - monitor-manager: Pass config to derive from when updating state Attachment 360072 [details] pushed as ab541e3 - monitor-config-migration: Fix a minor leak when parsing fails
I'm not sure if this is supposed to be fixed yet, but I'm still having problem 1 (scaling not remembered after restart) with current git master. While tweaking the DPI settings in Display Settings, I'm also getting the following messages in my logs (first one when changing monitor layout, second when changing scaling): gnome-control-c[30305]: Config not applicable: GDBus.Error:org.freedesktop.DBus.Error.AccessDenied: The requested configuration is based on stale information gnome-control-c[30305]: Config not applicable: GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Logical monitor scales must be identical
(In reply to Sebastiaan Lokhorst from comment #41) > I'm not sure if this is supposed to be fixed yet, but I'm still having > problem 1 (scaling not remembered after restart) with current git master. > > While tweaking the DPI settings in Display Settings, I'm also getting the > following messages in my logs (first one when changing monitor layout, > second when changing scaling): > > gnome-control-c[30305]: Config not applicable: > GDBus.Error:org.freedesktop.DBus.Error.AccessDenied: The requested > configuration is based on stale information > > gnome-control-c[30305]: Config not applicable: > GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Logical monitor scales > must be identical These two things seems to rather be issues in gnome-control-center, rather than mutter/gnome-shell. The first one means that the serial number g-c-c used for applying a configuration is older than the most recent state that was read by mutter (from xrandr). The second one means that gnome-control-center tries to configure different scales on X11, which is not supported.
Created attachment 360444 [details] [review] monitor-manager: remove ApplyConfiguration method from interface Unused since e8a62861c9db0d645fee4fddd5ac17e2b561317e commit.
Created attachment 360445 [details] [review] monitor-manager: replace tabs with spaces in interface file
Review of attachment 360444 [details] [review]: Makes sense to remove this, but I guess after branching (even though its non-functional on 3.26)
Maybe this bug should be split in two? Moving scaling-factor to new / separate bug... It seems that comment #2 has been missed, or it is not problem? Did you have time to think more about scaling-factor setting? It seems that GCC shows scale setting only if mutter reports at least two supported scales. The problem is small monitors... I used scaling-factor to test HiDPI, before 3.26 it was easy - just changing GSetting was enough. Now it does not work and I see it as regression. scaling-factor == 0 - if there is no configuration saved use scale that has been auto detected - if there is saved configuration then use scale that has been saved - in this case user can change scale from GCC if monitor supports two or more scales scaling-factor > 0 - can be used to force scale that would ignore saved value in configuration - maybe could be used as supported scale if value > 1
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.