Bug 482713

Bugs in DDC support, I suppose.

We don't really have control over the list of monitors that ddcutil tells us about. It's unclear to me why your monitor would sometimes be detected and sometimes not. Maybe ddcutil would always detect it after a while, but it doesn't wait long enough before reporting back the list of displays to Plasma's power management service. Not sure.

Chances are, once we react better to changes in detected monitors, this might get fixed in practice. For Plasma 6.1, we've already merged a patch that recognizes added or removed DDC monitors. More work is needed to hook this up to the Brightness and Color applet so the slider gets added and removed on the fly, without having to log in again to restart Plasma.

In the meantime now that I know DDC is a thing I've learned there is something called "ddcci-driver-linux" which provides a kernel module that apparently exposes DDC brightness control as typical brightness control so it just works everywhere

This problem doesn't seem to be present on Plasma 6.0.5 that comes with Manjaro

(In reply to rjawiygvozd from comment #4)
> This problem doesn't seem to be present on Plasma 6.0.5 that comes with
> Manjaro

Actually no, it still exists

I don't think it is my case, but with 6.1 I have issues when the monitor goes to sleep (DPMS). Probably, when it wakes up, Powerdevil tries to work with HDMI/DP i2c too early, and the monitor sometimes isn't yet ready to respond.

I have fixed(seem, still testing) it with this patch:
diff --git a/daemon/controllers/ddcutildisplay.cpp b/daemon/controllers/ddcutildisplay.cpp
index 1e1a274..09a5c93 100644
--- a/daemon/controllers/ddcutildisplay.cpp
+++ b/daemon/controllers/ddcutildisplay.cpp
@@ -49,7 +49,15 @@ DDCutilDisplay::DDCutilDisplay(DDCA_Display_Ref displayRef)
     // backed by libddcutil (like the ddcutil CLI itself) from functioning.
 
     DDCA_Display_Handle displayHandle = nullptr;
-    if (status = ddca_open_display2(m_displayRef, true, &displayHandle); status != DDCRC_OK) {
+    status = ~status;
+    for (int i = 0; i < 2 && status != DDCRC_OK; ++i) {
+        if (status = ddca_open_display2(m_displayRef, true, &displayHandle); status != DDCRC_OK) {
+            QThread::msleep(200);
+            continue;
+        }
+    }
+
+    if (status != DDCRC_OK) {
         qCWarning(POWERDEVIL) << "[DDCutilDisplay]: ddca_open_display2" << status;
         return;
     }

Just a quick fix, but it can potentially confirm my theory. So, maybe it is required to add some mechanism to handle this situations.

Nice, would you like to submit that upstream so the developers can take a look. Generally patches in Bugzilla tickets get overlooked.

Yes, sure. When I am certain it is working, I will make an MR. Today, I encountered the issue again with this patch, so the place I am fixing is probably wrong. Now, I have enabled more logs and am testing another fix.

(In reply to zvova7890 from comment #6)
> I don't think it is my case, but with 6.1 I have issues when the monitor
> goes to sleep (DPMS). Probably, when it wakes up, Powerdevil tries to work
> with HDMI/DP i2c too early, and the monitor sometimes isn't yet ready to
> respond.

Thanks for running this experiment. Might be related to Bug 476540, which is also seemingly related to DPMS. (My theory is that we should disable any brightness commands while DPMS has the monitor turned off, and then apply the last requested brightness after the monitor comes back.)

Could you check what the value of `status` is when ddca_open_display2() fails? ddcutil_status_codes.h doesn't have a "wait and try again" status code like EAGAIN, but I wonder if perhaps DDCRC_DPMS_ASLEEP (value -3030) serves a similar purpose in this case.

I'm getting -3029 error.

Created MR: https://invent.kde.org/plasma/powerdevil/-/merge_requests/393

Git commit 584cfdf0256bc7034be0b3fadf94f7b486597aa6 by Xaver Hugl.
Committed on 28/06/2024 at 13:17.
Pushed by zamundaaa into branch 'master'.

core: reload actions on brightness controller changes

Whether or not the brightness control action is supported depends on the currently
connected list of screens, which is dynamic and can't just be checked on startup.
To fix this, reload actions when devices get added or removed

M  +24   -27   daemon/powerdevilcore.cpp
M  +2    -2    daemon/powerdevilcore.h

https://invent.kde.org/plasma/powerdevil/-/commit/584cfdf0256bc7034be0b3fadf94f7b486597aa6

A possibly relevant merge request was started @ https://invent.kde.org/plasma/powerdevil/-/merge_requests/395

Git commit d91bc62fa6ff93ac62ded148fee3722deab41442 by Xaver Hugl.
Committed on 28/06/2024 at 15:00.
Pushed by zamundaaa into branch 'Plasma/6.1'.

core: reload actions on brightness controller changes

Whether or not the brightness control action is supported depends on the currently
connected list of screens, which is dynamic and can't just be checked on startup.
To fix this, reload actions when devices get added or removed

(cherry picked from commit 584cfdf0256bc7034be0b3fadf94f7b486597aa6)

Co-authored-by: Xaver Hugl <xaver.hugl@gmail.com>

M  +4    -0    daemon/controllers/screenbrightnesscontroller.cpp
M  +1    -0    daemon/controllers/screenbrightnesscontroller.h
M  +23   -27   daemon/powerdevilcore.cpp
M  +2    -2    daemon/powerdevilcore.h

https://invent.kde.org/plasma/powerdevil/-/commit/d91bc62fa6ff93ac62ded148fee3722deab41442

Git commit 220b6b9531b21bb3d2cc790c6a3718af3df7aa7b by Volodymyr Zolotopupov.
Committed on 02/07/2024 at 08:35 CET.
Pushed by jpetso into branch 'master'.

ddcutildisplay: give some time before changing brightness after the monitor resumes

M  +4    -1    daemon/controllers/ddcutildisplay.cpp

https://invent.kde.org/plasma/powerdevil/-/commit/220b6b9531b21bb3d2cc790c6a3718af3df7aa7b

And also cherry-picked into Plasma/6.1 (will be released with 6.1.2):
https://invent.kde.org/plasma/powerdevil/-/commit/0c084dcb444173273b60934af7bdb321d39dbf13

rjawiygvozd@gmail.com,

Could you please put this into /etc/environment and catch the situation when you have unavailable brightness controls?
QT_LOGGING_RULES="*powerdevil*.debug=true"

Then save the logs: journalctl -b -t org_kde_powerdevil > /tmp/powerdevil.log
... and attach here. 

Just wan't to understand what is going on.

*** Bug 489563 has been marked as a duplicate of this bug. ***

Created attachment 171302 [details]
powerdevil.log

(In reply to rjawiygvozd from comment #17)
> Created attachment 171302 [details]
> powerdevil.log

I had to restart the session multiple times before I replicated it but the last one has the problem.
This is Plasma 6.1.1

(In reply to rjawiygvozd from comment #18)
> (In reply to rjawiygvozd from comment #17)
> > Created attachment 171302 [details]
> > powerdevil.log
> 
> I had to restart the session multiple times before I replicated it but the
> last one has the problem.
> This is Plasma 6.1.1

Also I don't know if this is relevant but this monitor actually has a horrible startup time and whenever it goes from bios graphics mode or whatever it's called to a proper screen resolution it also spends at least a few seconds in the black screen, to the point that pc boots way before I can actually see anything

and this transition is also happening when it goes from sddm to plasma

It is probably due to a race condition between KWin mode setting up and Powerdevil DDC/CI work. DDC detection should probably also be delayed.

*** Bug 487959 has been marked as a duplicate of this bug. ***

Created attachment 173580 [details]
Log after connection with HDMI-HDMI and brightness slider was shown

Created attachment 173581 [details]
Log after connection with DP-HDMI and brightness slider was missing

I've also had similar odd behaviour so thought I'd record it here. For more than three years using KDE on Tumbleweed I had my monitor (HP M24f 1080p) plugged into my PC via the bundled HDMI cable, and was blissfully unaware that changing the brightness of a monitor from software was even possible. Suddenly – and I forget when exactly this was, but earlier this year – I noticed that a brightness slider had appeared next to the Night Control toggle. And it worked! I assumed that a Plasma update had added either the functionality or the compatibility with my monitor.

I don't remember it being only sporadically available, though I didn't use it all that much.

A short while ago, maybe a month or so but no idea really, I noticed that the option was gone. And it stayed gone, so I assumed it was a regression. I didn't think to report it at the time.

Now I got to wondering why it didn't work any more, did some googling, found this bug report. I yesterday switched to using a DP to HDMI cable rather than the old HDMI cable, and over the course of several reboots and logout/login cycles it doesn't seem to ever work with this cable.

Weirdly, when I now switch back to the old cable, or another HDMI-HDMI cable, the brightness control seems to consistently appear. Which would imply that maybe the new DP-HDMI cable can't carry the DCC signal, something that would be entirely unsurprising – but the thing is, it is only now when I'm fiddling around with it and swapping back and forth that the brightness control appears with the HDMI-HDMI cable, whereas at least for the past few weeks I am absolutely 100% certain that it wasn't. But now I can't reproduce that.

Another point is that if I swap from one cable to another during a session (i.e. the monitor is disconnected then reconnected via a different cable and port on the GPU) there are no new entries printed to the log, and the brightness slider either remains hidden or remains shown, but if I boot with HDMI-HDMI, get shown the slider, then switch to DP-HDMI, the slider is still shown but no longer has any effect (indeed, if I try to change the brightness the widget gets stuck and freezes).

Should it be relevant, the cable in question is this one: https://www.amazon.de/UGREEN-DisplayPort-Uni-Direktional-Multi-Screen-Kompatibel/dp/B07V3RYBVY . It's advertised specifically as being one-way only, but at least some information must flow from the monitor to the PC as it still picks up the model name for instance.

I've attached the log output of both the successful and failed cases. If I ever replicate the brightness slider failing to show with the HDMI-HDMI connection then I'll post a log of that, too.

Git commit 44e6922ae9f06a3e2fa3e7640be7eb32591cd579 by Jakob Petsovits.
Committed on 14/10/2024 at 22:36.
Pushed by jpetso into branch 'master'.

daemon: Retry failed DDC/CI reads and writes repeatedly

DDC/CI communication with monitors can be unreliable,
especially when the monitor is still in the process of
fully waking up. The moment that we receive a connection
or DPMS awake event may not be the moment that brightness
commands start working.

Prior to this commit, `DDCutilDisplay` protected itself against
this case with a drastic measure: if a read or write command
failed, the object would claim brightness controls as
unsupported. This error condition meant that we won't end up
with inconsistent state, but it also makes the monitor's
brightness slider go away (or fall back to software
brightness controls if KWin wants those instead).

This commit still uses the same failure mode, but will try
harder before we give up on the monitor altogether.
Both initialization (reading the initial brightness value)
and `setBrightness()` will now retry a few times if the
first attempt didn't work out. Subsequent retries will be
spaced out further, until we finally give up on that monitor.

To avoid exposing uninitialized `DDCutilDisplay` objects,
a list of pending displays now holds these objects while
they're waiting for another initialization attempt.

`setBrightness()` will only be performed once initialization
has succeeded, so the two operations are mutually exclusive.
We can reuse DDCutilDisplay's existing brightness delay timer
for both kinds of retry operations.
Related: bug 493329

M  +23   -11   daemon/controllers/ddcutildetector.cpp
M  +69   -15   daemon/controllers/ddcutildisplay.cpp
M  +8    -2    daemon/controllers/ddcutildisplay.h

https://invent.kde.org/plasma/powerdevil/-/commit/44e6922ae9f06a3e2fa3e7640be7eb32591cd579

Git commit 48453745b1cfb0fce66e16ec9ef8caf961e79677 by Jakob Petsovits.
Committed on 14/10/2024 at 23:04.
Pushed by jpetso into branch 'Plasma/6.2'.

daemon: Retry failed DDC/CI reads and writes repeatedly

DDC/CI communication with monitors can be unreliable,
especially when the monitor is still in the process of
fully waking up. The moment that we receive a connection
or DPMS awake event may not be the moment that brightness
commands start working.

Prior to this commit, `DDCutilDisplay` protected itself against
this case with a drastic measure: if a read or write command
failed, the object would claim brightness controls as
unsupported. This error condition meant that we won't end up
with inconsistent state, but it also makes the monitor's
brightness slider go away (or fall back to software
brightness controls if KWin wants those instead).

This commit still uses the same failure mode, but will try
harder before we give up on the monitor altogether.
Both initialization (reading the initial brightness value)
and `setBrightness()` will now retry a few times if the
first attempt didn't work out. Subsequent retries will be
spaced out further, until we finally give up on that monitor.

To avoid exposing uninitialized `DDCutilDisplay` objects,
a list of pending displays now holds these objects while
they're waiting for another initialization attempt.

`setBrightness()` will only be performed once initialization
has succeeded, so the two operations are mutually exclusive.
We can reuse DDCutilDisplay's existing brightness delay timer
for both kinds of retry operations.
Related: bug 493329


(cherry picked from commit 44e6922ae9f06a3e2fa3e7640be7eb32591cd579)

Co-authored-by: Jakob Petsovits <jpetso@petsovits.com>

M  +23   -11   daemon/controllers/ddcutildetector.cpp
M  +69   -15   daemon/controllers/ddcutildisplay.cpp
M  +8    -2    daemon/controllers/ddcutildisplay.h

https://invent.kde.org/plasma/powerdevil/-/commit/48453745b1cfb0fce66e16ec9ef8caf961e79677

Hey guys, the commit above is introducing repeated retries for PowerDevil's DDC/CI support. This applies to both initialization and setting brightness. Plasma 6.2.1 will have the change, please test if this fixes any issues for you and leave a comment in case something still isn't working.

Thanks!

Patch applied. I will test it.

(In reply to Julien Delquié from comment #29)
> Patch applied. I will test it.

Didn’t see anything strange, but patch doesn’t apply anymore against powerdevil v6.2.1.

(In reply to Julien Delquié from comment #30)
> Didn’t see anything strange, but patch doesn’t apply anymore against
> powerdevil v6.2.1.

PowerDevil in 6.2.1 already includes it (and a minor fix-up that also always uses repeated tries after monitor wake-up).

(In reply to Jakob Petsovits from comment #31)
> (In reply to Julien Delquié from comment #30)
> > Didn’t see anything strange, but patch doesn’t apply anymore against
> > powerdevil v6.2.1.
> 
> PowerDevil in 6.2.1 already includes it (and a minor fix-up that also always
> uses repeated tries after monitor wake-up).

Oh I remember you already told this in a previous comment, sorry!

This seems very similar to my findings  - https://bugs.kde.org/show_bug.cgi?id=494233

Some kind of regression has been implemented.
This bug was fixed for me, but it came back again a week or so ago.