Skip to content

[TEST v2] fix suspend/resume with SoundWire #1320

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

Closed
plbossart wants to merge 4 commits into from
Closed

[TEST v2] fix suspend/resume with SoundWire #1320

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
4e8d12e
soundwire: stream: update state machine and add state checks
plbossart Oct 12, 2019
97eb428
soundwire: stream: do not update parameters during DISABLED-PREPARED …
plbossart Oct 15, 2019
4c0ed40
soundwire: intel: suspend-resume transition in .prepare()
bardliao Oct 14, 2019
abb4b39
[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
55 changes: 40 additions & 15 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
+----------+ +-----+------+ +-+--+-----+
| 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 Down Expand Up @@ -276,7 +284,7 @@ stream. From ASoC DPCM framework, this stream state is linked to

.. 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 +340,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 +369,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
3 changes: 3 additions & 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,8 @@ struct sdw_cdns_stream_config {
* @bus: Bus handle
* @stream_type: Stream type
* @link_id: Master link id
* @suspend_reinitialize: status flag indicating PDI needs to be reinitialized
* after a TRIGGER_SUSPEND.
*/
struct sdw_cdns_dma_data {
char *name;
Expand All @@ -89,6 +91,7 @@ struct sdw_cdns_dma_data {
struct sdw_bus *bus;
enum sdw_stream_type stream_type;
int link_id;
bool suspend_reinitialize;
};

/**
Expand Down
100 changes: 96 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 All @@ -810,11 +820,15 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
goto error;
}

dma->suspend_reinitialize = false;

/* do run-time configurations for SHIM, ALH and PDI/PORT */
intel_pdi_shim_configure(sdw, pdi);
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 +865,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 +890,40 @@ static int intel_prepare(struct snd_pcm_substream *substream,
return -EIO;
}

return sdw_prepare_stream(dma->stream);
if (dma->suspend_reinitialize) {
/*
* .prepare() is called when dealing with underflows or
* after system resume. In the latter case, we need to
* re-initialize the ALH settings, which is not needed
* in the underflow case
*/

/* 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;

dma->suspend_reinitialize = false;
}

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 +932,28 @@ 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);
dma->suspend_reinitialize = true;

@bardliao bardliao Oct 16, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart set suspend_reinitialize = true here may not be the best place. The reason we need to reinitialize cdns is when system is suspended. So I think it is better to set suspend_reinitialize = true in intel_suspend(). We may need bardliao@d0247d3

@plbossart plbossart Oct 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we can set suspend_reinitialize in intel_suspend. The reason is that we need to reprogram the ALH stream ID, and if you have more than one DAI for a given device then with your patch you would only restore the first ALH stream ID. It's got to be done on a DAI-basis, so the flag has to be stored in the dma structure.

@plbossart plbossart Oct 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, you are right @bardliao that if the DAI is prepared but not triggered, it still needs to be re-initialized. I guess the solution is a mix of yours and mine, we can't use TRIGGER suspend but can't use the .suspend either since each DAI needs to be reinitialized.

/* fallthrough */
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_STOP:
ret = sdw_disable_stream(dma->stream);
break;
Expand All @@ -904,6 +967,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 +981,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 +1006,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 +1022,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 +1039,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 +1396,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 +1426,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 +1443,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 +1459,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 +1477,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 +1511,8 @@ static int intel_resume(struct device *dev)
return ret;
}

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

return ret;
}

Expand All @@ -1440,6 +1528,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 +1551,8 @@ static int intel_resume_runtime(struct device *dev)
return ret;
}

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

return ret;
}

Expand Down
Loading