radxa-dragon-q8b: pick up EDID changes behind the HDMI bridge - #10343
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a Dragon Q8B ALSA installation hook, expands SC8280XP kernel support, and updates HDMI bridge handling plus Qualcomm DisplayPort recovery, hotplug, PHY, and audio paths. ChangesDragon Q8B board support
Display and audio support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Display as msm_dp_display
participant Controller as msm_dp_ctrl_retrain_link
participant Audio as msm_dp_audio_restore
participant DRM as DRM connector
Display->>Controller: retrain link after recovery
Controller-->>Display: return link status
Display->>Audio: restore active audio configuration
Display->>DRM: emit hotplug event after retry exhaustion
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
08
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patch (1)
70-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSynchronize EDID work with unplug teardown.
cancel_delayed_work()cancels only pending work and may return whilemsm_dp_edid_recheck_work()is already running. The running callback can still callmsm_dp_panel_recheck_edid()aftermsm_dp_aux_enable_xfers(dp->aux, false), reading DDC when AUX transfers are disabled.Use a synchronization point that ensures the worker has stopped before AUX teardown. Use
cancel_delayed_work_sync()if it cannot deadlock with the unplug path; ifmode_config.mutexcauses deadlock, serialize the worker and unplug path with a shared lock instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patch` around lines 70 - 75, Synchronize the EDID worker with unplug teardown by updating the delayed-work cancellation in the unplug path to use cancel_delayed_work_sync(), ensuring msm_dp_edid_recheck_work() has completed before msm_dp_aux_enable_xfers(dp->aux, false). If this introduces a mode_config.mutex deadlock, instead protect both the worker and unplug teardown with a shared lock while preserving the existing EDID recheck behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/kernel/linux-sc8280xp-vendor.config`:
- Around line 1273-1293: Regenerate the kernel configuration overlay using
rewrite-kernel-config with the appropriate BOARD and BRANCH values, replacing
the full generated defconfig in linux-sc8280xp-vendor.config with the resulting
minimal delta. Review the regenerated diff and retain only the intended
configuration changes before merging.
---
Outside diff comments:
In
`@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patch`:
- Around line 70-75: Synchronize the EDID worker with unplug teardown by
updating the delayed-work cancellation in the unplug path to use
cancel_delayed_work_sync(), ensuring msm_dp_edid_recheck_work() has completed
before msm_dp_aux_enable_xfers(dp->aux, false). If this introduces a
mode_config.mutex deadlock, instead protect both the worker and unplug teardown
with a shared lock while preserving the existing EDID recheck behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0466e99f-7575-452a-8c3c-f92bc6fbef9a
📒 Files selected for processing (8)
config/boards/radxa-dragon-q8b.confconfig/kernel/linux-sc8280xp-vendor.configconfig/optional/boards/radxa-dragon-q8b/_packages/bsp-cli/usr/share/alsa/ucm2/Qualcomm/sc8280xp/Dragon-Q8B-HiFi.confconfig/optional/boards/radxa-dragon-q8b/_packages/bsp-cli/usr/share/alsa/ucm2/conf.d/sc8280xp/RadxaComputerCo.Ltd.-RadxaDragonQ8B-1.1.confpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-restore-audio-open-pcm.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-restore-audio-config.patch
💤 Files with no reviewable changes (2)
- config/optional/boards/radxa-dragon-q8b/_packages/bsp-cli/usr/share/alsa/ucm2/conf.d/sc8280xp/RadxaComputerCo.Ltd.-RadxaDragonQ8B-1.1.conf
- config/optional/boards/radxa-dragon-q8b/_packages/bsp-cli/usr/share/alsa/ucm2/Qualcomm/sc8280xp/Dragon-Q8B-HiFi.conf
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch (3)
129-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate lock and state failures instead of reporting recovery success.
When
drm_modeset_lock()fails, the code jumps tounlockwhilercis still0. The same occurs when the second power/plug check skipsmsm_dp_ctrl_retrain_link(). The success path then clears retries and setsDRM_MODE_LINK_STATUS_GOODwithout a successful retrain. This can suppress the retry and hotplug fallback.Store lock errors in
rc, and gate theGOODpath on an explicit successful-retrain result.Also applies to: 156-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch` around lines 129 - 151, Update the recovery flow around drm_modeset_lock and msm_dp_ctrl_retrain_link so every lock failure is propagated through rc rather than leaving it successful. Treat the power_on/plugged guard skipping retrain as unsuccessful, and gate retry clearing plus DRM_MODE_LINK_STATUS_GOOD on an explicit successful retrain result. Preserve the existing retry and hotplug fallback behavior for all failure paths.
195-200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet
LINK_STATUS_BADbefore sending the final recovery hotplug.
msm_dp_bridge_atomic_enable()schedules recovery on train failure without updating the connector link-status property. The recovery path only writes GOOD after success and its final hotplug comment relies on BAD being set previously. If atomic-enable failure exhausts retries while the prior status was GOOD, userspace may skip the required fresh modeset. Set the status in the recovery worker before falling through to the hotplug event, or carry the pending failure state into the worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch` around lines 195 - 200, Update the link-training failure path in msm_dp_bridge_atomic_enable and the associated hpd_recovery_work handling to set the connector link-status property to LINK_STATUS_BAD before issuing the final recovery hotplug. Ensure the pending failure state is preserved when recovery is scheduled, while retaining the existing LINK_STATUS_GOOD update on successful recovery.
114-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the recovery path on the shared serialization lock.
The new path acquires DRM modeset locks, but HPD teardown still uses
dp->plugged_lockaroundplugged,power_on, and controller/clock state. A teardown can race this recovery work, so the checks / controller call should be covered by the same lock that protects HPD state transitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch` around lines 114 - 145, Update the recovery path in the HPD recovery function around msm_dp_ctrl_retrain_link to acquire and hold dp->plugged_lock while checking dp->plugged and power_on and invoking controller retraining. Preserve the existing DRM modeset locking, but ensure the shared HPD serialization lock covers the state checks and controller/clock access so teardown cannot race them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch`:
- Around line 129-151: Update the recovery flow around drm_modeset_lock and
msm_dp_ctrl_retrain_link so every lock failure is propagated through rc rather
than leaving it successful. Treat the power_on/plugged guard skipping retrain as
unsuccessful, and gate retry clearing plus DRM_MODE_LINK_STATUS_GOOD on an
explicit successful retrain result. Preserve the existing retry and hotplug
fallback behavior for all failure paths.
- Around line 195-200: Update the link-training failure path in
msm_dp_bridge_atomic_enable and the associated hpd_recovery_work handling to set
the connector link-status property to LINK_STATUS_BAD before issuing the final
recovery hotplug. Ensure the pending failure state is preserved when recovery is
scheduled, while retaining the existing LINK_STATUS_GOOD update on successful
recovery.
- Around line 114-145: Update the recovery path in the HPD recovery function
around msm_dp_ctrl_retrain_link to acquire and hold dp->plugged_lock while
checking dp->plugged and power_on and invoking controller retraining. Preserve
the existing DRM modeset locking, but ensure the shared HPD serialization lock
covers the state checks and controller/clock access so teardown cannot race
them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3306b5b6-3f4f-4168-bfe5-cc6a2810e9df
📒 Files selected for processing (3)
patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-restore-audio-open-pcm.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch
💤 Files with no reviewable changes (1)
- patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-recheck-edid-after-plug.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-restore-audio-open-pcm.patch
SuperKali
force-pushed
the
q8b-vendor-dp-audio-m2
branch
from
August 5, 2026 21:21
06176e7 to
319b13b
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
SuperKali
marked this pull request as draft
August 5, 2026 22:02
Member
Author
|
WIP |
SuperKali
changed the title
SuperKali
marked this pull request as ready for review
August 6, 2026 10:30
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
patch/kernel/archive/sc8280xp-vendor/drm-bridge-simple-restart-edid-caching-converters.patch (1)
281-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWriting
connector->statusfrom.get_modesfights the probe helper.
drm_helper_probe_single_connector_modes()setsconnector->statusfrom the detect result, and it runs.get_modesafter that. The assignment at line 286 is overwritten on the next probe, and the disconnected state is really carried bysimple_bridge_detect()throughreset_pending. Removing the direct assignment keeps one owner for the connector status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/sc8280xp-vendor/drm-bridge-simple-restart-edid-caching-converters.patch` around lines 281 - 291, Remove the direct connector->status assignment from the .get_modes implementation around the mode-list cleanup and hpd_work scheduling. Let simple_bridge_detect() and the probe helper own connector status through reset_pending, while preserving the existing mode destruction, hpd_disconnected update, work scheduling, and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@patch/kernel/archive/sc8280xp-vendor/arm64-dts-dragon-q8b-hdmi-bridge-supply-timing.patch`:
- Around line 25-37: Verify that vcc_5v_s is the sole CH7218A supply and has no
other consumers before removing regulator-boot-on. Update the 3.3 V rail timing
and associated message to describe the actual HDMI bridge supply hardware, and
ensure the startup/off-on delays apply to the correct rail rather than the reset
RC rail.
In
`@patch/kernel/archive/sc8280xp-vendor/drm-bridge-simple-restart-edid-caching-converters.patch`:
- Around line 96-100: Update simple_bridge_reset_locked() so it checks powered
and vdd before clearing reset_pending or edid_sampled, preserving reset_pending
when no restart occurs; clear both flags only after the power-cycle restart
completes. In the access at the referenced line, use
READ_ONCE(sbridge->reset_pending) to match the existing WRITE_ONCE/READ_ONCE
access pattern.
- Around line 89-139: Move the regulator power cycle out of
simple_bridge_reset_locked() when invoked by simple_bridge_post_disable(): add a
deferred work item whose handler acquires sbridge->lock and performs the reset,
while post_disable() only marks reset_pending and queues the work. Preserve
disconnected detection while reset_pending is set, and ensure the worker is
safely cancelled or synchronized during simple_bridge_detach().
In
`@patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-treat-a-replug-as-a-sink-change.patch`:
- Around line 35-42: Update the replug handling around the hpd_isr_status check
so downstream consumers reliably observe a sink change rather than coalescing
the back-to-back disconnected and connected notifications. Confirm all
registered bridge consumers preserve both states; otherwise introduce and
propagate an explicit sink-change/replug path, while retaining normal connected
notification behavior.
- Around line 36-38: Declare and initialize was_plugged within the affected
function or remove the dependency, ensuring the conditional
drm_bridge_hpd_notify call remains valid and the patch builds independently.
---
Nitpick comments:
In
`@patch/kernel/archive/sc8280xp-vendor/drm-bridge-simple-restart-edid-caching-converters.patch`:
- Around line 281-291: Remove the direct connector->status assignment from the
.get_modes implementation around the mode-list cleanup and hpd_work scheduling.
Let simple_bridge_detect() and the probe helper own connector status through
reset_pending, while preserving the existing mode destruction, hpd_disconnected
update, work scheduling, and return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a0619cf-e735-4c9f-b1b6-973f265df1ff
📒 Files selected for processing (9)
patch/kernel/archive/sc8280xp-vendor/arm64-dts-dragon-q8b-hdmi-bridge-supply-timing.patchpatch/kernel/archive/sc8280xp-vendor/drm-bridge-simple-restart-edid-caching-converters.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-quiet-transient-link-training-noise.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-restore-audio-open-pcm.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-restore-audio-config.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-stop-enabling-pll-unlocked-irq.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-treat-a-replug-as-a-sink-change.patchpatch/kernel/archive/sc8280xp-vendor/drm-msm-dp-verify-the-phy-is-up-before-powering-it.patch
🚧 Files skipped from review as they are similar to previous changes (3)
- patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-restore-audio-config.patch
- patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-retrain-link-in-place.patch
- patch/kernel/archive/sc8280xp-vendor/drm-msm-dp-restore-audio-open-pcm.patch
EvilOlaf
approved these changes
Aug 6, 2026
Contributor
|
✅ This PR has been reviewed and approved — all set for merge! |
The Q8B profile only matches once the DMI rules from the Radxa alsa-ucm-conf fork are present, and that part is not upstream, so the UCM files carried in the BSP were never picked up. Pull the backport package at build time and drop the local copies. Signed-off-by: SuperKali <hello@superkali.me>
Without POWER_SEQUENCING_PCIE_M2 the M.2 E-key connector node stays without a driver and fw_devlink parks the onboard USB hub and the WLAN PCIe port in deferred probe. Enable it along with the wifi drivers the slot is likely to see, the QSEE TPM, and drop a stray malformed line. Signed-off-by: SuperKali <hello@superkali.me>
Link maintenance retrains the link under an active stream but leaves the audio registers on the previous link parameters, and a disable with the PCM still open never reaches the codec shutdown, so playback stays silent until the sink is suspended and resumed by hand. Signed-off-by: SuperKali <hello@superkali.me>
The deferred EDID recheck is gated on the sink being a DP branch device, and the bridge on the only board using this kernel reports no downstream port even though it converts to HDMI, so the work was never armed. Remove it rather than keep dead code around. Signed-off-by: SuperKali <hello@superkali.me>
The CH7218A samples its sink EDID only while booting, so a KVM switching the EDID it emulates goes unnoticed, and its supply is the only way to restart it. Two msm fixes ride along: the replug interrupt that was enabled but never read, and the phy left powered across exit. Signed-off-by: SuperKali <hello@superkali.me>
The bodies had grown to three and four paragraphs restating the same point at different zoom levels. Trim them to two, and regenerate the series so the hunk headers carry function context and the diffstats match what git format-patch emits. No code changes. Signed-off-by: SuperKali <hello@superkali.me>
simple_bridge_reset_locked() cleared reset_pending before checking the rail was still up, so a restart asked for after post_disable had already dropped the supply was thrown away and the converter kept serving the stale EDID. Clear the flags once the restart runs, and name the rail the reset RC actually hangs off in the DTS description. Signed-off-by: SuperKali <hello@superkali.me>
SuperKali
force-pushed
the
q8b-vendor-dp-audio-m2
branch
from
August 6, 2026 11:47
b1f374c to
d745a8d
Compare
github-actions
Bot
removed
the
Ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The Q8B puts a CH7218A between the SoC and the HDMI connector, and the converter samples the EDID of whatever sits downstream only while its firmware boots. A monitor swapped behind a KVM, or a KVM switching the EDID it emulates, goes unnoticed: the converter keeps serving the copy it took at power-up, and the connector keeps advertising a sink that is no longer there. An earlier revision of this PR said there was no driver fix for that. There is one, it just isn't pretty.
Nothing on the converter is reachable from the SoC. Its serial port lands on two test points with the pull-ups unpopulated, and its reset pin hangs off an RC to the 3.3V rail with no GPIO anywhere near it, so the supply is the only thing software can touch. The bridge driver now notices the EDID changing under a connection that never dropped, which can only mean the converter went back for it and failed, and cycles the rail so it samples again on the way up. The off time and the boot time are properties of the board and come from the supply description in the DT.
Cutting the rail naively takes the board down. The source sees a real unplug, msm runs its hotplug handling from the IRQ thread in parallel with an atomic commit, and pulls the PHY and link clocks out from under it. On sc8280xp an unclocked register access raises an SError and the hypervisor resets the SoC, leaving no oops and nothing in the journal. Masking hotplug at the source for the duration of the cycle fixes it.
Two msm bugs fell out of the investigation and stand on their own. The controller raises a replug interrupt when a sink leaves and returns too quickly for the two edges to be reported apart, the driver enables it in DP_DP_HPD_INT_MASK, and no branch in the IRQ thread ever reads it. And msm_dp_ctrl_phy_exit takes the PHY down without powering it off first, which every other caller in that file does, leaving the PHY core carrying a power count for hardware that is already gone.
The rest carries over from the earlier revision. Audio survives a sink replug and a link retrain instead of going silent until the sink is suspended by hand, the UCM files come from Radxa's own alsa-ucm-conf package rather than being carried in the BSP by hand, and the kernel config picks up the options the vendor kernel wants for M.2 power sequencing, wifi and the QSEE TPM.
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes