[PATCH 0/6] soundwire: stream: fix state machines and transitions

[PATCH 0/6] soundwire: stream: fix state machines and transitions

[Pierre-Louis Bossart]

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.

Bard Liao (1):
  soundwire: stream: only prepare stream when it is configured.

Pierre-Louis Bossart (3):
  soundwire: stream: remove redundant pr_err traces
  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 for a stream that has not been
    prepared

 Documentation/driver-api/soundwire/stream.rst | 63 ++++++++----
 drivers/soundwire/stream.c                    | 97 +++++++++++++++----
 2 files changed, 124 insertions(+), 36 deletions(-)


base-commit: 09f6a72d014386939d21899921dd379006471a4b
-- 
2.20.1

[PATCH 1/6] soundwire: stream: remove redundant pr_err traces

[Pierre-Louis Bossart]

Only keep pr_err to flag critical configuration errors that will
typically only happen during system integration.

For errors on prepare/deprepare/enable/disable, the caller can do a
much better job with more information on the DAI and device that
caused the issue.

Suggested-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/stream.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index e69f94a8c3a8..178ae92b8cc1 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1554,8 +1554,6 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
 	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_prepare_stream(stream);
-	if (ret < 0)
-		pr_err("Prepare for stream:%s failed: %d\n", stream->name, ret);
 
 	sdw_release_bus_lock(stream);
 	return ret;
@@ -1622,8 +1620,6 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
 	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_enable_stream(stream);
-	if (ret < 0)
-		pr_err("Enable for stream:%s failed: %d\n", stream->name, ret);
 
 	sdw_release_bus_lock(stream);
 	return ret;
@@ -1698,8 +1694,6 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
 	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_disable_stream(stream);
-	if (ret < 0)
-		pr_err("Disable for stream:%s failed: %d\n", stream->name, ret);
 
 	sdw_release_bus_lock(stream);
 	return ret;
@@ -1756,8 +1750,6 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 	ret = _sdw_deprepare_stream(stream);
-	if (ret < 0)
-		pr_err("De-prepare for stream:%d failed: %d\n", ret, ret);
 
 	sdw_release_bus_lock(stream);
 	return ret;
-- 
2.20.1

[PATCH 2/6] soundwire: stream: update state machine and add state checks

[Pierre-Louis Bossart]

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 | 63 +++++++++++++------
 drivers/soundwire/stream.c                    | 37 +++++++++++
 2 files changed, 82 insertions(+), 18 deletions(-)

diff --git a/Documentation/driver-api/soundwire/stream.rst b/Documentation/driver-api/soundwire/stream.rst
index 5351bd2f34a8..9b7418ff8d59 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,13 +278,15 @@ 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 transitions from
+``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed.
 
 .. code-block:: c
 
-  int sdw_prepare_stream(struct sdw_stream_runtime * stream);
+  int sdw_prepare_stream(struct sdw_stream_runtime * stream, bool resume);
 
 
 SDW_STREAM_ENABLED
@@ -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;
 }
-- 
2.20.1

[PATCH 3/6] soundwire: stream: only prepare stream when it is configured.

[Pierre-Louis Bossart]

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) {
-- 
2.20.1

[PATCH 4/6] soundwire: stream: do not update parameters during DISABLED-PREPARED transition

[Pierre-Louis Bossart]

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);
-- 
2.20.1

[PATCH 5/6] soundwire: stream: fix support for multiple Slaves on the same link

[Pierre-Louis Bossart]

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>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.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;
+			}
 		}
 	}
 
-- 
2.20.1

[PATCH 6/6] soundwire: stream: don't program ports for a stream that has not been prepared

[Pierre-Louis Bossart]

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 | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index da10f38298c0..198372977187 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -604,12 +604,23 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
  *
  * @bus: SDW bus instance
  */
-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 +1513,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 +1613,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 +1698,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 +1780,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;
-- 
2.20.1

Re: [PATCH 2/6] soundwire: stream: update state machine and add state checks

[Vinod Koul]

On 08-01-20, 11:54, Pierre-Louis Bossart wrote:

>  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,13 +278,15 @@ 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 transitions from
> +``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed.
>  
>  .. code-block:: c
>  
> -  int sdw_prepare_stream(struct sdw_stream_runtime * stream);
> +  int sdw_prepare_stream(struct sdw_stream_runtime * stream, bool resume);

so what does the additional argument of resume do..?

> 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)

and it is not modified here, so is the doc correct or this..?

-- 
~Vinod

Re: [PATCH 4/6] soundwire: stream: do not update parameters during DISABLED-PREPARED transition

[Vinod Koul]

On 08-01-20, 11:54, Pierre-Louis Bossart wrote:
> 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;

Should this not be handled by the caller..? I do not like to deduce this
here as the info is already available in dai driver, so go ahead and
propagate it and get it from caller when it is required..

-- 
~Vinod

Re: [PATCH 6/6] soundwire: stream: don't program ports for a stream that has not been prepared

[Vinod Koul]

On 08-01-20, 11:54, Pierre-Louis Bossart wrote:
> 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

side note, one of these above err logs should be removed :)

> [  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

This is not relevant for kernel, pls remove

> 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 | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index da10f38298c0..198372977187 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -604,12 +604,23 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
>   *
>   * @bus: SDW bus instance
>   */
> -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.

we can go upto 80 chars, make sure you align the above comment block as
such

> +		 */
> +		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 +1513,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 +1613,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 +1698,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);

Can you do a converse test as well, when the streams are running and
concurrently two stream are stopped, it would be good to get it confirmed...

>  		if (ret < 0) {
>  			dev_err(bus->dev, "Program params failed: %d\n", ret);
>  			return ret;
> @@ -1769,7 +1780,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;
> -- 
> 2.20.1

-- 
~Vinod

Re: [PATCH 1/6] soundwire: stream: remove redundant pr_err traces

[Vinod Koul]

On 08-01-20, 11:54, Pierre-Louis Bossart wrote:
> Only keep pr_err to flag critical configuration errors that will
> typically only happen during system integration.
> 
> For errors on prepare/deprepare/enable/disable, the caller can do a
> much better job with more information on the DAI and device that
> caused the issue.

Applied, thanks

-- 
~Vinod

Re: [alsa-devel] [PATCH 4/6] soundwire: stream: do not update parameters during DISABLED-PREPARED transition

[Pierre-Louis Bossart]

>> +	/*
>> +	 * 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;
> 
> Should this not be handled by the caller..? I do not like to deduce this
> here as the info is already available in dai driver, so go ahead and
> propagate it and get it from caller when it is required..

No, this update_params boolean is used later on to modify the bandwidth 
computation. These values are not accessible to the caller (and should 
absolutely be kept private/opaque).

Re: [alsa-devel] [PATCH 6/6] soundwire: stream: don't program ports for a stream that has not been prepared

[Pierre-Louis Bossart]

>> GitHub issue: https://github.com/thesofproject/linux/issues/1637
> 
> This is not relevant for kernel, pls remove

Why? it's not uncommon to have bugzilla links, why would we lose the 
publicly-available information because GitHub is used?

>>   	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.
> 
> we can go upto 80 chars, make sure you align the above comment block as
> such

this is formatted by emacs, and with long words you get spaces at the end.

>>   		/* Program params */
>> -		ret = sdw_program_params(bus);
>> +		ret = sdw_program_params(bus, false);
> 
> Can you do a converse test as well, when the streams are running and
> concurrently two stream are stopped, it would be good to get it confirmed...

we cannot concurrently stop two streams since we take a bus lock. That's 
a problem but it'll have to be addressed separately. the problem with 
multiple streams addressed here is when one is CONFIGURED, which does 
not require a bus lock.

Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks

[Pierre-Louis Bossart]

>> -  int sdw_prepare_stream(struct sdw_stream_runtime * stream);
>> +  int sdw_prepare_stream(struct sdw_stream_runtime * stream, bool resume);
> 
> so what does the additional argument of resume do..?
> 
>> 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)
> 
> and it is not modified here, so is the doc correct or this..?

the doc is correct and the code is updated in

[PATCH 4/6] soundwire: stream: do not update parameters during 
DISABLED-PREPARED transition

Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks

[Pierre-Louis Bossart]

On 1/10/20 10:30 AM, Pierre-Louis Bossart wrote:
> 
>>> -  int sdw_prepare_stream(struct sdw_stream_runtime * stream);
>>> +  int sdw_prepare_stream(struct sdw_stream_runtime * stream, bool 
>>> resume);
>>
>> so what does the additional argument of resume do..?
>>
>>> 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)
>>
>> and it is not modified here, so is the doc correct or this..?
> 
> the doc is correct and the code is updated in
> 
> [PATCH 4/6] soundwire: stream: do not update parameters during 
> DISABLED-PREPARED transition

Sorry, wrong answer, my bad. The code block in the documentation is 
incorrect.

The Patch 4/6 implements the transition mentioned in the documentation, 
but the extra parameter is a left-over from an earlier version. This 
case is now handled internally. We did revert to the initial prototype 
after finding out that dealing with transitions in the caller is 
error-prone.

Will fix in v2, thanks for spotting this.

Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks

[Vinod Koul]

On 11-01-20, 05:30, Pierre-Louis Bossart wrote:
> 
> 
> On 1/10/20 10:30 AM, Pierre-Louis Bossart wrote:
> > 
> > > > -  int sdw_prepare_stream(struct sdw_stream_runtime * stream);
> > > > +  int sdw_prepare_stream(struct sdw_stream_runtime * stream,
> > > > bool resume);
> > > 
> > > so what does the additional argument of resume do..?
> > > 
> > > > 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)
> > > 
> > > and it is not modified here, so is the doc correct or this..?
> > 
> > the doc is correct and the code is updated in
> > 
> > [PATCH 4/6] soundwire: stream: do not update parameters during
> > DISABLED-PREPARED transition
> 
> Sorry, wrong answer, my bad. The code block in the documentation is
> incorrect.
> 
> The Patch 4/6 implements the transition mentioned in the documentation, but
> the extra parameter is a left-over from an earlier version. This case is now
> handled internally. We did revert to the initial prototype after finding out
> that dealing with transitions in the caller is error-prone.

Glad that you agree with me on something!

-- 
~Vinod