Repository navigation
Route an all-in-one's built-in panel to the kernel backlight - #8053
Chessing234 wants to merge 3 commits into
Conversation
The helper exited 1 whether the connector answers no DDC display at all or a present monitor failed one read. Those want opposite handling -- the first is permanent and worth routing elsewhere, the second is transient and worth retrying -- so give the first its own status. It already tracks the difference internally to decide what to cache. read_brightness's status could not reach the exit anyway: `read -r ... < <(read_brightness)` reports the read builtin, not the process substitution.
An all-in-one wires its built-in panel to a DisplayPort connector, so the output name carries no hint that it is internal and the panel is sent to DDC/CI, which it does not answer. Brightness then fails outright -- no slider in the Display panel, dead brightness keys -- with a working backlight device sitting in /sys/class/backlight. Fall back to that backlight when the connector answers no DDC display and the machine has no eDP/LVDS/DSI connector that could own the backlight instead. Both halves are load-bearing: the first keeps a real external monitor on DDC, including one attached to the same all-in-one, and the second keeps a laptop's DDC-less external monitor failing rather than moving the laptop's own panel while the user asks for the other one. Read the connectors from DRM rather than counting Hyprland outputs, the way omarchy-hw-external-monitors already does: Hyprland invents a FALLBACK output when every monitor is disabled, so an output count is not a count of panels. Reported with a working diff in omacom#8015; this takes the same direction through the DDC result rather than a monitor count, which also covers an all-in-one with a second display attached.
There was a problem hiding this comment.
Pull request overview
Routes DDC-less all-in-one panels to the kernel backlight while preserving DDC routing for external displays.
Changes:
- Adds DDC absence status handling and kernel-backlight fallback.
- Detects internal connectors through DRM.
- Adds topology and failure-path tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bin/omarchy-brightness-display |
Adds DRM-aware backlight fallback routing. |
bin/omarchy-brightness-display-ddc |
Distinguishes absent DDC displays from read failures. |
test/shell.d/brightness-display-test.sh |
Adds all-in-one and failure-path fixtures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rm -rf "$runtime_dir/omarchy-brightness-display-ddc" | ||
| brightness=$(DRM_PATH="$drm_aio" DDC_CONNECTOR=DP-2 run_brightness --monitor DP-2) || | ||
| fail "an external monitor on an all-in-one still uses DDC" "the command exited nonzero" | ||
| [[ $brightness == "50" ]] || fail "an external monitor on an all-in-one still uses DDC" "actual: $brightness" |
There was a problem hiding this comment.
split the fixture — there's an AIO-alone tree and a docked one with DP-2 connected.
The fixture marked DP-2 disconnected, so the case that claims to cover an all-in-one with an external monitor attached was not describing that machine at all. Split the all-in-one DRM tree in two -- alone, and with a second display connected -- and run each case against the set it names. Cover the built-in panel of the docked machine too. A second connected display is not an internal panel, so it cannot be what the backlight belongs to, and nothing was checking that.
|
fair — the fixture was not describing the machine the comment claimed. the routing result happens to be the same either way, since the internal-connector scan only looks at eDP/LVDS/DSI, but a case that says "external monitor attached" against a disconnected DP-2 could never catch a change that reads the connected set. split the all-in-one DRM tree into two, alone and docked with DP-2 connected, and each case now runs against the set it names. also added the one your note implied: the built-in panel of the docked machine still has to reach the backlight. a second connected display is not an internal panel, so it cannot be what the backlight belongs to — nothing was checking that. 17 assertions now, green with the fix and red on the first new case without it. |
|
@dhh @ryanrhughes ready for review |
|
folded into #12267 for easier review. |
Fixes #8015.
omarchy-brightness-displaypicks its backend from the connector name:eDP/LVDS/DSImeans the kernel backlight, anything else means DDC/CI. All-in-ones wire their built-in panel to a DisplayPort connector, so it is classified as external, sent to DDC/CI — which a built-in panel does not answer — and brightness fails outright. No slider in the Display panel, deadXF86MonBrightnesskeys, and a working backlight device sitting unused in/sys/class/backlight.What this does
Falls back to the kernel backlight when both are true:
eDP/LVDS/DSIconnector that could own the backlight instead.Both halves matter. (1) keeps a real external monitor on DDC — including one plugged into the same all-in-one, which the reporter's monitor-count version could not. (2) keeps a laptop's DDC-less external monitor failing rather than silently moving the laptop's own panel while the user is asking for the other one.
To make (1) possible,
omarchy-brightness-display-ddcnow exits 3 for "this connector answers no DDC display" and keeps 1 for a transient read failure. It already tracked that difference internally to decide what to negative-cache; it just could not report it. Along the way,read -r bus current maximum < <(read_brightness)reported thereadbuiltin's status rather thanread_brightness's, so no status could have escaped that function anyway.Connectors are read from DRM (
OMARCHY_DRM_PATH, the wayomarchy-hw-external-monitorsalready does) rather than by counting Hyprland outputs — Hyprland invents aFALLBACKoutput when every monitor is disabled, so an output count is not a count of panels (#8027).Verification
test/shell.d/brightness-display-test.shgained DRM fixtures for both machine shapes and five cases:DP-1, no DDC displayDP-1, no DDC display,60%brightnessctl set 60%DP-2, answers DDCDP-2, no DDC displayDP-1, transient read failureAll 16 assertions in that file pass, plus
monitor-state-testandbin-style-test. Reverting only thebin/changes makes the first new case fail, so the tests have teeth.Not run: this machine has no Hyprland/Quickshell, so the Display panel was not opened and no real DDC or backlight hardware was touched — the evidence above is stub-level, and the reporter offered to test a patch on the affected iMac.