From 8e84b6a4e7f188638748d2ac0455a94799530aa1 Mon Sep 17 00:00:00 2001 From: Ranjani Sridharan Date: Thu, 21 Apr 2022 15:31:48 -0500 Subject: [PATCH 01/14] ASoC: SOF: remove incorrect clearing of prepared flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the system is suspended while a PCM is paused, it doesn't receive the SUSPEND trigger. So, the SOF driver has to ensure that the PCM and the widgets associated with the paused PCM are freed in the firmware during suspend. This is handled in the sof_tear_down_left_over_pipelines() call. But since the state of this PCM is SUSPENDED, we end up clearing the prepared flag for the PCM before freeing it. This results in IPC errors while freeing the widgets. But because the widget use_counts are reset to 0 even though the IPC fails, releasing the paused stream after resuming from suspend proceeds normally. Fix the IPC errors by removing the clearing of the prepared flag in sof_set_hw_params_upon_resume(). In fact, we can remove the sof_set_hw_params_upon_resume() and call snd_sof_dsp_hw_params_upon_resume() directly. This will ensure that the PCM is freed in the firmware before the IPC's for freeing the widgets are sent. BugLink: https://github.com/thesofproject/linux/issues/3543 Reviewed-by: Bard Liao Reviewed-by: Péter Ujfalusi Signed-off-by: Ranjani Sridharan Signed-off-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220421203201.1550328-2-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/pm.c | 2 +- sound/soc/sof/sof-audio.c | 36 ------------------------------------ sound/soc/sof/sof-audio.h | 1 - 3 files changed, 1 insertion(+), 38 deletions(-) diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c index 44008dd075c2..fa3f5514c00f 100644 --- a/sound/soc/sof/pm.c +++ b/sound/soc/sof/pm.c @@ -194,7 +194,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend) /* prepare for streams to be resumed properly upon resume */ if (!runtime_suspend) { - ret = sof_set_hw_params_upon_resume(sdev->dev); + ret = snd_sof_dsp_hw_params_upon_resume(sdev); if (ret < 0) { dev_err(sdev->dev, "error: setting hw_params flag during suspend %d\n", diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c index e2ec60887568..7ecc84f9872b 100644 --- a/sound/soc/sof/sof-audio.c +++ b/sound/soc/sof/sof-audio.c @@ -413,42 +413,6 @@ bool snd_sof_stream_suspend_ignored(struct snd_sof_dev *sdev) return false; } -int sof_set_hw_params_upon_resume(struct device *dev) -{ - struct snd_sof_dev *sdev = dev_get_drvdata(dev); - struct snd_pcm_substream *substream; - struct snd_sof_pcm *spcm; - snd_pcm_state_t state; - int dir; - - /* - * SOF requires hw_params to be set-up internally upon resume. - * So, set the flag to indicate this for those streams that - * have been suspended. - */ - list_for_each_entry(spcm, &sdev->pcm_list, list) { - for_each_pcm_streams(dir) { - /* - * do not reset hw_params upon resume for streams that - * were kept running during suspend - */ - if (spcm->stream[dir].suspend_ignored) - continue; - - substream = spcm->stream[dir].substream; - if (!substream || !substream->runtime) - continue; - - state = substream->runtime->status->state; - if (state == SNDRV_PCM_STATE_SUSPENDED) - spcm->prepared[dir] = false; - } - } - - /* set internal flag for BE */ - return snd_sof_dsp_hw_params_upon_resume(sdev); -} - int sof_pcm_stream_free(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream, struct snd_sof_pcm *spcm, int dir, bool free_widget_list) { diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h index a0af7c421fd9..f36c4f62bc99 100644 --- a/sound/soc/sof/sof-audio.h +++ b/sound/soc/sof/sof-audio.h @@ -434,7 +434,6 @@ static inline void snd_sof_compr_init_elapsed_work(struct work_struct *work) { } int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params); /* PM */ -int sof_set_hw_params_upon_resume(struct device *dev); bool snd_sof_stream_suspend_ignored(struct snd_sof_dev *sdev); bool snd_sof_dsp_only_d0i3_compatible_stream_active(struct snd_sof_dev *sdev); From 51ec71dc0cc90e6683ebda7f5ea0ddb71265ab23 Mon Sep 17 00:00:00 2001 From: Ranjani Sridharan Date: Thu, 21 Apr 2022 15:31:49 -0500 Subject: [PATCH 02/14] ASoC: SOF: Intel: Add IPC-specific dai ops for IPC3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Bard Liao Reviewed-by: Péter Ujfalusi Signed-off-by: Ranjani Sridharan Signed-off-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220421203201.1550328-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 cb499f3905ce..b7839fd04dfb 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 f5bac91c335b..98c4e4f61e7c 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 f9cb9f1f0237..8891077d8d8c 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 6e05c7759480..f520d1cf70c9 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 f845064c3589..f19517dffd62 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 816571305f24..ed76f736afb4 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); From 309e6e557482415c32338b118f0eb17600d98060 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:50 -0500 Subject: [PATCH 03/14] ASOC: SOF: Intel: hda-dai: consistent naming for HDA DAI and HDA link DMA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Intel documentation refers to the concepts of 'HDAudio host DMA' (system memory <--> DSP) and 'HDaudio link DMA' (DSP <--> peripherals). We currently use the prefix 'hda_link' to describe DAI operations, which can be confused for dailink operations. Since the topology tokens refer unambiguously to the 'HDA' dai, let's drop the link prefix for dai-related ops/callbacks. Conversely let's use 'hda_link_dma' for routines related to the DMA management. In a follow-up patch we will introduce the 'hda_dai_link' prefix for dailink ops/callbacks. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 52 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 8891077d8d8c..65f3ff519624 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -50,8 +50,8 @@ static bool hda_check_fes(struct snd_soc_pcm_runtime *rtd, } static struct hdac_ext_stream * - hda_link_stream_assign(struct hdac_bus *bus, - struct snd_pcm_substream *substream) +hda_link_stream_assign(struct hdac_bus *bus, + struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct sof_intel_hda_stream *hda_stream; @@ -162,9 +162,9 @@ static int hda_link_dma_params(struct hdac_ext_stream *hext_stream, return 0; } -static int hda_link_dai_widget_update(struct sof_intel_hda_stream *hda_stream, - struct snd_soc_dapm_widget *w, - int channel, bool widget_setup) +static int hda_dai_widget_update(struct sof_intel_hda_stream *hda_stream, + struct snd_soc_dapm_widget *w, + int channel, bool widget_setup) { struct snd_sof_dai_config_data data; @@ -177,9 +177,9 @@ static int hda_link_dai_widget_update(struct sof_intel_hda_stream *hda_stream, return hda_ctrl_dai_widget_free(w, SOF_DAI_CONFIG_FLAGS_NONE, &data); } -static int hda_link_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_dai *dai) +static int hda_dai_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) { struct hdac_stream *hstream = substream->runtime->private_data; struct hdac_bus *bus = hstream->bus; @@ -213,7 +213,7 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, w = dai->capture_widget; /* set up the DAI widget and send the DAI_CONFIG with the new tag */ - ret = hda_link_dai_widget_update(hda_stream, w, stream_tag - 1, true); + ret = hda_dai_widget_update(hda_stream, w, stream_tag - 1, true); if (ret < 0) return ret; @@ -239,8 +239,8 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, return hda_link_dma_params(hext_stream, &p_params); } -static int hda_link_pcm_prepare(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int 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); @@ -254,11 +254,11 @@ static int hda_link_pcm_prepare(struct snd_pcm_substream *substream, dev_dbg(sdev->dev, "hda: prepare stream dir %d\n", substream->stream); - return hda_link_hw_params(substream, &rtd->dpcm[stream].hw_params, + return hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, dai); } -static int hda_link_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) +static int hda_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) { struct snd_sof_widget *swidget = w->dobj.private; struct snd_soc_component *component = swidget->scomp; @@ -276,8 +276,8 @@ static int hda_link_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) return ret; } -static int ipc3_hda_link_pcm_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int ipc3_hda_dai_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); @@ -316,7 +316,7 @@ static int ipc3_hda_link_pcm_trigger(struct snd_pcm_substream *substream, /* * free DAI widget during stop/suspend to keep widget use_count's balanced. */ - ret = hda_link_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); + ret = hda_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); if (ret < 0) return ret; @@ -330,7 +330,7 @@ static int ipc3_hda_link_pcm_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_PAUSE_PUSH: snd_hdac_ext_link_stream_clear(hext_stream); - ret = hda_link_dai_config_pause_push_ipc(w); + ret = hda_dai_config_pause_push_ipc(w); if (ret < 0) return ret; break; @@ -340,8 +340,8 @@ static int ipc3_hda_link_pcm_trigger(struct snd_pcm_substream *substream, return 0; } -static int hda_link_hw_free(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int hda_dai_hw_free(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) { unsigned int stream_tag; struct sof_intel_hda_stream *hda_stream; @@ -372,7 +372,7 @@ static int hda_link_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_link_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); + ret = hda_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); if (ret < 0) return ret; @@ -395,11 +395,11 @@ static int hda_link_hw_free(struct snd_pcm_substream *substream, return 0; } -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 = ipc3_hda_link_pcm_trigger, - .prepare = hda_link_pcm_prepare, +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, }; #endif @@ -531,7 +531,7 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) 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; + ops->drv[i].ops = &ipc3_hda_dai_ops; #endif } break; From b44c99f11de2c9f01b5526e668c476de976fd14d Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:51 -0500 Subject: [PATCH 04/14] ASoC: SOF: Intel: hda-dai: simplify hda_dai_widget_update() prototype MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the argument "struct sof_intel_hda_stream *hda_stream" is not used, remove. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-5-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 65f3ff519624..3113f61ae737 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; From 5ef85c9e42e5fc549e934669fdca352b3da97ec4 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:52 -0500 Subject: [PATCH 05/14] ASoC: SOF: Intel: hda-dai: use snd_soc_dai_get_widget() helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use helper instead of open-coding the same thing multiple times. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 3113f61ae737..245009809894 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -203,10 +203,7 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, stream_tag = hdac_stream(hext_stream)->stream_tag; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - w = dai->playback_widget; - else - w = dai->capture_widget; + 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); @@ -359,10 +356,7 @@ static int hda_dai_hw_free(struct snd_pcm_substream *substream, 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; + 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); @@ -407,10 +401,7 @@ static int ssp_dai_setup_or_free(struct snd_pcm_substream *substream, struct snd { struct snd_soc_dapm_widget *w; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - w = dai->playback_widget; - else - w = dai->capture_widget; + w = snd_soc_dai_get_widget(dai, substream->stream); if (setup) return hda_ctrl_dai_widget_setup(w, SOF_DAI_CONFIG_FLAGS_NONE, NULL); From f321ffc8d93639181af0512938e2b0630ca28051 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:53 -0500 Subject: [PATCH 06/14] ASoC: SOF: Intel: hda-dai: split link DMA and dai operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-7-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 222 +++++++++++++++++++++------------- 1 file changed, 139 insertions(+), 83 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 245009809894..d5ca5b1fefe6 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; + struct snd_soc_dapm_widget *w; + int stream_tag; + + hext_stream = snd_soc_dai_get_dma_data(dai, substream); + if (!hext_stream) + return -EINVAL; + + 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 */ + 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 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; - if (hext_stream->link_prepared) - return 0; - - dev_dbg(sdev->dev, "hda: prepare stream dir %d\n", substream->stream); - - return hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, - dai); + 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); + dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); + + ret = hda_link_dma_prepare(substream); + if (ret < 0) + return ret; + + 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(dai->dev, "In %s cmd=%d\n", __func__, cmd); - - w = snd_soc_dai_get_widget(dai, substream->stream); - + 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,9 +336,6 @@ 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); - ret = hda_dai_config_pause_push_ipc(w); - if (ret < 0) - return ret; break; default: return -EINVAL; @@ -330,40 +343,62 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, return 0; } -static int hda_dai_hw_free(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) { - unsigned int stream_tag; - 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_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); + 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: + break; + } + return 0; +} + +static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) +{ + 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 = hstream->bus; + struct hdac_ext_stream *hext_stream; + 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 From 9272d6c2af6427df8d7fe665ede6a1bf97d0ca8c Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:54 -0500 Subject: [PATCH 07/14] ASoC: SOF: Intel: hda-dai: regroup dai and link DMA operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just code move with no functionality change, to clearly separate out the 'dai' operation from the link DMA ones. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-8-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 253 +++++++++++++++++----------------- 1 file changed, 127 insertions(+), 126 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index d5ca5b1fefe6..20eb4097ce75 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -162,20 +162,6 @@ static int hda_link_dma_params(struct hdac_ext_stream *hext_stream, return 0; } -static int hda_dai_widget_update(struct snd_soc_dapm_widget *w, - int channel, bool widget_setup) -{ - struct snd_sof_dai_config_data data; - - data.dai_data = channel; - - /* set up/free DAI widget and send DAI_CONFIG IPC */ - if (widget_setup) - return hda_ctrl_dai_widget_setup(w, SOF_DAI_CONFIG_FLAGS_2_STEP_STOP, &data); - - return hda_ctrl_dai_widget_free(w, SOF_DAI_CONFIG_FLAGS_NONE, &data); -} - static int hda_link_dma_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -220,39 +206,6 @@ static int hda_link_dma_hw_params(struct snd_pcm_substream *substream, return hda_link_dma_params(hext_stream, &p_params); } -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; - struct snd_soc_dapm_widget *w; - int stream_tag; - - hext_stream = snd_soc_dai_get_dma_data(dai, substream); - if (!hext_stream) - return -EINVAL; - - 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 */ - 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); @@ -261,46 +214,6 @@ static int hda_link_dma_prepare(struct snd_pcm_substream *substream) 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) -{ - struct snd_sof_widget *swidget = w->dobj.private; - struct snd_soc_component *component = swidget->scomp; - struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component); - const struct sof_ipc_tplg_ops *tplg_ops = sdev->ipc->ops->tplg; - int ret = 0; - - if (tplg_ops->dai_config) { - ret = tplg_ops->dai_config(sdev, swidget, SOF_DAI_CONFIG_FLAGS_PAUSE, NULL); - if (ret < 0) - dev_err(sdev->dev, "%s: DAI config failed for widget %s\n", __func__, - w->name); - } - - return ret; -} - -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_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; - - if (hext_stream->link_prepared) - return 0; - - dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); - - ret = hda_link_dma_prepare(substream); - if (ret < 0) - return ret; - - 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; @@ -343,6 +256,133 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) return 0; } +static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) +{ + 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 = hstream->bus; + struct hdac_ext_stream *hext_stream; + struct hdac_ext_link *link; + int stream_tag; + + hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); + if (!hext_stream) { + dev_dbg(cpu_dai->dev, "%s: hext_stream is not assigned\n", __func__); + return -EINVAL; + } + + link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); + if (!link) + return -EINVAL; + + 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); + } + + 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_widget_update(struct snd_soc_dapm_widget *w, + int channel, bool widget_setup) +{ + struct snd_sof_dai_config_data data; + + data.dai_data = channel; + + /* set up/free DAI widget and send DAI_CONFIG IPC */ + if (widget_setup) + return hda_ctrl_dai_widget_setup(w, SOF_DAI_CONFIG_FLAGS_2_STEP_STOP, &data); + + return hda_ctrl_dai_widget_free(w, SOF_DAI_CONFIG_FLAGS_NONE, &data); +} + +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; + struct snd_soc_dapm_widget *w; + int stream_tag; + + hext_stream = snd_soc_dai_get_dma_data(dai, substream); + if (!hext_stream) + return -EINVAL; + + 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 */ + 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_dai_config_pause_push_ipc(struct snd_soc_dapm_widget *w) +{ + struct snd_sof_widget *swidget = w->dobj.private; + struct snd_soc_component *component = swidget->scomp; + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component); + const struct sof_ipc_tplg_ops *tplg_ops = sdev->ipc->ops->tplg; + int ret = 0; + + if (tplg_ops->dai_config) { + ret = tplg_ops->dai_config(sdev, swidget, SOF_DAI_CONFIG_FLAGS_PAUSE, NULL); + if (ret < 0) + dev_err(sdev->dev, "%s: DAI config failed for widget %s\n", __func__, + w->name); + } + + return ret; +} + +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_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; + + if (hext_stream->link_prepared) + return 0; + + dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); + + ret = hda_link_dma_prepare(substream); + if (ret < 0) + return ret; + + return hda_dai_hw_params_update(substream, &rtd->dpcm[stream].hw_params, dai); +} + + static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { @@ -379,45 +419,6 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, return 0; } -static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) -{ - 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 = hstream->bus; - struct hdac_ext_stream *hext_stream; - struct hdac_ext_link *link; - int stream_tag; - - hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); - if (!hext_stream) { - dev_dbg(cpu_dai->dev, - "%s: hext_stream is not assigned\n", __func__); - return -EINVAL; - } - - link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); - if (!link) - return -EINVAL; - - 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); - } - - 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) { From d1c73a213b462058e91654b5d1d493b3003375cd Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:55 -0500 Subject: [PATCH 08/14] ASoC: SOF: sof-audio: flag errors on pipeline teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-9-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 572bcbfdb356..3d00d371fbf3 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; } From 81622503229943363858cd7ae1330f49b131dfbc Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:56 -0500 Subject: [PATCH 09/14] ASOC: SOF: Intel: hda-dai: add hda_dai_hw_free_ipc() helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do the same thing from different places, let's use a helper. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-10-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 20eb4097ce75..0521cb755a8a 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -382,6 +382,16 @@ static int ipc3_hda_dai_prepare(struct snd_pcm_substream *substream, return hda_dai_hw_params_update(substream, &rtd->dpcm[stream].hw_params, dai); } +static int hda_dai_hw_free_ipc(int stream, /* direction */ + struct snd_soc_dai *dai) +{ + struct snd_soc_dapm_widget *w; + + w = snd_soc_dai_get_widget(dai, stream); + + /* free the link DMA channel in the FW and the DAI widget */ + return hda_dai_widget_update(w, DMA_CHAN_INVALID, false); +} static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) @@ -402,7 +412,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(w, DMA_CHAN_INVALID, false); + ret = hda_dai_hw_free_ipc(substream->stream, dai); if (ret < 0) return ret; @@ -422,21 +432,13 @@ static int ipc3_hda_dai_trigger(struct snd_pcm_substream *substream, 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; + return hda_dai_hw_free_ipc(substream->stream, dai); } static const struct snd_soc_dai_ops ipc3_hda_dai_ops = { From f09e92844eabd6a65feab0c548a7cf6741cfa39d Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:57 -0500 Subject: [PATCH 10/14] ASoC: SOF: Intel: hda-dai: move code to deal with hda dai/dailink suspend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-11-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 0521cb755a8a..c1ff7145745b 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 ad11df345be7..c068a3f2f6df 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; + int ret; - /* set internal flag for BE */ - list_for_each_entry(s, &bus->stream_list, list) { - hext_stream = stream_to_hdac_ext_stream(s); + /* 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__); - /* - * 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); - } - } -#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 f520d1cf70c9..e52cade75617 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. From 23b1944e46ab4cd7cbd4ef999f814fc6c6f2eb88 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:58 -0500 Subject: [PATCH 11/14] ASoC: SOF: Intel: hda-dai: improve suspend case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add comments and re-align with the TRIGGER_SUSPEND case with an additional call to hda_dai_hw_free_ipc() to free-up resources. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-12-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index c1ff7145745b..dbccd75defe8 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -456,6 +456,7 @@ static int hda_dai_suspend(struct hdac_bus *bus) struct hdac_stream *s; const char *name; int stream_tag; + int ret; /* set internal flag for BE */ list_for_each_entry(s, &bus->stream_list, list) { @@ -468,20 +469,32 @@ static int hda_dai_suspend(struct hdac_bus *bus) * explicitly during suspend. */ if (hext_stream->link_substream) { + struct snd_soc_dai *cpu_dai; + struct snd_soc_dai *codec_dai; + rtd = asoc_substream_to_rtd(hext_stream->link_substream); - name = asoc_rtd_to_codec(rtd, 0)->component->name; + cpu_dai = asoc_rtd_to_cpu(rtd, 0); + codec_dai = asoc_rtd_to_codec(rtd, 0); + name = codec_dai->component->name; link = snd_hdac_ext_bus_get_link(bus, name); if (!link) return -EINVAL; + /* + * we don't need to call snd_hdac_ext_link_stream_clear(he_stream) + * since we can only reach this case in the pause_push state, and + * the TRIGGER_PAUSE_PUSH already stops the DMA + */ + if (hdac_stream(hext_stream)->direction == SNDRV_PCM_STREAM_PLAYBACK) { + stream_tag = hdac_stream(hext_stream)->stream_tag; + snd_hdac_ext_link_clear_stream_id(link, stream_tag); + } 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); + /* 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; } } From 722cbbfaed2a290b1de1fb0ec4ee9a15ec240f7c Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:31:59 -0500 Subject: [PATCH 12/14] ASoC: SOF: Intel: hda-dai: reset dma_data and release stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Ranjani Sridharan Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-13-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 dbccd75defe8..644f75081edd 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; From 880924cad12e96092364467cb7b3ad7a689bec55 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:32:00 -0500 Subject: [PATCH 13/14] ASoC: SOF: Intel: add helper for link DMA cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do the same operations from different places, add a helper to enforce consistency and make the programming sequences clearer. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-14-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/soc/sof/intel/hda-dai.c | 112 ++++++++++++++-------------------- 1 file changed, 45 insertions(+), 67 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 644f75081edd..53600c6c2911 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -128,6 +128,40 @@ hda_link_stream_assign(struct hdac_bus *bus, return res; } +static int hda_link_dma_cleanup(struct snd_pcm_substream *substream, + struct hdac_stream *hstream, + struct snd_soc_dai *cpu_dai, + struct snd_soc_dai *codec_dai, + bool trigger_suspend_stop) +{ + struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); + struct hdac_bus *bus = hstream->bus; + struct sof_intel_hda_stream *hda_stream; + struct hdac_ext_link *link; + int stream_tag; + + link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); + if (!link) + return -EINVAL; + + if (trigger_suspend_stop) + snd_hdac_ext_link_stream_clear(hext_stream); + + 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); + } + 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_link_dma_params(struct hdac_ext_stream *hext_stream, struct hda_pipe_params *params) { @@ -221,13 +255,7 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) 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; + int ret; dev_dbg(cpu_dai->dev, "%s: cmd=%d\n", __func__, cmd); if (!hext_stream) @@ -240,15 +268,9 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) break; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - snd_hdac_ext_link_stream_clear(hext_stream); - - 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); - } - 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; + ret = hda_link_dma_cleanup(substream, hstream, cpu_dai, codec_dai, true); + if (ret < 0) + return ret; break; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: @@ -267,36 +289,13 @@ static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) 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 = hstream->bus; struct hdac_ext_stream *hext_stream; - struct hdac_ext_link *link; - int stream_tag; hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); - if (!hext_stream) { - dev_dbg(cpu_dai->dev, "%s: hext_stream is not assigned\n", __func__); - return -EINVAL; - } + if (!hext_stream) + return 0; - link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name); - if (!link) - return -EINVAL; - - 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); - } - - 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; + return hda_link_dma_cleanup(substream, hstream, cpu_dai, codec_dai, false); } static int hda_dai_widget_update(struct snd_soc_dapm_widget *w, @@ -457,15 +456,11 @@ 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; int ret; /* 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); @@ -482,34 +477,17 @@ static int hda_dai_suspend(struct hdac_bus *bus) rtd = asoc_substream_to_rtd(hext_stream->link_substream); cpu_dai = asoc_rtd_to_cpu(rtd, 0); codec_dai = asoc_rtd_to_codec(rtd, 0); - name = codec_dai->component->name; - link = snd_hdac_ext_bus_get_link(bus, name); - if (!link) - return -EINVAL; - /* - * we don't need to call snd_hdac_ext_link_stream_clear(he_stream) - * since we can only reach this case in the pause_push state, and - * the TRIGGER_PAUSE_PUSH already stops the DMA - */ - if (hdac_stream(hext_stream)->direction == SNDRV_PCM_STREAM_PLAYBACK) { - 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; + ret = hda_link_dma_cleanup(hext_stream->link_substream, s, + cpu_dai, codec_dai, false); + if (ret < 0) + return ret; /* 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; From c4eb48f7739fc0dae7e6b8319a77261fc1b61d74 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Thu, 21 Apr 2022 15:32:01 -0500 Subject: [PATCH 14/14] ASoC: SOF: Intel: hda-dai: protect hw_params against successive calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Rander Wang Reviewed-by: Ranjani Sridharan Reviewed-by: Péter Ujfalusi Reviewed-by: Bard Liao Link: https://lore.kernel.org/r/20220421203201.1550328-15-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 53600c6c2911..9823230d2ef4 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;