[TEST] Deep-buffer with better DPCM locking #3201
Closed
Conversation
This file contains 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
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>
plbossart
force-pushed
the
fix/dpcm-lock4
branch
from
a791856
to
9ce2d72
Compare
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>
The stream parameter is not used, remove before further simplifications. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
sdw_config_stream() only verifies the compatibility between information provided by the Slave driver and the stream configuration. There is no problem if we add the slave runtime to the list earlier. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Pass the index directly to sdw_is_valid_port_range(), this will be useful for further simplifications. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code only has a config helper that allocates memory, start adding alloc/config/free for ports, as a first step in the simplification of the stream API. This change removes a kfree() on a configuration error, this should have not impact on existing platforms and error handling will be revisited in follow-up patches to make sure invalid configurations have not impact on memory allocation. 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>
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>
This function is not used anywhere, including soc-pcm.c Remove dead code. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
No one uses the following functions outside of soc-pcm.c snd_soc_dpcm_can_be_free_stop() snd_soc_dpcm_can_be_params() snd_soc_dpcm_be_can_update() In preparation for locking changes, move them to static functions and remove the EXPORT_SYMBOL_GPL() Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Minor realignment before more changes. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
fix/dpcm-lock4
branch
from
9ce2d72
to
5d714a3
Compare
In preparation for more changes, add two new helpers to hide the actual type of lock being used for DPCM. Since DPCM functions are not used from interrupt handlers, use spin_lock_irq instead of the spin_lock_irqsave version. While most of the uses of DPCM are internal to soc-pcm.c, some drivers in soc/fsl and soc/sh do make use of DPCM-related loops that will require protection, adding EXPORT_SYMBOL_GPL() is needed for those drivers. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The D in DPCM stands for 'dynamic', which means that connections between FE and BE can evolve. Commit a976486 ("ASoC: dpcm: prevent snd_soc_dpcm use after free") started to protect some of the for_each_dpcm_be() loops, but there are still many cases that were not modified. This patch adds protection for all the loops, with the notable exception of the dpcm_be_dai_trigger(), where additional changes are required. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow the locking model used within soc-pcm.c Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow the locking model used within soc-pcm.c Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow the locking model used within soc-pcm.c Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow the PCM stream example and use either a spin-lock or a mutex. This assumes that all FEs in a card have the same 'atomicity'. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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. This can be fixed by using the snd_soc_dpcm_lock/unlock() helpers which internally rely on a spinlock or mutex, depending on the dailink nonatomic nature. This locking solves another problem discussed on the alsa-mailing list [1], where BEs can be disconnected dynamically, leading to potential accesses to freed memory. [1] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/ 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 mutex. 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 (see note [1] below) 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. [1] the existing logic does not move the BE state back to PAUSED when the FE2 is stopped. This patch does not change the logic; it would be painful to keep a history of changes on the FE side, the state machine is already rather complicated with transitions based on the last BE state and the trigger type. Reported-by: Bard Liao <bard.liao@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
fix/dpcm-lock4
branch
from
5d714a3
to
0ac4b26
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up on #3146 with a DPCM lock that takes FE atomicity into account.