[RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs

[Hui Wang]

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