* [RFC][PATCH v2 0/1] design a way to change audio Jack state by software
@ 2020-12-16 11:46 Hui Wang
2020-12-16 11:46 ` [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs Hui Wang
0 siblings, 1 reply; 6+ messages in thread
From: Hui Wang @ 2020-12-16 11:46 UTC (permalink / raw)
To: alsa-devel, tiwai, perex, kai.vehmanen
the changes in the V2:
- using debugfs instead of sysfs
- using jack_ctrl to create a folder instead of snd_jack, since ASoC drivers
could create multi jack_ctrls within a snd_jack
- create a folder for each jack_ctrl instead for all jack_ctrls
This is the layout of folder on a machine with 2 sound cards:
root@hwang4-ThinkPad-P1-Gen-3:/sys/kernel/debug# tree /sys/kernel/debug/sound-core/
/sys/kernel/debug/sound-core/
├── card0
│ ├── HDMI!DP,pcm=10 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=11 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=3 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=7 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=8 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ └── HDMI!DP,pcm=9 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
└── card1
├── HDMI!DP,pcm=3 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── HDMI!DP,pcm=4 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── HDMI!DP,pcm=5 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── Headphone Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── Headset Jack
│ ├── jackin_inject
│ └── sw_inject_enable
└── Mic Jack
├── jackin_inject
└── sw_inject_enable
The sw_inject_enable has rw mode, the jackin_inject has write_only mode.
If users want to inject a plugin to Headphone Jack, they could run:
echo 1 > 'Headphone Jack'/sw_inject_enable /* now, this jack state can't be changed by hw events */
cat 'Headphone Jack'/sw_inject_enable
ack: Headphone Jack Inject Enabled: 1
echo 1 > 'Headphone Jack'/jackin_inject /* this will inject a plugin to Headphone Jack */
After testing, run
echo 0 > 'Headphone Jack'/sw_inject_enable /* the hw events will change the jack state */
Hui Wang (1):
alsa: jack: implement software jack injection via debugfs
include/sound/core.h | 2 +
sound/core/init.c | 7 ++
sound/core/jack.c | 202 ++++++++++++++++++++++++++++++++++++-------
sound/core/sound.c | 8 ++
4 files changed, 188 insertions(+), 31 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs
2020-12-16 11:46 [RFC][PATCH v2 0/1] design a way to change audio Jack state by software Hui Wang
@ 2020-12-16 11:46 ` Hui Wang
2020-12-17 16:45 ` Kai Vehmanen
0 siblings, 1 reply; 6+ messages in thread
From: Hui Wang @ 2020-12-16 11:46 UTC (permalink / raw)
To: alsa-devel, tiwai, perex, kai.vehmanen
We want to perform remote audio auto test, need the audio jack to
change from plugout to plugin or vice versa by software ways.
Here the design is creating a sound-core root folder in the debugfs
dir, and each sound card will create a folder cardN under sound-core,
then the sound jack will create folders by jack_ctrl->ctrl->id.name,
and will create 2 file nodes jackin_inject and sw_inject_enable in
the folder, this is the layout of folder on a machine with 2 sound
cards:
$tree $debugfs_mount_dir/sound-core
sound-core/
├── card0
│ ├── HDMI!DP,pcm=10 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=11 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=3 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=7 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ ├── HDMI!DP,pcm=8 Jack
│ │ ├── jackin_inject
│ │ └── sw_inject_enable
│ └── HDMI!DP,pcm=9 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
└── card1
├── HDMI!DP,pcm=3 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── HDMI!DP,pcm=4 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── HDMI!DP,pcm=5 Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── Headphone Jack
│ ├── jackin_inject
│ └── sw_inject_enable
├── Headset Jack
│ ├── jackin_inject
│ └── sw_inject_enable
└── Mic Jack
├── jackin_inject
└── sw_inject_enable
Suppose users want to enable jack injection for Headphone, they need
to run $sudo sh -c 'echo 1 > 'Headphone Jack'/sw_inject_enable', then
users could change the Headphone Jack state through jackin_inject and
this Jack's state will not be changed by non-injection ways anymore
until users echo 0 to sw_inject_enable.
Users could run $sudo sh -c 'echo 1 > 'Headphone Jack'/jackin_inject'
to trigger the Headphone jack to plugin or echo 0 to trigger it to
plugout.
If users finish their test, they could run
$sudo sh -c 'echo 0 > 'Headphone Jack'/sw_inject_enable' to disable
injection and let non-injection ways control this Jack.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
include/sound/core.h | 2 +
sound/core/init.c | 7 ++
sound/core/jack.c | 202 ++++++++++++++++++++++++++++++++++++-------
sound/core/sound.c | 8 ++
4 files changed, 188 insertions(+), 31 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h
index 0462c577d7a3..11d61e4248ee 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -122,6 +122,7 @@ struct snd_card {
size_t total_pcm_alloc_bytes; /* total amount of allocated buffers */
struct mutex memory_mutex; /* protection for the above */
+ struct dentry *debugfs_root; /* debugfs root for card */
#ifdef CONFIG_PM
unsigned int power_state; /* power state */
@@ -180,6 +181,7 @@ static inline struct device *snd_card_get_device_link(struct snd_card *card)
extern int snd_major;
extern int snd_ecards_limit;
extern struct class *sound_class;
+extern struct dentry *sound_core_debugfs_root;
void snd_request_card(int card);
diff --git a/sound/core/init.c b/sound/core/init.c
index 764dbe673d48..a9ceaf788019 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -13,6 +13,7 @@
#include <linux/time.h>
#include <linux/ctype.h>
#include <linux/pm.h>
+#include <linux/debugfs.h>
#include <linux/completion.h>
#include <sound/core.h>
@@ -163,6 +164,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
{
struct snd_card *card;
int err;
+ char name[8];
if (snd_BUG_ON(!card_ret))
return -EINVAL;
@@ -246,6 +248,10 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
dev_err(parent, "unable to create card info\n");
goto __error_ctl;
}
+
+ sprintf(name, "card%d", idx);
+ card->debugfs_root = debugfs_create_dir(name, sound_core_debugfs_root);
+
*card_ret = card;
return 0;
@@ -418,6 +424,7 @@ int snd_card_disconnect(struct snd_card *card)
/* notify all devices that we are disconnected */
snd_device_disconnect_all(card);
+ debugfs_remove(card->debugfs_root);
snd_info_card_disconnect(card);
if (card->registered) {
device_del(&card->card_dev);
diff --git a/sound/core/jack.c b/sound/core/jack.c
index 503c8af79d55..fa6b3cdc0431 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -8,6 +8,8 @@
#include <linux/input.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
#include <sound/jack.h>
#include <sound/core.h>
#include <sound/control.h>
@@ -16,6 +18,9 @@ struct snd_jack_kctl {
struct snd_kcontrol *kctl;
struct list_head list; /* list of controls belong to the same jack */
unsigned int mask_bits; /* only masked status bits are reported via kctl */
+ struct snd_jack *jack; /* pointer to struct snd_jack */
+ bool sw_inject_enable; /* allow to inject plug event via debugfs */
+ struct dentry *jack_debugfs_root; /* jack_kctl debugfs root */
};
#ifdef CONFIG_SND_JACK_INPUT_DEV
@@ -109,12 +114,174 @@ static int snd_jack_dev_register(struct snd_device *device)
}
#endif /* CONFIG_SND_JACK_INPUT_DEV */
+static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
+{
+ struct snd_jack_kctl *jack_kctl;
+ unsigned int mask_bits = 0;
+#ifdef CONFIG_SND_JACK_INPUT_DEV
+ int i;
+#endif
+ list_for_each_entry(jack_kctl, &jack->kctl_list, list) {
+ if (jack_kctl->sw_inject_enable == from_inject)
+ snd_kctl_jack_report(jack->card, jack_kctl->kctl,
+ status & jack_kctl->mask_bits);
+ else if (jack_kctl->sw_inject_enable)
+ mask_bits |= jack_kctl->mask_bits;
+ }
+
+#ifdef CONFIG_SND_JACK_INPUT_DEV
+ if (!jack->input_dev)
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
+ int testbit = SND_JACK_BTN_0 >> i;
+
+ if (!from_inject)
+ testbit &= ~mask_bits;
+ if (jack->type & testbit)
+ input_report_key(jack->input_dev, jack->key[i],
+ status & testbit);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
+ int testbit = 1 << i;
+
+ if (!from_inject)
+ testbit &= ~mask_bits;
+ if (jack->type & testbit)
+ input_report_switch(jack->input_dev,
+ jack_switch_types[i],
+ status & testbit);
+ }
+
+ input_sync(jack->input_dev);
+#endif /* CONFIG_SND_JACK_INPUT_DEV */
+}
+
+#ifdef CONFIG_DEBUG_FS
+static ssize_t sw_inject_enable_read(struct file *file,
+ char __user *to, size_t count, loff_t *ppos)
+{
+ struct snd_jack_kctl *jack_kctl = file->private_data;
+ char *buf;
+ int len, ret;
+
+ buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ len = scnprintf(buf, PAGE_SIZE, "%s: %s\t\t%s: %i\n", "Jack", jack_kctl->kctl->id.name,
+ "Inject Enabled", jack_kctl->sw_inject_enable);
+ ret = simple_read_from_buffer(to, count, ppos, buf, len);
+
+ kfree(buf);
+ return ret;
+}
+
+static ssize_t sw_inject_enable_write(struct file *file,
+ const char __user *from, size_t count, loff_t *ppos)
+{
+ struct snd_jack_kctl *jack_kctl = file->private_data;
+ char *buf;
+ int ret, err;
+ unsigned long enable;
+
+ buf = kzalloc(count, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = simple_write_to_buffer(buf, count, ppos, from, count);
+ err = kstrtoul(buf, 0, &enable);
+ if (err) {
+ ret = err;
+ goto exit;
+ }
+
+ jack_kctl->sw_inject_enable = !!enable;
+
+ exit:
+ kfree(buf);
+ return ret;
+}
+
+static ssize_t jackin_inject_write(struct file *file,
+ const char __user *from, size_t count, loff_t *ppos)
+{
+ struct snd_jack_kctl *jack_kctl = file->private_data;
+ char *buf;
+ int ret, err;
+ unsigned long enable;
+
+ if (!jack_kctl->sw_inject_enable)
+ return -EINVAL;
+
+ buf = kzalloc(count, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = simple_write_to_buffer(buf, count, ppos, from, count);
+ err = kstrtoul(buf, 0, &enable);
+ if (err) {
+ ret = err;
+ goto exit;
+ }
+
+ _snd_jack_report(jack_kctl->jack, !!enable ? jack_kctl->mask_bits : 0, true);
+
+ exit:
+ kfree(buf);
+ return ret;
+}
+
+static const struct file_operations sw_inject_enable_fops = {
+ .open = simple_open,
+ .read = sw_inject_enable_read,
+ .write = sw_inject_enable_write,
+ .llseek = default_llseek,
+};
+
+static const struct file_operations jackin_inject_fops = {
+ .open = simple_open,
+ .write = jackin_inject_write,
+ .llseek = default_llseek,
+};
+
+static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
+ struct snd_jack_kctl *jack_kctl)
+{
+ char *tname;
+
+ /* the folder's name can't contains '/', need to replace it with '!' as lib/kobject.c does */
+ tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);
+ if (!tname)
+ return -ENOMEM;
+ strreplace(tname, '/', '!');
+ jack_kctl->jack_debugfs_root = debugfs_create_dir(tname, jack->card->debugfs_root);
+ kfree(tname);
+
+ debugfs_create_file("sw_inject_enable", 0644, jack_kctl->jack_debugfs_root, jack_kctl,
+ &sw_inject_enable_fops);
+
+ debugfs_create_file("jackin_inject", 0200, jack_kctl->jack_debugfs_root, jack_kctl,
+ &jackin_inject_fops);
+
+ return 0;
+}
+#else
+static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
+ struct snd_jack_kctl *jack_kctl)
+{
+ return 0;
+}
+#endif
+
static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
{
struct snd_jack_kctl *jack_kctl;
jack_kctl = kctl->private_data;
if (jack_kctl) {
+ debugfs_remove(jack_kctl->jack_debugfs_root);
list_del(&jack_kctl->list);
kfree(jack_kctl);
}
@@ -122,7 +289,10 @@ static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
static void snd_jack_kctl_add(struct snd_jack *jack, struct snd_jack_kctl *jack_kctl)
{
+ jack_kctl->jack = jack;
list_add_tail(&jack_kctl->list, &jack->kctl_list);
+ if (!strstr(jack_kctl->kctl->id.name, "Phantom"))
+ snd_jack_debugfs_add_inject_node(jack, jack_kctl);
}
static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *name, unsigned int mask)
@@ -339,39 +509,9 @@ EXPORT_SYMBOL(snd_jack_set_key);
*/
void snd_jack_report(struct snd_jack *jack, int status)
{
- struct snd_jack_kctl *jack_kctl;
-#ifdef CONFIG_SND_JACK_INPUT_DEV
- int i;
-#endif
-
if (!jack)
return;
- list_for_each_entry(jack_kctl, &jack->kctl_list, list)
- snd_kctl_jack_report(jack->card, jack_kctl->kctl,
- status & jack_kctl->mask_bits);
-
-#ifdef CONFIG_SND_JACK_INPUT_DEV
- if (!jack->input_dev)
- return;
-
- for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
- int testbit = SND_JACK_BTN_0 >> i;
-
- if (jack->type & testbit)
- input_report_key(jack->input_dev, jack->key[i],
- status & testbit);
- }
-
- for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
- int testbit = 1 << i;
- if (jack->type & testbit)
- input_report_switch(jack->input_dev,
- jack_switch_types[i],
- status & testbit);
- }
-
- input_sync(jack->input_dev);
-#endif /* CONFIG_SND_JACK_INPUT_DEV */
+ return _snd_jack_report(jack, status, false);
}
EXPORT_SYMBOL(snd_jack_report);
diff --git a/sound/core/sound.c b/sound/core/sound.c
index b75f78f2c4b8..3039e317df93 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -9,6 +9,7 @@
#include <linux/time.h>
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/debugfs.h>
#include <sound/core.h>
#include <sound/minors.h>
#include <sound/info.h>
@@ -39,6 +40,9 @@ MODULE_ALIAS_CHARDEV_MAJOR(CONFIG_SND_MAJOR);
int snd_ecards_limit;
EXPORT_SYMBOL(snd_ecards_limit);
+struct dentry *sound_core_debugfs_root;
+EXPORT_SYMBOL_GPL(sound_core_debugfs_root);
+
static struct snd_minor *snd_minors[SNDRV_OS_MINORS];
static DEFINE_MUTEX(sound_mutex);
@@ -395,6 +399,9 @@ static int __init alsa_sound_init(void)
unregister_chrdev(major, "alsa");
return -ENOMEM;
}
+
+ sound_core_debugfs_root = debugfs_create_dir("sound-core", NULL);
+
#ifndef MODULE
pr_info("Advanced Linux Sound Architecture Driver Initialized.\n");
#endif
@@ -403,6 +410,7 @@ static int __init alsa_sound_init(void)
static void __exit alsa_sound_exit(void)
{
+ debugfs_remove(sound_core_debugfs_root);
snd_info_done();
unregister_chrdev(major, "alsa");
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs
2020-12-16 11:46 ` [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs Hui Wang
@ 2020-12-17 16:45 ` Kai Vehmanen
2020-12-18 15:17 ` Takashi Iwai
2020-12-19 10:17 ` Hui Wang
0 siblings, 2 replies; 6+ messages in thread
From: Kai Vehmanen @ 2020-12-17 16:45 UTC (permalink / raw)
To: Hui Wang; +Cc: tiwai, alsa-devel, kai.vehmanen
Hey,
I gave a quick test spin and features seems to work as advertized. A few
minor comments on the code. If Jaroslav you think this would be ok as an
approach, I can give a more extensive test run on this.
On Wed, 16 Dec 2020, Hui Wang wrote:
> We want to perform remote audio auto test, need the audio jack to
> change from plugout to plugin or vice versa by software ways.
>
> Here the design is creating a sound-core root folder in the debugfs
> dir, and each sound card will create a folder cardN under sound-core,
> then the sound jack will create folders by jack_ctrl->ctrl->id.name,
> and will create 2 file nodes jackin_inject and sw_inject_enable in
> the folder, this is the layout of folder on a machine with 2 sound
> cards:
> $tree $debugfs_mount_dir/sound-core
> sound-core/
> ├── card0
> │ ├── HDMI!DP,pcm=10 Jack
this combination of "!,= " characters in filenames is a bit non-unixy,
but maybe in 2020 we are ready for this.
> +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
> +{
> + struct snd_jack_kctl *jack_kctl;
> + unsigned int mask_bits = 0;
> +#ifdef CONFIG_SND_JACK_INPUT_DEV
> + int i;
> +#endif
> + list_for_each_entry(jack_kctl, &jack->kctl_list, list) {
> + if (jack_kctl->sw_inject_enable == from_inject)
> + snd_kctl_jack_report(jack->card, jack_kctl->kctl,
> + status & jack_kctl->mask_bits);
> + else if (jack_kctl->sw_inject_enable)
> + mask_bits |= jack_kctl->mask_bits;
> + }
I'm wondering if it would be worth the code duplication to have the
inject-variant of this code in a separate function. I find the above code
to set up "mask_bits" a bit hard to read and this adds a layer of
complexity to anyone just wanting to look at the regular jack report code
path.
> +static ssize_t sw_inject_enable_write(struct file *file,
> + const char __user *from, size_t count, loff_t *ppos)
> +{
> + struct snd_jack_kctl *jack_kctl = file->private_data;
> + char *buf;
> + int ret, err;
> + unsigned long enable;
> +
> + buf = kzalloc(count, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = simple_write_to_buffer(buf, count, ppos, from, count);
> + err = kstrtoul(buf, 0, &enable);
> + if (err) {
> + ret = err;
> + goto exit;
> + }
> +
> + jack_kctl->sw_inject_enable = !!enable;
Here it's a bit annoying that after you disable sw_inject, the kcontrol
values are not restored to reflrect actual hw state (until there are
new jack events from hw). User-space cannot completely handle the
save'n'restore as it cannot detect if real hw jack status changed
during the sw-inject test. OTOH, this would require caching the most
recent value and maybe not worth the effort.
> +static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
> + struct snd_jack_kctl *jack_kctl)
> +{
> + char *tname;
> +
> + /* the folder's name can't contains '/', need to replace it with '!' as lib/kobject.c does */
> + tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);
This goes over 100-column limit and triggers a checkpatch complaint.
Br, Kai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs
2020-12-17 16:45 ` Kai Vehmanen
@ 2020-12-18 15:17 ` Takashi Iwai
2020-12-19 12:07 ` Hui Wang
2020-12-19 10:17 ` Hui Wang
1 sibling, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2020-12-18 15:17 UTC (permalink / raw)
To: Kai Vehmanen; +Cc: Hui Wang, alsa-devel
On Thu, 17 Dec 2020 17:45:05 +0100,
Kai Vehmanen wrote:
>
> Hey,
>
> I gave a quick test spin and features seems to work as advertized. A few
> minor comments on the code. If Jaroslav you think this would be ok as an
> approach, I can give a more extensive test run on this.
The tree representation looks better than the previous one, IMO.
The exact contents would need more brush up, though; e.g. the content
of each jack could be shown in a debugfs node as well as the
injection. Or the type and the mask-to-be-injected can be shown
there, too.
> > +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
> > +{
> > + struct snd_jack_kctl *jack_kctl;
> > + unsigned int mask_bits = 0;
> > +#ifdef CONFIG_SND_JACK_INPUT_DEV
> > + int i;
> > +#endif
> > + list_for_each_entry(jack_kctl, &jack->kctl_list, list) {
> > + if (jack_kctl->sw_inject_enable == from_inject)
> > + snd_kctl_jack_report(jack->card, jack_kctl->kctl,
> > + status & jack_kctl->mask_bits);
> > + else if (jack_kctl->sw_inject_enable)
> > + mask_bits |= jack_kctl->mask_bits;
> > + }
>
> I'm wondering if it would be worth the code duplication to have the
> inject-variant of this code in a separate function. I find the above code
> to set up "mask_bits" a bit hard to read and this adds a layer of
> complexity to anyone just wanting to look at the regular jack report code
> path.
Yes, that's my impression, too. The logic is hard to follow.
> > +static ssize_t sw_inject_enable_write(struct file *file,
> > + const char __user *from, size_t count, loff_t *ppos)
> > +{
> > + struct snd_jack_kctl *jack_kctl = file->private_data;
> > + char *buf;
> > + int ret, err;
> > + unsigned long enable;
> > +
> > + buf = kzalloc(count, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + ret = simple_write_to_buffer(buf, count, ppos, from, count);
> > + err = kstrtoul(buf, 0, &enable);
> > + if (err) {
> > + ret = err;
> > + goto exit;
> > + }
> > +
> > + jack_kctl->sw_inject_enable = !!enable;
>
> Here it's a bit annoying that after you disable sw_inject, the kcontrol
> values are not restored to reflrect actual hw state (until there are
> new jack events from hw). User-space cannot completely handle the
> save'n'restore as it cannot detect if real hw jack status changed
> during the sw-inject test. OTOH, this would require caching the most
> recent value and maybe not worth the effort.
Right, but I guess this can be ignored.
Or, as I mentioned in the above, we may expose the current value in
each node instead, and writing a value to this node is treated as
injection. Then the rest requirement is rather masking from the
hardware update.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs
2020-12-17 16:45 ` Kai Vehmanen
2020-12-18 15:17 ` Takashi Iwai
@ 2020-12-19 10:17 ` Hui Wang
1 sibling, 0 replies; 6+ messages in thread
From: Hui Wang @ 2020-12-19 10:17 UTC (permalink / raw)
To: Kai Vehmanen; +Cc: tiwai, alsa-devel
On 12/18/20 12:45 AM, Kai Vehmanen wrote:
> Hey,
>
> I gave a quick test spin and features seems to work as advertized. A few
> minor comments on the code. If Jaroslav you think this would be ok as an
> approach, I can give a more extensive test run on this.
[snip]
> sound-core/
> ├── card0
> │ ├── HDMI!DP,pcm=10 Jack
> this combination of "!,= " characters in filenames is a bit non-unixy,
> but maybe in 2020 we are ready for this.
OK, will try to remove those characters.
>> +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
>> +{
>> + struct snd_jack_kctl *jack_kctl;
[snip]
>> + char *tname;
>> +
>> + /* the folder's name can't contains '/', need to replace it with '!' as lib/kobject.c does */
>> + tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);
> This goes over 100-column limit and triggers a checkpatch complaint.
Got it, will fix it.
Thanks.
>
> Br, Kai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs
2020-12-18 15:17 ` Takashi Iwai
@ 2020-12-19 12:07 ` Hui Wang
0 siblings, 0 replies; 6+ messages in thread
From: Hui Wang @ 2020-12-19 12:07 UTC (permalink / raw)
To: Takashi Iwai, Kai Vehmanen; +Cc: alsa-devel
On 12/18/20 11:17 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 17:45:05 +0100,
> Kai Vehmanen wrote:
>> Hey,
>>
>> I gave a quick test spin and features seems to work as advertized. A few
>> minor comments on the code. If Jaroslav you think this would be ok as an
>> approach, I can give a more extensive test run on this.
> The tree representation looks better than the previous one, IMO.
> The exact contents would need more brush up, though; e.g. the content
> of each jack could be shown in a debugfs node as well as the
> injection. Or the type and the mask-to-be-injected can be shown
> there, too.
OK, got it, will add more nodes for a jack, the nodes will bring more
info of the jack to the userspace.
>
>>> +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
>>> +{
>>> + struct snd_jack_kctl *jack_kctl;
>>> + unsigned int mask_bits = 0;
>>> +#ifdef CONFIG_SND_JACK_INPUT_DEV
>>> + int i;
>>> +#endif
>>> + list_for_each_entry(jack_kctl, &jack->kctl_list, list) {
>>> + if (jack_kctl->sw_inject_enable == from_inject)
>>> + snd_kctl_jack_report(jack->card, jack_kctl->kctl,
>>> + status & jack_kctl->mask_bits);
>>> + else if (jack_kctl->sw_inject_enable)
>>> + mask_bits |= jack_kctl->mask_bits;
>>> + }
>> I'm wondering if it would be worth the code duplication to have the
>> inject-variant of this code in a separate function. I find the above code
>> to set up "mask_bits" a bit hard to read and this adds a layer of
>> complexity to anyone just wanting to look at the regular jack report code
>> path.
> Yes, that's my impression, too. The logic is hard to follow.
I think it is really complicated, That is my design:
- If a jack_ctrl's sw_inject is enabled, the jack_report will only
report status from injection (block hw events), if it is disabled, the
jack_report will only report status from hw events (block injection).
That is why I have to add a parameter from_inject
- A snd_jack could contain multi jack_ctrls, the
snd_jack_report(status) is based on snd_jack instead of jack_ctrls, but
sw_inject_enable is based on jack_ctrls instead of snd_jack. Suppose a
snd_jack has 2 jack_ctrls A and B, A's sw_inject is enabled. Suppose
Jack of A triggers a hw events and snd_jack_report() is called, the
status should be blocked since A's sw_inject is enabled, also the
input_dev's event of this jack_ctrl should be blocked too, I added
mask_bits |= jack_kctl->mask_bits just for blocking the input-dev's report.
So far, I could not design a cleaner and simpler function to implement
the idea above.
>
>
>>> +static ssize_t sw_inject_enable_write(struct file *file,
>>> + const char __user *from, size_t count, loff_t *ppos)
>>> +{
>>> + struct snd_jack_kctl *jack_kctl = file->private_data;
>>> + char *buf;
>>> + int ret, err;
>>> + unsigned long enable;
>>> +
>>> + buf = kzalloc(count, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + ret = simple_write_to_buffer(buf, count, ppos, from, count);
>>> + err = kstrtoul(buf, 0, &enable);
>>> + if (err) {
>>> + ret = err;
>>> + goto exit;
>>> + }
>>> +
>>> + jack_kctl->sw_inject_enable = !!enable;
>> Here it's a bit annoying that after you disable sw_inject, the kcontrol
>> values are not restored to reflrect actual hw state (until there are
>> new jack events from hw). User-space cannot completely handle the
>> save'n'restore as it cannot detect if real hw jack status changed
>> during the sw-inject test. OTOH, this would require caching the most
>> recent value and maybe not worth the effort.
> Right, but I guess this can be ignored.
>
> Or, as I mentioned in the above, we may expose the current value in
> each node instead, and writing a value to this node is treated as
> injection. Then the rest requirement is rather masking from the
> hardware update.
Also, I could add a hw_status_cache in the snd_jack_kctl{}, and use it
to implement save-and-restore for the jack's state.
Thanks.
>
>
> thanks,
>
> Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-19 12:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 11:46 [RFC][PATCH v2 0/1] design a way to change audio Jack state by software Hui Wang
2020-12-16 11:46 ` [RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs Hui Wang
2020-12-17 16:45 ` Kai Vehmanen
2020-12-18 15:17 ` Takashi Iwai
2020-12-19 12:07 ` Hui Wang
2020-12-19 10:17 ` Hui Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.