Skip to content

[TEST v3] fix suspend/resume with SoundWire #1327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 4 commits into from
Oct 18, 2019
Merged

[TEST v3] fix suspend/resume with SoundWire #1327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4c2c26
soundwire: stream: update state machine and add state checks
plbossart Oct 12, 2019
45d5895
soundwire: stream: do not update parameters during DISABLED-PREPARED …
plbossart Oct 15, 2019
ac7d26f
soundwire: intel: reinitialize IP+DSP in .prepare()
bardliao Oct 14, 2019
2503fd5
[HACK] add traces to debug aplay suspend/resume issue
plbossart Oct 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 45 additions & 18 deletions Documentation/driver-api/soundwire/stream.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----------------------
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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

Expand Down
1 change: 1 addition & 0 deletions drivers/soundwire/cadence_master.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ struct sdw_cdns_stream_config {
* @bus: Bus handle
* @stream_type: Stream type
* @link_id: Master link id
* after a TRIGGER_SUSPEND.
*/
struct sdw_cdns_dma_data {
char *name;
Expand Down
93 changes: 89 additions & 4 deletions drivers/soundwire/intel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ struct sdw_intel {
struct sdw_cdns cdns;
int instance;
struct sdw_intel_link_res *link_res;
struct snd_pcm_hw_params *hw_params;
struct sdw_cdns_pdi *pdi;
#ifdef CONFIG_DEBUG_FS
struct dentry *debugfs;
#endif
Expand Down Expand Up @@ -760,6 +762,8 @@ static int intel_startup(struct snd_pcm_substream *substream,
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
int ret;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

if (pm_runtime_enabled(cdns->dev)) {
ret = pm_runtime_get_sync(cdns->dev);
if (ret < 0) {
Expand All @@ -770,7 +774,11 @@ static int intel_startup(struct snd_pcm_substream *substream,
}
}

return sdw_stream_setup(substream, dai);
ret = sdw_stream_setup(substream, dai);

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);

return ret;
}

static int intel_hw_params(struct snd_pcm_substream *substream,
Expand All @@ -787,6 +795,8 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
int ret;
bool pcm = true;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
return -EIO;
Expand Down Expand Up @@ -815,6 +825,8 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
intel_pdi_alh_configure(sdw, pdi);
sdw_cdns_config_stream(cdns, ch, dir, pdi);

sdw->pdi = pdi;
sdw->hw_params = params;

/* Inform DSP about PDI stream number */
ret = intel_params_stream(sdw, substream, dai, params,
Expand Down Expand Up @@ -851,14 +863,23 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
dev_err(cdns->dev, "add master to stream failed:%d\n", ret);

kfree(pconfig);

error:

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);
return ret;
}

static int intel_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_cdns_dma_data *dma;
int ch, dir;
int ret;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
Expand All @@ -867,7 +888,36 @@ static int intel_prepare(struct snd_pcm_substream *substream,
return -EIO;
}

return sdw_prepare_stream(dma->stream);
/*
* .prepare() is called after system resume, where we need to
* reinitialize the SHIM/ALH/Cadence IP. To avoid dealing with
* complicated state machines, we just re-initialize in all
* cases since there are no side effects.
*/

/* configure stream */
ch = params_channels(sdw->hw_params);
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
dir = SDW_DATA_DIR_RX;
else
dir = SDW_DATA_DIR_TX;

intel_pdi_shim_configure(sdw, sdw->pdi);
intel_pdi_alh_configure(sdw, sdw->pdi);
sdw_cdns_config_stream(cdns, ch, dir, sdw->pdi);

/* Inform DSP about PDI stream number */
ret = intel_params_stream(sdw, substream, dai, sdw->hw_params,
sdw->instance,
sdw->pdi->intel_alh_id);
if (ret)
goto err;

ret = sdw_prepare_stream(dma->stream);

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);
err:
return ret;
}

static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
Expand All @@ -876,21 +926,27 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
struct sdw_cdns_dma_data *dma;
int ret;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
dev_err(dai->dev, "failed to get dma data in %s", __func__);
return -EIO;
}

switch (cmd) {
case SNDRV_PCM_TRIGGER_RESUME:
dev_err(dai->dev, "%s: %s: resume\n", __func__, dai->name);
/* fallthrough */
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
case SNDRV_PCM_TRIGGER_RESUME:
ret = sdw_enable_stream(dma->stream);
break;

case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_SUSPEND:
dev_err(dai->dev, "%s: %s: suspend\n", __func__, dai->name);
/* fallthrough */
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_STOP:
ret = sdw_disable_stream(dma->stream);
break;
Expand All @@ -904,6 +960,9 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
dev_err(dai->dev,
"%s trigger %d failed: %d",
__func__, cmd, ret);

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);

return ret;
}

Expand All @@ -915,6 +974,8 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
struct sdw_cdns_dma_data *dma;
int ret;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
return -EIO;
Expand All @@ -938,8 +999,12 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
return ret;
}

sdw->hw_params = NULL;
sdw->pdi = NULL;
sdw_release_stream(dma->stream);

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);

return 0;
}

Expand All @@ -950,6 +1015,8 @@ static void intel_shutdown(struct snd_pcm_substream *substream,
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
int ret;

dev_err(dai->dev, "%s: %s: start\n", __func__, dai->name);

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
return;
Expand All @@ -965,6 +1032,8 @@ static void intel_shutdown(struct snd_pcm_substream *substream,
"pm_runtime_put_autosuspend failed in %s:, ret %d\n",
__func__, ret);
}

dev_err(dai->dev, "%s: %s: done\n", __func__, dai->name);
}

static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
Expand Down Expand Up @@ -1320,6 +1389,8 @@ static int intel_suspend(struct device *dev)
return 0;
}

dev_err(dev, "%s start\n", __func__);

if (pm_runtime_status_suspended(dev)) {
dev_dbg(dev,
"%s: pm_runtime status: suspended\n",
Expand Down Expand Up @@ -1348,6 +1419,8 @@ static int intel_suspend(struct device *dev)

intel_shim_wake(sdw, false);

dev_err(dev, "%s done\n", __func__);

return 0;
}

Expand All @@ -1363,6 +1436,8 @@ static int intel_suspend_runtime(struct device *dev)
return 0;
}

dev_err(dev, "%s start\n", __func__);

ret = sdw_cdns_enable_interrupt(cdns, false);
if (ret < 0) {
dev_err(dev, "cannot disable interrupts on suspend\n");
Expand All @@ -1377,6 +1452,8 @@ static int intel_suspend_runtime(struct device *dev)

intel_shim_wake(sdw, false);

dev_err(dev, "%s done\n", __func__);

return 0;
}

Expand All @@ -1393,6 +1470,8 @@ static int intel_resume(struct device *dev)
return 0;
}

dev_err(dev, "%s start\n", __func__);

if (md->pm_runtime_suspended) {
dev_dbg(dev,
"%s: pm_runtime status was suspended, forcing active\n",
Expand Down Expand Up @@ -1425,6 +1504,8 @@ static int intel_resume(struct device *dev)
return ret;
}

dev_err(dev, "%s done\n", __func__);

return ret;
}

Expand All @@ -1440,6 +1521,8 @@ static int intel_resume_runtime(struct device *dev)
return 0;
}

dev_err(dev, "%s start\n", __func__);

ret = intel_init(sdw);
if (ret) {
dev_err(dev, "%s failed: %d", __func__, ret);
Expand All @@ -1461,6 +1544,8 @@ static int intel_resume_runtime(struct device *dev)
return ret;
}

dev_err(dev, "%s done\n", __func__);

return ret;
}

Expand Down
Loading