Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 315371 Resurrect power manager's code for using DDC/CI to adjust external display's brightness
Starred by 4 users Project Member Reported by derat@chromium.org, Nov 6 2013 Back to list
Status: Verified
Owner:
Closed: Mar 2014
Cc:
Components:
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 349356


Sign in to add a comment
I should resurrect powerd's code for querying and setting an external display's brightness via DDC/CI. This code was briefly used for stumpy, but it was disabled (some monitors reported incorrect values, and the user experience was poor when a Chromebox was connected to a display that also had other video sources connected to it) and then it broke. I'll add tests when I bring it back and replace the usleep() calls with an asynchronous API.

I'll also update ExternalBacklightController to optionally support querying an external display's brightness at startup and adjusting it in response to brightness key events.

(I might rename BacklightController to BrightnessController while I'm at it.)
 
Comment 1 by derat@chromium.org, Dec 16 2013
Labels: -M-33 M-34
Comment 2 by derat@chromium.org, Feb 11 2014
Labels: -M-34 M-35
We don't need this yet.
Comment 3 by derat@chromium.org, Feb 26 2014
Labels: -Pri-2 Pri-1
Comment 4 by derat@chromium.org, Feb 27 2014
Here's my thinking for how this should work (adapted from email):

a) If present, the scaler should act reasonably irrespective of what Chrome does (e.g. restore the previous brightness when powered on, let the user adjust the brightness via the OSD, display the OSD whenever the brightness is changed, etc.).

b) When the user hits the brightness-up or brightness-down keys, powerd will set the new brightness level via DDC/CI.

c) If I'm not mistaken, powerd needs to set an absolute brightness level; it can't e.g. just say "increase the brightness by X". I'm planning to make powerd cache the brightness level (for the case where the user hits a key over and over) and only refresh it via DDC/CI if it's been several seconds since the last time it was set or fetched (to handle the case where the user has changed the brightness via the OSD in the meantime).

d) Chrome shouldn't display any UI about the external display's brightness; we'll let the display's OSD handle that.
Comment 5 by derat@chromium.org, Feb 27 2014
Status: Started
Comment 6 by derat@chromium.org, Feb 28 2014
Note to self: --ash-enable-brightness-control to enable these keys in Chrome.
Comment 7 by derat@chromium.org, Mar 5 2014
Blockedon: chromium:349356
Comment 8 by derat@chromium.org, Mar 6 2014
Cc: marc...@chromium.org
Labels: Iteration-101
Hooray, I have this working!* Now I need to write a bunch of tests.

* at least for the Lenovo and LG displays that I've connected to a stumpy
Comment 9 by derat@chromium.org, Mar 7 2014
Another note to self: Add metrics so I can see how often this fails.
Comment 10 by moch@chromium.org, Mar 7 2014
Cc: moch@chromium.org
Project Member Comment 11 by bugdroid1@chromium.org, Mar 11 2014
------------------------------------------------------------------------
r256176 | derat@chromium.org | 2014-03-11T10:08:11.826604Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/about_flags.cc?r1=256176&r2=256175&pathrev=256176
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/generated_resources.grd?r1=256176&r2=256175&pathrev=256176
   M http://src.chromium.org/viewvc/chrome/trunk/src/ash/system/chromeos/brightness/tray_brightness.cc?r1=256176&r2=256175&pathrev=256176

chromeos: Add external brightness control to chrome://flags.

Make --ash-enable-brightness-control settable via
chrome://flags to permit testing of DDC/CI-based brightness
adjustments of external displays in powerd.

Also add a bit of code to ensure that even if powerd
announces a brightness change via D-Bus, Chrome never
displays a bubble if there's no internal display connected.

BUG= 315371 

Review URL: https://codereview.chromium.org/191973004
------------------------------------------------------------------------
Comment 12 by derat@chromium.org, Mar 11 2014
Chrome OS changes for this (bugdroid seems broken):

https://chromium-review.googlesource.com/#/c/189023/
https://chromium-review.googlesource.com/#/c/189203/
https://chromium-review.googlesource.com/#/c/189440/
https://chromium-review.googlesource.com/#/c/189399/

The metrics that I'm adding to track success and failure in the new powerd code are:

  Power.ExternalBrightnessRequestResult
  Power.ExternalBrightnessReadResult
  Power.ExternalBrightnessWriteResult
  Power.ExternalDisplayOpenResult
Project Member Comment 13 by bugdroid1@chromium.org, Mar 12 2014
Project: chromiumos/platform/power_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 81f2efeb81c4a773a172dcf03743871ae61e73a4

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : I221b1edcfbca36f72282db086cc6697956953570
Reviewed-at    : https://chromium-review.googlesource.com/189440

power: Make metrics-sending code stubbable.

Rename MetricsReporter to MetricsCollector and add a new
singleton MetricsSender class that wraps the external
MetricsLibrary class that actually talks to Chrome (whew).

All metrics reporting is currently initiated by
MetricsCollector. Adding a singleton allows classes deeper
in the code to report metrics directly without needing to
propagate their state all the way up to MetricsCollector.

BUG=chromium:315371
TEST=ran existing tests; also checked via
     chrome://histograms that metrics are still reported

common/metrics_sender.cc
common/metrics_sender.h
common/metrics_sender_stub.cc
common/metrics_sender_stub.h
power_manager.gyp
powerd/daemon.cc
powerd/daemon.h
powerd/metrics_collector.cc
powerd/metrics_collector.h
powerd/metrics_collector_unittest.cc
Project Member Comment 14 by bugdroid1@chromium.org, Mar 12 2014
Project: chromiumos/platform/power_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 0b279dfea6b68aa38610e1fca3135189b52a2347

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : Ie690d9d896cfc26ae0b300082cf64bd8402be579
Reviewed-at    : https://chromium-review.googlesource.com/189203

power: Bring back code for adjusting external brightness.

This adds an ExternalDisplay class that can adjust an
external display's brightness via the DDC/CI protocol. It
also updates the ExternalBacklightController class to watch
for changes to the set of connected external displays and
adjust their brightness in response to user requests to
increase and decrease the screen brightness.

BUG=chromium:315371
TEST=added unit tests; also manually confirmed that
     brightness keys work on stumpy with various displays
     now after telling chrome to send requests

power_manager.gyp
powerd/daemon.cc
powerd/policy/external_backlight_controller.cc
powerd/policy/external_backlight_controller.h
powerd/policy/external_backlight_controller_unittest.cc
powerd/system/display/external_display.cc
powerd/system/display/external_display.h
powerd/system/display/external_display_unittest.cc
Project Member Comment 15 by bugdroid1@chromium.org, Mar 12 2014
Project: chromiumos/platform/power_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 34341a32afcb2d202cc3cf1d45e13efb8348b365

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : I56731b97d25935e816dfc7787c388a51a3e7ce74
Reviewed-at    : https://chromium-review.googlesource.com/189023

power: Add DisplayWatcher and DisplayInfo.

Add a new DisplayWatcher class that can be used to learn
about changes to connected displays. This replaces
Input::IsDisplayConnected() and also returns the I2C device
that can be used to communicate with the display, which is
needed to control external displays' brightness levels.

Also create a new powerd/system/display subdirectory and
move DisplayPowerSetter there.

BUG=chromium:315371
TEST=manual; also added unit tests

power_manager.gyp
powerd/daemon.cc
powerd/daemon.h
powerd/policy/external_backlight_controller.cc
powerd/policy/external_backlight_controller_unittest.cc
powerd/policy/input_controller.cc
powerd/policy/input_controller.h
powerd/policy/input_controller_unittest.cc
powerd/policy/internal_backlight_controller.cc
powerd/policy/internal_backlight_controller_unittest.cc
powerd/system/display/display_info.cc
powerd/system/display/display_info.h
powerd/system/display/display_power_setter.cc
powerd/system/display/display_power_setter.h
powerd/system/display/display_power_setter_stub.cc
powerd/system/display/display_power_setter_stub.h
powerd/system/display/display_watcher.cc
powerd/system/display/display_watcher.h
powerd/system/display/display_watcher_observer.h
powerd/system/display/display_watcher_stub.cc
powerd/system/display/display_watcher_stub.h
powerd/system/display/display_watcher_unittest.cc
powerd/system/input.cc
powerd/system/input.h
powerd/system/input_interface.h
powerd/system/input_stub.cc
powerd/system/input_stub.h
powerd/system/input_unittest.cc
tools/get_powerd_initial_backlight_level.cc
Project Member Comment 16 by bugdroid1@chromium.org, Mar 14 2014
Project: chromiumos/platform/power_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 7e40fcc9c5446d761bbedc0b0c6f8ab8b5252bc1

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : I8f019ff84842bc33fb004bf34ec1b5b2c0cb1d79
Reviewed-at    : https://chromium-review.googlesource.com/189399

power: Add metrics for external brightness operations.

Add histogram metrics recording the results of communication
with external displays via DDC/CI:

  Power.ExternalBrightnessRequestResult
  Power.ExternalBrightnessReadResult
  Power.ExternalBrightnessWriteResult
  Power.ExternalDisplayOpenResult

Also make user-requested adjustments of external displays be
reported via the Power.UserBrightnessAdjustmentsPerSession
metric (formerly only used for internal displays).

BUG=chromium:315371
TEST=added tests; also manually checked via
     chrome://histograms that metrics are reported after
     adjusting an external display's brightness

common/metrics_constants.cc
common/metrics_constants.h
power_manager.gyp
powerd/daemon.cc
powerd/metrics_collector.cc
powerd/metrics_collector_unittest.cc
powerd/policy/external_backlight_controller.cc
powerd/policy/external_backlight_controller.h
powerd/policy/external_backlight_controller_unittest.cc
powerd/system/display/external_display.cc
powerd/system/display/external_display.h
powerd/system/display/external_display_unittest.cc
Project Member Comment 17 by bugdroid1@chromium.org, Mar 18 2014
Project: chromiumos/platform/power_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 05bb3d1932d94fed84c75a8944bc6cf5614129fd

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : I86898634e75c0e4af6eef7c11634715cecb286f6
Reviewed-at    : https://chromium-review.googlesource.com/190268

power: Add --decrease and --increase to backlight_dbus_tool.

This makes it easier to test the external display
brightness-adjustment code.

BUG=chromium:315371
TEST=manual

tools/backlight_dbus_tool.cc
Project Member Comment 18 by bugdroid1@chromium.org, Mar 18 2014
Project: chromiumos/platform/login_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : 98bb989a731b2c327f2ebccdda9d04b3bc24222a

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : Ia71793b7a8e2c1eebb924550870b2971b693e6cc
Reviewed-at    : https://chromium-review.googlesource.com/190122

session_manager: Enable brightness control for monroe.

Pass the --ash-enable-brightness-control flag to Chrome when
running on monroe.

BUG=chromium:315371
TEST=none

session_manager_setup.sh
Comment 19 by derat@chromium.org, Mar 28 2014
Status: Fixed
Project Member Comment 20 by bugdroid1@chromium.org, Apr 1 2014
Project: chromiumos/platform/login_manager
Branch : master
Author : Daniel Erat <derat@chromium.org>
Commit : fade0c66c27cb843d274041db0cfcb9a3b7703fd

Code-Review  0 : Daniel Erat, chrome-internal-fetch
Code-Review  +2: Chris Masone
Commit-Queue 0 : Chris Masone, chrome-internal-fetch
Commit-Queue +1: Daniel Erat
Verified     0 : Chris Masone, chrome-internal-fetch
Verified     +1: Daniel Erat
Change-Id      : Ife0423fdce48a2334842ba56676d6facdf05734e
Reviewed-at    : https://chromium-review.googlesource.com/192138

session_manager: Remove --ash-enable-brightness-control.

This behavior that was enabled by this flag is on-by-default
now.

BUG=chromium:315371
TEST=none

session_manager_setup.sh
Status: Verified
What command would I use at the Crosh shell on my Chromebook to decrease the brightness on my external monitor? 
Sign in to add a comment