Message ID | 20220622074653.179078-2-vitalyr@opensource.cirrus.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | ALSA: hda: cirrus: Add initial DSP support and firmware loading | expand |
On 6/22/2022 9:46 AM, Vitaly Rodionov wrote: > From: Stefan Binding <sbinding@opensource.cirrus.com> > > The cs35l41 part contains a DSP which is able to run firmware. > The cs_dsp library can be used to control the DSP. > These controls can be exposed to userspace using ALSA controls. > This library adds apis to be able to interface between > cs_dsp and hda drivers and expose the relevant controls as > ALSA controls. > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > MAINTAINERS | 1 + > sound/pci/hda/Kconfig | 4 + > sound/pci/hda/Makefile | 2 + > sound/pci/hda/hda_cs_dsp_ctl.c | 207 +++++++++++++++++++++++++++++++++ > sound/pci/hda/hda_cs_dsp_ctl.h | 33 ++++++ > 5 files changed, 247 insertions(+) > create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.c > create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.h > ... > + > +static unsigned int wmfw_convert_flags(unsigned int in) > +{ > + unsigned int out, rd, wr, vol; > + > + rd = SNDRV_CTL_ELEM_ACCESS_READ; > + wr = SNDRV_CTL_ELEM_ACCESS_WRITE; > + vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE; > + > + out = 0; > + > + if (in) { > + out |= rd; > + if (in & WMFW_CTL_FLAG_WRITEABLE) > + out |= wr; > + if (in & WMFW_CTL_FLAG_VOLATILE) > + out |= vol; > + } else { > + out |= rd | wr | vol; > + } > + > + return out; > +} This is more question of preference, so you can leave above function as is, but you could also do something like the following, which is bit shorter: static unsigned int wmfw_convert_flags(unsigned int in) { unsigned int out = SNDRV_CTL_ELEM_ACCESS_READ; if (!in) return SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_VOLATILE; if (in & WMFW_CTL_FLAG_WRITEABLE) out |= SNDRV_CTL_ELEM_ACCESS_WRITE; if (in & WMFW_CTL_FLAG_VOLATILE) out |= SNDRV_CTL_ELEM_ACCESS_VOLATILE; return out; } > + > +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) > +{ > + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; > + struct snd_kcontrol_new *kcontrol; > + struct snd_kcontrol *kctl; > + int ret = 0; > + > + if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { > + dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name, > + cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE); > + return -EINVAL; > + } > + > + kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); > + if (!kcontrol) > + return -ENOMEM; > + > + kcontrol->name = ctl->name; > + kcontrol->info = hda_cs_dsp_coeff_info; > + kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER; > + kcontrol->private_value = (unsigned long)ctl; > + kcontrol->access = wmfw_convert_flags(cs_ctl->flags); > + > + kcontrol->get = hda_cs_dsp_coeff_get; > + kcontrol->put = hda_cs_dsp_coeff_put; > + > + kctl = snd_ctl_new1(kcontrol, NULL); Wouldn't kctl = snd_ctl_new1(kcontrol, ctl); work instead of kcontrol->private_value = (unsigned long)ctl; ... kctl = snd_ctl_new1(kcontrol, NULL); ? You can then get the value using snd_kcontrol_chip() macro, so instead of doing quite long lines with casts like: struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)kctl->private_value; you could do struct hda_cs_dsp_coeff_ctl *ctl = snd_kcontrol_chip(kctl);
On Wed, 22 Jun 2022 10:40:20 +0200, Amadeusz Sławiński wrote: > > > +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) > > +{ > > + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; > > + struct snd_kcontrol_new *kcontrol; > > + struct snd_kcontrol *kctl; > > + int ret = 0; > > + > > + if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { > > + dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name, > > + cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE); > > + return -EINVAL; > > + } > > + > > + kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); > > + if (!kcontrol) > > + return -ENOMEM; > > + > > + kcontrol->name = ctl->name; > > + kcontrol->info = hda_cs_dsp_coeff_info; > > + kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER; > > + kcontrol->private_value = (unsigned long)ctl; > > + kcontrol->access = wmfw_convert_flags(cs_ctl->flags); > > + > > + kcontrol->get = hda_cs_dsp_coeff_get; > > + kcontrol->put = hda_cs_dsp_coeff_put; > > + > > + kctl = snd_ctl_new1(kcontrol, NULL); > > Wouldn't > kctl = snd_ctl_new1(kcontrol, ctl); > work instead of > kcontrol->private_value = (unsigned long)ctl; > ... > kctl = snd_ctl_new1(kcontrol, NULL); > ? The second argument to snd_ctl_new1() is set to private_data field, while private_value is taken from snd_kcontrol_new template. Takashi
On Wed, 22 Jun 2022 09:46:40 +0200, Vitaly Rodionov wrote: > > +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) > +{ > + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; > + struct snd_kcontrol_new *kcontrol; > + struct snd_kcontrol *kctl; > + int ret = 0; > + > + if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { > + dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name, > + cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE); > + return -EINVAL; > + } > + > + kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); > + if (!kcontrol) > + return -ENOMEM; > + > + kcontrol->name = ctl->name; > + kcontrol->info = hda_cs_dsp_coeff_info; > + kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER; > + kcontrol->private_value = (unsigned long)ctl; > + kcontrol->access = wmfw_convert_flags(cs_ctl->flags); > + > + kcontrol->get = hda_cs_dsp_coeff_get; > + kcontrol->put = hda_cs_dsp_coeff_put; > + > + kctl = snd_ctl_new1(kcontrol, NULL); > + if (!kctl) { > + ret = -ENOMEM; > + goto err; > + } > + ctl->kctl = kctl; > + > + ret = snd_ctl_add(ctl->card, kctl); > + if (ret) > + dev_err(cs_ctl->dsp->dev, "Failed to add KControl: %s - Ret: %d\n", kcontrol->name, > + ret); > + else > + dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol->name); snd_ctl_add() releases the kctl automatically upon errors, hence assigning ctl->kctl may lead to a use-after-free. Therefore ctl->kctl should be assigned after the success of snd_ctl_add(). > +int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info) > +{ > + struct cs_dsp *cs_dsp = cs_ctl->dsp; > + char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; > + struct hda_cs_dsp_coeff_ctl *ctl; > + const char *region_name; > + int ret; > + > + if (cs_ctl->flags & WMFW_CTL_FLAG_SYS) > + return 0; > + > + region_name = cs_dsp_mem_region_name(cs_ctl->alg_region.type); > + if (!region_name) { > + dev_err(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type); > + return -EINVAL; > + } > + > + ret = scnprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s %s %.12s %x", info->device_name, > + cs_dsp->name, hda_cs_dsp_fw_text[info->fw_type], cs_ctl->alg_region.alg); > + > + if (cs_ctl->subname) { > + int avail = SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret - 2; > + int skip = 0; > + > + /* Truncate the subname from the start if it is too long */ > + if (cs_ctl->subname_len > avail) > + skip = cs_ctl->subname_len - avail; > + > + snprintf(name + ret, SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret, > + " %.*s", cs_ctl->subname_len - skip, cs_ctl->subname + skip); > + } > + > + ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); > + if (!ctl) > + return -ENOMEM; > + ctl->cs_ctl = cs_ctl; > + ctl->card = info->card; > + > + ctl->name = kmemdup(name, strlen(name) + 1, GFP_KERNEL); This is kstrdup() :) But, we don't need to keep the name string persistently at all. It's copied onto kcontrol id field, and the original string is no longer needed after that point. So you can pass the name as is to hda_cs_dsp_add_kcontrol(). > + if (!ctl->name) { > + ret = -ENOMEM; > + dev_err(cs_dsp->dev, "Cannot save ctl name\n"); > + goto err_ctl; > + } > + > + cs_ctl->priv = ctl; > + > + return hda_cs_dsp_add_kcontrol(ctl); Hm, this leaves ctl even if it returns an error, i.e. some leaks? > +err_ctl: > + dev_err(cs_dsp->dev, "Error adding control: %s\n", name); > + kfree(ctl); > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_add, SND_HDA_CS_DSP_CONTROLS); > + > +void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl) > +{ > + struct hda_cs_dsp_coeff_ctl *ctl = cs_ctl->priv; > + > + snd_ctl_remove_id(ctl->card, &ctl->kctl->id); > + kfree(ctl->name); > + kfree(ctl); > +} > +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_remove, SND_HDA_CS_DSP_CONTROLS); Is hda_cs_dsp_control_remove() *always* called explicitly for all added controls at the device removal / unbind? ALSA control core also releases the remaining controls by itself, and if the objects are released there, it'll lead to memory leaks for ctl object. If the snd_kcontrol may be freed by itself without hda_cs_dsp_control_remove() call, it should have a proper private_free callback to free the assigned ctl object (also better to reset ctl->cs_ctl->priv field, too). Takashi
diff --git a/MAINTAINERS b/MAINTAINERS index c49b3552f977..fabdd0981ba7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4799,6 +4799,7 @@ S: Maintained F: Documentation/devicetree/bindings/sound/cirrus,cs* F: include/dt-bindings/sound/cs* F: sound/pci/hda/cs* +F: sound/pci/hda/hda_cs_dsp_ctl.* F: sound/soc/codecs/cs* CIRRUS LOGIC DSP FIRMWARE DRIVER diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 79ade4787d95..d1fd6cf82beb 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -94,6 +94,10 @@ config SND_HDA_PATCH_LOADER config SND_HDA_SCODEC_CS35L41 tristate +config SND_HDA_CS_DSP_CONTROLS + tristate + depends on CS_DSP + config SND_HDA_SCODEC_CS35L41_I2C tristate "Build CS35L41 HD-audio side codec support for I2C Bus" depends on I2C diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index 3e7bc608d45f..00d306104484 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -31,6 +31,7 @@ snd-hda-codec-hdmi-objs := patch_hdmi.o hda_eld.o snd-hda-scodec-cs35l41-objs := cs35l41_hda.o snd-hda-scodec-cs35l41-i2c-objs := cs35l41_hda_i2c.o snd-hda-scodec-cs35l41-spi-objs := cs35l41_hda_spi.o +snd-hda-cs-dsp-ctls-objs := hda_cs_dsp_ctl.o # common driver obj-$(CONFIG_SND_HDA) := snd-hda-codec.o @@ -54,6 +55,7 @@ obj-$(CONFIG_SND_HDA_CODEC_HDMI) += snd-hda-codec-hdmi.o obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o +obj-$(CONFIG_SND_HDA_CS_DSP_CONTROLS) += snd-hda-cs-dsp-ctls.o # this must be the last entry after codec drivers; # otherwise the codec patches won't be hooked before the PCI probe diff --git a/sound/pci/hda/hda_cs_dsp_ctl.c b/sound/pci/hda/hda_cs_dsp_ctl.c new file mode 100644 index 000000000000..0f4f02e0a538 --- /dev/null +++ b/sound/pci/hda/hda_cs_dsp_ctl.c @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// HDA DSP ALSA Control Driver +// +// Copyright 2022 Cirrus Logic, Inc. +// +// Author: Stefan Binding <sbinding@opensource.cirrus.com> + +#include <linux/module.h> +#include <sound/soc.h> +#include <linux/firmware/cirrus/cs_dsp.h> +#include <linux/firmware/cirrus/wmfw.h> +#include "hda_cs_dsp_ctl.h" + +#define ADSP_MAX_STD_CTRL_SIZE 512 + +struct hda_cs_dsp_coeff_ctl { + const char *name; + struct cs_dsp_coeff_ctl *cs_ctl; + struct snd_card *card; + struct snd_kcontrol *kctl; +}; + +static const char * const hda_cs_dsp_fw_text[HDA_CS_DSP_NUM_FW] = { + [HDA_CS_DSP_FW_SPK_PROT] = "Prot", + [HDA_CS_DSP_FW_SPK_CALI] = "Cali", + [HDA_CS_DSP_FW_SPK_DIAG] = "Diag", + [HDA_CS_DSP_FW_MISC] = "Misc", +}; + +static int hda_cs_dsp_coeff_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *uinfo) +{ + struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)kctl->private_value; + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES; + uinfo->count = cs_ctl->len; + + return 0; +} + +static int hda_cs_dsp_coeff_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol) +{ + struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)kctl->private_value; + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; + char *p = ucontrol->value.bytes.data; + int ret = 0; + + mutex_lock(&cs_ctl->dsp->pwr_lock); + ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, p, cs_ctl->len); + mutex_unlock(&cs_ctl->dsp->pwr_lock); + + return ret; +} + +static int hda_cs_dsp_coeff_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol) +{ + struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)kctl->private_value; + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; + char *p = ucontrol->value.bytes.data; + int ret; + + mutex_lock(&cs_ctl->dsp->pwr_lock); + ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, p, cs_ctl->len); + mutex_unlock(&cs_ctl->dsp->pwr_lock); + + return ret; +} + +static unsigned int wmfw_convert_flags(unsigned int in) +{ + unsigned int out, rd, wr, vol; + + rd = SNDRV_CTL_ELEM_ACCESS_READ; + wr = SNDRV_CTL_ELEM_ACCESS_WRITE; + vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE; + + out = 0; + + if (in) { + out |= rd; + if (in & WMFW_CTL_FLAG_WRITEABLE) + out |= wr; + if (in & WMFW_CTL_FLAG_VOLATILE) + out |= vol; + } else { + out |= rd | wr | vol; + } + + return out; +} + +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) +{ + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; + struct snd_kcontrol_new *kcontrol; + struct snd_kcontrol *kctl; + int ret = 0; + + if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { + dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name, + cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE); + return -EINVAL; + } + + kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); + if (!kcontrol) + return -ENOMEM; + + kcontrol->name = ctl->name; + kcontrol->info = hda_cs_dsp_coeff_info; + kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER; + kcontrol->private_value = (unsigned long)ctl; + kcontrol->access = wmfw_convert_flags(cs_ctl->flags); + + kcontrol->get = hda_cs_dsp_coeff_get; + kcontrol->put = hda_cs_dsp_coeff_put; + + kctl = snd_ctl_new1(kcontrol, NULL); + if (!kctl) { + ret = -ENOMEM; + goto err; + } + ctl->kctl = kctl; + + ret = snd_ctl_add(ctl->card, kctl); + if (ret) + dev_err(cs_ctl->dsp->dev, "Failed to add KControl: %s - Ret: %d\n", kcontrol->name, + ret); + else + dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol->name); + +err: + kfree(kcontrol); + + return ret; +} + +int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info) +{ + struct cs_dsp *cs_dsp = cs_ctl->dsp; + char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; + struct hda_cs_dsp_coeff_ctl *ctl; + const char *region_name; + int ret; + + if (cs_ctl->flags & WMFW_CTL_FLAG_SYS) + return 0; + + region_name = cs_dsp_mem_region_name(cs_ctl->alg_region.type); + if (!region_name) { + dev_err(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type); + return -EINVAL; + } + + ret = scnprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s %s %.12s %x", info->device_name, + cs_dsp->name, hda_cs_dsp_fw_text[info->fw_type], cs_ctl->alg_region.alg); + + if (cs_ctl->subname) { + int avail = SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret - 2; + int skip = 0; + + /* Truncate the subname from the start if it is too long */ + if (cs_ctl->subname_len > avail) + skip = cs_ctl->subname_len - avail; + + snprintf(name + ret, SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret, + " %.*s", cs_ctl->subname_len - skip, cs_ctl->subname + skip); + } + + ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); + if (!ctl) + return -ENOMEM; + ctl->cs_ctl = cs_ctl; + ctl->card = info->card; + + ctl->name = kmemdup(name, strlen(name) + 1, GFP_KERNEL); + if (!ctl->name) { + ret = -ENOMEM; + dev_err(cs_dsp->dev, "Cannot save ctl name\n"); + goto err_ctl; + } + + cs_ctl->priv = ctl; + + return hda_cs_dsp_add_kcontrol(ctl); + +err_ctl: + dev_err(cs_dsp->dev, "Error adding control: %s\n", name); + kfree(ctl); + return ret; +} +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_add, SND_HDA_CS_DSP_CONTROLS); + +void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl) +{ + struct hda_cs_dsp_coeff_ctl *ctl = cs_ctl->priv; + + snd_ctl_remove_id(ctl->card, &ctl->kctl->id); + kfree(ctl->name); + kfree(ctl); +} +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_remove, SND_HDA_CS_DSP_CONTROLS); + +MODULE_DESCRIPTION("CS_DSP ALSA Control HDA Library"); +MODULE_AUTHOR("Stefan Binding, <sbinding@opensource.cirrus.com>"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_cs_dsp_ctl.h b/sound/pci/hda/hda_cs_dsp_ctl.h new file mode 100644 index 000000000000..1c6d0fc9a2cc --- /dev/null +++ b/sound/pci/hda/hda_cs_dsp_ctl.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * HDA DSP ALSA Control Driver + * + * Copyright 2022 Cirrus Logic, Inc. + * + * Author: Stefan Binding <sbinding@opensource.cirrus.com> + */ + +#ifndef __HDA_CS_DSP_CTL_H__ +#define __HDA_CS_DSP_CTL_H__ + +#include <sound/soc.h> +#include <linux/firmware/cirrus/cs_dsp.h> + +enum hda_cs_dsp_fw_id { + HDA_CS_DSP_FW_SPK_PROT, + HDA_CS_DSP_FW_SPK_CALI, + HDA_CS_DSP_FW_SPK_DIAG, + HDA_CS_DSP_FW_MISC, + HDA_CS_DSP_NUM_FW +}; + +struct hda_cs_dsp_ctl_info { + struct snd_card *card; + enum hda_cs_dsp_fw_id fw_type; + const char *device_name; +}; + +int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info); +void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl); + +#endif /*__HDA_CS_DSP_CTL_H__*/