DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
ClosedPublic

Authored by dvogel on Nov 2 2017, 4:56 PM.

Details

Summary

Reworked & simplified based on feedback from DDCUtil author in D5381.
Fixed CMakeList to add definitions for use in the code.
Updated the code to use the the ddcutil 0.9.5 C API

Current limitations:

  • multiple DDC-capable monitors connected will be used, however only one slider is showed. All monitors will be affected with the same brightness value.
  • ddcutil has to be setup so that it can be used as user (1. load i2c-dev on boot, 2. udev rule to affect /dev/i2c-* nodes to the i2c user group, 3. add your user to the i2c group.)
Test Plan

Tested on X session seems stable
On Wayland, the capabilities enumeration seems a bit inconsistent. Fixed by using "the old capability testing method": test each parameter by trying to read its current value.
Tested with dell 2212HM alone, and also with Magedok t116 as second display on AMD gpu using Mesa.
Using OpenSuse Krypton.

Diff Detail

Repository
R122 Powerdevil
Lint
Lint Skipped
Unit
Unit Tests Skipped
dvogel created this revision.Nov 2 2017, 4:56 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 2 2017, 4:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Looking fine, cannot really comment, though. Without context makes it hard to review.

daemon/backends/upower/ddcutilbrightness.cpp
26

Use initializer:

DDCutilBrightness::DDCUtilBrightness()
    : m_usedVcp({0x10})
63

Coding style:

for (int iVcp = 0; iVcp < m_usedVcp.count(); ++iVcp) {
65

Use .value() instead of operator[] which is const

68–69

Coding style: Put on same line

} else {
84

Leave preprocessor directives unindented

160

Can m_supportedVcp_perDisp be empty? .at(0) will assert if so.

dvogel updated this revision to Diff 32187.Apr 15 2018, 2:55 PM
dvogel marked 5 inline comments as done.

Applied changes suggested by broulik.
CMake fixed, using the checkbox for WITH_DDCUTIL in Kdevelop now works.
I must appologise for the long time this is taking. The diff is based on 5.12.4, since 5.12.80 is not available on my system (Arch). I will try to rebase.

lgtm

CMakeLists.txt
58

we typically use HAVE_xxx but fine with me

backends/upower/ddcutilbrightness.h
27 ↗(On Diff #32187)

Doesn't this need to stay ifdef?

broulik accepted this revision.Apr 16 2018, 10:18 AM
This revision is now accepted and ready to land.Apr 16 2018, 10:18 AM
dvogel updated this revision to Diff 33105.Apr 25 2018, 8:43 PM

Applied changes to the CMakeList.
Somehow the previous diff was messed-up: downloading it would provide a diff with the wrong folder structure...

Hm, any reason why this is still pending?

ngraham added a subscriber: ngraham.Apr 3 2019, 9:06 PM

Probably because @dvogel doesn't have commit access and nobody ever landed the patch on his behalf. :( Feel free to do so now I guess, since @kbroulik has accepted it.

dvogel added a comment.Apr 4 2019, 5:33 AM

Probably because @dvogel doesn't have commit access and nobody ever landed the patch on his behalf. :( Feel free to do so now I guess, since @kbroulik has accepted it.

Yes, kind of. However now, the DDCUtil API changed quite a lot (only function prototypes AFAIK) since then. The main issue for me was that powerdevil only supports one brightnessController at the time. So as long as it does, the implementation of this feature will be doggy.

So does this patch need revision now?

dvogel added a comment.Apr 4 2019, 1:18 PM

Yes it has to be modified. I got it to compile again over lunch on a laptop, however could not test it (has to be on a desktop, since powerdevil picks integrated screens first for brightness).

dvogel updated this revision to Diff 55605.EditedApr 6 2019, 9:41 PM

I went a bit back to it: it is again in a working state with the current DDCUtil API.
Still, single slider independently of the number of displays supporting ddc connected.
Tested with my dell 2212hm and a Magedok t116 via HDMI on AMD gpu,

Could you be a bit more specific in the Title and Summary section regarding just what this does?

@broulik, still all good and ready to land?

dvogel retitled this revision from DDCUtil: Improved DDCUtil support for brightness control to DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors.Apr 7 2019, 8:49 AM
dvogel edited the summary of this revision. (Show Details)
dvogel edited the test plan for this revision. (Show Details)
dvogel edited the test plan for this revision. (Show Details)Apr 7 2019, 9:03 AM
dvogel updated this revision to Diff 55690.Apr 7 2019, 4:30 PM
dvogel marked an inline comment as done.

Reverted to the "old way" of trying ddcutil compatibility of the detected displays: by trying to read the feature setting. The ddca_get_feature_list_by_dref function was giving inconsistent results when used right after boot... (logging out an in again seemed to help, but not always..).

dvogel edited the test plan for this revision. (Show Details)Apr 8 2019, 7:00 AM
broulik accepted this revision.Apr 9 2019, 2:00 PM
broulik added inline comments.
CMakeLists.txt
77

Stray whitespace

daemon/backends/upower/ddcutilbrightness.cpp
38

nullptr

71

Remove commented code

82

Coding style: if (...) {

@dvogel I'll land this for you after you address those review comments.

dvogel updated this revision to Diff 55854.Apr 9 2019, 6:49 PM

Applied changes suggested by @broulik.
Thank you for your help.

This revision was automatically updated to reflect the committed changes.

Thanks @dvogel! Nice patch. May it be the second of many. :) For the next one, you might consider setting up arc, which makes submitting and managing patches easier for you, and reviewing and landing them easier for us. See https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

There's a build failure on CI now: https://build.kde.org/view/OS%20-%20Linux/job/Administration/job/Dependency%20Build%20Plasma%20kf5-qt5%20SUSEQt5.12/lastFailedBuild/console

CMake Error at daemon/backends/CMakeLists.txt:49 (add_library):
No SOURCES given to target: powerdevilupowerbackend

You had the files in the wrong place, I moved them, not it builds :)

add_compile_definitions requires cmake 3.13, so breaks build in Neon where cmake from Bionic is 3.10