[PATCH 0/5] ASoC: Intel: Skylake: Topology and shutdown fixes

[PATCH 0/5] ASoC: Intel: Skylake: Topology and shutdown fixes

[Cezary Rojewski]

Even though skylake-driver is going to be replaced by the avs-driver,
the goal is to keep it functional on all the configurations it supports
until its EOL. When comparing chrome trees against upstream
skylake-driver, couple fixes pop up that are not part of upstream
repository. These fixes are backed up by real bugs (issue trackers),
address real problems. There is no reason for them to stay in the
internal tree.

Except for the last patch, all changes combined together address issue
when the driver updates the presumably static audio format descriptions
coming from the topology files through its "fixup" functions. As long as
given audio format is used by a single path, nothing collides and any
updates are harmless. However, when multiple paths e.g.: DMIC and HDMI1
utilize the same audio format descriptor, any updates caused by the
opening of the first path, may cause the second to fail.

The last change from the set fixes driver hang sporadically occurring
during shutdown procedure. Once HDAudio links are powered down along
with the AudioDSP, the hang stops reproducing.

Cezary Rojewski (5):
  ASoC: Intel: Skylake: Update pipe_config_idx before filling BE params
  ASoC: Intel: Skylake: Remove skl_tplg_is_multi_fmt()
  ASoC: Intel: Skylake: Drop pipe_config_idx
  ASoC: Intel: Skylake: Introduce single place for pipe-config selection
  ASoC: Intel: Skylake: Fix driver hang during shutdown

 sound/soc/intel/skylake/skl-topology.c | 73 +++++++++-----------------
 sound/soc/intel/skylake/skl-topology.h |  1 -
 sound/soc/intel/skylake/skl.c          |  5 +-
 3 files changed, 28 insertions(+), 51 deletions(-)

-- 
2.25.1

[PATCH 1/5] ASoC: Intel: Skylake: Update pipe_config_idx before filling BE params

[Cezary Rojewski]

Without updating the index before BE copier config is filled with
hardware parameters, outdated parameters are used instead.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index e06eac592da1..fc3d719d93e1 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1837,16 +1837,24 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 {
 	struct nhlt_specific_cfg *cfg;
 	struct skl_pipe *pipe = mconfig->pipe;
+	struct skl_pipe_params save = *pipe->p_params;
 	struct skl_pipe_fmt *pipe_fmt;
 	struct skl_dev *skl = get_skl_ctx(dai->dev);
 	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
 	u8 dev_type = skl_tplg_be_dev_type(mconfig->dev_type);
+	int ret;
 
 	skl_tplg_fill_dma_id(mconfig, params);
 
 	if (link_type == NHLT_LINK_HDA)
 		return 0;
 
+	*pipe->p_params = *params;
+	ret = skl_tplg_get_pipe_config(skl, mconfig);
+	if (ret)
+		goto err;
+
+	dev_dbg(skl->dev, "%s using pipe config: %d\n", __func__, pipe->pipe_config_idx);
 	if (pipe->direction == SNDRV_PCM_STREAM_PLAYBACK)
 		pipe_fmt = &pipe->configs[pipe->pipe_config_idx].out_fmt;
 	else
@@ -1865,10 +1873,15 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 		dev_err(dai->dev, "Blob NULL for id:%d type:%d dirn:%d ch:%d, freq:%d, fmt:%d\n",
 			mconfig->vbus_id, link_type, params->stream,
 			params->ch, params->s_freq, params->s_fmt);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	return 0;
+
+err:
+	*pipe->p_params = save;
+	return ret;
 }
 
 static int skl_tplg_be_set_src_pipe_params(struct snd_soc_dai *dai,
-- 
2.25.1

[PATCH 2/5] ASoC: Intel: Skylake: Remove skl_tplg_is_multi_fmt()

[Cezary Rojewski]

Rather than forcing userspace to select proper format with enumerable
kcontrols, select it ourselves based on provided hw_params.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 40 --------------------------
 1 file changed, 40 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index fc3d719d93e1..f144acae1b44 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -582,38 +582,6 @@ static int skl_tplg_unload_pipe_modules(struct skl_dev *skl,
 	return ret;
 }
 
-static bool skl_tplg_is_multi_fmt(struct skl_dev *skl, struct skl_pipe *pipe)
-{
-	struct skl_pipe_fmt *cur_fmt;
-	struct skl_pipe_fmt *next_fmt;
-	int i;
-
-	if (pipe->nr_cfgs <= 1)
-		return false;
-
-	if (pipe->conn_type != SKL_PIPE_CONN_TYPE_FE)
-		return true;
-
-	for (i = 0; i < pipe->nr_cfgs - 1; i++) {
-		if (pipe->direction == SNDRV_PCM_STREAM_PLAYBACK) {
-			cur_fmt = &pipe->configs[i].out_fmt;
-			next_fmt = &pipe->configs[i + 1].out_fmt;
-		} else {
-			cur_fmt = &pipe->configs[i].in_fmt;
-			next_fmt = &pipe->configs[i + 1].in_fmt;
-		}
-
-		if (!CHECK_HW_PARAMS(cur_fmt->channels, cur_fmt->freq,
-				     cur_fmt->bps,
-				     next_fmt->channels,
-				     next_fmt->freq,
-				     next_fmt->bps))
-			return true;
-	}
-
-	return false;
-}
-
 /*
  * Here, we select pipe format based on the pipe type and pipe
  * direction to determine the current config index for the pipeline.
@@ -636,14 +604,6 @@ skl_tplg_get_pipe_config(struct skl_dev *skl, struct skl_module_cfg *mconfig)
 		return 0;
 	}
 
-	if (skl_tplg_is_multi_fmt(skl, pipe)) {
-		pipe->cur_config_idx = pipe->pipe_config_idx;
-		pipe->memory_pages = pconfig->mem_pages;
-		dev_dbg(skl->dev, "found pipe config idx:%d\n",
-			pipe->cur_config_idx);
-		return 0;
-	}
-
 	if (pipe->conn_type == SKL_PIPE_CONN_TYPE_NONE || pipe->nr_cfgs == 1) {
 		dev_dbg(skl->dev, "No conn_type or just 1 pathcfg, taking 0th for %d\n",
 			pipe->ppl_id);
-- 
2.25.1

[PATCH 3/5] ASoC: Intel: Skylake: Drop pipe_config_idx

[Cezary Rojewski]

Field ->pipe_config_idx duplicates the job of ->cur_config_idx so
remove it.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 10 +++++-----
 sound/soc/intel/skylake/skl-topology.h |  1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index f144acae1b44..567a3b661ce4 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1351,9 +1351,9 @@ static int skl_tplg_multi_config_set_get(struct snd_kcontrol *kcontrol,
 		return -EIO;
 
 	if (is_set)
-		pipe->pipe_config_idx = ucontrol->value.enumerated.item[0];
+		pipe->cur_config_idx = ucontrol->value.enumerated.item[0];
 	else
-		ucontrol->value.enumerated.item[0]  =  pipe->pipe_config_idx;
+		ucontrol->value.enumerated.item[0] = pipe->cur_config_idx;
 
 	return 0;
 }
@@ -1814,11 +1814,11 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 	if (ret)
 		goto err;
 
-	dev_dbg(skl->dev, "%s using pipe config: %d\n", __func__, pipe->pipe_config_idx);
+	dev_dbg(skl->dev, "%s using pipe config: %d\n", __func__, pipe->cur_config_idx);
 	if (pipe->direction == SNDRV_PCM_STREAM_PLAYBACK)
-		pipe_fmt = &pipe->configs[pipe->pipe_config_idx].out_fmt;
+		pipe_fmt = &pipe->configs[pipe->cur_config_idx].out_fmt;
 	else
-		pipe_fmt = &pipe->configs[pipe->pipe_config_idx].in_fmt;
+		pipe_fmt = &pipe->configs[pipe->cur_config_idx].in_fmt;
 
 	/* update the blob based on virtual bus_id*/
 	cfg = intel_nhlt_get_endpoint_blob(dai->dev, skl->nhlt,
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index 017ac0ef324d..6db0fd7bad49 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -324,7 +324,6 @@ struct skl_pipe {
 	struct skl_path_config configs[SKL_MAX_PATH_CONFIGS];
 	struct list_head w_list;
 	bool passthru;
-	u32 pipe_config_idx;
 };
 
 enum skl_module_state {
-- 
2.25.1

[PATCH 4/5] ASoC: Intel: Skylake: Introduce single place for pipe-config selection

[Cezary Rojewski]

Provide a single location for pipe config selection where all fields
that have to be updated whenever ->pipe_config_idx changes can be
updated accordingly.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 567a3b661ce4..b20643b83401 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -582,6 +582,12 @@ static int skl_tplg_unload_pipe_modules(struct skl_dev *skl,
 	return ret;
 }
 
+static void skl_tplg_set_pipe_config_idx(struct skl_pipe *pipe, int idx)
+{
+	pipe->cur_config_idx = idx;
+	pipe->memory_pages = pipe->configs[idx].mem_pages;
+}
+
 /*
  * Here, we select pipe format based on the pipe type and pipe
  * direction to determine the current config index for the pipeline.
@@ -600,16 +606,14 @@ skl_tplg_get_pipe_config(struct skl_dev *skl, struct skl_module_cfg *mconfig)
 	int i;
 
 	if (pipe->nr_cfgs == 0) {
-		pipe->cur_config_idx = 0;
+		skl_tplg_set_pipe_config_idx(pipe, 0);
 		return 0;
 	}
 
 	if (pipe->conn_type == SKL_PIPE_CONN_TYPE_NONE || pipe->nr_cfgs == 1) {
 		dev_dbg(skl->dev, "No conn_type or just 1 pathcfg, taking 0th for %d\n",
 			pipe->ppl_id);
-		pipe->cur_config_idx = 0;
-		pipe->memory_pages = pconfig->mem_pages;
-
+		skl_tplg_set_pipe_config_idx(pipe, 0);
 		return 0;
 	}
 
@@ -628,10 +632,8 @@ skl_tplg_get_pipe_config(struct skl_dev *skl, struct skl_module_cfg *mconfig)
 
 		if (CHECK_HW_PARAMS(params->ch, params->s_freq, params->s_fmt,
 				    fmt->channels, fmt->freq, fmt->bps)) {
-			pipe->cur_config_idx = i;
-			pipe->memory_pages = pconfig->mem_pages;
+			skl_tplg_set_pipe_config_idx(pipe, i);
 			dev_dbg(skl->dev, "Using pipe config: %d\n", i);
-
 			return 0;
 		}
 	}
@@ -1351,7 +1353,7 @@ static int skl_tplg_multi_config_set_get(struct snd_kcontrol *kcontrol,
 		return -EIO;
 
 	if (is_set)
-		pipe->cur_config_idx = ucontrol->value.enumerated.item[0];
+		skl_tplg_set_pipe_config_idx(pipe, ucontrol->value.enumerated.item[0]);
 	else
 		ucontrol->value.enumerated.item[0] = pipe->cur_config_idx;
 
-- 
2.25.1

[PATCH 5/5] ASoC: Intel: Skylake: Fix driver hang during shutdown

[Cezary Rojewski]

AudioDSP cores and HDAudio links need to be turned off on shutdown to
ensure no communication or data transfer occurs during the procedure.

Fixes: c5a76a246989 ("ASoC: Intel: Skylake: Add shutdown callback")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 9bd9f9866898..998bd0232cf1 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1107,7 +1107,10 @@ static void skl_shutdown(struct pci_dev *pci)
 	if (!skl->init_done)
 		return;
 
-	snd_hdac_stop_streams_and_chip(bus);
+	snd_hdac_stop_streams(bus);
+	snd_hdac_ext_bus_link_power_down_all(bus);
+	skl_dsp_sleep(skl->dsp);
+
 	list_for_each_entry(s, &bus->stream_list, list) {
 		stream = stream_to_hdac_ext_stream(s);
 		snd_hdac_ext_stream_decouple(bus, stream, false);
-- 
2.25.1