Deep buffer prerequisites - WIP #3146
Closed
Conversation
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
This was referenced Sep 9, 2021
plbossart
requested review from
RanderWang,
bardliao,
kv2019i,
lyakh and
ranj063
Member
Author
|
It's been a while since I saw "All checks have passed" on my work. Maybe that's a sign the universe is sending that I should leave early today. |
Member
Author
|
local tests with thesofproject/sof#4611 don't expose any blatant issue, all PASS. Running more automated tests now. |
ranj063
reviewed
Sep 14, 2021
Collaborator
ranj063
left a comment
ranj063
left a comment
There was a problem hiding this comment.
ths is a pretty impressive series @plbossart. Only minor comments to address. Also what is the last patch for? Are you seeing issues with widget ref counting with this PR?
sound/soc/codecs/hdac_hda.c
Outdated
Collaborator
There was a problem hiding this comment.
oh I love this. I think we struggled with this for a long time 2-3 years ago and we finally have an elegant solution.
Member
Author
There was a problem hiding this comment.
elegant is not always a qualifier that applies to me, so I will take the compliment, thank you!
drivers/soundwire/stream.c
Outdated
Collaborator
There was a problem hiding this comment.
was this kfree() removal intentional?
Member
Author
There was a problem hiding this comment.
yes, by moving the list_add_tail earlier the 's_rt' memory chunk will be freed in sdw_release_master_stream(m_rt, stream); line 1400
drivers/soundwire/stream.c
Outdated
Collaborator
There was a problem hiding this comment.
typo in commit message: "have not impact" , twice
drivers/soundwire/stream.c
Outdated
Collaborator
There was a problem hiding this comment.
it feels like this function only adds an error message based on the result of the macro. It makes sense if it is used in multiple places but from this commit I can tell if it is. So maybe we can just remove the function and add the mesage here to simpliy the code
Member
Author
There was a problem hiding this comment.
we could do this as a follow-up optimization. I wanted to keep this as is since there's a TODO that should be implemented at some point with additional checks. For now we only check for the min and max ranges allowed in the standard for data ports, but if/when platform firmware describes the devices we will be able to check precisely if a given port exists or not in a device.
drivers/soundwire/stream.c
Outdated
Collaborator
There was a problem hiding this comment.
type in commit. 'This will it easier"
Member
Author
Yes, in the case of HDaudio I see widgets not released. I tracked the errors in #3151. It's really weird that with the pause_push1/pause_push2/suspend/resume/pause_release2/pause_release1 I see an underflow 2 seconds after the last pause_release1. It could be a stream assignment issue or a firmware issue, hard to find. It's 100% reproducible with Up Extreme, but if suspend-resume doesn't work on yours not sure how to progress. |
plbossart
force-pushed
the
fix/deep-buffer-prerequisites
branch
3 times, most recently
from
fd31a37 to
2236b2d
Compare
Collaborator
|
I think in the kernel they started to require scripts or shell command lines (e.g. sed) that were used to produce some automatically generated patches. Not sure whether this is a current requirement though and whether it's still upheld. To help review such large automated changes we could use one of the following options (or some others too for sure) (ordered by decreasing efficiency and complexity):
|
Member
Author
if you are referring to the renaming, the only thing that can be automatic is the variable definition. The use of stream or hstream has to be handled manually. I am not aware of any tool that would be smart enough to rename a variable and its uses? |
The existing code maximizes confusion by using 'stream' and 'hstream' variables of different types. Examples: struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream; with some additional copy/paste remains: struct hdac_ext_stream *azx_dev; This patch suggests a consistent naming across all 'hdac_ext_stream' functions. The convention is: struct hdac_stream *hstream; struct hdac_ext_stream *he_stream; No functionality change - just renaming of variables and more consistent indentation. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…members The existing code maximizes confusion by using 'stream' and 'hstream' variables of different types, e.g: struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream; This confusion is partly inherited from legacy code but SOF contributors added their own creative spin, e.g. struct hdac_ext_stream *link_dev; struct hdac_ext_stream *dsp_stream; struct hdac_ext_stream hda_stream; and my personal favorite: stream = &hda_stream->hda_stream; This patch suggests a consistent naming across all Intel code related to HDAudio stream management. The convention is - by hierarchical order: struct sof_intel_hda_stream *hda_stream; struct hdac_ext_stream *he_stream; struct hdac_stream *hstream; No functionality change - just renaming of variables/members. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The HDAudio ASoC support relies on the set_tdm_slots() helper to store the HDaudio stream tag in the tx_mask. This only works because of the pre-existing order in soc-pcm.c, where the hw_params() is handled for codec_dais *before* cpu_dais. When the order is reversed, the stream_tag is used as a mask in the codec fixup functions: /* fixup params based on TDM slot masks */ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && codec_dai->tx_mask) soc_pcm_codec_params_fixup(&codec_params, codec_dai->tx_mask); As a result of this confusion, the codec_params_fixup() ends-up generating bad channel masks, depending on what stream_tag was allocated. We could add a flag to state that the tx_mask is really not a mask, but it would be quite ugly to persist in overloading concepts. Instead, this patch suggests a more generic get/set 'stream' API based on the existing model for SoundWire. We can expand the concept to store 'stream' opaque information that is specific to different DAI types. In the case of HDAudio DAIs, we only need to store a stream tag as an unsigned char pointer. The TDM rx_ and tx_masks should really only be used to store masks. Rename get_sdw_stream/set_sdw_stream callbacks and helpers as get_stream/set_stream. No functionality change beyond the rename. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Overloading the tx_mask with a linear value is asking for trouble and only works because the codec_dai hw_params() is called before the cpu_dai hw_params(). Move to the more generic set_stream() API to pass the hdac_stream information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We have a helper, use it to simplify widget lookup Suggested-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
TRIGGER_RESUME is not supported on Intel platforms, let's remove untested/dead code. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
… DMA The Intel documentation refers to the concepts of 'HDAudio host DMA' (system memory <--> DSP) and 'HDaudio link DMA' (DSP <--> peripherals). We currently use the prefix 'hda_link' to describe DAI operations, which can be confused for dailink operations. Since the topology tokens refer unambiguously to the 'HDA' dai, let's drop the link prefix for dai-related ops/callbacks. Conversely let's use 'hda_link_dma' for routines related to the DMA management. In a follow-up patch we will introduce the 'hda_dai_link' prefix for dailink ops/callbacks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We don't need to pass a hda_stream to find the sdev to find a device for dev_dbg. Just pass the device. While we're at it, also update error log Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
First split the dailink and dai operations in separate functions, that will still be called from the dai callbacks. In an ideal world, these dailink routines would be moved and invoked directly from the dailink callbacks. This would be quite invasive and impact multiple machine drivers, so for now we keep the split internal to the SOF driver. The main intent here is to split 'link management' from 'SOF IPC'. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Just code move with no functionality change, to clearly separate out the 'dai' operation from the 'dailink' ones. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
cppcheck warning:
drivers/soundwire/intel.c:1557:10: style: Variable 'ret' is assigned a
value that is never used. [unreadVariable]
int ret = 0;
^
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Split loops before moving the allocation and configuration to separate functions. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Continue the split with two functions for master and slave, and remove unused arguments. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We can only check for Slave port ranges, the ports are not defined at the Master level. Also move the function to the 'slave port' block. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
re-group all the helpers in one location with a code move. For consistency the 'slave' helpers are placed before the 'master' helpers. Also remove unused arguments and rename the 'release' function to 'free' for consistency. No functional change in this patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Only do the allocation in that function, and move check for allocation in the caller. This will it easier to split allocation and configuration. No functionality change in this patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Split the two parts so that we can do multiple configurations during ALSA/ASoC hw_params stage. Also follow existing convention sdw_<object>_<action> used at lower level. No functionality change here. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Code move before splitting the function in two. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Split the two parts so that we can do multiple configurations during ALSA/ASoC hw_params stage. Also follow existing convention sdw_<object>_<action> used at lower level. No functionality change here. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Group all exported functions prior to split of add in alloc/config stages necessary for support of multiple calls to hw_params() by ALSA/ASoC core. Pure code move, no functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The naming is rather inconsistent, use the sdw_<object>_<action> convention, and move the free routine after alloc/config. No functionality change beyond rename/move. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Simplify sdw_stream_add_slave() by moving the linked list management inside of the sdw_slave_alloc_rt_free() helper, this also makes the alloc/free helpers more symmetrical. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Separate alloc and config parts so that follow-up patches can allow for multiple calls to sdw_stream_add_slave/master. This is a feature from the ALSA/ASoC frameworks which is not supported today. This is an invasive patch which modifies the error handling flow, with cleanups only done when an allocation fails. Configuration failures only return an error code. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Before we split the alloc and config steps, we need a helper to find the Slave runtime for a stream. The helper is based on the search loop in sdw_slave_rt_free(), which can now be simplified. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…imes The sdw_stream_add_slave/master() functions are called from the .hw_params stage. We need to make sure the functions can be called multiple times. In this version, we assume that only 'audio' parameters provide in the hw_params() can change. If the number of ports could change dynamically depending on the stream configuration (number of channels, etc), we would need to free-up all the stream resources and reallocate them. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The stream management currently flags an 'inconsistent state' error when a change is requested multiple times. This was added on purpose to identify programming mistakes. In hindsight, there was no real reason to fail if the logic at the ASoC-DPCM level invokes the same callback multiple times. It's perfectly acceptable to just return and not flag an error when there is nothing to do. The main concern with the state management is to trap errors such as trying to enable a stream that was not prepared first. This patch suggests allowing the stream functions to be idempotent, i.e. they can be called multiple times. Note that the prepare case was already handling multiple calls, this was added in commit c32464c ("soundwire: stream: only prepare stream when it is configured.") Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We don't really need to pass a substream to the callback, we only need the direction. No functionality change, only simplification to enable improve suspend with paused streams. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This reverts commit 048ecd7. A better implementation will be provided in follow-up patches. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This patch provides both a simplification of the suspend flows and a better balanced operation during suspend/resume transition, as part of the transition of Sound Open Firmware (SOF) to dynamic pipelines: the DSP resources are only enabled when required instead of enabled on startup. The exiting code relies on a convoluted way of dealing with suspend signals. Since there is no .suspend DAI callback, we used the component .suspend and marked all the component DAI dmas as 'suspended'. The information was used in the .prepare stage to differentiate resume operations from xrun handling, and only reinitialize SHIM registers and DMA in the former case. While this solution has been working reliably for about 2 years, there is a much better solution consisting in trapping the TRIGGER_SUSPEND in the .trigger DAI ops. The DMA is still marked in the same way for the .prepare op to run, but in addition the callbacks sent to DSP firmware are now balanced. Normal operation: hw_params -> intel_params_stream hw_free -> intel_free_stream suspend -> intel_free_stream prepare -> intel_params_stream This balanced operation was not required with existing SOF firmware relying on static pipelines instantiated at every boot. With the on-going transition to dynamic pipelines, it's however a requirement to keep the use count for the DAI widget balanced across all transitions. The component suspend is not removed but instead modified to deal with a corner case: when a substream is PAUSED, the ALSA core does not throw the TRIGGER_SUSPEND. This is problematic since the refcount for all pipelines and widgets is not balanced, leading to issues on resume. The trigger callback keeps track of the 'paused' state with a new flag, which is tested during the component suspend called later to release the remaining DSP resources. These resources will be re-enabled in the .prepare step. The IPC used in the TRIGGER_SUSPEND to release DSP resources is not a problem since the BE dailink is already marked as non-atomic. Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
When more than one FE is connected to a BE, e.g. in a mixing use case, the BE can be triggered multiple times when the FE are opened/started concurrently. This race condition is problematic in the case of SoundWire BE dailinks, and this is not desirable in a general case. The code carefully checks when the BE can be stopped or hw_free'ed, but the trigger code does not use any mutual exclusion. Fix by using the same spinlock already used to check FE states, and set the state before the trigger. In case of errors, the initial state will be restored. This patch does not change how the triggers are handled, it only makes sure the states are handled in critical sections. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
On start/pause_release/resume, when more than one FE is connected to the same BE, it's possible that the trigger is sent more than once. This is not desirable, we only want to trigger a BE once, which is straightforward to implement with a refcount. For stop/pause/suspend, the problem is more complicated: the check implemented in snd_soc_dpcm_can_be_free_stop() may fail due to a conceptual deadlock when we trigger the BE before the FE. In this case, the FE states have not yet changed, so there are corner cases where the TRIGGER_STOP is never sent - the dual case of start where multiple triggers might be sent. This patch suggests an unconditional trigger in all cases, without checking the FE states, using a refcount protected by a spinlock. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
A BE connected to more than one FE, e.g. in a mixer case, can go through the following transitions. play FE1 -> BE state is START pause FE1 -> BE state is PAUSED play FE2 -> BE state is START stop FE2 -> BE state is STOP release FE1 -> BE state is START stop FE1 -> BE state is STOP play FE1 -> BE state is START pause FE1 -> BE state is PAUSED play FE2 -> BE state is START release FE1 -> BE state is START stop FE2 -> BE state is START stop FE1 -> BE state is STOP The existing code for PAUSE_RELEASE only allows for the case where the BE is paused, which clearly would not work in the sequences above. Extend the allowed states to restart the BE when PAUSE_RELEASE is received. Reported-by: Bard Liao <bard.liao@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Before suspending, walk through all the widgets to make sure all refcounts are zero. If not, the resume will not work and random errors will be reported. Adding this paranoia check will help identify leaks and broken sequences. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We do the same thing from different places, let's use a helper. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…pend The location of the code was not optimal and prevents us from using helpers, let's move it to hda-dai.c, add comments and re-align with the TRIGGER_SUSPEND case with an additional call to hda_dai_hw_free_ipc() to free-up resources. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The sequences are missing a call to snd_soc_dai_set_dma_data() when the stream is cleared, as well as a release of the stream, and tests to avoid pointer dereferences. This fixes an underflow issue in a corner case with two streams paused before a suspend-resume cycle. After resume, the pause_release of the last stream causes an underflow due to an invalid sequence. This problem probably existed since the beginning and is only see with prototypes of a 'deep-buffer' capability, which depends on additional ASoC fixes, so there's is no Fixes: tag and no real requirement to backport this patch. BugLink: thesofproject#3151 Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
fix/deep-buffer-prerequisites
branch
from
c487ff7 to
929aeaa
Compare
Member
Author
|
closing, all patches either already merged upstream, topic/sof-dev or still being reviewed. |
fengguang
pushed a commit
to 0day-ci/linux
that referenced
this pull request
Dec 15, 2021
Merge series from Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>: We've been adding a 'deep buffer' PCM device to several SOF topologies in order to reduce power consumption. The typical use-case would be music playback over a headset: this additional PCM device provides more buffering and longer latencies, leaving the rest of the system sleep for longer periods. Notifications and 'regular' low-latency audio playback would still use the 'normal' PCM device and be mixed with the 'deep buffer' before rendering on the headphone endpoint. The tentative direction would be to expose this alternate device to PulseAudio/PipeWire/CRAS via the UCM SectionModifier definitions. That seemed a straightforward topology change until our automated validation stress tests started reporting issues on SoundWire platforms, when e.g. two START triggers might be send and conversely the STOP trigger is never sent. The SoundWire stream state management flagged inconsistent states when the two 'normal' and 'deep buffer' devices are used concurrently with rapid play/stop/pause monkey testing. Looking at the soc-pcm.c code, it seems that the BE state management needs a lot of love. a) there is no consistent protection for the BE state. In some parts of the code, the state updates are protected by a spinlock but in the trigger they are not. When we open/play/close the two PCM devices in stress tests, we end-up testing a state that is being modified. That can't be good. b) there is a conceptual deadlock: on stop we check the FE states to see if a shared BE can be stopped, but since we trigger the BE first the FE states have not been modified yet, so the TRIGGER_STOP is never sent. This patchset suggests the removal of the dedicated 'dpcm_lock' and follows the design suggested by Takashi Iwai. By default the protection relies on the 'pcm_mutex', except for the FE and BE triggers where the mutex cannot be used. In this case, the FE PCM lock is used instead. In the cases where a BE is added/removed, the pcm_mutex and FE PCM lock are both taken. In addition, the BE PCM lock is used to serialize access to a shared BE. With these patches I am able to run our entire validation suite without any issues with this new 'deep buffer' topology, and no regressions on existing solutions [1]. The tests were reproduced by Bard Liao for SoundWire devices. One might ask 'how come we didn't see this earlier'? The answer is probably that the .trigger callbacks in most implementations seems to perform DAPM operations, and sending the triggers multiple times is not an issue. In the case of SoundWire, we do use the .trigger callback to reconfigure the bus using the 'bank switch' mechanism. It could be acceptable to tolerate a trigger multiple times, but the deadlock on stop cannot be fixed at the SoundWire level alone. Opens: 1) The issues reported by Nvidia on the RFCv3 may or may not be present. We'd need test results to make sure the locking update does not introduce a regression on Tegra. 2) There are other reports of kernel oopses [2] that seem related to the lack of protection. I'd be good to confirm if this patchset solve these problems as well. [1] thesofproject#3146 [2] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/ changes since RFCv3: Used two patches from Takashi. We now use the pcm_mutex, the FE stream lock when adding and deleting a BE, and the BE stream lock to handle concurrency between streams using the same BE. Added a patch to use GFP_ATOMIC for the DPCM structure. Fixed PAUSE_RELEASE transition (GitHub comment from Kai Vehmanen) changes since RFCv2: Removal of dpcm_lock to use FE PCM locks (credits to Takashi Iwai for the suggestion). The FE PCM lock is now used before each use of for_each_dpcm_be() - with the exception of the trigger where the lock is already taken. This change is also applied in drivers which make use of this loop (compress, SH, FSL). Addition of BE PCM lock to deal with mutual exclusion between triggers for the same BE. Alignment of the BE atomicity on the FE on connections, this is required to avoid sleeping in atomic context. Additional cleanups (indentation, static functions) changes since RFC v1: Removed unused function Removed exported symbols only used in soc-pcm.c, used static instead Use a mutex instead of a spinlock Protect all for_each_dpcm_be() loops Fix bugs introduced in the refcount Pierre-Louis Bossart (4): ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure ASoC: soc-pcm: align BE 'atomicity' with that of the FE ASoC: soc-pcm: test refcount before triggering ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE Takashi Iwai (2): ASoC: soc-pcm: Fix and cleanup DPCM locking ASoC: soc-pcm: serialize BE triggers include/sound/soc-dpcm.h | 2 + include/sound/soc.h | 2 - sound/soc/soc-core.c | 1 - sound/soc/soc-pcm.c | 351 +++++++++++++++++++++++++++------------ 4 files changed, 246 insertions(+), 110 deletions(-) -- 2.25.1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This draft is the collection of all the patches I think are necessary to enable the deep-buffer support with SOF (SoundWire and HDaudio).
the volume of the patches is rather large since instead of filtering multiple hw_params calls for the SoundWire solution I decided to make it legal. It required a large rewrite of the 'stream' API to be able to split allocation from configuration.