Skip to content

Backlight: AIO kernel route and apple-panel-bl priority - #12267

Open
Chessing234 wants to merge 3 commits into
omacom:quattrofrom
Chessing234:fold/backlight-aio-apple
Open

Chessing234 wants to merge 3 commits into
omacom:quattrofrom
Chessing234:fold/backlight-aio-apple

Conversation

@Chessing234

@Chessing234 Chessing234 commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Same changes as #8053 #8131 , folded so review is one place.

Test plan

  • Spot-check each folded PR's test plan still applies

Fixes #8125

@llstrk

llstrk commented Sep 25, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Two conditions let the new all-in-one fallback adjust the built-in panel when a different display was targeted. Both are described below.

Verified: The apple-panel-bl priority change, the new DDC exit-status plumbing and the all-in-one fallback for a single built-in panel behave as described. Evidence comes from repository tests plus scenario probes with mocked ddcutil/brightnessctl and synthetic DRM/backlight trees:

Scenario Base Head
All-in-one, built-in DP-1 not listed by ddcutil detect, read exit 1 40 from the kernel backlight
Same, 60% exit 1 brightnessctl -d <backlight> set 60%
Docked all-in-one, external DP-2 answers DDC DDC DDC
Laptop (connected eDP), DDC-less external, read / +5% / 50% exit 1 exit 1, no backlight write
Internal eDP-1, Apple display, no focused monitor unchanged unchanged
hw-display: 228600000.dsi.0 + apple-panel-bl 228600000.dsi.0 apple-panel-bl
  • Status 3 propagates from find_bus through read_brightness to every call site. Replacing read ... < <(read_brightness) || exit 1 with a command substitution is what lets the status through. A found bus with a failing getvcp/setvcp still returns 1 and does not fall back.
  • No other script or QML file calls omarchy-brightness-display-ddc directly.
  • All new test cases pass on head. Against base bin/, the first new all-in-one case and the apple-panel-bl-versus-DSI case fail. The remaining new cases behave the same on base and head and act as regression guards.

Fallback ignores which display was targeted

backlight_owns_named_monitor checks only that the machine has no connected eDP/LVDS/DSI connector. It does not check which monitor was named. On an all-in-one with a second display that ddcutil detect does not list, requests for that second display go to the built-in panel's backlight:

DRM:        card1-DP-1 connected (built-in), card1-HDMI-A-1 connected (external)
detect:     lists neither connector            (synthetic fixture)

--monitor HDMI-A-1 +5%   base: exit 1    head: brightnessctl -d acpi_video0 set 45%, exit 0
--monitor HDMI-A-1 30%   base: exit 1    head: brightnessctl -d acpi_video0 set 30%
--monitor HDMI-A-1       base: exit 1    head: prints the built-in panel's value

On nouveau, this can also affect a DDC-capable external DP monitor, according to source reading of ddcutil 3.0.2 and Linux (not tested on hardware). nouveau names each DP AUX I2C adapter after its connector (for example DP-2). ddcutil skips nouveau adapters not named nvkm-*, so under that normal topology neither the built-in nor an external DP connector gets a bus from detect.

Impact: Brightness keys and the Display panel slider for the external display change the built-in panel instead, and the panel shows the built-in panel's value for the external monitor. This is the wrong-screen effect the laptop guard is meant to prevent. Before this PR, these requests failed.

Suggested change: Only fall back when the named monitor can be tied to the backlight. For example, fall back when it is the only connected non-internal DRM connector, or the only connected connector that detect did not list. Otherwise keep failing. A docked fixture with a second display missing from detect, asserting a nonzero exit and no brightnessctl set call, would cover this.

A failed ddcutil detect is reported as "no DDC display"

detect_bus runs ddcutil ... detect --brief 2>/dev/null | awk ... without pipefail, so its status is awk's, which is 0. When ddcutil exits nonzero or is missing, the output is empty and find_bus caches unavailable and returns 3. ddcutil detect also exits 0 when it finds nothing.

ddcutil exits 1 or 127, no output
  -> detect_bus status 0 (awk), empty bus
  -> cache "unavailable", return 3        (base: 1)
  -> no internal connector -> kernel backlight write
  -> repeated for 60 s from the cache

In a synthetic docked all-in-one probe, a detect failure sent +5% for the external DP-2 to brightnessctl -d acpi_video0 set 45%. The same connector answers DDC when detect succeeds.

Impact: A transient detect failure becomes a write to the wrong display for up to a minute on machines without an internal connector. The 60-second negative cache existed before, but the fallback now acts on it.

Suggested change: Check PIPESTATUS[0] (or set pipefail locally) in detect_bus. When ddcutil itself fails, return 1 without writing the unavailable cache, so that 3 only means that ddcutil ran and did not list the connector.

Scope note: The fallback triggers only when detect does not list the connector. According to ddcutil 3.0.2 source, a connector whose EDID ddcutil can read (from sysfs on i915, xe, amdgpu and radeon) is listed even without DDC/CI support. Its getvcp then fails with status 1, which this PR deliberately does not fall back on. A built-in DP panel on those drivers may therefore still fail as before. This was not tested on hardware. ddcutil --skip-ddc-checks detect --brief output from an affected machine would show which case applies.

Documentation note: 228600000.dsi.0 is the Apple Silicon Touch Bar's backlight (Asahi panel-summit; tiny-dfr manages the Touch Bar under that name), not a node without visible effect. The bin/omarchy-hw-display comment could say so. The gmux_backlight + apple-panel-bl and 228600000.dsi.0 + appletb_backlight test cases pair devices from different platforms, so they check ordering only. The last one expects the Touch Bar node to be chosen as the display backlight.


Review information

Test scope: Pinned head 5fb88b8 against base 9c5482c. Repository tests and about 30 scenario probes ran in an isolated sandbox with mocked ddcutil, brightnessctl, Hyprland helpers and OSD, plus synthetic DRM and backlight trees. Dependency behavior comes from reading source: ddcutil 3.0.2, Linux 7.2, the Asahi kernel (7.1 series), tiny-dfr and Hyprland 0.56.2. No real all-in-one, Apple Silicon machine, DDC bus or backlight was tested.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

@Chessing234

Copy link
Copy Markdown
Contributor Author

addressed the review: backlight fallback only when the named monitor is the unique connected connector missing from ddcutil detect.

@Chessing234
Chessing234 force-pushed the fold/backlight-aio-apple branch from 5fb88b8 to 6ad4296 Compare September 25, 2026 15:40
@omarchybot omarchybot added bug Something isn't working compatibility Make Omarchy work better on everything! labels Sep 27, 2026
@llstrk

llstrk commented Sep 27, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Follow-up to the earlier review and the author's reply, checked at 6ad4296.

Verified: The wrong-display fallback is fixed for the reported case, and a failed ddcutil detect no longer moves the built-in panel for another display. The scope note and the documentation note from the earlier review still apply. There is one new cost: each fallback check now runs an extra ddcutil detect.

The rebase kept the first two commits unchanged. Only 6ad4296 is new, and it touches only bin/omarchy-brightness-display and its test.

Earlier findings

Earlier finding Status at 6ad4296
Fallback ignores which display was targeted Resolved for the reported topology
Failed ddcutil detect reported as "no DDC display" Wrong-display effect resolved; helper unchanged
Nouveau sub-claim in the first finding Unverified, and a closer source reading points the other way
Scope note: built-in panel listed by detect never falls back Still present
Documentation note: 228600000.dsi.0 is the Touch Bar Still present

Verified, fallback now checks the named display. The fallback applies only when the named monitor is the only connected DRM connector that detect does not list, as the reply states. Synthetic all-in-one with DP-1 (built-in) and HDMI-A-1, both connected and neither listed by detect, backlight acpi_video0:

                             base     5fb88b8                 6ad4296
--monitor HDMI-A-1 +5%       exit 1   set acpi_video0 45%     exit 1, no write
--monitor HDMI-A-1 30%       exit 1   set acpi_video0 30%     exit 1, no write
--monitor HDMI-A-1 (read)    exit 1   prints 40               exit 1

The new repository test catches this. It passes on 6ad4296 and fails against the bin/ of 5fb88b8. Hyprland names that are not DRM connectors (HEADLESS-1, FALLBACK) also no longer fall back. On 5fb88b8 they returned the built-in panel's value. One narrower case remains: the rule assumes the built-in panel is the connector detect misses. When detect lists the built-in panel and misses exactly one other connected output, that output still drives the built-in backlight in a synthetic probe. No common hardware producing that combination was identified.

Verified, failed detect. detect_bus in bin/omarchy-brightness-display-ddc is unchanged, so a failing or missing ddcutil still gives status 3 and a 60-second unavailable cache. The fallback now runs its own detect, though. When that also fails, every connected connector counts as unlisted. A docked all-in-one then has two and does not fall back. In a synthetic docked probe with ddcutil exiting 1 or 127, --monitor DP-2 +5% exits 1 with no backlight write (on 5fb88b8 it set acpi_video0 to 45%). A single-display all-in-one still falls back, and the only display is the correct target there. Checking PIPESTATUS[0] in detect_bus would still stop a failed probe from being cached as "no DDC display", but it is no longer needed to prevent writes to the wrong display.

Correction, nouveau sub-claim. The earlier review said that ddcutil skips nouveau DP AUX adapters, so external DP monitors on nouveau would not be listed. A closer reading of Linux 7.2 and ddcutil 3.0.2 suggests the opposite. nouveau also registers nvkm-* adapters (nvkm/subdev/i2c/auxch.c, bus.c), ddcutil's name filter keeps nvkm-* adapters (sysfs_simple.c), and nouveau does not appear to link its connectors to a bus, so connectors can then be matched by EDID. If so, nouveau displays are listed. This is not tested on hardware. It no longer affects wrong-display writes, because two unlisted connectors now block the fallback.

Built-in panel listed by detect still gets no fallback (scope note, unchanged)

The fallback only runs on status 3, when detect does not list the connector. In ddcutil 3.0.2, detect reports every bus with a readable EDID (ddc_displays.c). On i915, xe, amdgpu and radeon it can read that EDID from sysfs. A built-in DP panel with a readable EDID is therefore listed, its getvcp fails with status 1, and brightness fails as before. Given the nouveau reading above, this may include the iMac that the all-in-one route was written for. That is inferred from source only. ddcutil --skip-ddc-checks detect --brief output from an affected machine would show which case applies.

Extra ddcutil detect on every fallback check

backlight_owns_named_monitor runs ddcutil --skip-ddc-checks detect --brief itself, without the helper's 60-second cache. Across three reads in a row on a synthetic single-display all-in-one, detect ran 2, 1 and 1 times on 6ad4296, against 1, 0 and 0 on 5fb88b8. Laptops and machines without a backlight device return before this check.

Impact: On an all-in-one where the fallback applies, every brightness key press and every 5-second poll of the open Display panel now runs a full detect. Brightness key repeats that arrive while it runs are dropped by the script's flock -n. Timing was not measured.

Suggested change: Cache the listed-connector set next to the helper's unavailable marker, or have omarchy-brightness-display-ddc report it, instead of running detect again.

Trade-off note: A docked all-in-one whose external display is also missing from detect now gets no fallback for its built-in panel either (--monitor DP-1 exits 1; 5fb88b8 printed 40). This fails safe, and base failed there too. The docked repository test covers only an external display that detect lists.

Documentation note (unchanged): 228600000.dsi.0 is the Apple Silicon Touch Bar backlight (Asahi panel-summit), not a node without visible effect. The last new hw-display test still expects it to be chosen over appletb_backlight. Open #13362 and #9835 also add apple-panel-bl priority, but they exclude 228600000.dsi.0 and test that it is never chosen. Their bin/omarchy-hw-display change conflicts textually with this PR's comment above the same candidate list, and their test contradicts this PR's last case. Whichever lands second will need to reconcile the two.

Related PR: Draft #13286 replaces the body of bin/omarchy-brightness-display with exec omarchy-media brightness display "$@". At omarchy-media 660d541, every non-internal monitor other than an Apple display goes to DDC with no backlight fallback, and its backlight picker has no apple-panel-bl entry. If both land, the all-in-one route and the apple-panel-bl priority would need porting there.


Review information

Test scope: Pinned head 6ad4296 against base d201fb9, compared with the earlier head 5fb88b8. Repository tests and scenario probes ran in an isolated sandbox with mocked ddcutil, brightnessctl, Hyprland helpers and OSD, plus synthetic DRM and backlight trees. Dependency behavior comes from reading source: ddcutil 3.0.2, Linux 7.2, the Asahi kernel, tiny-dfr, aquamarine 0.15.1 and omarchy-media 660d541. No real all-in-one, iMac, Apple Silicon machine, DDC bus or backlight was tested.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at 6ad4296be97879e0e7724180525df2e17530c634, against quattro (d201fb9, which has not touched these files since). The apple-panel-bl half holds up. The all-in-one half probably does not engage on the reported iMacs, and in the topology those reports describe it can move the built-in panel when another display was asked for.

What ran. On a disposable Omarchy worker: brightness-display-test.sh (18), hw-display-test.sh (12), monitor-state-test.sh and brightness-display-apple-cache-test.sh all pass. ./test/cli has one failure, vscode generated theme references current theme file, which fails the same way on the base and is not from this change. I also ran my own probe with mocked ddcutil/brightnessctl and a synthetic DRM tree against the base, the head, and the head with bin/ reverted. No real all-in-one or Apple Silicon machine was used, so this is evidence about the logic, not the hardware.

Apple Silicon (#8125). With 228600000.dsi.0 and apple-panel-bl present, base and revert pick 228600000.dsi.0, the head picks apple-panel-bl. That matches what @CuraMagis found by hand. One correction: on Touch Bar MacBooks under Asahi, 228600000.dsi.0 is the Touch Bar's backlight (tiny-dfr treats it as one), not a node that does nothing. The comment at bin/omarchy-hw-display:14-15 should say so, and the last new case in test/shell.d/hw-display-test.sh (228600000.dsi.0 + appletb_backlight → 228600000.dsi.0) asserts that a Touch Bar gets picked as the display backlight. That is existing fallback behaviour, but the test shouldn't lock it in.

All-in-ones (#8015). The fallback only runs on exit 3, which means detect listed no bus for the connector. In my probe it works for that case: --monitor DP-1 fails on base and revert, and reads/sets acpi_video0 on the head. Both reporters on #8015, though, show ddcutil detect listing the built-in panel ("does not support DDC/CI. (I2C slave address x37 is unresponsive.)"). --skip-ddc-checks skips the DDC check, so it doesn't hide that panel. detect_bus finds its bus, getvcp fails, the helper exits 1, and the head fails exactly as the base does. My probe shows that for a listed but unresponsive DP-1.

That same topology causes the wrong-display write. Take an iMac whose built-in DP-1 is listed but unresponsive, plus a connected external DP-2 that detect does not list. --monitor DP-2 +5% calls brightnessctl -d acpi_video0 set 45% on the head. On the base it exits 1. backlight_owns_named_monitor (bin/omarchy-brightness-display:75-96) treats "the only connected connector detect missed" as the one that owns the backlight, and on these machines the built-in panel is the one detect lists. It also drops the card prefix (lines 66 and 89), so on a multi-GPU machine card0-DP-1 being listed marks card1-DP-1 as listed too.

Smaller points. backlight_owns_named_monitor runs an uncached ddcutil detect on every call where the fallback applies: two per call at first, then one per call even while the helper's 60-second negative cache holds. That is every brightness key repeat and every 5-second poll of the open Display panel. The negative tests in brightness-display-test.sh only check for a non-zero exit, so they never show that no brightnessctl set happened.

Second opinion. Codex Medium reviewed the same head. It agreed that the fallback won't fire for a listed but unresponsive panel. Independence isn't guaranteed, since it can read this session. It raised the wrong-display case and the card-prefix problem on its own. I reproduced the wrong-display case in the probe above and confirmed the card prefix by reading the code. It found nothing wrong with the exit-status plumbing or with the reordered step path.

Next. This is waiting on @Chessing234. A way forward is to split the two fixes. The apple-panel-bl change, with the comment and the Touch Bar test corrected, can stand alone and needs @CuraMagis to confirm it on their machine. The all-in-one route needs a signal that holds when detect lists the panel. ddcutil --skip-ddc-checks detect --brief output from @roju's machine would show which case it is in. The route also must not pick the backlight's owner by elimination. I've linked this PR to #8125 only, because I couldn't show that it fixes #8015. Draft #13286 rewrites bin/omarchy-brightness-display, and #13362 changes the same omarchy-hw-display list with the opposite expectation for 228600000.dsi.0. Whichever lands second will need reconciling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working compatibility Make Omarchy work better on everything!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Brightness slider/keys silently no-op on Apple Silicon: wrong backlight device picked (228600000.dsi.0 instead of apple-panel-bl)

3 participants