Repository navigation
drm/apple: add T6030 panel brightness control - #183
Conversation
Port the SF 44fe00e5eb58 panel-level implementation and its 4b318c41 fixture-lifetime correction onto PR177's current shared backlight API. Keep panel_nits on the T6030 board record, with separate immutable root machine admission for J514S and J516S. Other 14.7 boards and external pipes keep their existing level encoding and policy behavior. Register the requested default without sending it. Keep the loader's unknown level until userspace requests brightness, and report actual brightness as unknown until a successful present carries a level. Preserve the H17P fade/default behavior and its soft-DPMS dispatch; the 14.7 panel uses its own known-level-only blank and restore hooks. Use the existing v14 RPC lock to serialize a swap with publication of its active framebuffer and copied surface. Brightness work holds that lock and the active framebuffer reference throughout its present. Close the queue gate under that lock, then cancel outside it. Permanent teardown also publishes quiescing, drains shared registration/update work, and unregisters the class device while its CRTC is still valid. Retain the original KUnit suite and add actual present/publication thread interleavings, running-work suspend, real DPMS dispatch, failed-submit actual-level checks, and shared registration/unbind controls. Static stubs replace only the RPC boundary and a one-shot publication pause; the latter continues through the real publication body. Based-on: 44fe00e5eb58d54baf1cc9ad607293e774bfed44 Based-on: 4b318c4140e83e44033168836fe8cf4f76e16d80 Based-on: omacom/linux-aurora PR148 commit 0252453 (DjDeveloperr) Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
Give the framebuffer fixtures real private GEM objects on a GEM-capable DRM device. Release their initial references after the framebuffer and active-scanout references, with the matching public object release. Initialize the mode-state spinlock used by the actual unbind path. Set the fixture panel dimensions so real DPMS dispatch also detects a regression that routes this panel through H17P soft DPMS. Exercise the J514S admission branch as well as J516S and the other-machine refusal. Production code is unchanged. Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
Wrap and align the new declarations and calls, describe the gate spinlock, and remove an extra blank line. Split two fixture field assignments and write its null-callback assertion as a boolean expectation. Document that resume reopens admission and leaves a pending level for the next normal CRTC restore present. The production C token stream is unchanged after removing comments and whitespace. Keep the qualified encoder's constant-only layout assertions; their checkpatch comparison warning does not indicate an operand-order bug. New files are already covered by the Apple DRM maintainer path. Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
After the running brightness present and suspend have completed, leave a new level pending and invoke the real callback as a previously queued callback could arrive. It must refuse the level without another RPC even though the framebuffer is still pinned. This separately checks callback admission as well as the queue-side gate. Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
[Medium impact] The PR appears safe to merge; no actionable defects were found. SummaryAdds panel brightness control for J514S/J516S on the 14.7 display path.
No actionable issues were found. No previous review threads were supplied.
|
maralcbr
left a comment
There was a problem hiding this comment.
Thanks @iconidentify, this is a careful port. Brightness presents now take the same RPC lock as normal presents, the default 140 is never sent before userspace asks for a level, and H17P keeps its own soft-DPMS, fades and suspend/resume ops. Teardown is right too: the class device goes away before the CRTC on unbind and remove, and the shared registration and update work is drained. check-commits.py passes on all four commits, and the cumulative checkpatch run leaves only the MAINTAINERS warning and the two static_assert operand-order warnings you explained in 964323e.
One thing before merge:
- Kernel CI hasn't run on this PR.
kernel-ci.yml:18-20andkernel-ci-label.yml:10-13only trigger for PRs intointegration/aurora, so the only check here is Greptile. The two new suites are included underCONFIG_DRM_APPLE_DISPLAY_KUNIT_TEST, whichaurora.kunitconfig:79enables, so CI's KUnit job will run them once this targetsintegration/aurora. Please retarget once #177 lands. Changing the base alone won't start CI, so it needs a push or theci:buildlabel after the retarget. I'll approve once it's green with the 18 new cases passing and no new splats.
Smaller things:
- A refused level stops the panel until reboot, and the retry path can't run. Any refused swap on the internal panel sets
v14->failed(iomfb_v14_7.c:2837-2838). This PR adds swaps that carry a level from a brightness write (:2964-2969) and from DPMS blank once a level is known (:2983-2986, which sends 0). The retry queued indcp_v14_swap_nits_done(:2777-2780) is always refused by the worker, becausefailedis latched right after. Keeping the panel failure policy as it is is fine with me. Could you drop that retry for the panel, or add a comment that a refusal there is final, and makeiomfb_v14_7_backlight_test.c:437-443test that behaviour rather than a queued retry? - The prototype for
dcp_work_register_backlightis in the wrong place. It'sVISIBLE_IF_KUNITatdcp.c:2977, but it's only declared in the two test files (iomfb_v14_7_backlight_test.c:18,iomfb_v14_7_backlight_lifetime_test.c:13). That means a KUnit build ofdcp.cwarns under the default-Wmissing-prototypes. Please move the declaration into the existingCONFIG_KUNITblock atdcp-internal.h:699and drop the two local ones.
Nits, fine as follow-ups:
iomfb_v14_7_swap.h:36-37points atdrivers/gpu/drm/apple-m3/iomfb_v14_7.h, which doesn't exist in this tree. Please cite the SiliconForest commit and #148 instead.- Optional:
dcp_backlight_ops_v14uses the sharedcontrols_device(dcp_backlight.c:557-561). WithCONFIG_FB_BACKLIGHT, the first fbdev unblank callsupdate_status()with the advertised 140 and sends it, even though userspace never asked for a level. Our shipped config doesn't enableFB_BACKLIGHT, so this doesn't reach our users today. Returning false from a v14controls_devicewhile the level is unknown would close it. - Optional: the DPMS test stops once power-on marks the level pending (
iomfb_v14_7_backlight_lifetime_test.c:297-299), and the running-work suspend test stops before resume (:263-268). Taking both through the next real present and checking the encoded level would cover the restore.
For release (not the merge): the hardware evidence is J516S only, with 500 → 200 → 300 → 500. Before this ships I'd like a DPMS off/on cycle and brightness 0 on J516S, since a refused level stops the panel. I'd also like one suspend/resume with a level set, and J514S at least once. We have no M3 in our lab, so that evidence has to come from your side.
A refused internal swap latches the panel failure state. Do not queue a brightness retry that cannot be admitted after that failure; exercise the real refused swap and subsequent work admission in the lifecycle test. Declare the KUnit-visible registration worker in the shared internal header so its definition has a prototype in KUnit builds. Replace the nonexistent variant-tree reference with the public source commit. Keep fbdev unblank notifications from sending the advertised default before a level is known. Exercise the registered class operations before and after a real present, and take the DPMS and suspend controls through the restore present and its encoded level. Link: #183 (review) Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
Retarget the follow-up after PR177 merged. Keep main's panel EDID and swap-timestamp fields and encode timestamps before the scoped brightness level block; both additions retain their independent board admission. Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
A late class kick may queue a worker even after the panel failure is latched; the worker refuses to send the swap. Drain that asynchronous callback before checking that no work remains, and keep the submit-count assertion on both the queued and directly invoked callback paths. Generated-by: LLM Signed-off-by: Chris Kearney <303316+iconidentify@users.noreply.github.com>
|
Thanks for the review, Marcelo. The fixes are pushed at fa224e8, and this now targets integration/aurora with a full CI build requested. The refused-panel retry is gone, the worker declaration is in dcp-internal.h, and the source citation is fixed. I also closed the fbdev default-level case and extended the DPMS and suspend tests through the restore present. Locally: 19/19 backlight cases pass without guest diagnostics; both deliberately reintroduced regressions fail their intended assertions, with passing controls. The broader display run is 301/301, with the same existing relink-fixture warning as the base. Could you take another look for approval once CI is green? The additional hardware release checks you requested remain listed explicitly in the PR; I haven't claimed those runs. |
|
I confirm this patchet works on my M3 Pro - brightness buttons now work. |
joshuaswarren
left a comment
There was a problem hiding this comment.
Reviewed fa224e8. The panel admission stays limited to the J514S and J516S. The requested default doesn't send at boot. actual_brightness stays unknown until a matching present succeeds. Brightness presents share the RPC lock with normal presents. Suspend closes admission before draining work. Teardown blocks late registration. I looked at those paths and found no blocking issue. Eryk reported today that the brightness keys work on his M3 Pro. We don't have a T6030 in the lab. So that's his hardware result, not a test I ran.
|
Tested on the lab laptops as part of a combined hand build (also carrying 186, 179, 202). Posted here for 183. What was built
Results Per machine: candidate boots one-shot cleanly, 0 failed units, smoke equal to the same machine's own stock baseline (display, wifi, input, audio tone, ANE, battery; known gpu/seat-audio script-limit failures identical to baseline), apple-panel-bl backlight set/readback OK on both (max 509 on the M1, 500 on the M2, set 120 resp. 250 and read back), gpu-compute max_abs_err identical to baseline, gpu-dmesg clean, no new dmesg error class vs baseline (only known tas2764/of_irq/brcmf flake classes), dcp module loads with zero dcp error lines on both candidate boots. Regressions: none found on T8103/T6021. The 16-inch M1 Max run is pending (its lab window was taken by other tests); I will add one line to this comment thread when it ran. Not covered - stated plainly: the T6030 panel brightness path itself (this feature targets M3 Pro/Max hardware; no such machine in this fleet). On the M1-family/M2-family machines this PR must be inert, and that is what was tested: it compiles, boots and behaves like the baseline. The new KUnit/selftest files (iomfb_v14_7_backlight_test.c, _lifetime_test.c) are compiled into the tree but the test harness was not run on these laptops. |
scottjones
left a comment
There was a problem hiding this comment.
We reviewed fa224e8. Nothing here blocks merge on the code side, and the earlier review points are all addressed: the panel retry is gone, the worker prototype is in dcp-internal.h, the citation is fixed, and fbdev can't send the default 140.
What we checked:
- Other 14.7 boards. Everything new keys on board->panel_nits, which only the T6030 record sets, and on the J514S/J516S machine check. On J613 and J615 the backlight code takes the same branches as before. The soft DPMS rewrite in dcp.c is the same logic as before, and suspend/resume do nothing there. The present path now holds v14->lock across the whole present instead of only the swap and the publish. It sends the same RPCs in the same order, and the framebuffer puts still happen after the unlock. From reading the code, we're comfortable this can't change the M3 Air panel, though nobody has run it on a J613 yet.
- The level encoding. dcp_v14_nits_binary64() matches a real IEEE double for every value from 0 to 500, and nothing above 500 gets encoded.
- Teardown. Unbind, remove and the probe devres action all close the gate, drain the work and remove the class device before the CRTC goes away. A v14 panel can't rebind, so closing the gate for good is fine.
- Tests. The CI KUnit run at this head has all 19 new cases passing. W=1 builds of dcp.o, dcp_backlight.o and iomfb_v14_7.o are clean, with and without KUnit. The DCP host suites match the base. We also ran the pure cases (encoding, gate, plan, policy) on the host and broke one guard at a time: the 500 nit bound, the closed gate staying closed after resume, ENODATA before a level is known, the quiescing skip, DPMS following only a known level, and the mantissa shift. A case failed for each one.
One request, ideally before merge: once a level is known, a DPMS blank now sends 0 nits with the black swap (dcp_v14_blank, then dcp_bl_effective with dpms_off), and a userspace write of 0 reaches the wire the same way. Nothing floors it. If the firmware refuses that swap, swap_locked latches v14->failed, and the panel stays stuck until reboot. The latch was already there; what's new is that the blank swap carries a level. In practice the level is known in nearly every session, because systemd-backlight restores it at boot, so the first idle blank on any J514S or J516S takes this path. Nobody has run it yet, here or in #148. So we'd like one J516S run of brightness 0 and a DPMS off/on cycle before this lands. Marcelo listed these for release, and that's defensible for integration/aurora, but it's one idle cycle and the failure only clears with a reboot. If 0 turns out to be refused, the easy fix is to not follow DPMS (the blank swap is already black) and to floor writes at 1 nit until it's known to work.
Suspend/resume with a level set and J514S can stay release items as Marcelo listed them.
maralcbr
left a comment
There was a problem hiding this comment.
Reviewed fa224e8. All my earlier points are addressed, and CI is green with the 19 new KUnit cases passing, so I'm approving as I said I would.
@scottjones I agree with your reading. Once a level is known, dcp_v14_blank() marks DPMS off, the black swap carries NITS_PRESENT=1 with 0.0, and a refusal latches v14->failed (iomfb_v14_7.c:2835-2849). A userspace write of 0 gets there the same way, and so does an fbdev core blank once the level is known. Before this PR the swap never carried a level, so this is the one new wire state with no hardware evidence.
I'd still keep it as a fast follow rather than a merge gate. integration/aurora only reaches edge testers, the failure is visible on the first idle and clears with a reboot, and it's the same item I listed for release. But it's cheap, so let's get it this week. @iconidentify @erykwieliczko, whoever gets to it first (Eryk is already on an M3 Pro: which board, J514S or J516S, and which build?):
- Set a non-zero level and wait until
actual_brightnessreads it back. echo 0 > /sys/class/backlight/apple-panel-bl/brightness, then wait untilactual_brightnessreads 0. Don't restore straight away: the write only queues work, so an early restore can replace the 0 before it's sent. Check the panel is dark, then restore a level and check it comes back.- From a confirmed non-zero level, run
hyprctl dispatch dpms off, wait a few seconds, thendpms on. - Post
dmesg | grep -iE 'dcp|flip failed|brightness present'.
Set up a way to get back in first (SSH, or a timed restore) before you darken the panel.
If the firmware refuses 0, the fix is Scott's: stop following DPMS (the blank swap is already black) and floor the level at 1 nit. The floor has to cover the core-blank and DPMS paths too (dcp_bl_effective()), not just userspace writes. I'd rather not do that pre-emptively, since a lit backlight behind a black frame costs battery on every idle.
Suspend/resume with a level set, and J514S hardware, stay release items.
|
@erykwieliczko, thank you for testing these patches on your M3 Pro. We work on Neural Engine support for the M3 chips and we have no M3 Pro in the lab. If you are willing, we would like the first two Neural Engine stages from you. They do not start the ANE core. Stage 0 only reads the device tree. Stage 1 asks the kernel to power the ANE power domains and then reads five power-manager words. The module writes no ANE register. The setup does write persistent boot configuration: one overlay file and one line in the opt-in file. The steps say what each command writes and how to undo it. Please have a fallback boot first. The exact steps are here, with the M3 Pro column: joshuaswarren/omarchy-ane#176 (comment) If you run it, please post the output on that issue. This is optional, and a stop at any step is a useful answer too. |
Adds panel brightness control for J514S/J516S on the 14.7 display path. This is the separate fast follow to the merged #177 and now targets integration/aurora.
The panel keeps the loader’s brightness until userspace requests a level. The default requested value is not treated as actual brightness: actual_brightness returns ENODATA until a successful matching present carries a level. Levels use the existing public encoding, capped at 500 nits or a lower described panel maximum. Other 14.7 boards retain their current admission, and H17P keeps its existing policy.
Brightness presents now share the framebuffer/RPC lock with normal presents. Suspend closes queue admission before draining work; teardown also stops shared registration/update work and removes the class device before its CRTC can disappear. Review caught the registration/unbind race, and the tests exercise it along with real DPMS dispatch, concurrent presents and a running brightness RPC during suspend.
Based on the qualified 44fe00e5 implementation, its fixture correction, and Dj’s panel-backlight work in #148.
Validation at f4ef473:
Hardware artifact: f947a7d43fd2cbfd6c7986c55cbe02da976a991a7594f2d635b10b9827b98c8c; lab run j1022-161901-18ec. The first attempt stopped before the brightness exercise because the second dock tunnel was not yet discovered. The unchanged artifact passed with a bounded discovery wait (two seconds observed).
Limits: J514S admission is unit-tested; hardware testing was J516S only. No suspend or photometric qualification. The lab job’s full OMT launcher still exits 126 due to its existing permissions issue. The installed SiliconForest desktop guard selects software for this temporary kernel; independent native GL/VK probes pass, but native compositor qualification is not claimed. The existing Dell refresh difference (original 30 Hz, candidate advertises 60 Hz) is recorded separately, without a physical-picture claim.
Prepared with LLM assistance and independently reviewed; commits carry Generated-by and Chris’s sign-off.
Review follow-up (2026-10-23):
The follow-up is source/test qualified; the hardware result above remains specifically the original f4ef473 build. Release qualification still needs brightness 0 and DPMS off/on on J516S, a supported suspend/resume run with a level set, and J514S hardware evidence. None is claimed here.
Local follow-up checks: 19/19 focused backlight cases pass with lockdep,
KASAN, UBSAN and work debug objects, with no guest diagnostics. Two
regression variants (reintroducing the refused-panel retry and admitting
fbdev before a level is known) each build, run all 19 cases and fail the
intended assertions. All three surrounding controls pass 19/19 with no
guest diagnostics. The broader display regression run passes 301/301;
its existing dcp-relink fixture warning is recorded separately.