Re: [Sound-open-firmware] Loading coefficients for Audio IP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

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@xxxxxxxxx>
    Signed-off-by: Vinod Koul <vinod.koul@xxxxxxxxx>

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@xxxxxxxxx>

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

2015-09-15
[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





_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux