Skip to content

[TEST] Fix suspend/resume with SoundWire #1314

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] Fix suspend/resume with SoundWire #1314

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
b83dd26
soundwire: stream: fix state machine and add state checks
plbossart Oct 12, 2019
0b4bf13
soundwire: intel: stream deprepare on TRIGGER_STOP/SUSPEND/PAUSE_PUSH
plbossart Oct 12, 2019
57099d4
soundwire: intel: re-initialize ALH PDIs during suspend/resume transi…
bardliao Oct 14, 2019
43c19f4
[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
12 changes: 6 additions & 6 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,12 +156,12 @@ 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 |
+----------+ +------------+ +----------+
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
116 changes: 105 additions & 11 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 @@ -610,6 +612,7 @@ static int intel_params_stream(struct sdw_intel *sdw,
params_data.link_id = link_id;
params_data.alh_stream_id = alh_stream_id;

sdw->hw_params = hw_params;

@RanderWang RanderWang Oct 15, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bardliao why not move "sdw->hw_params = hw_params;" to intel_hw_params, like

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

I know: intel_params_stream is invoked in intel_prepare, but this line of code do nothing for this case.

@bardliao bardliao Oct 15, 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.

@RanderWang Good suggestion. Thanks.

if (res->ops && res->ops->params_stream && res->dev)
return res->ops->params_stream(res->dev,
&params_data);
Expand Down Expand Up @@ -760,6 +763,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 +775,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 +796,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,12 +821,14 @@ 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;
/* Inform DSP about PDI stream number */
ret = intel_params_stream(sdw, substream, dai, params,
sdw->instance,
Expand Down Expand Up @@ -851,14 +864,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,14 +889,48 @@ 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);

dma->suspend_reinitialize = false;
Comment thread
plbossart marked this conversation as resolved.
Resolved
}

ret = sdw_prepare_stream(dma->stream);

@bardliao bardliao Oct 15, 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.

If we call sdw_deprepare_stream() in .hw_free(), intel_prepare() may be called on stream->state = DISABLED. We don't need to call sdw_deprepare_stream(), but we should return 0 in such case.


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

return ret;
}

static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
struct sdw_cdns_dma_data *dma;
int ret;
int err;

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

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
Expand All @@ -883,16 +939,31 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
}

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 15, 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 SNDRV_PCM_TRIGGER_SUSPEND is not always triggered. For example. SNDRV_PCM_TRIGGER_SUSPEND will not be triggered If we do aplay -> pulse -> suspend -> resume -> release. However, we have to reinitialize in this case since the system is suspended. So my suggestion is to set it in intel_suspend().

/* fallthrough */
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_STOP:
ret = sdw_disable_stream(dma->stream);

if (!ret) {
err = sdw_deprepare_stream(dma->stream);

@bardliao bardliao Oct 15, 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.

I still think it is better to call sdw_deprepare_stream() in .hw_free(). I saw intel_prepare() is trigger twice without intel_trigger() when pulseaudio is enabled.

if (err) {
dev_err(dai->dev,
"sdw_deprepare_stream: failed %d", err);
ret = err;
}
}

break;

default:
Expand All @@ -904,6 +975,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,16 +989,12 @@ 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;

ret = sdw_deprepare_stream(dma->stream);
if (ret) {
dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
return ret;
}

ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
if (ret < 0) {
dev_err(dai->dev, "remove master from stream %s failed: %d\n",
Expand All @@ -938,8 +1008,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 +1024,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 +1041,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 +1398,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 +1428,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 +1445,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 +1461,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 +1479,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 +1513,8 @@ static int intel_resume(struct device *dev)
return ret;
}

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

return ret;
}

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

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

return ret;
}

Expand Down
Loading