GNOME Bugzilla – Bug 767804
Some fixes for the underscan implementation
Last modified: 2021-07-05 13:44:07 UTC
We ship these patches in Endless. The first one to support different values of the underscan atom is only really required in conjunction with a patched Intel driver that exposes a different value for the "underscan" atom. But it seems a good idea and a nice cleanup to have it upstream too. The second patch streamlines the way the underscan borders are computed and stored, and should make the experience smoother on platforms which have support for the "on" value of the "underscan" atom.
Created attachment 329977 [details] [review] MonitorManager: support different values for underscan atom Some driver implementation may use a different value. For example, the Intel driver shipped with Endless uses "crop" to trigger a behavior that would not be compatible with the definition of what "on" means. This commit just adds infrastructure for a modular value, but does not actually add support for any other value.
Created attachment 329978 [details] [review] MonitorManager: store and use underscan border values Centralize the place the underscan border values are set, and more consistently use them instead of calculating them from the width every time.
Created attachment 330506 [details] [review] MonitorManager: check for underscan setting validity upfront Instead of doing a roundtrip to the X server before setting it, rely on the previous value fetched before the configuration was sent over DBus. This matches the argument check we already do elsewhere, and will allow us to more easily add an additional condition to determine if underscan is supported.
Created attachment 330507 [details] [review] MonitorManager: only support underscan when mode allows it Underscan should only be available for certain modes; in particular, it should only be available for HDMI displays with a compatible resolution.
Anyone that wants to take a look at these?
Review of attachment 329977 [details] [review]: ::: src/backends/meta-monitor-manager-private.h @@ +148,3 @@ gboolean supports_underscanning; + char *underscan_value; Doesn't seem like this belong here, since it's a xrandr detail. I also only see it ever used by MetaMonitorManagerXrandr, so it should probably be placed in the drivers private. ::: src/backends/x11/meta-monitor-manager-xrandr.c @@ +250,3 @@ output_get_supports_underscanning_xrandr (MetaMonitorManagerXrandr *manager_xrandr, + MetaOutput *output, + char **underscan_name) Here you call it underscan_*name*, elsewhere its underscan_*value* @@ +885,3 @@ + &meta_output->underscan_value); + if (!meta_output->supports_underscanning) + g_clear_pointer (&meta_output->underscan_value, g_free); No need for this right? You only set underscan_value to something if underscanning is supported.
Review of attachment 329978 [details] [review]: The commit message confused me a bit. What the patch seems to do is to change from using the current mode for determining the underscan to using xrandr if it was already set. In other words, I think the field you added to the generic layer should stay in the xrandr private field. I don't think it'll be used by the KSM version, as keeping a set of extra ints synchronized is just more error prone than doing the multiplication, and getting them doesn't involve any IPC so no need to cache anything. ::: src/backends/x11/meta-monitor-manager-xrandr.c @@ +222,3 @@ } static gboolean No point in making this return a gboolean, you always ignore the return value. @@ +259,3 @@ + *underscan_hborder = ((int*)hborder_buffer)[0]; + if (underscan_vborder) + *underscan_vborder = ((int*)vborder_buffer)[0]; No need to make these conditional, you only ever call this function with not-NULL undersscan_(h|v)border @@ +1085,3 @@ + output->underscan_hborder = output->crtc->current_mode->width * OVERSCAN_COMPENSATION_BORDER; + if (output->underscan_vborder == 0) + output->underscan_vborder = output->crtc->current_mode->height * OVERSCAN_COMPENSATION_BORDER; I guess output_get_underscanning_borders_xrandr() will never set them as 0 explicitly right?
Review of attachment 330506 [details] [review]: Makes sense to me.
Review of attachment 330507 [details] [review]: I suppose this make sense. But why isn't UHDTV underscan:able? ::: src/backends/meta-monitor-manager-private.h @@ +411,3 @@ GBytes *edid); gboolean meta_output_is_laptop (MetaOutput *output); +gboolean meta_output_is_underscan_compatible (MetaOutput *output); nit: I think a better name would be meta_output_supports_underscan() ::: src/backends/meta-monitor-manager.c @@ +1490,3 @@ +static gboolean +is_hdtv_resolution (guint width, + guint height) nit: the type of MetaMonitorMode::width,height is just "int" (not guint, gint etc). @@ +1494,3 @@ + return (width == 1920 && height == 1080) || + (width == 1440 && height == 1080) || + (width == 1280 && height == 720); If you put the whole expression inside () it'll align properly automatically.
Created attachment 334691 [details] [review] MonitorManager: support different values for underscan atom Some driver implementation may use a different value. For example, the Intel driver shipped with Endless uses "crop" to trigger a behavior that would not be compatible with the definition of what "on" means. This commit just adds infrastructure for a modular value, but does not actually add support for any other value.
Created attachment 334692 [details] [review] MonitorManager: store and use underscan border xrandr values Instead of always trying to derive them from the current mode, store the xrandr border values inside MetaOutputXrandr, and only calculate them if they haven't been set before. This fixes cases where the border would be doubly-applied.
Created attachment 334693 [details] [review] MonitorManager: check for underscan setting validity upfront Instead of doing a roundtrip to the X server before setting it, rely on the previous value fetched before the configuration was sent over DBus. This matches the argument check we already do elsewhere, and will allow us to more easily add an additional condition to determine if underscan is supported.
Created attachment 334694 [details] [review] MonitorManager: only support underscan when mode allows it Underscan should only be available for certain modes; in particular, it should only be available for HDMI displays with a compatible resolution.
Created attachment 334695 [details] [review] MonitorManager: factor out function to fetch an atom property
Thanks, Jonas. I should have fixed all of your comments now.
Ping? Any opinion on the new patchset?
Jonas or others: any other thoughts on these patches?
(In reply to Jonas Ådahl from comment #6) > Review of attachment 329977 [details] [review] [review]: > > ::: src/backends/meta-monitor-manager-private.h > @@ +148,3 @@ > gboolean supports_underscanning; > > + char *underscan_value; > > Doesn't seem like this belong here, since it's a xrandr detail. I also only > see it ever used by MetaMonitorManagerXrandr, so it should probably be > placed in the drivers private. I think it would be valuable to have underscan_value be a property of MetaOutput (similar to supports_underscanning, which is already there), as that would be useful from other places, if we need to account for the type of underscanning that is available. For instance, in Endless we have this patch currently applied (based in the initial set of patches from Cosimo): https://github.com/endlessm/mutter/commit/21d6ed5f10b23f50465d20cd9db682c94e548eb2 ...where having underscan_value as a property (set from the xrandr backend) would make the code cleaner than if it was inside the driver_private for that backend (see https://github.com/endlessm/mutter/commit/21d6ed5f10b23f50465d20cd9db682c94e548eb2#diff-b50148df7cac921c7a5b00fdc587a0c7R1976)
@cosimo JFTR I put this patch on top of yours while rebasing mutter for Endless: https://github.com/endlessm/mutter/commit/2d27c068575ead1902d0e7e1605eba93c94940f4 Ideally, if Jonas agree, I'd say squash that into your first patch. Also, your patchset no longer applies cleanly on top of master, could you please rebase it?
Created attachment 349247 [details] [review] MonitorManager: support different values for underscan atom Some driver implementation may use a different value. For example, the Intel driver shipped with Endless uses "crop" to trigger a behavior that would not be compatible with the definition of what "on" means. This commit just adds infrastructure for a modular value, but does not actually add support for any other value.
Created attachment 349248 [details] [review] MonitorManager: store and use underscan border xrandr values Instead of always trying to derive them from the current mode, store the xrandr border values inside MetaOutputXrandr, and only calculate them if they haven't been set before. This fixes cases where the border would be doubly-applied.
Created attachment 349249 [details] [review] MonitorManager: check for underscan setting validity upfront Instead of doing a roundtrip to the X server before setting it, rely on the previous value fetched before the configuration was sent over DBus. This matches the argument check we already do elsewhere, and will allow us to more easily add an additional condition to determine if underscan is supported.
Created attachment 349250 [details] [review] MonitorManager: only support underscan when mode allows it Underscan should only be available for certain modes; in particular, it should only be available for HDMI displays with a compatible resolution.
Created attachment 349251 [details] [review] MonitorManager: factor out function to fetch an atom property
Mario, I rebased all the patches to latest master. About your suggestion, I see the point but I am not entirely sure that having the underscan name as a generic property of MetaOutput is the way to go, since that concept may not make sense at all for another backend that does not use an X driver.
Thanks for rebasing this, Cosimo. About my suggestion, I don't really see the big picture here, so it might very well be non-sense. It just comes from the fact that I spent some time trying to figure out how to implement the patch from [1] on top of your current patches without having access to the underscan_value variable via the MetaOutput. That said, perhaps you can take a look to this with that patch of ours in mind? I'm sure you or Jonas are in a much better position than me to come up with something that makes sense both for usptream and downstream. For now, though, I just put a patch of mine on top of yours, in our local fork [2] [1] https://github.com/endlessm/mutter/commit/21d6ed5f10b23f50465d20cd9db682c94e548eb2 [2] https://github.com/endlessm/mutter/commit/2d27c068575ead1902d0e7e1605eba93c94940f4
Another rebase of GNOME Shell for Endless is due and I found this bug again while rebasing mutter, so I think a ping is in order. Any comment? For now I'll just rebase our patches on top of our mutter, but it would be nice to try to share as much as possible.
(In reply to Mario Sánchez Prada from comment #27) > Another rebase of GNOME Shell for Endless is due and I found this bug again > while rebasing mutter, so I think a ping is in order. > > Any comment? For now I'll just rebase our patches on top of our mutter, but > it would be nice to try to share as much as possible. Sorry for the delays with these patches. Now on master, things related to this (monitor management etc) has calmed down a lot, so I'd be happy to review. The current patches probably need various changes however, as a lot of things have moved away from meta-monitor-manager.c.
Thanks for the feedback, Jonas. As you pointed out, I've noticed too that these patches would require quite some changes to apply on top of master/3.26, which is the reason I've temporarily put them aside while rebasing the rest of our patches on top of mutter, in order to take care of these ones properly. Actually, for this particular set of patches, it's probably going to be Georges instead of me (or maybe the two of us) who work on rebasing them, so I'm CCing him here now to keep him in the loop. (@Georges: this is the upstream counterpart to our internal ticket T20655)
Of course, I forgot to add Georges to CC before submitting the comment... Doing it now (@Georges, please check Comment #29)
Thanks. I'll keep this in my list.
@Jonas: In the end, I'll be taking over this task from Endless' side. I've already spent some time yesterday and today studying the related changes and my plan is first to get them to work fine on top of the already-rebased-but-yet-not-in-production fork of mutter 3.26 I have on endless to help me understand it better, and then I'd ideally like to upstream as much as it's possible. In the meantime, if you want to provide any hint / clue that you consider might be useful, I'm all ears!
Heads-up on this: It turned out that porting our downstream patches on top of mutter 3.26 was not as simple as I thought it would (mostly due to me knowing nothing and having to spend a fair amount of time understanding how this worked in our 3.24 and how it would work now on 3.26), so I could not really have much proposed here yet. That said, at some point I was going to propose at least the first 4-5 patches (the ones attached here) of our downstream changes here, after having them working on 3.26, I realized they would need to be re-written again for master, since that's changed again and will need rewriting again. Thus, due to "work bandwith issues", I'm afraid I'll have to postpone working on this upstream at least until the next time we rebase the shell, likely in 6 months from now. Sorry about that
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.