commit 4bf2e385aa ("ASoC: core: Init pcm runtime work early to
avoid warnings") has added generic close_delayed_work() which checks
close_delayed_work_func
static void close_delayed_work(...) {
...
=> if (rtd->close_delayed_work_func)
rtd->close_delayed_work_func(rtd);
}
So, we don't need to have NULL function for Codec2Codec.
=> static void codec2codec_close_delayed_work()
{
/*
* Currently nothing to do for c2c links
* Since c2c links are internal nodes in the DAPM graph and
* don't interface with the outside world or application layer
* we don't have to do any special handling on close.
*/
}
int soc_new_pcm(...)
{
...
if (rtd->dai_link->params)
=> rtd->close_delayed_work_func = codec2codec_close_delayed_work;
else
rtd->close_delayed_work_func = snd_soc_close_delayed_work;
...
}
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87sfle4dzk.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
commit b7898396f4 ("ASoC: soc-pcm: Fix and cleanup DPCM locking")
added __soc_pcm_close() for non-lock version of soc_pcm_close().
But soc_pcm_close() is not using it. It is no problem, but confusable.
static int __soc_pcm_close(...)
{
=> return soc_pcm_clean(rtd, substream, 0);
}
static int soc_pcm_close(...)
{
...
snd_soc_dpcm_mutex_lock(rtd);
=> soc_pcm_clean(rtd, substream, 0);
snd_soc_dpcm_mutex_unlock(rtd);
return 0;
}
This patch use it.
Fixes: b7898396f4 ("ASoC: soc-pcm: Fix and cleanup DPCM locking")
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87czctgg3w.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
soc_new_pcm() sets pcm->no_device_suspend, but it sets other pcm->xxx
at the same function with different timing.
pcm->no_device_suspend setup timing has no effect. This patch tidyup it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87bksdgflq.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
When DPCM tries to add valid BE connections at dpcm_add_paths(), it
doesn't check whether the picked BE actually supports for the given
stream direction. Due to that, when an asymmetric BE stream is
present, it picks up wrongly and this may result in a NULL dereference
at a later point where the code assumes the existence of a
corresponding BE substream.
This patch adds the check for the presence of the substream for the
target BE for avoiding the problem above.
Note that we have already some fix for non-existing BE substream at
commit 6246f283d5 ("ASoC: dpcm: skip missing substream while
applying symmetry"). But the code path we've hit recently is rather
happening before the previous fix. So this patch tries to fix at
picking up a BE instead of parsing BE lists.
Fixes: bbf7d3b1c4 ("ASoC: soc-pcm: align BE 'atomicity' with that of the FE")
Reported-by: Alex Natalsson <harmoniesworlds@gmail.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Link: https://lore.kernel.org/r/CADs9LoPZH_D+eJ9qjTxSLE5jGyhKsjMN7g2NighZ16biVxsyKw@mail.gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20220801170510.26582-1-tiwai@suse.de
Signed-off-by: Mark Brown <broonie@kernel.org>
When an FE, typically non-atomic, is connected to an atomic BE, we
force the BE as non-atomic. There's no reason to throw a warning, this
is a perfectly fine configuration and a conversion that's required
by-design.
This removes the unconditional warnings such as
[ 12.054213] iDisp1: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
[ 12.074693] iDisp2: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
[ 12.096612] iDisp3: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
[ 12.118637] iDisp4: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
[ 12.140660] dmic01: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
[ 12.147521] dmic16k: dpcm_be_connect: FE is nonatomic but BE is not, forcing BE as nonatomic
and demotes them to dev_dbg(), as suggested in review comments.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20220708200641.26923-1-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
A obvious editing mistake caught with a cppcheck warning
sound/soc/soc-pcm.c:2132:8: style: Variable 'ret' is reassigned a
value before the old one has been used. [redundantAssignment]
ret = soc_pcm_trigger(be_substream, cmd);
^
sound/soc/soc-pcm.c:2126:9: note: ret is assigned
ret = soc_pcm_trigger(be_substream,
^
sound/soc/soc-pcm.c:2129:9: note: ret is assigned
ret = soc_pcm_trigger(be_substream,
^
Fixes: 374b50e234 ('ASoC: soc-pcm: improve BE transition for TRIGGER_START')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Link: https://lore.kernel.org/r/20220520210615.607229-1-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
When the BE was in PAUSED state, the correct trigger is PAUSE_RELEASE.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Link: https://lore.kernel.org/r/20220406190056.233481-3-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Commit 3aa1e96a2b ("ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE")
did not modify the existing logic and kept the same logic for the following
transition
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
At the time it was identified by reviewers that a better solution
might consist in
play FE1 -> BE state is START
pause FE1 -> BE state is PAUSED
play FE2 -> BE state is START
stop FE2 -> BE state is PAUSE <<< !!
release FE1 -> BE state is START
stop FE1 -> BE state is STOP
This patch suggest a transition to PAUSE when all the 'active' streams
are paused. This would allow for a more consistent resource management
for platforms where PAUSE and STOP are handled differently.
To track the special case of an FE going from PAUSE_PUSH to STOP, we
add a state variable for each FE context. This 'fe_pause' boolean is
set on PAUSE_PUSH and cleared on either PAUSE_RELEASE and STOP
triggers.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Link: https://lore.kernel.org/r/20220406190056.233481-2-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
At the kzalloc() call in dpcm_be_connect(), there is no spin lock involved.
It's merely protected by card->pcm_mutex, instead. The spinlock is applied
at the later call with snd_soc_pcm_stream_lock_irq() only for the list
manipulations. (See it's *_irq(), not *_irqsave(); that means the context
being sleepable at that point.) So, we can use GFP_KERNEL safely there.
This patch revert commit d8a9c6e1f6 ("ASoC: soc-pcm: use GFP_ATOMIC for
dpcm structure") which is no longer needed since commit b7898396f4
("ASoC: soc-pcm: Fix and cleanup DPCM locking").
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Link: https://lore.kernel.org/r/e740f1930843060e025e3c0f17ec1393cfdafb26.1648757961.git.christophe.jaillet@wanadoo.fr
Signed-off-by: Mark Brown <broonie@kernel.org>
The recent fix for DPCM locking also covered the loop in
dpcm_be_disconnect() with the FE stream lock. This caused an
unexpected side effect, thought: calling debugfs_remove_recursive() in
the spinlock may lead to lockdep splats as the code there assumes the
SOFTIRQ-safe context.
For avoiding the problem, this patch changes the disconnection
procedure to two phases: at first, the matching entries are removed
from the linked list, then the resources are freed outside the lock.
Fixes: b7898396f4 ("ASoC: soc-pcm: Fix and cleanup DPCM locking")
Reported-and-tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20220119155249.26754-3-tiwai@suse.de
Signed-off-by: Mark Brown <broonie@kernel.org>
The recent change for DPCM locking caused spurious lockdep warnings.
Actually the warnings are false-positive, as those are triggered due
to the nested stream locks for FE and BE. Since both locks belong to
the same lock class, lockdep sees it as if a deadlock.
For fixing this, we need to take PCM stream locks for BE with the
nested lock primitives. Since currently snd_pcm_stream_lock*() helper
assumes only the top-level single locking, a new helper function
snd_pcm_stream_lock_irqsave_nested() is defined for a single-depth
nested lock, which is now used in the BE DAI trigger that is always
performed inside a FE stream lock.
Fixes: b2ae806630 ("ASoC: soc-pcm: serialize BE triggers")
Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
Reported-and-tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Link: https://lore.kernel.org/r/73018f3c-9769-72ea-0325-b3f8e2381e30@redhat.com
Link: https://lore.kernel.org/alsa-devel/9a0abddd-49e9-872d-2f00-a1697340f786@samsung.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20220119155249.26754-2-tiwai@suse.de
Signed-off-by: Mark Brown <broonie@kernel.org>
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
play FE1 -> BE state is START
play FE2 -> BE state is START (no change)
pause FE1 -> BE state is START (no change)
pause FE2 -> BE state is PAUSED
release FE1 -> BE state is START
release FE2 -> BE state is START (no change)
stop FE1 -> BE state is START (no change)
stop FE2 -> 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, and increase the refcount if the BE is already in START.
[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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-7-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-6-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
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.
This patch relies on the existing BE PCM lock, which takes atomicity into
account. The locking model assumes that all interactions start with
the FE, so that there is no deadlock between FE and BE locks.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
[test, checkpatch fix and clarification of commit message by plbossart]
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-5-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The existing locking for DPCM has several issues
a) a confusing mix of card->mutex and card->pcm_mutex.
b) a dpcm_lock spinlock added inconsistently and on paths that could
be recursively taken. The use of irqsave/irqrestore was also overkill.
The suggested model is:
1) The pcm_mutex is the top-most protection of BE links in the FE. The
pcm_mutex is applied always on either the top PCM callbacks or the
external call from DAPM, not taken in the internal functions.
2) the FE stream lock is taken in higher levels before invoking
dpcm_be_dai_trigger()
3) when adding and deleting a BE, both the pcm_mutex and FE stream
lock are taken.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
[clarification of commit message by plbossart]
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-4-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-3-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
We allocate a structure in dpcm_be_connect(), which may be called in
atomic context. Using GFP_KERNEL is not quite right, we have to use
GFP_ATOMIC to prevent the allocator from sleeping.
Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-2-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Current soc-pcm.c :: soc_pcm_pointer() is assuming that
component driver might update runtime->delay silently in
snd_soc_pcm_component_pointer() (= A).
static snd_pcm_uframes_t soc_pcm_pointer(...)
{
...
/* clearing the previous total delay */
=> runtime->delay = 0;
(A) offset = snd_soc_pcm_component_pointer(substream);
/* base delay if assigned in pointer callback */
=> delay = runtime->delay;
...
}
1) The behavior that ".pointer callback secretly updates
runtime->delay" is strange and confusable.
2) Current snd_soc_pcm_component_pointer() uses 1st found component's
.pointer callback only, thus it is no problem for now.
But runtime->delay might be overwrote if it adjusted to multiple
components in the future.
3) Component delay is updated at .pointer callback timing (secretly).
But some components which doesn't have .pointer callback might want
to increase runtime->delay for some reasons.
We already have .delay function for DAI, but not have for Component.
This patch adds new snd_soc_pcm_component_delay() for it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/874k8cy25t.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Current soc_pcm_pointer() is manually calculating
both CPU-DAI's max delay (= A)
and Codec-DAI's max delay (= B).
static snd_pcm_uframes_t soc_pcm_pointer(...)
{
...
^ for_each_rtd_cpu_dais(rtd, i, cpu_dai)
(A) cpu_delay = max(cpu_delay, ...);
v delay += cpu_delay;
^ for_each_rtd_codec_dais(rtd, i, codec_dai)
(B) codec_delay = max(codec_delay, ...);
v delay += codec_delay;
runtime->delay = delay;
...
}
Current soc_pcm_pointer() and the total delay calculating
is not readable / difficult to understand.
This patch update snd_soc_dai_delay() to snd_soc_pcm_dai_delay(),
and calcule both CPU/Codec delay in one function.
Link: https://lore.kernel.org/r/87fszl4yrq.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/875yssy25z.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
DAI active count is not exchanged during for_each_rtd_dais()
loops. We don't need to keep snd_soc_dai_stream_active() as
"active" on soc_pcm_hw_clean(). This patch avoid verbose code.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87ilxvt7e6.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
soc_pcm_hw_clean() is using "continue" during for_each_rtd_dais(),
but it is very verbose. This patch cleanup it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87k0ibt7ej.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Recent changes in soc-pcm completely broke basic support for mixers on
Intel systems: the filters on BE states prevent the connection of a
second mixer input while the back-end is already active.
Rather than reverting the changes, which would be problematic for
Tegra systems, this patch suggests an additional filter which will
only apply to Tegra systems. This is a temporary solution which will
have to be revisited - additional issues have been reported with DPCM.
Fixes: 0c25db3f76 ('ASoC: soc-pcm: Don't reconnect an already active BE')
Suggested-by: Sameer Pujar <spujar@nvidia.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Link: https://lore.kernel.org/r/20211004212141.193136-1-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Export a couple of DAPM functions that can be used by
ASoC drivers to determine connected widgets when a PCM
is started.
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Link: https://lore.kernel.org/r/20210927120517.20505-6-peter.ujfalusi@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In some cases, multiple FE components have the same BE component in their
respective DPCM paths. One such example would be a mixer component, which
can receive two or more inputs and sends a mixed output. In such cases,
to avoid reconfiguration of already active DAI (mixer output DAI in this
case), check the BE stream state to filter out the redundancy.
In summary, allow connection of BE if the respective current stream state
is either NEW or CLOSED.
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Link: https://lore.kernel.org/r/1631551342-25469-2-git-send-email-spujar@nvidia.com
Signed-off-by: Mark Brown <broonie@kernel.org>
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>
Message-Id: <20210817164054.250028-3-pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
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>
Message-Id: <20210817164054.250028-2-pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch cleanups below cppcheck warning.
sound/soc/soc-pcm.c:1624:30: style: The scope of the variable 'codec_stream' can be reduced. [variableScope]
struct snd_soc_pcm_stream *codec_stream;
^
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87o8aozf28.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch cleanups below cppcheck warning.
sound/soc/soc-pcm.c:1305:30: style: The scope of the variable 'widget' can be reduced. [variableScope]
struct snd_soc_dapm_widget *widget;
^
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87pmv4zf2c.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch cleanups below cppcheck warning.
sound/soc/soc-pcm.c:2578:22: style: The scope of the variable 'codec_dai' can be reduced. [variableScope]
struct snd_soc_dai *codec_dai;
^
sound/soc/soc-pcm.c:2580:6: style: The scope of the variable 'stream' can be reduced. [variableScope]
int stream;
^
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87r1fkzf2g.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch cleanups below cppcheck warning.
sound/soc/soc-pcm.c:631:9: style: The scope of the variable 'r' can be reduced. [variableScope]
int i, r, ret = 0;
^
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87sg00zf2l.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch cleanups below cppcheck warning.
sound/soc/soc-pcm.c:446:29: style: The scope of the variable 'pcm_codec' can be reduced. [variableScope]
struct snd_soc_pcm_stream *pcm_codec, *pcm_cpu;
^
sound/soc/soc-pcm.c:446:41: style: The scope of the variable 'pcm_cpu' can be reduced. [variableScope]
struct snd_soc_pcm_stream *pcm_codec, *pcm_cpu;
^
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87tukgzf2p.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
On stream stop, currently CPU DAI stop sequence invoked first
followed by DMA. For Few platforms, it is required to stop the
DMA first before stopping CPU DAI.
Introduced new flag in dai_link structure for reordering stop sequence.
Based on flag check, ASoC core will re-order the stop sequence.
Fixes: 4378f1fbe9 ("ASoC: soc-pcm: Use different sequence for start/stop trigger")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Link: https://lore.kernel.org/r/20210716123015.15697-1-vijendar.mukunda@amd.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In case, where the loops are not executed for a reason, the uninitialized
variable 'err' is returned to the caller. Make code fully predictible
and assign zero in the declaration.
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/20210614071746.1787072-1-perex@perex.cz
Signed-off-by: Mark Brown <broonie@kernel.org>
soc_pcm_params_symmetry() checks CPU / Codec symmetry.
Unfortunately there was bug on it (= A) which didn't check Codec.
But is back by (B).
A: v5.7: commit c840f7698d ("ASoC: soc-pcm: Merge for_each_rtd_cpu/codec_dais()")
B: v5.12: commit 3a90672111 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")
In total,
old - v5.6 (= Generation-1):
symmetric_rate : DAI_Link / CPU / Codec
symmetric_channels : DAI_Link / CPU / Codec
symmetric_sample_bits : DAI_Link / CPU / Codec
v5.7 - v5.11 (= Generation-2): (= because of bug by (A))
symmetric_rate : DAI_Link / CPU
symmetric_channels : DAI_Link / CPU / Codec
symmetric_sample_bits : DAI_Link / CPU / Codec
v5.12 - (= Generation-3): (= back by (B))
symmetric_rate : DAI_Link / CPU / Codec
symmetric_channels : DAI_Link / CPU / Codec
symmetric_sample_bits : DAI_Link / CPU / Codec
OTOH, we can use DPCM which is configured by FE / BE.
Both FE / BE uses dummy-DAI.
FE: CPU <-> dummy-DAI
BE: dummy-DAI <-> Codec
One note is that we can use .be_hw_params_fixup in DPCM case.
This means BE settings might be fixuped/updated by FE.
This feature is used for example on MIXer case.
It can be happen not only for rate, but for channels/sample_bits too.
Because of these reasons, below issue happen on
Generation-1 / Generation-3, if...
1) Sound Card used DPCM
2) It exchanges rate to 48kHz by using .be_hw_params_fixup()
3) Codec had symmetric_rate = 1
I didn't confirm, but maybe same things happen
if it exchanged channels/sample_bits at Generation-1/2/3 too.
# aplay 44100.wav
# aplay 44100.wav
=> [kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: snd-soc-dummy-dai:44100 - soc_pcm_params_symmetry:48000
[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22
[kernel] fe.rsnd-dai.0: ASoC: hw_params BE failed -22
aplay: set_params:1407: Unable to install hw params:
ACCESS: RW_INTERLEAVED
FORMAT: S16_LE
SUBFORMAT: STD
SAMPLE_BITS: 16
FRAME_BITS: 32
CHANNELS: 2
RATE: 44100
PERIOD_TIME: (23219 23220)
PERIOD_SIZE: 1024
PERIOD_BYTES: 4096
PERIODS: 4
BUFFER_TIME: (92879 92880)
BUFFER_SIZE: 4096
BUFFER_BYTES: 16384
TICK_TIME: 0
soc_pcm_params_symmetry() checks by below
if (symmetry)
for_each_rtd_cpu_dais(rtd, i, cpu_dai)
if (cpu_dai->xxx && cpu_dai->xxx != d.xxx) {
dev_err(rtd->dev, "...");
return -EINVAL;
}
Because of above reason 3) (= Codec had symmetric_rate = 1)
BE can't ignore "if (symmetric)".
At 1st aplay, soc_pcm_params_symmetry() ignores it,
because dummy-DAI->rate is 0.
After this check, each DAI sets/keep settings.
In above sample case, BE gets 48000 and FE gets 44100,
and it happen BE -> FE order.
Because DPCM is sharing *same* dummy-DAI,
dummy-DAI sets as 48000 by BE, and is overwrote by 44100 by FE.
This settings never be cleaned (= a) after 1st aplay,
because dummy-DAI is used from FE/BE, never be last user (b).
static int soc_pcm_hw_clean(...)
{
...
for_each_rtd_dais(rtd, i, dai) {
...
(b) if (snd_soc_dai_active(dai) == 1)
(a) soc_pcm_set_dai_params(dai, NULL);
...
}
...
}
At 2nd aplay, BE gets 48000 but dummy-DAI is keeping 44100,
soc_pcm_params_symmetry() checks will fail.
To solve this issue, this patch ignores dummy-DAI
at soc_pcm_params_symmetry()
Link: https://lore.kernel.org/r/87a6q0z4xt.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87y2djxa2n.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
It indicates unmatched symmetry value, but not indicates on which DAI.
This patch indicates it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/871rbbyono.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
__soc_pcm_params_symmetry() macro is using "name" as parameter
which will be exchanged to rate/channles/sample_bit, like below
dai->name => dai->rate
dai->name => dai->channels
dai->name => dai->sample_bit
But, dai itself has "name". This means
1) It is very confusable naming
2) It can't use dai->name
This patch use "xxx" instead of "name"
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/8735vryoob.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Add snd_soc_pcm_component_ack back, which can be used to get an
updated buffer pointer in the platform driver.
On Asymmetric multiprocessor, this pointer can be sent to Cortex-M
core for audio processing.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Link: https://lore.kernel.org/r/1615516725-4975-2-git-send-email-shengjiu.wang@nxp.com
Signed-off-by: Mark Brown <broonie@kernel.org>
All snd_soc_component_xxx() and snd_soc_pcm_component_xxx() itself
indicate error message if failed.
Its caller doesn't need to indicate duplicated error message.
This patch removes it.
All snd_soc_component_xxx() indicate error message if failed.
Its caller doesn't need to indicate duplicated error message.
This patch removes it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/878s6puta6.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
All snd_soc_dai_xxx() and snd_soc_pcm_dai_xxx() itself
indicate error message if failed.
Its caller doesn't need to indicate duplicated error message.
This patch removes it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87a6r5utaa.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
soc_pcm_hw_free() never fail, error message is not needed.
We can't use void function for it, because it is used
part of struct snd_pcm_ops :: hw_free.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87czw1utaj.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
This patch follow above style at dpcm_fe/be_dai_prepare()
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87eeghutap.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
This patch follow above style at dpcm_fe/be_dai_hw_params()
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87ft0xutat.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
This patch follow above style at dpcm_fe/be_dai_startup().
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87h7ldutay.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
This patch also
do below to dpcm_run_update_startup()
1) remove duplicated ret = -EINVAL
2) remove blank line
do below to dpcm_run_update_shutdown()
1) remove unused ret
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87im5tutb3.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
This patch follow above style at dpcm_apply_symmetry(...)
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87k0q9utb9.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Indicating error message when failed case is very useful for debuging.
In many case, its style is like below.
int function(...)
{
...
return ret;
}
int caller(...)
{
...
ret = function(...);
if (ret < 0)
dev_err(...)
...
}
This is not so bad, but in this style *each caller* needs to indicate
duplicate same error message, and some caller is forgetting to do it.
And caller can't indicate detail function() error information.
If function() indicates error message, we can get same and
detail information without forgot.
int function(...)
{
...
if (ret < 0)
dev_err(...)
return ret;
}
int caller(...)
{
...
ret = function(...);
...
}
Now, dpcm_be_dai_trigger() user uses it like below.
err = dpcm_be_dai_trigger(...);
if (err < 0)
dev_err(..., "ASoC: trigger FE failed %d\n", err);
But we can get more detail information if dpcm_be_dai_trigger() itself
had dev_err(). And above error message is confusable,
failed is *BE*, not *FE*.
This patch indicates error message at dpcm_be_dai_trigger().
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87lfaputbe.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
