| Message ID | 20250416-drm_adp_fixes-v1-0-772699f13293@jannau.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Apple Display Pipe driver fixes | expand |
> This is preferable to driver changes since keeps the device powered on > if the adpdrm module is not available during boot. Struggling to parse this sentence, do you mean to say: > Driver changes are preferred, since that patch keeps the device > powered on if the adpdrm module is not available during boot.
On Wed, Apr 16, 2025 at 04:54:38PM -0400, Alyssa Rosenzweig wrote: > > This is preferable to driver changes since keeps the device powered on > > if the adpdrm module is not available during boot. > > Struggling to parse this sentence, do you mean to say: > > > Driver changes are preferred, since that patch keeps the device > > powered on if the adpdrm module is not available during boot. no. The sentence misses "it" between "since" and "keeps". I meant to say that the linked devicetree change is preferable. A hypothetical adp_drv.c change to keep the power-domain during s2idle only works if the driver is loaded. The changes in this series (especially Patch 1/4) just prevent the soft-locked CPU after resume. The touch bar display won't display anything. Janne
While looking at a suspend issue in the Asahi downstream kernel I noticed several issues in the the ADP driver. This series fixes those issue. The root cause of the issue was that the device is unexpectedly powered down on suspend. The driver relies on initialization done by the boot loader and can not bring the device up from reset. The change in [1] annotates the power-domain as always on on devices with touchbars. This is preferable to driver changes since keeps the device powered on if the adpdrm module is not available during boot. The device comes out of reset firing interrupts with a rate of 60Hz even if vblank interrupts are disabled. This itself is not an issue. The event_lock outside of the irq handler is locked with spin_lock() resulting in a deadlock if the irq fires while the lock is held and processed on the same CPU core. This happens eventually and results in a soft-locked CPU. [Patch 1/4] "drm: adp: Use spin_lock_irqsave for drm device event_lock" addresses this. In addition I noticed that the driver does not use drm_crtc_vblank_on() and instead enables HW vblank interrupts in probe(). This may have been done to avoid errors from drm_crtc_vblank_get() after crtc reset. drm_crtc_vblank_reset() intentionally leaves struct drm_vblank_crtc in state where drm_crtc_vblank_get() cannot enable vblank interrupts. Handle this case explictly in [Patch 2/4] "drm: adp: Handle drm_crtc_vblank_get() errors". [Patch 3/4] "drm: adp: Enable vblank interrupts in crtc's .atomic_enable" then uses the expected drm_crtc_vblank_on() call to enable vblank interrupts. [Patch 4/4] "drm: adp: Remove pointless irq_lock spinlock" removes an unnecessary spinlock protecting the irq handler from itself. [1] https://lore.kernel.org/asahi/20250416-arm64_dts_apple_touchbar-v1-1-e1c0b53b9125@jannau.net/ --- Janne Grunau (4): drm: adp: Use spin_lock_irqsave for drm device event_lock drm: adp: Handle drm_crtc_vblank_get() errors drm: adp: Enable vblank interrupts in crtc's .atomic_enable drm: adp: Remove pointless irq_lock spin lock drivers/gpu/drm/adp/adp_drv.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20250416-drm_adp_fixes-1abfd4edecfe Best regards,