From 5408d714c55d063a4885d46823c82b4c1af33580 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 31 Aug 2021 08:50:07 -0500 Subject: [PATCH 1/8] ASOC: SOF: Intel: use snd_soc_dai_get_widget() We have a helper, use it to simplify widget lookup Suggested-by: Peter Ujfalusi Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-dai.c | 20 ++++---------------- sound/soc/sof/intel/hda.c | 10 ++-------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index d71165fb6fad38..d3541a2b0c11eb 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -233,10 +233,7 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, hda_stream = hstream_to_sof_hda_stream(link_dev); - 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_link_dai_widget_update(hda_stream, w, stream_tag - 1, true); @@ -330,10 +327,7 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream, break; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - 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 DAI widget during stop/suspend to keep widget use_count's balanced. @@ -385,10 +379,7 @@ static int hda_link_hw_free(struct snd_pcm_substream *substream, hda_stream = hstream_to_sof_hda_stream(link_dev); - 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_link_dai_widget_update(hda_stream, w, DMA_CHAN_INVALID, false); @@ -437,10 +428,7 @@ static int ssp_dai_setup_or_free(struct snd_pcm_substream *substream, struct snd struct sof_ipc_fw_version *v; struct snd_sof_dev *sdev; - 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); swidget = w->dobj.private; component = swidget->scomp; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index fe656d29f772a1..06870004c70fac 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -186,10 +186,7 @@ static int sdw_params_stream(struct device *dev, struct snd_soc_dai *d = params_data->dai; struct snd_soc_dapm_widget *w; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - w = d->playback_widget; - else - w = d->capture_widget; + w = snd_soc_dai_get_widget(d, substream->stream); return sdw_dai_config_ipc(sdev, w, params_data->link_id, params_data->alh_stream_id, d->id, true); @@ -203,10 +200,7 @@ static int sdw_free_stream(struct device *dev, struct snd_soc_dai *d = free_data->dai; struct snd_soc_dapm_widget *w; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - w = d->playback_widget; - else - w = d->capture_widget; + w = snd_soc_dai_get_widget(d, substream->stream); /* send invalid stream_id */ return sdw_dai_config_ipc(sdev, w, free_data->link_id, 0xFFFF, d->id, false); From 8bd8cf9774a7ea5318ee5940394b221cec4dc49c Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 30 Aug 2021 21:53:03 -0500 Subject: [PATCH 2/8] ASoC: SOF: Intel: hda-dai: remove support for TRIGGER_RESUME TRIGGER_RESUME is not supported on Intel platforms, let's remove untested/dead code. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-dai.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index d3541a2b0c11eb..58ce7940f8d3e5 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -311,16 +311,6 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream, dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd); switch (cmd) { - case SNDRV_PCM_TRIGGER_RESUME: - /* set up hw_params */ - ret = hda_link_pcm_prepare(substream, dai); - if (ret < 0) { - dev_err(dai->dev, - "error: setting up hw_params during resume\n"); - return ret; - } - - fallthrough; case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: snd_hdac_ext_link_stream_start(link_dev); From b37e57ccdba56f4d9981bd0fa391b21fddb8d0c7 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 30 Aug 2021 21:41:35 -0500 Subject: [PATCH 3/8] ASOC: SOF: Intel: hda-dai: consistent naming for HDA DAI and HDA link DMA 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 --- sound/soc/sof/intel/hda-dai.c | 63 +++++++++++++++++------------------ 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 58ce7940f8d3e5..5e1bbe95c199ef 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -57,8 +57,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_dma_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; @@ -183,9 +183,9 @@ static struct sof_ipc_dai_config *hda_dai_update_config(struct snd_soc_dapm_widg return config; } -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_dev *sdev = hda_stream->sdev; struct sof_ipc_dai_config *config; @@ -203,9 +203,9 @@ static int hda_link_dai_widget_update(struct sof_intel_hda_stream *hda_stream, return hda_ctrl_dai_widget_free(w); } -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; @@ -222,7 +222,7 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, /* get stored dma data if resuming from system suspend */ link_dev = snd_soc_dai_get_dma_data(dai, substream); if (!link_dev) { - link_dev = hda_link_stream_assign(bus, substream); + link_dev = hda_link_dma_stream_assign(bus, substream); if (!link_dev) return -EBUSY; @@ -236,7 +236,7 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, 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_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; @@ -266,8 +266,8 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream, return hda_link_dma_params(link_dev, &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 *link_dev = snd_soc_dai_get_dma_data(dai, substream); @@ -281,12 +281,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, - dai); + return hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, dai); } -static int hda_link_pcm_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int hda_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) { struct hdac_ext_stream *link_dev = snd_soc_dai_get_dma_data(dai, substream); @@ -322,7 +321,7 @@ static int 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; @@ -343,8 +342,8 @@ static int 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 +371,7 @@ static int hda_link_hw_free(struct snd_pcm_substream *substream, w = snd_soc_dai_get_widget(dai, substream->stream); /* 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 +394,11 @@ static int hda_link_hw_free(struct snd_pcm_substream *substream, return 0; } -static const struct snd_soc_dai_ops hda_link_dai_ops = { - .hw_params = hda_link_hw_params, - .hw_free = hda_link_hw_free, - .trigger = hda_link_pcm_trigger, - .prepare = hda_link_pcm_prepare, +static const struct snd_soc_dai_ops hda_dai_ops = { + .hw_params = hda_dai_hw_params, + .hw_free = hda_dai_hw_free, + .trigger = hda_dai_trigger, + .prepare = hda_dai_prepare, }; #endif @@ -621,7 +620,7 @@ struct snd_soc_dai_driver skl_dai[] = { #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) { .name = "iDisp1 Pin", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -629,7 +628,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp2 Pin", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -637,7 +636,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp3 Pin", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -645,7 +644,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "iDisp4 Pin", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 8, @@ -653,7 +652,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Analog CPU DAI", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, @@ -665,7 +664,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Digital CPU DAI", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, @@ -677,7 +676,7 @@ struct snd_soc_dai_driver skl_dai[] = { }, { .name = "Alt Analog CPU DAI", - .ops = &hda_link_dai_ops, + .ops = &hda_dai_ops, .playback = { .channels_min = 1, .channels_max = 16, From e9d2b5b60b15d61331a99ac7d5fa6279a5c0d636 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 30 Aug 2021 22:36:56 -0500 Subject: [PATCH 4/8] ASoC: SOF: Intel: hda-dai: simplify hda_dai_widget_update() prototype We don't need to pass a hda_stream to find the sdev to find a device for dev_dbg. Just pass the device. While we're at it, also update error log Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-dai.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 5e1bbe95c199ef..68faa24b156160 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -183,16 +183,15 @@ static struct sof_ipc_dai_config *hda_dai_update_config(struct snd_soc_dapm_widg return config; } -static int hda_dai_widget_update(struct sof_intel_hda_stream *hda_stream, +static int hda_dai_widget_update(struct device *dev, struct snd_soc_dapm_widget *w, int channel, bool widget_setup) { - struct snd_sof_dev *sdev = hda_stream->sdev; struct sof_ipc_dai_config *config; config = hda_dai_update_config(w, channel); if (!config) { - dev_err(sdev->dev, "error: no config for DAI %s\n", w->name); + dev_err(dev, "%s: no config for DAI %s\n", __func__, w->name); return -ENOENT; } @@ -236,7 +235,7 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, 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(hda_stream, w, stream_tag - 1, true); + ret = hda_dai_widget_update(dai->dev, w, stream_tag - 1, true); if (ret < 0) return ret; @@ -321,7 +320,7 @@ static int 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(dai->dev, w, DMA_CHAN_INVALID, false); if (ret < 0) return ret; @@ -371,7 +370,7 @@ static int hda_dai_hw_free(struct snd_pcm_substream *substream, 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(hda_stream, w, DMA_CHAN_INVALID, false); + ret = hda_dai_widget_update(dai->dev, w, DMA_CHAN_INVALID, false); if (ret < 0) return ret; From 17377ac711514102c602fe573980ee9ebe356b4c Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 30 Aug 2021 22:33:18 -0500 Subject: [PATCH 5/8] ASoC: SOF: Intel: hda-dai: split dailink and dai operations First split the dailink and dai operations in separate functions, that will still be called from the dai callbacks. In a follow-up step these dailink routines will be moved and invoked from the dailink callbacks. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-dai.c | 213 +++++++++++++++++++++------------- 1 file changed, 134 insertions(+), 79 deletions(-) diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 68faa24b156160..5bbe0d62b58e79 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -202,43 +202,31 @@ static int hda_dai_widget_update(struct device *dev, return hda_ctrl_dai_widget_free(w); } -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_dai_link_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 *link_dev; 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_ext_stream *link_dev; 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 */ - link_dev = snd_soc_dai_get_dma_data(dai, substream); + link_dev = snd_soc_dai_get_dma_data(cpu_dai, substream); if (!link_dev) { link_dev = hda_link_dma_stream_assign(bus, substream); if (!link_dev) return -EBUSY; - snd_soc_dai_set_dma_data(dai, substream, (void *)link_dev); + snd_soc_dai_set_dma_data(cpu_dai, substream, (void *)link_dev); } stream_tag = hdac_stream(link_dev)->stream_tag; - hda_stream = hstream_to_sof_hda_stream(link_dev); - - 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(dai->dev, 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; @@ -265,49 +253,88 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, return hda_link_dma_params(link_dev, &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 *link_dev; + struct snd_soc_dapm_widget *w; + int stream_tag; + + link_dev = snd_soc_dai_get_dma_data(dai, substream); + if (!link_dev) + return -EINVAL; + + stream_tag = hdac_stream(link_dev)->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(dai->dev, 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_dai_link_hw_params(substream, params); + if (ret < 0) + return ret; + + return hda_dai_hw_params_update(substream, params, dai); +} + +static int hda_dai_link_prepare(struct snd_pcm_substream *substream) { - struct hdac_ext_stream *link_dev = - 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); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct hdac_ext_stream *link_dev = snd_soc_dai_get_dma_data(cpu_dai, substream); + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(cpu_dai->component); int stream = substream->stream; if (link_dev->link_prepared) return 0; - dev_dbg(sdev->dev, "hda: prepare stream dir %d\n", substream->stream); + dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); - return hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, dai); + return hda_dai_link_hw_params(substream, &rtd->dpcm[stream].hw_params); } -static int hda_dai_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int hda_dai_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) { - struct hdac_ext_stream *link_dev = - 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; - struct hdac_stream *hstream; - struct hdac_bus *bus; - int stream_tag; + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(dai->component); + int stream = substream->stream; int ret; - hstream = substream->runtime->private_data; - bus = hstream->bus; - rtd = asoc_substream_to_rtd(substream); + dev_dbg(sdev->dev, "%s: prepare stream dir %d\n", __func__, substream->stream); - link = snd_hdac_ext_bus_get_link(bus, asoc_rtd_to_codec(rtd, 0)->component->name); + ret = hda_dai_link_prepare(substream); + if (ret < 0) + return ret; + + return hda_dai_hw_params_update(substream, &rtd->dpcm[stream].hw_params, dai); +} + +static int hda_dai_link_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 *link_dev = 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; - hda_stream = hstream_to_sof_hda_stream(link_dev); - - dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd); + dev_dbg(cpu_dai->dev, "%s: cmd=%d\n", __func__, cmd); switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: @@ -315,15 +342,6 @@ static int hda_dai_trigger(struct snd_pcm_substream *substream, break; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - w = snd_soc_dai_get_widget(dai, substream->stream); - - /* - * free DAI widget during stop/suspend to keep widget use_count's balanced. - */ - ret = hda_dai_widget_update(dai->dev, w, DMA_CHAN_INVALID, false); - if (ret < 0) - return ret; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { stream_tag = hdac_stream(link_dev)->stream_tag; snd_hdac_ext_link_clear_stream_id(link, stream_tag); @@ -341,40 +359,56 @@ static int 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 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 *link_dev; struct snd_soc_dapm_widget *w; int ret; - hstream = substream->runtime->private_data; - bus = hstream->bus; - rtd = asoc_substream_to_rtd(substream); - link_dev = snd_soc_dai_get_dma_data(dai, substream); + ret = hda_dai_link_trigger(substream, cmd); + if (ret < 0) + return ret; + + dev_dbg(dai->dev, "%s: cmd=%d\n", __func__, cmd); + switch (cmd) { + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_STOP: + w = snd_soc_dai_get_widget(dai, substream->stream); + + /* + * free DAI widget during stop/suspend to keep widget use_count's balanced. + */ + ret = hda_dai_widget_update(dai->dev, w, DMA_CHAN_INVALID, false); + if (ret < 0) + return ret; + + break; + default: + break; + } + return 0; +} + +static int hda_dai_link_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 *link_dev; + struct hdac_ext_link *link; + int stream_tag; + link_dev = snd_soc_dai_get_dma_data(cpu_dai, substream); if (!link_dev) { - dev_dbg(dai->dev, + dev_dbg(cpu_dai->dev, "%s: link_dev is not assigned\n", __func__); return -EINVAL; } - hda_stream = hstream_to_sof_hda_stream(link_dev); - - 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(dai->dev, 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; @@ -383,16 +417,37 @@ 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(link_dev, HDAC_EXT_STREAM_TYPE_LINK); link_dev->link_prepared = 0; /* free the host DMA channel reserved by hostless streams */ + hda_stream = hstream_to_sof_hda_stream(link_dev); 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_dai_link_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(dai->dev, w, DMA_CHAN_INVALID, false); + if (ret < 0) + return ret; + + return 0; +} + static const struct snd_soc_dai_ops hda_dai_ops = { .hw_params = hda_dai_hw_params, .hw_free = hda_dai_hw_free, From 0c2166d839e1af8be1ec2ea99acb52baf18f0daa Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 31 Aug 2021 14:59:56 -0500 Subject: [PATCH 6/8] [HACK] ASoC: add traces to show BE fixup is called 3 times Traces captured on UpExtreme: root@plb-UP-WHL01:~# speaker-test -Dhw:0,0 -c2 -r48000 -t sine -l1 The BE fixup is called three times, one in the BE hw_params, once for the codec dai and once for the cpu dai. WHY? [ 23.948549] Analog Playback and Capture: dpcm_be_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948567] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948574] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948578] Analog Playback and Capture: dpcm_be_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948587] snd_hda_codec_realtek ehdaudio0D0: snd_soc_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948593] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948598] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948602] snd_hda_codec_realtek ehdaudio0D0: snd_soc_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948607] snd_hda_codec_realtek ehdaudio0D0: plb: hdac_hda_dai_hw_params: start dai Analog Codec DAI [ 23.948614] snd_hda_codec_realtek ehdaudio0D0: plb: hdac_hda_dai_hw_params: done Analog Codec DAI [ 23.948622] sof-audio-pci-intel-cnl 0000:00:1f.3: snd_soc_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948627] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948631] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948636] sof-audio-pci-intel-cnl 0000:00:1f.3: snd_soc_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948640] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: hda_dai_hw_params: start dai Analog CPU DAI [ 23.949507] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: hda_dai_hw_params: done dai Analog CPU DAI Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/hdac_hda.c | 5 +++++ sound/soc/soc-dai.c | 3 +++ sound/soc/soc-pcm.c | 2 ++ sound/soc/sof/intel/hda-dai.c | 8 +++++++- sound/soc/sof/pcm.c | 4 ++++ 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 390dd6c7f6a506..0d1f4eb07c4f1f 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -208,6 +208,8 @@ static int hdac_hda_dai_hw_params(struct snd_pcm_substream *substream, unsigned int format_val; unsigned int maxbps; + dev_info(component->dev, "plb: %s: start dai %s\n", __func__, dai->name); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) maxbps = dai->driver->playback.sig_bits; else @@ -229,6 +231,9 @@ static int hdac_hda_dai_hw_params(struct snd_pcm_substream *substream, } hda_pvt->pcm[dai->id].format_val[substream->stream] = format_val; + + dev_info(component->dev, "plb: %s: done %s\n", __func__, dai->name); + return 0; } diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 3db0fcf24385af..720ef550ce1eb3 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -392,10 +392,13 @@ int snd_soc_dai_hw_params(struct snd_soc_dai *dai, if (dai->driver->ops && dai->driver->ops->hw_params) { + dev_info(dai->dev, "%s: plb before snd_soc_link_be_hw_params_fixup\n", __func__); + /* perform any topology hw_params fixups before DAI */ ret = snd_soc_link_be_hw_params_fixup(rtd, params); if (ret < 0) goto end; + dev_info(dai->dev, "%s: plb after snd_soc_link_be_hw_params_fixup\n", __func__); ret = dai->driver->ops->hw_params(substream, params, dai); } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index fc1854e3e43fd8..6fdd081212ec67 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1901,9 +1901,11 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) sizeof(struct snd_pcm_hw_params)); /* perform any hw_params fixups */ + dev_info(be->dev, "%s: plb before snd_soc_link_be_hw_params_fixup\n", __func__); ret = snd_soc_link_be_hw_params_fixup(be, &dpcm->hw_params); if (ret < 0) goto unwind; + dev_info(be->dev, "%s: plb after snd_soc_link_be_hw_params_fixup\n", __func__); /* copy the fixed-up hw params for BE dai */ memcpy(&be->dpcm[stream].hw_params, &dpcm->hw_params, diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 5bbe0d62b58e79..4d46d879d82ac6 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -279,11 +279,17 @@ static int hda_dai_hw_params(struct snd_pcm_substream *substream, { int ret; + dev_info(dai->dev, "plb: %s: start dai %s\n", __func__, dai->name); + ret = hda_dai_link_hw_params(substream, params); if (ret < 0) return ret; - return hda_dai_hw_params_update(substream, params, dai); + ret = hda_dai_hw_params_update(substream, params, dai); + + dev_info(dai->dev, "plb: %s: done dai %s\n", __func__, dai->name); + + return ret; } static int hda_dai_link_prepare(struct snd_pcm_substream *substream) diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c index 56db927390c98b..92020f36d2f55e 100644 --- a/sound/soc/sof/pcm.c +++ b/sound/soc/sof/pcm.c @@ -717,6 +717,8 @@ int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_pa struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component); struct snd_soc_dpcm *dpcm; + dev_info(component->dev, "plb: %s: start\n", __func__); + /* no topology exists for this BE, try a common configuration */ if (!dai) { dev_warn(component->dev, @@ -831,6 +833,8 @@ int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_pa break; } + dev_info(component->dev, "plb: %s: done\n", __func__); + return 0; } EXPORT_SYMBOL(sof_pcm_dai_link_fixup); From 06b62e06ece22dfd7cc97d890a3d75006adeb661 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 30 Aug 2021 13:11:20 -0500 Subject: [PATCH 7/8] ASoC: soc-pcm: enforce order between cpu and codec dais The existing code handles cpu and codec dais inconsistently, sometimes codec-first and sometimes cpu-first. The most puzzling case is hw_params, where the codec dais are handled first, in a reversal of the DAPM graph order. This order is particularly problematic for SoundWire-based solutions, where the master runtime is allocated by the Slave driver, a clear layering violation to work-around the order imposed by soc-pcm.c. This patch enforces a strict cpu-dai first behavior. There is no change besides moving code blocks around. Signed-off-by: Pierre-Louis Bossart --- sound/soc/soc-pcm.c | 55 ++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 6fdd081212ec67..383e2a6517fa5b 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -453,16 +453,6 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream) int i; unsigned int bits = 0, cpu_bits = 0; - for_each_rtd_codec_dais(rtd, i, codec_dai) { - struct snd_soc_pcm_stream *pcm_codec = snd_soc_dai_get_pcm_stream(codec_dai, stream); - - if (pcm_codec->sig_bits == 0) { - bits = 0; - break; - } - bits = max(pcm_codec->sig_bits, bits); - } - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { struct snd_soc_pcm_stream *pcm_cpu = snd_soc_dai_get_pcm_stream(cpu_dai, stream); @@ -472,9 +462,18 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream) } cpu_bits = max(pcm_cpu->sig_bits, cpu_bits); } + soc_pcm_set_msb(substream, cpu_bits); + + for_each_rtd_codec_dais(rtd, i, codec_dai) { + struct snd_soc_pcm_stream *pcm_codec = snd_soc_dai_get_pcm_stream(codec_dai, stream); + if (pcm_codec->sig_bits == 0) { + bits = 0; + break; + } + bits = max(pcm_codec->sig_bits, bits); + } soc_pcm_set_msb(substream, bits); - soc_pcm_set_msb(substream, cpu_bits); } static void soc_pcm_hw_init(struct snd_pcm_hardware *hw) @@ -940,6 +939,23 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, if (ret < 0) goto out; + for_each_rtd_cpu_dais(rtd, i, cpu_dai) { + /* + * Skip CPUs which don't support the current stream + * type. See soc_pcm_init_runtime_hw() for more details + */ + if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) + continue; + + ret = snd_soc_dai_hw_params(cpu_dai, substream, params); + if (ret < 0) + goto out; + + /* store the parameters for each DAI */ + soc_pcm_set_dai_params(cpu_dai, params); + snd_soc_dapm_update_dai(substream, params, cpu_dai); + } + for_each_rtd_codec_dais(rtd, i, codec_dai) { struct snd_pcm_hw_params codec_params; @@ -983,23 +999,6 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, snd_soc_dapm_update_dai(substream, &codec_params, codec_dai); } - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - /* - * Skip CPUs which don't support the current stream - * type. See soc_pcm_init_runtime_hw() for more details - */ - if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) - continue; - - ret = snd_soc_dai_hw_params(cpu_dai, substream, params); - if (ret < 0) - goto out; - - /* store the parameters for each DAI */ - soc_pcm_set_dai_params(cpu_dai, params); - snd_soc_dapm_update_dai(substream, params, cpu_dai); - } - ret = snd_soc_pcm_component_hw_params(substream, params); out: mutex_unlock(&rtd->card->pcm_mutex); From e3a32ae5cfaf69acebca2d9fdcd4b45b1cb8d9da Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 31 Aug 2021 15:18:37 -0500 Subject: [PATCH 8/8] [HACK] ASoC: soc-pcm: codec/cpu dai order is due to set_tdm_mask() use For HDaudio we store the stream_tag as the tx_mask for the codec_dai. When the cpu_dai hw_params is done, we set the tdm mask as the stream tag. But since the codec_dai hw_params is done first, this does not interfere with codec_mask_fixup. When we revert and do the cpu_dai hw_params first, then we end-up using the stream tag in soc_pcm_codec_params_fixup(). This makes of course no sense, we end-up using garbage channel masks and there's no audio. The following trages were captured on TGL, one can clearly see that we use a linear set of tags, but we treat them as masks. That's just wrong. We need to stop overloading definitions and use a real stream_tag, as done for SoundWire. [ 17.792152] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 1, ignored [ 17.793842] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 2, ignored [ 17.795153] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 3, ignored [ 17.819472] snd_hda_codec_realtek ehdaudio0D0: soc_pcm_hw_params: plb: stream tag is 4, ignored [ 39.037775] snd_hda_codec_realtek ehdaudio0D0: soc_pcm_hw_params: plb: stream tag is 1, ignored Signed-off-by: Pierre-Louis Bossart --- sound/soc/soc-pcm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 383e2a6517fa5b..9145a18690b250 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -981,9 +981,11 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, /* fixup params based on TDM slot masks */ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && - codec_dai->tx_mask) - soc_pcm_codec_params_fixup(&codec_params, - codec_dai->tx_mask); + codec_dai->tx_mask) { + dev_info(codec_dai->dev, "%s: plb: stream tag is %d, ignored\n", __func__, codec_dai->tx_mask); + //soc_pcm_codec_params_fixup(&codec_params, + // codec_dai->tx_mask); + } if (substream->stream == SNDRV_PCM_STREAM_CAPTURE && codec_dai->rx_mask)