From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934696AbeEWVWl (ORCPT ); Wed, 23 May 2018 17:22:41 -0400 Received: from mail-yw0-f195.google.com ([209.85.161.195]:33387 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933886AbeEWVWh (ORCPT ); Wed, 23 May 2018 17:22:37 -0400 X-Google-Smtp-Source: AB8JxZpC2e+3xCtgJVxCx7KdH8aW79cOh7/JRA6tNjy3JAIhotCAlHAjMl8K0/1L1fGAME6HaBd3NZQnCF2ZCt9hI3Q= MIME-Version: 1.0 References: <20180522165842.233949-1-groeck@google.com> <649b1c14-e440-0c89-a59c-dc663344faa3@linux.intel.com> In-Reply-To: <649b1c14-e440-0c89-a59c-dc663344faa3@linux.intel.com> From: Guenter Roeck Date: Wed, 23 May 2018 14:22:24 -0700 Message-ID: Subject: Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files To: pierre-louis.bossart@linux.intel.com Cc: Liam Girdwood , alsa-devel@alsa-project.org, linux-kernel , Takashi Iwai , Mark Brown , "Patel, Chintan M" , Guenter Roeck Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 23, 2018 at 1:28 PM Pierre-Louis Bossart < pierre-louis.bossart@linux.intel.com> wrote: > On 05/22/2018 11:58 AM, Guenter Roeck wrote: > > From: Guenter Roeck > > > > Commit dc31e741db49 ("ASoC: topology: ABI - Add the types for BE > > DAI") introduced sound topology files version 5. Initially, this > > change made the topology code incompatible with v4 topology files. > > Backwards compatibility with v4 configuration files was > > subsequently added with commit 288b8da7e992 ("ASoC: topology: > > Support topology file of ABI v4"). > > > > Unfortunately, backwards compatibility was never fully implemented. > > > > First, the manifest size in (Skylake) v4 configuration files is set > > to 0, which causes manifest_new_ver() to bail out with error messages > > similar to the following. > > > > snd_soc_skl 0000:00:1f.3: ASoC: invalid manifest size > > snd_soc_skl 0000:00:1f.3: tplg component load failed-22 > > snd_soc_skl 0000:00:1f.3: Failed to init topology! > > snd_soc_skl 0000:00:1f.3: ASoC: failed to probe component -22 > > skl_n88l25_m98357a skl_n88l25_m98357a: ASoC: failed to instantiate card -22 > > skl_n88l25_m98357a: probe of skl_n88l25_m98357a failed with error -22 > > > > After this problem is fixed, the following error message is seen instead. > > > > snd_soc_skl 0000:00:1f.3: ASoC: old version of manifest > > snd_soc_skl 0000:00:1f.3: Invalid descriptor token 1093938482 > > snd_soc_skl 0000:00:1f.3: ASoC: failed to load widget media0_in cpr 0 > > snd_soc_skl 0000:00:1f.3: tPlg component load failed-22 > > > > This message is seen because backwards compatibility for loading widgets > > was never implemented. > > > > Both problems have been widely reported. Various attempts were made to > > fix the problem, usually by providing v5 configuration files. However, > > all those attempts result in incomplete ASoC configuration. > > > > Let's implement backward compatibility properly to solve the problem > > for good. > > > > Signed-off-by: Guenter Roeck > I don't have any devices on which I can test but the code looks ok > compared to chromeos-3.18 (minor comments below). > Reviewed-by: Pierre-Louis Bossart > > --- > > Tested on Caroline (Samsung Chromebook Pro) running v4.17-rc6 plus > > this patch, with original (v4) configuration file. Also tested on > > several other Chromebooks with this patch on top of chromeos-4.14. > > > > Additional real world test coverage would be useful before moving forward. > > > > sound/soc/intel/skylake/skl-topology.c | 235 +++++++++++++++++++++++++ > > sound/soc/soc-topology.c | 7 +- > > 2 files changed, 240 insertions(+), 2 deletions(-) > > > > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c > > index 3b1dca419883..cc81e200e6b7 100644 > > --- a/sound/soc/intel/skylake/skl-topology.c > > +++ b/sound/soc/intel/skylake/skl-topology.c > > @@ -19,6 +19,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -30,6 +31,79 @@ > > #include "../common/sst-dsp.h" > > #include "../common/sst-dsp-priv.h" > > > > +/* v4 configuration data */ > > +struct skl_dfw_v4_module_pin { > > + u16 module_id; > > + u16 instance_id; > > +} __packed; > > + > > +struct skl_dfw_v4_module_fmt { > > + u32 channels; > > + u32 freq; > > + u32 bit_depth; > > + u32 valid_bit_depth; > > + u32 ch_cfg; > > + u32 interleaving_style; > > + u32 sample_type; > > + u32 ch_map; > > +} __packed; > > + > > +struct skl_dfw_v4_module_caps { > > + u32 set_params:2; > > + u32 rsvd:30; > > + u32 param_id; > > + u32 caps_size; > > + u32 caps[HDA_SST_CFG_MAX]; > > +}; > > + > > +struct skl_dfw_v4_pipe { > > + u8 pipe_id; > > + u8 pipe_priority; > > + u16 conn_type:4; > > + u16 rsvd:4; > > + u16 memory_pages:8; > > +} __packed; > > + > > +struct skl_dfw_v4_module { > > + char uuid[SKL_UUID_STR_SZ]; > > + > > + u16 module_id; > > + u16 instance_id; > > + u32 max_mcps; > > + u32 mem_pages; > > + u32 obs; > > + u32 ibs; > > + u32 vbus_id; > > + > > + u32 max_in_queue:8; > > + u32 max_out_queue:8; > > + u32 time_slot:8; > > + u32 core_id:4; > > + u32 rsvd1:4; > > + > > + u32 module_type:8; > > + u32 conn_type:4; > > + u32 dev_type:4; > > + u32 hw_conn_type:4; > > + u32 rsvd2:12; > > + > > + u32 params_fixup:8; > > + u32 converter:8; > > + u32 input_pin_type:1; > > + u32 output_pin_type:1; > > + u32 is_dynamic_in_pin:1; > > + u32 is_dynamic_out_pin:1; > > + u32 is_loadable:1; > > + u32 rsvd3:11; > > + > > + struct skl_dfw_v4_pipe pipe; > > + struct skl_dfw_v4_module_fmt in_fmt[MAX_IN_QUEUE]; > > + struct skl_dfw_v4_module_fmt out_fmt[MAX_OUT_QUEUE]; > > + struct skl_dfw_v4_module_pin in_pin[MAX_IN_QUEUE]; > > + struct skl_dfw_v4_module_pin out_pin[MAX_OUT_QUEUE]; > > + struct skl_dfw_v4_module_caps caps; > > +} __packed; > > + > All the structures match what I can see in sof-topology.h for chromeos-3.18. > So far so good. > > #define SKL_CH_FIXUP_MASK (1 << 0) > > #define SKL_RATE_FIXUP_MASK (1 << 1) > > #define SKL_FMT_FIXUP_MASK (1 << 2) > > @@ -2724,6 +2798,160 @@ static int skl_tplg_get_desc_blocks(struct device *dev, > > return -EINVAL; > > } > > > > +/* > > + * 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 int skl_tplg_add_pipe_v4(struct device *dev, > > + struct skl_module_cfg *mconfig, struct skl *skl, > > + struct skl_dfw_v4_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) { > > + mconfig->pipe = ppl->pipe; > > + return 0; > > + } > > + } > > + > > + ppl = devm_kzalloc(dev, sizeof(*ppl), GFP_KERNEL); > > + if (!ppl) > > + return -ENOMEM; > > + > > + pipe = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL); > > + if (!pipe) > > + return -ENOMEM; > > + > > + params = devm_kzalloc(dev, sizeof(*params), GFP_KERNEL); > > + if (!params) > > + return -ENOMEM; > > + > > + 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); > > + > > + mconfig->pipe = pipe; > > + mconfig->pipe->state = SKL_PIPE_INVALID; > That last assignment does not seem necessary? pipe->state is already set > above. You are correct. Removed. > > + > > + return 0; > > +} > > + > > +static void skl_fill_module_pin_info_v4(struct skl_dfw_v4_module_pin *dfw_pin, > > + struct skl_module_pin *m_pin, > > + bool is_dynamic, int max_pin) > > +{ > > + int i; > > + > > + for (i = 0; i < max_pin; i++) { > > + m_pin[i].id.module_id = dfw_pin[i].module_id; > > + m_pin[i].id.instance_id = dfw_pin[i].instance_id; > > + m_pin[i].in_use = false; > > + m_pin[i].is_dynamic = is_dynamic; > > + m_pin[i].pin_state = SKL_PIN_UNBIND; > > + } > > +} > > + > > +static void skl_tplg_fill_fmt_v4(struct skl_module_pin_fmt *dst_fmt, > > + struct skl_dfw_v4_module_fmt *src_fmt, > > + int pins) > > +{ > > + int i; > > + > > + for (i = 0; i < pins; i++) { > > + dst_fmt[i].fmt.channels = src_fmt[i].channels; > > + dst_fmt[i].fmt.s_freq = src_fmt[i].freq; > > + dst_fmt[i].fmt.bit_depth = src_fmt[i].bit_depth; > > + dst_fmt[i].fmt.valid_bit_depth = src_fmt[i].valid_bit_depth; > > + dst_fmt[i].fmt.ch_cfg = src_fmt[i].ch_cfg; > > + dst_fmt[i].fmt.ch_map = src_fmt[i].ch_map; > > + dst_fmt[i].fmt.interleaving_style = > > + src_fmt[i].interleaving_style; > > + dst_fmt[i].fmt.sample_type = src_fmt[i].sample_type; > > + } > > +} > > + > > +static int skl_tplg_get_pvt_data_v4(struct snd_soc_tplg_dapm_widget *tplg_w, > > + struct skl *skl, struct device *dev, > > + struct skl_module_cfg *mconfig) > > +{ > > + struct skl_dfw_v4_module *dfw = > > + (struct skl_dfw_v4_module *)tplg_w->priv.data; > > + int ret; > > + > > + dev_dbg(dev, "Parsing Skylake v4 widget topology data\n"); > > + > > + ret = guid_parse(dfw->uuid, (guid_t *)mconfig->guid); > > + if (ret) > > + return ret; > > + mconfig->id.module_id = -1; > > + mconfig->id.instance_id = dfw->instance_id; > > + mconfig->module->resources[0].cps = dfw->max_mcps; > > + mconfig->module->resources[0].ibs = dfw->ibs; > > + mconfig->module->resources[0].obs = dfw->obs; > > + mconfig->core_id = dfw->core_id; > > + mconfig->module->max_input_pins = dfw->max_in_queue; > > + mconfig->module->max_output_pins = dfw->max_out_queue; > > + mconfig->module->loadable = dfw->is_loadable; > > + skl_tplg_fill_fmt_v4(mconfig->module->formats[0].inputs, dfw->in_fmt, > > + MAX_IN_QUEUE); > > + skl_tplg_fill_fmt_v4(mconfig->module->formats[0].outputs, dfw->out_fmt, > > + MAX_OUT_QUEUE); > Not clear to me if there is a confusion between MAX_IN_QUEUE and > MODULE_MAX_IN_PINS. The two values happen to be identical. The target (mconfig->module->formats[]) size is MAX_IN_QUEUE. Upstream v4.4/v4.5 use both defines interchangeably as far as I can see. sound/soc/intel/skylake/skl-topology.h: struct skl_module_fmt in_fmt[MODULE_MAX_IN_PINS]; sound/soc/intel/skylake/skl-tplg-interface.h: struct skl_dfw_module_fmt in_fmt[MAX_IN_QUEUE]; I could make it min(MAX_IN_QUEUE, dfw->max_in_queue) Would that be better ? > > + > > + mconfig->params_fixup = dfw->params_fixup; > > + mconfig->converter = dfw->converter; > > + mconfig->m_type = dfw->module_type; > > + mconfig->vbus_id = dfw->vbus_id; > > + mconfig->module->resources[0].is_pages = dfw->mem_pages; > > + > > + ret = skl_tplg_add_pipe_v4(dev, mconfig, skl, &dfw->pipe); > > + if (ret) > > + return ret; > > + > > + mconfig->dev_type = dfw->dev_type; > > + mconfig->hw_conn_type = dfw->hw_conn_type; > > + mconfig->time_slot = dfw->time_slot; > > + mconfig->formats_config.caps_size = dfw->caps.caps_size; > chromeos-3.18 has this: > if (dfw_config->is_loadable) > memcpy(mconfig->guid, dfw_config->uuid, > ARRAY_SIZE(dfw_config->uuid)); > Is this needed here? Direct memcpy doesn't work anymore since the uuid format is different. The above is replaced with (unconditional) ret = guid_parse(dfw->uuid, (guid_t *)mconfig->guid); if (ret) return ret; at the beginning of skl_tplg_get_pvt_data_v4(). The new code, as far as I can see, loads the uuid unconditionally if it finds SND_SOC_TPLG_TUPLE_TYPE_UUID. I wanted to be on the safe side and decided to do the same. Thanks, Guenter > > + > > + mconfig->m_in_pin = devm_kzalloc(dev, > > + MAX_IN_QUEUE * sizeof(*mconfig->m_in_pin), > > + GFP_KERNEL); > > + if (!mconfig->m_in_pin) > > + return -ENOMEM; > > + > > + mconfig->m_out_pin = devm_kzalloc(dev, > > + MAX_OUT_QUEUE * sizeof(*mconfig->m_out_pin), > > + GFP_KERNEL); > > + if (!mconfig->m_out_pin) > > + return -ENOMEM; > > + > > + skl_fill_module_pin_info_v4(dfw->in_pin, mconfig->m_in_pin, > > + dfw->is_dynamic_in_pin, > > + mconfig->module->max_input_pins); > > + skl_fill_module_pin_info_v4(dfw->out_pin, mconfig->m_out_pin, > > + dfw->is_dynamic_out_pin, > > + mconfig->module->max_output_pins); > > + > > + if (mconfig->formats_config.caps_size) { > > + dev_warn(dev, "caps size %d not supported, will be ignored\n", > > + mconfig->formats_config.caps_size); > > + mconfig->formats_config.caps_size = 0; > > + } > > + > > + return 0; > > +} > > + > > /* > > * Parse the private data for the token and corresponding value. > > * The private data can have multiple data blocks. So, a data block > > @@ -2739,6 +2967,13 @@ static int skl_tplg_get_pvt_data(struct snd_soc_tplg_dapm_widget *tplg_w, > > char *data; > > int ret; > > > > + /* > > + * v4 configuration files have a valid UUID at the start of > > + * the widget's private data. > > + */ > > + if (uuid_is_valid((char *)tplg_w->priv.data)) > > + return skl_tplg_get_pvt_data_v4(tplg_w, skl, dev, mconfig); > > + > > /* Read the NUM_DATA_BLOCKS descriptor */ > > array = (struct snd_soc_tplg_vendor_array *)tplg_w->priv.data; > > ret = skl_tplg_get_desc_blocks(dev, array); > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > > index 986b8b2f90fb..d66b2e5ccd67 100644 > > --- a/sound/soc/soc-topology.c > > +++ b/sound/soc/soc-topology.c > > @@ -2293,8 +2293,11 @@ static int manifest_new_ver(struct soc_tplg *tplg, > > *manifest = NULL; > > > > if (src->size != sizeof(*src_v4)) { > > - dev_err(tplg->dev, "ASoC: invalid manifest size\n"); > > - return -EINVAL; > > + dev_warn(tplg->dev, "ASoC: invalid manifest size %d\n", > > + src->size); > > + if (src->size) > > + return -EINVAL; > > + src->size = sizeof(*src_v4); > > } > > > > dev_warn(tplg->dev, "ASoC: old version of manifest\n");