* [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).