Repository navigation
Conversation
6f9d0c5 to
323ce0a
Compare
689db1c to
2d7bc0a
Compare
DjDeveloperr
left a comment
There was a problem hiding this comment.
[7/7] drm/apple: m4: review at 2d7bc0a
Gating: fine. iomfb_m4 only builds with CONFIG_DRM_APPLE_DCP_T8132 and runs only for DCP_FIRMWARE_M4, and external means apple,t8132-dcpext, which only t8132-j713-j715.dtsi enables. T6050/M5 never reaches it, and iomfb_m4.c merges cleanly with the M5 branch.
Stop/restart pairing: on success they mirror each other. Stop runs A500, drain, RTKit idle, CPU stop, display DART off, firmware DART off, PMGR off. Restart runs PMGR on, firmware DART on, display DART on, retained wake, finish. The failure paths don't pair.
Findings (most severe first)
- A failed stop after
ext_stopped = truehas no rollback, and nothing ever restarts it. If display-DART off, firmware-DART off orconsumer_off()fails,.suspendreturns the error, and the PM core never calls.resumefor a device whose.suspendfailed..completeonly clearsexternal_suspended. The CPU stays stopped, RX held, both DARTscio_active, and a DART whoseoff()got pastsleep_close()stays closed. The next unmap on it panics through #1416's writer gate, and a release panics.
There's a concrete trigger:apple_dart_cio_off()returns-EBUSYwhenever a map or unmap is in flight on that DART. Suggestion: unwind in reverse withconsumer_on(needs #1414 note 1),apple_dart_cio_on()x2 (documented to undo a partial off), the retained wake andcio_finish()x2. At minimum, restart from.completewhenext_stoppedis still set. - No pre-check.
iomfb_m4_power_gate()first asks every provider (iomfb_m4_gate_check(), includingapple_pmgr_ps_consumer_check()); this path doesn't. A failed or already-off PMGR chain shows up only at the last step, after the CPU stopped and both DARTs went off, and lands in (1). - A failed resume leaves the DARTs closed. If
iomfb_m4_ext_start()fails atconsumer_onor a DARTon(), the PM core only logs the error and the system carries on withcio_activeset and possibly closed DARTs. The next framebuffer or shared-buffer unmap then panics. - A refused stop quarantines the session. Failures before the CPU stops (A500, drain finding messages, the late message) call
dcp_m4_failed()and also refuse the sleep. The internal path does the former but not the latter, so after a refused sleep the external display stays dead until reboot or replug. - J715 shares t8132-j713-j715.dtsi, but #1416 binds owners only on
apple,j713. J715 logs "no DART owner for a deep sleep: -95" on every boot and refuses each deep sleep with-ENODEVin.suspend. The outcome is unchanged (its dispext DART refused before), but the refusal now comes later and louder.
Verdict: needs changes, (1) to (3).
Review only: read at the head commit, not built or run (no J713 here). Written with Claude Code.
| fail: | ||
| dev_err(dcp->dev, "external DCP stop failed at %s: %d\n", step, ret); | ||
| dcp_m4_failed(dcp, ret); | ||
| if (dcp->ext_stopped) |
There was a problem hiding this comment.
After ext_stopped = true this returns with the DCP CPU stopped, RX held, both DARTs still cio_active, and possibly one of them closed. .suspend failing means the PM core never calls dcp_platform_resume() for this device, and dcp_platform_complete() doesn't restart it either. A closed DART then panics on the next unmap (#1416's writer gate), and any release panics. apple_dart_cio_off() returns -EBUSY from apple_dart_sleep_close() whenever a writer is in flight, so this is reachable. Suggest unwinding here in reverse (apple_pmgr_ps_consumer_on(), apple_dart_cio_on() on both, which the header says undoes a partial off, apple_rtkit_wake_retained(), then apple_dart_cio_finish() on both), or at least calling iomfb_m4_ext_start() from .complete when ext_stopped is still set.
| return -EALREADY; | ||
| if (!r || r->ext_qn || dcp->transaction.calls || !READ_ONCE(dcp->ext_dart_down)) | ||
| return -EBUSY; | ||
| ret = dcp_m4_runtime_live(r); |
There was a problem hiding this comment.
The internal path asks every provider before its first step (iomfb_m4_gate_check()); this one only checks the DART binding. Adding apple_pmgr_ps_consumer_check(dcp->dev) here means a failed or already-off PMGR chain refuses the sleep with nothing touched, instead of failing at the last step, after the CPU and DARTs are down.
| return 0; | ||
| fail: | ||
| dev_err(dcp->dev, "external DCP stop failed at %s: %d\n", step, ret); | ||
| dcp_m4_failed(dcp, ret); |
There was a problem hiding this comment.
This quarantines the session for every failure, including the ones before RTKit idle where nothing has changed yet (A500 NAK, drain finding messages), and the sleep is refused as well. The internal path takes the same dcp_m4_failed() but doesn't refuse the sleep. Here the user gets a refused sleep and a dead external display until reboot or replug. Consider returning -EBUSY without dcp_m4_failed() for failures before apple_rtkit_idle().
| apple_dart_cio_finish(dcp->ext_darts[0]); | ||
| } | ||
| if (ret) { | ||
| dev_err(dcp->dev, "external DCP restart failed at %s: %d\n", step, ret); |
There was a problem hiding this comment.
Resume errors are only logged by the PM core, so after this the system runs on with both DARTs cio_active and, if consumer_on or a DART on() failed, closed. The next unmap of a framebuffer or DCP shared buffer then panics (#1416). Even when the session is lost, the DART transactions need to end, with streams off but bookkeeping released.
| iomfb_m4_ext_bind_dart(dcp, piodma, &dcp->ext_darts[1]); | ||
| of_node_put(piodma); | ||
| if (ret) | ||
| dev_info(dev, "no DART owner for a deep sleep: %d\n", ret); |
There was a problem hiding this comment.
On J715 (same t8132-j713-j715.dtsi, dcpext enabled) this always fails with -EOPNOTSUPP, because #1416 only binds on apple,j713. Every deep sleep with an adopted session is then refused with -ENODEV in .suspend. Its dispext DART refused deep sleep before too, so the outcome is unchanged, but either include J715 in #1416 or make this message say the board isn't supported.
A deep sleep powers the J713's external display coprocessors off. Each one is adopted running from the boot loader and until now was left running across a sleep, which is only valid for suspend-to-idle: its DARTs refuse a deep sleep, and the sleep controller refuses one with a bound display coprocessor still running. Stop an external DCP that has a session in its deep-sleep suspend, as the internal one is powered down: release the AP link, let RTKit idle, stop the CPU, power its display DART and then its firmware DART down through their owner interface, and power its CPU, front-end and system domains off. In resume, power the domains on, restore both DARTs and start the CPU again through RTKit's retained wake. The firmware starts from its reset vector on the image it kept, and the display comes back with its next enable. The PMGR chain is asked first. A refusal before RTKit idles changes nothing and keeps the session. A failure after the CPU stopped is undone in reverse, and the sleep is refused: a CPU whose domain went off starts again through the retained wake, one that was only stopped runs on and gets RTKit's ordinary wake. A failed restart still ends both DART transactions and fails the session. The sequence is the internal DCP's deep-sleep power-down applied to the external ones, for the T8140 sleep controller's T8132 support. This covers a DCP with nothing attached too: it has a session from adoption and is stopped and restarted the same way. The DART owners are bound at probe. Binding adds a device link, which must not happen from a PM callback. A board whose DARTs have no owner (the J715 for now) keeps refusing a deep sleep with an external display. Based-on-work-by: Bartosz Nitka <niteria@gmail.com> Based-on-work-by: Diljit Singh <hey@djdev.me> Signed-off-by: Erik Bourget <erik@ebourget.net>
323ce0a to
903277c
Compare
2d7bc0a to
f5a016e
Compare
The J713's two external display coprocessors are adopted running from the boot loader and until now were left running across a sleep. That is only valid for suspend-to-idle: a deep sleep powers them off, their DARTs refuse it, and the sleep controller refuses to sleep with a bound display coprocessor still running.
Stop an external DCP in its deep-sleep suspend, the way the internal one is powered down: release the AP link, let RTKit idle, stop the CPU, power its display DART and then its firmware DART down, and power its CPU, front-end and system domains off. In resume, power the domains on, restore both DARTs and start the CPU again through RTKit's retained wake. The firmware starts from the image it kept, and the display comes back when the connection manager pairs its tunnel again.
The PMGR chain is asked first. A refusal before RTKit idles changes nothing and keeps the session. A failure after the CPU stopped is undone in reverse and refuses the sleep: a CPU whose domain went off starts again through the retained wake, one that was only stopped runs on with RTKit's ordinary wake. A failed restart still ends both DART transactions and fails the session. A board without DART owners (the J715 for now) says so at probe and keeps refusing a deep sleep with an external display.
A DCP with nothing attached has a session from adoption and is stopped and restarted the same way. With this, the J713 sleeps and wakes with a dock and external displays attached.
Part 7/7 of J713 docked S2R, stacked on the native radio series (#1384), which is stacked on #1314.
Series order: #1414 → #1415 → #1416 → #1417 → #1418 → #1419 → #1420.
Depends on #1419.
Validation of the series on this base:
Based-on-work-by:trailers and style in thetools/asahifixtures.The changes since that version (the DT-selected DART owners and memory-only updates while closed, the unplug handling, the failure unwinds, the stock tb.c resume restored, the PMGR and RTKit hardening) are built and covered by the host suites, but have not run on the J713 yet.
Not covered: repeated sleeps on one boot, wake sources other than the RTC, the desktop's own sleep path (lid, logind), a dock unplugged during the sleep on hardware, and docks other than the TS5. Both native parameters (
pcie_apple.cio_s2r,thunderbolt_apple.cio_s2r) default to off.This series was developed and tested by Claude (Anthropic), working with me on the J713.