On Wed, 19 Dec 2018 09:46:37 +0100, Daniel Baluta wrote:

Hello Pierre, Liam, Mark,

What is the standard ALSA way of loading tables of coefficients for various audio IPs?

We have IPs like PDM or ASRC which need large table of coefficients depending on configuration. This tables can reach up to 120K in size.

Creating this kind of tables as arrays of integers in kernel doesn't seem to be a common used solution and we will end up with large files that contains only numbers.

Another approach that we are thinking of is to create and compile this tables in userspace as raw binary files and then loading them in kernel using the firmware interface.

Would this be an acceptable approach?

This is a topic we discuss every year in the conference ;)

Takashi

Hi Daniel,

What is the standard ALSA way of loading tables of coefficients for various audio IPs?

We have IPs like PDM or ASRC which need large table of coefficients depending on configuration. This tables can reach up to 120K in size.

Creating this kind of tables as arrays of integers in kernel doesn't seem to be a common used solution and we will end up with large files that contains only numbers.

Another approach that we are thinking of is to create and compile this tables in userspace as raw binary files and then loading them in kernel using the firmware interface.

I think there are two separate parts in your question (not trying to pontificate but split variables).

1. how do we download binary tables that are related to hardware interfaces or firmware modules?

In that case the "coefficients" don't depend on use cases but are set in stone either at build time or when the device is flashed. They never vary once the boot starts. You absolutely do not want these coefficients in the kernel, and I can think of two ways of dealing with this case

1.a. request_firmware(). If you are using the topology framework, there may be a way of adding tokens so that the file names for the coefficient doesn't have to be a constant string in the kernel.

1.b adding binary data as part of the topology file. I vaguely recall discussing this with Liam, not sure if it's feasible.

1.c a new custom interface to add modules to the firmware. Liam worked on that part to add new code, but it could be used for coefficients since the symbol address resolution would be identical.

2. how do we download binary data related to processing/algorithms, which can vary depending on use cases and possibly user interaction?

That part is more complicated and I don't have a turn-key solution, I only started looking into this last week and you'll find below all my raw notes (all based on publicly-available information).

The main issue here is that ALSA BYTE controls are limited to 512 bytes, which doesn't cut it for most applications. There were two successive changes, one to add "extended controls" and a second to use TLVs. I personally missed that second step and wasn't aware that they were used by Intel in the Skylake driver. After looking in more details at all the online information I am really not hot on this direction. There are a couple of issues with TLVs

2.a The API was abused big time. The "T" in TLV should have been a standard one, but it turns out everyone just used whatever they wanted.

2.b. the API isn't a good fit for the type of data being handled. the "L" in TLV is supposed to indicate the length of the binary data, but the API can also pass a length information. I find this really confusing. Such binary data should be handled in an 'atomic' way, all or none.

2.c and fundamentally I don't see what TLVs bring over regular extended controls. If the driver ignores the length passed by the application and only uses the encoded length, then it'd just be equivalent to use a custom encoding and an extended control.

In the notes from the last two or three ALSA miniconfs there are mentions of looking into other directions such as hwdep:. I must admit I am not familiar with this interface so there's some learning needed on my side. Patrick Lai from Qualcomm also mentioned a number of times the need to support 'calibration' files and I wonder what the need really is.

I am also wondering if there is a need for reads of large binary data. I can see cases where you could extract state information, but this could be abused if people used it for probes or data logging, which is really streaming and can be handled e.g. with the compressed API. And you would probably need a 'freeze/snapshot' command in the first place otherwise the state could be inconsistent between the initial part of the read and the last.

At any rate, for SOF I don't feel we have to comply with any legacy, but rather we should try and help the ALSA community do the right thing with a solution that is agreed upon and will be acceptable for a number of years. Restarting with controversial concepts doesn't seem like a good direction to me.

Most likely something that will happen in 2019 though. Happy Holidays y'all.

Regards

-Pierre

Raw notes on ALSA binary control support:

2014-05-02 d98812082c87 ASoC: add SND_SOC_BYTES_EXT

we need _EXT version for SND_SOC_BYTES so that DSPs can use this to pass data     for DSP modules

Signed-off-by: Vinod Koul vinod.koul@intel.com

2014-07-15 7523a271682fc     ASoC: core: add a helper for extended byte controls using TLV

ALSA supports arbitrary length TLVs for each kcontrol that can be used     to pass metadata about the control (e.g. volumes, enum information). The     same transport mechanism is now used for arbitrary length data by     defining a new helper.

Signed-off-by: Omair Mohammed Abdullah omair.m.abdullah@intel.com     Signed-off-by: Vinod Koul vinod.koul@intel.com

2016-11-23 50a4f98d3452    ASoC: core: mark SND_SOC_BYTES_EXT as deprecated

Since we have SND_SOC_BYTES_TLV control to lets devices have     larger size data sent, we do not need SND_SOC_BYTES_EXT with 512     byte limitation so mark it deprecated

Signed-off-by: Vinod Koul vinod.koul@intel.com

[PLB] I have no idea why TLVs were marked as a better solution

2015-09-15 [alsa-devel] [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow to a protocol of ctl and tlv operation. At least, this macro and related kernel APIs include two misunderstandings:  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV    packet paylaod, snd_soc_bytes_info_ext() performs in this way.  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV    packet structure, snd_soc_bytes_tlv_callback() performs in this way.

In a policy of kernel land development, it's quite worse to break protocols for applications. Therefore, developers are discouraged to use these kernel APIs.

In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of developers who need to add control elements which transfer data larger than the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512 bytes. However, as long as the size is less than the size; e.g. 512 bytes, SND_SOC_BYTES_EXT is still available. Although there is actually the limitation of maximum data size, it's better to use this API for stable application interface till better alternative ways are implemented in future.

For these reasons, this commit reverts the previous commit which lead developers to the worse behaviour.

Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>

[PLB] Sakamoto-san was very vocal on this and provided even more details at

https://patchwork.kernel.org/patch/9304649/

While this thread is difficult to follow (turn off social media and get a large mug of coffee), it does go over all the known TLV problems. One of the main issues with TLV seems to be that if the application passes a large buffer of data, only the parts that can be handled at the ASoC level will be handled. The application has no means to know how much was written nor how much it gets back on a read. Maybe this could be improved by first querying the control size and asking the application to respect that max size. But to some extent letting the application muck with the length isn't quite right in the first place, the data should be used as is in an atomic way.

In latest code:

git grep SND_SOC_BYTES_EXT include/sound/soc.h: * SND_SOC_BYTES_EXT is deprecated, please USE SND_SOC_BYTES_TLV instead include/sound/soc.h:#define SND_SOC_BYTES_EXT(xname, xcount, xhandler_get, xhandler_put) \ sound/soc/codecs/da7218.c:      SND_SOC_BYTES_EXT("Sidetone BiQuad Coefficients", sound/soc/codecs/da7218.c:      SND_SOC_BYTES_EXT("BiQuad Coefficients", sound/soc/codecs/nau8810.c:     SND_SOC_BYTES_EXT("EQ Parameters", 10, sound/soc/codecs/nau8822.c:     SND_SOC_BYTES_EXT("EQ Parameters", 10, sound/soc/codecs/nau8825.c:     SND_SOC_BYTES_EXT("BIQ Coefficients", 20, sound/soc/codecs/wm5102.c:SND_SOC_BYTES_EXT("Output Compensation Coefficient", 2, sound/soc/intel/haswell/sst-haswell-pcm.c: SND_SOC_BYTES_EXT("Waves Set Param", WAVES_PARAM_COUNT,

git grep SND_SOC_BYTES_TLV include/sound/soc.h: * SND_SOC_BYTES_EXT is deprecated, please USE SND_SOC_BYTES_TLV instead include/sound/soc.h:#define SND_SOC_BYTES_TLV(xname, xcount, xhandler_get, xhandler_put) \

[PLB]: There is no clear mention in the kernel code that TLVs are actually used

git grep SKL_CONTROL_TYPE_BYTE_TLV include/uapi/sound/skl-tplg-interface.h:#define SKL_CONTROL_TYPE_BYTE_TLV       0x100 sound/soc/intel/skylake/skl-topology.c: {SKL_CONTROL_TYPE_BYTE_TLV, skl_tplg_tlv_control_get,

static int skl_tplg_tlv_control_get(struct snd_kcontrol *kcontrol,             unsigned int __user *data, unsigned int size) {     struct soc_bytes_ext *sb =             (struct soc_bytes_ext *)kcontrol->private_value;     struct skl_algo_data *bc = (struct skl_algo_data *)sb->dobj.private;     struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);     struct skl_module_cfg *mconfig = w->priv;     struct skl *skl = get_skl_ctx(w->dapm->dev);

if (w->power)         skl_get_module_params(skl->skl_sst, (u32 *)bc->params,                       bc->size, bc->param_id, mconfig);

/* decrement size for TLV header */     size -= 2 * sizeof(u32);

/* check size as we don't want to send kernel data */     if (size > bc->max)         size = bc->max;

if (bc->params) {         if (copy_to_user(data, &bc->param_id, sizeof(u32)))             return -EFAULT;         if (copy_to_user(data + 1, &size, sizeof(u32)))             return -EFAULT;         if (copy_to_user(data + 2, bc->params, size))             return -EFAULT;     }

return 0; }

static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,             const unsigned int __user *data, unsigned int size) {     struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);     struct skl_module_cfg *mconfig = w->priv;     struct soc_bytes_ext *sb =             (struct soc_bytes_ext *)kcontrol->private_value;     struct skl_algo_data *ac = (struct skl_algo_data *)sb->dobj.private;     struct skl *skl = get_skl_ctx(w->dapm->dev);

if (ac->params) {         if (size > ac->max)             return -EINVAL;

ac->size = size;         /*          * if the param_is is of type Vendor, firmware expects actual          * parameter id and size from the control.          */         if (ac->param_id == SKL_PARAM_VENDOR_ID) {             if (copy_from_user(ac->params, data, size))                 return -EFAULT;         } else {             if (copy_from_user(ac->params,                        data + 2, size))                 return -EFAULT;         }

if (w->power)             return skl_set_module_params(skl->skl_sst,                         (u32 *)ac->params, ac->size,                         ac->param_id, mconfig);     }

return 0; }

[PLB] So Skylake does appear to use an implicit TLV, but the TLV information is not propagated as is to firmware. Custom methods are used to pass the data in chunks.

https://www.alsa-project.org/main/index.php/Miniconf_2017 TLV API for binary controls Picking up the discussion from last year. Problem: Can't figure out what kind of TLV control it is It's a hack to use TLV for binary controls, probably worth reconsidering. Only users are Cirrus and Intel. Only for controls with more than 512 bytes for Cirrus. Tidy up existing one? Add a flag saying that it's binary nonsense - disagreement. Add TLV headers in the data? Probably breaks Intel who have headers that look like they mean something. One goal for using TLV was to get data through existing API clients. Takashi would prefer to add a new API for this. Indirect pointers sound complicated. Maybe use file based interface (ioctl takes a fd). Sounds like new ioctls, Charles volunteers to implement.

https://www.alsa-project.org/main/index.php/Miniconf_2018 Byte controls

Already covered, Sakamoto-san’s new elem API probably can have no limit if desired     Vinod: it’s easier for usespace to handle a single userspace interface     Charles: Cirrus has few large controls     Having to extend every single UCM implementation is a pain     tiwai: controls need to be readable at any time     Calibration loading         QC currently has a calibration mechanism parallel to standard ALSA one. QC code available on codeaurora. Want to re-do this with upstream constructs.         2 methods aware of upstream currently             Pass calibration through byte control from user space             Go through request firmware         Discussion converging towards hwdep         Vinod: fudge hwdep in alsa-lib         Charles: happy to investigate what the hwdep solution would look like from cirrus PoV         Patrick: need to provide different calibration data depending on sample rate.         UCM: Problems with Android but otherwise good