Skip to content
2 changes: 2 additions & 0 deletions drivers/soundwire/cadence_master.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ struct sdw_cdns_stream_config {
* @link_id: Master link id
* @hw_params: hw_params to be applied in .prepare step
* @suspended: status set when suspended, to be used in .prepare
* @paused: status set in .trigger, to be used in suspend
*/
struct sdw_cdns_dma_data {
char *name;
Expand All @@ -96,6 +97,7 @@ struct sdw_cdns_dma_data {
int link_id;
struct snd_pcm_hw_params *hw_params;
bool suspended;
bool paused;
};

/**
Expand Down
102 changes: 84 additions & 18 deletions drivers/soundwire/intel.c
Original file line number Diff line number Diff line change
Expand Up @@ -711,15 +711,15 @@ intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
}

static int intel_params_stream(struct sdw_intel *sdw,
struct snd_pcm_substream *substream,
int stream,
struct snd_soc_dai *dai,
struct snd_pcm_hw_params *hw_params,
int link_id, int alh_stream_id)
{
struct sdw_intel_link_res *res = sdw->link_res;
struct sdw_intel_stream_params_data params_data;

params_data.substream = substream;
params_data.stream = stream; /* direction */
params_data.dai = dai;
params_data.hw_params = hw_params;
params_data.link_id = link_id;
Expand All @@ -732,14 +732,14 @@ static int intel_params_stream(struct sdw_intel *sdw,
}

static int intel_free_stream(struct sdw_intel *sdw,
struct snd_pcm_substream *substream,
int stream,
struct snd_soc_dai *dai,
int link_id)
{
struct sdw_intel_link_res *res = sdw->link_res;
struct sdw_intel_stream_free_data free_data;

free_data.substream = substream;
free_data.stream = stream; /* direction */
free_data.dai = dai;
free_data.link_id = link_id;

Expand Down Expand Up @@ -846,6 +846,12 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
if (!dma)
return -EIO;

if (dma->hw_params) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@plbossart can I handle this part in SOF by using the configred flag in

int hda_ctrl_dai_widget_setup(struct snd_soc_dapm_widget *w)

Then we won't have to repeat it for all DAIs

Copy link
Collaborator

Choose a reason for hiding this comment

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

no I think you need this anyway. I've added an extra patch to my PR to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, you lost me here. the patch in your PR seems to address all DAIs, so what I added in the SoundWire-specific part seems like dead code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

but my code wouldnt deal with the PDI stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't get the answer @ranj063. Wouldn't your change result in the SDW DAI hw_free() being called?

I am worried of a possible double-free if you free on the SOF side and I redo it on the SoundWire side - since we are using different variables to decide to do a free...

Copy link
Collaborator

Choose a reason for hiding this comment

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

no @plbossart. My change only deals with the use_counts for the DAI widget. It will call only call hda_ctrl_dai_widget_free().

There won't be a double free because the first call will reset the configured flag for the DAI.

Copy link
Member Author

Choose a reason for hiding this comment

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

See the call graph @ranj063 there will be a double free...

intel_hw_params
  intel_hw_free
    intel_free_stream
      sdw_dai_config_ipc
        hda_ctrl_dai_widget_free

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to revert "ASoC: SOF: handle paused streams during system suspend" now. Otherwise I will see

=== PAUSE ===                                                             Suspended. Trying resume. Failed. Restarting stream. aplay: suspend:1691: suspend: prepare error: Invalid argument
[  880.519173] sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx: 0x30100000: GLB_TPLG_MSG: PIPE_NEW
[  880.519273] sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx error for 0x30100000 (msg/reply size: 48/20): -22
[  880.519275] sof-audio-pci-intel-tgl 0000:00:1f.3: error: failed to load widget PIPELINE.31.ALH258.OUT
[  880.519276] sof-audio-pci-intel-tgl 0000:00:1f.3: error: failed to restore pipeline after resume -22
[  880.519277] PM: dpm_run_callback(): pci_pm_resume+0x0/0x80 returns -22
[  880.519284] sof-audio-pci-intel-tgl 0000:00:1f.3: PM: failed to resume async: error -22

dmesg.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @bardliao there must be something we missed, IIRC you are using firmware 1.8 and we're doing something different. Gah.

/* reconfiguration is currently not supported */
ret = 0;
goto error;
}

ch = params_channels(params);
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
dir = SDW_DATA_DIR_RX;
Expand All @@ -871,12 +877,13 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
sdw_cdns_config_stream(cdns, ch, dir, pdi);

/* store pdi and hw_params, may be needed in prepare step */
dma->paused = false;
dma->suspended = false;
dma->pdi = pdi;
dma->hw_params = params;

/* Inform DSP about PDI stream number */
ret = intel_params_stream(sdw, substream, dai, params,
ret = intel_params_stream(sdw, substream->stream, dai, params,
sdw->instance,
pdi->intel_alh_id);
if (ret)
Expand Down Expand Up @@ -953,7 +960,7 @@ static int intel_prepare(struct snd_pcm_substream *substream,
sdw_cdns_config_stream(cdns, ch, dir, dma->pdi);

/* Inform DSP about PDI stream number */
ret = intel_params_stream(sdw, substream, dai,
ret = intel_params_stream(sdw, substream->stream, dai,
dma->hw_params,
sdw->instance,
dma->pdi->intel_alh_id);
Expand Down Expand Up @@ -987,7 +994,7 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
return ret;
}

ret = intel_free_stream(sdw, substream, dai, sdw->instance);
ret = intel_free_stream(sdw, substream->stream, dai, sdw->instance);
if (ret < 0) {
dev_err(dai->dev, "intel_free_stream: failed %d\n", ret);
return ret;
Expand Down Expand Up @@ -1041,15 +1048,7 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_cdns_dma_data *dma;

/*
* The .prepare callback is used to deal with xruns and resume operations. In the case
* of xruns, the DMAs and SHIM registers cannot be touched, but for resume operations the
* DMAs and SHIM registers need to be initialized.
* the .trigger callback is used to track the suspend case only.
*/
if (cmd != SNDRV_PCM_TRIGGER_SUSPEND)
return 0;
int ret;

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
Expand All @@ -1058,9 +1057,75 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn
return -EIO;
}

dma->suspended = true;
switch (cmd) {
case SNDRV_PCM_TRIGGER_SUSPEND:

return intel_free_stream(sdw, substream, dai, sdw->instance);
/*
* The .prepare callback is used to deal with xruns and resume operations.
* In the case of xruns, the DMAs and SHIM registers cannot be touched,
* but for resume operations the DMAs and SHIM registers need to be initialized.
* the .trigger callback is used to track the suspend case only.
*/

dma->suspended = true;

ret = intel_free_stream(sdw, substream->stream, dai, sdw->instance);
break;

case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
dma->paused = true;
break;
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
dma->paused = false;
break;
default:
break;
}

return ret;
}

static int intel_component_dais_suspend(struct snd_soc_component *component)
{
struct snd_soc_dai *dai;

/*
* In the corner case where a SUSPEND happens during a PAUSE, the ALSA core
* does not throw the TRIGGER_SUSPEND. This leaves the DAIs in an unbalanced state.
* Since the component suspend is called last, we can trap this corner case
* and force the DAIs to release their resources.
*/
for_each_component_dais(component, 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 stream;
int ret;

dma = dai->playback_dma_data;
stream = SNDRV_PCM_STREAM_PLAYBACK;
if (!dma) {
dma = dai->capture_dma_data;
stream = SNDRV_PCM_STREAM_CAPTURE;
}

if (!dma)
continue;

if (dma->suspended)
continue;

if (dma->paused) {
dma->suspended = true;

ret = intel_free_stream(sdw, stream, dai, sdw->instance);
if (ret < 0)
return ret;
}
}

return 0;
}

static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
Expand All @@ -1087,6 +1152,7 @@ static const struct snd_soc_dai_ops intel_pdm_dai_ops = {

static const struct snd_soc_component_driver dai_component = {
.name = "soundwire",
.suspend = intel_component_dais_suspend,
};

static int intel_create_dai(struct sdw_cdns *cdns,
Expand Down
15 changes: 15 additions & 0 deletions drivers/soundwire/stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -1679,6 +1679,11 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)

sdw_acquire_bus_lock(stream);

if (stream->state == SDW_STREAM_ENABLED) {
ret = 0;
goto state_err;
}

if (stream->state != SDW_STREAM_PREPARED &&
stream->state != SDW_STREAM_DISABLED) {
pr_err("%s: %s: inconsistent state state %d\n",
Expand Down Expand Up @@ -1762,6 +1767,11 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)

sdw_acquire_bus_lock(stream);

if (stream->state == SDW_STREAM_DISABLED) {
ret = 0;
goto state_err;
}

if (stream->state != SDW_STREAM_ENABLED) {
pr_err("%s: %s: inconsistent state state %d\n",
__func__, stream->name, stream->state);
Expand Down Expand Up @@ -1837,6 +1847,11 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)

sdw_acquire_bus_lock(stream);

if (stream->state == SDW_STREAM_DEPREPARED) {
ret = 0;
goto state_err;
}

if (stream->state != SDW_STREAM_PREPARED &&
stream->state != SDW_STREAM_DISABLED) {
pr_err("%s: %s: inconsistent state state %d\n",
Expand Down
4 changes: 2 additions & 2 deletions include/linux/soundwire/sdw_intel.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
* firmware.
*/
struct sdw_intel_stream_params_data {
struct snd_pcm_substream *substream;
int stream;
struct snd_soc_dai *dai;
struct snd_pcm_hw_params *hw_params;
int link_id;
Expand All @@ -105,7 +105,7 @@ struct sdw_intel_stream_params_data {
* firmware.
*/
struct sdw_intel_stream_free_data {
struct snd_pcm_substream *substream;
int stream;
struct snd_soc_dai *dai;
int link_id;
};
Expand Down
2 changes: 2 additions & 0 deletions include/sound/soc-dpcm.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ struct snd_soc_dpcm_runtime {
enum snd_soc_dpcm_state state;

int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */

int be_start; /* refcount protected by dpcm_lock */
};

#define for_each_dpcm_fe(be, stream, _dpcm) \
Expand Down
Loading