Repository navigation
Conversation
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>
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>
uninitialized stack variable leading to error with KASAN enabled. [ 330.969459] sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx succeeded: 0x60040000: GLB_STREAM_MSG: TRIG_START [ 330.970132] rt711 sdw:0:025d:0711:00: [rt711_sdw_write] 00e0 <= 0003 [ 330.970215] rt711 sdw:0:025d:0711:00: [rt711_sdw_write] 00f0 <= 0003 [ 330.970219] rt711 sdw:0:025d:0711:00: rt711_clock_config complete, clk_freq=4800000 [ 330.970930] soundwire_intel soundwire_intel.link.0: ASoC: error at soc_dai_trigger on SDW0 Pin2: -30591 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>
Since the flow for DPCM is based on taking a lock for the FE first, we need to make sure during the connection between a BE and an FE that they both use the same 'atomicity', otherwise we may sleep in atomic context. If the FE is nonatomic, this patch forces the BE to be nonatomic as well. That should have no negative impact since the BE 'inherits' the FE properties. However, if the FE is atomic and the BE is not, then the configuration is flagged as invalid. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> [ removed FE stream lock by tiwai ] Signed-off-by: Takashi Iwai <tiwai@suse.de>
Use card->pcm_mutex consistently instead of mixture of card->mutex and card->pcm_mutex. This is used for protecting the BE link in each FE. Replace the half-baked dpcm_lock spinlock with the PCM stream lock of each FE for protecting the BE link in addition to the pcm_mutex above for avoiding the race in FE trigger. The additional FE stream locks are taken only at adding and deleting the BE link entries. The pcm_mutex is applied always on either the top PCM callbacks or the external call from DAPM, not taken in the internal functions. Signed-off-by: Takashi Iwai <tiwai@suse.de>
For avoiding the race among concurrent accesses to BEs. Signed-off-by: Takashi Iwai <tiwai@suse.de>
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 the BE PCM stream lock. 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
requested review from
RanderWang,
bardliao,
dbaluta,
jack-cy-yu,
keyonjie,
kv2019i,
lgirdwood,
libinyang,
lyakh,
naveen-manohar,
oder-chiou,
ranj063,
ryans-lee and
shumingfan
as code owners
October 15, 2021 17:07
This was referenced Oct 18, 2021
plbossart
marked this pull request as draft
November 5, 2021 21:35
Member
Author
|
closing, all patches either already merged upstream, topic/sof-dev or still being reviewed. |
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
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.
Updated branch with the 3 patches suggested by @tiwai in https://lore.kernel.org/alsa-devel/20211013143050.244444-1-pierre-louis.bossart@linux.intel.com/T/#mce9e1eeb8a69cf75fdada37f0f3d67262e61a514