[alsa-devel] [PATCH v2 0/9] Add DSP topology management for SKL
The SKL driver does not code DSP topology in driver. With this series the ASoC topology framework is used to parse the topology information to create widgets, controls and route map for the FW topology.
It adds routines for SKL DSP module configuration, compute resources for modules, initialize and bind the pipe modules. It uses the SKL IPC library added earlier to send IPC for initialize the module, bind/unbind modules. Last patch in this series removes the unused dais.
changes in v2: - add more explanation in changelog and code - add explanation of MCPS - fix whitespace issues - remove skl widget check on event handler as it is redundant - add switch for depth - add else for direction based code - remove cast for NHLT blob query - make debug prints to error - remove dump info - clean some debug prints
Jeeja KP (9): ASoC: Intel: Skylake: Add pipe and modules handlers ASoC: Intel: Skylake: Add module configuration helpers ASoC: Intel: Skylake: add DSP platform widget event handlers ASoC: Intel: Skylake: Add FE and BE hw_params handling ASoC: Intel: Skylake: Add topology core init and handlers ASoC: Intel: Skylake: Initialize and load DSP controls ASoC: Intel: Skylake: Add DSP support and enable it ASoC: Intel: Skylake: Initialize NHLT table ASoC: Intel: Skylake: Remove unused CPU dai's
sound/soc/intel/skylake/Makefile | 3 +- sound/soc/intel/skylake/skl-pcm.c | 171 ++-- sound/soc/intel/skylake/skl-topology.c | 1262 ++++++++++++++++++++++++++ sound/soc/intel/skylake/skl-topology.h | 21 + sound/soc/intel/skylake/skl-tplg-interface.h | 78 ++ sound/soc/intel/skylake/skl.c | 29 +- sound/soc/intel/skylake/skl.h | 11 + 7 files changed, 1507 insertions(+), 68 deletions(-) create mode 100644 sound/soc/intel/skylake/skl-topology.c
From: Jeeja KP jeeja.kp@intel.com
SKL driver needs to instantiate pipelines and modules in the DSP. The topology in the DSP is modelled as DAPM graph with a PGA representing a module instance and mixer representing a pipeline for a group of modules along with the mixer itself.
Here we start adding building block for handling these. We add resource checks (memory/compute) for pipelines, find the modules in a pipeline, init modules in a pipe and lastly bind/unbind modules in a pipe These will be used by pipe event handlers in subsequent patches
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/Makefile | 3 +- sound/soc/intel/skylake/skl-topology.c | 220 +++++++++++++++++++++++++++++++++ sound/soc/intel/skylake/skl-topology.h | 10 ++ sound/soc/intel/skylake/skl.h | 11 ++ 4 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 sound/soc/intel/skylake/skl-topology.c
diff --git a/sound/soc/intel/skylake/Makefile b/sound/soc/intel/skylake/Makefile index 27db22178204..914b6dab9bea 100644 --- a/sound/soc/intel/skylake/Makefile +++ b/sound/soc/intel/skylake/Makefile @@ -1,4 +1,5 @@ -snd-soc-skl-objs := skl.o skl-pcm.o skl-nhlt.o skl-messages.o +snd-soc-skl-objs := skl.o skl-pcm.o skl-nhlt.o skl-messages.o \ +skl-topology.o
obj-$(CONFIG_SND_SOC_INTEL_SKYLAKE) += snd-soc-skl.o
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c new file mode 100644 index 000000000000..6d0eee6a04ff --- /dev/null +++ b/sound/soc/intel/skylake/skl-topology.c @@ -0,0 +1,220 @@ +/* + * skl-topology.c - Implements Platform component ALSA controls/widget + * handlers. + * + * Copyright (C) 2014-2015 Intel Corp + * Author: Jeeja KP jeeja.kp@intel.com + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as version 2, as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/firmware.h> +#include <sound/soc.h> +#include <sound/soc-topology.h> +#include "skl-sst-dsp.h" +#include "skl-sst-ipc.h" +#include "skl-topology.h" +#include "skl.h" +#include "skl-tplg-interface.h" + +/* + * SKL DSP driver modelling uses only few DAPM widgets so for rest we will + * ignore. This helpers checks if the SKL driver handles this widget type + */ +static int is_skl_dsp_widget_type(struct snd_soc_dapm_widget *w) +{ + switch (w->id) { + case snd_soc_dapm_dai_link: + case snd_soc_dapm_dai_in: + case snd_soc_dapm_aif_in: + case snd_soc_dapm_aif_out: + case snd_soc_dapm_dai_out: + case snd_soc_dapm_switch: + return false; + default: + return true; + } +} + +static int skl_tplg_bind_unbind_pipes(struct skl_module_cfg *src_module, + struct skl_module_cfg *sink_module, struct skl_sst *ctx, bool bind) +{ + int ret; + + if (!bind) { + ret = skl_stop_pipe(ctx, src_module->pipe); + if (ret < 0) + return ret; + + ret = skl_unbind_modules(ctx, src_module, sink_module); + } else { + ret = skl_bind_modules(ctx, src_module, sink_module); + } + + return ret; +} + +/* + * Each pipelines needs memory to be allocated. Check if we have free memory + * from available pool. Then only add this to pool + * This is freed when pipe is deleted + * Note: DSP does actual memory management we only keep track for complete + * pool + */ +static bool skl_tplg_is_pipe_mem_available(struct skl *skl, + struct skl_module_cfg *mconfig) +{ + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: module_id =%d instance=%d\n", __func__, + mconfig->id.module_id, mconfig->id.instance_id); + + if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) { + dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n", + skl->resource.max_mem, skl->resource.mem); + return false; + } + + skl->resource.mem += mconfig->pipe->memory_pages; + return true; +} + +/* + * Pipeline needs needs DSP CPU resources for computation, this is quantified + * in MCPS (Million Clocks Per Second) required for module/pipe + * + * Each pipelines needs mcps to be allocated. Check if we have mcps for this + * pipe. This adds the mcps to driver counter + * This is removed on pipeline delete + */ +static bool skl_tplg_is_pipe_mcps_available(struct skl *skl, + struct skl_module_cfg *mconfig) +{ + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: module_id = %d instance=%d\n", __func__, + mconfig->id.module_id, mconfig->id.instance_id); + + if (skl->resource.mcps + mconfig->mcps > skl->resource.max_mcps) { + dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n", + skl->resource.max_mcps, skl->resource.mcps); + return false; + } + + skl->resource.mcps += mconfig->mcps; + return true; +} + +/* + * A pipe can have multiple modules each of the will be a DAPM widget as + * well. While managing a pipeline we need to get the list of all the + * widgets in a pipelines, so this helper - skl_tplg_get_pipe_widget() helps + * to get the SKL type widgets in that pipeline + */ +static int skl_tplg_get_pipe_widget(struct device *dev, + struct snd_soc_dapm_widget *w, struct skl_pipe *pipe) +{ + struct skl_module_cfg *src_module = NULL; + struct snd_soc_dapm_path *p = NULL; + struct skl_pipe_module *p_module = NULL; + + p_module = devm_kzalloc(dev, sizeof(*p_module), GFP_KERNEL); + if (!p_module) + return -ENOMEM; + + p_module->w = w; + list_add_tail(&p_module->node, &pipe->w_list); + + list_for_each_entry(p, &w->sinks, list_source) { + if ((p->sink->priv == NULL) + && (!is_skl_dsp_widget_type(w))) + continue; + + if ((p->sink->priv != NULL) && (p->connect) + && (is_skl_dsp_widget_type(p->sink))) { + src_module = p->sink->priv; + if (pipe->ppl_id == src_module->pipe->ppl_id) { + dev_dbg(dev, "found widget=%s\n", p->sink->name); + skl_tplg_get_pipe_widget(dev, p->sink, pipe); + } + } + } + return 0; +} + +/* + * Inside a pipe instance, we can have various modules. These modules need + * to instantiated in DSP by invoking INIT_MODULE IPC, which is achieved by + * skl_init_module() routine, so invoke that for all modules in a pipeline + */ +static int skl_tplg_init_pipe_modules(struct skl *skl, struct skl_pipe *pipe) +{ + struct skl_pipe_module *w_module; + struct snd_soc_dapm_widget *w; + struct skl_module_cfg *mconfig; + struct skl_sst *ctx = skl->skl_sst; + int ret = 0; + + dev_dbg(ctx->dev, "%s: pipe=%d\n", __func__, pipe->ppl_id); + list_for_each_entry(w_module, &pipe->w_list, node) { + w = w_module->w; + dev_dbg(ctx->dev, "Pipe Module =%s\n", w->name); + mconfig = w->priv; + + /* check resource available */ + if (!skl_tplg_is_pipe_mcps_available(skl, mconfig)) + return -ENOMEM; + + ret = skl_init_module(ctx, mconfig, NULL); + if (ret < 0) + return ret; + } + + return 0; +} + +/* + * Once all the modules in a pipe are instantiated, they need to be + * connected. + * On removal, before deleting a pipeline the modules need to disconnected. + * + * This is achieved by binding/unbinding these modules + */ +static int skl_tplg_bind_unbind_pipe_modules(struct skl_sst *ctx, + struct skl_pipe *pipe, bool bind) +{ + struct skl_pipe_module *w_module; + struct skl_module_cfg *src_module = NULL; + struct skl_module_cfg *dst_module; + int ret = 0; + + dev_dbg(ctx->dev, "%s: pipe=%d\n", __func__, pipe->ppl_id); + list_for_each_entry(w_module, &pipe->w_list, node) { + dst_module = w_module->w->priv; + + if (src_module == NULL) { + src_module = dst_module; + continue; + } + + if (bind) + ret = skl_bind_modules(ctx, src_module, dst_module); + else + ret = skl_unbind_modules(ctx, src_module, dst_module); + if (ret < 0) + return ret; + + src_module = dst_module; + } + return 0; +} diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h index 8c7767baa94f..73d7916ee33e 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -263,6 +263,16 @@ struct skl_module_cfg { struct skl_specific_cfg formats_config; };
+struct skl_pipeline { + struct skl_pipe *pipe; + struct list_head node; +}; + +struct skl_dapm_path_list { + struct snd_soc_dapm_path *dapm_path; + struct list_head node; +}; + int skl_create_pipeline(struct skl_sst *ctx, struct skl_pipe *pipe);
int skl_run_pipe(struct skl_sst *ctx, struct skl_pipe *pipe); diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index f7fdbb02947f..e980d7897642 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -48,6 +48,13 @@ #define AZX_REG_VS_SDXEFIFOS_XBASE 0x1094 #define AZX_REG_VS_SDXEFIFOS_XINTERVAL 0x20
+struct skl_dsp_resource { + u32 max_mcps; + u32 max_mem; + u32 mcps; + u32 mem; +}; + struct skl { struct hdac_ext_bus ebus; struct pci_dev *pci; @@ -57,6 +64,10 @@ struct skl {
void __iomem *nhlt; /* nhlt ptr */ struct skl_sst *skl_sst; /* sst skl ctx */ + + struct skl_dsp_resource resource; + struct list_head ppl_list; + struct list_head dapm_path_list; };
#define skl_to_ebus(s) (&(s)->ebus)
On Mon, Aug 17, 2015 at 10:56:36PM +0530, Vinod Koul wrote:
+static int skl_tplg_bind_unbind_pipes(struct skl_module_cfg *src_module,
- struct skl_module_cfg *sink_module, struct skl_sst *ctx, bool bind)
+{
- int ret;
- if (!bind) {
ret = skl_stop_pipe(ctx, src_module->pipe);if (ret < 0)return ret;ret = skl_unbind_modules(ctx, src_module, sink_module);- } else {
ret = skl_bind_modules(ctx, src_module, sink_module);- }
- return ret;
+}
Can we *please* stop having these functions which optionally do several different things with nothing at all shared between the different paths? It just adds a layer of indentation in the function and makes everything harder to read (including at the call site - how does the reader know if a given call will bind or unbind?).
I'm sure I've raised this before.
+static bool skl_tplg_is_pipe_mem_available(struct skl *skl,
struct skl_module_cfg *mconfig)
I'm not seeing any users of this function (unlike the mcps checker).
- if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) {
dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",skl->resource.max_mem, skl->resource.mem);return false;- }
I'm not sure how the user is going to be able to tell what exceeded the maximum memory here.
+static bool skl_tplg_is_pipe_mcps_available(struct skl *skl,
struct skl_module_cfg *mconfig)
This looks an awful lot like the memory check. Can we make a struct or ideally array for the constraints and then have a single function which checks them all?
- list_for_each_entry(p, &w->sinks, list_source) {
if ((p->sink->priv == NULL)&& (!is_skl_dsp_widget_type(w)))continue;if ((p->sink->priv != NULL) && (p->connect)&& (is_skl_dsp_widget_type(p->sink))) {
This is harder to read with the extra brackets.
On Sat, Sep 19, 2015 at 09:00:23AM -0700, Mark Brown wrote:
On Mon, Aug 17, 2015 at 10:56:36PM +0530, Vinod Koul wrote:
+static int skl_tplg_bind_unbind_pipes(struct skl_module_cfg *src_module,
- struct skl_module_cfg *sink_module, struct skl_sst *ctx, bool bind)
+{
- int ret;
- if (!bind) {
ret = skl_stop_pipe(ctx, src_module->pipe);if (ret < 0)return ret;ret = skl_unbind_modules(ctx, src_module, sink_module);- } else {
ret = skl_bind_modules(ctx, src_module, sink_module);- }
- return ret;
+}
Can we *please* stop having these functions which optionally do several different things with nothing at all shared between the different paths? It just adds a layer of indentation in the function and makes everything harder to read (including at the call site - how does the reader know if a given call will bind or unbind?).
Well while reading based on the argument bind we would know if we are doing bind or unbind. While binding we call only bind. On unbind we have to stop and then unbind.
I will remove this and few other wrappers like this which will help in readability and reviews
I'm sure I've raised this before.
Sorry to miss that, will fix this time for sure
+static bool skl_tplg_is_pipe_mem_available(struct skl *skl,
struct skl_module_cfg *mconfig)I'm not seeing any users of this function (unlike the mcps checker).
It is called in skl_tplg_mixer_dapm_pre_pmu_event() which is in Patch 3 of this series.
- if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) {
dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",skl->resource.max_mem, skl->resource.mem);return false;- }
I'm not sure how the user is going to be able to tell what exceeded the maximum memory here.
Module Id and Instance Id are printed before this but yes that is debug, so merging the two to error alone makes sense, will do that here
+static bool skl_tplg_is_pipe_mcps_available(struct skl *skl,
struct skl_module_cfg *mconfig)This looks an awful lot like the memory check. Can we make a struct or ideally array for the constraints and then have a single function which checks them all?
No it only two checks, one for MCPS and one for memory. We cannot have single function as they are checked at two places. Due to FW construction. MCPS is property of each module whereas memory is for complete pipe. For each module while initialization we check MCPS whereas for pipe creation we check memory
- list_for_each_entry(p, &w->sinks, list_source) {
if ((p->sink->priv == NULL)&& (!is_skl_dsp_widget_type(w)))continue;if ((p->sink->priv != NULL) && (p->connect)&& (is_skl_dsp_widget_type(p->sink))) {This is harder to read with the extra brackets.
Will fix this
On Mon, Sep 21, 2015 at 09:07:13AM +0530, Vinod Koul wrote:
On Sat, Sep 19, 2015 at 09:00:23AM -0700, Mark Brown wrote:
On Mon, Aug 17, 2015 at 10:56:36PM +0530, Vinod Koul wrote:
- if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) {
dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",skl->resource.max_mem, skl->resource.mem);return false;- }
I'm not sure how the user is going to be able to tell what exceeded the maximum memory here.
Module Id and Instance Id are printed before this but yes that is debug, so merging the two to error alone makes sense, will do that here
They're printed at debug level so might not appear but this is an error message.
From: Jeeja KP jeeja.kp@intel.com
To configure a module, driver needs to send input and output PCM params for a module in DSP. The FE PCM params come from hw_params ie from user, for a BE they also come from hw_params but from BE-link fixups. So based on PCM params required driver has to find a converter module (src/updown/format) and then do the conversion and calculate PCM params in these pipelines In this patch we add the helper modules which allow driver to do these calculations.
Signed-off-by: Hardik T Shah hardik.t.shah@intel.com Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-topology.c | 126 +++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 6d0eee6a04ff..81b0683bbdeb 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -27,6 +27,10 @@ #include "skl.h" #include "skl-tplg-interface.h"
+#define SKL_CH_FIXUP_MASK (1 << 0) +#define SKL_RATE_FIXUP_MASK (1 << 1) +#define SKL_FMT_FIXUP_MASK (1 << 2) + /* * SKL DSP driver modelling uses only few DAPM widgets so for rest we will * ignore. This helpers checks if the SKL driver handles this widget type @@ -115,6 +119,126 @@ static bool skl_tplg_is_pipe_mcps_available(struct skl *skl, return true; }
+static void skl_dump_mconfig(struct skl_sst *ctx, + struct skl_module_cfg *mcfg) +{ + dev_dbg(ctx->dev, "Dumping config\n"); + dev_dbg(ctx->dev, "Input Format:\n"); + dev_dbg(ctx->dev, "channels = %d\n", mcfg->in_fmt.channels); + dev_dbg(ctx->dev, "s_freq = %d\n", mcfg->in_fmt.s_freq); + dev_dbg(ctx->dev, "ch_cfg = %d\n", mcfg->in_fmt.ch_cfg); + dev_dbg(ctx->dev, "valid bit depth = %d\n", mcfg->in_fmt.valid_bit_depth); + dev_dbg(ctx->dev, "Output Format:\n"); + dev_dbg(ctx->dev, "channels = %d\n", mcfg->out_fmt.channels); + dev_dbg(ctx->dev, "s_freq = %d\n", mcfg->out_fmt.s_freq); + dev_dbg(ctx->dev, "valid bit depth = %d\n", mcfg->out_fmt.valid_bit_depth); + dev_dbg(ctx->dev, "ch_cfg = %d\n", mcfg->out_fmt.ch_cfg); +} + +static void skl_tplg_update_params(struct skl_module_fmt *fmt, + struct skl_pipe_params *params, int fixup) +{ + if (fixup & SKL_RATE_FIXUP_MASK) + fmt->s_freq = params->s_freq; + if (fixup & SKL_CH_FIXUP_MASK) + fmt->channels = params->ch; + if (fixup & SKL_FMT_FIXUP_MASK) + fmt->valid_bit_depth = params->s_fmt; +} + +/* + * A pipeline may have modules which impact the pcm parameters, like SRC, + * channel converter, format converter. + * We need to calculate the output params by applying the 'fixup' + * Topology will tell driver which type of fixup is to be applied by + * supplying the fixup mask, so based on that we calculate the output + * + * Now In FE the pcm hw_params is source/target format. Same is applicable + * for BE with its hw_params invoked. + * here based on FE, BE pipeline and direction we calculate the input and + * outfix and then apply that for a module + */ +static void skl_tplg_update_params_fixup(struct skl_module_cfg *m_cfg, + struct skl_pipe_params *params, bool is_fe) +{ + int in_fixup, out_fixup; + struct skl_module_fmt *in_fmt, *out_fmt; + + in_fmt = &m_cfg->in_fmt; + out_fmt = &m_cfg->out_fmt; + + if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if (is_fe) { + in_fixup = m_cfg->params_fixup; + out_fixup = (~m_cfg->converter) & m_cfg->params_fixup; + } else { + out_fixup = m_cfg->params_fixup; + in_fixup = (~m_cfg->converter) & m_cfg->params_fixup; + } + } else { + if (is_fe) { + out_fixup = m_cfg->params_fixup; + in_fixup = (~m_cfg->converter) & m_cfg->params_fixup; + } else { + in_fixup = m_cfg->params_fixup; + out_fixup = (~m_cfg->converter) & m_cfg->params_fixup; + } + } + + skl_tplg_update_params(in_fmt, params, in_fixup); + skl_tplg_update_params(out_fmt, params, out_fixup); +} + +/* + * A module needs input and output buffers, which are dependent upon pcm + * params, so once we have calculate params, we need buffer calculation as + * well. + */ +static void skl_tplg_update_buffer_size(struct skl_sst *ctx, + struct skl_module_cfg *mcfg) +{ + int multiplier = 1; + + if (mcfg->m_type == SKL_MODULE_TYPE_SRCINT) + multiplier = 5; + + mcfg->ibs = (mcfg->in_fmt.s_freq / 1000) * + (mcfg->in_fmt.channels) * + (mcfg->in_fmt.bit_depth >> 3) * + multiplier; + + mcfg->obs = (mcfg->out_fmt.s_freq / 1000) * + (mcfg->out_fmt.channels) * + (mcfg->out_fmt.bit_depth >> 3) * + multiplier; +} + +static void skl_tplg_update_module_params(struct snd_soc_dapm_widget *w, + struct skl_sst *ctx) +{ + struct skl_module_cfg *m_cfg = w->priv; + struct skl_pipe_params *params = m_cfg->pipe->p_params; + int p_conn_type = m_cfg->pipe->conn_type; + bool is_fe; + + if (!m_cfg->params_fixup) + return; + + dev_dbg(ctx->dev, "Mconfig for widget=%s BEFORE updation\n", w->name); + skl_dump_mconfig(ctx, m_cfg); + + if (p_conn_type == SKL_PIPE_CONN_TYPE_FE) + is_fe = true; + else + is_fe = false; + + skl_tplg_update_params_fixup(m_cfg, params, is_fe); + skl_tplg_update_buffer_size(ctx, m_cfg); + + dev_dbg(ctx->dev, "Mconfig for widget=%s AFTER updation\n", w->name); + skl_dump_mconfig(ctx, m_cfg); +} + /* * A pipe can have multiple modules each of the will be a DAPM widget as * well. While managing a pipeline we need to get the list of all the @@ -175,6 +299,8 @@ static int skl_tplg_init_pipe_modules(struct skl *skl, struct skl_pipe *pipe) if (!skl_tplg_is_pipe_mcps_available(skl, mconfig)) return -ENOMEM;
+ /* apply fix/conversion to module params based on FE/BE params*/ + skl_tplg_update_module_params(w, ctx); ret = skl_init_module(ctx, mconfig, NULL); if (ret < 0) return ret;
From: Jeeja KP jeeja.kp@intel.com
The Skylake driver topology model tries to model the firmware rule for pipeline and module creation. The creation rule is: - Create Pipe - Add modules to Pipe - Connect the modules (bind) - Start the pipes
Similarly destroy rule is: - Stop the pipe - Disconnect it (unbind) - Delete the pipe
In driver we use Mixer, as there will always be ONE mixer in a pipeline to model a pipe. The modules in pipe are modelled as PGA widgets. The DAPM sequencing rules (mixer and then PGA) are used to create the sequence DSP expects as depicted above, and then widget handlers for PMU and PMD events help in that.
This patch adds widget event handlers for PRE/POST PMU and PRE/POST PMD event for mixer and pga modules. These event handlers invoke pipeline creation, destroy, module creation, module bind, unbind and pipeline bind unbind
Event handler sequencing is implement to target the DSP FW sequence expectations to enable path from source to sink pipe for Playback/Capture.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Hardik T Shah hardik.t.shah@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-topology.c | 416 +++++++++++++++++++++++++++++++++ 1 file changed, 416 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 81b0683bbdeb..184697188a96 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -344,3 +344,419 @@ static int skl_tplg_bind_unbind_pipe_modules(struct skl_sst *ctx, } return 0; } + +/* + * Mixer module represents a pipeline. So in the Pre-PMU event of mixer we + * need create the pipeline. So we do following: + * - check the resources + * - Create the pipeline + * - Initialize the modules in pipeline + * - finally bind all modules together + */ + +static int skl_tplg_mixer_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + int ret; + struct skl_module_cfg *mconfig = w->priv; + struct skl_pipe *s_pipe = mconfig->pipe; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name); + + /* check resource available */ + if (!skl_tplg_is_pipe_mcps_available(skl, mconfig)) + return -EBUSY; + + if (!skl_tplg_is_pipe_mem_available(skl, mconfig)) + return -ENOMEM; + + /* + * Create list of modules for pipe + * list contains modules from source to sink + */ + ret = skl_create_pipeline(ctx, mconfig->pipe); + if (ret < 0) + return ret; + + if (list_empty(&s_pipe->w_list)) { + ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe); + if (ret < 0) + return ret; + } + + /* Init all pipe modules from source to sink */ + ret = skl_tplg_init_pipe_modules(skl, s_pipe); + if (ret < 0) + return ret; + + /* Bind modules from source to sink */ + return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true); +} +/* + * A PGA represents a module in a pipeline. So in the Pre-PMU event of PGA + * we need to do following: + * - Bind to sink pipeline + * Since the sink pipes can be running and we don't get mixer event on + * connect for already running mixer, we need to find the sink pipes + * here and bind to them. This way dynamic connect works. + * - Start sink pipeline, if not running + * - Then run current pipe + */ +static int skl_tplg_pga_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_path *p; + struct skl_dapm_path_list *path_list; + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + struct skl_sst *ctx = skl->skl_sst; + int ret = 0; + + dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name); + + source = w; + src_mconfig = source->priv; + + /* + * find which sink it is connected to, bind with the sink, + * if sink is not started, start sink pipe first, then start + * this pipe + */ + list_for_each_entry(p, &w->sinks, list_source) { + if (!p->connect) + continue; + + dev_dbg(ctx->dev, "%s: src widget=%s\n", __func__, w->name); + dev_dbg(ctx->dev, "%s: sink widget=%s\n", __func__, p->sink->name); + + /* + * here we will check widgets in sink pipelines, so that can + * be any widgets type and we are only interested if they are + * ones used for SKL so check that first + */ + if ((p->sink->priv != NULL) && is_skl_dsp_widget_type(p->sink)) { + + sink = p->sink; + src_mconfig = source->priv; + sink_mconfig = sink->priv; + + /* Bind source to sink, mixin is always source */ + ret = skl_tplg_bind_unbind_pipes(src_mconfig, + sink_mconfig, ctx, true); + if (ret) + return ret; + + /* Start sinks pipe first */ + if (sink_mconfig->pipe->state != SKL_PIPE_STARTED) { + ret = skl_run_pipe(ctx, sink_mconfig->pipe); + if (ret) + return ret; + } + + path_list = kzalloc(sizeof(struct skl_dapm_path_list), + GFP_KERNEL); + if (NULL == path_list) + return -ENOMEM; + + /* Add connected path to one global list */ + path_list->dapm_path = p; + list_add_tail(&path_list->node, &skl->dapm_path_list); + break; + } + } + + /* Start source pipe last after starting all sinks */ + ret = skl_run_pipe(ctx, src_mconfig->pipe); + if (ret) + return ret; + + return 0; +} + +/* + * in the Post-PMU event of mixer we need to do following: + * - Check if this pipe is running + * - if not, then + * - bind this pipeline to its source pipeline + * if source pipe is already running, this means it is a dynamic + * connection and we need to bind only to that pipe + * - start this pipeline + */ +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + int ret = 0; + struct snd_soc_dapm_path *p; + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + struct skl_sst *ctx = skl->skl_sst; + int src_pipe_started = 0; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + sink = w; + sink_mconfig = sink->priv; + + /* + * If source pipe is already started, that means source is driving + * one more sink before this sink got connected, Since source is + * started, bind this sink to source and start this pipe. + */ + list_for_each_entry(p, &w->sources, list_sink) { + if (!p->connect) + continue; + dev_dbg(ctx->dev, "sink widget=%s\n", w->name); + dev_dbg(ctx->dev, "src widget=%s\n", p->source->name); + + /* + * here we will check widgets in sink pipelines, so that can + * be any widgets type and we are only interested if they are + * ones used for SKL so check that first + */ + if ((p->source->priv != NULL) && is_skl_dsp_widget_type(p->source)) { + source = p->source; + src_mconfig = source->priv; + sink_mconfig = sink->priv; + src_pipe_started = 1; + + /* + * check pipe state, then no need to bind or start the + * pipe + */ + if (src_mconfig->pipe->state != SKL_PIPE_STARTED) + src_pipe_started = 0; + } + } + + if (src_pipe_started) { + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, true); + if (ret) + return ret; + + return skl_run_pipe(ctx, sink_mconfig->pipe); + } + + return ret; +} + +/* + * in the Pre-PMD event of mixer we need to do following: + * - Stop the pipe + * - find the source connections and remove that from dapm_path_list + * - unbind with source pipelines if still connected + */ +static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + int ret = 0, path_found = 0; + struct skl_dapm_path_list *path_list, *tmp_list; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + sink = w; + sink_mconfig = sink->priv; + + /* Stop the pipe */ + ret = skl_stop_pipe(ctx, sink_mconfig->pipe); + if (ret) + return ret; + + list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) { + if (path_list->dapm_path->sink == sink) { + dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name); + source = path_list->dapm_path->source; + src_mconfig = source->priv; + path_found = 1; + + list_del(&path_list->node); + kfree(path_list); + break; + } + } + + /* + * If path_found == 1, that means pmd for source pipe has + * not occurred, source is connected to some other sink. + * so its responsibility of sink to unbind itself from source. + */ + if (path_found) + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, false); + + return ret; +} + +/* + * in the Post-PMD event of mixer we need to do following: + * - Free the mcps used + * - Free the mem used + * - Unbind the modules within the pipeline + * - Delete the pipeline (modules are not required to be explicitly + * deleted, pipeline delete is enough here + */ +static int skl_tplg_mixer_dapm_post_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct skl_module_cfg *mconfig = w->priv; + int ret = 0; + struct skl_sst *ctx = skl->skl_sst; + struct skl_pipe *s_pipe = mconfig->pipe; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + skl->resource.mcps -= mconfig->mcps; + + ret = skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, false); + if (ret < 0) + return ret; + + ret = skl_delete_pipe(ctx, mconfig->pipe); + skl->resource.mem -= mconfig->pipe->memory_pages; + + return ret; +} + +/* + * in the Post-PMD event of PGA we need to do following: + * - Free the mcps used + * - Stop the pipeline + * - In source pipe is connected, unbind with source pipelines + */ +static int skl_tplg_pga_dapm_post_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + int ret = 0, path_found = 0; + struct skl_dapm_path_list *path_list, *tmp_path_list; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + source = w; + src_mconfig = source->priv; + + skl->resource.mcps -= src_mconfig->mcps; + /* Stop the pipe since this is a mixin module */ + ret = skl_stop_pipe(ctx, src_mconfig->pipe); + if (ret) + return ret; + + list_for_each_entry_safe(path_list, tmp_path_list, &skl->dapm_path_list, node) { + if (path_list->dapm_path->source == source) { + dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name); + sink = path_list->dapm_path->sink; + sink_mconfig = sink->priv; + path_found = 1; + + list_del(&path_list->node); + kfree(path_list); + break; + } + } + + /* + * This is a connector and if path is found that means + * unbind between source and sink has not happened yet + */ + if (path_found) + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, false); + + return ret; +} + +static struct skl *get_skl_ctx(struct device *dev) +{ + struct hdac_ext_bus *ebus = dev_get_drvdata(dev); + + return ebus_to_skl(ebus); +} + +/* + * In modelling, we assume there will be ONLY one mixer in a pipeline. If + * mixer is not required then it is treated as static mixer aka vmixer with + * a hard path to source module + * So we don't need to check if source is started or not as hard path puts + * dependency on each other + */ +static int skl_tplg_vmixer_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_mixer_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_mixer_dapm_post_pmd_event(w, skl); + } + + return 0; +} + +/* + * In modelling, we assume there will be ONLY one mixer in a pipeline. If a + * second one is required that is created as another pipe entity. + * The mixer is responsible for pipe management and represent a pipeline + * instance + */ +static int skl_tplg_mixer_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_mixer_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMU: + return skl_tplg_mixer_dapm_post_pmu_event(w, skl); + + case SND_SOC_DAPM_PRE_PMD: + return skl_tplg_mixer_dapm_pre_pmd_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_mixer_dapm_post_pmd_event(w, skl); + } + + return 0; +} + +/* + * In modelling, we assumed rest of the modules in pipeline are PGA. But we + * are interested in last PGA (leaf PGA) in a pipeline to disconnect with + * the sink when it is running (two FE to one BE or one FE to two BE) + * scenarios + */ +static int skl_tplg_pga_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) + +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_pga_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_pga_dapm_post_pmd_event(w, skl); + } + + return 0; +}
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
The Skylake driver topology model tries to model the firmware rule for pipeline and module creation. The creation rule is:
- Create Pipe
- Add modules to Pipe
- Connect the modules (bind)
- Start the pipes
Similarly destroy rule is:
- Stop the pipe
- Disconnect it (unbind)
- Delete the pipe
In driver we use Mixer, as there will always be ONE mixer in a pipeline to model a pipe. The modules in pipe are modelled as PGA widgets. The DAPM sequencing rules (mixer and then PGA) are used to create the sequence DSP expects as depicted above, and then widget handlers for PMU and PMD events help in that.
This patch adds widget event handlers for PRE/POST PMU and PRE/POST PMD event for mixer and pga modules. These event handlers invoke pipeline creation, destroy, module creation, module bind, unbind and pipeline bind unbind
Event handler sequencing is implement to target the DSP FW sequence expectations to enable path from source to sink pipe for Playback/Capture.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Hardik T Shah hardik.t.shah@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
sound/soc/intel/skylake/skl-topology.c | 416 +++++++++++++++++++++++++++++++++ 1 file changed, 416 insertions(+)
snip...
+/*
- in the Pre-PMD event of mixer we need to do following:
- Stop the pipe
- find the source connections and remove that from dapm_path_list
- unbind with source pipelines if still connected
- */
+static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w,
struct skl *skl)+{
- struct snd_soc_dapm_widget *source, *sink;
- struct skl_module_cfg *src_mconfig, *sink_mconfig;
- int ret = 0, path_found = 0;
- struct skl_dapm_path_list *path_list, *tmp_list;
- struct skl_sst *ctx = skl->skl_sst;
- dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
- sink = w;
- sink_mconfig = sink->priv;
- /* Stop the pipe */
- ret = skl_stop_pipe(ctx, sink_mconfig->pipe);
- if (ret)
return ret;- list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) {
if (path_list->dapm_path->sink == sink) {dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name);source = path_list->dapm_path->source;src_mconfig = source->priv;path_found = 1;list_del(&path_list->node);kfree(path_list);break;}- }
There is a lot of list walking and manipulation in this series and it's not clear where any locks are being held to prevent list corruption. I'm assuming list items are being added and removed as part of loading/unloading the topology data but it looks like we are also manipulating component lists during DAPM events ?
Liam
On Thu, Sep 17, 2015 at 10:47:58AM +0100, Liam Girdwood wrote:
Hi Liam,
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
The Skylake driver topology model tries to model the firmware rule for pipeline and module creation. The creation rule is:
- Create Pipe
- Add modules to Pipe
- Connect the modules (bind)
- Start the pipes
Similarly destroy rule is:
- Stop the pipe
- Disconnect it (unbind)
- Delete the pipe
In driver we use Mixer, as there will always be ONE mixer in a pipeline to model a pipe. The modules in pipe are modelled as PGA widgets. The DAPM sequencing rules (mixer and then PGA) are used to create the sequence DSP expects as depicted above, and then widget handlers for PMU and PMD events help in that.
This patch adds widget event handlers for PRE/POST PMU and PRE/POST PMD event for mixer and pga modules. These event handlers invoke pipeline creation, destroy, module creation, module bind, unbind and pipeline bind unbind
Event handler sequencing is implement to target the DSP FW sequence expectations to enable path from source to sink pipe for Playback/Capture.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Hardik T Shah hardik.t.shah@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
sound/soc/intel/skylake/skl-topology.c | 416 +++++++++++++++++++++++++++++++++ 1 file changed, 416 insertions(+)
snip...
+/*
- in the Pre-PMD event of mixer we need to do following:
- Stop the pipe
- find the source connections and remove that from dapm_path_list
- unbind with source pipelines if still connected
- */
+static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w,
struct skl *skl)+{
- struct snd_soc_dapm_widget *source, *sink;
- struct skl_module_cfg *src_mconfig, *sink_mconfig;
- int ret = 0, path_found = 0;
- struct skl_dapm_path_list *path_list, *tmp_list;
- struct skl_sst *ctx = skl->skl_sst;
- dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
- sink = w;
- sink_mconfig = sink->priv;
- /* Stop the pipe */
- ret = skl_stop_pipe(ctx, sink_mconfig->pipe);
- if (ret)
return ret;- list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) {
if (path_list->dapm_path->sink == sink) {dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name);source = path_list->dapm_path->source;src_mconfig = source->priv;path_found = 1;list_del(&path_list->node);kfree(path_list);break;}- }
There is a lot of list walking and manipulation in this series and it's not clear where any locks are being held to prevent list corruption. I'm assuming list items are being added and removed as part of loading/unloading the topology data but it looks like we are also manipulating component lists during DAPM events ?
We have a driver list dapm_path_list where we store the widgets powered up. This gives us a very quick reference of the paths which are powered up in the graph and helps fast traversal to check if we should power up a path as path is connected to something else which is powered up already (mixng two paths) and similarly while disconnecting.
Please note all these are handled only in event handlers for widgets, so they will be invoked by dapm with mutex, dapm_mutex held, so we didn't think we would need another lock here
On Thu, 2015-09-17 at 17:08 +0530, Vinod Koul wrote:
On Thu, Sep 17, 2015 at 10:47:58AM +0100, Liam Girdwood wrote:
Hi Liam,
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
The Skylake driver topology model tries to model the firmware rule for pipeline and module creation. The creation rule is:
- Create Pipe
- Add modules to Pipe
- Connect the modules (bind)
- Start the pipes
Similarly destroy rule is:
- Stop the pipe
- Disconnect it (unbind)
- Delete the pipe
In driver we use Mixer, as there will always be ONE mixer in a pipeline to model a pipe. The modules in pipe are modelled as PGA widgets. The DAPM sequencing rules (mixer and then PGA) are used to create the sequence DSP expects as depicted above, and then widget handlers for PMU and PMD events help in that.
This patch adds widget event handlers for PRE/POST PMU and PRE/POST PMD event for mixer and pga modules. These event handlers invoke pipeline creation, destroy, module creation, module bind, unbind and pipeline bind unbind
Event handler sequencing is implement to target the DSP FW sequence expectations to enable path from source to sink pipe for Playback/Capture.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Hardik T Shah hardik.t.shah@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
sound/soc/intel/skylake/skl-topology.c | 416 +++++++++++++++++++++++++++++++++ 1 file changed, 416 insertions(+)
snip...
+/*
- in the Pre-PMD event of mixer we need to do following:
- Stop the pipe
- find the source connections and remove that from dapm_path_list
- unbind with source pipelines if still connected
- */
+static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w,
struct skl *skl)+{
- struct snd_soc_dapm_widget *source, *sink;
- struct skl_module_cfg *src_mconfig, *sink_mconfig;
- int ret = 0, path_found = 0;
- struct skl_dapm_path_list *path_list, *tmp_list;
- struct skl_sst *ctx = skl->skl_sst;
- dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
- sink = w;
- sink_mconfig = sink->priv;
- /* Stop the pipe */
- ret = skl_stop_pipe(ctx, sink_mconfig->pipe);
- if (ret)
return ret;- list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) {
if (path_list->dapm_path->sink == sink) {dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name);source = path_list->dapm_path->source;src_mconfig = source->priv;path_found = 1;list_del(&path_list->node);kfree(path_list);break;}- }
There is a lot of list walking and manipulation in this series and it's not clear where any locks are being held to prevent list corruption. I'm assuming list items are being added and removed as part of loading/unloading the topology data but it looks like we are also manipulating component lists during DAPM events ?
We have a driver list dapm_path_list where we store the widgets powered up. This gives us a very quick reference of the paths which are powered up in the graph and helps fast traversal to check if we should power up a path as path is connected to something else which is powered up already (mixng two paths) and similarly while disconnecting.
Please note all these are handled only in event handlers for widgets, so they will be invoked by dapm with mutex, dapm_mutex held, so we didn't think we would need another lock here
Ok, I was thinking that may be the case. It may be worth while stating this in comments where this applies.
Liam
On Thu, Sep 17, 2015 at 01:25:07PM +0100, Liam Girdwood wrote:
There is a lot of list walking and manipulation in this series and it's not clear where any locks are being held to prevent list corruption. I'm assuming list items are being added and removed as part of loading/unloading the topology data but it looks like we are also manipulating component lists during DAPM events ?
We have a driver list dapm_path_list where we store the widgets powered up. This gives us a very quick reference of the paths which are powered up in the graph and helps fast traversal to check if we should power up a path as path is connected to something else which is powered up already (mixng two paths) and similarly while disconnecting.
Please note all these are handled only in event handlers for widgets, so they will be invoked by dapm with mutex, dapm_mutex held, so we didn't think we would need another lock here
Ok, I was thinking that may be the case. It may be worth while stating this in comments where this applies.
It's actually specfied that we add all connected paths to drivers list. I am okay to add more, will do so here if there any anymore comments from Mark, otherwise will add as an update in patches after this series
Thanks
On Mon, Aug 17, 2015 at 10:56:38PM +0530, Vinod Koul wrote:
- if (list_empty(&s_pipe->w_list)) {
ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe);if (ret < 0)return ret;- }
I'm not entirely clear what this is supposed to do or if it does it correctly - I suspect it's trying to always create the pipe widget which might be a bit clearer if it just created the pipe widget. The fact that it's checking if a list is empty is a bit worrying, what if some but not all of the entries needed on that list are there? Is just getting a widget really sufficient initialisation?
- return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true);
+} +/*
- A PGA represents a module in a pipeline. So in the Pre-PMU event of PGA
Blank line between functions and the next thing. I am getting fed up of seeing this.
On Sat, Sep 19, 2015 at 09:11:22AM -0700, Mark Brown wrote:
On Mon, Aug 17, 2015 at 10:56:38PM +0530, Vinod Koul wrote:
- if (list_empty(&s_pipe->w_list)) {
ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe);if (ret < 0)return ret;- }
I'm not entirely clear what this is supposed to do or if it does it correctly - I suspect it's trying to always create the pipe widget which might be a bit clearer if it just created the pipe widget. The fact that it's checking if a list is empty is a bit worrying, what if some but not all of the entries needed on that list are there? Is just getting a widget really sufficient initialisation?
For quick reference, on first run of a pipe, we create a w_list of all widgets in that pipe. This list is not freed on PMD event as widgets within a pipe are static. This saves us cycles to get widgets in pipe every time.
So if we already initialized all the widgets of a pipeline we don't need that anymore. I will add comment for this as well
- return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true);
+} +/*
- A PGA represents a module in a pipeline. So in the Pre-PMU event of PGA
Blank line between functions and the next thing. I am getting fed up of seeing this.
Will fix these
Thanks
From: Jeeja KP jeeja.kp@intel.com
For FE and BE, the PCM parameters come from FE and BE hw_params values passed. For a FE we convert the FE params to DSP expected module format and pass to DSP. For a BE we need to find the gateway settings (i2s/PDM) to be applied. These are queried from NHLT table and applied.
Further for BE based on direction the settings are applied as either source or destination parameters.
These helpers here allow the format to be calculated and queried as per firmware format.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-topology.c | 283 +++++++++++++++++++++++++++++++++ sound/soc/intel/skylake/skl-topology.h | 11 ++ 2 files changed, 294 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 184697188a96..287935eab68b 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -760,3 +760,286 @@ static int skl_tplg_pga_event(struct snd_soc_dapm_widget *w,
return 0; } + +/* + * The FE params are passed by hw_params of the DAI. + * On hw_params, the params are stored in Gateway module of the FE and we + * need to calculate the format in DSP module configuration, that conversion + * is done here + */ +static int skl_tplg_update_pipe_params(struct device *dev, + struct skl_module_cfg *mconfig, struct skl_pipe_params *params) +{ + struct skl_pipe *pipe = mconfig->pipe; + struct skl_module_fmt *format = NULL; + + memcpy(pipe->p_params, params, sizeof(*params)); + + if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) + format = &mconfig->in_fmt; + else + format = &mconfig->out_fmt; + + /* set the hw_params */ + format->s_freq = params->s_freq; + format->channels = params->ch; + format->valid_bit_depth = skl_get_bit_depth(params->s_fmt); + + /* + * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so + * update bit depth accordingly + */ + switch (format->valid_bit_depth) { + case SKL_DEPTH_16BIT: + format->bit_depth = format->valid_bit_depth; + break; + + case SKL_DEPTH_24BIT: + format->bit_depth = SKL_DEPTH_32BIT; + break; + + default: + dev_err(dev, "Invalid bit depth %x for pipe\n", + format->valid_bit_depth); + return -EINVAL; + } + + if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) { + mconfig->ibs = (format->s_freq / 1000) * + (format->channels) * + (format->bit_depth >> 3); + } else { + mconfig->obs = (format->s_freq / 1000) * + (format->channels) * + (format->bit_depth >> 3); + } + + return 0; +} + +/* + * Query the module config for the FE DAI + * This is used to find the hw_params set for that DAI and apply to FE + * pipeline + */ +static struct skl_module_cfg * +skl_tplg_fe_get_cpr_module(struct snd_soc_dai *dai, int stream) +{ + struct snd_soc_dapm_widget *w; + struct snd_soc_dapm_path *p = NULL; + + dev_dbg(dai->dev, "%s: enter, dai-name=%s\n", __func__, dai->name); + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + w = dai->playback_widget; + dev_dbg(dai->dev, "Stream name=%s\n", w->name); + list_for_each_entry(p, &w->sinks, list_source) { + if (p->connect && p->sink->power && + is_skl_dsp_widget_type(p->sink)) + continue; + + if (p->sink->priv) { + dev_dbg(dai->dev, "set params for %s\n", p->sink->name); + return p->sink->priv; + } + } + } else { + w = dai->capture_widget; + dev_dbg(dai->dev, "Stream name=%s\n", w->name); + list_for_each_entry(p, &w->sources, list_sink) { + if (p->connect && p->source->power && + is_skl_dsp_widget_type(p->source)) + continue; + + if (p->source->priv) { + dev_dbg(dai->dev, "set params for %s\n", p->source->name); + return p->source->priv; + } + } + } + return NULL; +} + +int skl_tplg_fe_update_params(struct snd_soc_dai *dai, + struct skl_pipe_params *params) +{ + struct skl_module_cfg *m_cfg; + + m_cfg = skl_tplg_fe_get_cpr_module(dai, params->stream); + + if (m_cfg) + skl_tplg_update_pipe_params(dai->dev, m_cfg, params); + + return 0; +} + +static u8 skl_tplg_be_link_type(int dev_type) +{ + int ret; + + switch (dev_type) { + case SKL_DEVICE_BT: + ret = NHLT_LINK_SSP; + break; + + case SKL_DEVICE_DMIC: + ret = NHLT_LINK_DMIC; + break; + + case SKL_DEVICE_I2S: + ret = NHLT_LINK_SSP; + break; + + case SKL_DEVICE_HDALINK: + ret = NHLT_LINK_HDA; + break; + + default: + ret = NHLT_LINK_INVALID; + break; + } + + return ret; +} + +/* + * Fill the BE gateway parameters + * The BE gateway expects a blob of parameters which are kept in the ACPI + * NHLT blob, so query the blob for interface type (i2s/pdm) and instance. + * The port can have multiple settings so pick based on the PCM + * parameters + */ +static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai, + struct skl_module_cfg *mconfig, struct skl_pipe_params *params) +{ + struct skl_pipe *pipe = mconfig->pipe; + struct nhlt_specific_cfg *cfg; + struct skl *skl = get_skl_ctx(dai->dev); + int link_type = skl_tplg_be_link_type(mconfig->dev_type); + + memcpy(pipe->p_params, params, sizeof(*params)); + + /* update the blob based on virtual bus_id*/ + cfg = skl_get_ep_blob(skl, mconfig->vbus_id, link_type, params->s_fmt, + params->ch, params->s_freq, params->stream); + if (cfg) { + mconfig->formats_config.caps_size = cfg->size; + memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size); + } else { + dev_err(dai->dev, "Blob is NULL for id %x type %d dirn %d\n", + mconfig->vbus_id, link_type, params->stream); + dev_err(dai->dev, "PCM: ch %d, freq %d, fmt %d\n", + params->ch, params->s_freq, params->s_fmt); + return -EINVAL; + } + + return 0; +} + +static int skl_tplg_be_set_params(struct snd_soc_dai *dai, + struct snd_soc_dapm_widget *w, struct skl_pipe_params *params) +{ + if (!w->power) { + dev_dbg(dai->dev, "set params for widget=%s\n", w->name); + return skl_tplg_be_fill_pipe_params(dai, w->priv, params); + } + + return -EBUSY; +} + +static int skl_tplg_be_set_src_pipe_params(struct snd_soc_dai *dai, + struct snd_soc_dapm_widget *w, struct skl_pipe_params *params) +{ + struct snd_soc_dapm_path *p; + int ret = 0; + + dev_dbg(dai->dev, "%s: widget name=%s\n", __func__, w->name); + + list_for_each_entry(p, &w->sources, list_sink) { + if (p->connect && is_skl_dsp_widget_type(p->source) && + p->source->priv) { + ret = skl_tplg_be_set_params(dai, p->source, params); + + if (ret < 0) + return ret; + } else { + ret = skl_tplg_be_set_src_pipe_params(dai, p->source, + params); + } + } + + return ret; +} + +static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai, + struct snd_soc_dapm_widget *w, struct skl_pipe_params *params) +{ + struct snd_soc_dapm_path *p = NULL; + int ret = 0; + + dev_dbg(dai->dev, "widget name=%s\n", w->name); + + list_for_each_entry(p, &w->sinks, list_source) { + if (p->connect && is_skl_dsp_widget_type(p->sink) && + p->sink->priv) { + ret = skl_tplg_be_set_params(dai, p->sink, params); + if (ret < 0) + return ret; + + } else { + ret = skl_tplg_be_set_sink_pipe_params( + dai, p->sink, params); + } + } + + return ret; +} + +/* + * BE hw_params can be a source parameters (capture) or sink parameters + * (playback). Based on sink and source we need to either find the source + * list or the sink list and set the pipeline parameters + */ +int skl_tplg_be_update_params(struct snd_soc_dai *dai, + struct skl_pipe_params *params) +{ + struct snd_soc_dapm_widget *w; + + dev_dbg(dai->dev, "%s: dai-name=%s\n", __func__, dai->name); + + if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) { + w = dai->playback_widget; + + dev_dbg(dai->dev, "Stream name %s\n", w->name); + return skl_tplg_be_set_src_pipe_params(dai, w, params); + + } else { + w = dai->capture_widget; + + dev_dbg(dai->dev, "Stream name %s\n", w->name); + return skl_tplg_be_set_sink_pipe_params(dai, w, params); + } + + return 0; +} + +/* Run/Stop the FE pipeline */ +int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start, + int stream) +{ + struct skl *skl = get_skl_ctx(dai->dev); + struct skl_sst *ctx = skl->skl_sst; + struct skl_module_cfg *mconfig = NULL; + + dev_dbg(dai->dev, "%s: enter, dai-name=%s dir=%d\n", __func__, dai->name, stream); + + mconfig = skl_tplg_fe_get_cpr_module(dai, stream); + if (mconfig != NULL) { + if (start) + return skl_run_pipe(ctx, mconfig->pipe); + else + return skl_stop_pipe(ctx, mconfig->pipe); + } + + return 0; +} diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h index 73d7916ee33e..94c721e4d095 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -273,6 +273,17 @@ struct skl_dapm_path_list { struct list_head node; };
+int skl_tplg_fe_update_params(struct snd_soc_dai *dai, + struct skl_pipe_params *params); +int skl_tplg_be_update_params(struct snd_soc_dai *dai, + struct skl_pipe_params *params); +void skl_tplg_set_be_dmic_config(struct snd_soc_dai *dai, + struct skl_pipe_params *params, int stream); +int skl_tplg_init(struct snd_soc_platform *platform, + struct hdac_ext_bus *ebus); +int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, + bool start, int stream); + int skl_create_pipeline(struct skl_sst *ctx, struct skl_pipe *pipe);
int skl_run_pipe(struct skl_sst *ctx, struct skl_pipe *pipe);
On Mon, Aug 17, 2015 at 10:56:39PM +0530, Vinod Koul wrote:
+/* Run/Stop the FE pipeline */ +int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start,
int stream)+{
- struct skl *skl = get_skl_ctx(dai->dev);
- struct skl_sst *ctx = skl->skl_sst;
- struct skl_module_cfg *mconfig = NULL;
- dev_dbg(dai->dev, "%s: enter, dai-name=%s dir=%d\n", __func__, dai->name, stream);
- mconfig = skl_tplg_fe_get_cpr_module(dai, stream);
- if (mconfig != NULL) {
if (start)return skl_run_pipe(ctx, mconfig->pipe);elsereturn skl_stop_pipe(ctx, mconfig->pipe);- }
- return 0;
+}
Another one of these wrappers with basically no shared code :(
On Sat, Sep 19, 2015 at 09:22:10AM -0700, Mark Brown wrote:
On Mon, Aug 17, 2015 at 10:56:39PM +0530, Vinod Koul wrote:
+/* Run/Stop the FE pipeline */ +int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start,
int stream)+{
- struct skl *skl = get_skl_ctx(dai->dev);
- struct skl_sst *ctx = skl->skl_sst;
- struct skl_module_cfg *mconfig = NULL;
- dev_dbg(dai->dev, "%s: enter, dai-name=%s dir=%d\n", __func__, dai->name, stream);
- mconfig = skl_tplg_fe_get_cpr_module(dai, stream);
- if (mconfig != NULL) {
if (start)return skl_run_pipe(ctx, mconfig->pipe);elsereturn skl_stop_pipe(ctx, mconfig->pipe);- }
- return 0;
+}
Another one of these wrappers with basically no shared code :(
Yes this is wrapper over pipeline start and stop. This function is invoked from the PCM trigger code so we though providing a wrapper makes it look better. I will remove this and invoke skl_run_pipe and skl_stop_pipe from PCM trigger now
Will check if we can remove other wrappers as well
Thanks
From: Jeeja KP jeeja.kp@intel.com
The SKL driver does not code DSP topology in driver. It uses the newly added ASoC topology core to parse the topology information (controls, widgets and map) from topology binary. Each topology element passed private data which contains information that driver used to identify the module instance within firmware and send IPCs for that module to DSP firmware along with parameters. This patch adds init routine to invoke topology load and callback for topology creation.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-topology.c | 217 +++++++++++++++++++++++++++ sound/soc/intel/skylake/skl-tplg-interface.h | 78 ++++++++++ 2 files changed, 295 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 287935eab68b..d18ce8c461a4 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -1043,3 +1043,220 @@ int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start,
return 0; } + +const static struct snd_soc_tplg_widget_events skl_tplg_widget_ops[] = { + {SKL_MIXER_EVENT, skl_tplg_mixer_event}, + {SKL_VMIXER_EVENT, skl_tplg_vmixer_event}, + {SKL_PGA_EVENT, skl_tplg_pga_event}, +}; + +/* + * The topology binary passes the pin info for a module so initialize the pin + * info passed into module instance + */ +static void skl_fill_module_pin_info(struct device *dev, + struct skl_module_pin *m_pin, + int max_pin) +{ + int i; + + for (i = 0; i < max_pin; i++) { + m_pin[i].id.module_id = 0; + m_pin[i].id.instance_id = 0; + m_pin[i].in_use = false; + m_pin[i].is_dynamic = true; + m_pin[i].pin_index = i; + } +} + +/* + * Add pipeline from topology binary into driver pipeline list + * + * If already added we return that instance + * Otherwise we create a new instance and add into driver list + */ +static struct skl_pipe *skl_tplg_add_pipe(struct device *dev, + struct skl *skl, struct skl_dfw_pipe *dfw_pipe) +{ + struct skl_pipeline *ppl; + struct skl_pipe *pipe; + struct skl_pipe_params *params; + + list_for_each_entry(ppl, &skl->ppl_list, node) { + if (ppl->pipe->ppl_id == dfw_pipe->pipe_id) + return ppl->pipe; + } + + ppl = devm_kzalloc(dev, sizeof(*ppl), GFP_KERNEL); + if (!ppl) + return NULL; + + pipe = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL); + if (!pipe) + return NULL; + + params = devm_kzalloc(dev, sizeof(*params), GFP_KERNEL); + if (!params) + return NULL; + + pipe->ppl_id = dfw_pipe->pipe_id; + pipe->memory_pages = dfw_pipe->memory_pages; + pipe->pipe_priority = dfw_pipe->pipe_priority; + pipe->conn_type = dfw_pipe->conn_type; + pipe->state = SKL_PIPE_INVALID; + pipe->p_params = params; + INIT_LIST_HEAD(&pipe->w_list); + + ppl->pipe = pipe; + list_add(&ppl->node, &skl->ppl_list); + + return ppl->pipe; +} + +/* + * Topology core widget load callback + * + * This is used to save the private data for each widget which gives + * information to the driver about module and pipeline parameters which DSP + * FW expects like ids, resource values, formats etc + */ +static int skl_tplg_widget_load(struct snd_soc_component *cmpnt, + struct snd_soc_dapm_widget *w, + struct snd_soc_tplg_dapm_widget *tplg_w) +{ + int ret; + struct hdac_ext_bus *ebus = snd_soc_component_get_drvdata(cmpnt); + struct skl *skl = ebus_to_skl(ebus); + struct hdac_bus *bus = ebus_to_hbus(ebus); + struct skl_module_cfg *mconfig; + struct skl_pipe *pipe; + struct skl_dfw_module *dfw_config = (struct skl_dfw_module *)tplg_w->priv.data; + + if (!tplg_w->priv.size) + goto bind_event; + + mconfig = devm_kzalloc(bus->dev, sizeof(*mconfig), GFP_KERNEL); + + if (!mconfig) + return -ENOMEM; + + w->priv = mconfig; + mconfig->id.module_id = dfw_config->module_id; + mconfig->id.instance_id = dfw_config->instance_id; + mconfig->mcps = dfw_config->max_mcps; + mconfig->ibs = dfw_config->ibs; + mconfig->obs = dfw_config->obs; + mconfig->core_id = dfw_config->core_id; + mconfig->max_in_queue = dfw_config->max_in_queue; + mconfig->max_out_queue = dfw_config->max_out_queue; + mconfig->is_loadable = dfw_config->is_loadable; + mconfig->in_fmt.channels = dfw_config->in_fmt.channels; + mconfig->in_fmt.s_freq = dfw_config->in_fmt.freq; + mconfig->in_fmt.bit_depth = dfw_config->in_fmt.bit_depth; + mconfig->in_fmt.valid_bit_depth = dfw_config->in_fmt.valid_bit_depth; + mconfig->in_fmt.ch_cfg = dfw_config->in_fmt.ch_cfg; + mconfig->out_fmt.channels = dfw_config->out_fmt.channels; + mconfig->out_fmt.s_freq = dfw_config->out_fmt.freq; + mconfig->out_fmt.bit_depth = dfw_config->out_fmt.bit_depth; + mconfig->out_fmt.valid_bit_depth = dfw_config->out_fmt.valid_bit_depth; + mconfig->out_fmt.ch_cfg = dfw_config->out_fmt.ch_cfg; + mconfig->params_fixup = dfw_config->params_fixup; + mconfig->converter = dfw_config->converter; + mconfig->m_type = dfw_config->module_type; + mconfig->vbus_id = dfw_config->vbus_id; + + pipe = skl_tplg_add_pipe(bus->dev, skl, &dfw_config->pipe); + if (pipe) + mconfig->pipe = pipe; + + mconfig->dev_type = dfw_config->dev_type; + mconfig->hw_conn_type = dfw_config->hw_conn_type; + mconfig->time_slot = dfw_config->time_slot; + mconfig->formats_config.caps_size = dfw_config->caps.caps_size; + + mconfig->m_in_pin = devm_kzalloc(bus->dev, (mconfig->max_in_queue) * + sizeof(*mconfig->m_in_pin), + GFP_KERNEL); + if (!mconfig->m_in_pin) + return -ENOMEM; + + mconfig->m_out_pin = devm_kzalloc(bus->dev, (mconfig->max_in_queue) * + sizeof(*mconfig->m_out_pin), + GFP_KERNEL); + if (!mconfig->m_out_pin) + return -ENOMEM; + + skl_fill_module_pin_info(bus->dev, mconfig->m_in_pin, + mconfig->max_in_queue); + skl_fill_module_pin_info(bus->dev, mconfig->m_out_pin, + mconfig->max_out_queue); + + if (mconfig->formats_config.caps_size == 0) + goto bind_event; + + mconfig->formats_config.caps = (u32 *)devm_kzalloc(bus->dev, + mconfig->formats_config.caps_size, GFP_KERNEL); + + if (mconfig->formats_config.caps == NULL) + return -ENOMEM; + + memcpy(mconfig->formats_config.caps, dfw_config->caps.caps, + dfw_config->caps.caps_size); + +bind_event: + if (tplg_w->event_type == 0) { + dev_info(bus->dev, "ASoC: No event handler required\n"); + return 0; + } + + ret = snd_soc_tplg_widget_bind_event(w, skl_tplg_widget_ops, + ARRAY_SIZE(skl_tplg_widget_ops), tplg_w->event_type); + + if (ret) { + dev_err(bus->dev, "%s: No matching event handlers found for %d\n", + __func__, tplg_w->event_type); + return -EINVAL; + } + + return 0; +} + +static struct snd_soc_tplg_ops skl_tplg_ops = { + .widget_load = skl_tplg_widget_load, +}; + +/* This will be read from topology manifest, currently defined here */ +#define SKL_MAX_MCPS 30000000 +#define SKL_FW_MAX_MEM 1000000 + +/* + * SKL topology init routine + */ +int skl_tplg_init(struct snd_soc_platform *platform, struct hdac_ext_bus *ebus) +{ + int ret; + const struct firmware *fw; + struct hdac_bus *bus = ebus_to_hbus(ebus); + struct skl *skl = ebus_to_skl(ebus); + + ret = request_firmware(&fw, "dfw_sst.bin", bus->dev); + if (ret < 0) { + dev_err(bus->dev, "config firmware request failed with %d\n", ret); + return ret; + } + + /* + * The complete tplg for SKL is loaded as index 0, we don't use + * any other index + */ + ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0); + if (ret < 0) { + dev_err(bus->dev, "tplg component load failed%d\n", ret); + return -EINVAL; + } + + skl->resource.max_mcps = SKL_MAX_MCPS; + skl->resource.max_mem = SKL_FW_MAX_MEM; + + return 0; +} diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h index a50689825bca..b258c90d89f1 100644 --- a/sound/soc/intel/skylake/skl-tplg-interface.h +++ b/sound/soc/intel/skylake/skl-tplg-interface.h @@ -19,6 +19,27 @@ #ifndef __HDA_TPLG_INTERFACE_H__ #define __HDA_TPLG_INTERFACE_H__
+/* Default types range from 0~12. type can range from 0 to 0xff + * SST types start at higher to avoid any overlapping in future */ +#define SOC_CONTROL_TYPE_HDA_SST_ALGO_PARAMS 200 +#define SOC_CONTROL_TYPE_HDA_SST_MUX 201 +#define SOC_CONTROL_TYPE_HDA_SST_MIX 201 +#define SOC_CONTROL_TYPE_HDA_SST_BYTE 203 + +#define HDA_SST_CFG_MAX 900 /* size of copier cfg*/ +#define MAX_IN_QUEUE 8 +#define MAX_OUT_QUEUE 8 + +/* Event types goes here */ +/* Reserve event type 0 for no event handlers */ +enum skl_event_types { + SKL_EVENT_NONE = 0, + SKL_MIXER_EVENT, + SKL_MUX_EVENT, + SKL_VMIXER_EVENT, + SKL_PGA_EVENT +}; + /** * enum skl_ch_cfg - channel configuration * @@ -85,4 +106,61 @@ enum skl_dev_type { SKL_DEVICE_HDALINK = 0x4, SKL_DEVICE_NONE }; + +struct skl_dfw_module_pin { + u16 module_id; + u16 instance_id; + u8 pin_id; + bool is_dynamic; +} __packed; + +struct skl_dfw_module_fmt { + u32 channels; + u32 freq; + u32 bit_depth; + u32 valid_bit_depth; + u32 ch_cfg; +} __packed; + +struct skl_dfw_module_caps { + u32 caps_size; + u32 caps[HDA_SST_CFG_MAX]; +}; + +struct skl_dfw_pipe { + u8 pipe_id; + u8 pipe_priority; + u16 conn_type; + u32 memory_pages; +} __packed; + +struct skl_dfw_module { + u16 module_id; + u16 instance_id; + u32 max_mcps; + u8 core_id; + u8 max_in_queue; + u8 max_out_queue; + u8 is_loadable; + u8 conn_type; + u8 dev_type; + u8 hw_conn_type; + u8 time_slot; + u32 obs; + u32 ibs; + u32 params_fixup; + u32 converter; + u32 module_type; + u32 vbus_id; + struct skl_dfw_pipe pipe; + struct skl_dfw_module_fmt in_fmt; + struct skl_dfw_module_fmt out_fmt; + struct skl_dfw_module_caps caps; +} __packed; + +struct skl_dfw_algo_data { + u32 max; + char *params; +} __packed; + #endif
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
The SKL driver does not code DSP topology in driver. It uses the newly added ASoC topology core to parse the topology information (controls, widgets and map) from topology binary. Each topology element passed private data which contains information that driver used to identify the module instance within firmware and send IPCs for that module to DSP firmware along with parameters. This patch adds init routine to invoke topology load and callback for topology creation.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
+/*
- SKL topology init routine
- */
+int skl_tplg_init(struct snd_soc_platform *platform, struct hdac_ext_bus *ebus) +{
- int ret;
- const struct firmware *fw;
- struct hdac_bus *bus = ebus_to_hbus(ebus);
- struct skl *skl = ebus_to_skl(ebus);
- ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
- if (ret < 0) {
dev_err(bus->dev, "config firmware request failed with %d\n", ret);
It would be good to say what file name we are failing with here.
return ret;- }
- /*
* The complete tplg for SKL is loaded as index 0, we don't use* any other index*/- ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);
- if (ret < 0) {
dev_err(bus->dev, "tplg component load failed%d\n", ret);return -EINVAL;- }
- skl->resource.max_mcps = SKL_MAX_MCPS;
- skl->resource.max_mem = SKL_FW_MAX_MEM;
- return 0;
+} diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h index a50689825bca..b258c90d89f1 100644 --- a/sound/soc/intel/skylake/skl-tplg-interface.h +++ b/sound/soc/intel/skylake/skl-tplg-interface.h @@ -19,6 +19,27 @@ #ifndef __HDA_TPLG_INTERFACE_H__ #define __HDA_TPLG_INTERFACE_H__
+/* Default types range from 0~12. type can range from 0 to 0xff
- SST types start at higher to avoid any overlapping in future */
+#define SOC_CONTROL_TYPE_HDA_SST_ALGO_PARAMS 200 +#define SOC_CONTROL_TYPE_HDA_SST_MUX 201 +#define SOC_CONTROL_TYPE_HDA_SST_MIX 201 +#define SOC_CONTROL_TYPE_HDA_SST_BYTE 203
These are lower than 0xff
Liam
On Fri, Sep 18, 2015 at 10:55:57AM +0100, Liam Girdwood wrote:
+/* Default types range from 0~12. type can range from 0 to 0xff
- SST types start at higher to avoid any overlapping in future */
+#define SOC_CONTROL_TYPE_HDA_SST_ALGO_PARAMS 200 +#define SOC_CONTROL_TYPE_HDA_SST_MUX 201 +#define SOC_CONTROL_TYPE_HDA_SST_MIX 201 +#define SOC_CONTROL_TYPE_HDA_SST_BYTE 203
These are lower than 0xff
Thanks for pointing, I will update these...
From: Jeeja KP jeeja.kp@intel.com
Initialize and creates DSP controls if processing pipe capability is supported by HW. Updates the dma_id, hw_params to module param to be used when DSP module has to be configured.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-pcm.c | 138 ++++++++++++++++++++++++++++++-------- 1 file changed, 111 insertions(+), 27 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 7d617bf493bc..3e491149bf0c 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -24,6 +24,7 @@ #include <sound/pcm_params.h> #include <sound/soc.h> #include "skl.h" +#include "skl-topology.h"
#define HDA_MONO 1 #define HDA_STEREO 2 @@ -214,6 +215,7 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream, struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev); struct hdac_ext_stream *stream = get_hdac_ext_stream(substream); struct snd_pcm_runtime *runtime = substream->runtime; + struct skl_pipe_params p_params = {0}; int ret, dma_id;
dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name); @@ -228,6 +230,13 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream, dma_id = hdac_stream(stream)->stream_tag - 1; dev_dbg(dai->dev, "dma_id=%d\n", dma_id);
+ p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.ch = params_channels(params); + p_params.s_freq = params_rate(params); + p_params.host_dma_id = dma_id; + p_params.stream = substream->stream; + skl_tplg_fe_update_params(dai, &p_params); + return 0; }
@@ -268,6 +277,46 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream, return skl_substream_free_pages(ebus_to_hbus(ebus), substream); }
+static int skl_be_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct skl_pipe_params p_params = {0}; + + dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name); + + p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.ch = params_channels(params); + p_params.s_freq = params_rate(params); + p_params.stream = substream->stream; + skl_tplg_be_update_params(dai, &p_params); + + return 0; +} + +static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd, + struct snd_soc_dai *dai) +{ + dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd); + + switch (cmd) { + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + case SNDRV_PCM_TRIGGER_RESUME: + return skl_tplg_set_fe_pipeline_state(dai, true, + substream->stream); + + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + case SNDRV_PCM_TRIGGER_SUSPEND: + return skl_tplg_set_fe_pipeline_state(dai, false, + substream->stream); + + default: + return 0; + } + + return 0; +} + static int skl_link_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -277,9 +326,10 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream); struct skl_dma_params *dma_params; struct snd_soc_dai *codec_dai = rtd->codec_dai; - int dma_id; + struct skl_pipe_params p_params = {0}; + + dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
- pr_debug("%s\n", __func__); link_dev = snd_hdac_ext_stream_assign(ebus, substream, HDAC_EXT_STREAM_TYPE_LINK); if (!link_dev) @@ -293,7 +343,14 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream, if (dma_params) dma_params->stream_tag = hdac_stream(link_dev)->stream_tag; snd_soc_dai_set_dma_data(codec_dai, substream, (void *)dma_params); - dma_id = hdac_stream(link_dev)->stream_tag - 1; + + p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.ch = params_channels(params); + p_params.s_freq = params_rate(params); + p_params.stream = substream->stream; + p_params.link_dma_id = hdac_stream(link_dev)->stream_tag - 1; + + skl_tplg_be_update_params(dai, &p_params);
return 0; } @@ -308,8 +365,6 @@ static int skl_link_pcm_prepare(struct snd_pcm_substream *substream, unsigned int format_val = 0; struct skl_dma_params *dma_params; struct snd_soc_dai *codec_dai = rtd->codec_dai; - struct snd_pcm_hw_params *params; - struct snd_interval *channels, *rate; struct hdac_ext_link *link;
dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name); @@ -317,18 +372,6 @@ static int skl_link_pcm_prepare(struct snd_pcm_substream *substream, dev_dbg(dai->dev, "already stream is prepared - returning\n"); return 0; } - params = devm_kzalloc(dai->dev, sizeof(*params), GFP_KERNEL); - if (params == NULL) - return -ENOMEM; - - channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - channels->min = channels->max = substream->runtime->channels; - rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - rate->min = rate->max = substream->runtime->rate; - snd_mask_set(¶ms->masks[SNDRV_PCM_HW_PARAM_FORMAT - - SNDRV_PCM_HW_PARAM_FIRST_MASK], - substream->runtime->format); -
dma_params = (struct skl_dma_params *) snd_soc_dai_get_dma_data(codec_dai, substream); @@ -399,13 +442,13 @@ static int skl_link_hw_free(struct snd_pcm_substream *substream, return 0; }
-static int skl_hda_be_startup(struct snd_pcm_substream *substream, +static int skl_be_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { return pm_runtime_get_sync(dai->dev); }
-static void skl_hda_be_shutdown(struct snd_pcm_substream *substream, +static void skl_be_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { pm_runtime_mark_last_busy(dai->dev); @@ -418,20 +461,28 @@ static struct snd_soc_dai_ops skl_pcm_dai_ops = { .prepare = skl_pcm_prepare, .hw_params = skl_pcm_hw_params, .hw_free = skl_pcm_hw_free, + .trigger = skl_pcm_trigger, };
static struct snd_soc_dai_ops skl_dmic_dai_ops = { - .startup = skl_hda_be_startup, - .shutdown = skl_hda_be_shutdown, + .startup = skl_be_startup, + .hw_params = skl_be_hw_params, + .shutdown = skl_be_shutdown, +}; + +static struct snd_soc_dai_ops skl_be_ssp_dai_ops = { + .startup = skl_be_startup, + .hw_params = skl_be_hw_params, + .shutdown = skl_be_shutdown, };
static struct snd_soc_dai_ops skl_link_dai_ops = { - .startup = skl_hda_be_startup, + .startup = skl_be_startup, .prepare = skl_link_pcm_prepare, .hw_params = skl_link_hw_params, .hw_free = skl_link_hw_free, .trigger = skl_link_pcm_trigger, - .shutdown = skl_hda_be_shutdown, + .shutdown = skl_be_shutdown, };
static struct snd_soc_dai_driver skl_platform_dai[] = { @@ -488,6 +539,24 @@ static struct snd_soc_dai_driver skl_platform_dai[] = { }, /* BE CPU Dais */ { + .name = "SSP0 Pin", + .ops = &skl_be_ssp_dai_ops, + .playback = { + .stream_name = "ssp0 Tx", + .channels_min = HDA_STEREO, + .channels_max = HDA_STEREO, + .rates = SNDRV_PCM_RATE_48000, + .formats = SNDRV_PCM_FMTBIT_S16_LE, + }, + .capture = { + .stream_name = "ssp0 Rx", + .channels_min = HDA_STEREO, + .channels_max = HDA_STEREO, + .rates = SNDRV_PCM_RATE_48000, + .formats = SNDRV_PCM_FMTBIT_S16_LE, + }, +}, +{ .name = "iDisp Pin", .ops = &skl_link_dai_ops, .playback = { @@ -577,7 +646,7 @@ static int skl_platform_open(struct snd_pcm_substream *substream) return 0; }
-static int skl_pcm_trigger(struct snd_pcm_substream *substream, +static int skl_coupled_trigger(struct snd_pcm_substream *substream, int cmd) { struct hdac_ext_bus *ebus = get_bus_ctx(substream); @@ -651,7 +720,7 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream, return 0; }
-static int skl_dsp_trigger(struct snd_pcm_substream *substream, +static int skl_decoupled_trigger(struct snd_pcm_substream *substream, int cmd) { struct hdac_ext_bus *ebus = get_bus_ctx(substream); @@ -708,9 +777,9 @@ static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream, struct hdac_ext_bus *ebus = get_bus_ctx(substream);
if (ebus->ppcap) - return skl_dsp_trigger(substream, cmd); + return skl_decoupled_trigger(substream, cmd); else - return skl_pcm_trigger(substream, cmd); + return skl_coupled_trigger(substream, cmd); }
/* calculate runtime delay from LPIB */ @@ -877,7 +946,17 @@ static int skl_pcm_new(struct snd_soc_pcm_runtime *rtd) return retval; }
+static int skl_platform_soc_probe(struct snd_soc_platform *platform) +{ + struct hdac_ext_bus *ebus = dev_get_drvdata(platform->dev); + + if (ebus->ppcap) + return skl_tplg_init(platform, ebus); + + return 0; +} static struct snd_soc_platform_driver skl_platform_drv = { + .probe = skl_platform_soc_probe, .ops = &skl_platform_ops, .pcm_new = skl_pcm_new, .pcm_free = skl_pcm_free, @@ -890,6 +969,11 @@ static const struct snd_soc_component_driver skl_component = { int skl_platform_register(struct device *dev) { int ret; + struct hdac_ext_bus *ebus = dev_get_drvdata(dev); + struct skl *skl = ebus_to_skl(ebus); + + INIT_LIST_HEAD(&skl->ppl_list); + INIT_LIST_HEAD(&skl->dapm_path_list);
ret = snd_soc_register_platform(dev, &skl_platform_drv); if (ret) {
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
Initialize and creates DSP controls if processing pipe capability is supported by HW. Updates the dma_id, hw_params to module param to be used when DSP module has to be configured.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
sound/soc/intel/skylake/skl-pcm.c | 138 ++++++++++++++++++++++++++++++-------- 1 file changed, 111 insertions(+), 27 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 7d617bf493bc..3e491149bf0c 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -24,6 +24,7 @@ #include <sound/pcm_params.h> #include <sound/soc.h> #include "skl.h" +#include "skl-topology.h"
#define HDA_MONO 1 #define HDA_STEREO 2 @@ -214,6 +215,7 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream, struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev); struct hdac_ext_stream *stream = get_hdac_ext_stream(substream); struct snd_pcm_runtime *runtime = substream->runtime;
struct skl_pipe_params p_params = {0}; int ret, dma_id;
dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
@@ -228,6 +230,13 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream, dma_id = hdac_stream(stream)->stream_tag - 1; dev_dbg(dai->dev, "dma_id=%d\n", dma_id);
- p_params.s_fmt = snd_pcm_format_width(params_format(params));
- p_params.ch = params_channels(params);
- p_params.s_freq = params_rate(params);
- p_params.host_dma_id = dma_id;
- p_params.stream = substream->stream;
- skl_tplg_fe_update_params(dai, &p_params);
- return 0;
}
@@ -268,6 +277,46 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream, return skl_substream_free_pages(ebus_to_hbus(ebus), substream); }
+static int skl_be_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct skl_pipe_params p_params = {0};
- dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
Seeing a lot of dev_dbg(__func__) in this series that really are just tracing. Probably best to trace the calls properly or remove if it's just development debugging.
Liam
On Fri, Sep 18, 2015 at 10:58:54AM +0100, Liam Girdwood wrote:
+static int skl_be_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct skl_pipe_params p_params = {0};
- dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
Seeing a lot of dev_dbg(__func__) in this series that really are just tracing. Probably best to trace the calls properly or remove if it's just development debugging.
Yes tracing will be added later, I will remove bunch of these but keep few to help debug as we are still adding features :)
On Fri, Sep 18, 2015 at 10:58:54AM +0100, Liam Girdwood wrote:
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
struct snd_soc_dai *dai)+{
- struct skl_pipe_params p_params = {0};
- dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
Seeing a lot of dev_dbg(__func__) in this series that really are just tracing. Probably best to trace the calls properly or remove if it's just development debugging.
It's all through the whole Intel DSP codebase. :(
On Sat, Sep 19, 2015 at 09:26:20AM -0700, Mark Brown wrote:
On Fri, Sep 18, 2015 at 10:58:54AM +0100, Liam Girdwood wrote:
On Mon, 2015-08-17 at 22:56 +0530, Vinod Koul wrote:
struct snd_soc_dai *dai)+{
- struct skl_pipe_params p_params = {0};
- dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
Seeing a lot of dev_dbg(__func__) in this series that really are just tracing. Probably best to trace the calls properly or remove if it's just development debugging.
It's all through the whole Intel DSP codebase. :(
Yes the code is getting features added so we thought of keeping this for a while, but I agree this doesn't help much, we are adding tracing anyway so lets add that. Will remove most of these...
Thanks
From: Jeeja KP jeeja.kp@intel.com
If processing pipe capability is supported, add DSP support. Adds initialization/free/suspend/resume DSP functionality.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 348d094e81d6..2f1890e703c6 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -166,11 +166,16 @@ static int skl_runtime_suspend(struct device *dev) struct pci_dev *pci = to_pci_dev(dev); struct hdac_ext_bus *ebus = pci_get_drvdata(pci); struct hdac_bus *bus = ebus_to_hbus(ebus); + struct skl *skl = ebus_to_skl(ebus); + int ret;
dev_dbg(bus->dev, "in %s\n", __func__);
/* enable controller wake up event */ snd_hdac_chip_updatew(bus, WAKEEN, 0, STATESTS_INT_MASK); + ret = skl_suspend_dsp(skl); + if (ret < 0) + return ret;
snd_hdac_bus_stop_chip(bus); snd_hdac_bus_enter_link_reset(bus); @@ -183,7 +188,7 @@ static int skl_runtime_resume(struct device *dev) struct pci_dev *pci = to_pci_dev(dev); struct hdac_ext_bus *ebus = pci_get_drvdata(pci); struct hdac_bus *bus = ebus_to_hbus(ebus); - struct skl *hda = ebus_to_skl(ebus); + struct skl *skl = ebus_to_skl(ebus); int status;
dev_dbg(bus->dev, "in %s\n", __func__); @@ -191,12 +196,12 @@ static int skl_runtime_resume(struct device *dev) /* Read STATESTS before controller reset */ status = snd_hdac_chip_readw(bus, STATESTS);
- skl_init_pci(hda); + skl_init_pci(skl); snd_hdac_bus_init_chip(bus, true); /* disable controller Wake Up event */ snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
- return 0; + return skl_resume_dsp(skl); } #endif /* CONFIG_PM */
@@ -457,17 +462,19 @@ static int skl_probe(struct pci_dev *pci,
/* check if dsp is there */ if (ebus->ppcap) { - /* TODO register with dsp IPC */ - dev_dbg(bus->dev, "Register dsp\n"); + err = skl_init_dsp(skl); + if (err < 0) { + dev_dbg(bus->dev, "error failed to register dsp\n"); + goto out_free; + } } - if (ebus->mlcap) snd_hdac_ext_bus_get_ml_capabilities(ebus);
/* create device for soc dmic */ err = skl_dmic_device_register(skl); if (err < 0) - goto out_free; + goto out_dsp_free;
/* register platform dai and controls */ err = skl_platform_register(bus->dev); @@ -491,6 +498,8 @@ out_unregister: skl_platform_unregister(bus->dev); out_dmic_free: skl_dmic_device_unregister(skl); +out_dsp_free: + skl_free_dsp(skl); out_free: skl->init_failed = 1; skl_free(ebus); @@ -507,6 +516,7 @@ static void skl_remove(struct pci_dev *pci) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci); skl_platform_unregister(&pci->dev); + skl_free_dsp(skl); skl_dmic_device_unregister(skl); skl_free(ebus); dev_set_drvdata(&pci->dev, NULL);
From: Jeeja KP jeeja.kp@intel.com
Load and Initialize Non HDA Link Table in Skylake driver to get platform configuration.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com --- sound/soc/intel/skylake/skl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 2f1890e703c6..ca135b8ab5c0 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -458,6 +458,11 @@ static int skl_probe(struct pci_dev *pci, if (err < 0) goto out_free;
+ skl->nhlt = skl_nhlt_init(bus->dev); + + if (skl->nhlt == NULL) + goto out_free; + pci_set_drvdata(skl->pci, ebus);
/* check if dsp is there */
On Mon, Aug 17, 2015 at 10:56:43PM +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
Load and Initialize Non HDA Link Table in Skylake driver to get platform configuration.
Didn't we need this before we started trying to use the NHLT?
On Sat, Sep 19, 2015 at 09:27:20AM -0700, Mark Brown wrote:
On Mon, Aug 17, 2015 at 10:56:43PM +0530, Vinod Koul wrote:
From: Jeeja KP jeeja.kp@intel.com
Load and Initialize Non HDA Link Table in Skylake driver to get platform configuration.
Didn't we need this before we started trying to use the NHLT?
We have only added NHLT code, not calls to it. Since the whole driver series is not merged we thought best to add enabling bits in later patches, so added here at last.
Thanks
From: Jeeja KP jeeja.kp@intel.com
We need to create CPU DAI for each endpoint instance. For this we should have one DMIC DAI, one HDA DAI and SSP DAI. Thus, DMIC23, HDA-SPK/AMIC was not required so this patch removes them
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-pcm.c | 33 --------------------------------- 1 file changed, 33 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 3e491149bf0c..c96411dc89f5 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -579,17 +579,6 @@ static struct snd_soc_dai_driver skl_platform_dai[] = { }, }, { - .name = "DMIC23 Pin", - .ops = &skl_dmic_dai_ops, - .capture = { - .stream_name = "DMIC23 Rx", - .channels_min = HDA_STEREO, - .channels_max = HDA_STEREO, - .rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000, - .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE, - }, -}, -{ .name = "HD-Codec Pin", .ops = &skl_link_dai_ops, .playback = { @@ -607,28 +596,6 @@ static struct snd_soc_dai_driver skl_platform_dai[] = { .formats = SNDRV_PCM_FMTBIT_S16_LE, }, }, -{ - .name = "HD-Codec-SPK Pin", - .ops = &skl_link_dai_ops, - .playback = { - .stream_name = "HD-Codec-SPK Tx", - .channels_min = HDA_STEREO, - .channels_max = HDA_STEREO, - .rates = SNDRV_PCM_RATE_48000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, - }, -}, -{ - .name = "HD-Codec-AMIC Pin", - .ops = &skl_link_dai_ops, - .capture = { - .stream_name = "HD-Codec-AMIC Rx", - .channels_min = HDA_STEREO, - .channels_max = HDA_STEREO, - .rates = SNDRV_PCM_RATE_48000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, - }, -}, };
static int skl_platform_open(struct snd_pcm_substream *substream)
On Mon, Aug 17, 2015 at 10:56:35PM +0530, Vinod Koul wrote:
The SKL driver does not code DSP topology in driver. With this series the ASoC topology framework is used to parse the topology information to create widgets, controls and route map for the FW topology.
It adds routines for SKL DSP module configuration, compute resources for modules, initialize and bind the pipe modules. It uses the SKL IPC library added earlier to send IPC for initialize the module, bind/unbind modules. Last patch in this series removes the unused dais.
Hi Mark,
I was wondering if you have any feedback on this series ?
Thanks
On Thu, Sep 03, 2015 at 01:44:54PM +0530, Vinod Koul wrote:
I was wondering if you have any feedback on this series ?
Please don't send content free pings (as I'm sure I've mentioned before). I've not looked at this yet because it's another large, invasive patch series which you sent at the very end of the development cycle.
On Fri, Sep 11, 2015 at 12:45:00PM +0100, Mark Brown wrote:
On Thu, Sep 03, 2015 at 01:44:54PM +0530, Vinod Koul wrote:
I was wondering if you have any feedback on this series ?
Please don't send content free pings (as I'm sure I've mentioned before). I've not looked at this yet because it's another large, invasive patch series which you sent at the very end of the development cycle.
So, I've now looked at these, though not fully. To expand on the above a bit: these are big bits of very device specific code that's really hard to read and understand and when I do read it I'm seeing similar issues again and again both in terms of big picture stuff and the trivial details (including the inconsistencies in things like commenting style that I'm not specifically pulling up). I'm really worried that I'm often seeing fragments of things that may or may not work well together.
These Intel devices not only have a lot more code than most devices do, they also get a lot more exposure than most devices do to end users so I actually have to try to make some effort to understand what's going on and can't skimp on review even though I feel like I am. I'm really concerned about where we're heading here and how robustly the results are going to work.
This all really slows down review, both directly in requiring detailed checks of hard to follow the code and as a result of making almost all other code review seem much more productive.
As well as the low level code quality issues it would really help if rather than continually sending large blocks of functionality we were starting from something that was a minimally viable but functional system and then extending that. That should be a lot easier to follow.
On Sat, Sep 19, 2015 at 09:56:12AM -0700, Mark Brown wrote:
On Fri, Sep 11, 2015 at 12:45:00PM +0100, Mark Brown wrote:
On Thu, Sep 03, 2015 at 01:44:54PM +0530, Vinod Koul wrote:
I was wondering if you have any feedback on this series ?
Please don't send content free pings (as I'm sure I've mentioned before). I've not looked at this yet because it's another large, invasive patch series which you sent at the very end of the development cycle.
So, I've now looked at these, though not fully. To expand on the above a bit: these are big bits of very device specific code that's really hard to read and understand and when I do read it I'm seeing similar issues again and again both in terms of big picture stuff and the trivial details (including the inconsistencies in things like commenting style that I'm not specifically pulling up). I'm really worried that I'm often seeing fragments of things that may or may not work well together.
These Intel devices not only have a lot more code than most devices do, they also get a lot more exposure than most devices do to end users so I actually have to try to make some effort to understand what's going on and can't skimp on review even though I feel like I am. I'm really concerned about where we're heading here and how robustly the results are going to work.
This all really slows down review, both directly in requiring detailed checks of hard to follow the code and as a result of making almost all other code review seem much more productive.
As well as the low level code quality issues it would really help if rather than continually sending large blocks of functionality we were starting from something that was a minimally viable but functional system and then extending that. That should be a lot easier to follow.
Thanks Mark for the review,
I wouldn't have pinged but had been more than few weeks so was wondering if this got missed. I did realise you were travelling and merge window came up, but still thought Mark's typical turnaound is 1-2 weeks so was expecting something. I do understand this take time but no harm in checking if it is beyond usual window one expects.
Now on the code due to inherent nature of HW (we have HDA + DSP + I2S!!!) and SW scalability, we do need lots of code for basic audio. Btw _this_ patch series is the last one to get basic minimal audio out of SKL. After this we will add machine and then features...
We did decide to use DPCM, DAPM and topology here to solve manging the DSP using these frameworks. yes hard coding a simple mixer and two pipes could have made code look easy to follow but then we would be redoing those bits as we scale these to defferent designs.
And to provide what we are trying here I have attached the SKL Topology conf file to give a picture of topology we are trying to build as a reference.
On code issues, I will fix the style and wrappers issues you raised and other instances where this might be required.
Also where you feel something is missing please do ask. Sometimes things are perceived intuitive for us as we are doing this for quire some time but may not be so for fresh eyes :)
Thanks
On Mon, Sep 21, 2015 at 09:27:39AM +0530, Vinod Koul wrote:
I wouldn't have pinged but had been more than few weeks so was wondering if this got missed. I did realise you were travelling and merge window came up, but still thought Mark's typical turnaound is 1-2 weeks so was expecting something. I do understand this take time but no harm in checking if it is beyond usual window one expects.
I'm also not going to merge large patches which have wide impact close to the merge window and I don't tend to review things without a view to merging them, especially not things that are more work to review.
We did decide to use DPCM, DAPM and topology here to solve manging the DSP using these frameworks. yes hard coding a simple mixer and two pipes could have made code look easy to follow but then we would be redoing those bits as we scale these to defferent designs.
Sure, and my point here is that that this process of redoing things would most likely be a lot faster since it'd be a lot easier to follow which would make the review a lot easier.
And to provide what we are trying here I have attached the SKL Topology conf file to give a picture of topology we are trying to build as a reference.
I think you missed the attachment here...
Also where you feel something is missing please do ask. Sometimes things are perceived intuitive for us as we are doing this for quire some time but may not be so for fresh eyes :)
I have been doing this mostly but it's easy to miss things without the picture of where we're heading.
On Mon, Sep 21, 2015 at 09:33:53AM -0700, Mark Brown wrote:
On Mon, Sep 21, 2015 at 09:27:39AM +0530, Vinod Koul wrote:
We did decide to use DPCM, DAPM and topology here to solve manging the DSP using these frameworks. yes hard coding a simple mixer and two pipes could have made code look easy to follow but then we would be redoing those bits as we scale these to defferent designs.
Sure, and my point here is that that this process of redoing things would most likely be a lot faster since it'd be a lot easier to follow which would make the review a lot easier.
And to provide what we are trying here I have attached the SKL Topology conf file to give a picture of topology we are trying to build as a reference.
I think you missed the attachment here...
Oops sorry about that, here you go
participants (3)
-
Liam Girdwood -
Mark Brown -
Vinod Koul