[joshuaswarren]

[Joshua's agent] Draft. This head has not been booted and no hardware has shown the square. The stream evidence below comes from a different tree (our K114c line) that carries the same ISP-side gate removal.

Per the thread with rttdevs and Dj (camera thread, seq 722, 725, 742): the camera is on by default and the indicator never gates it. This is the one shared implementation; J700 (rttdevs) stacks on it.

Contract (header include/linux/soc/apple/display.h):
int apple_dcp_camera_indicator_set(struct device *dcp, bool on). on commits a small kernel-owned square on a reserved overlay plane (swap slot named by apple,camera-indicator-surface in the DCP node) and returns 0 once that request is committed; it does not wait. A delayed work withdraws the square, with a warning, if no present carried it within 1 s. An error means no indicator. The ISP node opts in with apple,camera-indicator = <&dcp>. Boards with a hardware light do not set it. It is a best-effort software indicator, not a privacy guarantee.

ISP side: request before the firmware starts the channel, remove after the stream stops on every stop path (also after an IPC failure). If the DCP is absent, unbound or cannot show the dot, the driver warns once and streams anyway. The DCP lookup never defers probe; the device reference is held only when a device exists. VIDEO_APPLE_ISP depends on DRM_APPLE || !DRM_APPLE.

DCP side: the plane belongs to the kernel. A userspace commit that changes it (FB_ID=0, CRTC_ID=0, own framebuffer, move) is put back to the current state instead of failing, so a console restore or a blind clear of all planes still works. Unchanged, or disabled together with its CRTC (DPMS, suspend), it is left alone. The call holds the DCP device lock so it cannot unbind underneath.

Proof:

• Pure state policy (dcp-indicator.h) with a KUnit suite; built as a userspace program: 5 cases, 0 failed. Ryan ran the suite on arm64 at 9312fb9: 229/231, 1 skip, 1 failure, fixed in 5125ea3.
• ISP KUnit cases: a failed request still streams; no DCP means no call; request before channel start, removal after stop; a failed channel start or failed firmware still removes the square. Compiled with CONFIG_KUNIT=y, not run.
• drivers/gpu/drm/apple and drivers/media/platform/apple/isp build with LLVM on this base.
• A non-author review found three issues (console restore rejected, blocking commit, unbind race); all three are fixed in this version.

Not verified: whether the DCP composites the chosen slot (H17P path assumed; dcp_indicator_swap_done hooks only iomfb_scanout_complete_h17p, on the M4 path no present confirms it, so the square is withdrawn after 1 s with a warning), the 27.x layout, the mode-change case (the square keeps its position), dt_binding_check. The binding property is added to the T6050 ISP schema only; other ISP schemas need the same line when a board uses it. No DT nodes in this PR; the K114c nodes stay in our tree.

@RyanTheTide for the shared path. Hardware proof comes from our boot owner after a boot slot.

Hardware evidence (a 14-inch M5 Max, camera sensor 0x958, firmware 27.3 display state iBoot framebuffer, DCP disabled in the device tree so the indicator call is never made):

• Before: our K114c tree refused to stream on a node without a light GPIO (a module parameter, -EPERM from apple_isp_start_capture). The refusal came after isp_configure_capture had passed, so stop_camera shut the firmware down, and the next start timed out on request 0x0b09 (-62). The same order failed the same way on two kernels, and a first attempt with the parameter set streamed on a third.
• After, with the gate deleted (kernel 7.3.0-k114c-ARCH-cam1b): first start after boot streams, no parameter: /dev/video0 NV12 1920x1080, 2 cycles x 10 frames, 31104000 B per cycle, no timeout, no OPCODE error. The last frames are a smooth dark image (mean luma 25-35 of 255, adjacent-pixel difference 1.25 against 12.9 for random pairs); the first 3 frames are black level.
• Not shown: the dot (no DCP was bound), the DCP half on hardware, a stream that fails after configure. The upstream ISP driver on integration/aurora has no gate, so this PR adds none; the evidence is for the contract "the camera never waits for the indicator".

[RyanTheTide]

J700 side is stacked on this as branch camdot/j700-camera-indicator (e4318c7): apple,camera-indicator-surface = <2> on the J700 DCP and apple,camera-indicator = <&dcp> on its ISP, plus the property in apple,t8140-isp.yaml. On H17P the plane goes through iomfb_template.c like any other plane, and slot 2 is inside the 0x7 mask the clear and power-off swaps reset. Hardware row on the J700 to follow.

Two points from reading the DCP side:

1. apple_plane_atomic_check() turns any userspace change to the indicator plane back into the old state and returns 0. That keeps an implicit disable (fbdev restore, a modeset that pulls in every plane) from taking the square down, which is good. But a compositor that probes the plane explicitly, with a TEST_ONLY commit putting its own framebuffer there, is told it worked, and the real commit then shows nothing of its buffer. Could explicit userspace framebuffers on this plane fail with -EINVAL instead, keeping the silent restore only for disables? Then compositors that try overlays fall back cleanly.

2. IND_FILL is 0xffff9500 (orange). macOS shows the camera indicator in green and uses orange for the microphone, so green would match what users of these machines expect.

[RyanTheTide]

KUnit arm64 on 9312fb9 (+ the J700 DT commit): 229/231 pass, 1 skip, 1 failure: apple-isp-camera.isp_test_stream_failure_lights_off expects set_off_at == 2 and gets 3. The firmware-start stub takes step 2 (on = 1, fw start = 2, off = 3), so the expectation looks off by one rather than the code. apple-dcp-indicator 5/5 and the other three indicator ISP cases pass. The J700 DTB built from the stacked branch carries apple,camera-indicator-surface = <2> on the DCP and the phandle on the ISP.

[RyanTheTide]

J700 row with camdot/j700-camera-indicator (e4318c7 on this PR), Neo, DTB-only change on the live Stage 2 (the two indicator properties), one-shot test kernel, every stream to /dev/null:

• The ISP picks up the phandle (probe notice), and the DCP reserves slot 2.
• 23 start/stop cycles (45 frames each): the 48x48 indicator framebuffer is on the reserved plane in the DRM atomic state during every stream and gone after every stop. set(true) never warned, so each one confirmed a present. Streams rc=0 throughout.
• Panel off (Mutter PowerSaveMode 3): set(true) returns -ENODEV, warned once, and the stream still runs (rc=0).
• Not covered by this row: on-screen pixels. This kernel has no blend-CRC knob, and nobody has looked at the panel yet.

Two failures seen on this base, both also present with the indicator absent (same kernel, production DTB), so they are not from this PR: the internal DCP crashes on DPMS off ("IOP idle not acknowledged", "Invalid AP power: 8"), and a stream across s2idle fails to restart (ISP firmware boot stage 1: -19). The current production kernel has neither.

[joshuaswarren]

[Joshua's agent] CHANGES
Cross-model review (Claude)

Head 9312fb9. Read-only review of the diff plus the head tree. Nothing was booted. I did not build: the worktree has no top-level Makefile, so the DRM_APPLE=m/y/n result below comes from Kconfig and header reading only.

Earlier findings (console restore rejected, blocking commit, unbind race): the fixes are correct.

• Restore: plane.c:238-280 puts a changed userspace state back instead of failing. The going_off test (plane.c:258) lets DPMS, suspend and drm_atomic_helper_shutdown pass.
• Unbind: guard(device) plus ci->ctl (dcp-indicator.c:236) serialises set() against dcp_indicator_release (dcp-indicator.c:62). The DCP's component_del unbinds the DRM master first, so set() cannot reach a freed apple_dcp.
• Commit: it is now nonblocking, and only the 1 s wait for the present remains.

Findings, most important first:

1. KUnit failure still in this head. isp-cam-test.c:829 expects set_off_at == 2. The isp_test_fw_start stub takes step 2 (on=1, fw start=2, off=3), so it should be 3. Ryan already reported 229/231 on this head. The fault is in the test, not the code. Fix the number, or make the stub not count.

2. A rebind of the DRM master loses the indicator for good. dcp_indicator_release sets ci->has_surf = false (dcp-indicator.c:67) when the drm_device goes away, but only dcp_indicator_probe sets it (dcp.c:6616), and the DCP stays bound. On the next apple_probe_per_dcp the guard camera_ind.has_surf (apple_drv.c:1176) is false, so no plane is created. The same applies after a deferred probe retry of apple_drv. From then on set(true) returns -ENODEV, silently apart from one warning. Also, set(false) returns before dcp_indicator_end (dcp-indicator.c:237-238), so a LIT state can stay LIT across the unbind. Keep has_surf and surf as DT-derived constants and clear only plane, fb and the state in release.

3. set(true) blocks streamon for up to 1 s, which conflicts with "the camera never waits for the indicator" (dcp-indicator.c:255, called from isp-cam.c:1102). The hook exists only in iomfb_scanout_complete_h17p (iomfb.c:3342). On any other DCP generation that opts in, every stream start costs the full second, and then the square is withdrawn (dcp-indicator.c:258-260), so no dot is ever shown. Either gate the DT property on H17P at probe (dcp.c:6616), or do the on-request off the stream-start path (a worker) so the wait does not delay the firmware start.

4. The ISP can drop the indicator silently at probe. apple_isp_indicator_dcp returns NULL with no message when the phandle exists but of_find_device_by_node finds nothing (isp-drv.c:689). That happens if the ISP node probes before the DCP platform device is created, since this never defers. The camera then streams with no dot and no log line. At minimum dev_warn there. Better: look the DCP up lazily in apple_isp_start_capture.

5. With DRM_APPLE=n the property is still honoured. isp->dcp is set and the probe prints "best-effort software camera indicator via the DCP" (isp-drv.c:719). The stub then returns -ENODEV on every start (display.h:22-29). The notice is false in that config. Print it only when IS_ENABLED(CONFIG_DRM_APPLE), or skip the lookup.

6. dev_warn_once (isp-cam.c:1104) is once per boot. Later failures (panel off, timeout) are invisible, and for an indicator that is the thing worth seeing. Use a ratelimited warning, or warn once per stream.

7. dcp_indicator_probe runs before the J700 surface-0 fix-up (dcp.c:6616 vs dcp.c:6622). apple,camera-indicator-surface = <0> on J700 passes the test_bit check and then collides with surface 0 that the next block sets. Move the call after that block.

8. The failsafe ext-DCP path allocates apple_dcp without calling dcp_indicator_probe (dcp.c:7005-7010). An ISP phandle that points at such a node reaches guard(mutex)(&ci->ctl) (dcp-indicator.c:236) on a mutex that was never initialised. It works without lockdep but warns with DEBUG_MUTEXES or lockdep. Initialise ctl, lock and presented where apple_dcp is allocated, or test has_surf before locking.

9. Stale comments say the opposite of the contract. apple_drv.c:1190 says "the camera stays off". dcp-indicator.c:90 says "the camera cannot start". On failure the camera does stream.

Checked and fine:

• isp->dcp == NULL: every use of isp->dcp is guarded by apple_isp_camera_indicator() (isp-cam.c:1101, 1112, 1131). put_device(NULL) is safe (isp-drv.c:907, 931). isp_test_no_indicator_still_streams covers it.
• Stop path: vb2_video_unregister_device stops streaming inside apple_isp_remove_video, before put_device(isp->dcp) (isp-drv.c:931). No use-after-put.
• Failure and stop paths: a failed ch_start removes the square (isp-cam.c:1112). The stop path removes it even after an IPC failure (isp-cam.c:1131). A repeated set(false) is harmless.
• Kconfig (by reading):
  • depends on DRM_APPLE || !DRM_APPLE (isp/Kconfig:16) caps the ISP at m when DRM_APPLE=m, so a built-in ISP cannot reference a module symbol.
  • DRM_APPLE=y: any ISP setting links.
  • DRM_APPLE=n: the inline stub is used, so there is no undefined symbol.
  • With m/m, loading the ISP module pulls in appledrm through the symbol dependency, even on boards without the property. That is acceptable but worth a line in the commit message.
• Unconditional KUNIT_STATIC_STUB_REDIRECT in isp-cmd.c is a no-op without KUnit and uses the hooks with KUnit=m. Not built.

Not verified here:

• No boot and no DCP composition of the slot.
• DRM_APPLE=m/y/n builds, and LLVM sparse and kunit runs.
• dt_binding_check.
• Atomic-check restore against a real compositor.

Verdict: CHANGES for items 1-3 (test failure, lost indicator after a master rebind, 1 s stall with no dot on non-H17P). The rest can follow in the same push.

[joshuaswarren]

[Joshua's agent] Review round at head 5125ea3 (two commits on top of 9312fb9). All nine findings in the CHANGES review are addressed. Both touched directories build with LLVM, no warnings. Not booted, KUnit compiled and not run.

1. KUnit: isp_test_stream_failure_lights_off now expects the off call at step 3 and also checks the firmware start at step 2.
2. Rebind: dcp_indicator_release no longer clears has_surf/surf (device-tree constants). It runs from dcp_comp_unbind, because a drmm action ran after the DCP's memory was freed, and it cancels the watch work, drops the framebuffer and ends the state. set(false) ends the state before anything else.
3. 1 s stall: set(true) returns once the request is committed. A delayed work confirms the present or withdraws the square after 1 s with a warning. The camera never waits.
4. The ISP keeps the phandle's node and looks the DCP device up when a stream starts, with a rate-limited warning while it is missing.
5. Without CONFIG_DRM_APPLE the property is not looked up, so the notice no longer appears.
6. The failure warning is rate-limited, not once per boot.
7. dcp_indicator_probe runs after the J700 surface-0 fix-up.
8. set() tests has_surf before locking, so an ext-failsafe DCP never reaches the uninitialised mutex. On the M5 it cannot happen: in the booted K114c device tree every DCP, ext included, is disabled.
9. The two stale comments are corrected. The unread indicator_fb field is deleted.

[joshuaswarren]

[Joshua's agent] CHANGES
Cross-model review (Claude)

Head 5125ea3, re-review of my nine findings plus regressions. Read from the diff against 9312fb9 and the head tree. Not booted, no kernel build. The host tests I did run.

Verdicts on my nine:

1. KUnit off-by-one: fixed, isp-cam-test.c:829-830 (fw start at 2, off at 3).
2. Rebind: fixed. has_surf and surf are no longer cleared. dcp_indicator_release now runs from dcp_comp_unbind (dcp.c:6010), before drm_dev_put (apple_drv.c unbinds, runs drm_atomic_helper_shutdown, then component_unbind_all). It is idempotent, so the extra direct calls at dcp.c:7078 and 7086 are harmless. The cancel_delayed_work_sync runs before taking ctl (dcp-indicator.c:65), so there is no deadlock with the watch work.
3. 1 s stall: fixed. set(true) returns after the commit and the delayed work confirms or withdraws (dcp-indicator.c:234-305).
4. Lookup: fixed by the lazy lookup at stream start (isp-cam.c:1094-1108). The reference is still put at remove, and dcp_np is put on both paths.
5. DRM_APPLE=n: fixed. isp-drv.c skips the lookup when !IS_ENABLED(CONFIG_DRM_APPLE).
6. Rate-limited warning: fixed.
7. J700 ordering: fixed, dcp.c:6621.
8. Uninitialised mutex: fixed. has_surf is tested before ctl is taken.
9. Stale comments: fixed, and the dead indicator_fb is removed.

New problems:

A. Host tests fail at this head, and at 9312fb9 too. tools/asahi/isp-tests.py --suite session, buffers and returns do not compile. receive and queue pass. I reproduced it from a git archive of both heads in /dev/shm. Cause: the harness copies the bodies of isp_cmd_send and apple_isp_stop_capture into a host C file. The PR added three things to those bodies that the host file does not define:

• KUNIT_STATIC_STUB_REDIRECT is now unconditional in isp-cmd.c:78.
• apple_isp_camera_indicator() and apple_isp_dcp_indicator_set() are called from isp-cam.c:1151.
• The isp->dcp member.
  The clang errors are exactly those four. I confirmed the diagnosis on a scratch copy only. In tools/asahi/isp/session.c I added #define KUNIT_STATIC_STUB_REDIRECT(...) do {} while (0), a void *dcp; member, and two stub functions (camera_indicator returns dcp != NULL, dcp_indicator_set returns 0). --suite session then printed PASS. buffers and returns still fail on the same missing names. Their templates need the same stubs; I did not chase them. Apply the same stubs to every template the harness builds, or have the harness extract with a prelude.

B. Stale watch can withdraw a newer request (dcp-indicator.c:244-250). set(false) uses the non-sync cancel_delayed_work (dcp-indicator.c:280). If the watch work has already started and is waiting on ctl, a quick stop followed by a start lets it run after the new begin. It then sees PENDING, warns and withdraws the new square before its own 1 s. The window is small (the work must start in the 1 s boundary while a stop and start happen). The fix is cheap: record a generation or jiffies deadline at begin and have the watch compare it.

C. A stream running across a master rebind has no dot afterwards. Release ends the state (dcp-indicator.c:80) and the next bind builds the plane, but nothing re-requests the square for a stream that is still on. This is a minor, best-effort gap; one line in the commit message is enough.

Checked and fine: the isp->dcp NULL paths (dcp_np NULL means no bind, no call); put_device(NULL) and of_node_put(NULL) are safe; the three Kconfig cases (by reading) are unchanged and still consistent with the lazy lookup.

Not verified: kernel build for DRM_APPLE=m/y/n (the worktree has no top-level Makefile), system_percpu_wq on bases older than this tree, any hardware behaviour.

Verdict: CHANGES, for A only (host tests broken). B and C can follow in the same push.

[joshuaswarren]

[Joshua's agent] GREEN
Cross-model review (Claude)

Head 6b60eeb. The only change since 5125ea3 is the fixture commit: tools/asahi/isp/session.c and tools/asahi/isp/buffers.c. No driver code changed, so my review of 5125ea3 (all nine earlier findings fixed) still holds.

Host tests, run from a git archive of this head in /dev/shm (no kernel build), python3 tools/asahi/isp-tests.py --suite <s>:

• session: PASS
• buffers: PASS
• receive: PASS
• returns: PASS
• queue: PASS

The fixtures add exactly what the driver change needs: a no-op KUNIT_STATIC_STUB_REDIRECT, a dcp member, and camera_indicator/dcp_indicator_set stubs with no DCP. This matches the cause I gave earlier. The stubs mean these suites do not exercise the indicator path; the ISP KUnit cases cover it.

Still open, not blocking, for a follow-up or a commit-message line:

• Stale watch (dcp-indicator.c:244-250, :280). A watch that has already started can withdraw a newer request after a quick stop and start. Compare a generation or deadline recorded at begin.
• A stream that is already running across a DRM master rebind shows no dot afterwards.

Not verified: kernel build for DRM_APPLE=m/y/n (the worktree has no top-level Makefile; the Kconfig result is from reading), KUnit runs, and any hardware behaviour.

[joshuaswarren]

GREEN at 6b60eeb
Cross-model review (GLM)
Model: zai/glm-5.3
[Joshua's agent]

aurora-silicon/linux #1744 (draft; camera indicator; base integration/aurora; 3 commits after my first look at 9312fb92). Joshua's agent review, not an upstream maintainer review. No hardware run: the dot has not been shown on a panel, and I did not try. This covers the code, the host tests and the KUnit suite as run on this seat.

History, so the record is straight. At 9312fb92 I found two real failures, both reproduced, and both are fixed in this head:

1. CI host-tests was red from the code: tools/asahi/isp-tests.py compiles apple_isp_stop_capture and isp_cmd_send straight out of the driver; the PR added apple_isp_camera_indicator, apple_isp_dcp_indicator_set, isp->dcp and a KUNIT_STATIC_STUB_REDIRECT call that the fixtures lacked, so suites session, buffers and returns (and their 9 negative controls) did not compile (rc 1; base rc 0).
2. The ISP KUnit suite had never been run (the PR says "compiled, not run"). I ran it on arm64 under QEMU: 52/53, isp_test_stream_failure_lights_off failed (trace.set_off_at == 3, expected 2). The sequence is on=1, firmware start=2, off=3, so the test was wrong, not the driver.

At this head (all run by me on a detached checkout):

• tools/asahi/run-host-tests.py --only isp-tests: 16/16 (5 suites, 11 negative controls) pass. The CI host-tests check is now success.
• ISP KUnit on arm64 QEMU (--kunitconfig=drivers/media/platform/apple/isp, CONFIG_WERROR=n): 53/53, including the corrected isp_test_stream_failure_lights_off (now also asserts the firmware start is event 2).
• drivers/gpu/drm/apple/ and drivers/media/platform/apple/isp/ built with W=1 against arm64 defconfig plus DRM_APPLE=m, VIDEO_APPLE_ISP=m: rc 0, 0 warnings. dcp-indicator.o, plane.o, iomfb.o, isp-cam.o, isp-drv.o all compiled.
• dcp-indicator-test.c (pure state policy, 5 cases): 5/5 pass. The file includes kunit-shim.h, which is not in the tree, so I wrote a 15-line shim of my own to run it; the PR text says it was run "as a userspace program" without saying where the shim is.
• My mutations on the ISP side (KUnit, rebuilt each time): W1 (no indicator-off after a failed channel start) caught (1 test); W2 (no off in stop_capture) caught (2); W3 (on never requested) caught (3). Control 53/53.

What I read in the new commits and found fine: ec9a1b06 replaces the 1 s wait with a delayed work: set(true) returns once the request is committed and the work withdraws a square no present has carried (it checks DCP_IND_PENDING under ci->ctl). dcp_indicator_release now runs from dcp_comp_unbind (cancel_delayed_work_sync before taking ctl, then drops the fb and resets the state), so no drmm action outlives the DCP. 5125ea3b finds the DCP at stream start (the DCP may probe later) and keeps the reference until remove; put_device and of_node_put are NULL-safe on the probe error path and in remove.

Gaps, not blocking, none with a failing input:

• The stream-start DCP lookup is untested. W4 (apple_isp_indicator_bind returns at once) and W5 (the call dropped from start_capture) both pass 53/53, because the tests set isp->dcp by hand. If that lookup broke, the camera would stream with no dot and no failing test. Hardware or a KUnit case with a fake platform device would cover it.
• The DCP half (dcp-indicator.c commit/watch/release, the plane.c restore logic) is exercised only through the 5 pure-state cases and the build. No test runs the commit path, the 1 s withdrawal, or the unbind.
• On the M4-class (non-h17p) path no swap_done hook exists, so the square is withdrawn after 1 s while the camera keeps streaming (the PR says this). The camera no longer waits for it, which is the right way round.
• A delayed work armed by a set(true) that races the DCP's component unbind (not the whole device unbind) could outlive dcp_indicator_release; I did not find a path to a use-after-free (device lock serialises set() against driver remove) and did not reproduce one.

CI at this head (8 checks, all completed, all success): source, the three DWC3/ATC builds, rust-build, host-tests, kunit, dt-checks (it finished after my runs; I did not run dt_binding_check myself). I did not rerun any check.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-13T04:29:02.155922Z	6b60eeb	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[joshuaswarren]

[Joshua's agent] New commit faddbaea7e1e on #1744. The head moved, so the GREENs at 6b60eeb9 need a re-review of this one commit (git diff 6b60eeb9..faddbaea).

It closes the w7X gap and the two minors left by the Claude review:

1. Stream-start bind test. The platform-device lookup now goes through a stubbable wrapper, and the new case isp_test_dcp_bound_at_stream_start checks three things. A missing DCP gives a stream with no dot. The next stream binds the DCP and requests the dot before the firmware start. A bound DCP is not looked up again.
2. Stale watch. set(true) records a deadline. A watch armed for an earlier request that was stopped and requested again returns before the deadline, so it cannot withdraw the newer request.
3. Stream across a DRM rebind. A request that is still wanted now survives dcp_comp_unbind (dcp_ind_suspend). The first present after the next bind queues a resume that requests the square again. A stop, a failed commit or a withdrawal ends the request, so a failing commit cannot loop. Release drops the fb and plane under the lock, then cancels the two works, so no present can queue them again. The resume trigger is swap_done, which exists only on the h17p path, as for the confirmation.

Run by me:

• DRM and ISP dirs build with no warnings (arm64, LLVM).
• ISP KUnit on arm64 QEMU: all five suites pass (camera 22/22, dapf 3, sharedmalloc 9, v4l2 13, ipc-layout 6), including the new case.
• Mutations on the new case, each rebuilt and run: bind call dropped from start_capture (W5) fails isp_test_dcp_bound_at_stream_start (21/22); apple_isp_indicator_bind returning at once (W4) fails the same case (21/22).
• indicator_resumes_only_a_kept_request built as a userspace program passes 6/6. With dcp_ind_end() changed to keep the request it fails.
• tools/asahi/run-host-tests.py --only isp-tests: 16/16 (the host fixtures needed no change).

Not run: the resume path and the deadline guard on a panel or in a KUnit case (no test drives the DRM work), DRM_APPLE=n, and any hardware.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b60eeb9b3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[joshuaswarren]

[Joshua's agent] GREEN
Cross-model review (Claude)

Head faddbae. Delta 6b60eeb..faddbae, one commit, read statically (KUnit not run by me; w7Y reports the ISP KUnit run on arm64 QEMU).

(a) Stream-start bind test (isp-cam-test.c:794-832, isp-cam.c:1093-1115)

• Bind dropped: isp->dcp stays NULL, so KUNIT_EXPECT_PTR_EQ(isp->dcp, ...) fails, lookup_calls reads 0 or 1, and set_on_at is 0 instead of 2.
• Bind returns early: the same three expectations fail.
• Removing the isp->dcp || guard: the third call looks up again, so lookup_calls is 3, not 2.
• The stub is a real seam: apple_isp_dcp_lookup is a KUNIT_STATIC_STUB_REDIRECT wrapper and the only lookup call site. Teardown never reaches the put_device(isp->dcp) calls in isp-drv.c:897,922, and the fake pdev is kunit_kzalloced.

(b) Stale watch deadline (dcp-indicator.c:255-257, :282)

• deadline is written in dcp_indicator_request() and read in the watch, both under ctl; mod_delayed_work runs after it is computed, so a legitimate fire is never early. A stale watch that was already running when the request was re-armed now returns. time_before is wrap-safe. set(false) still uses non-waiting cancel_delayed_work(), and the deadline check covers that gap.

(c) Request survives a DRM rebind

• wanted is set in dcp_ind_begin, cleared only in dcp_ind_end; dcp_ind_suspend keeps it. indicator_resumes_only_a_kept_request covers suspend, fresh begin, end, and suspend after end.
• dcp_indicator_release clears fb, plane and state under ctl plus the spinlock, then cancels the watch and resume work after dropping ctl; once fb is NULL neither swap_done (:364-370) nor the resume work (:297-312) can act. attach_plane publishes fb under the spinlock after plane is set.
• Both works are cancelled synchronously in dcp_comp_unbind (dcp.c:6010) before DCP teardown. A failed resume goes through dcp_indicator_end, so there is no retry loop.

Not checked / gaps

• Nothing tests swap_done scheduling the resume work, or the resume work body: dropping schedule_work at dcp-indicator.c:370 would survive the suite. Worth a follow-up case.
• No lock-order analysis between ctl held in resume while taking modeset locks and the unbind path (taking ctl there is not new).
• Did not confirm the tools/asahi host tests still build with the new wanted field and dcp_ind_suspend.

[joshuaswarren]

CHANGES at faddbae
Cross-model review (GLM)
Model: zai/glm-5.3
[Joshua's agent]

My GREEN at 6b60eeb (comment 6843602150) is withdrawn. I missed an open Codex finding that was already posted on that head and did not weigh it. It is still open at this head and I can place it in the kernel source.

Finding (blocks): the indicator on/off commit is nonblocking, and -EBUSY loses it. drivers/gpu/drm/apple/dcp-indicator.c:216, drm_atomic_nonblocking_commit(state).

• drm_atomic_helper_setup_commit() calls stall_checks(crtc, nonblock) (drivers/gpu/drm/drm_atomic_helper.c:2381-2410). With nonblock set it returns -EBUSY whenever the CRTC's newest commit has not completed flip_done. It repeats the check for the plane and for the connector (around :2590 and :2612). A compositor that flips every frame has such a commit in flight for much of each frame. This is the ordinary state of a live desktop, not a corner case.
• Stop path: apple_dcp_camera_indicator_set(dev, false) (:338-347) cancels the watch, calls dcp_indicator_end() (state OFF, wanted=false), then runs dcp_indicator_commit(dcp, false). If that returns -EBUSY it only logs camera indicator off commit failed and returns 0. The plane is still enabled. Nothing is pending and nothing retries. The reserved-plane policy in plane.c (dcp_ind_plane_must_restore) puts the square back after every later userspace clear. So the dot stays on screen after STREAMOFF until the next camera session happens to turn it off. The ISP side also hides the failure: it treats off as "always succeeds".
• The watch withdrawal at :264-267 and the failed-on cleanup at :283-288 discard the return value the same way.
• Start path: a -EBUSY on the on-commit makes dcp_indicator_request() return an error and the camera streams with no dot. That is within "best-effort", but on a busy desktop it is a frequent outcome rather than a rare one.
• Reproduction: static only. I read the helper source at this head; I could not run a compositor against a panel. Failing input: STREAMOFF (or the 1 s watch) while a nonblocking flip is in flight on the same CRTC.
• Fix options, author's choice. For the off and withdrawal commits, use a blocking drm_atomic_commit(): both run in sleepable process or work context. The alternative is to keep an "off pending" request and retry from the existing ci->watch on -EBUSY. Either way a failed off must not report success. The retry state needs a test in the pure-state suite.

Disclosed, not blocking: userspace can assign a framebuffer to the advertised overlay and it is discarded (Codex P1, plane.c:253-285). dcp_ind_plane_must_restore puts the old state back on any non-kernel change that is not a disable-with-CRTC. The kernel does this on purpose so that blind clears succeed. The consequence is that a compositor which chooses this overlay for direct scanout gets a successful TEST_ONLY and a successful commit, and its pixels are never shown. I did not find a test for it in either suite. It matters only on a DCP that has apple,camera-indicator-surface, which the K114c does not. It needs an explicit decision (hide the plane from userspace, or reject non-clear assignments), not a silent default.

What I ran at faddbae, all pass

• ISP host harness (tools/asahi/run-host-tests.py --only isp-tests): 16/16.
• ISP KUnit on arm64 QEMU (kunit.py run --arch=arm64 --kunitconfig=drivers/media/platform/apple/isp): 54/54, including isp_test_dcp_bound_at_stream_start.
• drivers/gpu/drm/apple/ and drivers/media/platform/apple/isp/ built at W=1 (arm64 cross, DRM_APPLE=m, VIDEO_APPLE_ISP=m): zero warnings in the changed files.
• dcp-indicator-test.c as a userspace program with my own kunit-shim.h: 6/6, including indicator_resumes_only_a_kept_request.
• CI at this head: 8/8 success (source, DWC3/ATC x3, rust-build, kunit, host-tests, dt-checks).

Mutations I made, each restored after the run

• ISP side (KUnit), all caught:
  • W1 no off after a failed channel start: caught.
  • W2 no off in stop_capture: caught.
  • W3 on never requested: caught.
  • W4 lookup never binds: now caught by isp_test_dcp_bound_at_stream_start. It survived at 6b60eeb and I disclosed it. This closes that gap.
  • W5 lookup dropped from start_capture: now caught.
  • W6 bind finds the DCP but never stores it: caught.
  • W7 a bound DCP is looked up again: caught.
• State side (dcp-indicator.h, userspace test), all caught:
  • V1 dcp_ind_end keeps wanted: caught.
  • V2 dcp_ind_begin never sets wanted: caught.
  • V3 dcp_ind_wants_resume ignores the OFF state: caught.
  • V4 dcp_ind_wants_resume never true: caught.

Not covered, and the reason

• I did not mutate time_before(jiffies, ci->deadline) in dcp_indicator_watch, or the cancel_work_sync ordering in dcp_indicator_release. Neither suite can reach them. They need a DCP and a panel. I read the ordering and the deadline race and found no fault: deadline is written and read under ctl, and mod_delayed_work replaces a pending timer. This is reading only.
• The resume path on a real rebind, DRM_APPLE=n, and the -EBUSY finding above are not run on my side. No panel, no boot.
• Resume does not loop on a failed commit: dcp_indicator_request ends the request on error.

[joshuaswarren]

[Joshua's agent] CHANGES
Cross-model review (Claude)
Head faddbae.

This withdraws my Claude GREEN at this head (6844044301). I missed Codex's open P2 (review comment 4559659280) and agree with w7X's finding (6844396901): dcp-indicator.c:216 commits on and off with drm_atomic_nonblocking_commit(), which returns -EBUSY from the helper's stall checks while the CRTC's newest commit has not reached flip_done. The off path logs and returns success with the state already OFF, so under a compositor flipping every frame the square can stay up after capture stops. The withdrawal path (:264-267) and the failed-on cleanup share the same hole. The off request needs to be retried until it lands, or committed in a way that waits for the pending flip, and a test should drive the -EBUSY case.

[joshuaswarren]

Fix for the indicator-off bug (w7X 6844396901, Codex P2 4559659280). New head 707a95521cdf. The reviews need a redo at this head.

Cause: the off commit is nonblocking, so a pending flip makes it return -EBUSY. The stop path and the watch withdrawal logged that and reported success. The plane stayed enabled, and plane.c restored the square after later userspace clears.

Change (dcp-indicator.c/.h, dcp-internal.h):

• New dcp_indicator_off() serves the stop path and the watch withdrawal. On -EBUSY it keeps off_pending and queues off_retry (50 ms) until the commit lands. Another error is warned about as final.
• A new request, the plane going away, or release cancels the retry. Release cancels off_retry after dropping ctl.
• The failed-on cleanup no longer commits off. A refused on commit changed nothing on the plane.
• swap_done now queues the resume work inside the ci->lock section (the open note from the earlier Claude review).

Proof: new case indicator_off_retries_a_pending_flip in the pure-state suite. Against a version that treats -EBUSY as done it fails 6 expectations. With the change the suite passes 7/7 (userspace build). The DRM and ISP objects build with no warnings. The ISP host tests pass 16/16.

Not run: a real -EBUSY from a compositor, the retry on hardware, swap_done and resume, and DRM_APPLE=n. The pure-state test covers the decision, not the work item or the DRM commit.

Open decision for Main (Codex P1, plane.c): userspace can assign a framebuffer to the advertised overlay and the kernel discards it. It matters only on a DCP with apple,camera-indicator-surface, which the K114c does not have. The choice is to hide the plane or reject non-clear assignments. I changed nothing here.

[joshuaswarren]

Second round on the Claude review of 707a955 and the Codex P1. New head 648dd4ca8758, two new commits on top of 707a95521cdf. Review git diff 707a95521cdf..648dd4ca8758. The reviews need a redo at this head.

ed1887f5c7b7 drm/apple: refuse a userspace assignment to the reserved indicator plane (Codex P1)

• atomic_check now returns -EINVAL for any userspace change to the reserved plane that is not a clear, so a compositor sees it in its TEST_ONLY commit and falls back.
• A clear (FB_ID=0 and CRTC_ID=0, as the console restore does) still succeeds and is put back. The kernel's own commit, an unchanged plane and a plane disabled with its CRTC pass.
• dcp_ind_plane_must_restore() is replaced by dcp_ind_plane_policy() (PASS, RESTORE, REJECT).
• Test: indicator_plane_reservation fails on a version that restores instead of rejecting.

648dd4ca8758 drm/apple: retry the camera indicator on commit too, and test the retry

• The on commit got -EBUSY and the request ended, so the camera streamed with no square. Both directions now go through dcp_ind_apply(). A -EBUSY keeps the request and queues the retry work, which asks again until the commit lands. Stop, withdrawal, a new request, losing the plane and release drop or cancel the retry. Another error is final.
• The retry decision and queueing live in the header behind a two-call seam (commit, queue_retry), so the pure-state suite drives the code the work item runs. off_pending and dcp_ind_off_result() are gone.
• Tests: indicator_refused_commit_is_retried and indicator_retry_ends_with_the_request. A mutant with queue_retry removed fails 4 expectations. A mutant that does not retry an on commit fails 4. The suite passes 8/8 in userspace. DRM and ISP objects build with no warnings.

Not run: the real work item, a real -EBUSY or TEST_ONLY commit from a compositor, hardware, DRM_APPLE=n. The container_of glue and the DRM commit stay untested; the seam covers the decision and the queueing only.

[joshuaswarren]

[Joshua's agent] GREEN
Cross-model review (Claude)
Head 648dd4c. Static review of the delta 707a955..648dd4c in full. Together with the earlier reviews at faddbae and 707a95 it covers the whole PR at this head; this delta touches only the indicator files, plane.c and the test. Not built, booted or run (no KUnit run).

This supersedes my CHANGES 6844572960.

Open threads at this head

• Codex P1 4559659269 (reserved plane): resolved by ed1887f. apple_plane_atomic_check returns -EINVAL for any userspace change that is not a clear. A blind clear (fb and crtc NULL) is still restored and succeeds. A no-op clear and a disable with the CRTC going inactive both pass. The kernel's own indicator commit is matched against indicator_commit and passes first. TEST_ONLY runs the same check, so a compositor sees the refusal at test time.
• Codex P2 4559659280 (off commit refused for a pending flip): resolved at 707a95. 648dd4c also retries the on commit, so a busy screen no longer leaves the camera streaming with no square.
• RyanTheTide's reviews are test reports; their findings were fixed earlier. No open Kilo or CodeRabbit threads. Codex has not yet reviewed the two newest commits.

Retry paths

• retry is a single NONE/ON/OFF state served by one delayed work, so an on and an off retry can never both be pending; a later attempt replaces the earlier one.
• A stop during an on retry clears it and issues the off commit. That commit is retried in turn if it is also refused.
• Release clears fb under ctl and cancels the work with cancel_delayed_work_sync.
• Retries continue only while commits keep returning -EBUSY, and release ends them.

Tests

• The new cases drive apply, retry and the plane policy through fake commit and queue hooks. I traced the expected counts by hand.
• P3, not blocking: they would still pass if the real dcp_indicator_queue_retry body or the plane.c call site were removed.

Nit (not blocking): struct dcp_ind_ops is an indirection layer for two callbacks, which an upstream reviewer may question.

[joshuaswarren]

CHANGES at 648dd4c
Cross-model review (GLM)
Model: zai/glm-5.3
[Joshua's agent]

The indicator-off finding (my comment 6844396901) is fixed at this head. The fix for the Codex P1 introduces a new failure: a suspend with the indicator lit makes the resume commit fail with -EINVAL.

Finding (blocks): DCP_IND_PLANE_REJECT fails drm_atomic_helper_resume() when the square was lit at suspend. plane.c:261-267, dcp-indicator.h dcp_ind_plane_policy().

• apple_platform_suspend() calls drm_mode_config_helper_suspend() (apple_drv.c:1990), and resume calls drm_mode_config_helper_resume() (apple_drv.c:2023). drm_atomic_helper_suspend() duplicates the current state first, so if the square is lit the saved state carries the reserved plane with fb = ci->fb and a CRTC. It then disables every plane.
• Resume is drm_atomic_helper_resume() (drm_atomic_helper.c:3917). It calls drm_mode_config_reset(), so the plane's current state is the disabled reset state. It then commits the saved state through drm_atomic_commit(). atomic_check runs for every plane in that state.
• For the reserved plane, apple_plane_atomic_check() sees: not the indicator's own commit (commit != indicator_commit), not unchanged (fb and CRTC differ from the reset state), not going off (the CRTC is being enabled), not a clear (fb is set). dcp_ind_plane_policy(true, false, false, false, false) returns REJECT, and atomic_check returns -EINVAL. The whole resume commit fails, so the panel does not come back. Before ed1887f the same input took the RESTORE branch, put the old state back and resumed fine.
• It happens when the camera is streaming and the DRM suspend callback runs before the ISP's. The ISP's own suspend stops the stream and turns the indicator off (apple_isp_suspend -> apple_isp_video_suspend -> apple_isp_stop_streaming), but it only does so when it is reached first, and I did not find an ordering between the two devices. If the indicator is off at suspend, the saved plane state is disabled, matches the reset state, and the resume commit passes.
• Reproduction: static. I read the helpers at this head. I did not suspend a machine, and I could not check the device order. The policy function result for these arguments is in dcp-indicator-test.c:204, where the same arguments are pinned to REJECT.
• Fix: userspace cannot name the kernel's framebuffer (it is drm_framebuffer_unregister_private'd at dcp-indicator.c:157). So in apple_plane_atomic_check(), a new state whose fb == ci->fb is the kernel's own saved state and is not userspace. Pass it (the square is lit again, which matches a camera that resumes) or restore it, but do not reject it. A test for that argument in the pure-state suite would pin it.

The earlier findings, checked at this head

• Indicator off lost to -EBUSY (my 6844396901, Codex P2): fixed. dcp_ind_apply() keeps the request on -EBUSY, records which commit was refused (retry), and queues dcp_indicator_retry every 50 ms. The stop path and the watch withdrawal go through dcp_indicator_off(). A request that ends, a new request, a suspend (dcp_ind_suspend) and release all drop or cancel the retry. I traced the races: retry is only touched under ci->ctl; the work queued from swap_done is now queued under ci->lock, so release cannot cancel before it is queued; a retry that races a new request is a no-op because dcp_ind_begin clears it.
• Userspace framebuffer silently dropped on the advertised overlay (Codex P1): rejected with -EINVAL as asked, with the exception above.

What I ran at 648dd4c

• ISP host harness (isp-tests): 16/16.
• ISP KUnit on arm64 QEMU: 54/54. (The ISP files are unchanged since faddbae; I ran it anyway.)
• W=1 object build of drivers/gpu/drm/apple/ and drivers/media/platform/apple/isp/ (arm64 cross, DRM_APPLE=m, VIDEO_APPLE_ISP=m): no warnings in the changed files.
• dcp-indicator-test.c as a userspace program with my kunit-shim.h: 8/8.
• CI at this head: 8/8 success (source, DWC3/ATC x3, rust-build, kunit, host-tests, dt-checks).
• Codex has not posted on 707a9552, ed1887f5 or 648dd4ca. Its two inline comments are still the old ones on 6b60eeb9 (re-anchored to 707a9552) and are answered by these commits. Nothing new to list.

Mutations I made in dcp-indicator.h, each restored, all 14 caught by the pure-state suite

• Retry: apply never queues; every refusal recorded as OFF; every refusal recorded as ON; dcp_ind_end keeps the retry; dcp_ind_begin keeps it; dcp_ind_suspend keeps it; retry ignores a missing plane; apply does not clear the retry after a commit; apply retries any error not only -EBUSY; retry never reports a final error.
• Policy: restore instead of reject; reject a blind clear; ignore the CRTC going off; ignore unchanged.
• Result by name: M1-M13 all fail at least one expectation. The control passes 8/8.

Not covered by any suite (disclosed)

• The glue in dcp-indicator.c (the retry work item, the ci->ctl locking, the cancel order in release) and the clearing argument computed in plane.c have no test. A mutation of clearing = true (every userspace change treated as a clear and put back) would not be caught.
• The OFF retry has no bound other than the request ending: if a flip never completes (the hung-fence case that apple_drv.c:575-583 describes), it polls every 50 ms until release. That is cheap, and the display is already wedged in that case. I mention it so it is a choice and not a surprise.
• Resume path, retry under a real compositor, DRM_APPLE=n: not run (no panel, no boot).

[joshuaswarren]

Third round, for the GLM CHANGES at 648dd4c (6845101560). New head 9ed5a0fe7949, one new commit on top. Review git diff 648dd4ca8758..9ed5a0fe7949. The reviews need a redo at this head.

9ed5a0fe7949 drm/apple: do not refuse the kernel's own indicator framebuffer on resume

• Confirmed by reading the helpers: drm_atomic_helper_suspend() saves the lit square, drm_atomic_helper_resume() resets the plane and commits it back, and atomic_check returned -EINVAL for an own fb that was neither unchanged nor the indicator's commit.
• apple_plane now holds a pointer to the kernel framebuffer (set at attach, cleared at release, compared by pointer only). dcp_ind_plane_policy() takes own_fb and puts such a state back like a clear: the plane resumes disabled, and the existing resume path asks for the square again if the camera still streams.
• Test: indicator_plane_reservation pins own_fb to RESTORE. A mutant that ignores own_fb fails it. Suite 8/8 in userspace. DRM objects build with no warnings.

Not run: the pointer comparison in plane.c, a real suspend and resume. The unbounded 50 ms off retry (hung-fence case) is left as is.

[RyanTheTide]

The reserved-plane policy, the retry and the resume fix read correctly, and with no indicator properties in a DT nothing changes at runtime. Two small asks: switch IND_FILL to green to match what macOS shows for the camera (orange is the microphone), and refuse apple,camera-indicator-surface at probe on DCP profiles without the H17P swap-done hook, since the square would always be withdrawn there. The earlier A18 Pro row was on an older head, so it needs a rerun on this one with the J700 DT (stream cycles, a busy compositor for the retry path, a suspend with the camera running) before merge.

[joshuaswarren]

[Joshua's agent] CHANGES
Cross-model review (Claude)
Head 9ed5a0f.

The own_fb fix stops the -EINVAL on resume. It also exposes a gap that both of my earlier passes missed: when the square was lit at suspend, nothing turns it back on after resume.

1. MEDIUM: the indicator stays dark after resume. Affected code: plane.c atomic_check, dcp_ind_plane_policy(), and dcp_indicator_request().
   • Lit across suspend:
     • The suspend commit passes (going_off), and the ISP-side state stays LIT.
     • On resume, drm_atomic_helper_resume() recommits the saved state with the kernel fb. own_fb maps that to RESTORE, so the plane goes back to disabled.
     • dcp_ind_wants_resume needs state OFF, so nothing asks for the square again. A stream that survives suspend runs with no indicator while the state machine says LIT.
     • The indicator_plane_reservation test asserts this RESTORE outcome, so it pins the gap.
   • ISP resumes before DRM:
     • set(true) hits !cs->active in dcp_indicator_commit and returns -ENODEV.
     • dcp_indicator_request then calls dcp_indicator_end, which clears wanted, so no retry is queued.
   • Suggested fix:
     • On the own_fb RESTORE path, or in the CRTC enable path, mark the state OFF (dcp_ind_suspend) and schedule_work(&ci->resume).
     • Treat -ENODEV from an inactive CRTC as "keep wanted, state OFF" rather than final.
     • Add a test case for each.
2. MEDIUM: the own_fb test covers only the pure policy.
   • Changing clearing || own_fb to clearing fails indicator_plane_reservation (dcp-indicator-test.c:207), which is good.
   • These mutants all still pass:
     • plane.c passing a constant false for own_fb.
     • indicator_fb never being set in dcp_indicator_attach_plane.
     • indicator_fb never being cleared in dcp_indicator_release.
   • A KUnit atomic_check case on a fake plane, or a small helper that gathers the arguments and that the test calls, would pin the wiring.
3. LOW: dcp-indicator-test.c includes kunit-shim.h under DCP_IND_USERSPACE, but that header is neither in the tree at this head nor in the PR, so the userspace build fails. Add the header or drop the #ifdef.
4. LOW, not a regression: after a DRM unbind and rebind, the saved state holds the old fb, which no longer matches indicator_fb. This predates the PR.

Checked and fine:

• fb lifetime: a pointer compare against a referenced fb, cleared under ctl before drm_framebuffer_put. No use-after-free or ABA found.
• Kernel commits only: the check runs under the plane lock, and indicator_commit is set only around the kernel's own commit.
• Userspace lockout: drm_framebuffer_unregister_private keeps userspace from looking up the kernel fb.
• Earlier fixes: the -EBUSY retry on the on and off commits, and the dcp_ind_ops seam tests, still hold.

Open bot threads, both outdated and addressed by the current head:

• r4559659269 (Codex P1): reject scanout on the reserved plane.
• r4559659280 (Codex P2): retry a refused off.

No human threads are open.

Not checked: kernel build, a real KUnit run, the apple_drv.c suspend hooks, ISP pm-notifier ordering, and any hardware.

[joshuaswarren]

Fourth round, for the Claude CHANGES at 9ed5a0f (6845385928). New head e8e20e8919d9, one new commit on top. Review git diff 9ed5a0fe7949..e8e20e8919d9. The reviews need a redo at this head.

e8e20e8919d9 drm/apple: show the camera indicator again after a display resume

• Item 1, lit across suspend: apple_drm_resume_outputs() now calls dcp_indicator_display_resumed() for every CRTC after drm_mode_config_helper_resume(). The plane came back disabled, so the state goes OFF, and a request that still stands queues the resume work. A request stopped during the suspend asks for nothing.
• Item 1, ISP resumes first: -ENODEV from the commit (CRTC inactive, DRM unbound) keeps the request with the square OFF and returns 0. No watch is armed for a commit that did not happen. The display resuming asks again.
• Item 2, wiring: the own framebuffer is now identified by fb->funcs, which are the indicator's alone. The indicator_fb pointer on the plane, its set at attach, its clear at release and the stale pointer after a rebind (item 4) are gone. dcp_ind_plane_policy() takes has_fb, has_crtc and own_fb, so the clear rule is pinned too.
• Item 3: tests/kunit-shim.h is added; gcc -DDCP_IND_USERSPACE -I. dcp-indicator-test.c builds as a plain program.
• Tests were written first and failed to build. Mutants that drop the -ENODEV keep, drop the resume reset, or break the own-fb and clear rule each fail one case. The suite passes 10/10 in userspace. DRM and ISP objects build with no warnings.

Not pinned: the own-fb argument in plane.c (dcp_indicator_owns_fb(new_plane_state->fb)) and the call in apple_drv.c. Each is one expression, and no DRM-object KUnit exists for this driver. A constant false in plane.c would still pass. Not run: a real suspend and resume, a panel, DRM_APPLE=n.

[joshuaswarren]

[Joshua's agent] CHANGES
Cross-model review (Claude)
Head e8e20e8.

The four earlier findings are mostly closed:

• The resume logic is fixed.
• kunit-shim.h is present, and the userspace build passes all 10 cases.
• Matching the kernel's framebuffer by its fb->funcs table removes the stale-pointer problem after a rebind.
• fb->funcs identity is safe. dcp_indicator_fb_funcs is static, userspace framebuffers use drm_gem_fb_funcs, and the kernel framebuffer is unregistered.

New findings:

1. HIGH, apple_drv.c:2025-2026: apple_drm_resume_outputs() calls to_apple_crtc(crtc)->dcp on every CRTC. On T8132 the external CRTCs are not struct apple_crtc (t8132_ext_drm.c:1038). Every other CRTC loop in this file skips them with t8132_ext_crtc(crtc) (lines 191, 691, 1743, 1782 and others). This loop does not, so with an external display attached it reads a garbage dcp pointer and hands it to platform_get_drvdata(). Fix: add if (t8132_ext_crtc(crtc)) continue;. I verified this by reading the code at this head; I did not boot it.

2. MEDIUM, dcp-indicator.c:369-372: dcp_indicator_display_resumed resets the state to OFF under ci->lock only, not ctl.

   • A set(true) that lands between the DRM resume commit and this call gets its state forced to OFF.
   • The resume work then re-requests an unchanged plane, so no swap may come.
   • The 1 s watch then withdraws the square while the camera streams. The window is narrow.
   • Fix: take ctl here, or move the reset into dcp_indicator_resume under ctl.

3. MEDIUM: the wiring is still untested. The userspace test compiles only the header, so these mutants survive:

   • dropping the apple_drv.c call;
   • owns_fb returning false;
   • wrong plane.c arguments;
   • removing the keep check at dcp-indicator.c:343-348.

   A KUnit case on a fake plane and CRTC would kill them, and so would a small argument-gathering helper that the test calls.

4. LOW: with the CRTC inactive, set(true) returns 0 silently, and the re-ask depends on swap_done, which only the h17p path has. Add a dev_dbg on the keep. Consider refusing the property at probe on profiles without that hook, which RyanTheTide's review also asks for.

Header mutants I ran:

• Killed: dropping the -ENODEV branch, dropping the resume reset, and dropping the wanted check.
• Survived: own_fb && has_fb changed to own_fb. This one is equivalent.

Open threads: two Codex threads, both outdated and addressed by code. Neither is resolved yet.

• r4559659269: P1, reserved-plane reject.
• r4559659280: P2, off retry.

RyanTheTide's review comments (green IND_FILL, a probe-time refusal, a hardware rerun at this head) are review comments, not threads.

Not checked: kernel build, KUnit, hardware, and the full ISP and DT diffs.

[joshuaswarren]

Fix for the CHANGES at e8e20e8919d9 (6845498232). New head 836fd3753a11; delta: git diff e8e20e8919d9..836fd3753a11.

1. HIGH, apple_drm_resume_outputs: the new loop now skips t8132_ext_crtc(crtc) like the other CRTC loops.
2. MEDIUM, resume reset race: dcp_indicator_display_resumed takes ci->ctl, then the plane lock (the order dcp_indicator_commit uses), and resets the state only if the plane came back dark: dcp_ind_display_resumed(ind, plane_dark). Taking ctl alone would only move the race. A plane that resume restored lit is now left alone too: the resume commit's present confirms it, and the old reset made the re-request an unchanged plane with no swap. New test case: a lit plane keeps its state, a dark one asks again.
3. MEDIUM, wiring: the policy arguments (unchanged, going_off, own_fb) are now gathered by dcp_ind_plane_check() in dcp-indicator.h over the plane and CRTC state, and plane.c calls it. dcp_indicator_owns_fb() is gone; the check compares fb->funcs with the exported dcp_indicator_fb_funcs. The userspace suite is 11 cases; mutants of each compared field, own_fb, going_off, the kernel_commit argument and the dark test each fail one case. Still untested: the apple_drv.c call and the plane.c call site themselves.
4. LOW: the -ENODEV keep logs at dev_dbg. A DCP on the H17G profile has no swap_done hook, so the probe leaves the indicator off there with a warning and the DCP still probes. I did not make this a probe failure: a missing privacy dot should not take the display down. It checks the profile only, not the firmware template.

Checked: userspace tests 11/11, DRM and ISP objects build with no warnings. Not run: hardware, a real suspend and resume, DRM_APPLE=n.

[RyanTheTide]

J700 row on this PR's 648dd4c (cherry-picked onto integration 7945f4b, which carries #1754), plus the J700 DT from #1758. Neo, one-shot test kernel, every stream to /dev/null:

• 20/20 start/stop cycles: the indicator framebuffer is on the reserved plane during each stream and gone from the DRM state after each stop; streams rc=0.
• Panel off (Mutter PowerSaveMode 3): the stream still runs (rc=0) without the indicator.
• s2idle 2/2 and s2r 2/2 with a stream running: the stream survives, the plane is back right after resume and gone after stop. 0 AVC.
• KUnit arm64: 234/235 (1 existing skip); apple-dcp-indicator 8/8, apple-isp-camera 22/22.

One bug on H17P: every off commit is dropped by the firmware path.

apple-dcp 412e00000.dcp: present dropped: backlight prepare -61
apple-dcp 412e00000.dcp: swap failed! status 4294967295

That pair shows up once per stop (27 on that boot, one per camera stop). The off commit only holds the indicator plane, so the swap has no surface: iomfb_apply_backlight_h17p() -> dcp_bl_prepare() returns -ENODATA for !have_surface, and only firmware 27 takes surface-less swaps. The DRM state says the plane is off, but the firmware never gets the removal, so slot 2 stays on the panel. The DCP blend CRC (debugfs crtc-0/crc, cursor parked top left, same clock minute) agrees: the CRC changes when the stream starts (indicator blended) and stays on the "with indicator" value after the stream stops and after a PowerSaveMode 3 -> 0 cycle.

Adding the CRTC's other planes to the indicator commit, so each indicator swap also re-presents the primary surface, should avoid it:

	cs = drm_atomic_get_crtc_state(state, crtc);
	...
	ret = drm_atomic_add_affected_planes(state, crtc);
	if (ret)
		goto out;

The on commit works today only because it carries the indicator surface itself.

[joshuaswarren]

New head f928c0f5147e. Two changes since 836fd3753a11.

1. Rebased onto integration 17903f58cab4, which carries c8682135 (apple: T8140 DCP/ISP fixes for DPMS off and suspend-to-idle #1754, ISP after s2idle). The rebase was clean; the chain is otherwise unchanged.
2. New top commit f928c0f5147e, for RyanTheTide's J700 finding (Aurora seq 807): the off commit held only the indicator plane, and on the H17P path a swap with no other surface is dropped (iomfb_apply_backlight_h17p() -> dcp_bl_prepare() returns -ENODATA for !have_surface; only firmware 27 takes it). The panel never saw the removal, and the dot stayed. dcp_indicator_commit now calls drm_atomic_add_affected_planes(state, crtc), so every indicator swap re-presents the primary. The on commit takes the same path.

Test: none that fails first. The rule is a DRM atomic call plus the firmware's reaction to a surface-less swap, and a userspace fake would only echo the call. The repro is Ryan's: 27 present dropped: backlight prepare -61 / swap failed! pairs on one boot, and the blend CRC staying at the "with indicator" value after stop. Please rerun the CRC rows at this head. I did not run it on hardware, and I do not know whether T6050 needs the same fix.

Checked: userspace suite 11/11, DRM and ISP objects build with no warnings at this head. Delta: git range-diff 6970a51ef8c3..836fd3753a11 17903f58cab4..f928c0f5147e.

[joshuaswarren]

[Joshua's agent] GREEN
Cross-model review (Claude)
Head f928c0f.

The earlier findings, checked at this head:

• Ext-CRTC crash on resume: closed. apple_drv.c:2026 skips t8132_ext_crtc.
• Resume reset race: closed. The hook takes ctl, then the plane mutex, then ci->lock, and resets only a dark plane.
• Argument gathering: now in dcp_ind_plane_check(), with 11 userspace cases. I ran 16 mutants at 836fd37: 15 were killed, and the one survivor is equivalent.
• Silent keep: closed. There is a dev_dbg on the keep, and H17G skips the property with a warning.
• Rebase: git range-diff shows all 12 commits unchanged against 836fd37.

New commit, drm_atomic_add_affected_planes(). It closes the H17P off-commit drop on a code read:

• It is called after the CRTC state is in the commit, under the same acquire ctx. -EDEADLK goes through the existing backoff.
• Only this CRTC's current planes are added (old_crtc_state->plane_mask).
• The primary re-runs apple_plane_atomic_check and becomes visible. iomfb_flush then marks its surface present, so dcp_backlight_prepare takes the have_surface path, not the -ENODATA one.
• dcp_ind_plane_check runs only for the reserved plane, so the primary is not rejected.

This still needs RyanTheTide's J700 CRC rerun at this head (the off value, and no backlight prepare -61 / swap failed! lines).

Low, non-blocking:

1. dcp-indicator.h:119 and dcp-indicator.c:367 say a plane can come back from resume "lit". The resume commit gets RESTORE (plane.c:246-264), so the dark branch is the normal path. The lit branch only covers a set(true) that lands between the resume commit and the hook taking ctl. The code is right; the comments need rewording.
2. With no framebuffer on any plane of the CRTC, the off swap is still surface-less on non-27 firmware. This is rare.
3. Nothing tests the dark line, the apple_drv.c / plane.c call sites, or the new call. Removing the new call survives every test, so the hardware rerun is the real check.
4. On T6050 the off swap now goes through dcp_backlight_prepare(have_surface=true). That is untested, as the commit message says.
5. IND_FILL is still orange (0xffff9500). Ryan asked for green.

Two Codex threads are open but outdated: the reserved-plane reject and the off retry. The code at this head addresses both.

Not run: kernel build, KUnit, hardware.

[joshuaswarren]

[Joshua's agent] New head 0f6091e76bc8 (was f928c0f5147e). One commit on top, no other change:

• The camera indicator is now green (IND_FILL 0xff34c759), as RyanTheTide asked. It was orange.
• The comments in dcp-indicator.h and dcp-indicator.c about a resumed plane are corrected. The resume commit restores the plane, so a dark plane is the normal path. A lit plane means a set(true) raced the resume, and the reset leaves it alone.

No logic change. The userspace suite still passes 11/11.

Ready for the J700 CRC rerun on this head. The rerun also covers the off commit from the earlier round: dcp_indicator_commit now adds every affected plane before the off swap, so the H17P firmware should no longer drop it ("present dropped: backlight prepare -61", "swap failed!"). I could not write a test that fails first for that call. The rerun is the proof.

#1756 is rebased on this head and only changes its commit message.

[joshuaswarren]

[Joshua's agent] GREEN
Cross-model review (Claude)
Head 0f6091e.

The one commit on top of f928c0f, which had a Claude GREEN, changes only the indicator colour and two comments:

• IND_FILL is now 0xff34c759 (green), as Ryan asked.
• dcp-indicator.h:117 and dcp-indicator.c:365 now say what the code does: the reserved plane comes back dark because the resume commit restores it (DCP_IND_PLANE_RESTORE), and a lit plane can only be a set(true) that lands after the resume commit. That closes low items 1 and 5 from my f928c0f review.

No logic changed. Still open: Ryan's J700 CRC rerun on this head. CI is pending.