GNOME Bugzilla – Bug 640237
GnomeRRConfig and GnomeRRCrtc use different validation routines
Last modified: 2012-06-07 18:37:01 UTC
In gnome_rr_config_applicable, gnome_rr_screen_get_ranges is queried for the min and max dimensions of the virtual screen. Those are multiplied together and the total number of pixels are checked to make sure that they fall within min_width * min_height <= width * height <= max_width * max_height In gnome_rr_crtc_set_config_with_time, however, the height and width are validated separately. This makes it possible for a configuration to come back as valid from gnome_rr_config_applicable, but be rejected by gnome_rr_crtc_set_config_with_time for being out of bounds. This is unfortunate, since gnome-settings-daemon specifically checks for out of bounds errors coming back from gnome_rr_config_applicable and tries to compensate for them. The attached patch will cause gnome_rr_config_applicable to validate each dimension independently, matching the behavior or gnome_rr_crtc_set_config_with_time.
Created attachment 179015 [details] [review] When validating GnomeRRConfigs, check each dimension independently
Federico: can you take a look at this patch?
Thanks for bringing this up; validating sizes is something that has already tripped us in the past. Originally, gnome_rr_config_applicable() indeed had a simple check like (min_width <= width <= max_width). However, we found out that this didn't work at all for rotated displays. The framebuffer size would stay the same, but the proposed/rotated size wouldn't fit in the (width, height) limit. It would *work* fine, however, if you just tested for the total number of pixels, instead of testing each dimension separately. See commit 8ba5e616c for where this happened. I *think* I've seen another bug that said that we should in fact check for the dimensions as well as the total number of pixels, but I can't find it now. Instead of changing crtc_assignment_new() like in your patch, I think we need to make gnome_rr_crtc_set_config_with_time() *not* do any such check itself. It doesn't have enough context, as it doesn't know the offsets that the other CRTCs will get, and thus the required size. What do you think? (As an aside, the code in gnome-settings-daemon basically does, "okay, I don't know if the configuration I generated will actually fit in the hardware's limitations, so I'll just chop off displays until they fit". It doesn't try to be smart; it's just about seeing how many displays it can turn on, in a left-to-right ordering, while still fitting in whatever the driver says it can do. It does the checking by calling gnome_rr_config_applicable(), which is already supposed to know the right way to check the sizes. So, I don't think g-s-d has a problem in this respect.)
The specific behavior that led me to chase down this patch involved the fglrx driver, which for unfathomable reasons always reports that its maximum screen size is 4096x4096. (Unless you override it in xorg.conf) If I had two monitors arranged side-by-side, the width was too much for the driver, but the area was within 4096 * 4096, so g-s-d tried to apply the change and failed. (I was unable to apply the same configuration with xrandr, so the driver seems to actually enforce that maximum) As it is right now, gnome_rr_config_applicable() uses crtc_assignment_new(), and without my patch would return TRUE when in fact it should return FALSE. I'm not sure what to do with gnome_rr_crtc_set_config_with_time(), though. In particular, it seems that removing the check would change a useful error (GNOME_RR_ERROR_BOUNDS_ERROR) into a less useful and more generic one (GNOME_RR_ERROR_RANDR_ERROR), and it doesn't look like X will tell you why the XRRSetCrtcConfig() call failed.
*** Bug 677474 has been marked as a duplicate of this bug. ***
I think max_width/max_height need to be big enough to accomodate rotation for rotations to succeed. My assumption is the change that was made to check total number of pixels papered over a different problem (perhaps a miscalculation of width/height). I talked to Federico about this and he said he's okay with reverting the patch, so I think that's the right course of action.
Created attachment 215870 [details] [review] Revert "Allow rotation if the virtual size has the correct number of pixels" This reverts commit 8ba5e616ceb10ae8f1138eaa550ec65742195b78. It makes the checking too liberal and allows things through that will ultimately fail.
Attachment 215870 [details] pushed as fe6fdee - Revert "Allow rotation if the virtual size has the correct number of pixels"