After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 727023 - Better error when additional output cannot be enabled
Better error when additional output cannot be enabled
Status: RESOLVED OBSOLETE
Product: gnome-control-center
Classification: Core
Component: Display
3.10.x
Other All
: Normal normal
: ---
Assigned To: Debarshi Ray
Control-Center Maintainers
: 728618 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-03-25 15:19 UTC by Debarshi Ray
Modified: 2021-06-09 16:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
display: make the Apply button unsensitive for invalid configuration (1.19 KB, patch)
2014-03-25 17:07 UTC, Giovanni Campagna
committed Details | Review
display: Remove redundant statement and variable definition (1.18 KB, patch)
2014-03-25 17:28 UTC, Debarshi Ray
committed Details | Review
display: Silence -Wmaybe-uninitialized (7.22 KB, patch)
2014-03-25 17:41 UTC, Debarshi Ray
committed Details | Review
gnome-rr: Initialize GnomeRROutputInfo:rotation to GNOME_RR_ROTATION_0 (1.66 KB, patch)
2014-03-26 14:39 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2014-03-25 15:19:51 UTC
If I plug in a second external monitor to my docking station (one builtin display and two externals), then the third output shows up as "off" in the Display panel. Which is fine because my hardware can only handle 2 outputs. However, I can still click the third output and try to enable it from the setup dialog. This does not do anything apart from sending me back to the panel.

I think we should somehow communicate to the user that enabling this output is not expected to work. Maybe by not showing the setup dialog, or by keeping the "Apply" insensitive in the dialog?

This is on a Lenovo x220 with Sandy Bridge graphics, but I am told that you can reproduce using a Lenovo T420 too.
Comment 1 Giovanni Campagna 2014-03-25 16:06:54 UTC
For the Sandy Bridge case, the HW only has two display pipes, and we have that information in user space (there are only two CRTCs). gnome-control-center already checks that the configuration is applicable before making the button sensitive, so it is supposed to work.
This could be a bug in gnome-control-center (maybe it lost the check?), gnome-desktop (maybe it's mistakenly cloning two monitors?) or mutter (maybe we expose one CRTC too many on the bus?).

OTOH, the T420 has an Ivybridge, which has 3 display pipes and 2 clock generators, so it can technically drive three monitors, if two of them use a link of the same type (ie, both HDMI or both DP) and the same mode (size and refresh).
Unfortunately, the current DRM API is too limited to expose this information, which is buried deep in the i915 code in the kernel, so there is nothing we can do in mutter or gnome-control-center to proactively inform the user.
Comment 2 Debarshi Ray 2014-03-25 16:57:22 UTC
(In reply to comment #1)
> gnome-control-center
> already checks that the configuration is applicable before making the button
> sensitive, so it is supposed to work.
> This could be a bug in gnome-control-center (maybe it lost the check?),

I doubt that it checked in the recent past. The bug was pointed out by the RHEL7 QA folks, which is GNOME 3.8 and had the older UI, and I can reproduce using GNOME 3.11.x.
Comment 3 Giovanni Campagna 2014-03-25 17:02:52 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > gnome-control-center
> > already checks that the configuration is applicable before making the button
> > sensitive, so it is supposed to work.
> > This could be a bug in gnome-control-center (maybe it lost the check?),
> 
> I doubt that it checked in the recent past. The bug was pointed out by the
> RHEL7 QA folks, which is GNOME 3.8 and had the older UI, and I can reproduce
> using GNOME 3.11.x.

I'm quite sure the old UI used to check, but yeah, the new one doesn't, so let's fix this one.
Comment 4 Giovanni Campagna 2014-03-25 17:07:50 UTC
Created attachment 272874 [details] [review]
display: make the Apply button unsensitive for invalid configuration

If the configuration is not applicable, due to HW constraints we
know about, make the button insensitive, to avoid an error dialog
later on.
Comment 5 Debarshi Ray 2014-03-25 17:25:10 UTC
Review of attachment 272874 [details] [review]:

Did you test this? It does not work for me. When I plug in the 3rd output and click on it, the Apply button stays insensitive. If I turn off the 2nd output in order to turn on the 3rd, then the Apply button does not come back. At that point I don't have a way to get back either the 2nd or 3rd output.
Comment 6 Debarshi Ray 2014-03-25 17:28:04 UTC
Created attachment 272877 [details] [review]
display: Remove redundant statement and variable definition
Comment 7 Debarshi Ray 2014-03-25 17:41:26 UTC
Created attachment 272885 [details] [review]
display: Silence -Wmaybe-uninitialized
Comment 8 Debarshi Ray 2014-03-26 11:10:55 UTC
Review of attachment 272874 [details] [review]:

What is happening here is that when GnomeRR tries to check whether a CRTC can be assigned to the inactive output (see crtc_assignment_assign), the rotation of the OutputInfo is GNOME_RR_ROTATION_NEXT. I am not familiar with the semantics of the GnomeRR API, but it looks to me that this is meant to be an invalid value, and none of the CRTCs claim to support it. Hence gnome_rr_config_applicable fails.

One way to fix this could be to ensure that OutputInfo->priv->rotation is initialized with GNOME_RR_ROTATION_0 during construction. The other could be to do that in gnome_rr_config_load_current.
Comment 9 Debarshi Ray 2014-03-26 14:39:22 UTC
Created attachment 273000 [details] [review]
gnome-rr: Initialize GnomeRROutputInfo:rotation to GNOME_RR_ROTATION_0

I decided to do it during contruction because that looked like the more sensible place to do it. With this patch and the one from Giovanni, things work as expected.
Comment 10 Giovanni Campagna 2014-03-26 17:28:09 UTC
(In reply to comment #8)
> Review of attachment 272874 [details] [review]:
> 
> What is happening here is that when GnomeRR tries to check whether a CRTC can
> be assigned to the inactive output (see crtc_assignment_assign), the rotation
> of the OutputInfo is GNOME_RR_ROTATION_NEXT. I am not familiar with the
> semantics of the GnomeRR API, but it looks to me that this is meant to be an
> invalid value, and none of the CRTCs claim to support it. Hence
> gnome_rr_config_applicable fails.

This makes no sense: if the output is inactive, we should not assign it to a CRTC (and indeed we don't, see real_assign_crtcs), so it doesn't matter what rotation it has.
Comment 11 Debarshi Ray 2014-03-27 13:10:36 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > Review of attachment 272874 [details] [review] [details]:
> > 
> > What is happening here is that when GnomeRR tries to check whether a CRTC can
> > be assigned to the inactive output (see crtc_assignment_assign), the rotation
> > of the OutputInfo is GNOME_RR_ROTATION_NEXT. I am not familiar with the
> > semantics of the GnomeRR API, but it looks to me that this is meant to be an
> > invalid value, and none of the CRTCs claim to support it. Hence
> > gnome_rr_config_applicable fails.
> 
> This makes no sense: if the output is inactive, we should not assign it to a
> CRTC (and indeed we don't, see real_assign_crtcs), so it doesn't matter what
> rotation it has.

TLDR: setup_listbox_row_activated in cc-display-panel.c

Every time you click on one of "primary", "secondary", "mirror", "turn off" in the GtkListBox the current GnomeRROutputInfo is activated depending whether "turn off" was chosen or not. So when gnome_rr_config_applicable is called after that (as in your first patch), it can check whether this output can actually be activated or not. Otherwise, as you noted, real_assign_crtcs will skip it and return TRUE instead of actually checking for applicability.
Comment 12 Giovanni Campagna 2014-04-01 20:59:39 UTC
Review of attachment 273000 [details] [review]:

Right, this makes sense.
Comment 13 Debarshi Ray 2014-04-02 11:22:35 UTC
Comment on attachment 273000 [details] [review]
gnome-rr: Initialize GnomeRROutputInfo:rotation to GNOME_RR_ROTATION_0

Thanks for the review, Giovanni.
Comment 14 Bastien Nocera 2014-04-02 12:33:00 UTC
Review of attachment 272877 [details] [review]:

++

Not redudant though, it's unused. Redundant means you're doing it somewhere else.
Comment 15 Bastien Nocera 2014-04-02 12:35:53 UTC
Review of attachment 272885 [details] [review]:

Meh. I wish we didn't have to reindent all of that, but...
Comment 16 Bastien Nocera 2014-04-22 06:21:14 UTC
*** Bug 728618 has been marked as a duplicate of this bug. ***
Comment 17 Debarshi Ray 2014-06-03 21:10:21 UTC
Review of attachment 272874 [details] [review]:

It should be OK to push this now that the accompanying gnome-rr patch has gone in.
Comment 18 Bastien Nocera 2014-06-04 17:21:46 UTC
I'll reopen this with the associated ui-review.
Comment 19 André Klapper 2021-06-09 16:10:34 UTC
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 bug report at
  https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/

Thank you for your understanding and your help.