GNOME Bugzilla – Bug 744544
monitor-manager-kms: Add common modes
Last modified: 2016-09-12 18:08:59 UTC
This patch is the result of me trying to get mirror mode working. I noticed that under X the outputs always had more modes available than on kms and that's why g-c-c's displays panel wasn't able to setup mirroring because we need the same mode available on both outputs for it to work. So, I basically adapted the relevant code from the X server's modesetting driver to add and prune the modes for an output although there are some checks that I'm not sure about and left them as FIXMEs for now. On X, the standard modes are generated at build time but I'm not sure if we want to go that way and if yes, can we just copy and adapt the same awk script that's used there? Comments welcome.
Created attachment 296849 [details] [review] monitor-manager-kms: Add default vesa or otherwise standard modes Some output devices don't advertise all the modes they can actually display. In most cases, standard vesa modes are accepted by such devices so we should add them to the pool of available modes like xfree86 DDXen do. This is particularly relevant for projectors because, often, the only matching mode, required for mirroring, supported by both attached outputs is one of these standard modes.
Review of attachment 296849 [details] [review]: I feel like DRM should do something like this in drm_mode.c upstream. Is that not possible?
(In reply to Jasper St. Pierre from comment #2) > I feel like DRM should do something like this in drm_mode.c upstream. Is > that not possible? You mean in the kernel? Would have to ask the kernel folks. This surely looks like something that could be shared with other compositors though.
*** Bug 756791 has been marked as a duplicate of this bug. ***
Review of attachment 296849 [details] [review]: ::: src/backends/native/meta-monitor-manager-kms.c @@ +273,3 @@ +static float +drm_mode_vrefresh (const drmModeModeInfo *mode) Hm, do we really not have this in userspace anywhere? Ugh. @@ +301,3 @@ + meta_mode->height = mode->vdisplay; + meta_mode->refresh_rate = drm_mode_vrefresh (mode); + meta_mode->driver_private = g_slice_dup (drmModeModeInfo, mode); Sort of sucks that we're slice_dup'ing the whole thing when we already have it in static memory, but oh well. @@ +373,3 @@ + } + + max_vrefresh = MAX (max_vrefresh, 60.0); What about 50Hz displays? @@ +374,3 @@ + + max_vrefresh = MAX (max_vrefresh, 60.0); + max_vrefresh *= (1 + 0.01); This needs a comment. @@ +402,3 @@ + + g_free (meta_output->modes); + meta_output->modes = new_modes; Silly question, but why are we tracking manager->modes if we don't seem to ever use it, and are now putting modes which don't exist in manager->modes in the output mode array? I think we should just get rid of manager->modes in that case. @@ +475,3 @@ + manager->modes[i].mode_id = i; + meta_mode_from_drm_mode (&manager->modes[i], + &meta_default_drm_mode_infos[i - g_hash_table_size (modes)]); This is a bit ugly. Mind having a separate index? for (int j = 0; j < G_N_ELEMENTS (...); j++)
Can we get this fixed up and merged ? I'm noticing my laptop panel shows just a measly single 1600x900 under Wayland, when X has all these nice modes to offer.
I think we should add more than the vesa modes. For instance we should add 720p, 1600x900 and 1080p on 16:9 monitors as well.
Talking to Rob Clark, there is a 'scaling mode' property on connectors which, if present, is a pretty good indicator that the device can do the necessary scaling to support non-native modes.
*** Bug 758402 has been marked as a duplicate of this bug. ***
(In reply to Jasper St. Pierre from comment #5) > +drm_mode_vrefresh (const drmModeModeInfo *mode) > > Hm, do we really not have this in userspace anywhere? Ugh. I couldn't find anything in libdrm. The logic here is mostly from xorg's xf86Modes.c now with Daniel Stone's changes to improve precision. > + meta_mode->driver_private = g_slice_dup (drmModeModeInfo, mode); > > Sort of sucks that we're slice_dup'ing the whole thing when we already have > it in static memory, but oh well. Well, the real modes we get from the kernel aren't static memory and this has to work for those too. > + max_vrefresh = MAX (max_vrefresh, 60.0); > > What about 50Hz displays? That's a good question, I just got this from xorg's modesetting driver. I suppose we might not even need this check and just go with the max refresh rate we already determined before from the output's real modes? Need to ask an expert about this. I'm keeping it for now with a comment. > + max_vrefresh *= (1 + 0.01); > > This needs a comment. I changed it to the define as it exists in xorg's xf86str.h > Silly question, but why are we tracking manager->modes if we don't seem to > ever use it, and are now putting modes which don't exist in manager->modes > in the output mode array? manager->modes is the array of all available MetaMonitorModes which are orthogonal to outputs (we could even have an API for users to manually add modes like the X server allows). We then associate the MetaMonitorModes that fit an output to that MetaOutput. That's the MetaOutput->modes array but note that these are just pointers to the structs in the manager->modes array. I think the code was structured this way because it nicely fits the DisplayConfig DBus API which also has a global pool of outputs and then for each output has an array of mode IDs referencing the modes in the global pool. > This is a bit ugly. Mind having a separate index? Sure.
Created attachment 319044 [details] [review] monitor-manager-kms: Add default vesa and other standard modes Some output devices don't advertise all the modes they can actually display. In most cases, standard vesa modes are accepted by such devices so we should add them to the pool of available modes like xfree86 DDXen do. This is particularly relevant for projectors because, often, the only matching mode, required for mirroring, supported by both attached outputs is one of these standard modes.
i'd still much rather see the 16:9 modes than 640x350 and 1792x1344 but I guess we should fix xorg first, since we're copying from it.
Has this been forgotten ? Can we revive it ?
(In reply to Matthias Clasen from comment #13) > Has this been forgotten ? Can we revive it ? It'll need to be rebased, and I'm afraid it will have plenty of conflicts. The code it changes has been restructured quite a bit.
Created attachment 334811 [details] [review] monitor-manager-kms: Add common modes Some output devices only advertise their preferred mode even though they're able to display others too. This means we can include some common modes in each output's supported list. This is particularly important for mirroring, since we can only mirror outputs which are using the same resolution.
I added a small python script to generate the modes array but I opted to include the generated file too to avoid adding a build time dependency on the cvt tool. I don't expect we'll be changing the modes list too much so it shouldn't be a maintenance burden. Also, instead of using X's list, I just came up with what I think is a reasonable list of common modes for common hardware out there nowadays.
Review of attachment 334811 [details] [review]: ::: src/backends/native/meta-monitor-manager-kms.c @@ +668,3 @@ static void +add_common_modes (MetaMonitorManager *manager, + MetaOutput *meta_output) nit: meta_output -> output (cleaned that up in the rest of the file a while ago) @@ +702,3 @@ + mode->vdisplay >= preferred->vdisplay && + drm_mode_vrefresh (mode) >= drm_mode_vrefresh (preferred)) + continue; These conditions seems a bit confusing to me. Reading them out loud its * skip if the size is larger than the max size of known modes on any axis * skip if the refresh rate is higher than max known refresh rate these two makes sense; don't add modes that doesn't seem like they'll be supported. Then comes the last one: * skip if the size is larger than preferred on *both* axis *and* refresh rate is higher than preferred How can this one ever happen? It seems like the first two should always cover this case? @@ +709,3 @@ + new_modes = g_new0 (MetaMonitorMode *, meta_output->n_modes + array->len); + memcpy (new_modes, meta_output->modes, meta_output->n_modes * sizeof (MetaMonitorMode *)); + memcpy (new_modes + meta_output->n_modes, array->pdata, array->len * sizeof (MetaMonitorMode *)); You should be able to do meta_output->modes = g_renew (MetaMonitorMode *, meta_output->n_modes + array_len); and then skip the one of the memcpy:s as well as the free and variable switch.
Created attachment 334900 [details] [review] monitor-manager-kms: Add common modes -- (In reply to Jonas Ådahl from comment #17) > nit: meta_output -> output (cleaned that up in the rest of the file a while > ago) fixed > These conditions seems a bit confusing to me. Reading them out loud its > > * skip if the size is larger than the max size of known modes on any axis > > * skip if the refresh rate is higher than max known refresh rate > > these two makes sense; don't add modes that doesn't seem like they'll be > supported. > > Then comes the last one: > > * skip if the size is larger than preferred on *both* axis *and* refresh > rate is higher than preferred > > How can this one ever happen? It seems like the first two should always > cover this case? Yeah, I mostly got this from the modesetting X driver but it really doesn't make much sense, I guess the intent was just to avoid adding a common mode that's exactly the same as the preferred (note the equal comparison) but in that case why not check that for all modes? That would make this operation O(n^2) though so I guess we can just ignore that and leave any similar mode filtering to the UI in g-c-c. > @@ +709,3 @@ > You should be able to do > > meta_output->modes = g_renew (MetaMonitorMode *, meta_output->n_modes + > array_len); > > and then skip the one of the memcpy:s as well as the free and variable > switch. sure
*** Bug 768605 has been marked as a duplicate of this bug. ***
Review of attachment 334900 [details] [review]: LGTM
Pushing with a bit more autofoo to regen the header if the script changes as long as the cvt tool is in the path. Attachment 334900 [details] pushed as 9a07607 - monitor-manager-kms: Add common modes