* [PATCH 2/6] soundwire: stream: update state machine and add state checks
[not found] <20200108175438.13121-1-pierre-louis.bossart@linux.intel.com>
@ 2020-01-08 17:54 ` Pierre-Louis Bossart
2020-01-10 6:48 ` Vinod Koul
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-08 17:54 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank,
srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang,
Ranjani Sridharan, Pierre-Louis Bossart, Sanyog Kale,
Jonathan Corbet, open list:DOCUMENTATION
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/6] soundwire: stream: update state machine and add state checks
2020-01-08 17:54 ` [PATCH 2/6] soundwire: stream: update state machine and add state checks Pierre-Louis Bossart
@ 2020-01-10 6:48 ` Vinod Koul
2020-01-10 16:30 ` [alsa-devel] " Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2020-01-10 6:48 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank,
srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang,
Ranjani Sridharan, Sanyog Kale, Jonathan Corbet,
open list:DOCUMENTATION
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks
2020-01-10 6:48 ` Vinod Koul
@ 2020-01-10 16:30 ` Pierre-Louis Bossart
2020-01-11 11:30 ` Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-10 16:30 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank,
srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang,
Ranjani Sridharan, Sanyog Kale, Jonathan Corbet,
open list:DOCUMENTATION
>> - 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks
2020-01-10 16:30 ` [alsa-devel] " Pierre-Louis Bossart
@ 2020-01-11 11:30 ` Pierre-Louis Bossart
2020-01-13 5:22 ` Vinod Koul
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-11 11:30 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, Jonathan Corbet, tiwai, gregkh,
open list:DOCUMENTATION, linux-kernel, Ranjani Sridharan,
broonie, srinivas.kandagatla, jank, slawomir.blauciak,
Sanyog Kale, Bard liao, Rander Wang
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH 2/6] soundwire: stream: update state machine and add state checks
2020-01-11 11:30 ` Pierre-Louis Bossart
@ 2020-01-13 5:22 ` Vinod Koul
0 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2020-01-13 5:22 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, Jonathan Corbet, tiwai, gregkh,
open list:DOCUMENTATION, linux-kernel, Ranjani Sridharan,
broonie, srinivas.kandagatla, jank, slawomir.blauciak,
Sanyog Kale, Bard liao, Rander Wang
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-13 5:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200108175438.13121-1-pierre-louis.bossart@linux.intel.com>
2020-01-08 17:54 ` [PATCH 2/6] soundwire: stream: update state machine and add state checks Pierre-Louis Bossart
2020-01-10 6:48 ` Vinod Koul
2020-01-10 16:30 ` [alsa-devel] " Pierre-Louis Bossart
2020-01-11 11:30 ` Pierre-Louis Bossart
2020-01-13 5:22 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).