BRA/BPT Support for AMD Platforms(ACP 7.0/7.1/7.2 variants) and code fixes - #5867
BRA/BPT Support for AMD Platforms(ACP 7.0/7.1/7.2 variants) and code fixes#5867saba-kareem wants to merge 4 commits into
Conversation
|
Can one of the admins verify this patch?
|
There was a problem hiding this comment.
Pull request overview
Adds BRA/BPT (bulk register access / bulk payload transfer) support for AMD SoundWire managers on ACP 7.0+ platforms, including synchronization to protect shared BRA resources across multiple manager instances, and tightens SoundWire core/Intel handling around BPT stream lifecycle and exclusivity.
Changes:
- Plumb a new ACP-wide
acp_bra_lockmutex through AMD ACP/PS sound drivers into the SoundWire AMD manager to serialize BRA DMA/PTE programming across instances. - Update SoundWire core stream handling to treat BPT as bus-exclusive vs active audio streams and to avoid (re)programming non-BPT runtimes while a BPT transfer owns the bus.
- Implement AMD BRA/BPT transfer support (DMA + ATU/PTE setup, error handling, runtime PM + suspend/remove safety guards) and fix Intel ACE2x BPT stream publication/teardown ordering.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sound/soc/sof/amd/acp.h | Adds acp_bra_lock to ACP device data for BRA/BPT cross-instance serialization. |
| sound/soc/sof/amd/acp.c | Initializes and passes acp_bra_lock into SoundWire AMD resources (SOF path). |
| sound/soc/amd/ps/acp63.h | Documents and adds acp_bra_lock to ACP63 device data. |
| sound/soc/amd/ps/pci-ps.c | Initializes and passes acp_bra_lock into SoundWire AMD resources (PS path). |
| include/linux/soundwire/sdw_amd.h | Extends AMD SoundWire resource/pdata and manager state for BRA/BPT support and serialization. |
| drivers/soundwire/stream.c | Ensures consistent BPT snapshot during programming and enforces BPT vs audio-stream exclusivity rules. |
| drivers/soundwire/intel_ace2x.c | Makes BPT stream publish/clear ordering lockless-safe and fixes teardown paths to use the local stream handle. |
| drivers/soundwire/amd_manager.h | Adds BRA/BPT-related registers/bit definitions and timeouts. |
| drivers/soundwire/amd_manager.c | Implements AMD BRA/BPT DMA transfer flow, PTE programming, error handling, and suspend/remove safety sequencing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe9659b to
1becc08
Compare
|
@saba-kareem You might want to look at #5604 for BPT and audio stream coexisting. The idea is that the BPT stream can use the remaining available bandwidth. |
|
@bardliao At present, we have topology where only speaker peripherals connected to same Soundwire manager instance for this generation platforms. We need to revise our bandwidth calculation to align with your proposed logic. We will adopt the shared bandwidth calculation approach for future platform deployments. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Denying audio streams while BPT/BRA is already running is a big problem, see comments below.
you really need to special-case the resume part, not try to make audio and BPT/BRA run concurrently unless you have enough bandwidth for all streams.
| * ENABLED state, i.e. one that is reserving or moving data over the bus. A BPT | ||
| * transfer must not be started in that case. Streams that are only allocated | ||
| * but idle (ALLOCATED/CONFIGURED/DISABLED/DEPREPARED) reserve no active bus | ||
| * bandwidth and do not block BPT. |
There was a problem hiding this comment.
the problem is that if those audio streams become active DURING the BPT transfer they might fail because ALL the bandwidth was already allocated to BPT/BRA.
The sharing of the bus between audio and BPT/BRA is only possible if there is enough bandwidth for all streams in parallel.
This is way too invasive IMHO.
There was a problem hiding this comment.
@plbossart Agreed. Treating "idle but allocated" as free to share ignores the fact that an idle stream can go active mid-transfer, and the bus can only carry both if the bandwidth for all streams in parallel is actually reserved — which this patch never checks. That makes it too invasive, as you say. I'll drop this relaxation and stop keying the decision off stream state.
| * safe to run BPT alongside audio streams that are only | ||
| * allocated but idle (e.g. left DISABLED across system suspend | ||
| * on a power-off-mode platform, whose codec must re-download | ||
| * firmware over BPT on resume before the stream is re-enabled). |
There was a problem hiding this comment.
what happens if an inactive audio stream becomes active while BPT/BRA has all the bandwidth...
There was a problem hiding this comment.
@plbossart Right — with this patch that activation is exactly what fails, since the bandwidth was never reserved for both. There's no good answer here because the premise (idle == shareable) is wrong. The only case I actually need is resume-time firmware download, where the streams are DISABLED-across-suspend and cannot become active until resume completes. I'll special-case that explicitly instead of allowing general idle-stream coexistence, so this "becomes active mid-BPT" window no longer exists.
| dev_err(m_rt->bus->dev, | ||
| "%s: %s: BPT transfer in progress\n", | ||
| __func__, stream->name); | ||
| ret = -EBUSY; |
There was a problem hiding this comment.
yeah and that's a BIG problem. That will lead to e.g. UI sounds not being played or a questionable user-space handling of those errors.
You can't just change the policy to solve your download problem.
I think you really need to account for the fact that you need the BPT/BRA support while the system resumes, and you need to have something that flags that requirement. Using the fact that a stream is active or inactive is not a good way of tracking this state change, because it will apply to cases unrelated to system resume.
There was a problem hiding this comment.
@plbossart
Understood, and agreed this is the core issue — returning -EBUSY from prepare/enable would drop UI sounds and push error handling onto userspace, and changing the global policy to solve a download problem is the wrong fix. I'll remove these guards and the policy change.
The actual requirement is much narrower than the patch implies: on a power-off-mode resume the codec has lost power and must re-download its firmware over BPT/BRA before its stream — left DISABLED across suspend — can be re-enabled. In that resume window the audio streams cannot legitimately become active yet, so there is no real concurrency and no bandwidth to share; the ordering is naturally "download, then re-enable."
So instead of a bus-wide policy change I'll flag that requirement explicitly — a flag that marks "BPT/BRA firmware download needed on resume" — and permit BPT alongside an allocated-but-idle stream only under that flag, with the contract that no audio activation happens during the flagged download. Everything outside that path keeps today's audio↔BPT mutual exclusion unchanged. Let me rework along those lines and resend.
intel_ace2x_bpt_open_stream() calls sdw_slave_bpt_stream_add(), which via sdw_stream_add_slave() -> sdw_master_rt_alloc() allocates the master runtime, links it into bus->m_rt_list and raises bus->bpt_stream_refcount. Several later failure paths (PDI allocation, port-config allocation and sdw_stream_add_master()) jump to the remove_slave label, which only calls sdw_stream_remove_slave() followed by sdw_release_stream(). sdw_stream_remove_slave() frees only the slave runtime and ports; it does not reach sdw_master_rt_free(). The master runtime is therefore left on bus->m_rt_list pointing at the just-freed stream, and bpt_stream_refcount stays non-zero. Because sdw_master_rt_alloc() rejects a new BPT allocation while bpt_stream_refcount > 0, every subsequent BPT transfer on that bus is rejected with -EBUSY until the driver is reloaded. Route these error paths through the remove_master label so that sdw_stream_remove_master() frees the master runtime and drops the refcount before the stream is released, mirroring the error-path unwind in amd_sdw_bpt_open_stream(). Drop the now-unused remove_slave label; its sdw_stream_remove_slave() call still runs by falling through from remove_master, and is a no-op once the master runtime (and with it the slave runtimes) has been freed. Fixes: 4c1ce9f ("soundwire: intel_ace2x: add BPT send_async/wait callbacks") Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
The BPT (Bulk Payload Transport) stream pointer bus->bpt_stream is read locklessly by the SoundWire core to tell whether a BPT transfer owns the bus. For those readers to be safe the pointer and bus->bpt_stream_refcount must stay consistent: an observer that sees refcount == 0 under bus_lock must also see bpt_stream == NULL. Make intel_ace2x maintain that ordering: - Publish bus->bpt_stream with WRITE_ONCE() only after the master runtime has been added and bpt_stream_refcount raised, and (on the open path) before the in-open sdw_prepare_stream(). - Clear it with WRITE_ONCE() before sdw_stream_remove_master() drops the refcount on the close and error paths. Add a clear_bpt_stream label so paths that already published the pointer clear it, while the pre-publish failure paths skip the clear. - Snapshot the pointer into a local once (READ_ONCE()) so the open/close/error paths act on a single stable value instead of repeatedly re-reading the shared field. This is a no-op under the current policy, where BPT and audio streams are mutually exclusive, but establishes the ordering the core relies on once BPT is allowed to run alongside idle audio streams. Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
…are idle sdw_master_rt_alloc() rejected a BPT (Bulk Payload Transport) stream allocation whenever any audio stream was allocated on the bus (bus->stream_refcount > 0). On a power-off-mode platform, an amplifier that was left DISABLED across system suspend still holds an allocated but idle stream runtime, yet it must re-download its firmware over BPT on resume before that stream can be re-enabled. The blanket refcount check made the resume-time BPT transfer fail with -EBUSY. Rather than key this off the audio stream state - which would relax the policy for every caller and every scenario, not just a controlled firmware download - let the manager explicitly flag the transfer. Add bus->bpt_fw_download, which a BPT-capable manager sets around a firmware download that it knows may coexist with allocated-but-idle audio streams and for which it guarantees no audio stream is made active on the bus for the duration. sdw_master_rt_alloc() now rejects a BPT allocation when: - another BPT transfer is already allocated, or - an audio stream is actively using the bus (PREPARED/ENABLED), or - an audio stream is merely allocated but idle and the manager has not set bus->bpt_fw_download. Add sdw_bus_has_active_stream() for the active-stream test; it returns true only for streams in the PREPARED or ENABLED state. When bpt_fw_download is clear (the default, and the only possibility for managers that do not opt in) the three checks together reproduce the original strict policy exactly: any allocated stream, active or idle, still blocks BPT. Only a manager that sets the flag can allocate BPT alongside idle audio streams, and even then an actively streaming stream still blocks it. The audio path is still protected while a flagged download runs: sdw_program_params() skips master runtimes other than the active BPT stream while bus->bpt_stream is set, so BPT preparation does not rewrite the transport/port parameters of idle audio runtimes or deliver BPT bus parameters to their peripherals via sdw_notify_config(). The bus-wide SDW_SCP_BUSCLOCK_SCALE programming is intentionally left unfiltered, as every attached peripheral must track the actual bus clock. The filter keys off bus->bpt_stream, which managers publish with WRITE_ONCE() only after raising bpt_stream_refcount and clear before dropping it; sdw_program_params() reads it with READ_ONCE(). So an audio path that sees refcount == 0 under bus_lock also sees bpt_stream == NULL and programs its own parameters instead of being skipped. BPT and active audio are mutually exclusive on the bus: a flagged download is only started while all audio streams are idle, and the manager that sets bpt_fw_download guarantees that no audio stream is made active (PREPARED/ENABLED) on the bus for the transfer's duration. Serialising the two is the manager's/codec's responsibility (for example, the codec completes its firmware download before starting its own stream). In practice the flagged download runs in the codec's power-off-mode resume path, before its stream - left DISABLED across suspend - is re-enabled, and while userspace tasks are still frozen for system resume, so no userspace PCM operation (prepare, enable or hw_free) can race the download window. Because of that guarantee, the only sdw_program_params() passes while bus->bpt_stream is set are the BPT stream's own prepare/enable/disable/deprepare passes; the filter above keeps those passes from reprogramming or re-notifying the idle audio runtimes that remain allocated on the bus. It is not a mechanism for running audio traffic concurrently with a download. While at it, make the allocation-time rejection messages state the actual reason instead of printing the now-misleading stream_refcount. Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
Add Bulk Register Access (BRA) / Bulk Payload Transport (BPT) support for AMD SoundWire platforms. This enables high-speed firmware download to SoundWire peripherals via DP0, using the ACP BRA DMA engine. Key design points: - Uses the SoundWire stream framework (sdw_prepare_stream, sdw_enable_stream, sdw_disable_stream, sdw_deprepare_stream) for all DP0 port programming and bank switches. No manual DP0 register writes or bank mirrors are needed. - BRA transport parameters (hstart, hstop, SampleInterval, BytesPerFrame) are computed dynamically from the current bus frame shape, not hardcoded. - The ACP BPT DMA engine is triggered by the bank switch performed inside sdw_enable_stream(), and stopped by the bank switch in sdw_disable_stream(). - Non-contiguous firmware sections are handled by iterating per-section: large sections use BRA DMA, small sections (< one BRA frame) fall back to sdw_nwrite/sdw_nread. - BPT stream m_rt entries are skipped in audio compute_params to prevent BPT transport parameters from corrupting audio port block offset calculations. - DP0 port_params, xport_params, and port_enable callbacks return early for BPT streams since the ACP BRA descriptor registers handle DP0 configuration independently. - bus->bpt_stream is published with WRITE_ONCE() only after the stream runtime is added and bpt_stream_refcount is raised under bus_lock, and is cleared before the runtime is removed and the stream is freed, so the lockless DP0 port callbacks never observe a half-initialised or freed stream pointer. - bus->bpt_fw_download is set around the transfer so sdw_master_rt_alloc() admits the BPT allocation while audio streams on the same bus are allocated but idle (for example an amplifier left DISABLED across a power-off-mode suspend that must re-download firmware on resume). It is cleared on the close and open error paths. An audio stream that is actively using the bus still blocks the transfer. - On an aborted or timed-out transfer the ACP BPT DMA engine is disarmed (PORT_EN=0) before the sdw_disable_stream() bank switch, so the bank switch cannot re-trigger a DMA write into the buffer that is freed once the transfer returns. - A per-manager bpt_lock serialises concurrent BPT transfers from multiple slave probes. pm_runtime keeps the bus clock active during transfers. - PTE-based ACP ATU mapping provides DMA scatter-gather for the firmware buffer. The ATU maps up to 512 4KB pages (2 MB per transfer); each transfer is additionally bounded by the SoundWire BPT limit of 1 MB (SDW_BPT_MSG_MAX_BYTES), which the driver enforces. Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
1becc08 to
929443b
Compare
BRA/BPT Support for AMD Platforms(ACP 7.0/7.1/7.2 variants) and includes fixes in intel code.