* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 16:58 [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files Guenter Roeck
@ 2018-05-22 17:14 ` Mark Brown
2018-05-22 19:59 ` [alsa-devel] " Pierre-Louis Bossart
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-22 17:14 UTC (permalink / raw)
To: Guenter Roeck
Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, alsa-devel,
linux-kernel, Chintan Patel, Guenter Roeck, Pierre-Louis Bossart
[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]
On Tue, May 22, 2018 at 09:58:42AM -0700, Guenter Roeck wrote:
> 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.
This is the first time I've heard that there were still ongoing issues,
it's extremely disappointing - I'm guessing the attempts have all been
at the distro level. Userspace fixes clearly aren't the thing to do
here, as discussed at the time the original fixes were done this is an
ABI and needs to be treated as such.
None of the systems I have access to have ever used topology so I've had
poor visibility of how this is working in practice, I guess I'm going to
start seeing this stuff next time I upgrade my laptop (unless someone is
making suitable non-x86 laptops by then!).
> Let's implement backward compatibility properly to solve the problem
> for good.
Thanks for doing this. I'll give this some time for review from the
Intel people - adding Pierre as well since this is a bit of a device
thing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-22 17:14 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-22 17:14 UTC (permalink / raw)
To: Guenter Roeck
Cc: Pierre-Louis Bossart, alsa-devel, Liam Girdwood, linux-kernel,
Takashi Iwai, Chintan Patel, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 1086 bytes --]
On Tue, May 22, 2018 at 09:58:42AM -0700, Guenter Roeck wrote:
> 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.
This is the first time I've heard that there were still ongoing issues,
it's extremely disappointing - I'm guessing the attempts have all been
at the distro level. Userspace fixes clearly aren't the thing to do
here, as discussed at the time the original fixes were done this is an
ABI and needs to be treated as such.
None of the systems I have access to have ever used topology so I've had
poor visibility of how this is working in practice, I guess I'm going to
start seeing this stuff next time I upgrade my laptop (unless someone is
making suitable non-x86 laptops by then!).
> Let's implement backward compatibility properly to solve the problem
> for good.
Thanks for doing this. I'll give this some time for review from the
Intel people - adding Pierre as well since this is a bit of a device
thing.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 16:58 [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files Guenter Roeck
2018-05-22 17:14 ` Mark Brown
@ 2018-05-22 19:59 ` Pierre-Louis Bossart
2018-05-22 21:59 ` Guenter Roeck
2018-05-23 8:24 ` Mark Brown
2018-05-23 13:54 ` Takashi Iwai
2018-05-23 20:28 ` [alsa-devel] " Pierre-Louis Bossart
3 siblings, 2 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2018-05-22 19:59 UTC (permalink / raw)
To: Guenter Roeck, Liam Girdwood
Cc: alsa-devel, linux-kernel, Takashi Iwai, Mark Brown,
Chintan Patel, Guenter Roeck
On 05/22/2018 11:58 AM, Guenter Roeck wrote:
> From: Guenter Roeck <groeck@chromium.org>
>
> 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.
Thanks for starting this. I'll ask other Intel folks to look at this.
I am surprised btw by the 'widely reported' attribute: a Google search
for "SKL v4 topology backwards-compatible" give me nothing but a pointer
to this patch and to the best of my knowledge this has not been reported
on alsa-devel, has it? What other platforms might be affected by this
problem?
I am also not convinced by the notion that maintaining topology files is
only a userspace/distro issue. This would mean some distros will have
access to the required topology files, possibly enabling DSP processing
capabilities, but other will not and will not be able to enable even
basic playback/capture. Just like we have a basic firmware with limited
functionality in /lib/firmware/intel, it would make sense to require a
basic .conf file in alsa-lib for every upstream machine driver - along
possibly with a basic UCM file so that audio works no matter what distro
people use.
>
> Signed-off-by: Guenter Roeck <groeck@chromium.org>
> ---
> 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 <linux/slab.h>
> #include <linux/types.h>
> #include <linux/firmware.h>
> +#include <linux/uuid.h>
> #include <sound/soc.h>
> #include <sound/soc-topology.h>
> #include <uapi/sound/snd_sst_tokens.h>
> @@ -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;
> +
> #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;
> +
> + 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);
> +
> + 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;
> +
> + 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");
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 19:59 ` [alsa-devel] " Pierre-Louis Bossart
@ 2018-05-22 21:59 ` Guenter Roeck
2018-05-23 9:49 ` Mark Brown
2018-05-23 8:24 ` Mark Brown
1 sibling, 1 reply; 42+ messages in thread
From: Guenter Roeck @ 2018-05-22 21:59 UTC (permalink / raw)
To: pierre-louis.bossart
Cc: alsa-devel, linux-kernel, Takashi Iwai, Liam Girdwood,
Mark Brown, Patel, Chintan M, Guenter Roeck
On Tue, May 22, 2018 at 12:59 PM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:
>
>
> On 05/22/2018 11:58 AM, Guenter Roeck wrote:
> > From: Guenter Roeck <groeck@chromium.org>
> >
> > 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.
> Thanks for starting this. I'll ask other Intel folks to look at this.
> I am surprised btw by the 'widely reported' attribute: a Google search
> for "SKL v4 topology backwards-compatible" give me nothing but a pointer
> to this patch and to the best of my knowledge this has not been reported
> on alsa-devel, has it? What other platforms might be affected by this
> problem?
>
>
Most people are not aware that they have a backwards compatibility problem
with the topology file. They will typically notice and report that it is
impossible to get audio working with an upstream kernel on a recent
Chromebook, and as users they rarely know about development mailing lists.
Some technical discussions are at
https://github.com/GalliumOS/galliumos-distro/issues/379
https://github.com/GalliumOS/galliumos-distro/issues/274
though I had seen others earlier. Guess next time I'll have to preserve
links. Anyway, I am not really much interested in an argument/discussion
about my use of the term "widely". I'll rephrase that paragraph in the next
version of the patch, should it come to that.
Guenter
I am also not convinced by the notion that maintaining topology files is
> only a userspace/distro issue. This would mean some distros will have
> access to the required topology files, possibly enabling DSP processing
> capabilities, but other will not and will not be able to enable even
> basic playback/capture. Just like we have a basic firmware with limited
> functionality in /lib/firmware/intel, it would make sense to require a
> basic .conf file in alsa-lib for every upstream machine driver - along
> possibly with a basic UCM file so that audio works no matter what distro
> people use.
>
> > Signed-off-by: Guenter Roeck <groeck@chromium.org>
> > ---
> > 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 <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/firmware.h>
> > +#include <linux/uuid.h>
> > #include <sound/soc.h>
> > #include <sound/soc-topology.h>
> > #include <uapi/sound/snd_sst_tokens.h>
> > @@ -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;
> > +
> > #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;
> > +
> > + 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);
> > +
> > + 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;
> > +
> > + 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");
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 21:59 ` Guenter Roeck
@ 2018-05-23 9:49 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 9:49 UTC (permalink / raw)
To: Guenter Roeck
Cc: pierre-louis.bossart, Liam Girdwood, alsa-devel, linux-kernel,
Takashi Iwai, Patel, Chintan M, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
On Tue, May 22, 2018 at 02:59:50PM -0700, Guenter Roeck wrote:
> Most people are not aware that they have a backwards compatibility problem
> with the topology file. They will typically notice and report that it is
> impossible to get audio working with an upstream kernel on a recent
> Chromebook, and as users they rarely know about development mailing lists.
> Some technical discussions are at
> https://github.com/GalliumOS/galliumos-distro/issues/379
> https://github.com/GalliumOS/galliumos-distro/issues/274
> though I had seen others earlier. Guess next time I'll have to preserve
> links. Anyway, I am not really much interested in an argument/discussion
> about my use of the term "widely". I'll rephrase that paragraph in the next
> version of the patch, should it come to that.
Right, looks like the disconnect I thought it was - this has been
discussed a lot within your distro (and possibly others) but not outside
it so the rest of us are all a bit surprised.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 9:49 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 9:49 UTC (permalink / raw)
To: Guenter Roeck
Cc: pierre-louis.bossart, alsa-devel, Liam Girdwood, linux-kernel,
Takashi Iwai, Patel, Chintan M, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 1001 bytes --]
On Tue, May 22, 2018 at 02:59:50PM -0700, Guenter Roeck wrote:
> Most people are not aware that they have a backwards compatibility problem
> with the topology file. They will typically notice and report that it is
> impossible to get audio working with an upstream kernel on a recent
> Chromebook, and as users they rarely know about development mailing lists.
> Some technical discussions are at
> https://github.com/GalliumOS/galliumos-distro/issues/379
> https://github.com/GalliumOS/galliumos-distro/issues/274
> though I had seen others earlier. Guess next time I'll have to preserve
> links. Anyway, I am not really much interested in an argument/discussion
> about my use of the term "widely". I'll rephrase that paragraph in the next
> version of the patch, should it come to that.
Right, looks like the disconnect I thought it was - this has been
discussed a lot within your distro (and possibly others) but not outside
it so the rest of us are all a bit surprised.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 9:49 ` Mark Brown
(?)
@ 2018-05-23 13:15 ` Guenter Roeck
-1 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 13:15 UTC (permalink / raw)
To: Mark Brown
Cc: pierre-louis.bossart, alsa-devel, Liam Girdwood, linux-kernel,
Takashi Iwai, Patel, Chintan M, Guenter Roeck
On Wed, May 23, 2018 at 2:49 AM Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 22, 2018 at 02:59:50PM -0700, Guenter Roeck wrote:
>
> > Most people are not aware that they have a backwards compatibility
> problem
> > with the topology file. They will typically notice and report that it is
> > impossible to get audio working with an upstream kernel on a recent
> > Chromebook, and as users they rarely know about development mailing
> lists.
> > Some technical discussions are at
>
> > https://github.com/GalliumOS/galliumos-distro/issues/379
> > https://github.com/GalliumOS/galliumos-distro/issues/274
>
> > though I had seen others earlier. Guess next time I'll have to preserve
> > links. Anyway, I am not really much interested in an argument/discussion
> > about my use of the term "widely". I'll rephrase that paragraph in the
> next
> > version of the patch, should it come to that.
>
> Right, looks like the disconnect I thought it was - this has been
> discussed a lot within your distro (and possibly others) but not outside
> it so the rest of us are all a bit surprised.
>
That isn't "my" distro. My distro, if anything, would be ChromeOS, and the
associated bug would be
https://bugs.chromium.org/p/chromium/issues/detail?id=786522.
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 19:59 ` [alsa-devel] " Pierre-Louis Bossart
@ 2018-05-23 8:24 ` Mark Brown
2018-05-23 8:24 ` Mark Brown
1 sibling, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 8:24 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Guenter Roeck, Liam Girdwood, alsa-devel, linux-kernel,
Takashi Iwai, Chintan Patel, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 927 bytes --]
On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
> I am also not convinced by the notion that maintaining topology files is
> only a userspace/distro issue. This would mean some distros will have access
> to the required topology files, possibly enabling DSP processing
> capabilities, but other will not and will not be able to enable even basic
> playback/capture. Just like we have a basic firmware with limited
> functionality in /lib/firmware/intel, it would make sense to require a basic
> .conf file in alsa-lib for every upstream machine driver - along possibly
> with a basic UCM file so that audio works no matter what distro people use.
The point here is that people should be able to update their kernel
without updating their userspace so things have to work with whatever
they have right now - anything that relies on shipping new firmware or
configuration files to userspace is a problem.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 8:24 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 8:24 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, Guenter Roeck, linux-kernel, Takashi Iwai,
Liam Girdwood, Chintan Patel, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 927 bytes --]
On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
> I am also not convinced by the notion that maintaining topology files is
> only a userspace/distro issue. This would mean some distros will have access
> to the required topology files, possibly enabling DSP processing
> capabilities, but other will not and will not be able to enable even basic
> playback/capture. Just like we have a basic firmware with limited
> functionality in /lib/firmware/intel, it would make sense to require a basic
> .conf file in alsa-lib for every upstream machine driver - along possibly
> with a basic UCM file so that audio works no matter what distro people use.
The point here is that people should be able to update their kernel
without updating their userspace so things have to work with whatever
they have right now - anything that relies on shipping new firmware or
configuration files to userspace is a problem.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 8:24 ` Mark Brown
@ 2018-05-23 13:42 ` Pierre-Louis Bossart
-1 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2018-05-23 13:42 UTC (permalink / raw)
To: Mark Brown
Cc: Guenter Roeck, Liam Girdwood, alsa-devel, linux-kernel,
Takashi Iwai, Chintan Patel, Guenter Roeck
On 5/23/18 3:24 AM, Mark Brown wrote:
> On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
>
>> I am also not convinced by the notion that maintaining topology files is
>> only a userspace/distro issue. This would mean some distros will have access
>> to the required topology files, possibly enabling DSP processing
>> capabilities, but other will not and will not be able to enable even basic
>> playback/capture. Just like we have a basic firmware with limited
>> functionality in /lib/firmware/intel, it would make sense to require a basic
>> .conf file in alsa-lib for every upstream machine driver - along possibly
>> with a basic UCM file so that audio works no matter what distro people use.
>
> The point here is that people should be able to update their kernel
> without updating their userspace so things have to work with whatever
> they have right now - anything that relies on shipping new firmware or
> configuration files to userspace is a problem.
Agree.
My point was a bit different: distributions like Gallium start without
the relevant topology files and UCM settings, and we should have a
reference to quickly enable audio without having to borrow and modify
files from another distro. I faced this issue when I worked with the
Gallium folks to enable audio on Rambi and Cyan Chromebooks and ended-up
creating this reference myself.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 13:42 ` Pierre-Louis Bossart
0 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2018-05-23 13:42 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Guenter Roeck, linux-kernel, Takashi Iwai,
Liam Girdwood, Chintan Patel, Guenter Roeck
On 5/23/18 3:24 AM, Mark Brown wrote:
> On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
>
>> I am also not convinced by the notion that maintaining topology files is
>> only a userspace/distro issue. This would mean some distros will have access
>> to the required topology files, possibly enabling DSP processing
>> capabilities, but other will not and will not be able to enable even basic
>> playback/capture. Just like we have a basic firmware with limited
>> functionality in /lib/firmware/intel, it would make sense to require a basic
>> .conf file in alsa-lib for every upstream machine driver - along possibly
>> with a basic UCM file so that audio works no matter what distro people use.
>
> The point here is that people should be able to update their kernel
> without updating their userspace so things have to work with whatever
> they have right now - anything that relies on shipping new firmware or
> configuration files to userspace is a problem.
Agree.
My point was a bit different: distributions like Gallium start without
the relevant topology files and UCM settings, and we should have a
reference to quickly enable audio without having to borrow and modify
files from another distro. I faced this issue when I worked with the
Gallium folks to enable audio on Rambi and Cyan Chromebooks and ended-up
creating this reference myself.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 13:42 ` Pierre-Louis Bossart
@ 2018-05-23 13:56 ` Takashi Iwai
-1 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 13:56 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Mark Brown, alsa-devel, Guenter Roeck, Liam Girdwood,
Guenter Roeck, Chintan Patel, linux-kernel
On Wed, 23 May 2018 15:42:59 +0200,
Pierre-Louis Bossart wrote:
>
> On 5/23/18 3:24 AM, Mark Brown wrote:
> > On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
> >
> >> I am also not convinced by the notion that maintaining topology files is
> >> only a userspace/distro issue. This would mean some distros will have access
> >> to the required topology files, possibly enabling DSP processing
> >> capabilities, but other will not and will not be able to enable even basic
> >> playback/capture. Just like we have a basic firmware with limited
> >> functionality in /lib/firmware/intel, it would make sense to require a basic
> >> .conf file in alsa-lib for every upstream machine driver - along possibly
> >> with a basic UCM file so that audio works no matter what distro people use.
> >
> > The point here is that people should be able to update their kernel
> > without updating their userspace so things have to work with whatever
> > they have right now - anything that relies on shipping new firmware or
> > configuration files to userspace is a problem.
>
> Agree.
>
> My point was a bit different: distributions like Gallium start without
> the relevant topology files and UCM settings, and we should have a
> reference to quickly enable audio without having to borrow and modify
> files from another distro. I faced this issue when I worked with the
> Gallium folks to enable audio on Rambi and Cyan Chromebooks and
> ended-up creating this reference myself.
This sounds rather like a question how the upstream distributes the
stuff properly...
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 13:56 ` Takashi Iwai
0 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 13:56 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Mark Brown, alsa-devel, Guenter Roeck, Liam Girdwood,
Guenter Roeck, Chintan Patel, linux-kernel
On Wed, 23 May 2018 15:42:59 +0200,
Pierre-Louis Bossart wrote:
>
> On 5/23/18 3:24 AM, Mark Brown wrote:
> > On Tue, May 22, 2018 at 02:59:35PM -0500, Pierre-Louis Bossart wrote:
> >
> >> I am also not convinced by the notion that maintaining topology files is
> >> only a userspace/distro issue. This would mean some distros will have access
> >> to the required topology files, possibly enabling DSP processing
> >> capabilities, but other will not and will not be able to enable even basic
> >> playback/capture. Just like we have a basic firmware with limited
> >> functionality in /lib/firmware/intel, it would make sense to require a basic
> >> .conf file in alsa-lib for every upstream machine driver - along possibly
> >> with a basic UCM file so that audio works no matter what distro people use.
> >
> > The point here is that people should be able to update their kernel
> > without updating their userspace so things have to work with whatever
> > they have right now - anything that relies on shipping new firmware or
> > configuration files to userspace is a problem.
>
> Agree.
>
> My point was a bit different: distributions like Gallium start without
> the relevant topology files and UCM settings, and we should have a
> reference to quickly enable audio without having to borrow and modify
> files from another distro. I faced this issue when I worked with the
> Gallium folks to enable audio on Rambi and Cyan Chromebooks and
> ended-up creating this reference myself.
This sounds rather like a question how the upstream distributes the
stuff properly...
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 13:56 ` Takashi Iwai
(?)
@ 2018-05-23 14:49 ` Guenter Roeck
2018-05-23 14:58 ` Takashi Iwai
2018-05-23 15:52 ` [alsa-devel] " Mark Brown
-1 siblings, 2 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 14:49 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, linux-kernel, pierre-louis.bossart, Liam Girdwood,
Mark Brown, Patel, Chintan M, Guenter Roeck
On Wed, May 23, 2018 at 6:56 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> >
> > My point was a bit different: distributions like Gallium start without
> > the relevant topology files and UCM settings, and we should have a
> > reference to quickly enable audio without having to borrow and modify
> > files from another distro. I faced this issue when I worked with the
> > Gallium folks to enable audio on Rambi and Cyan Chromebooks and
> > ended-up creating this reference myself.
>
> This sounds rather like a question how the upstream distributes the
> stuff properly...
>
Not really, or not only. My problem, in particular, is that I want (need)
to be able to run different kernel versions with the same distribution, and
I want to be able to swap kernel versions without having to update the root
file system. That doesn't work if I have to use v4 configuration files for
v4.4 and older kernels, and v5 configuration files for v4.9 and later
kernels (besides, as I mentioned, that the v5 configuration files I found
were all incomplete). In my opinion, thre should be exactly one
configuration file for a given device, and there should be no need for
users (or distributions) to hunt for the correct version of the file.
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 14:49 ` Guenter Roeck
@ 2018-05-23 14:58 ` Takashi Iwai
2018-05-23 15:52 ` [alsa-devel] " Mark Brown
1 sibling, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 14:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: pierre-louis.bossart, Mark Brown, alsa-devel, Guenter Roeck,
Liam Girdwood, Patel, Chintan M, linux-kernel
On Wed, 23 May 2018 16:49:35 +0200,
Guenter Roeck wrote:
>
> On Wed, May 23, 2018 at 6:56 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > >
> > > My point was a bit different: distributions like Gallium start without
> > > the relevant topology files and UCM settings, and we should have a
> > > reference to quickly enable audio without having to borrow and modify
> > > files from another distro. I faced this issue when I worked with the
> > > Gallium folks to enable audio on Rambi and Cyan Chromebooks and
> > > ended-up creating this reference myself.
> >
> > This sounds rather like a question how the upstream distributes the
> > stuff properly...
> >
> Not really, or not only. My problem, in particular, is that I want (need)
> to be able to run different kernel versions with the same distribution, and
> I want to be able to swap kernel versions without having to update the root
> file system. That doesn't work if I have to use v4 configuration files for
> v4.4 and older kernels, and v5 configuration files for v4.9 and later
> kernels (besides, as I mentioned, that the v5 configuration files I found
> were all incomplete). In my opinion, thre should be exactly one
> configuration file for a given device, and there should be no need for
> users (or distributions) to hunt for the correct version of the file.
No doubt about the kernel backward compatibility, it should be
supported.
It's another problem, though, that downstream takes some wild stuff
without properly upstreaming. A known story everywhere.
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 14:58 ` Takashi Iwai
0 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 14:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, linux-kernel, pierre-louis.bossart, Liam Girdwood,
Mark Brown, Patel, Chintan M, Guenter Roeck
On Wed, 23 May 2018 16:49:35 +0200,
Guenter Roeck wrote:
>
> On Wed, May 23, 2018 at 6:56 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > >
> > > My point was a bit different: distributions like Gallium start without
> > > the relevant topology files and UCM settings, and we should have a
> > > reference to quickly enable audio without having to borrow and modify
> > > files from another distro. I faced this issue when I worked with the
> > > Gallium folks to enable audio on Rambi and Cyan Chromebooks and
> > > ended-up creating this reference myself.
> >
> > This sounds rather like a question how the upstream distributes the
> > stuff properly...
> >
> Not really, or not only. My problem, in particular, is that I want (need)
> to be able to run different kernel versions with the same distribution, and
> I want to be able to swap kernel versions without having to update the root
> file system. That doesn't work if I have to use v4 configuration files for
> v4.4 and older kernels, and v5 configuration files for v4.9 and later
> kernels (besides, as I mentioned, that the v5 configuration files I found
> were all incomplete). In my opinion, thre should be exactly one
> configuration file for a given device, and there should be no need for
> users (or distributions) to hunt for the correct version of the file.
No doubt about the kernel backward compatibility, it should be
supported.
It's another problem, though, that downstream takes some wild stuff
without properly upstreaming. A known story everywhere.
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 14:49 ` Guenter Roeck
2018-05-23 14:58 ` Takashi Iwai
@ 2018-05-23 15:52 ` Mark Brown
1 sibling, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 15:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: Takashi Iwai, pierre-louis.bossart, alsa-devel, Guenter Roeck,
Liam Girdwood, Patel, Chintan M, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
On Wed, May 23, 2018 at 07:49:35AM -0700, Guenter Roeck wrote:
> I want to be able to swap kernel versions without having to update the root
> file system. That doesn't work if I have to use v4 configuration files for
> v4.4 and older kernels, and v5 configuration files for v4.9 and later
> kernels (besides, as I mentioned, that the v5 configuration files I found
> were all incomplete). In my opinion, thre should be exactly one
> configuration file for a given device, and there should be no need for
> users (or distributions) to hunt for the correct version of the file.
I think it's fine for there to be new firmwares with corresponding new
configs, and that it's fine for those to have some minimum kernel
version requirement (though desirable to avoid it). The firmwares could
be enabling completely new or different features, that's one of the
advantages of making the firmwares replaceable, and it's reasonable for
that to result in config changes. What's not fine is for a new kernel
to require that you change anything about the firmware and config setup
on your system.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 16:58 [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files Guenter Roeck
@ 2018-05-23 13:54 ` Takashi Iwai
2018-05-22 19:59 ` [alsa-devel] " Pierre-Louis Bossart
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 13:54 UTC (permalink / raw)
To: Guenter Roeck
Cc: Liam Girdwood, alsa-devel, Guenter Roeck, Chintan Patel,
Mark Brown, Jaroslav Kysela, linux-kernel
On Tue, 22 May 2018 18:58:42 +0200,
Guenter Roeck wrote:
>
> +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];
> +};
Missing __packed attribute?
And I'm wondering whether we should move these definitions to uapi
headers.
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 13:54 ` Takashi Iwai
0 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-23 13:54 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, linux-kernel, Liam Girdwood, Mark Brown,
Chintan Patel, Guenter Roeck
On Tue, 22 May 2018 18:58:42 +0200,
Guenter Roeck wrote:
>
> +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];
> +};
Missing __packed attribute?
And I'm wondering whether we should move these definitions to uapi
headers.
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 13:54 ` Takashi Iwai
@ 2018-05-23 13:56 ` Mark Brown
-1 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 13:56 UTC (permalink / raw)
To: Takashi Iwai
Cc: Guenter Roeck, Liam Girdwood, alsa-devel, Guenter Roeck,
Chintan Patel, Jaroslav Kysela, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 160 bytes --]
On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
> And I'm wondering whether we should move these definitions to uapi
> headers.
Yes, we should.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 13:56 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 13:56 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Guenter Roeck, linux-kernel, Liam Girdwood,
Chintan Patel, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 160 bytes --]
On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
> And I'm wondering whether we should move these definitions to uapi
> headers.
Yes, we should.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 13:56 ` Mark Brown
(?)
@ 2018-05-23 15:54 ` Guenter Roeck
2018-05-23 15:58 ` Mark Brown
-1 siblings, 1 reply; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 15:54 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Liam Girdwood, Patel,
Chintan M, Guenter Roeck
On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
>
> > And I'm wondering whether we should move these definitions to uapi
> > headers.
>
> Yes, we should.
Are you sure ? They used to be in
sound/soc/intel/skylake/skl-tplg-interface.h.
I took my clue from sound/soc/soc-topology.c, where the v4 structures are
also
declared locally and not in the uapi files.
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 15:54 ` Guenter Roeck
@ 2018-05-23 15:58 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 15:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: Takashi Iwai, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 682 bytes --]
On Wed, May 23, 2018 at 08:54:18AM -0700, Guenter Roeck wrote:
> On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
> > > And I'm wondering whether we should move these definitions to uapi
> > > headers.
> > Yes, we should.
> Are you sure ? They used to be in
> sound/soc/intel/skylake/skl-tplg-interface.h.
> I took my clue from sound/soc/soc-topology.c, where the v4 structures are
> also
> declared locally and not in the uapi files.
I'm saying we should move them there. They're clearly part of the
userspace ABI and therefore belong in uapi, it was a mistake to let them
be elsewhere.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-23 15:58 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-23 15:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, Takashi Iwai, linux-kernel, Liam Girdwood, Patel,
Chintan M, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 682 bytes --]
On Wed, May 23, 2018 at 08:54:18AM -0700, Guenter Roeck wrote:
> On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
> > > And I'm wondering whether we should move these definitions to uapi
> > > headers.
> > Yes, we should.
> Are you sure ? They used to be in
> sound/soc/intel/skylake/skl-tplg-interface.h.
> I took my clue from sound/soc/soc-topology.c, where the v4 structures are
> also
> declared locally and not in the uapi files.
I'm saying we should move them there. They're clearly part of the
userspace ABI and therefore belong in uapi, it was a mistake to let them
be elsewhere.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 15:58 ` Mark Brown
(?)
@ 2018-05-23 16:17 ` Guenter Roeck
2018-05-24 9:52 ` Takashi Iwai
2018-05-24 14:18 ` Mark Brown
-1 siblings, 2 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 16:17 UTC (permalink / raw)
To: Mark Brown
Cc: Takashi Iwai, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 08:54:18AM -0700, Guenter Roeck wrote:
> > On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
> > > > And I'm wondering whether we should move these definitions to uapi
> > > > headers.
> > > Yes, we should.
> > Are you sure ? They used to be in
> > sound/soc/intel/skylake/skl-tplg-interface.h.
> > I took my clue from sound/soc/soc-topology.c, where the v4 structures
are
> > also
> > declared locally and not in the uapi files.
> I'm saying we should move them there. They're clearly part of the
> userspace ABI and therefore belong in uapi, it was a mistake to let them
> be elsewhere.
They define a firmware file format. Not sure if I would call that userspace
ABI.
I don't mind adding the structures to
sound/soc/intel/skylake/skl-tplg-interface.h,
but it seems a bit out of scope to tie this with moving the file to
include/uapi/sound.
I think that should be a separate discussion.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 16:17 ` Guenter Roeck
@ 2018-05-24 9:52 ` Takashi Iwai
2018-05-24 14:18 ` Mark Brown
1 sibling, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-24 9:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Brown, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
On Wed, 23 May 2018 18:17:23 +0200,
Guenter Roeck wrote:
>
> On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
>
> > On Wed, May 23, 2018 at 08:54:18AM -0700, Guenter Roeck wrote:
> > > On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > > > On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
>
> > > > > And I'm wondering whether we should move these definitions to uapi
> > > > > headers.
>
> > > > Yes, we should.
>
> > > Are you sure ? They used to be in
> > > sound/soc/intel/skylake/skl-tplg-interface.h.
> > > I took my clue from sound/soc/soc-topology.c, where the v4 structures
> are
> > > also
> > > declared locally and not in the uapi files.
>
> > I'm saying we should move them there. They're clearly part of the
> > userspace ABI and therefore belong in uapi, it was a mistake to let them
> > be elsewhere.
>
> They define a firmware file format. Not sure if I would call that userspace
> ABI.
The point is that the defined data is used from both kernel and user
space. Although strictly speaking it might be no "ABI", it'd make
sense a lot to put the stuff into uapi directory, so that the kernel
can expose it properly and user-space can refer it as the single
source.
> I don't mind adding the structures to
> sound/soc/intel/skylake/skl-tplg-interface.h,
> but it seems a bit out of scope to tie this with moving the file to
> include/uapi/sound.
> I think that should be a separate discussion.
Yes, it should be done by another patch.
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-24 9:52 ` Takashi Iwai
0 siblings, 0 replies; 42+ messages in thread
From: Takashi Iwai @ 2018-05-24 9:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, linux-kernel, Liam Girdwood, Mark Brown, Patel,
Chintan M, Guenter Roeck
On Wed, 23 May 2018 18:17:23 +0200,
Guenter Roeck wrote:
>
> On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
>
> > On Wed, May 23, 2018 at 08:54:18AM -0700, Guenter Roeck wrote:
> > > On Wed, May 23, 2018 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > > > On Wed, May 23, 2018 at 03:54:54PM +0200, Takashi Iwai wrote:
>
> > > > > And I'm wondering whether we should move these definitions to uapi
> > > > > headers.
>
> > > > Yes, we should.
>
> > > Are you sure ? They used to be in
> > > sound/soc/intel/skylake/skl-tplg-interface.h.
> > > I took my clue from sound/soc/soc-topology.c, where the v4 structures
> are
> > > also
> > > declared locally and not in the uapi files.
>
> > I'm saying we should move them there. They're clearly part of the
> > userspace ABI and therefore belong in uapi, it was a mistake to let them
> > be elsewhere.
>
> They define a firmware file format. Not sure if I would call that userspace
> ABI.
The point is that the defined data is used from both kernel and user
space. Although strictly speaking it might be no "ABI", it'd make
sense a lot to put the stuff into uapi directory, so that the kernel
can expose it properly and user-space can refer it as the single
source.
> I don't mind adding the structures to
> sound/soc/intel/skylake/skl-tplg-interface.h,
> but it seems a bit out of scope to tie this with moving the file to
> include/uapi/sound.
> I think that should be a separate discussion.
Yes, it should be done by another patch.
thanks,
Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 16:17 ` Guenter Roeck
@ 2018-05-24 14:18 ` Mark Brown
2018-05-24 14:18 ` Mark Brown
1 sibling, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-24 14:18 UTC (permalink / raw)
To: Guenter Roeck
Cc: Takashi Iwai, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 816 bytes --]
On Wed, May 23, 2018 at 09:17:23AM -0700, Guenter Roeck wrote:
> On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
> > I'm saying we should move them there. They're clearly part of the
> > userspace ABI and therefore belong in uapi, it was a mistake to let them
> > be elsewhere.
> They define a firmware file format. Not sure if I would call that userspace
> ABI.
It's a binary provided by userspace to the kernel, I'd say that's
clearly an ABI.
> I don't mind adding the structures to
> sound/soc/intel/skylake/skl-tplg-interface.h,
> but it seems a bit out of scope to tie this with moving the file to
> include/uapi/sound.
> I think that should be a separate discussion.
Is there some reason not to just do it while we're looking at this? I
don't see why we wouldn't want to do this.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-24 14:18 ` Mark Brown
0 siblings, 0 replies; 42+ messages in thread
From: Mark Brown @ 2018-05-24 14:18 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, Takashi Iwai, linux-kernel, Liam Girdwood, Patel,
Chintan M, Guenter Roeck
[-- Attachment #1.1: Type: text/plain, Size: 816 bytes --]
On Wed, May 23, 2018 at 09:17:23AM -0700, Guenter Roeck wrote:
> On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
> > I'm saying we should move them there. They're clearly part of the
> > userspace ABI and therefore belong in uapi, it was a mistake to let them
> > be elsewhere.
> They define a firmware file format. Not sure if I would call that userspace
> ABI.
It's a binary provided by userspace to the kernel, I'd say that's
clearly an ABI.
> I don't mind adding the structures to
> sound/soc/intel/skylake/skl-tplg-interface.h,
> but it seems a bit out of scope to tie this with moving the file to
> include/uapi/sound.
> I think that should be a separate discussion.
Is there some reason not to just do it while we're looking at this? I
don't see why we wouldn't want to do this.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-24 14:18 ` Mark Brown
@ 2018-05-24 14:55 ` Guenter Roeck
-1 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-24 14:55 UTC (permalink / raw)
To: Mark Brown
Cc: Takashi Iwai, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
On Thu, May 24, 2018 at 7:18 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 09:17:23AM -0700, Guenter Roeck wrote:
> > On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
> > > I'm saying we should move them there. They're clearly part of the
> > > userspace ABI and therefore belong in uapi, it was a mistake to let
them
> > > be elsewhere.
> > They define a firmware file format. Not sure if I would call that
userspace
> > ABI.
> It's a binary provided by userspace to the kernel, I'd say that's
> clearly an ABI.
> > I don't mind adding the structures to
> > sound/soc/intel/skylake/skl-tplg-interface.h,
> > but it seems a bit out of scope to tie this with moving the file to
> > include/uapi/sound.
> > I think that should be a separate discussion.
> Is there some reason not to just do it while we're looking at this? I
> don't see why we wouldn't want to do this.
I don't mind doing this. However, keeping the change local and in a single
patch
would make it easier to backport, and I think that the ability to backport
would be
essential to get more than one-person test coverage. I also would have liked
feedback from someone at Intel, at least for the Skylake specific
structures.
Anyway, what file do you have in mind for the structure definitions, both
for the ones
in soc-tolopogy.c and the ones needed in skl-topology.c ? Everything into
asoc.h,
or something else ?
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
@ 2018-05-24 14:55 ` Guenter Roeck
0 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-24 14:55 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Liam Girdwood, Patel,
Chintan M, Guenter Roeck
On Thu, May 24, 2018 at 7:18 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 09:17:23AM -0700, Guenter Roeck wrote:
> > On Wed, May 23, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
> > > I'm saying we should move them there. They're clearly part of the
> > > userspace ABI and therefore belong in uapi, it was a mistake to let
them
> > > be elsewhere.
> > They define a firmware file format. Not sure if I would call that
userspace
> > ABI.
> It's a binary provided by userspace to the kernel, I'd say that's
> clearly an ABI.
> > I don't mind adding the structures to
> > sound/soc/intel/skylake/skl-tplg-interface.h,
> > but it seems a bit out of scope to tie this with moving the file to
> > include/uapi/sound.
> > I think that should be a separate discussion.
> Is there some reason not to just do it while we're looking at this? I
> don't see why we wouldn't want to do this.
I don't mind doing this. However, keeping the change local and in a single
patch
would make it easier to backport, and I think that the ability to backport
would be
essential to get more than one-person test coverage. I also would have liked
feedback from someone at Intel, at least for the Skylake specific
structures.
Anyway, what file do you have in mind for the structure definitions, both
for the ones
in soc-tolopogy.c and the ones needed in skl-topology.c ? Everything into
asoc.h,
or something else ?
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-24 14:55 ` Guenter Roeck
(?)
@ 2018-05-24 15:11 ` Mark Brown
2018-05-25 9:04 ` [alsa-devel] " Lin, Mengdong
-1 siblings, 1 reply; 42+ messages in thread
From: Mark Brown @ 2018-05-24 15:11 UTC (permalink / raw)
To: Guenter Roeck
Cc: Takashi Iwai, Liam Girdwood, alsa-devel, Guenter Roeck, Patel,
Chintan M, Jaroslav Kysela, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
On Thu, May 24, 2018 at 07:55:06AM -0700, Guenter Roeck wrote:
> On Thu, May 24, 2018 at 7:18 AM Mark Brown <broonie@kernel.org> wrote:
Your mail client formatting seems to be broken, the word wrapping is
really funky (it looks like it's breaking longer than 80 column lines in
the middle of paragraphs rather than flowing paragraphs within 80
columns).
> > > I don't mind adding the structures to
> > > sound/soc/intel/skylake/skl-tplg-interface.h,
> > > but it seems a bit out of scope to tie this with moving the file to
> > > include/uapi/sound.
> > > I think that should be a separate discussion.
> > Is there some reason not to just do it while we're looking at this? I
> > don't see why we wouldn't want to do this.
> I don't mind doing this. However, keeping the change local and in a single
> patch
> would make it easier to backport, and I think that the ability to backport
> would be
> essential to get more than one-person test coverage. I also would have liked
> feedback from someone at Intel, at least for the Skylake specific
> structures.
Oh, of course - I'm just saying we should do this, not that everything
needs to be in one patch. Obviously the code motion would be a separate
patch.
> Anyway, what file do you have in mind for the structure definitions, both
> for the ones
> in soc-tolopogy.c and the ones needed in skl-topology.c ? Everything into
> asoc.h,
> or something else ?
All of those that can appear in a firmware file, I don't super care
where they end up but possibly a separate header file or files to the
kernel specific ones as they might get used with Windows.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-24 15:11 ` Mark Brown
@ 2018-05-25 9:04 ` Lin, Mengdong
2018-05-25 13:20 ` Guenter Roeck
0 siblings, 1 reply; 42+ messages in thread
From: Lin, Mengdong @ 2018-05-25 9:04 UTC (permalink / raw)
To: Mark Brown, Guenter Roeck
Cc: alsa-devel, Takashi Iwai, linux-kernel, Liam Girdwood, Patel,
Chintan M, Guenter Roeck, Nc, Shreyas
> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-
> project.org] On Behalf Of Mark Brown
> Sent: Thursday, May 24, 2018 11:12 PM
> On Thu, May 24, 2018 at 07:55:06AM -0700, Guenter Roeck wrote:
> > On Thu, May 24, 2018 at 7:18 AM Mark Brown <broonie@kernel.org> wrote:
>
> Your mail client formatting seems to be broken, the word wrapping is really
> funky (it looks like it's breaking longer than 80 column lines in the middle of
> paragraphs rather than flowing paragraphs within 80 columns).
>
> > > > I don't mind adding the structures to
> > > > sound/soc/intel/skylake/skl-tplg-interface.h,
> > > > but it seems a bit out of scope to tie this with moving the file
> > > > to include/uapi/sound.
> > > > I think that should be a separate discussion.
>
> > > Is there some reason not to just do it while we're looking at this?
> > > I don't see why we wouldn't want to do this.
>
> > I don't mind doing this. However, keeping the change local and in a
> > single patch would make it easier to backport, and I think that the
> > ability to backport would be essential to get more than one-person
> > test coverage. I also would have liked feedback from someone at Intel,
> > at least for the Skylake specific structures.
>
> Oh, of course - I'm just saying we should do this, not that everything needs to be
> in one patch. Obviously the code motion would be a separate patch.
>
> > Anyway, what file do you have in mind for the structure definitions,
> > both for the ones in soc-tolopogy.c and the ones needed in
> > skl-topology.c ? Everything into asoc.h, or something else ?
>
> All of those that can appear in a firmware file, I don't super care where they end
> up but possibly a separate header file or files to the kernel specific ones as they
> might get used with Windows.
Please put the Skylake specific structures in a separate header file, not in asoc.h.
The file asoc.h is for generic topology structures which are platform independent. The topology code in alsa-lib never parses the platform specific structures.
The Skylake specific structures are needed by vendor applications like Intel topology tool (ITT) to define topology for different platforms. The applications can include both asoc.h, skl-tplg-interface.h and other device specific headers.
Thanks
Mengdong
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-25 9:04 ` [alsa-devel] " Lin, Mengdong
@ 2018-05-25 13:20 ` Guenter Roeck
0 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-25 13:20 UTC (permalink / raw)
To: mengdong.lin
Cc: Mark Brown, alsa-devel, Takashi Iwai, linux-kernel,
Liam Girdwood, Patel, Chintan M, Guenter Roeck, shreyas.nc
On Fri, May 25, 2018 at 2:04 AM Lin, Mengdong <mengdong.lin@intel.com>
wrote:
> > -----Original Message-----
> > From: alsa-devel-bounces@alsa-project.org [mailto:
alsa-devel-bounces@alsa-
> > project.org] On Behalf Of Mark Brown
> > Sent: Thursday, May 24, 2018 11:12 PM
> > On Thu, May 24, 2018 at 07:55:06AM -0700, Guenter Roeck wrote:
> > > On Thu, May 24, 2018 at 7:18 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > Your mail client formatting seems to be broken, the word wrapping is
really
> > funky (it looks like it's breaking longer than 80 column lines in the
middle of
> > paragraphs rather than flowing paragraphs within 80 columns).
> >
gmail. Teaches me to not send any patches from my corporate acccount;
it is all but impossible to teach gmail to leave formatting alone.
> Please put the Skylake specific structures in a separate header file, not
in asoc.h.
> The file asoc.h is for generic topology structures which are platform
independent. The topology code in alsa-lib never parses the platform
specific structures.
> The Skylake specific structures are needed by vendor applications like
Intel topology tool (ITT) to define topology for different platforms. The
applications can include both asoc.h, skl-tplg-interface.h and other device
specific headers.
https://patchwork.kernel.org/patch/10425395/
https://patchwork.kernel.org/patch/10425393/
https://patchwork.kernel.org/patch/10425387/
should hopefully be along that line.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 13:54 ` Takashi Iwai
(?)
(?)
@ 2018-05-23 16:29 ` Guenter Roeck
-1 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 16:29 UTC (permalink / raw)
To: Takashi Iwai
Cc: Liam Girdwood, alsa-devel, Guenter Roeck, Patel, Chintan M,
Mark Brown, Jaroslav Kysela, linux-kernel
On Wed, May 23, 2018 at 6:54 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 22 May 2018 18:58:42 +0200,
> Guenter Roeck wrote:
> >
> > +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];
> > +};
> Missing __packed attribute?
Yes and no. The original structure (see
sound/soc/intel/skylake/skl-tplg-interface.h
in v4.5.y and earlier) doesn't have the __packed attribute either. I guess
it doesn't hurt,
since it is all u32, so I'll add it in.
Guenter
> And I'm wondering whether we should move these definitions to uapi
> headers.
> thanks,
> Takashi
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-22 16:58 [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files Guenter Roeck
` (2 preceding siblings ...)
2018-05-23 13:54 ` Takashi Iwai
@ 2018-05-23 20:28 ` Pierre-Louis Bossart
2018-05-23 21:22 ` Guenter Roeck
3 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2018-05-23 20:28 UTC (permalink / raw)
To: Guenter Roeck, Liam Girdwood
Cc: alsa-devel, linux-kernel, Takashi Iwai, Mark Brown,
Chintan Patel, Guenter Roeck
On 05/22/2018 11:58 AM, Guenter Roeck wrote:
> From: Guenter Roeck <groeck@chromium.org>
>
> 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 <groeck@chromium.org>
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 <pierre-louis.bossart@linux.intel.com>
> ---
> 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 <linux/slab.h>
> #include <linux/types.h>
> #include <linux/firmware.h>
> +#include <linux/uuid.h>
> #include <sound/soc.h>
> #include <sound/soc-topology.h>
> #include <uapi/sound/snd_sst_tokens.h>
> @@ -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.
> +
> + 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.
> +
> + 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?
> +
> + 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");
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 20:28 ` [alsa-devel] " Pierre-Louis Bossart
@ 2018-05-23 21:22 ` Guenter Roeck
2018-05-24 3:38 ` Pierre-Louis Bossart
2018-05-25 13:40 ` Shreyas NC
0 siblings, 2 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-23 21:22 UTC (permalink / raw)
To: pierre-louis.bossart
Cc: Liam Girdwood, alsa-devel, linux-kernel, Takashi Iwai,
Mark Brown, Patel, Chintan M, Guenter Roeck
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 <groeck@chromium.org>
> >
> > 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 <groeck@chromium.org>
> 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 <pierre-louis.bossart@linux.intel.com>
> > ---
> > 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 <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/firmware.h>
> > +#include <linux/uuid.h>
> > #include <sound/soc.h>
> > #include <sound/soc-topology.h>
> > #include <uapi/sound/snd_sst_tokens.h>
> > @@ -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");
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 21:22 ` Guenter Roeck
@ 2018-05-24 3:38 ` Pierre-Louis Bossart
2018-05-25 13:40 ` Shreyas NC
1 sibling, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2018-05-24 3:38 UTC (permalink / raw)
To: Guenter Roeck
Cc: alsa-devel, linux-kernel, Takashi Iwai, Liam Girdwood,
Mark Brown, Patel, Chintan M, Guenter Roeck
>>> +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 ?
Looks like your code was fine in the first place.
>
>>> +
>>> + 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.
ok.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-23 21:22 ` Guenter Roeck
2018-05-24 3:38 ` Pierre-Louis Bossart
@ 2018-05-25 13:40 ` Shreyas NC
2018-05-25 14:09 ` Guenter Roeck
1 sibling, 1 reply; 42+ messages in thread
From: Shreyas NC @ 2018-05-25 13:40 UTC (permalink / raw)
To: Guenter Roeck
Cc: pierre-louis.bossart, alsa-devel, linux-kernel, Takashi Iwai,
Liam Girdwood, Mark Brown, Patel, Chintan M, Guenter Roeck
> > > +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];
> > > +
Any reason to not have this as u8?
commit 09305da97c7808b900985526aa9198233f32fb37 had changed this to u8..
<snip>
> > > +
> > > + 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.
>
In the new code, still does a memcpy(). So, I am not sure if I understand
why memcpy() does not work.
if (uuid_tkn->token == SKL_TKN_UUID) {
memcpy(guid, &uuid_tkn->uuid, 16);
return 0;
}
Replied on the older mail since Pierre had a similar question as well.
--Shreyas
--
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-25 13:40 ` Shreyas NC
@ 2018-05-25 14:09 ` Guenter Roeck
2018-05-25 17:41 ` Guenter Roeck
0 siblings, 1 reply; 42+ messages in thread
From: Guenter Roeck @ 2018-05-25 14:09 UTC (permalink / raw)
To: shreyas.nc
Cc: pierre-louis.bossart, alsa-devel, linux-kernel, Takashi Iwai,
Liam Girdwood, Mark Brown, Patel, Chintan M, Guenter Roeck
On Fri, May 25, 2018 at 6:42 AM Shreyas NC <shreyas.nc@intel.com> wrote:
> > > > +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];
> > > > +
> Any reason to not have this as u8?
> commit 09305da97c7808b900985526aa9198233f32fb37 had changed this to u8..
No reason. I'll fix it.
> <snip>
> > > > +
> > > > + 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.
> >
> In the new code, still does a memcpy(). So, I am not sure if I understand
> why memcpy() does not work.
> if (uuid_tkn->token == SKL_TKN_UUID) {
> memcpy(guid, &uuid_tkn->uuid, 16);
> return 0;
> }
The new (v5) configuration files provide the uuid as binary and no longer
as text.
Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files
2018-05-25 14:09 ` Guenter Roeck
@ 2018-05-25 17:41 ` Guenter Roeck
0 siblings, 0 replies; 42+ messages in thread
From: Guenter Roeck @ 2018-05-25 17:41 UTC (permalink / raw)
To: shreyas.nc
Cc: pierre-louis.bossart, alsa-devel, linux-kernel, Takashi Iwai,
Liam Girdwood, Mark Brown, Patel, Chintan M, Guenter Roeck
On Fri, May 25, 2018 at 7:09 AM Guenter Roeck <groeck@google.com> wrote:
> On Fri, May 25, 2018 at 6:42 AM Shreyas NC <shreyas.nc@intel.com> wrote:
> > > > > +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];
> > > > > +
> > Any reason to not have this as u8?
> > commit 09305da97c7808b900985526aa9198233f32fb37 had changed this to u8..
> No reason. I'll fix it.
You confused me. Commit 09305da97c7808b900985526aa9198233f32fb37 changed
the uuid format in the configuration file from 40-byte string to 16-byte
binary. This is
an ABI change. As such, yes, there is a reason for 'char'. It is because v4
of the
configuration file, at least v4 as defined up to and including upstream
kernel v4.6,
provide the uuid as string, not as 16-byte hex value.
That this ABI change was made without ABI version number change is another
issue.
Guenter
> > <snip>
> > > > > +
> > > > > + 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.
> > >
> > In the new code, still does a memcpy(). So, I am not sure if I
understand
> > why memcpy() does not work.
> > if (uuid_tkn->token == SKL_TKN_UUID) {
> > memcpy(guid, &uuid_tkn->uuid, 16);
> > return 0;
> > }
> The new (v5) configuration files provide the uuid as binary and no longer
> as text.
> Guenter
^ permalink raw reply [flat|nested] 42+ messages in thread