Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12254041:
Reconfigure displays even if the output count didn't change (Closed)
|
Created:
7 years, 10 months ago by oshima Modified:
7 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReconfigure displays even if the output count didn't change
Remove obsolete STATE_DUAL_SECONDARY_ONLY
TBR=jamescook@chromium.org
BUG=chromium-os:12662
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182987
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #
Total comments: 4
Patch Set 3 : fixed comments #Patch Set 4 : rebase #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.cc:816: bool success = EnterState(display, screen, window, new_state, outputs); Does this mean that the output configuration happens even though it's not necessary? like, recover from suspend but no display configuration has changed? I suspect it will cause unnecessary display flickering or something like that in that case, right? Is there any way in early exit in EnterState? https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.cc:1321: UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.dual_primary_duration", change it to "Display.EnterState.extended" or something? https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.h:43: STATE_DUAL_EXTENDED, STATE_DUAL_PRIMARY_ONLY and _SECONDARY_ONLY are referred from ash/system/chromeos/tray_display.cc and chrome/browser/ui/webui/options/chromeos/display_options_handler.cc Please edit these files too. % git grep -l 'STATE_DUAL_\(PRIMARY\|SECONDARY\)_ONLY' ash/system/chromeos/tray_display.cc chrome/browser/ui/webui/options/chromeos/display_options_handler.cc chromeos/display/output_configurator.cc chromeos/display/output_configurator.h
https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.cc:816: bool success = EnterState(display, screen, window, new_state, outputs); On 2013/02/14 22:13:03, Jun Mukai wrote: > Does this mean that the output configuration happens even though it's not > necessary? like, recover from suspend but no display configuration has changed? > I suspect it will cause unnecessary display flickering or something like that in > that case, right? Is there any way in early exit in EnterState? Hmm, this wasn't the issue as there used to be no RRNotify_OutputChange event for resume, but I now see the RRNotify_OutputChange on resume. Stuart, is this something to do with your recent change? https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.cc:1321: UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.dual_primary_duration", On 2013/02/14 22:13:03, Jun Mukai wrote: > change it to "Display.EnterState.extended" or something? I won't touch UMA name as we have historical data. https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.h:43: STATE_DUAL_EXTENDED, On 2013/02/14 22:13:03, Jun Mukai wrote: > STATE_DUAL_PRIMARY_ONLY and _SECONDARY_ONLY are referred from > ash/system/chromeos/tray_display.cc and > chrome/browser/ui/webui/options/chromeos/display_options_handler.cc > Please edit these files too. > > % git grep -l 'STATE_DUAL_\(PRIMARY\|SECONDARY\)_ONLY' > ash/system/chromeos/tray_display.cc > chrome/browser/ui/webui/options/chromeos/display_options_handler.cc > chromeos/display/output_configurator.cc > chromeos/display/output_configurator.h Done.
On 2013/02/14 22:33:12, oshima wrote: > https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... > File chromeos/display/output_configurator.cc (right): > > https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... > chromeos/display/output_configurator.cc:816: bool success = EnterState(display, > screen, window, new_state, outputs); > On 2013/02/14 22:13:03, Jun Mukai wrote: > > Does this mean that the output configuration happens even though it's not > > necessary? like, recover from suspend but no display configuration has > changed? > > I suspect it will cause unnecessary display flickering or something like that > in > > that case, right? Is there any way in early exit in EnterState? > > Hmm, this wasn't the issue as there used to be no RRNotify_OutputChange event > for resume, but I now see the RRNotify_OutputChange on resume. > > Stuart, is this something to do with your recent change? Also it seems to me that resume takes longer than before, without this change.
The change of mine referred to is this https://gerrit.chromium.org/gerrit/#/c/42583/. I tried with this reverted, and it made no difference to the RRNotify_OutputChange events reported. I still see a sequence of RR_Connected/RR_Disconnected events when I resume with an external monitor. I get this sequence without my kernel change: OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected OutputConfigurator::ConfigureOutputs I get a similar sequence when I plug in an external monitor after booting.
On 2013/02/15 18:57:45, sabercrombie1 wrote: > The change of mine referred to is this > https://gerrit.chromium.org/gerrit/#/c/42583/. > > I tried with this reverted, and it made no difference to the > RRNotify_OutputChange events reported. I still see a sequence of > RR_Connected/RR_Disconnected events when I resume with an external monitor. > > I get this sequence without my kernel change: > > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Disconnected > OutputConfigurator::Dispatch RRNotify_OutputChange RR_Connected > OutputConfigurator::ConfigureOutputs > > I get a similar sequence when I plug in an external monitor after booting. Then something else might have regressed. At least, we shouldn't slow down the resume process. Let me double check with last week's image.
I should set the record straight on those events. I wasn't logged in, so I wasn't getting suspend/resume. With actual suspend/resume what I see with my kernel change is a lack of display change events on resume with or without an external monitor.
PTAL https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/12254041/diff/2001/chromeos/display/output_co... chromeos/display/output_configurator.cc:816: bool success = EnterState(display, screen, window, new_state, outputs); On 2013/02/14 22:33:12, oshima wrote: > On 2013/02/14 22:13:03, Jun Mukai wrote: > > Does this mean that the output configuration happens even though it's not > > necessary? like, recover from suspend but no display configuration has > changed? > > I suspect it will cause unnecessary display flickering or something like that > in > > that case, right? Is there any way in early exit in EnterState? > > Hmm, this wasn't the issue as there used to be no RRNotify_OutputChange event > for resume, but I now see the RRNotify_OutputChange on resume. > > Stuart, is this something to do with your recent change? Looks like this was temporary regression on TOT and it now get no OutputChange upon resume. This shouldn't cause any extra work as RRNotify_OutputChange does mean something has changed in outputs.
lgtm
LGTM aside from a couple of comments https://codereview.chromium.org/12254041/diff/14001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc (right): https://codereview.chromium.org/12254041/diff/14001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:183: // We use 'PRIMARY_ONLY' for non-mirroring state for now. Looks like this comment should be removed. https://codereview.chromium.org/12254041/diff/14001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (left): https://codereview.chromium.org/12254041/diff/14001/chromeos/display/output_c... chromeos/display/output_configurator.cc:423: // Default to primary only. This comment doesn't look accurate any more.
https://codereview.chromium.org/12254041/diff/14001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc (right): https://codereview.chromium.org/12254041/diff/14001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:183: // We use 'PRIMARY_ONLY' for non-mirroring state for now. On 2013/02/16 00:01:08, sabercrombie wrote: > Looks like this comment should be removed. Done. https://codereview.chromium.org/12254041/diff/14001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (left): https://codereview.chromium.org/12254041/diff/14001/chromeos/display/output_c... chromeos/display/output_configurator.cc:423: // Default to primary only. On 2013/02/16 00:01:08, sabercrombie wrote: > This comment doesn't look accurate any more. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/12254041/19001
Presubmit check for 12254041-19001 failed and returned exit status 1.
INFO:root:Found 4 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ash/system
Presubmit checks took 4.3s to calculate.
jamescook -> tbr'ing trivial change
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/12254041/19001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/12254041/8004
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/12254041/8004
Message was sent while issue was closed.
Change committed as 182987 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
