| Message ID | 20200323193623.3587-1-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: core: sysfs: show components string | expand |
On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: > Add attribute and show the components string. This is useful to see > what is provided to userspace and e.g. used by UCM to understand the > card configuration: > > root@plb:~# more /sys/class/sound/card0/components > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 Sysfs is supposed to be one value per file so this should be a directory with a file for each component I guess.
On 3/23/20 2:41 PM, Mark Brown wrote: > On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: >> Add attribute and show the components string. This is useful to see >> what is provided to userspace and e.g. used by UCM to understand the >> card configuration: >> >> root@plb:~# more /sys/class/sound/card0/components >> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 > > Sysfs is supposed to be one value per file so this should be a directory > with a file for each component I guess. that's fair, but the 'value' is already a string here. There's no syntax or grammar that would define what the contents of the string would be, so it'd be difficult to define any sort of ABI. Are you saying sysfs can't be used here?
On Mon, 23 Mar 2020 20:41:42 +0100, Mark Brown wrote: > > On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: > > Add attribute and show the components string. This is useful to see > > what is provided to userspace and e.g. used by UCM to understand the > > card configuration: > > > > root@plb:~# more /sys/class/sound/card0/components > > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 > > Sysfs is supposed to be one value per file so this should be a directory > with a file for each component I guess. Well, the actual representation of components itself is CSV, so this can be OK, IMO. Takashi
Hi, On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: > Add attribute and show the components string. This is useful to see > what is provided to userspace and e.g. used by UCM to understand the > card configuration: > > root@plb:~# more /sys/class/sound/card0/components > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 > > Note that the style uses what's recommended by checkpatch.pl and is > slightly different from other sysfs attributes. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > sound/core/init.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) I have a concern about this patch in a point of userspace interface. The 'component' field of 'struct snd_ctl_card_info' is just defined to have strings with space-separated chunks, and its actual value is not so fine-documented, thus it's largely different depending of developers of each driver. $ cat include/uapi/sound/asound.h ... 941 struct snd_ctl_card_info { ... 950 unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */ 951 }; On the other hand, the node of sysfs is quite common in Linux because it's tightly coupled to kernel objects. Let you see files under 'Documentation/ABI/'. We can find efforts to maintain sysfs node so safe and stable. Due to this reason, it's better to take more care when adding new node. Would I request you the reason to add this node and the reason that current ALSA control interface doesn't satisfy your requirement? > diff --git a/sound/core/init.c b/sound/core/init.c > index b02a99766351..decaf944a8ad 100644 > --- a/sound/core/init.c > +++ b/sound/core/init.c > @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev, > > static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL); > > +static ssize_t > +components_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct snd_card *card = container_of(dev, struct snd_card, card_dev); > + > + return scnprintf(buf, PAGE_SIZE, "%s\n", card->components); > +} > + > +static DEVICE_ATTR_RO(components); > + > static struct attribute *card_dev_attrs[] = { > &dev_attr_id.attr, > &dev_attr_number.attr, > + &dev_attr_components.attr, > NULL > }; > > -- > 2.20.1 Regards Takashi Sakamoto
On 3/23/20 8:53 PM, Takashi Sakamoto wrote: > Hi, > > On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: >> Add attribute and show the components string. This is useful to see >> what is provided to userspace and e.g. used by UCM to understand the >> card configuration: >> >> root@plb:~# more /sys/class/sound/card0/components >> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 >> >> Note that the style uses what's recommended by checkpatch.pl and is >> slightly different from other sysfs attributes. >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> sound/core/init.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) > > I have a concern about this patch in a point of userspace interface. > > The 'component' field of 'struct snd_ctl_card_info' is just defined to > have strings with space-separated chunks, and its actual value is not > so fine-documented, thus it's largely different depending of developers > of each driver. In case you missed it, the components are used by machine drivers to report e.g. number of speakers, mics, etc, so that UCM can find the right configuration. For a given family of products, the syntax will be fixed, e.g. hs stands for headset codec, etc. > $ cat include/uapi/sound/asound.h > ... > 941 struct snd_ctl_card_info { > ... > 950 unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */ > 951 }; > > On the other hand, the node of sysfs is quite common in Linux because > it's tightly coupled to kernel objects. Let you see files under > 'Documentation/ABI/'. We can find efforts to maintain sysfs node so > safe and stable. Due to this reason, it's better to take more care when > adding new node. > > Would I request you the reason to add this node and the reason that > current ALSA control interface doesn't satisfy your requirement? simplicity for user support. Anyone can peak at a sysfs file, not everyone is familiar with the alsa control interface. We get lots of bug reports from people who are asking for configuration help, and the simpler the command is the better. >> diff --git a/sound/core/init.c b/sound/core/init.c >> index b02a99766351..decaf944a8ad 100644 >> --- a/sound/core/init.c >> +++ b/sound/core/init.c >> @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev, >> >> static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL); >> >> +static ssize_t >> +components_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct snd_card *card = container_of(dev, struct snd_card, card_dev); >> + >> + return scnprintf(buf, PAGE_SIZE, "%s\n", card->components); >> +} >> + >> +static DEVICE_ATTR_RO(components); >> + >> static struct attribute *card_dev_attrs[] = { >> &dev_attr_id.attr, >> &dev_attr_number.attr, >> + &dev_attr_components.attr, >> NULL >> }; >> >> -- >> 2.20.1 > > > Regards > > Takashi Sakamoto >
On Mon, Mar 23, 2020 at 10:34:23PM -0500, Pierre-Louis Bossart wrote: > On 3/23/20 8:53 PM, Takashi Sakamoto wrote: > > Hi, > > > > On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote: > > > Add attribute and show the components string. This is useful to see > > > what is provided to userspace and e.g. used by UCM to understand the > > > card configuration: > > > > > > root@plb:~# more /sys/class/sound/card0/components > > > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 > > > > > > Note that the style uses what's recommended by checkpatch.pl and is > > > slightly different from other sysfs attributes. > > > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > > --- > > > sound/core/init.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > I have a concern about this patch in a point of userspace interface. > > > > The 'component' field of 'struct snd_ctl_card_info' is just defined to > > have strings with space-separated chunks, and its actual value is not > > so fine-documented, thus it's largely different depending of developers > > of each driver. > > In case you missed it, the components are used by machine drivers to report > e.g. number of speakers, mics, etc, so that UCM can find the right > configuration. For a given family of products, the syntax will be fixed, > e.g. hs stands for headset codec, etc. Actually the syntax is just fixed to devices which a part of ALSA in-kernel driver supports. There's no specification widely used to all of ALSA in-kernel drivers. (Of cource, it covers many of actual devices which assumed users own.) > > $ cat include/uapi/sound/asound.h > > ... > > 941 struct snd_ctl_card_info { > > ... > > 950 unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */ > > 951 }; > > > > On the other hand, the node of sysfs is quite common in Linux because > > it's tightly coupled to kernel objects. Let you see files under > > 'Documentation/ABI/'. We can find efforts to maintain sysfs node so > > safe and stable. Due to this reason, it's better to take more care when > > adding new node. > > > > Would I request you the reason to add this node and the reason that > > current ALSA control interface doesn't satisfy your requirement? > > simplicity for user support. Anyone can peak at a sysfs file, not everyone > is familiar with the alsa control interface. > > We get lots of bug reports from people who are asking for configuration > help, and the simpler the command is the better. For my information, would I request you to disclose the part of such reports? I need supplemental information about the reason to add the alternative path to expose it, especially the reason that no developers work to improve existent tool relevant to UCM and are going to wish to add the alternative without utilizing ALSA control character device. Regards Takashi Sakamoto
>>> On the other hand, the node of sysfs is quite common in Linux because >>> it's tightly coupled to kernel objects. Let you see files under >>> 'Documentation/ABI/'. We can find efforts to maintain sysfs node so >>> safe and stable. Due to this reason, it's better to take more care when >>> adding new node. >>> >>> Would I request you the reason to add this node and the reason that >>> current ALSA control interface doesn't satisfy your requirement? >> >> simplicity for user support. Anyone can peak at a sysfs file, not everyone >> is familiar with the alsa control interface. >> >> We get lots of bug reports from people who are asking for configuration >> help, and the simpler the command is the better. > > For my information, would I request you to disclose the part of such reports? > > I need supplemental information about the reason to add the alternative > path to expose it, especially the reason that no developers work to > improve existent tool relevant to UCM and are going to wish to add the > alternative without utilizing ALSA control character device. I don't understand your question, sorry. UCM already uses the control interface, it's not a matter of adding a new interface but making it easier to visualize the contents reported by the machine driver. See for example https://github.com/alsa-project/alsa-ucm-conf/blob/4722f5b3859903521ba0f92a64d86af31083ca50/ucm2/sof-hda-dsp/HiFi.conf#L145 when people report that their microphone is not reported by PulseAudio/UCM, it's very helpful to know what UCM was supposed to use in the first place. We don't have a debugger or step-by-step mechanisms to figure out what the configurations are. There is zero intent to advertise this sysfs node as a basis for applications to bypass the control interface, if that was what you thought I was promoting.
On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote: > when people report that their microphone is not reported by PulseAudio/UCM, > it's very helpful to know what UCM was supposed to use in the first place. > We don't have a debugger or step-by-step mechanisms to figure out what the > configurations are. If I get your intension correctly, the addition of sysfs node is just to investigate which use-case configuration is applied in cases that people get issues. If so, it's really exaggerative in a point of the concept of sysfs. I have two alternatives. If it's possible to focus on ALSA SoC part only, addition of node to debugfs is reasonable for this purpose. Another alternative is to change output of 'cards' node of procfs. The latter is commonly available for all cases. For example: diff --git a/sound/core/init.c b/sound/core/init.c index b02a99766351..9a04974c1d19 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -805,6 +805,8 @@ static void snd_card_info_read(struct snd_info_entry *entry, card->shortname); snd_iprintf(buffer, " %s\n", card->longname); + snd_iprintf(buffer, " %s\n", + card->component); } mutex_unlock(&snd_card_mutex); } (If you're investigating to use rules of udevd for automated application of UCM-based operation, there's a space to investigate the merit to expose information via sysfs node. But actually you're not...) Regards Takashi Sakamoto
On Mon, Mar 23, 2020 at 10:34:23PM -0500, Pierre-Louis Bossart wrote: > In case you missed it, the components are used by machine drivers to report > e.g. number of speakers, mics, etc, so that UCM can find the right > configuration. For a given family of products, the syntax will be fixed, > e.g. hs stands for headset codec, etc. If that's what you're looking for it sounds like a richer sysfs ABI which has these things in it more directly would be a more idiomatic fit (or like Sakamoto-san says adding controls, though that is a barrier to things like udev and so on like you say).
On 3/24/20 4:01 AM, Takashi Sakamoto wrote: > On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote: >> when people report that their microphone is not reported by PulseAudio/UCM, >> it's very helpful to know what UCM was supposed to use in the first place. >> We don't have a debugger or step-by-step mechanisms to figure out what the >> configurations are. > > If I get your intension correctly, the addition of sysfs node is just to > investigate which use-case configuration is applied in cases that people > get issues. If so, it's really exaggerative in a point of the concept of > sysfs. > > I have two alternatives. If it's possible to focus on ALSA SoC part only, > addition of node to debugfs is reasonable for this purpose. > > Another alternative is to change output of 'cards' node of procfs. The > latter is commonly available for all cases. For example: I initially wanted to use /proc but thought it was a thing from the past so I looked at sysfs. If this is the recommendation I don't mind using it. debugsfs is not something the average user is familiar with, and it's not available in all cases. I'd like to extend existing pieces of information than add new things.
Dne 24. 03. 20 v 14:15 Pierre-Louis Bossart napsal(a): > > > On 3/24/20 4:01 AM, Takashi Sakamoto wrote: >> On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote: >>> when people report that their microphone is not reported by PulseAudio/UCM, >>> it's very helpful to know what UCM was supposed to use in the first place. >>> We don't have a debugger or step-by-step mechanisms to figure out what the >>> configurations are. >> >> If I get your intension correctly, the addition of sysfs node is just to >> investigate which use-case configuration is applied in cases that people >> get issues. If so, it's really exaggerative in a point of the concept of >> sysfs. >> >> I have two alternatives. If it's possible to focus on ALSA SoC part only, >> addition of node to debugfs is reasonable for this purpose. >> >> Another alternative is to change output of 'cards' node of procfs. The >> latter is commonly available for all cases. For example: > > I initially wanted to use /proc but thought it was a thing from the past > so I looked at sysfs. If this is the recommendation I don't mind using it. > > debugsfs is not something the average user is familiar with, and it's > not available in all cases. I'd like to extend existing pieces of > information than add new things. I won't modify /proc/asound/cards in this case, but create a per card file like /proc/asound/card#/components . Thanks, Jaroslav
On Tue, 24 Mar 2020 14:15:44 +0100, Pierre-Louis Bossart wrote: > > > > On 3/24/20 4:01 AM, Takashi Sakamoto wrote: > > On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote: > >> when people report that their microphone is not reported by PulseAudio/UCM, > >> it's very helpful to know what UCM was supposed to use in the first place. > >> We don't have a debugger or step-by-step mechanisms to figure out what the > >> configurations are. > > > > If I get your intension correctly, the addition of sysfs node is just to > > investigate which use-case configuration is applied in cases that people > > get issues. If so, it's really exaggerative in a point of the concept of > > sysfs. > > > > I have two alternatives. If it's possible to focus on ALSA SoC part only, > > addition of node to debugfs is reasonable for this purpose. > > > > Another alternative is to change output of 'cards' node of procfs. The > > latter is commonly available for all cases. For example: > > I initially wanted to use /proc but thought it was a thing from the > past so I looked at sysfs. If this is the recommendation I don't mind > using it. procfs will practically never die, and it's already there, so I'm fine with that path, too, supposing that the primary purpose is for help debugging / analyzing. If it's used by UCM or whatever configuration tool, sysfs is the better choice, OTOH. > debugsfs is not something the average user is familiar with, and it's > not available in all cases. I'd like to extend existing pieces of > information than add new things. Right, debugfs isn't available per card as default, so it's no good option. Takashi
>>>> when people report that their microphone is not reported by PulseAudio/UCM, >>>> it's very helpful to know what UCM was supposed to use in the first place. >>>> We don't have a debugger or step-by-step mechanisms to figure out what the >>>> configurations are. >>> >>> If I get your intension correctly, the addition of sysfs node is just to >>> investigate which use-case configuration is applied in cases that people >>> get issues. If so, it's really exaggerative in a point of the concept of >>> sysfs. >>> >>> I have two alternatives. If it's possible to focus on ALSA SoC part only, >>> addition of node to debugfs is reasonable for this purpose. >>> >>> Another alternative is to change output of 'cards' node of procfs. The >>> latter is commonly available for all cases. For example: >> >> I initially wanted to use /proc but thought it was a thing from the >> past so I looked at sysfs. If this is the recommendation I don't mind >> using it. > > procfs will practically never die, and it's already there, so I'm fine > with that path, too, supposing that the primary purpose is for help > debugging / analyzing. If it's used by UCM or whatever configuration > tool, sysfs is the better choice, OTOH. > >> debugsfs is not something the average user is familiar with, and it's >> not available in all cases. I'd like to extend existing pieces of >> information than add new things. > > Right, debugfs isn't available per card as default, so it's no good > option. ok, let's go with procfs then, thanks for the feedback. I'll work on an update and resubmit. -Pierre
diff --git a/sound/core/init.c b/sound/core/init.c index b02a99766351..decaf944a8ad 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev, static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL); +static ssize_t +components_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + + return scnprintf(buf, PAGE_SIZE, "%s\n", card->components); +} + +static DEVICE_ATTR_RO(components); + static struct attribute *card_dev_attrs[] = { &dev_attr_id.attr, &dev_attr_number.attr, + &dev_attr_components.attr, NULL };
Add attribute and show the components string. This is useful to see what is provided to userspace and e.g. used by UCM to understand the card configuration: root@plb:~# more /sys/class/sound/card0/components HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715 Note that the style uses what's recommended by checkpatch.pl and is slightly different from other sysfs attributes. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/core/init.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)