[alsa-devel] [RFC 0/4] ALSA controls management using index/device/sub-devices fields
I tried to summarize my observation based on discussions concerning the instantiation of the ALSA controls, initiate in following thread: [PATCH] ASoC: core: allow control index different from 0 https://www.spinics.net/lists/ALSA-devel/msg55415.html
My test environment: sti platform, 3 playback devices with associated instances of controls
card 0: STIB2120 [STI-B2120], device 0: Uni Player #0 (HDMI)-i2s-hifi numid=1,iface=PCM,name='ELD' numid=3,iface=PCM,name='IEC958 Playback Default' numid=2,iface=PCM,name='PCM Playback Oversampling Freq. Adjustment' card 0: STIB2120 [STI-B2120], device 1: Uni Player #2 (DAC)-sas-dai-dac numid=4,iface=PCM,name='PCM Playback Oversampling Freq. Adjustment',device=2 card 0: STIB2120 [STI-B2120], device 2: Uni Player #3 (SPDIF)-sas-dai- spdif-out numid=6,iface=PCM,name='IEC958 Playback Default',device=2 numid=5,iface=PCM,name='PCM Playback Oversampling Freq. Adjustment',device=3
1) From user land point of view
1-1) How to Instantiate generic ALSA control
The point is the handling of multi instance of generic ALSA controls. In this case "prefix" can not be used. Controls have to be identified by a combination of device/sub-device/index
Examples of controls that seem generics: iface=PCM,name='Capture Channel Map' iface=PCM,name='Playback Channel Map' iface=PCM,name='ELD'
iface=MIXER,name='Master Playback Switch' iface=MIXER,name='Master Playback Volume' iface=MIXER,name='Capture Switch' iface=MIXER,name='Capture Volume'
iface=MIXER/PCM,name='IEC958 Playback Con Mask' iface=MIXER/PCM,name='IEC958 Playback Pro Mask' iface=MIXER/PCM,name='IEC958 Playback Default' iface=MIXER/PCM,name='IEC958 Playback Switch'
1-2) Different ways of using control instantiation Here is a short benchmark on applications using ALSA controls: - iecset and "amixer scontrols" base control instantiation on "index". - GStreamer and pulseaudio seems based on "device" (through the ALSA card conf files). - "amixer controls" support both.
I supposse that this is the reason why "device" and "index fields are both fixed in HDA-intel.cong aligned with driver.
1-3) Questions: - If i consider description provided by Takashi Sakamoto in thread: https://www.spinics.net/lists/ALSA-devel/msg55451.html
Should we always apply this rules? - MIXER control type: as it is not linked to PCM device but card, "index" is used to instantiate control. - PCM control type: as it is linked to PCM device, "device/subdevice" is used to instantiate control. Or could we consider that a control can be instantiated using device/subdevice fields and/or index fields?
- Concerning IEC controls, should we consider them as PCM or MIXER type controls? In current implementations both are used...
2) From driver point of view
2-1) None SOC drivers: Today solution seems to base indexation on both "index" and "device" number. Device indexation seems not linked to PCM device number Example in ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts"). http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...
=> The drawback is that each driver has to implement it. Could be nice to have in generic code....
2-2) SOC drivers: - it is not possible to use "index" using helper functions as snd_soc_cnew forces index to 0. Solution could be to implement in soc_core as done in HDA driver. => duplication of the code in ASoC and None ASoC drivers. - Relation chip between control and PCM device. if rules mentioned in 1-3 should be respected, need to link control to PCM device. This point seems quite tricky... At least DAI controls can be simply linked to PCM device during DAI-link probing.
3) Patches proposed: Based on these observations, here are some patches that i tested in my environment. There are complementary, and could answer to some limitations mentioned above.
3-1) Alsa-utils patch
- iecset: allow to select control with device and sub-device numbers This patch allows to access to 2 iec controls differentiated by device/sub-devices numbers => For me, this patch is mandatory to be able to address the ASoC IEC controls, in case of no fix is implemented to allows index field update in ASoC.
3-2) Alsa driver patches - ASoC: core: allow PCM control binding to PCM device Add relationship between DAIs PCM controls and PCM device.
- ALSA: control: increment index field for duplicated control. Generic implementation of the patch proposed in HDA driver (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...)
- ASoC: sti: use bind_pcm_ctl implementation of bind_pcm_ctl for sti driver.
Best Regards, Arnaud
Add relationchip between DAI PCM controls and PCM device. For some generic controls (for instance iec controls), it is useful to link control to the PCM device. Especially for application that uses a instance of the control based on the PCM device number.
Implementation is based on bind_pcm_ctl field in dai_driver struct to enable the link. When set to true DAI PCM controls device field is forced to the PCM device number.
Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- include/sound/soc-dai.h | 3 ++ sound/soc/soc-core.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 964b7de..52e89ee 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -233,6 +233,8 @@ struct snd_soc_dai_driver { int (*compress_new)(struct snd_soc_pcm_runtime *rtd, int num); /* DAI is also used for the control bus */ bool bus_control; + /* Bind IFACE_PCM controls to the PCM device */ + bool bind_pcm_ctl;
/* ops */ const struct snd_soc_dai_ops *ops; @@ -292,6 +294,7 @@ struct snd_soc_dai { unsigned int rx_mask;
struct list_head list; + struct list_head list_pcm_ctl; };
static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4afa8db..148b90f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -58,6 +58,11 @@ static LIST_HEAD(platform_list); static LIST_HEAD(codec_list); static LIST_HEAD(component_list);
+struct snd_soc_dai_pcm_ctls { + struct snd_kcontrol *controls; + struct list_head list; +}; + /* * This is a timeout to do a DAPM powerdown after a stream is closed(). * It can be used to eliminate pops between different playback streams, e.g. @@ -1362,6 +1367,32 @@ static void soc_set_name_prefix(struct snd_soc_card *card, } }
+static int soc_link_dai_pcm_controls(struct snd_soc_card *card, + struct snd_soc_dai *dai, struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_dai_pcm_ctls *ctls, *_ctls; + struct snd_kcontrol *kctl; + int i, err; + + list_for_each_entry_safe(ctls, _ctls, &dai->list_pcm_ctl, list) { + kctl = ctls->controls; + kctl->id.device = rtd->pcm->device; + + err = snd_ctl_add(card->snd_card, kctl); + if (err < 0) { + dev_err(card->dev, + "ASoC: Can't link %s: %s control(s) to %s :%d\n", + dai->name, kctl->id.name, + rtd->dai_link->stream_name, err); + return err; + } + list_del(&ctls->list); + kfree(ctls); + } + + return 0; +} + static int soc_probe_component(struct snd_soc_card *card, struct snd_soc_component *component) { @@ -1598,7 +1629,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, struct snd_soc_pcm_runtime *rtd, int order) { struct snd_soc_dai_link *dai_link = rtd->dai_link; - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct snd_soc_dai *dai, *cpu_dai = rtd->cpu_dai; int i, ret;
dev_dbg(card->dev, "ASoC: probe %s dai link %d late %d\n", @@ -1663,6 +1694,24 @@ static int soc_probe_link_dais(struct snd_soc_card *card, dai_link->stream_name, ret); return ret; } + + /* link CPU DAI pcm controls to pcm device */ + if (!list_empty(&cpu_dai->list_pcm_ctl)) { + ret = soc_link_dai_pcm_controls(card, cpu_dai, + rtd); + if (ret < 0) + return ret; + } + + /* link CODEC DAI pcm control to pcm device */ + for (i = 0; i < rtd->num_codecs; i++) { + dai = rtd->codec_dais[i]; + if (!list_empty(&dai->list_pcm_ctl)) + ret = soc_link_dai_pcm_controls(card, + dai, rtd); + if (ret < 0) + return ret; + } } else { INIT_DELAYED_WORK(&rtd->delayed_work, codec2codec_close_delayed_work); @@ -2218,6 +2267,49 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev, return 0; }
+static int snd_soc_add_dai_pcm_controls(struct snd_soc_card *soc_card, + struct snd_soc_dai *dai, const struct snd_kcontrol_new *controls, + int num_controls, const char *prefix, void *data) +{ + struct snd_soc_pcm_runtime *rtd; + struct snd_soc_dai_pcm_ctls *ctls_list; + struct snd_kcontrol *kctl; + int i, dai_found = 0; + + for (i = 0; i < num_controls; i++) { + const struct snd_kcontrol_new *control = &controls[i]; + + if (control->iface != SNDRV_CTL_ELEM_IFACE_PCM) { + dev_err(dai->dev, "%s: not a pcm device control !!!\n", + control->name); + return -EINVAL; + } + kctl = snd_soc_cnew(control, data, control->name, prefix); + if (IS_ERR(kctl)) + return PTR_ERR(kctl); + + ctls_list = kzalloc(sizeof(*ctls_list), GFP_KERNEL); + ctls_list->controls = kctl; + list_add(&ctls_list->list, &dai->list_pcm_ctl); + } + + if (dai->probed) { + /* pcm device exists. Control can be linked to it */ + list_for_each_entry(rtd, &soc_card->rtd_list, list) { + if (dai == rtd->cpu_dai) { + dai_found = 1; + break; + } + } + if (!dai_found) + return -EINVAL; + + soc_link_dai_pcm_controls(soc_card, dai, rtd); + } + + return 0; +} + struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card, const char *name) { @@ -2323,10 +2415,26 @@ EXPORT_SYMBOL_GPL(snd_soc_add_card_controls); int snd_soc_add_dai_controls(struct snd_soc_dai *dai, const struct snd_kcontrol_new *controls, int num_controls) { - struct snd_card *card = dai->component->card->snd_card; + struct snd_soc_card *soc_card = dai->component->card; + struct snd_card *card = soc_card->snd_card; + int ret, i;
- return snd_soc_add_controls(card, dai->dev, controls, num_controls, - NULL, dai); + if (!dai->driver || !dai->driver->bind_pcm_ctl) + return snd_soc_add_controls(card, dai->dev, controls, + num_controls, NULL, dai); + + for (i = 0; i < num_controls; i++) { + if (controls[i].iface == SNDRV_CTL_ELEM_IFACE_PCM) + ret = snd_soc_add_dai_pcm_controls(soc_card, dai, + &controls[i], 1, NULL, dai); + else + ret = snd_soc_add_controls(card, dai->dev, &controls[i], + 1, NULL, dai); + if (ret < 0) + return ret; + } + + return 0; } EXPORT_SYMBOL_GPL(snd_soc_add_dai_controls);
@@ -2806,6 +2914,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, if (!dai->driver->ops) dai->driver->ops = &null_dai_ops;
+ INIT_LIST_HEAD(&dai->list_pcm_ctl); + list_add(&dai->list, &component->dai_list); component->num_dai++;
Instead of returning an error when a control is already present, the index field is incremented and a warning message is printed. This allows drivers to instanciate same control without device and sub device number management ( MIXER type controls).
Change-Id: Ifcc60dca9d1cf4c3a424bb9653296678aa7953cb Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- sound/core/control.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index fb096cb..014e3f4 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -365,6 +365,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) struct snd_ctl_elem_id id; unsigned int idx; unsigned int count; + struct snd_kcontrol *elem_set; int err = -EINVAL;
if (! kcontrol) @@ -376,17 +377,24 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) goto error;
down_write(&card->controls_rwsem); - if (snd_ctl_find_id(card, &id)) { - up_write(&card->controls_rwsem); - dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n", - id.iface, - id.device, - id.subdevice, - id.name, - id.index); - err = -EBUSY; - goto error; + while (elem_set = snd_ctl_find_id(card, &id)) { + id.index++; + if (id.index > UINT_MAX - kcontrol->count) { + up_write(&card->controls_rwsem); + dev_err(card->dev, "no more free index for control %i:%i:%i:%s\n", + id.iface, id.device, id.subdevice, id.name); + err = -ENOSPC; + goto error; + } + } + if (kcontrol->id.index != id.index) { + dev_warn(card->dev, "control %i:%i:%i:%s:%i is already present\n", + id.iface, id.device, id.subdevice, id.name, id.index); + dev_warn(card->dev, "control index updated from %i to %i\n", + kcontrol->id.index, id.index); + kcontrol->id.index = id.index; } + if (snd_ctl_find_hole(card, kcontrol->count) < 0) { up_write(&card->controls_rwsem); err = -ENOMEM;
Hi,
On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
Instead of returning an error when a control is already present, the index field is incremented and a warning message is printed. This allows drivers to instanciate same control without device and sub device number management ( MIXER type controls).
Change-Id: Ifcc60dca9d1cf4c3a424bb9653296678aa7953cb Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com
sound/core/control.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index fb096cb..014e3f4 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -365,6 +365,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) struct snd_ctl_elem_id id; unsigned int idx; unsigned int count;
struct snd_kcontrol *elem_set; int err = -EINVAL;
if (! kcontrol)
@@ -376,17 +377,24 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) goto error;
down_write(&card->controls_rwsem);
- if (snd_ctl_find_id(card, &id)) {
up_write(&card->controls_rwsem);dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n",id.iface,id.device,id.subdevice,id.name,id.index);err = -EBUSY;goto error;
- while (elem_set = snd_ctl_find_id(card, &id)) {
id.index++;if (id.index > UINT_MAX - kcontrol->count) {up_write(&card->controls_rwsem);dev_err(card->dev, "no more free index for control %i:%i:%i:%s\n",id.iface, id.device, id.subdevice, id.name);err = -ENOSPC;goto error;}- }
- if (kcontrol->id.index != id.index) {
dev_warn(card->dev, "control %i:%i:%i:%s:%i is already present\n",id.iface, id.device, id.subdevice, id.name, id.index);dev_warn(card->dev, "control index updated from %i to %i\n",kcontrol->id.index, id.index);kcontrol->id.index = id.index;- if (snd_ctl_find_hole(card, kcontrol->count) < 0) { up_write(&card->controls_rwsem); err = -ENOMEM;
As Iwai-san explained below, this integration is a bit over-specification in control core, because your issue is specific in ALSA SoC part.
On Nov 8 2016 23:30, Takashi Iwai wrote:
Right, this behavior must be optional. For the normal drivers, the duplicated controls *are* errors, and we catch it instead. For drivers that are aware of confliction and want the automatic workaround (e.g. HD-audio driver does it already), this kind of code would be useful to be in the common place.
(http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.ht...)
For drivers outside of ALSA SoC part, developers can and should program control element sets with unique identification information, because design of sound card is fixed at development time. Therefore, these codes should be in ALSA SoC core, as I introduced once.
When we have the same requirement for drivers outside of ALSA SoC part, then we're going to move these codes from ALSA SoC core to ALSA control core, I think.
Regards
Takashi Sakamoto
Hello Takashi,
On 11/09/2016 05:04 AM, Takashi Sakamoto wrote:
Hi,
On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
Instead of returning an error when a control is already present, the index field is incremented and a warning message is printed. This allows drivers to instanciate same control without device and sub device number management ( MIXER type controls).
Change-Id: Ifcc60dca9d1cf4c3a424bb9653296678aa7953cb Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com
sound/core/control.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index fb096cb..014e3f4 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -365,6 +365,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) struct snd_ctl_elem_id id; unsigned int idx; unsigned int count;
struct snd_kcontrol *elem_set; int err = -EINVAL;
if (! kcontrol)
@@ -376,17 +377,24 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) goto error;
down_write(&card->controls_rwsem);
- if (snd_ctl_find_id(card, &id)) {
up_write(&card->controls_rwsem);dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n",id.iface,id.device,id.subdevice,id.name,id.index);err = -EBUSY;goto error;
- while (elem_set = snd_ctl_find_id(card, &id)) {
id.index++;if (id.index > UINT_MAX - kcontrol->count) {up_write(&card->controls_rwsem);dev_err(card->dev, "no more free index for control %i:%i:%i:%s\n",id.iface, id.device, id.subdevice, id.name);err = -ENOSPC;goto error;}- }
- if (kcontrol->id.index != id.index) {
dev_warn(card->dev, "control %i:%i:%i:%s:%i is already present\n",id.iface, id.device, id.subdevice, id.name, id.index);dev_warn(card->dev, "control index updated from %i to %i\n",kcontrol->id.index, id.index);kcontrol->id.index = id.index;- if (snd_ctl_find_hole(card, kcontrol->count) < 0) { up_write(&card->controls_rwsem); err = -ENOMEM;
As Iwai-san explained below, this integration is a bit over-specification in control core, because your issue is specific in ALSA SoC part.
On Nov 8 2016 23:30, Takashi Iwai wrote:
Right, this behavior must be optional. For the normal drivers, the duplicated controls *are* errors, and we catch it instead. For drivers that are aware of confliction and want the automatic workaround (e.g. HD-audio driver does it already), this kind of code would be useful to be in the common place.
(http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.ht...)
For drivers outside of ALSA SoC part, developers can and should program control element sets with unique identification information, because design of sound card is fixed at development time. Therefore, these codes should be in ALSA SoC core, as I introduced once.
When we have the same requirement for drivers outside of ALSA SoC part, then we're going to move these codes from ALSA SoC core to ALSA control core, I think.
I have generalized this in control.c, precisely because, as you pointed, HDA-audio as same requirement. In other words, this could also seem as an helper for the control indexation. Now no problem to limit it to ASoC as you propose (need to implement it in snd_soc_cnew to take into account prefix).
Now regarding the discussion and the set of patches in my RFC: In my environment, this patch is mandatory to be able to address 'IEC958 Playback Default' control using iecset application. if patch in iecset is accepted, it is no more mandatory. (http://www.spinics.net/lists/alsa-devel/msg56480.html)
So the right way to do it, is to propose the iecset patch in a first step. Then if it is rejected or if you estimate that patch makes sense anyway for other "generic" controls i will rework it and propose it.
Thanks and Regards Arnaud
Bind PCM control to PCM device created during DAI linking.
Change-Id: Idbc6cb8421fd3b8fd6e2fb93c84578410ea7b227 Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- sound/soc/sti/sti_uniperif.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sti/sti_uniperif.c b/sound/soc/sti/sti_uniperif.c index 549fac3..a958144 100644 --- a/sound/soc/sti/sti_uniperif.c +++ b/sound/soc/sti/sti_uniperif.c @@ -245,8 +245,6 @@ static int sti_uniperiph_dai_create_ctrl(struct snd_soc_dai *dai) * Uniperipheral instance ID */ ctrl = &uni->snd_ctrls[i]; - ctrl->index = uni->id; - ctrl->device = uni->id; }
return snd_soc_add_dai_controls(dai, uni->snd_ctrls, uni->num_ctrls); @@ -348,7 +346,8 @@ static int sti_uniperiph_dai_probe(struct snd_soc_dai *dai) static const struct snd_soc_dai_driver sti_uniperiph_dai_template = { .probe = sti_uniperiph_dai_probe, .suspend = sti_uniperiph_dai_suspend, - .resume = sti_uniperiph_dai_resume + .resume = sti_uniperiph_dai_resume, + .bind_pcm_ctl = true, };
static const struct snd_soc_component_driver sti_uniperiph_dai_component = {
ASoc implementation force the index to 0. In case of multi devices that export the iec control, we need device and subdevice fields to differentiate and address the different controls. This patch extends "-D" option to be able to address control using PCM device/sub-device id.
Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- iecset/iecset.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/iecset/iecset.c b/iecset/iecset.c index 92a93e8..8d7a800 100644 --- a/iecset/iecset.c +++ b/iecset/iecset.c @@ -89,6 +89,8 @@ static void usage(void) printf("Usage: iecset [options] [cmd arg...]\n"); printf("Options:\n"); printf(" -D device specifies the control device to use\n"); + printf(" for "-Dhw:X,Y,Z" Only control device X is mandatory\n"); + printf(" the device(Y) and subdevice(Z) are optinal (default = 0)\n"); printf(" -c card specifies the card number to use (equiv. with -Dhw:#)\n"); printf(" -n number specifies the control index number (default = 0)\n"); printf(" -x dump the dump the AESx hex code for IEC958 PCM parameters\n"); @@ -99,6 +101,15 @@ static void usage(void) } }
+static int parse_device(char *parms, int *ctl_dev, int *dev, int *sdev) +{ + + if (strncmp(parms, "hw", 2)) + return 1; + sscanf(parms,"hw:%d,%d,%d",ctl_dev, dev, sdev); + + return 0; +}
/* * parse iecset commands @@ -312,6 +323,7 @@ int main(int argc, char **argv) const char *dev = "default"; const char *spdif_str = SND_CTL_NAME_IEC958("", PLAYBACK, DEFAULT); int spdif_index = -1; + int cdev, spdif_dev = -1, spdif_sdev = -1; snd_ctl_t *ctl; snd_ctl_elem_list_t *clist; snd_ctl_elem_id_t *cid; @@ -330,7 +342,11 @@ int main(int argc, char **argv) while ((c = getopt(argc, argv, "D:c:n:xhi")) != -1) { switch (c) { case 'D': - dev = optarg; + if (parse_device(optarg, &cdev, &spdif_dev, &spdif_sdev)) + dev = optarg; + else + sprintf(tmpname, "hw:%d", cdev); + dev = tmpname; break; case 'c': i = atoi(optarg); @@ -376,15 +392,19 @@ int main(int argc, char **argv) }
controls = snd_ctl_elem_list_get_used(clist); - for (cidx = 0; cidx < controls; cidx++) { - if (!strcmp(snd_ctl_elem_list_get_name(clist, cidx), spdif_str)) - if (spdif_index < 0 || - snd_ctl_elem_list_get_index(clist, cidx) == spdif_index) + for (cidx = 0; cidx < controls; cidx++) + if (!strcmp(snd_ctl_elem_list_get_name(clist, cidx), spdif_str)) { + if ((spdif_index < 0 || + snd_ctl_elem_list_get_index(clist, cidx) == spdif_index) && + (spdif_dev < 0 || + snd_ctl_elem_list_get_device(clist, cidx) == spdif_dev) && + (spdif_sdev < 0 || + snd_ctl_elem_list_get_subdevice(clist, cidx) == spdif_sdev)) break; } if (cidx >= controls) { - fprintf(stderr, "control "%s" (index %d) not found\n", - spdif_str, spdif_index); + fprintf(stderr, "control "%s" (index %d, device %d, subdevice %d) not found\n", + spdif_str, spdif_index, spdif_dev, spdif_sdev); return 1; }
Arnaud Pouliquen wrote:
1-1) How to Instantiate generic ALSA control
The point is the handling of multi instance of generic ALSA controls. In this case "prefix" can not be used. Controls have to be identified by a combination of device/sub-device/index
Controls are identified either - by their numid, or - by the combination of iface/device/subdevice/name/index.
The device/subdevice fields are used to link controls with PCM devices; this is needed for per-stream volume controls or for the S/PDIF "Mask" controls, which set properties of some PCM stream.
Should we always apply this rules? - MIXER control type: as it is not linked to PCM device but card,"index" is used to instantiate control.
"index" is _always_ used.
- PCM control type: as it is linked to PCM device, "device/subdevice" is used to instantiate control. Or could we consider that a control can be instantiated using device/subdevice fields and/or index fields?
The amixer tool does not allow controls with the same iface/name/index values, regardless of the (sub)device values. This appears to be a bug in amixer (because the behaviour is different from the kernel's), but the (sub)device fields were never intended to be used by MIXER controls, and PCM controls typically are inactive when their stream is closed.
- Concerning IEC controls, should we consider them as PCM or MIXER type controls? In current implementations both are used...
MIXER controls are shown in alsamixer. PCM controls are hidden, and accessed by software.
Something like "IEC958 Playback Switch" should be MIXER, and "IEC958 Playback Con Mask", PCM.
- From driver point of view
2-1) None SOC drivers: Today solution seems to base indexation on both "index" and "device" number. Device indexation seems not linked to PCM device number Example in ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts"). http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...
=> The drawback is that each driver has to implement it. Could be nice to have in generic code....
In the good old times, the ALSA framework assumed that a driver knows all its controls, and handles conflicts by assignind index values.
It would be useful to let snd_ctl_add() assign a new index automatically, but only if explicitly requested (e.g., something like "#define SNDRV_CTL_INDEX_AUTO -1").
2-2) SOC drivers:
- Relation chip between control and PCM device. if rules mentioned in 1-3 should be respected, need to link control to PCM device.
What is this link needed for? Is that control set by software (PCM), or by the user (MIXER)?
Regards, Clemens
Hello Clemens,
On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
Arnaud Pouliquen wrote:
1-1) How to Instantiate generic ALSA control
The point is the handling of multi instance of generic ALSA controls. In this case "prefix" can not be used. Controls have to be identified by a combination of device/sub-device/index
Controls are identified either
- by their numid, or
- by the combination of iface/device/subdevice/name/index.
The device/subdevice fields are used to link controls with PCM devices; this is needed for per-stream volume controls or for the S/PDIF "Mask" controls, which set properties of some PCM stream.
Seems that today device/subdevice are also used to differentiate PCM control, using a "virtual" device/subdevice that is not linked to a "real PCM device. This is what i understand when i parse HDA-Intel.conf for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"
Should we always apply this rules? - MIXER control type: as it is not linked to PCM device but card,"index" is used to instantiate control.
"index" is _always_ used.
- PCM control type: as it is linked to PCM device, "device/subdevice" is used to instantiate control. Or could we consider that a control can be instantiated using device/subdevice fields and/or index fields?The amixer tool does not allow controls with the same iface/name/index values, regardless of the (sub)device values. This appears to be a bug in amixer (because the behaviour is different from the kernel's), but the (sub)device fields were never intended to be used by MIXER controls,
In fact only amixer simple controls are not handled using (sub)device fields. if simple control is only MIXER controls, it is not a bug as it respect rules.
and PCM controls typically are inactive when their stream is closed.
if we consider that 'IEC958 Playback Default' is a PCM control. This is not compatible with iecset application that sets the control without opening the PCM device.
- Concerning IEC controls, should we consider them as PCM or MIXER type controls? In current implementations both are used...
MIXER controls are shown in alsamixer. PCM controls are hidden, and accessed by software.
Something like "IEC958 Playback Switch" should be MIXER, and "IEC958 Playback Con Mask", PCM.
- From driver point of view
2-1) None SOC drivers: Today solution seems to base indexation on both "index" and "device" number. Device indexation seems not linked to PCM device number Example in ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts"). http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...
=> The drawback is that each driver has to implement it. Could be nice to have in generic code....
In the good old times, the ALSA framework assumed that a driver knows all its controls, and handles conflicts by assignind index values.
It would be useful to let snd_ctl_add() assign a new index automatically, but only if explicitly requested (e.g., something like "#define SNDRV_CTL_INDEX_AUTO -1").
you right, seems more reasonable, I will update this for a V2, if everyone is ok for this solution...
2-2) SOC drivers:
- Relation chip between control and PCM device. if rules mentioned in 1-3 should be respected, need to link control to PCM device.
What is this link needed for? Is that control set by software (PCM), or by the user (MIXER)?
It is for PCM controls only. Today In ASoC implementation, DAI driver is not directly linked to the PCM device. So it is not possible for PCM controls to set a coherent PCM device field value. For patch proposed , it could also be interesting to use a SNDRV_CTL_DEVICE_AUTO -1 to be coherent with your proposal above.
Thanks and Regards, Arnaud
On Tue, 08 Nov 2016 15:16:29 +0100, Arnaud Pouliquen wrote:
Hello Clemens,
On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
Arnaud Pouliquen wrote:
1-1) How to Instantiate generic ALSA control
The point is the handling of multi instance of generic ALSA controls. In this case "prefix" can not be used. Controls have to be identified by a combination of device/sub-device/index
Controls are identified either
- by their numid, or
- by the combination of iface/device/subdevice/name/index.
The device/subdevice fields are used to link controls with PCM devices; this is needed for per-stream volume controls or for the S/PDIF "Mask" controls, which set properties of some PCM stream.
Seems that today device/subdevice are also used to differentiate PCM control, using a "virtual" device/subdevice that is not linked to a "real PCM device. This is what i understand when i parse HDA-Intel.conf for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"
The HD-audio shouldn't be taken as the good reference. It has its own mapping there just to keep the backward compatibility with the old implementation that was basically broken.
Should we always apply this rules? - MIXER control type: as it is not linked to PCM device but card,"index" is used to instantiate control.
"index" is _always_ used.
- PCM control type: as it is linked to PCM device, "device/subdevice" is used to instantiate control. Or could we consider that a control can be instantiated using device/subdevice fields and/or index fields?The amixer tool does not allow controls with the same iface/name/index values, regardless of the (sub)device values. This appears to be a bug in amixer (because the behaviour is different from the kernel's), but the (sub)device fields were never intended to be used by MIXER controls,
In fact only amixer simple controls are not handled using (sub)device fields. if simple control is only MIXER controls, it is not a bug as it respect rules.
and PCM controls typically are inactive when their stream is closed.
if we consider that 'IEC958 Playback Default' is a PCM control. This is not compatible with iecset application that sets the control without opening the PCM device.
Yes, it's a slight inconsistency. But it's specific to "Default" stuff. All other controls are usually tied with PCM open/close.
- Concerning IEC controls, should we consider them as PCM or MIXER type controls? In current implementations both are used...
MIXER controls are shown in alsamixer. PCM controls are hidden, and accessed by software.
Something like "IEC958 Playback Switch" should be MIXER, and "IEC958 Playback Con Mask", PCM.
- From driver point of view
2-1) None SOC drivers: Today solution seems to base indexation on both "index" and "device" number. Device indexation seems not linked to PCM device number Example in ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts"). http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...
=> The drawback is that each driver has to implement it. Could be nice to have in generic code....
In the good old times, the ALSA framework assumed that a driver knows all its controls, and handles conflicts by assignind index values.
It would be useful to let snd_ctl_add() assign a new index automatically, but only if explicitly requested (e.g., something like "#define SNDRV_CTL_INDEX_AUTO -1").
you right, seems more reasonable, I will update this for a V2, if everyone is ok for this solution...
Right, this behavior must be optional. For the normal drivers, the duplicated controls *are* errors, and we catch it instead. For drivers that are aware of confliction and want the automatic workaround (e.g. HD-audio driver does it already), this kind of code would be useful to be in the common place.
thanks,
Takashi
On 11/08/2016 03:30 PM, Takashi Iwai wrote:
On Tue, 08 Nov 2016 15:16:29 +0100, Arnaud Pouliquen wrote:
Hello Clemens,
On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
Arnaud Pouliquen wrote:
1-1) How to Instantiate generic ALSA control
The point is the handling of multi instance of generic ALSA controls. In this case "prefix" can not be used. Controls have to be identified by a combination of device/sub-device/index
Controls are identified either
- by their numid, or
- by the combination of iface/device/subdevice/name/index.
The device/subdevice fields are used to link controls with PCM devices; this is needed for per-stream volume controls or for the S/PDIF "Mask" controls, which set properties of some PCM stream.
Seems that today device/subdevice are also used to differentiate PCM control, using a "virtual" device/subdevice that is not linked to a "real PCM device. This is what i understand when i parse HDA-Intel.conf for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"
The HD-audio shouldn't be taken as the good reference. It has its own mapping there just to keep the backward compatibility with the old implementation that was basically broken.
Should we always apply this rules? - MIXER control type: as it is not linked to PCM device but card,"index" is used to instantiate control.
"index" is _always_ used.
- PCM control type: as it is linked to PCM device, "device/subdevice" is used to instantiate control. Or could we consider that a control can be instantiated using device/subdevice fields and/or index fields?The amixer tool does not allow controls with the same iface/name/index values, regardless of the (sub)device values. This appears to be a bug in amixer (because the behaviour is different from the kernel's), but the (sub)device fields were never intended to be used by MIXER controls,
In fact only amixer simple controls are not handled using (sub)device fields. if simple control is only MIXER controls, it is not a bug as it respect rules.
and PCM controls typically are inactive when their stream is closed.
if we consider that 'IEC958 Playback Default' is a PCM control. This is not compatible with iecset application that sets the control without opening the PCM device.
Yes, it's a slight inconsistency. But it's specific to "Default" stuff. All other controls are usually tied with PCM open/close.
Thanks for the clarification. In this case, "[RFC 4/4 ] iecset: allow to select control with device and sub-device numbers" seems coherent, to allow access to this PCM controls with device/subdevice values... I will propose it in a separate patch-set to not mix kernel and application patch...
As 'IEC958 Playback Default' is specific, i will also try to rework and propose a new version of [PATCH v4 3/6] ALSA: pcm: add IEC958 channel status control helper (http://www.spinics.net/lists/alsa-devel/msg47615.html)
- Concerning IEC controls, should we consider them as PCM or MIXER type controls? In current implementations both are used...
MIXER controls are shown in alsamixer. PCM controls are hidden, and accessed by software.
Something like "IEC958 Playback Switch" should be MIXER, and "IEC958 Playback Con Mask", PCM.
- From driver point of view
2-1) None SOC drivers: Today solution seems to base indexation on both "index" and "device" number. Device indexation seems not linked to PCM device number Example in ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts"). http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...
=> The drawback is that each driver has to implement it. Could be nice to have in generic code....
In the good old times, the ALSA framework assumed that a driver knows all its controls, and handles conflicts by assignind index values.
It would be useful to let snd_ctl_add() assign a new index automatically, but only if explicitly requested (e.g., something like "#define SNDRV_CTL_INDEX_AUTO -1").
you right, seems more reasonable, I will update this for a V2, if everyone is ok for this solution...
Right, this behavior must be optional. For the normal drivers, the duplicated controls *are* errors, and we catch it instead. For drivers that are aware of confliction and want the automatic workaround (e.g. HD-audio driver does it already), this kind of code would be useful to be in the common place.
I will send a V2 that integrates SNDRV_CTL_INDEX_AUTO (and SNDRV_CTL_DEVICE_AUTO for the ASoC PCM link)
Thanks & Regards Arnaud
Hi,
On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
- Patches proposed:
Based on these observations, here are some patches that i tested in my environment. There are complementary, and could answer to some limitations mentioned above.
3-1) Alsa-utils patch
- iecset: allow to select control with device and sub-device numbers This patch allows to access to 2 iec controls differentiated by device/sub-devices numbers
=> For me, this patch is mandatory to be able to address the ASoC IEC controls, in case of no fix is implemented to allows index field update in ASoC.
3-2) Alsa driver patches
- ASoC: core: allow PCM control binding to PCM device
Add relationship between DAIs PCM controls and PCM device.
ALSA: control: increment index field for duplicated control. Generic implementation of the patch proposed in HDA driver (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...)
ASoC: sti: use bind_pcm_ctl
implementation of bind_pcm_ctl for sti driver.
In my view, this patchset includes two attempts: 1.to add a framework into ALSA SoC part to relate some control element sets to PCM devices 2.to allow drivers in ALSA SoC part to add some control element sets from the same codes according to entries of dtb
To achieve above 2nd attempt, it changes ALSA control core to accept several control element sets with the same name by assigning proper indexes to the sets.
Well, since your former messages, you continue using the word, 'general' or 'generic'. If it were somewhat general, it should satisfy requirements in whole this subsystem. Actually, there's none of such requirements outside of ALSA SoC part. For the drivers outside of ALSA SoC part, design of target sound card is fixed in advance, therefore programmers can assign control element sets to PCM devices in usual ways. At least, current infrastructure in ALSA control core can satisfy the programmers.
Therefore, I think that your attempts includes over-generalization. In theory, it can bring over-specification easily and it causes code complication or unmaintained codes.
Focusing on ALSA SoC part, there's a requirement to register control element sets from the same code, in fact. So I wonder why you don't start your explanation in an aspect of it. In short, I can't understand the reason that you adhere to such an inappropriate generalization for this subsystem.
In this patchset, you adhere to usage of 'index' field. But there's another way; assigning different identification information to the control element sets. Let us to start discussion from this point? At least, Iwai-san said, former driver for Intel High Definition Audio is necessarily not a good example for a model to c onsider about this issue and the usage of 'index' is not well-generalized[1].
Finally, it's better to clear main points of your issue, for practical and meaningful discussion for better approaches, before starting this discussion, I think. At least, there's over-generalization, misunderstandings of design and interfaces, driver-specific historical reasons and so on.
[1] [alsa-devel] [RFC 0/4] ALSA controls management using index/device/sub-devices fields http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.ht...
Regards
Takashi Sakamoto
Hi,
On 11/09/2016 01:33 PM, Takashi Sakamoto wrote:
Hi,
On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
- Patches proposed:
Based on these observations, here are some patches that i tested in my environment. There are complementary, and could answer to some limitations mentioned above.
3-1) Alsa-utils patch
- iecset: allow to select control with device and sub-device numbers This patch allows to access to 2 iec controls differentiated by device/sub-devices numbers
=> For me, this patch is mandatory to be able to address the ASoC IEC controls, in case of no fix is implemented to allows index field update in ASoC.
3-2) Alsa driver patches
- ASoC: core: allow PCM control binding to PCM device
Add relationship between DAIs PCM controls and PCM device.
ALSA: control: increment index field for duplicated control. Generic implementation of the patch proposed in HDA driver (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b4...)
ASoC: sti: use bind_pcm_ctl
implementation of bind_pcm_ctl for sti driver.
In my view, this patchset includes two attempts: 1.to add a framework into ALSA SoC part to relate some control element sets to PCM devices 2.to allow drivers in ALSA SoC part to add some control element sets from the same codes according to entries of dtb
To achieve above 2nd attempt, it changes ALSA control core to accept several control element sets with the same name by assigning proper indexes to the sets.
Well, since your former messages, you continue using the word, 'general' or 'generic'. If it were somewhat general, it should satisfy requirements in whole this subsystem. Actually, there's none of such requirements outside of ALSA SoC part. For the drivers outside of ALSA SoC part, design of target sound card is fixed in advance, therefore programmers can assign control element sets to PCM devices in usual ways. At least, current infrastructure in ALSA control core can satisfy the programmers.
Therefore, I think that your attempts includes over-generalization. In theory, it can bring over-specification easily and it causes code complication or unmaintained codes.
I based my RFC on the fact that i was facing same kind of problem than HDA driver (last time i mention it). In this context, i don't think that it was senseless to have a global approach. If not the good approach, let's refocus on ASoC, that's fine by me.
Focusing on ALSA SoC part, there's a requirement to register control element sets from the same code, in fact. So I wonder why you don't start your explanation in an aspect of it. In short, I can't understand the reason that you adhere to such an inappropriate generalization for this subsystem.
In this patchset, you adhere to usage of 'index' field. But there's another way; assigning different identification information to the control element sets. Let us to start discussion from this point? At least, Iwai-san said, former driver for Intel High Definition Audio is necessarily not a good example for a model to c onsider about this issue and the usage of 'index' is not well-generalized[1].
Finally, it's better to clear main points of your issue, for practical and meaningful discussion for better approaches, before starting this discussion, I think. At least, there's over-generalization, misunderstandings of design and interfaces, driver-specific historical reasons and so on.
[1] [alsa-devel] [RFC 0/4] ALSA controls management using index/device/sub-devices fields http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.ht...
So I propose to continue discussion on a simple and concrete use case: The 'IEC958 Playback Default' control.
In my ASoC driver i have one HDMI and one SPDIF outputs. so I have 2'IEC958 Playback Default' PCM controls. => Each control can be set independently.
Regarding control identification field (struct snd_ctl_elem_i): .numid; => set by ALSA framework .iface; => must be SNDRV_CTL_ELEM_IFACE_PCM .device; => must be linked to PCM device , but not possible for ASoC DAI... .subdevice => not used in ASoC implementation .name => 'IEC958 Playback Default' .index => not used in ASoC, forced to 0 by snd_soc_cnew
Other solution: use control "prefix"? not possible as control has a pre-defined name.
=> Only solution to differentiate the control: "device" field. (that seems coherent as it a PCM device).
Issues: - use "device" in ASOC DAI driver means that driver needs to define a "virtual" PCM device value, not the PCM device. => this break the rule that mention that PCM control should be linked to a PCM device. Furthermore, this "virtual" value has to be aligned with the one defined in alsa-lib conf file(s). - iecset uses only index to differentiate IEC controls. But in ASoC implementation this is not possible as index is forced to 0.
Regards Arnaud
Hi,
On Nov 9 2016 23:38, Arnaud Pouliquen wrote:
In my view, this patchset includes two attempts: 1.to add a framework into ALSA SoC part to relate some control element sets to PCM devices 2.to allow drivers in ALSA SoC part to add some control element sets from the same codes according to entries of dtb
To achieve above 2nd attempt, it changes ALSA control core to accept several control element sets with the same name by assigning proper indexes to the sets.
Well, since your former messages, you continue using the word, 'general' or 'generic'. If it were somewhat general, it should satisfy requirements in whole this subsystem. Actually, there's none of such requirements outside of ALSA SoC part. For the drivers outside of ALSA SoC part, design of target sound card is fixed in advance, therefore programmers can assign control element sets to PCM devices in usual ways. At least, current infrastructure in ALSA control core can satisfy the programmers.
Therefore, I think that your attempts includes over-generalization. In theory, it can bring over-specification easily and it causes code complication or unmaintained codes.
I based my RFC on the fact that i was facing same kind of problem than HDA driver (last time i mention it). In this context, i don't think that it was senseless to have a global approach.
Unless you have special interests in verb parser of HDA specification and work for it, it's better not to address to it. Else, you would be involved into more complicated stories; old and new Intel HDA driver, historical reasons of former Intel HDA driver, variations of HDA codecs produced by several vendors, vendor-dependent quirks of ACPI table, and so on.
If not the good approach, let's refocus on ASoC, that's fine by me.
In my taste, this approach is better for your main issue. After solving the issue and you have enough rest to work for more-generic one, you could do it.
Focusing on ALSA SoC part, there's a requirement to register control element sets from the same code, in fact. So I wonder why you don't start your explanation in an aspect of it. In short, I can't understand the reason that you adhere to such an inappropriate generalization for this subsystem.
In this patchset, you adhere to usage of 'index' field. But there's another way; assigning different identification information to the control element sets. Let us to start discussion from this point? At least, Iwai-san said, former driver for Intel High Definition Audio is necessarily not a good example for a model to c onsider about this issue and the usage of 'index' is not well-generalized[1].
Finally, it's better to clear main points of your issue, for practical and meaningful discussion for better approaches, before starting this discussion, I think. At least, there's over-generalization, misunderstandings of design and interfaces, driver-specific historical reasons and so on.
[1] [alsa-devel] [RFC 0/4] ALSA controls management using index/device/sub-devices fields http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.ht...
So I propose to continue discussion on a simple and concrete use case: The 'IEC958 Playback Default' control.
In my ASoC driver i have one HDMI and one SPDIF outputs. so I have 2'IEC958 Playback Default' PCM controls. => Each control can be set independently.
Yes. It's a demand in your issue. (here, I ignore a process to generalize the issue for wider range in this subsystem because it's another issue.)
Regarding control identification field (struct snd_ctl_elem_i): .numid; => set by ALSA framework .iface; => must be SNDRV_CTL_ELEM_IFACE_PCM .device; => must be linked to PCM device , but not possible for ASoC DAI... .subdevice => not used in ASoC implementation .name => 'IEC958 Playback Default' .index => not used in ASoC, forced to 0 by snd_soc_cnew
Other solution: use control "prefix"? not possible as control has a pre-defined name.
=> Only solution to differentiate the control: "device" field. (that seems coherent as it a PCM device).
This is one of solutions we can perform for this issue. As a feasibility study, in the end of this message, I wrote a small program with a feature of 'user-defined control element set' in ALSA control core.
I think usage of 'device' field is better than usage of prefix for 'name' field. It clearly represents the relationship between control element set and PCM devices. And it's just suitable for this issue.
Issues: - use "device" in ASOC DAI driver means that driver needs to define a "virtual" PCM device value, not the PCM device. => this break the rule that mention that PCM control should be linked to a PCM device.
Please show me where related codes and structures are. At least, I cannot understand what you said because it's really abstracted.
Furthermore, this "virtual" value has to be aligned with the one defined in alsa-lib conf file(s).
Ditto.
- iecset uses only index to differentiate IEC controls. But in ASoC implementation this is not possible as index is forced to 0.
_Apparently_, mixer APIs in alsa-lib is not well-designed to represent capacity of ALSA control core. It's not better to judge somwthing according to its design.
Although we need to improve iecset tool, this is another issue.
-------- 8< --------
#include <stdio.h> #include <stdlib.h>
#include <string.h> #include <errno.h>
#include <sys/types.h> #include <sys/stat.h> #include <fcntl.h>
#include <sys/ioctl.h>
#include <unistd.h>
#include <sound/asound.h>
int main(void) { int fd; struct snd_ctl_elem_info info = {0}; int first_numid = 0; int second_numid = 0;
fd = open("/dev/snd/controlC0", O_RDWR); if (fd < 0) { printf("open(2): %s\n", strerror(errno)); return EXIT_FAILURE; }
/* Identification information. */ info.id.iface = SNDRV_CTL_ELEM_IFACE_PCM; info.id.subdevice = 0; strncpy(info.id.name, "IEC958 Playback Default", sizeof(info.id.name)); info.id.index = 0;
/* Common information. */ info.type = SNDRV_CTL_ELEM_TYPE_IEC958; info.access = SNDRV_CTL_ELEM_ACCESS_READWRITE; info.count = 1; info.owner = 1;
/* Add the first control element set. */ info.id.numid = 0; info.id.device = 0; if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) { printf("ioctl(2): %s\n", strerror(errno)); goto end; } first_numid = info.id.numid;
/* Add the second control element set. */ info.id.numid = 0; info.id.device = 1; if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) { } second_numid = info.id.numid; end: if (first_numid > 0) { info.id.numid = first_numid; ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info); }
if (second_numid > 0) { info.id.numid = second_numid; ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info); }
close(fd);
return EXIT_SUCCESS; }
Regards
Takashi Sakamoto
hello Takashi,
On 11/12/2016 05:23 AM, Takashi Sakamoto wrote:
So I propose to continue discussion on a simple and concrete use case: The 'IEC958 Playback Default' control.
In my ASoC driver i have one HDMI and one SPDIF outputs. so I have 2'IEC958 Playback Default' PCM controls. => Each control can be set independently.
Yes. It's a demand in your issue. (here, I ignore a process to generalize the issue for wider range in this subsystem because it's another issue.)
Regarding control identification field (struct snd_ctl_elem_i): .numid; => set by ALSA framework .iface; => must be SNDRV_CTL_ELEM_IFACE_PCM .device; => must be linked to PCM device , but not possible for ASoC DAI... .subdevice => not used in ASoC implementation .name => 'IEC958 Playback Default' .index => not used in ASoC, forced to 0 by snd_soc_cnew
Other solution: use control "prefix"? not possible as control has a pre-defined name.
=> Only solution to differentiate the control: "device" field. (that seems coherent as it a PCM device).
This is one of solutions we can perform for this issue. As a feasibility study, in the end of this message, I wrote a small program with a feature of 'user-defined control element set' in ALSA control core.
I think usage of 'device' field is better than usage of prefix for 'name' field. It clearly represents the relationship between control element set and PCM devices. And it's just suitable for this issue.
This is also my view. Now base on your mail, that point possibility to extend name (http://www.spinics.net/lists/alsa-devel/msg56713.html):
I note that we can give unique names to control element set for IEC 60958. According to this document, Between "IEC958" and the rest, we can put arbitrary string.
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/Documentat...
I missed this, Seems that name is also another solution for IEC controls. Gstreamer and pulseaudio use PCM name to set the AES value (e.g. 'B2120.pcm.hdmi.0:CARD=0,AES0=4,AES1=130,AES2=0,AES3=2' so simple to adapt alsa conf file with good name. iecset could be adapted in a quite simple way using device name or an new option ( e.g. "-e name control name extension IEC958 <name> Playback Default")
But, perhaps better to use device... as it match with expected PCM control rule and as it could be applied for controls based on standard syntax: [LOCATION] SOURCE [CHANNEL] [DIRECTION] FUNCTION
Issues: - use "device" in ASOC DAI driver means that driver needs to define a "virtual" PCM device value, not the PCM device. => this break the rule that mention that PCM control should be linked to a PCM device.
Please show me where related codes and structures are. At least, I cannot understand what you said because it's really abstracted.
As example, here is my current implementation (correspond to the piece of code i would like to rework): http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232 - "device" field is set to the instance ID of the DAI uniperiph player. - "index" filed is also set but overwritten by ASoC core. In this implementation what i name "virtual device is the "device" value uni->id HDMI: uni->id = 0 ( PCM device associated is hw:0,0) SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)
If i don't set the device value then i can not create both controls. A better way should be to set device to PCM device. This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).
Furthermore, this "virtual" value has to be aligned with the one defined in alsa-lib conf file(s).Ditto.
I have not upstreamed my card conf file but an example is available at end of my mail. Value of the device field must be aligned with ASoC driver device values: HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0 SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
If these controls were associated to the PCM device, I should not have to fix the value but I should be able to retrieve it.
- iecset uses only index to differentiate IEC controls. But in ASoC implementation this is not possible as index is forced to 0._Apparently_, mixer APIs in alsa-lib is not well-designed to represent capacity of ALSA control core. It's not better to judge somwthing according to its design.
Not sure to follow you... I can not see any issue in alsa-lib. Just a limitation of using iecset with ASoC implementation.
Although we need to improve iecset tool, this is another issue.
From my point of view, iecset is part of the topic even if i agree that
details on potential update could/should be discuss in a separate thread. In case, PCM control device value is not aligned with PCM device, a workaround exists, even if it does not respect the alignment between PCM control and PCM device... For iecset, i have no solution except patch it, if we consider that index field incrementation is not the good solution for PCM controls. So for me it is important to keep it in mind in discussions.
-------------
confdir:pcm/iec958.conf confdir:pcm/hdmi.conf
STI-B2120.pcm.iec958.common { @args [ CARD DEVICE CTLINDEX AES0 AES1 AES2 AES3 ] @args.CARD { type string } @args.DEVICE { type integer } @args.CTLINDEX { type integer } @args.AES0 { type integer # consumer, not-copyright, emphasis-none, mode=0 default 0x00 } @args.AES1 { type integer # original, PCM coder default 0x00 } @args.AES2 { type integer # source and channel default 0x00 } @args.AES3 { type integer # fs=48000Hz, clock accuracy=1000ppm default 0x00 } type hooks slave.pcm { type hw card $CARD device $DEVICE } hooks.0 { type ctl_elems hook_args [ { interface PCM name "IEC958 Playback Default" device 3 lock true preserve true value [ $AES0 $AES1 $AES2 $AES3 ] } ] } hint.device $DEVICE }
STI-B2120.pcm.iec958.0 { @args [ CARD AES0 AES1 AES2 AES3 ] @args.CARD { type string } @args.AES0 { type integer } @args.AES1 { type integer } @args.AES2 { type integer } @args.AES3 { type integer } @func refer name { @func concat strings [ "cards.STI-B2120.pcm.iec958.common:" "CARD=" $CARD "," "DEVICE=2," "CTLINDEX=0," "AES0=" $AES0 "," "AES1=" $AES1 "," "AES2=" $AES2 "," "AES3=" $AES3 ] } }
STI-B2120.pcm.hdmi.common { @args [ CARD DEVICE CTLINDEX AES0 AES1 AES2 AES3 ] @args.CARD { type string default 0 } @args.DEVICE { type integer default 0 } @args.CTLINDEX { type integer } @args.AES0 { type integer # consumer, not-copyright, emphasis-none, mode=0 default 0x00 } @args.AES1 { type integer # original, PCM coder default 0x00 } @args.AES2 { type integer # source and channel default 0x00 } @args.AES3 { type integer # fs=48000Hz, clock accuracy=1000ppm default 0x00I missed this, Seems that name is also another solution for IEC controls.
} type hooks slave.pcm { type hw card $CARD device $DEVICE } hooks.0 { type ctl_elems hook_args [ { interface PCM name "IEC958 Playback Default" device 0 lock true preserve true value [ $AES0 $AES1 $AES2 $AES3 ] } ] } hint.device $DEVICE } STI-B2120.pcm.hdmi.0 { @args [ CARD AES0 AES1 AES2 AES3 ] @args.CARD { type string } @args.AES0 { type integer } @args.AES1 { type integer } @args.AES2 { type integer } @args.AES3 { type integer } @func refer name { @func concat strings [ "cards.STI-B2120.pcm.hdmi.common:" "CARD=" $CARD "," "DEVICE=0," "CTLINDEX=0," "AES0=" $AES0 "," "AES1=" $AES1 "," "AES2=" $AES2 "," "AES3=" $AES3 ] } }
Thanks and Regards Arnaud
On Nov 14 2016 22:58, Arnaud Pouliquen wrote:
Issues: - use "device" in ASOC DAI driver means that driver needs to define a "virtual" PCM device value, not the PCM device. => this break the rule that mention that PCM control should be linked to a PCM device.
Please show me where related codes and structures are. At least, I cannot understand what you said because it's really abstracted.
As example, here is my current implementation (correspond to the piece of code i would like to rework): http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232
- "device" field is set to the instance ID of the DAI uniperiph player.
- "index" filed is also set but overwritten by ASoC core.
In this implementation what i name "virtual device is the "device" value uni->id HDMI: uni->id = 0 ( PCM device associated is hw:0,0) SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)
If i don't set the device value then i can not create both controls. A better way should be to set device to PCM device. This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).
In below call graph, each module cannot directly indicate the 'device' number because 'struct snd_soc_card.num_rtd' is incremented by ALSA SoC core and the number is used as an argument for 'snd_pcm_new_internal()' or 'snd_pcm_new()'.
(sound/soc/soc-core.c) snd_soc_register_card() ->snd_soc_instantiate_card() ->soc_bind_dai_link() (each on card->num_links/dai_link_list) ->soc_new_pcm_runtime() ->soc_add_pcm_runtime() list_add_tail(&card->rtd_list) rtd->num = card->num_rtd card->num_rtd++ ->soc_probe_link_dais() (each on card->rtd_list) (sound/soc/soc-pcm.c) ->soc_new_pcm(rtd, num = rtd->num) ->snd_pcm_new_internal(num) ->snd_pcm_new(num) struct snd_soc_pcm_runtime.pcm = pcm ->snd_card_regiser()
Thus, modules are forced to 'guess' the card structure in a point of PCM character devices. I think you expressed this as 'virtual'.
Well, as a glance of ALSA SoC core, we have a call of 'struct snd_soc_card.late_probe()' just before 'snd_card_register()'. We can probably add ad-hoc control element sets for each PCM character devices.
snd_soc_register_card() ->snd_soc_instantiate_card() ->soc_bind_dai_link() ->soc_probe_link_dais() ->snd_soc_add_card_controls() ->struct snd_soc_card.late_probe() ->snd_card_register()
But it seems to be a bit cumbersome. I think it better to add a smart framework for the PCM-related controls. In this point, I agree with your direction.
Furthermore, this "virtual" value has to be aligned with the one defined in alsa-lib conf file(s).Ditto.
I have not upstreamed my card conf file but an example is available at end of my mail. Value of the device field must be aligned with ASoC driver device values: HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0 SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
If these controls were associated to the PCM device, I should not have to fix the value but I should be able to retrieve it.
As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different strategy to handle PCM-related control element set for IEC958 type. Your configuration does not work well for pcm.iec958 plugin.
- iecset uses only index to differentiate IEC controls. But in ASoC implementation this is not possible as index is forced
to 0.
_Apparently_, mixer APIs in alsa-lib is not well-designed to represent capacity of ALSA control core. It's not better to judge somwthing according to its design.
Not sure to follow you... I can not see any issue in alsa-lib. Just a limitation of using iecset with ASoC implementation.
Although we need to improve iecset tool, this is another issue.
From my point of view, iecset is part of the topic even if i agree that details on potential update could/should be discuss in a separate thread. In case, PCM control device value is not aligned with PCM device, a workaround exists, even if it does not respect the alignment between PCM control and PCM device... For iecset, i have no solution except patch it, if we consider that index field incrementation is not the good solution for PCM controls. So for me it is important to keep it in mind in discussions.
Yes, enough later, we need to discuss about user space.
However, your issue includes a batch of issues on both of kernel land and user land. Intractableness, toughness, hardness, something like that for each of them is not so light and small. I think it better to solve these child issues step by steap. In this context, I said 'another'.
Regards
Takashi Sakamoto
On 11/16/2016 10:34 AM, Takashi Sakamoto wrote:
On Nov 14 2016 22:58, Arnaud Pouliquen wrote:
Issues: - use "device" in ASOC DAI driver means that driver needs to define a "virtual" PCM device value, not the PCM device. => this break the rule that mention that PCM control should be linked to a PCM device.
Please show me where related codes and structures are. At least, I cannot understand what you said because it's really abstracted.
As example, here is my current implementation (correspond to the piece of code i would like to rework): http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232
- "device" field is set to the instance ID of the DAI uniperiph player.
- "index" filed is also set but overwritten by ASoC core.
In this implementation what i name "virtual device is the "device" value uni->id HDMI: uni->id = 0 ( PCM device associated is hw:0,0) SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)
If i don't set the device value then i can not create both controls. A better way should be to set device to PCM device. This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).
In below call graph, each module cannot directly indicate the 'device' number because 'struct snd_soc_card.num_rtd' is incremented by ALSA SoC core and the number is used as an argument for 'snd_pcm_new_internal()' or 'snd_pcm_new()'.
(sound/soc/soc-core.c) snd_soc_register_card() ->snd_soc_instantiate_card() ->soc_bind_dai_link() (each on card->num_links/dai_link_list) ->soc_new_pcm_runtime() ->soc_add_pcm_runtime() list_add_tail(&card->rtd_list) rtd->num = card->num_rtd card->num_rtd++ ->soc_probe_link_dais() (each on card->rtd_list) (sound/soc/soc-pcm.c) ->soc_new_pcm(rtd, num = rtd->num) ->snd_pcm_new_internal(num) ->snd_pcm_new(num) struct snd_soc_pcm_runtime.pcm = pcm ->snd_card_regiser()
Thus, modules are forced to 'guess' the card structure in a point of PCM character devices. I think you expressed this as 'virtual'.
Yes. Or fix an arbitrary PCM device value if not possible to guess.
Well, as a glance of ALSA SoC core, we have a call of 'struct snd_soc_card.late_probe()' just before 'snd_card_register()'. We can probably add ad-hoc control element sets for each PCM character devices.
snd_soc_register_card() ->snd_soc_instantiate_card() ->soc_bind_dai_link() ->soc_probe_link_dais() ->snd_soc_add_card_controls() ->struct snd_soc_card.late_probe() ->snd_card_register()
But it seems to be a bit cumbersome. I think it better to add a smart framework for the PCM-related controls. In this point, I agree with your direction.
Another point is that, at least in theory, a DAI driver can add a control after card creation. In this case PCM control should also be associated to PCM device. Late probe could not handle it.
If we are aligned on direction, what is your suggestion to continue discussion? Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow PCM control binding to PCM device? Other?
Furthermore, this "virtual" value has to be aligned with the one defined in alsa-lib conf file(s).Ditto.
I have not upstreamed my card conf file but an example is available at end of my mail. Value of the device field must be aligned with ASoC driver device values: HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0 SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
If these controls were associated to the PCM device, I should not have to fix the value but I should be able to retrieve it.
As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different strategy to handle PCM-related control element set for IEC958 type. Your configuration does not work well for pcm.iec958 plugin.
Please could you detail? HDMI supports more configurations than SPDIF (e.g HBRA). Furthermore application can retrieve HDMI sinks capability with EDID. So full agree that strategy can be different. That's why Two IEC controls are needed. But focusing on 'pcm/iec958.conf' and 'pcm/hdmi.conf', they are almost identical...
Thanks and Regards
Arnaud
On Nov 6 2016 22:43, Arnaud Pouliquen wrote:
Well, as a glance of ALSA SoC core, we have a call of 'struct snd_soc_card.late_probe()' just before 'snd_card_register()'. We can probably add ad-hoc control element sets for each PCM character devices.
snd_soc_register_card() ->snd_soc_instantiate_card() ->soc_bind_dai_link() ->soc_probe_link_dais() ->snd_soc_add_card_controls() ->struct snd_soc_card.late_probe() ->snd_card_register()
But it seems to be a bit cumbersome. I think it better to add a smart framework for the PCM-related controls. In this point, I agree with your direction.
Another point is that, at least in theory, a DAI driver can add a control after card creation. In this case PCM control should also be associated to PCM device. Late probe could not handle it.
I don't think so. But in your case, 'snd-soc-simple-card' is used to maintain own sound card instance, and this module has no handlers for the callback. So we don't utilize the callback for this aim unless integrating the module.
If we are aligned on direction, what is your suggestion to continue discussion? Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow PCM control binding to PCM device? Other?
If you focused on ALSA SoC part only, I'd not continue this discussion more. I'm not a developer for the part, and join in this discussion just for ALSA control interface.
I have not upstreamed my card conf file but an example is available at end of my mail. Value of the device field must be aligned with ASoC driver device values: HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0 SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
If these controls were associated to the PCM device, I should not have to fix the value but I should be able to retrieve it.
As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different strategy to handle PCM-related control element set for IEC958 type. Your configuration does not work well for pcm.iec958 plugin.
Please could you detail? HDMI supports more configurations than SPDIF (e.g HBRA). Furthermore application can retrieve HDMI sinks capability with EDID. So full agree that strategy can be different. That's why Two IEC controls are needed. But focusing on 'pcm/iec958.conf' and 'pcm/hdmi.conf', they are almost identical...
I prefer discuss after deciding your approach on kernel side. But in previous message I misunderstood that S/PDIF interface of your STI SoC in system side can handle IEC 60958 sub-frame. Actually, it seems not to be. In this case, usage of pcm hook for 'ctl_elems' type might be appropriate. I'm sorry to have confused you.
Regards
Takashi Sakamoto
If we are aligned on direction, what is your suggestion to continue discussion? Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow PCM control binding to PCM device? Other?
If you focused on ALSA SoC part only, I'd not continue this discussion more. I'm not a developer for the part, and join in this discussion just for ALSA control interface.
Anyway thanks for your time and your support.
Regards Arnaud
On Nov 9 2016 23:38, Arnaud Pouliquen wrote:
Other solution: use control "prefix"? not possible as control has a pre-defined name.
I note that we can give unique names to control element set for IEC 60958. According to this document, Between "IEC958" and the rest, we can put arbitrary string. https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/Documentat...
For your information.
Takashi Sakamoto
participants (4)
-
Arnaud Pouliquen -
Clemens Ladisch -
Takashi Iwai -
Takashi Sakamoto