[alsa-devel] [PATCH v2 0/5] soundwire: stream: fix state machines and transitions
The existing stream support works fine with simple cases, but does not map well with ALSA transitions for underflows/resume where prepare() can be called multiple times. Concurrency with multiple devices per links or multiple streams enabled on the same link also needs to be fixed.
These patches are the result of hours of validation on the Intel side and should benefit other implementations since there is nothing hardware-specific. The Intel-specific changes being reviewed do depend on those stream changes though to be functional.
Changes since v1: Removed spurious code block change flagged by Vinod
No change (replies provided in v1 thread) Github link issue is public, no reason to remove it Bandwidth computation on ALSA prepare/start (for resume cases) handled internally in stream layer. Kept emacs comment formatting. No additional code/test for concurrent streams (not supported due to locking)
Bard Liao (1): soundwire: stream: only prepare stream when it is configured.
Pierre-Louis Bossart (2): soundwire: stream: update state machine and add state checks soundwire: stream: do not update parameters during DISABLED-PREPARED transition
Rander Wang (2): soundwire: stream: fix support for multiple Slaves on the same link soundwire: stream: don't program ports when a stream that has not been prepared
Documentation/driver-api/soundwire/stream.rst | 61 +++++++++---- drivers/soundwire/stream.c | 90 ++++++++++++++++--- 2 files changed, 124 insertions(+), 27 deletions(-)
The state machine and notes don't accurately explain or allow transitions from STREAM_DEPREPARED and STREAM_DISABLED.
Add more explanations and allow for more transitions as a result of a trigger_stop(), trigger_suspend() and prepare(), depending on the ALSA/ASoC layer behavior defined by the INFO_RESUME and INFO_PAUSE flags.
Also add basic checks to help debug inconsistent states and illegal state machine transitions.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- Documentation/driver-api/soundwire/stream.rst | 61 +++++++++++++------ drivers/soundwire/stream.c | 37 +++++++++++ 2 files changed, 81 insertions(+), 17 deletions(-)
diff --git a/Documentation/driver-api/soundwire/stream.rst b/Documentation/driver-api/soundwire/stream.rst index 5351bd2f34a8..8bceece51554 100644 --- a/Documentation/driver-api/soundwire/stream.rst +++ b/Documentation/driver-api/soundwire/stream.rst @@ -156,22 +156,27 @@ Below shows the SoundWire stream states and state transition diagram. :: +-----------+ +------------+ +----------+ +----------+ | ALLOCATED +---->| CONFIGURED +---->| PREPARED +---->| ENABLED | | STATE | | STATE | | STATE | | STATE | - +-----------+ +------------+ +----------+ +----+-----+ - ^ - | - | - v - +----------+ +------------+ +----+-----+ + +-----------+ +------------+ +---+--+---+ +----+-----+ + ^ ^ ^ + | | | + __| |___________ | + | | | + v | v + +----------+ +-----+------+ +-+--+-----+ | RELEASED |<----------+ DEPREPARED |<-------+ DISABLED | | STATE | | STATE | | STATE | +----------+ +------------+ +----------+
-NOTE: State transition between prepare and deprepare is supported in Spec -but not in the software (subsystem) +NOTE: State transitions between ``SDW_STREAM_ENABLED`` and +``SDW_STREAM_DISABLED`` are only relevant when then INFO_PAUSE flag is +supported at the ALSA/ASoC level. Likewise the transition between +``SDW_DISABLED_STATE`` and ``SDW_PREPARED_STATE`` depends on the +INFO_RESUME flag.
-NOTE2: Stream state transition checks need to be handled by caller -framework, for example ALSA/ASoC. No checks for stream transition exist in -SoundWire subsystem. +NOTE2: The framework implements basic state transition checks, but +does not e.g. check if a transition from DISABLED to ENABLED is valid +on a specific platform. Such tests need to be added at the ALSA/ASoC +level.
Stream State Operations ----------------------- @@ -246,6 +251,9 @@ SDW_STREAM_PREPARED
Prepare state of stream. Operations performed before entering in this state:
+ (0) Steps 1 and 2 are omitted in the case of a resume operation, + where the bus bandwidth is known. + (1) Bus parameters such as bandwidth, frame shape, clock frequency, are computed based on current stream as well as already active stream(s) on Bus. Re-computation is required to accommodate current @@ -270,9 +278,11 @@ Prepare state of stream. Operations performed before entering in this state: After all above operations are successful, stream state is set to ``SDW_STREAM_PREPARED``.
-Bus implements below API for PREPARE state which needs to be called once per -stream. From ASoC DPCM framework, this stream state is linked to -.prepare() operation. +Bus implements below API for PREPARE state which needs to be called +once per stream. From ASoC DPCM framework, this stream state is linked +to .prepare() operation. Since the .trigger() operations may not +follow the .prepare(), a direct transition from +``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed.
.. code-block:: c
@@ -332,6 +342,14 @@ Bus implements below API for DISABLED state which needs to be called once per stream. From ASoC DPCM framework, this stream state is linked to .trigger() stop operation.
+When the INFO_PAUSE flag is supported, a direct transition to +``SDW_STREAM_ENABLED`` is allowed. + +For resume operations where ASoC will use the .prepare() callback, the +stream can transition from ``SDW_STREAM_DISABLED`` to +``SDW_STREAM_PREPARED``, with all required settings restored but +without updating the bandwidth and bit allocation. + .. code-block:: c
int sdw_disable_stream(struct sdw_stream_runtime * stream); @@ -353,9 +371,18 @@ state: After all above operations are successful, stream state is set to ``SDW_STREAM_DEPREPARED``.
-Bus implements below API for DEPREPARED state which needs to be called once -per stream. From ASoC DPCM framework, this stream state is linked to -.trigger() stop operation. +Bus implements below API for DEPREPARED state which needs to be called +once per stream. ALSA/ASoC do not have a concept of 'deprepare', and +the mapping from this stream state to ALSA/ASoC operation may be +implementation specific. + +When the INFO_PAUSE flag is supported, the stream state is linked to +the .hw_free() operation - the stream is not deprepared on a +TRIGGER_STOP. + +Other implementations may transition to the ``SDW_STREAM_DEPREPARED`` +state on TRIGGER_STOP, should they require a transition through the +``SDW_STREAM_PREPARED`` state.
.. code-block:: c
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 178ae92b8cc1..6aa0b5d370c0 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1553,8 +1553,18 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state != SDW_STREAM_CONFIGURED && + stream->state != SDW_STREAM_DEPREPARED && + stream->state != SDW_STREAM_DISABLED) { + pr_err("%s: %s: inconsistent state state %d\n", + __func__, stream->name, stream->state); + ret = -EINVAL; + goto state_err; + } + ret = _sdw_prepare_stream(stream);
+state_err: sdw_release_bus_lock(stream); return ret; } @@ -1619,8 +1629,17 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state != SDW_STREAM_PREPARED && + stream->state != SDW_STREAM_DISABLED) { + pr_err("%s: %s: inconsistent state state %d\n", + __func__, stream->name, stream->state); + ret = -EINVAL; + goto state_err; + } + ret = _sdw_enable_stream(stream);
+state_err: sdw_release_bus_lock(stream); return ret; } @@ -1693,8 +1712,16 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state != SDW_STREAM_ENABLED) { + pr_err("%s: %s: inconsistent state state %d\n", + __func__, stream->name, stream->state); + ret = -EINVAL; + goto state_err; + } + ret = _sdw_disable_stream(stream);
+state_err: sdw_release_bus_lock(stream); return ret; } @@ -1749,8 +1776,18 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) }
sdw_acquire_bus_lock(stream); + + if (stream->state != SDW_STREAM_PREPARED && + stream->state != SDW_STREAM_DISABLED) { + pr_err("%s: %s: inconsistent state state %d\n", + __func__, stream->name, stream->state); + ret = -EINVAL; + goto state_err; + } + ret = _sdw_deprepare_stream(stream);
+state_err: sdw_release_bus_lock(stream); return ret; }
From: Bard Liao yung-chuan.liao@linux.intel.com
We don't need to prepare the stream again if the stream is already prepared.
sdw_prepare_stream() could be called multiple times without calling sdw_deprepare_stream(). We call sdw_prepare_stream() in the prepare dai ops and sdw_deprepare_stream() in the hw_free dai ops. If an xrun happens, sdw_prepare_stream() will be called but sdw_deprepare_stream() will not, which results in an imbalance and an invalid total bandwidth.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/stream.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 6aa0b5d370c0..bd0bddf73830 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1544,7 +1544,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) */ int sdw_prepare_stream(struct sdw_stream_runtime *stream) { - int ret = 0; + int ret;
if (!stream) { pr_err("SoundWire: Handle not found for stream\n"); @@ -1553,6 +1553,11 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state == SDW_STREAM_PREPARED) { + ret = 0; + goto state_err; + } + if (stream->state != SDW_STREAM_CONFIGURED && stream->state != SDW_STREAM_DEPREPARED && stream->state != SDW_STREAM_DISABLED) {
After a system suspend, the ALSA/ASoC core will invoke the .prepare() callback and a TRIGGER_START when INFO_RESUME is not supported.
Likewise, when an underflow occurs, the .prepare callback will be invoked.
In both cases, the stream can be in DISABLED mode, and will transition into the PREPARED mode. We however don't want the bus bandwidth to be recomputed.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/stream.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index bd0bddf73830..c28ce7f0d742 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1460,7 +1460,8 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream) } }
-static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) +static int _sdw_prepare_stream(struct sdw_stream_runtime *stream, + bool update_params) { struct sdw_master_runtime *m_rt; struct sdw_bus *bus = NULL; @@ -1480,6 +1481,9 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) return -EINVAL; }
+ if (!update_params) + goto program_params; + /* Increment cumulative bus bandwidth */ /* TODO: Update this during Device-Device support */ bus->params.bandwidth += m_rt->stream->params.rate * @@ -1495,6 +1499,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) } }
+program_params: /* Program params */ ret = sdw_program_params(bus); if (ret < 0) { @@ -1544,6 +1549,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) */ int sdw_prepare_stream(struct sdw_stream_runtime *stream) { + bool update_params = true; int ret;
if (!stream) { @@ -1567,7 +1573,16 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream) goto state_err; }
- ret = _sdw_prepare_stream(stream); + /* + * when the stream is DISABLED, this means sdw_prepare_stream() + * is called as a result of an underflow or a resume operation. + * In this case, the bus parameters shall not be recomputed, but + * still need to be re-applied + */ + if (stream->state == SDW_STREAM_DISABLED) + update_params = false; + + ret = _sdw_prepare_stream(stream, update_params);
state_err: sdw_release_bus_lock(stream);
From: Rander Wang rander.wang@intel.com
The existing code will unconditionally return after dealing with the first Slave on a link. This return should only happen when there is an error case.
Tested on Comet Lake platform.
Signed-off-by: Rander Wang rander.wang@intel.com --- drivers/soundwire/stream.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index c28ce7f0d742..da10f38298c0 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -587,10 +587,11 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
if (slave->ops->bus_config) { ret = slave->ops->bus_config(slave, &bus->params); - if (ret < 0) + if (ret < 0) { dev_err(bus->dev, "Notify Slave: %d failed\n", slave->dev_num); - return ret; + return ret; + } } }
From: Rander Wang rander.wang@intel.com
In the Intel QA multi-pipelines test case, there are two pipelines for playback and capture on the same bus. The test fails with an error when setting port params:
[ 599.224812] rt711 sdw:0:25d:711:0: invalid dpn_prop direction 1 port_num 0 [ 599.224815] sdw_program_slave_port_params failed -22 [ 599.224819] intel-sdw sdw-master-0: Program transport params failed: -22 [ 599.224822] intel-sdw sdw-master-0: Program params failed: -22 [ 599.224828] sdw_enable_stream: SDW0 Pin2-Playback: done
This problem is root-caused to the programming of the capture stream ports while it is not yet prepared, the calling sequence is:
(1) hw_params for playback. The playback stream provide the port information to Bus. (2) stream_prepare for playback, Transport and port parameters are computed for playback. (3) hw_params for capture. The capture stream provide the port information to Bus, but it has not been prepared so is not accounted for in the bandwidth allocation. (4) stream_enable for playback. Program transport and port parameters for all masters and slaves. Since the transport and port parameters are not computed for capture stream, sdw_program_slave_port_params will generate a error when setting port params for capture.
in step (4), we should only program the ports for the stream that have been prepared. A stream that is only in CONFIGURED state should be ignored, its ports will be programmed when it becomes PREPARED.
Tested on Comet Lake.
GitHub issue: https://github.com/thesofproject/linux/issues/1637 Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/stream.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index da10f38298c0..00348d1fc606 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -603,13 +603,25 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt) * and Slave(s) * * @bus: SDW bus instance + * @prepare: true if sdw_program_params() is called by _prepare. */ -static int sdw_program_params(struct sdw_bus *bus) +static int sdw_program_params(struct sdw_bus *bus, bool prepare) { struct sdw_master_runtime *m_rt; int ret = 0;
list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) { + + /* + * this loop walks through all master runtimes for a + * bus, but the ports can only be configured while + * explicitly preparing a stream or handling an + * already-prepared stream otherwise. + */ + if (!prepare && + m_rt->stream->state == SDW_STREAM_CONFIGURED) + continue; + ret = sdw_program_port_params(m_rt); if (ret < 0) { dev_err(bus->dev, @@ -1502,7 +1514,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
program_params: /* Program params */ - ret = sdw_program_params(bus); + ret = sdw_program_params(bus, true); if (ret < 0) { dev_err(bus->dev, "Program params failed: %d\n", ret); goto restore_params; @@ -1602,7 +1614,7 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) bus = m_rt->bus;
/* Program params */ - ret = sdw_program_params(bus); + ret = sdw_program_params(bus, false); if (ret < 0) { dev_err(bus->dev, "Program params failed: %d\n", ret); return ret; @@ -1687,7 +1699,7 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) struct sdw_bus *bus = m_rt->bus;
/* Program params */ - ret = sdw_program_params(bus); + ret = sdw_program_params(bus, false); if (ret < 0) { dev_err(bus->dev, "Program params failed: %d\n", ret); return ret; @@ -1769,7 +1781,7 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream) m_rt->ch_count * m_rt->stream->params.bps;
/* Program params */ - ret = sdw_program_params(bus); + ret = sdw_program_params(bus, false); if (ret < 0) { dev_err(bus->dev, "Program params failed: %d\n", ret); return ret;
On 1/14/20 5:52 PM, Pierre-Louis Bossart wrote:
The existing stream support works fine with simple cases, but does not map well with ALSA transitions for underflows/resume where prepare() can be called multiple times. Concurrency with multiple devices per links or multiple streams enabled on the same link also needs to be fixed.
These patches are the result of hours of validation on the Intel side and should benefit other implementations since there is nothing hardware-specific. The Intel-specific changes being reviewed do depend on those stream changes though to be functional.
Vinod, these patches have been in the queue for quite some time, and v5.6-rc1 is out. Can we move on with the reviews? Thanks!
Changes since v1: Removed spurious code block change flagged by Vinod
No change (replies provided in v1 thread) Github link issue is public, no reason to remove it Bandwidth computation on ALSA prepare/start (for resume cases) handled internally in stream layer. Kept emacs comment formatting. No additional code/test for concurrent streams (not supported due to locking)
Bard Liao (1): soundwire: stream: only prepare stream when it is configured.
Pierre-Louis Bossart (2): soundwire: stream: update state machine and add state checks soundwire: stream: do not update parameters during DISABLED-PREPARED transition
Rander Wang (2): soundwire: stream: fix support for multiple Slaves on the same link soundwire: stream: don't program ports when a stream that has not been prepared
Documentation/driver-api/soundwire/stream.rst | 61 +++++++++---- drivers/soundwire/stream.c | 90 ++++++++++++++++--- 2 files changed, 124 insertions(+), 27 deletions(-)
On 14-01-20, 17:52, Pierre-Louis Bossart wrote:
The existing stream support works fine with simple cases, but does not map well with ALSA transitions for underflows/resume where prepare() can be called multiple times. Concurrency with multiple devices per links or multiple streams enabled on the same link also needs to be fixed.
These patches are the result of hours of validation on the Intel side and should benefit other implementations since there is nothing hardware-specific. The Intel-specific changes being reviewed do depend on those stream changes though to be functional.
Applied, thanks
participants (2)
-
Pierre-Louis Bossart
-
Vinod Koul