From: Ranjani Sridharan ranjani.sridharan@linux.intel.com

The BE DAI driver ops involve operations that are IPC-specific. For ex: for the HDA DAI, the trigger op involves sending the DAI_CONFIG IPC to the DSP to stop the DMA for the stop/pause commands. This sequence is different for IPC3 and IPC4. So, make the dai driver ops IPC-specific and set the IPC3-specific ops during the ops_init() callback.

Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/apl.c | 3 ++ sound/soc/sof/intel/cnl.c | 3 ++ sound/soc/sof/intel/hda-dai.c | 53 +++++++++++++++++++++-------------- sound/soc/sof/intel/hda.h | 2 ++ sound/soc/sof/intel/icl.c | 3 ++ sound/soc/sof/intel/tgl.c | 3 ++ 6 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c index cb499f3905cec..b7839fd04dfb7 100644 --- a/sound/soc/sof/intel/apl.c +++ b/sound/soc/sof/intel/apl.c @@ -43,6 +43,9 @@ int sof_apl_ops_init(struct snd_sof_dev *sdev) /* ipc */ sof_apl_ops.send_msg = hda_dsp_ipc_send_msg;

+ /* set DAI driver ops */ + hda_set_dai_drv_ops(sdev, &sof_apl_ops); + /* debug */ sof_apl_ops.debug_map = apl_dsp_debugfs; sof_apl_ops.debug_map_count = ARRAY_SIZE(apl_dsp_debugfs); diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c index f5bac91c335ba..98c4e4f61e7c4 100644 --- a/sound/soc/sof/intel/cnl.c +++ b/sound/soc/sof/intel/cnl.c @@ -261,6 +261,9 @@ int sof_cnl_ops_init(struct snd_sof_dev *sdev) /* ipc */ sof_cnl_ops.send_msg = cnl_ipc_send_msg;

+ /* set DAI driver ops */ + hda_set_dai_drv_ops(sdev, &sof_cnl_ops); + /* debug */ sof_cnl_ops.debug_map = cnl_dsp_debugfs; sof_cnl_ops.debug_map_count = ARRAY_SIZE(cnl_dsp_debugfs); diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index f9cb9f1f0237b..8891077d8d8c3 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -276,8 +276,8 @@ static int hda_link_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) return ret; }

-static int hda_link_pcm_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int ipc3_hda_link_pcm_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) { struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(dai, substream); @@ -395,10 +395,10 @@ static int hda_link_hw_free(struct snd_pcm_substream *substream, return 0; }

-static const struct snd_soc_dai_ops hda_link_dai_ops = { +static const struct snd_soc_dai_ops ipc3_hda_link_dai_ops = { .hw_params = hda_link_hw_params, .hw_free = hda_link_hw_free, - .trigger = hda_link_pcm_trigger, + .trigger = ipc3_hda_link_pcm_trigger, .prepare = hda_link_pcm_prepare, };

@@ -478,8 +478,8 @@ static int ssp_dai_prepare(struct snd_pcm_substream *substream, return ssp_dai_setup(substream, dai, true); }

-static int ssp_dai_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int ipc3_ssp_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) { if (cmd != SNDRV_PCM_TRIGGER_SUSPEND) return 0; @@ -507,15 +507,39 @@ static void ssp_dai_shutdown(struct snd_pcm_substream *substream, kfree(dma_data); }

-static const struct snd_soc_dai_ops ssp_dai_ops = { +static const struct snd_soc_dai_ops ipc3_ssp_dai_ops = { .startup = ssp_dai_startup, .hw_params = ssp_dai_hw_params, .prepare = ssp_dai_prepare, - .trigger = ssp_dai_trigger, + .trigger = ipc3_ssp_dai_trigger, .hw_free = ssp_dai_hw_free, .shutdown = ssp_dai_shutdown, };

+void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) +{ + int i; + + switch (sdev->pdata->ipc_type) { + case SOF_IPC: + for (i = 0; i < ops->num_drv; i++) { + if (strstr(ops->drv[i].name, "SSP")) { + ops->drv[i].ops = &ipc3_ssp_dai_ops; + continue; + } +#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) + if (strstr(ops->drv[i].name, "iDisp") || + strstr(ops->drv[i].name, "Analog") || + strstr(ops->drv[i].name, "Digital")) + ops->drv[i].ops = &ipc3_hda_link_dai_ops; +#endif + } + break; + default: + break; + } +} + /* * common dai driver for skl+ platforms. * some products who use this DAI array only physically have a subset of @@ -524,7 +548,6 @@ static const struct snd_soc_dai_ops ssp_dai_ops = { struct snd_soc_dai_driver skl_dai[] = { { .name = "SSP0 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -536,7 +559,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "SSP1 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -548,7 +570,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "SSP2 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -560,7 +581,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "SSP3 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -572,7 +592,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "SSP4 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -584,7 +603,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "SSP5 Pin", - .ops = &ssp_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -611,7 +629,6 @@ struct snd_soc_dai_driver skl_dai[] = { #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) { .name = "iDisp1 Pin", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -619,7 +636,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp2 Pin", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -627,7 +643,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp3 Pin", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -635,7 +650,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp4 Pin", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -643,7 +657,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Analog CPU DAI", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, @@ -655,7 +668,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Digital CPU DAI", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, @@ -667,7 +679,6 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Alt Analog CPU DAI", - .ops = &hda_link_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 6e05c77594809..f520d1cf70c90 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -761,4 +761,6 @@ int hda_ctrl_dai_widget_free(struct snd_soc_dapm_widget *w, unsigned int quirk_f

extern int sof_hda_position_quirk;

+void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops); + #endif diff --git a/sound/soc/sof/intel/icl.c b/sound/soc/sof/intel/icl.c index f845064c3589a..f19517dffd624 100644 --- a/sound/soc/sof/intel/icl.c +++ b/sound/soc/sof/intel/icl.c @@ -127,6 +127,9 @@ int sof_icl_ops_init(struct snd_sof_dev *sdev) /* dsp core get/put */ sof_icl_ops.core_get = hda_dsp_core_get;

+ /* set DAI driver ops */ + hda_set_dai_drv_ops(sdev, &sof_icl_ops); + return 0; }; EXPORT_SYMBOL_NS(sof_icl_ops_init, SND_SOC_SOF_INTEL_HDA_COMMON); diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c index 816571305f247..ed76f736afb46 100644 --- a/sound/soc/sof/intel/tgl.c +++ b/sound/soc/sof/intel/tgl.c @@ -76,6 +76,9 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev) /* ipc */ sof_tgl_ops.send_msg = cnl_ipc_send_msg;

+ /* set DAI driver ops */ + hda_set_dai_drv_ops(sdev, &sof_tgl_ops); + /* debug */ sof_tgl_ops.debug_map = tgl_dsp_debugfs; sof_tgl_ops.debug_map_count = ARRAY_SIZE(tgl_dsp_debugfs);

the argument "struct sof_intel_hda_stream *hda_stream" is not used, remove.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 65f3ff5196244..3113f61ae7370 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -162,8 +162,7 @@ static int hda_link_dma_params(struct hdac_ext_stream *hext_stream, return 0; }

-static int hda_dai_widget_update(struct sof_intel_hda_stream *hda_stream, - struct snd_soc_dapm_widget *w, +static int hda_dai_widget_update(struct snd_soc_dapm_widget *w, int channel, bool widget_setup) { struct snd_sof_dai_config_data data; @@ -186,7 +185,6 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, struct hdac_ext_stream *hext_stream; struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); - struct sof_intel_hda_stream *hda_stream; struct hda_pipe_params p_params = {0}; struct snd_soc_dapm_widget *w; struct hdac_ext_link *link; @@ -205,15 +203,13 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream,

stream_tag = hdac_stream(hext_stream)->stream_tag;

- hda_stream = hstream_to_sof_hda_stream(hext_stream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) w = dai->playback_widget; else w = dai->capture_widget;

/* set up the DAI widget and send the DAI_CONFIG with the new tag */ - ret = hda_dai_widget_update(hda_stream, w, stream_tag - 1, true); + ret = hda_dai_widget_update(w, stream_tag - 1, true); if (ret < 0) return ret;

@@ -281,7 +277,6 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, { struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(dai, substream); - struct sof_intel_hda_stream *hda_stream; struct snd_soc_pcm_runtime *rtd; struct snd_soc_dapm_widget *w; struct hdac_ext_link *link; @@ -298,8 +293,6 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, if (!link) return -EINVAL;

- hda_stream = hstream_to_sof_hda_stream(hext_stream); - dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd);

w = snd_soc_dai_get_widget(dai, substream->stream); @@ -316,7 +309,7 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, /* * free DAI widget during stop/suspend to keep widget use_count's balanced. */ - ret = hda_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); + ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); if (ret < 0) return ret;

@@ -372,7 +365,7 @@ static int hda_dai_hw_free(struct snd_pcm_substream *substream, w = dai->capture_widget;

/* free the link DMA channel in the FW and the DAI widget */ - ret = hda_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); + ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); if (ret < 0) return ret;

The link DMA state management is handled completely on the host side, while the DAI operations require an IPC. Split the first part in dedicated helpers.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 220 +++++++++++++++++++++------------- 1 file changed, 138 insertions(+), 82 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 245009809894b..d5ca5b1fefe67 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -176,40 +176,28 @@ static int hda_dai_widget_update(struct snd_soc_dapm_widget *w, return hda_ctrl_dai_widget_free(w, SOF_DAI_CONFIG_FLAGS_NONE, &data); }

-static int hda_dai_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_dai *dai) +static int hda_link_dma_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) { struct hdac_stream *hstream = substream->runtime->private_data; - struct hdac_bus *bus = hstream->bus; struct hdac_ext_stream *hext_stream; struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); struct hda_pipe_params p_params = {0}; - struct snd_soc_dapm_widget *w; + struct hdac_bus *bus = hstream->bus; struct hdac_ext_link *link; - int stream_tag; - int ret;

/* get stored dma data if resuming from system suspend */ - hext_stream = snd_soc_dai_get_dma_data(dai, substream); + hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); if (!hext_stream) { hext_stream = hda_link_stream_assign(bus, substream); if (!hext_stream) return -EBUSY;

- snd_soc_dai_set_dma_data(dai, substream, (void *)hext_stream); + snd_soc_dai_set_dma_data(cpu_dai, substream, (void *)hext_stream); }

- stream_tag = hdac_stream(hext_stream)->stream_tag; - - w = snd_soc_dai_get_widget(dai, substream->stream); - - /* set up the DAI widget and send the DAI_CONFIG with the new tag */ - ret = hda_dai_widget_update(w, stream_tag - 1, true); - if (ret < 0) - return ret; - link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); if (!link) return -EINVAL; @@ -232,23 +220,45 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, return hda_link_dma_params(hext_stream, &p_params); }

-static int hda_dai_prepare(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int hda_dai_hw_params_update(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) { - struct hdac_ext_stream *hext_stream = - snd_soc_dai_get_dma_data(dai, substream); - struct snd_sof_dev *sdev = - snd_soc_component_get_drvdata(dai->component); - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); - int stream = substream->stream; + struct hdac_ext_stream *hext_stream; + struct snd_soc_dapm_widget *w; + int stream_tag;

- if (hext_stream->link_prepared) - return 0; + hext_stream = snd_soc_dai_get_dma_data(dai, substream); + if (!hext_stream) + return -EINVAL; + + stream_tag = hdac_stream(hext_stream)->stream_tag;

- dev_dbg(sdev->dev, "hda: prepare stream dir %d\n", substream->stream); + w = snd_soc_dai_get_widget(dai, substream->stream);

- return hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, - dai); + /* set up the DAI widget and send the DAI_CONFIG with the new tag */ + return hda_dai_widget_update(w, stream_tag - 1, true); +} + +static int hda_dai_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + int ret; + + ret = hda_link_dma_hw_params(substream, params); + if (ret < 0) + return ret; + + return hda_dai_hw_params_update(substream, params, dai); +} + +static int hda_link_dma_prepare(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + int stream = substream->stream; + + return hda_link_dma_hw_params(substream, &rtd->dpcm[stream].hw_params); }

static int hda_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) @@ -269,31 +279,44 @@ static int hda_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) return ret; }

-static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int ipc3_hda_dai_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) { struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(dai, substream); - struct snd_soc_pcm_runtime *rtd; - struct snd_soc_dapm_widget *w; - struct hdac_ext_link *link; - struct hdac_stream *hstream; - struct hdac_bus *bus; - int stream_tag; + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(dai->component); + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + int stream = substream->stream; int ret;

- hstream = substream->runtime->private_data; - bus = hstream->bus; - rtd = asoc_substream_to_rtd(substream); + if (hext_stream->link_prepared) + return 0;

- link = snd_hdac_ext_bus_get_link(bus, asoc_rtd_to_codec(rtd, 0)->component->name); - if (!link) - return -EINVAL; + dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream);

- dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd); + ret = hda_link_dma_prepare(substream); + if (ret < 0) + return ret;

- w = snd_soc_dai_get_widget(dai, substream->stream); + return hda_dai_hw_params_update(substream, &rtd->dpcm[stream].hw_params, dai); +} + +static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) +{ + struct hdac_stream *hstream = substream->runtime->private_data; + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); + struct hdac_ext_link *link; + struct hdac_bus *bus = hstream->bus; + int stream_tag; + + link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); + if (!link) + return -EINVAL;

+ dev_dbg(cpu_dai->dev, "%s: cmd=%d\n", __func__, cmd); switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: @@ -303,13 +326,6 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_STOP: snd_hdac_ext_link_stream_clear(hext_stream);

- /* - * free DAI widget during stop/suspend to keep widget use_count's balanced. - */ - ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); - if (ret < 0) - return ret; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { stream_tag = hdac_stream(hext_stream)->stream_tag; snd_hdac_ext_link_clear_stream_id(link, stream_tag); @@ -320,50 +336,69 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_PAUSE_PUSH: snd_hdac_ext_link_stream_clear(hext_stream);

+ break; + default: + return -EINVAL; + } + return 0; +} + +static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) +{ + struct snd_soc_dapm_widget *w; + int ret; + + ret = hda_link_dma_trigger(substream, cmd); + if (ret < 0) + return ret; + + w = snd_soc_dai_get_widget(dai, substream->stream); + + dev_dbg(dai->dev, "%s: cmd=%d\n", __func__, cmd); + switch (cmd) { + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_STOP: + /* + * free DAI widget during stop/suspend to keep widget use_count's balanced. + */ + ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); + if (ret < 0) + return ret; + + break; + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: ret = hda_dai_config_pause_push_ipc(w); if (ret < 0) return ret; break; + default: - return -EINVAL; + break; } return 0; }

-static int hda_dai_hw_free(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) { - unsigned int stream_tag; + struct hdac_stream *hstream = substream->runtime->private_data; + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); struct sof_intel_hda_stream *hda_stream; - struct hdac_bus *bus; - struct hdac_ext_link *link; - struct hdac_stream *hstream; - struct snd_soc_pcm_runtime *rtd; + struct hdac_bus *bus = hstream->bus; struct hdac_ext_stream *hext_stream; - struct snd_soc_dapm_widget *w; - int ret; - - hstream = substream->runtime->private_data; - bus = hstream->bus; - rtd = asoc_substream_to_rtd(substream); - hext_stream = snd_soc_dai_get_dma_data(dai, substream); + struct hdac_ext_link *link; + int stream_tag;

+ hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); if (!hext_stream) { - dev_dbg(dai->dev, + dev_dbg(cpu_dai->dev, "%s: hext_stream is not assigned\n", __func__); return -EINVAL; }

- hda_stream = hstream_to_sof_hda_stream(hext_stream); - - w = snd_soc_dai_get_widget(dai, substream->stream); - - /* free the link DMA channel in the FW and the DAI widget */ - ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); - if (ret < 0) - return ret; - - link = snd_hdac_ext_bus_get_link(bus, asoc_rtd_to_codec(rtd, 0)->component->name); + link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); if (!link) return -EINVAL;

@@ -372,21 +407,42 @@ static int hda_dai_hw_free(struct snd_pcm_substream *substream, snd_hdac_ext_link_clear_stream_id(link, stream_tag); }

- snd_soc_dai_set_dma_data(dai, substream, NULL); + snd_soc_dai_set_dma_data(cpu_dai, substream, NULL); snd_hdac_ext_stream_release(hext_stream, HDAC_EXT_STREAM_TYPE_LINK); hext_stream->link_prepared = 0;

/* free the host DMA channel reserved by hostless streams */ + hda_stream = hstream_to_sof_hda_stream(hext_stream); hda_stream->host_reserved = 0;

return 0; }

+static int hda_dai_hw_free(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_dapm_widget *w; + int ret; + + ret = hda_link_dma_hw_free(substream); + if (ret < 0) + return ret; + + w = snd_soc_dai_get_widget(dai, substream->stream); + + /* free the link DMA channel in the FW and the DAI widget */ + ret = hda_dai_widget_update(w, DMA_CHAN_INVALID, false); + if (ret < 0) + return ret; + + return 0; +} + static const struct snd_soc_dai_ops ipc3_hda_dai_ops = { .hw_params = hda_dai_hw_params, .hw_free = hda_dai_hw_free, .trigger = ipc3_hda_dai_trigger, - .prepare = hda_dai_prepare, + .prepare = ipc3_hda_dai_prepare, };

#endif

Before suspending, walk through all the widgets to make sure all refcounts are zero. If not, the resume will not work and random errors will be reported. Adding this paranoia check will help identify leaks and broken sequences.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/ipc3-topology.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c index 572bcbfdb3566..3d00d371fbf31 100644 --- a/sound/soc/sof/ipc3-topology.c +++ b/sound/soc/sof/ipc3-topology.c @@ -2247,6 +2247,18 @@ static int sof_ipc3_tear_down_all_pipelines(struct snd_sof_dev *sdev, bool verif list_for_each_entry(sroute, &sdev->route_list, list) sroute->setup = false;

+ /* + * before suspending, make sure the refcounts are all zeroed out. There's no way + * to recover at this point but this will help root cause bad sequences leading to + * more issues on resume + */ + list_for_each_entry(swidget, &sdev->widget_list, list) { + if (swidget->use_count != 0) { + dev_err(sdev->dev, "%s: widget %s is still in use: count %d\n", + __func__, swidget->widget->name, swidget->use_count); + } + } + return 0; }

The location of the code was not optimal and prevents us from using helpers, let's move it to hda-dai.c.

No functionality change in this patch.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 58 +++++++++++++++++++++++++++++++++++ sound/soc/sof/intel/hda-dsp.c | 42 ++++--------------------- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 0521cb755a8af..c1ff7145745bc 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -448,6 +448,45 @@ static const struct snd_soc_dai_ops ipc3_hda_dai_ops = { .prepare = ipc3_hda_dai_prepare, };

+static int hda_dai_suspend(struct hdac_bus *bus) +{ + struct snd_soc_pcm_runtime *rtd; + struct hdac_ext_stream *hext_stream; + struct hdac_ext_link *link; + struct hdac_stream *s; + const char *name; + int stream_tag; + + /* set internal flag for BE */ + list_for_each_entry(s, &bus->stream_list, list) { + hext_stream = stream_to_hdac_ext_stream(s); + + /* + * clear stream. This should already be taken care for running + * streams when the SUSPEND trigger is called. But paused + * streams do not get suspended, so this needs to be done + * explicitly during suspend. + */ + if (hext_stream->link_substream) { + rtd = asoc_substream_to_rtd(hext_stream->link_substream); + name = asoc_rtd_to_codec(rtd, 0)->component->name; + link = snd_hdac_ext_bus_get_link(bus, name); + if (!link) + return -EINVAL; + + hext_stream->link_prepared = 0; + + if (hdac_stream(hext_stream)->direction == + SNDRV_PCM_STREAM_CAPTURE) + continue; + + stream_tag = hdac_stream(hext_stream)->stream_tag; + snd_hdac_ext_link_clear_stream_id(link, stream_tag); + } + } + + return 0; +} #endif

/* only one flag used so far to harden hw_params/hw_free/trigger/prepare */ @@ -733,3 +772,22 @@ struct snd_soc_dai_driver skl_dai[] = { }, #endif }; + +int hda_dsp_dais_suspend(struct snd_sof_dev *sdev) +{ + /* + * 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. + */ +#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) + int ret; + + ret = hda_dai_suspend(sof_to_bus(sdev)); + if (ret < 0) + return ret; +#endif + + return 0; +} diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index ad11df345be75..c068a3f2f6df3 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -894,44 +894,14 @@ int hda_dsp_shutdown(struct snd_sof_dev *sdev)

int hda_dsp_set_hw_params_upon_resume(struct snd_sof_dev *sdev) { -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) - struct hdac_bus *bus = sof_to_bus(sdev); - struct snd_soc_pcm_runtime *rtd; - struct hdac_ext_stream *hext_stream; - struct hdac_ext_link *link; - struct hdac_stream *s; - const char *name; - int stream_tag; - - /* set internal flag for BE */ - list_for_each_entry(s, &bus->stream_list, list) { - hext_stream = stream_to_hdac_ext_stream(s); - - /* - * clear stream. This should already be taken care for running - * streams when the SUSPEND trigger is called. But paused - * streams do not get suspended, so this needs to be done - * explicitly during suspend. - */ - if (hext_stream->link_substream) { - rtd = asoc_substream_to_rtd(hext_stream->link_substream); - name = asoc_rtd_to_codec(rtd, 0)->component->name; - link = snd_hdac_ext_bus_get_link(bus, name); - if (!link) - return -EINVAL; - - hext_stream->link_prepared = 0; + int ret;

- if (hdac_stream(hext_stream)->direction == - SNDRV_PCM_STREAM_CAPTURE) - continue; + /* make sure all DAI resources are freed */ + ret = hda_dsp_dais_suspend(sdev); + if (ret < 0) + dev_warn(sdev->dev, "%s: failure in hda_dsp_dais_suspend\n", __func__);

- stream_tag = hdac_stream(hext_stream)->stream_tag; - snd_hdac_ext_link_clear_stream_id(link, stream_tag); - } - } -#endif - return 0; + return ret; }

void hda_dsp_d0i3_work(struct work_struct *work) diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index f520d1cf70c90..e52cade756179 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -697,6 +697,7 @@ static inline bool hda_common_check_sdw_irq(struct snd_sof_dev *sdev)

/* common dai driver */ extern struct snd_soc_dai_driver skl_dai[]; +int hda_dsp_dais_suspend(struct snd_sof_dev *sdev);

/* * Platform Specific HW abstraction Ops.

The sequences are missing a call to snd_soc_dai_set_dma_data() when the stream is cleared, as well as a release of the stream, and tests to avoid pointer dereferences.

This fixes an underflow issue in a corner case with two streams paused before a suspend-resume cycle. After resume, the pause_release of the last stream causes an underflow due to an invalid sequence.

This problem probably existed since the beginning and is only see with prototypes of a 'deep-buffer' capability, which depends on additional ASoC fixes, so there's is no Fixes: tag and no real requirement to backport this patch.

BugLink: https://github.com/thesofproject/linux/issues/3151 Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index dbccd75defe84..644f75081edd3 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -230,6 +230,9 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) return -EINVAL;

dev_dbg(cpu_dai->dev, "%s: cmd=%d\n", __func__, cmd); + if (!hext_stream) + return 0; + switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: @@ -243,8 +246,10 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) stream_tag = hdac_stream(hext_stream)->stream_tag; snd_hdac_ext_link_clear_stream_id(link, stream_tag); } - + snd_soc_dai_set_dma_data(cpu_dai, substream, NULL); + snd_hdac_ext_stream_release(hext_stream, HDAC_EXT_STREAM_TYPE_LINK); hext_stream->link_prepared = 0; + break; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: snd_hdac_ext_link_stream_clear(hext_stream); @@ -370,7 +375,7 @@ static int ipc3_hda_dai_prepare(struct snd_pcm_substream *substream, int stream = substream->stream; int ret;

- if (hext_stream->link_prepared) + if (hext_stream && hext_stream->link_prepared) return 0;

dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); @@ -460,6 +465,8 @@ static int hda_dai_suspend(struct hdac_bus *bus)

/* set internal flag for BE */ list_for_each_entry(s, &bus->stream_list, list) { + struct sof_intel_hda_stream *hda_stream; + hext_stream = stream_to_hdac_ext_stream(s);

/* @@ -489,13 +496,20 @@ static int hda_dai_suspend(struct hdac_bus *bus) stream_tag = hdac_stream(hext_stream)->stream_tag; snd_hdac_ext_link_clear_stream_id(link, stream_tag); } + snd_soc_dai_set_dma_data(cpu_dai, hext_stream->link_substream, NULL); + snd_hdac_ext_stream_release(hext_stream, HDAC_EXT_STREAM_TYPE_LINK); hext_stream->link_prepared = 0;

+ /* free the host DMA channel reserved by hostless streams */ + hda_stream = hstream_to_sof_hda_stream(hext_stream); + hda_stream->host_reserved = 0; + /* for consistency with TRIGGER_SUSPEND we free DAI resources */ ret = hda_dai_hw_free_ipc(hdac_stream(hext_stream)->direction, cpu_dai); if (ret < 0) return ret; } + }

return 0;

Once we've set-up the HDA stream and its format, we currently don't support additional format changes. We already have a protection in the .prepare case, but this needs to be added in the hw_params too.

In mixing use cases where two DPCM FEs are connected to the same BE, if can happen that there are multiple calls to the BE hw_params when the two FEs are configured simultaneously.

This could alternatively be fixed at the DPCM level but that's a more intrusive change requiring infrastructure changes: this would need to be paired with the definition of fixed hw_params at the mixer level.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 53600c6c29116..9823230d2ef4a 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -336,8 +336,13 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) { + struct hdac_ext_stream *hext_stream = + snd_soc_dai_get_dma_data(dai, substream); int ret;

+ if (hext_stream && hext_stream->link_prepared) + return 0; + ret = hda_link_dma_hw_params(substream, params); if (ret < 0) return ret;