ASoC: SOF: Intel: hda: Fixes for ACE2+ HD-DMA stream constraints for PPLC_LLP corruption#5745
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Intel SOF HDA multi-link (mlink) initialization and LinkDMA stream allocation on ACE2+ platforms to avoid hardware errata affecting LLP/timestamps and PPLC/LLP stability.
Changes:
- Move one-time mlink enumeration into
hda_dsp_ctrl_init_chip()(post-CRST deassert) and make enumeration idempotent. - Add ACE2+ LinkDMA stream allocation constraints by tracking active/used stream indices per direction and link type, and extend the DMA ops API to pass
hlinkinto the allocator. - Introduce an mlink link-type classifier (
hda_bus_ml_link_get_type()) to categorize streams (HDA vs SDW vs UAOL vs other).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sound/soc/sof/intel/hda.h | Adds per-direction allocation masks and a persistent “HDA playback used” mask to sof_intel_hda_dev. |
| sound/soc/sof/intel/hda.c | Removes mlink enumeration/put calls from hda_init_caps() since enumeration is now done during controller init. |
| sound/soc/sof/intel/hda-mlink.c | Makes hda_bus_ml_init() skip re-enumeration; adds hda_bus_ml_link_get_type() helper. |
| sound/soc/sof/intel/hda-dai.c | Threads hlink into the assign_hext_stream() callback path. |
| sound/soc/sof/intel/hda-dai-ops.c | Implements constrained LinkDMA stream selection and updates active/used masks on assign/release. |
| sound/soc/sof/intel/hda-ctrl.c | Clears allocation masks on controller init and performs mlink enumeration post-reset. |
| include/sound/hda-mlink.h | Adds the link-type enum and exports the new link-type helper API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -670,8 +668,6 @@ static int hda_init_caps(struct snd_sof_dev *sdev) | |||
| if (!HDA_IDISP_CODEC(bus->codec_mask)) | |||
| hda_codec_i915_display_power(sdev, false); | |||
|
|
|||
There was a problem hiding this comment.
hda_bus_ml_put_all() is no longer called anywhere in the tree (search shows only the definition), which means non-alternate links keep their initial ref_count = 1 and will remain logically “in use” for the lifetime of the driver. This changes power-management behavior (e.g., hda_bus_ml_resume() will always power up all non-alt links after suspend). If the intent is only to keep links powered through first use, consider adding a later/conditional hda_bus_ml_put_all() (or equivalent refcount drop) once the PPLC/LLP issue is no longer a risk, or gating the behavior to the affected platforms only.
| /* | |
| * Drop the initial non-alternate multi-link references once the | |
| * first-use initialization sequence is complete so links are not | |
| * kept logically in use for the lifetime of the driver. | |
| */ | |
| hda_bus_ml_put_all(bus); |
| u32 link_dma_active_sdw_mask[2]; | ||
| u32 link_dma_active_multi_mask[2]; |
There was a problem hiding this comment.
The per-direction masks are declared with a hard-coded [2] size but are indexed by SNDRV_PCM_STREAM_*. Using SNDRV_PCM_STREAM_LAST + 1 for the array size would avoid a magic number and stay correct if ALSA stream direction constants ever change.
| u32 link_dma_active_sdw_mask[2]; | |
| u32 link_dma_active_multi_mask[2]; | |
| u32 link_dma_active_sdw_mask[SNDRV_PCM_STREAM_LAST + 1]; | |
| u32 link_dma_active_multi_mask[SNDRV_PCM_STREAM_LAST + 1]; |
| if (is_sdw) | ||
| concur_block_mask = sof_hda->link_dma_active_multi_mask[!stream_dir]; | ||
| else if (is_multi) | ||
| concur_block_mask = sof_hda->link_dma_active_sdw_mask[!stream_dir]; | ||
| if (is_play && !is_hda) |
There was a problem hiding this comment.
Using !stream_dir to select the opposite direction relies on SNDRV_PCM_STREAM_PLAYBACK/CAPTURE being exactly 0/1. Consider using stream_dir ^ 1 or an explicit conditional to avoid coupling this logic to the exact numeric values of the ALSA constants.
…chip() Move the hda_bus_ml_init() call from hda_init_caps() into hda_dsp_ctrl_init_chip(), right after the HDA controller reset has been de-asserted and unsolicited responses have been accepted. hda_dsp_ctrl_init_chip() already calls hda_bus_ml_reset_losidv() at the end of its sequence to clear the stream-to-link mapping. On first boot this call was a no-op because the multi-link list had not yet been populated: hda_bus_ml_init() only runs later in hda_init_caps(). Enumerating the links inside init_chip() makes the LOSIDV reset effective on first boot as well, without adding a second reset call from the probe path. hda_bus_ml_init() now returns early when the hlink_list is already populated, so the subsequent invocations from the D3 resume path (hda_resume() -> hda_dsp_ctrl_init_chip(false)) are no-ops. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Drop the hda_bus_ml_put_all() call at the end of hda_init_caps(). On multi-link (mlink) capable platforms the non-alternate links (HDaudio and iDisp) are powered on by hardware when CRST# is de-asserted (LCTL.SPA = 1) and their ref_count is pre-charged to 1 in hda_ml_alloc_h2link() to match this state. The put_all call immediately dropped that reference and toggled LCTL.SPA back to 0, relying on the first stream open to power the link up again. On ACE2+ platforms this redundant SPA 1->0->1 toggle at probe leaves the Processing Pipe Capability (PPLC) Linear Link Position counters in a state where they do not advance on the first stream after boot. The counters only start working after the first full runtime suspend/resume cycle, which includes a CRST# assert/deassert that fully resets the PPC AON block. Keep the non-alt links powered from CRST# de-assert through first use. System suspend still powers them down via hda_bus_ml_suspend(), and resume relies on CRST# de-assert to bring them back up, so no other path is affected. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The helper became unused after probe no longer drops all non-alt links, so remove the dead API and implementation. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
On ACE2+ platforms the link DMA stream allocator must avoid two hardware errata in mlink-capable systems: - Concurrent (cross-direction) hazard: when SoundWire shares a physical link DMA stream index with HDaudio, iDisp or UAOL across the two directions, the LLP and timestamp values for the affected stream are wrong. SSP and DMIC are not affected because every DMA request from those links carries one sample block. - Sequential (playback only) hazard: once a HDaudio or iDisp link has used a playback stream index, that index cannot drive any non HDA/iDisp link in the same direction until the next controller reset (CRST#). Track the active link type per direction in two masks (one for SoundWire, one for HDA/iDisp/UAOL) and the persistent set of playback stream indices touched by HDA/iDisp in a third mask. The link DMA allocator skips streams that would violate either rule. Streams are released from the active masks when the stream is released; all masks are cleared in hda_dsp_ctrl_init_chip() because the CRST# performed there clears the hardware state as well. A new helper hda_bus_ml_link_get_type() returns the link type from the existing extended link descriptor so the SOF allocator can tell SoundWire, HDA/iDisp and UAOL apart without duplicating the parsing. The implementation is generic. On platforms older than ACE2 every link is reported as HDA, only the sequential mask is ever set and it has no effect because no other link types are present, so behavior is unchanged. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
736f7d8 to
d0bfef8
Compare
|
Changes since v1:
|
| if (!HDA_IDISP_CODEC(bus->codec_mask)) | ||
| hda_codec_i915_display_power(sdev, false); | ||
|
|
||
| hda_bus_ml_put_all(bus); |
There was a problem hiding this comment.
@ujfalusi Just to double check, this does not keep the display codec powered up (if only SDW used), right? It shouldn't if only the link is powered. I'll do some local testing on this as well...
kv2019i
left a comment
There was a problem hiding this comment.
Local testing all ok, and reread of the commits is good as well. I think this is good to go!
This series improves multi-link initialization and link DMA stream allocation on Intel ACE2+ platforms.
Patch 1 moves multi-link enumeration from hda_init_caps() into hda_dsp_ctrl_init_chip(), right after controller reset de-assertion. This makes the LOSIDV reset effective on first boot and simplifies the probe and resume paths by ensuring link enumeration happens at a more appropriate point in the initialization sequence.
Patch 2 fixes Processing Pipe Capability (PPLC) Linear Link Position counter stalls on ACE2+ by keeping non-alternate links powered from controller reset through first use, rather than immediately powering them down at probe. This eliminates redundant SPA toggles that leave the counter in a non-functional state until the first suspend/resume cycle.
Patch 3 fixes critical hardware errata in mlink-capable systems on ACE2+ by tracking active link types per direction. It prevents two hardware hazards: the concurrent (cross-direction) hazard where SoundWire shares a link DMA stream index with HDaudio/iDisp/UAOL causing incorrect LLP and timestamp values, and the sequential (playback only) hazard where HDaudio/iDisp playback stream indices cannot be reused by other link types until controller reset. The implementation is generic and backward compatible—on older platforms it has no effect.
Together these changes provide a robust multi-link infrastructure foundation for ACE2+ platforms while maintaining backward compatibility with older hardware.