Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180522165842.233949-1-groeck@google.com> <649b1c14-e440-0c89-a59c-dc663344faa3@linux.intel.com>
In-Reply-To: <649b1c14-e440-0c89-a59c-dc663344faa3@linux.intel.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 23 May 2018 14:22:24 -0700
Message-ID: <CABXOdTdyWVtbWXR6O3VenwTezVLGvEfKuvH=Xz6e5D3np-ZQNw@mail.gmail.com>
Subject: Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards
 compatibility with v4 topology files
To:     pierre-louis.bossart@linux.intel.com
Cc:     Liam Girdwood <lgirdwood@gmail.com>, alsa-devel@alsa-project.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Takashi Iwai <tiwai@suse.com>, Mark Brown <broonie@kernel.org>,
        "Patel, Chintan M" <chintan.m.patel@intel.com>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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");