diff mbox series

[3/4] ALSA: timer: Introduce virtual userspace-driven timers

Message ID 20240726074750.626671-4-ivan.orlov0322@gmail.com (mailing list archive)
State Superseded
Headers show
Series Introduce userspace-driven ALSA timers | expand

Commit Message

Ivan Orlov July 26, 2024, 7:47 a.m. UTC
Implement two ioctl calls in order to support virtual userspace-driven
ALSA timers.

The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
snd_utimer_info struct as a parameter and returns a file descriptor of
a virtual timer. It also updates the `id` field of the snd_utimer_info
struct, which provides a unique identifier for the timer (basically,
the subdevice number which can be used when creating timer instances).

This patch also introduces a tiny id allocator for the userspace-driven
timers, which guarantees that we don't have more than 128 of them in the
system.

Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
the virtual timer (and calls snd_timer_interrupt for the timer under
the hood), causing all of the timer instances binded to this timer to
execute their callbacks.

The maximum amount of ticks available for the timer is 1 for the sake of
simplification of the userspace API. 'start', 'stop', 'open' and 'close'
callbacks for the userspace-driven timers are empty since we don't
really do any hardware initialization here.

Suggested-by: Axel Holzinger <aholzinger@gmx.de>
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 include/uapi/sound/asound.h |  17 +++
 sound/core/Kconfig          |  11 ++
 sound/core/timer.c          | 226 ++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)

Comments

Christophe JAILLET July 28, 2024, 6:52 a.m. UTC | #1
Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
> Implement two ioctl calls in order to support virtual userspace-driven
> ALSA timers.
> 
> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
> snd_utimer_info struct as a parameter and returns a file descriptor of
> a virtual timer. It also updates the `id` field of the snd_utimer_info
> struct, which provides a unique identifier for the timer (basically,
> the subdevice number which can be used when creating timer instances).
> 
> This patch also introduces a tiny id allocator for the userspace-driven
> timers, which guarantees that we don't have more than 128 of them in the
> system.
> 
> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
> the virtual timer (and calls snd_timer_interrupt for the timer under
> the hood), causing all of the timer instances binded to this timer to
> execute their callbacks.
> 
> The maximum amount of ticks available for the timer is 1 for the sake of
> simplification of the userspace API. 'start', 'stop', 'open' and 'close'
> callbacks for the userspace-driven timers are empty since we don't
> really do any hardware initialization here.
> 
> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---

Hi,

...

> diff --git a/sound/core/Kconfig b/sound/core/Kconfig
> index b970a1734647..3cf82641fc67 100644
> --- a/sound/core/Kconfig
> +++ b/sound/core/Kconfig
> @@ -251,6 +251,17 @@ config SND_JACK_INJECTION_DEBUG
>   	  Say Y if you are debugging via jack injection interface.
>   	  If unsure select "N".
>   
> +config SND_UTIMER
> +	bool "Enable support for userspace-controlled virtual timers"
> +	depends on SND_TIMER
> +	help
> +	  Say Y to enable the support of userspace-controlled timers. These
> +	  timers are purely virtual, and they are supposed to be triggered
> +	  from userspace. They could be quite useful when synchronizing the
> +	  sound timing with userspace applications (for instance, when sending
> +	  data through snd-aloop).
> +

Unneeded extra new line.

> +
>   config SND_VMASTER
>   	bool
>   

...

> +static void snd_utimer_free(struct snd_utimer *utimer)
> +{
> +	snd_timer_free(utimer->timer);
> +	snd_utimer_put_id(utimer);

Missing kfree(utimer->name); ?

> +	kfree(utimer);
> +}

...

> +static int snd_utimer_create(struct snd_utimer_info *utimer_info,
> +			     struct snd_utimer **r_utimer)
> +{
> +	struct snd_utimer *utimer;
> +	struct snd_timer *timer;
> +	struct snd_timer_id tid;
> +	int utimer_id;
> +	int err = 0;
> +	char *timer_name;
> +
> +	utimer = kzalloc(sizeof(*utimer), GFP_KERNEL);
> +	if (!utimer)
> +		return -ENOMEM;
> +
> +	timer_name = kzalloc(SNDRV_UTIMER_NAME_LEN, GFP_KERNEL);

kasprintf(GFP_KERNEL, "snd-utimer%d", utimer_id); ?
and SNDRV_UTIMER_NAME_LEN becomes useless too.

In snd_timer_new() it is copied in a char[64] anyway, and if utimer_id 
is small, we could even save a few bytes of memory.

CJ

> +	if (!timer_name) {
> +		kfree(utimer);
> +		return -ENOMEM;
> +	}
> +
> +	/* We hold the ioctl lock here so we won't get a race condition when allocating id */
> +	utimer_id = snd_utimer_take_id();
> +	if (utimer_id < 0) {
> +		err = utimer_id;
> +		goto err_take_id;
> +	}
> +
> +	sprintf(timer_name, "snd-utimer%d", utimer_id);
> +	utimer->name = timer_name;
> +	utimer->id = utimer_id;

...
Christophe JAILLET July 28, 2024, 6:59 a.m. UTC | #2
Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
> Implement two ioctl calls in order to support virtual userspace-driven
> ALSA timers.
> 
> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
> snd_utimer_info struct as a parameter and returns a file descriptor of
> a virtual timer. It also updates the `id` field of the snd_utimer_info
> struct, which provides a unique identifier for the timer (basically,
> the subdevice number which can be used when creating timer instances).
> 
> This patch also introduces a tiny id allocator for the userspace-driven
> timers, which guarantees that we don't have more than 128 of them in the
> system.
> 
> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
> the virtual timer (and calls snd_timer_interrupt for the timer under
> the hood), causing all of the timer instances binded to this timer to
> execute their callbacks.
> 
> The maximum amount of ticks available for the timer is 1 for the sake of
> simplification of the userspace API. 'start', 'stop', 'open' and 'close'
> callbacks for the userspace-driven timers are empty since we don't
> really do any hardware initialization here.
> 
> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---

...

> +#ifdef CONFIG_SND_UTIMER
> +/*
> + * Since userspace-driven timers are passed to userspace, we need to have an identifier
> + * which will allow us to use them (basically, the subdevice number of udriven timer).
> + *
> + * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to (SNDRV_UTIMERS_MAX_COUNT - 1).
> + * When we take one of them, the corresponding entry in snd_utimer_ids becomes true.
> + */
> +static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
> +
> +static void snd_utimer_put_id(struct snd_utimer *utimer)
> +{
> +	int timer_id = utimer->id;
> +
> +	snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
> +	snd_utimer_ids[timer_id] = false;
> +}
> +
> +static int snd_utimer_take_id(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
> +		if (!snd_utimer_ids[i]) {
> +			snd_utimer_ids[i] = true;
> +			return i;
> +		}
> +	}
> +
> +	return -EBUSY;
> +}

Also the bitmap API could be useful here.

CJ
Ivan Orlov July 28, 2024, 8:49 a.m. UTC | #3
On 7/28/24 07:52, Christophe JAILLET wrote:
> Hi,
> 

Hi Christophe,

> ...
> 
>> diff --git a/sound/core/Kconfig b/sound/core/Kconfig
>> index b970a1734647..3cf82641fc67 100644
>> --- a/sound/core/Kconfig
>> +++ b/sound/core/Kconfig
>> @@ -251,6 +251,17 @@ config SND_JACK_INJECTION_DEBUG
>>         Say Y if you are debugging via jack injection interface.
>>         If unsure select "N".
>> +config SND_UTIMER
>> +    bool "Enable support for userspace-controlled virtual timers"
>> +    depends on SND_TIMER
>> +    help
>> +      Say Y to enable the support of userspace-controlled timers. These
>> +      timers are purely virtual, and they are supposed to be triggered
>> +      from userspace. They could be quite useful when synchronizing the
>> +      sound timing with userspace applications (for instance, when 
>> sending
>> +      data through snd-aloop).
>> +
> 
> Unneeded extra new line.
> 
>> +
>>   config SND_VMASTER
>>       bool
> 
> ...
> 
>> +static void snd_utimer_free(struct snd_utimer *utimer)
>> +{
>> +    snd_timer_free(utimer->timer);
>> +    snd_utimer_put_id(utimer);
> 
> Missing kfree(utimer->name); ?
> 

Yeah, it definitely should be here... Thank you for finding this!

>> +    kfree(utimer);
>> +}
> 
> ...
> 
>> +static int snd_utimer_create(struct snd_utimer_info *utimer_info,
>> +                 struct snd_utimer **r_utimer)
>> +{
>> +    struct snd_utimer *utimer;
>> +    struct snd_timer *timer;
>> +    struct snd_timer_id tid;
>> +    int utimer_id;
>> +    int err = 0;
>> +    char *timer_name;
>> +
>> +    utimer = kzalloc(sizeof(*utimer), GFP_KERNEL);
>> +    if (!utimer)
>> +        return -ENOMEM;
>> +
>> +    timer_name = kzalloc(SNDRV_UTIMER_NAME_LEN, GFP_KERNEL);
> 
> kasprintf(GFP_KERNEL, "snd-utimer%d", utimer_id); ?
> and SNDRV_UTIMER_NAME_LEN becomes useless too.
> 
> In snd_timer_new() it is copied in a char[64] anyway, and if utimer_id 
> is small, we could even save a few bytes of memory.
> 

Wow, cool, I haven't heard of kasprintf but now I'll use it here in V2. 
Thanks!
Ivan Orlov July 28, 2024, 8:51 a.m. UTC | #4
On 7/28/24 07:59, Christophe JAILLET wrote:
> Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
>> Implement two ioctl calls in order to support virtual userspace-driven
>> ALSA timers.
>>
>> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
>> snd_utimer_info struct as a parameter and returns a file descriptor of
>> a virtual timer. It also updates the `id` field of the snd_utimer_info
>> struct, which provides a unique identifier for the timer (basically,
>> the subdevice number which can be used when creating timer instances).
>>
>> This patch also introduces a tiny id allocator for the userspace-driven
>> timers, which guarantees that we don't have more than 128 of them in the
>> system.
>>
>> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
>> the virtual timer (and calls snd_timer_interrupt for the timer under
>> the hood), causing all of the timer instances binded to this timer to
>> execute their callbacks.
>>
>> The maximum amount of ticks available for the timer is 1 for the sake of
>> simplification of the userspace API. 'start', 'stop', 'open' and 'close'
>> callbacks for the userspace-driven timers are empty since we don't
>> really do any hardware initialization here.
>>
>> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
> 
> ...
> 
>> +#ifdef CONFIG_SND_UTIMER
>> +/*
>> + * Since userspace-driven timers are passed to userspace, we need to 
>> have an identifier
>> + * which will allow us to use them (basically, the subdevice number 
>> of udriven timer).
>> + *
>> + * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to 
>> (SNDRV_UTIMERS_MAX_COUNT - 1).
>> + * When we take one of them, the corresponding entry in 
>> snd_utimer_ids becomes true.
>> + */
>> +static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
>> +
>> +static void snd_utimer_put_id(struct snd_utimer *utimer)
>> +{
>> +    int timer_id = utimer->id;
>> +
>> +    snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
>> +    snd_utimer_ids[timer_id] = false;
>> +}
>> +
>> +static int snd_utimer_take_id(void)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
>> +        if (!snd_utimer_ids[i]) {
>> +            snd_utimer_ids[i] = true;
>> +            return i;
>> +        }
>> +    }
>> +
>> +    return -EBUSY;
>> +}
> 
> Also the bitmap API could be useful here.
> 

Awesome, will use it in V2.

Thank you!
Christophe JAILLET July 28, 2024, 9:30 a.m. UTC | #5
Le 28/07/2024 à 10:51, Ivan Orlov a écrit :
> On 7/28/24 07:59, Christophe JAILLET wrote:
>> Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
>>> Implement two ioctl calls in order to support virtual userspace-driven
>>> ALSA timers.
>>>
>>> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
>>> snd_utimer_info struct as a parameter and returns a file descriptor of
>>> a virtual timer. It also updates the `id` field of the snd_utimer_info
>>> struct, which provides a unique identifier for the timer (basically,
>>> the subdevice number which can be used when creating timer instances).
>>>
>>> This patch also introduces a tiny id allocator for the userspace-driven
>>> timers, which guarantees that we don't have more than 128 of them in the
>>> system.
>>>
>>> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
>>> the virtual timer (and calls snd_timer_interrupt for the timer under
>>> the hood), causing all of the timer instances binded to this timer to
>>> execute their callbacks.
>>>
>>> The maximum amount of ticks available for the timer is 1 for the sake of
>>> simplification of the userspace API. 'start', 'stop', 'open' and 'close'
>>> callbacks for the userspace-driven timers are empty since we don't
>>> really do any hardware initialization here.
>>>
>>> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>>> ---
>>
>> ...
>>
>>> +#ifdef CONFIG_SND_UTIMER
>>> +/*
>>> + * Since userspace-driven timers are passed to userspace, we need to 
>>> have an identifier
>>> + * which will allow us to use them (basically, the subdevice number 
>>> of udriven timer).
>>> + *
>>> + * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to 
>>> (SNDRV_UTIMERS_MAX_COUNT - 1).
>>> + * When we take one of them, the corresponding entry in 
>>> snd_utimer_ids becomes true.
>>> + */
>>> +static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
>>> +
>>> +static void snd_utimer_put_id(struct snd_utimer *utimer)
>>> +{
>>> +    int timer_id = utimer->id;
>>> +
>>> +    snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
>>> +    snd_utimer_ids[timer_id] = false;
>>> +}
>>> +
>>> +static int snd_utimer_take_id(void)
>>> +{
>>> +    size_t i;
>>> +
>>> +    for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
>>> +        if (!snd_utimer_ids[i]) {
>>> +            snd_utimer_ids[i] = true;
>>> +            return i;
>>> +        }
>>> +    }
>>> +
>>> +    return -EBUSY;
>>> +}
>>
>> Also the bitmap API could be useful here.
>>
> 
> Awesome, will use it in V2.

Hmm, maybe DEFINE_IDA(), ida_alloc_max() and ida_free() would be even 
better.

CJ

> 
> Thank you!
> 
>
Ivan Orlov July 28, 2024, 9:42 a.m. UTC | #6
On 7/28/24 10:30, Christophe JAILLET wrote:
> Le 28/07/2024 à 10:51, Ivan Orlov a écrit :
>> On 7/28/24 07:59, Christophe JAILLET wrote:
>>> Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
>>>> Implement two ioctl calls in order to support virtual userspace-driven
>>>> ALSA timers.
>>>>
>>>> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
>>>> snd_utimer_info struct as a parameter and returns a file descriptor of
>>>> a virtual timer. It also updates the `id` field of the snd_utimer_info
>>>> struct, which provides a unique identifier for the timer (basically,
>>>> the subdevice number which can be used when creating timer instances).
>>>>
>>>> This patch also introduces a tiny id allocator for the userspace-driven
>>>> timers, which guarantees that we don't have more than 128 of them in 
>>>> the
>>>> system.
>>>>
>>>> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
>>>> the virtual timer (and calls snd_timer_interrupt for the timer under
>>>> the hood), causing all of the timer instances binded to this timer to
>>>> execute their callbacks.
>>>>
>>>> The maximum amount of ticks available for the timer is 1 for the 
>>>> sake of
>>>> simplification of the userspace API. 'start', 'stop', 'open' and 
>>>> 'close'
>>>> callbacks for the userspace-driven timers are empty since we don't
>>>> really do any hardware initialization here.
>>>>
>>>> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
>>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>>>> ---
>>>
>>> ...
>>>
>>>> +#ifdef CONFIG_SND_UTIMER
>>>> +/*
>>>> + * Since userspace-driven timers are passed to userspace, we need 
>>>> to have an identifier
>>>> + * which will allow us to use them (basically, the subdevice number 
>>>> of udriven timer).
>>>> + *
>>>> + * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to 
>>>> (SNDRV_UTIMERS_MAX_COUNT - 1).
>>>> + * When we take one of them, the corresponding entry in 
>>>> snd_utimer_ids becomes true.
>>>> + */
>>>> +static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
>>>> +
>>>> +static void snd_utimer_put_id(struct snd_utimer *utimer)
>>>> +{
>>>> +    int timer_id = utimer->id;
>>>> +
>>>> +    snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
>>>> +    snd_utimer_ids[timer_id] = false;
>>>> +}
>>>> +
>>>> +static int snd_utimer_take_id(void)
>>>> +{
>>>> +    size_t i;
>>>> +
>>>> +    for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
>>>> +        if (!snd_utimer_ids[i]) {
>>>> +            snd_utimer_ids[i] = true;
>>>> +            return i;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return -EBUSY;
>>>> +}
>>>
>>> Also the bitmap API could be useful here.
>>>
>>
>> Awesome, will use it in V2.
> 
> Hmm, maybe DEFINE_IDA(), ida_alloc_max() and ida_free() would be even 
> better.
> 

It looks like IDA allocator uses XArrays under the hood to allocate ids 
between 0 and INT_MAX... Considering the fact, that we currently could 
have up to 128 userspace-driven timers in the system, using XArrays 
seems a bit redundant, and I believe bitmap approach would be more 
efficient. What do you think?
Christophe JAILLET July 28, 2024, 10:29 a.m. UTC | #7
Le 28/07/2024 à 11:42, Ivan Orlov a écrit :
> On 7/28/24 10:30, Christophe JAILLET wrote:
>> Le 28/07/2024 à 10:51, Ivan Orlov a écrit :
>>> On 7/28/24 07:59, Christophe JAILLET wrote:
>>>> Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
>>>>> Implement two ioctl calls in order to support virtual userspace-driven
>>>>> ALSA timers.
>>>>>
>>>>> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
>>>>> snd_utimer_info struct as a parameter and returns a file descriptor of
>>>>> a virtual timer. It also updates the `id` field of the snd_utimer_info
>>>>> struct, which provides a unique identifier for the timer (basically,
>>>>> the subdevice number which can be used when creating timer instances).
>>>>>
>>>>> This patch also introduces a tiny id allocator for the 
>>>>> userspace-driven
>>>>> timers, which guarantees that we don't have more than 128 of them 
>>>>> in the
>>>>> system.
>>>>>
>>>>> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
>>>>> the virtual timer (and calls snd_timer_interrupt for the timer under
>>>>> the hood), causing all of the timer instances binded to this timer to
>>>>> execute their callbacks.
>>>>>
>>>>> The maximum amount of ticks available for the timer is 1 for the 
>>>>> sake of
>>>>> simplification of the userspace API. 'start', 'stop', 'open' and 
>>>>> 'close'
>>>>> callbacks for the userspace-driven timers are empty since we don't
>>>>> really do any hardware initialization here.
>>>>>
>>>>> Suggested-by: Axel Holzinger <aholzinger@gmx.de>
>>>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>>>>> ---
>>>>
>>>> ...
>>>>
>>>>> +#ifdef CONFIG_SND_UTIMER
>>>>> +/*
>>>>> + * Since userspace-driven timers are passed to userspace, we need 
>>>>> to have an identifier
>>>>> + * which will allow us to use them (basically, the subdevice 
>>>>> number of udriven timer).
>>>>> + *
>>>>> + * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to 
>>>>> (SNDRV_UTIMERS_MAX_COUNT - 1).
>>>>> + * When we take one of them, the corresponding entry in 
>>>>> snd_utimer_ids becomes true.
>>>>> + */
>>>>> +static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
>>>>> +
>>>>> +static void snd_utimer_put_id(struct snd_utimer *utimer)
>>>>> +{
>>>>> +    int timer_id = utimer->id;
>>>>> +
>>>>> +    snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
>>>>> +    snd_utimer_ids[timer_id] = false;
>>>>> +}
>>>>> +
>>>>> +static int snd_utimer_take_id(void)
>>>>> +{
>>>>> +    size_t i;
>>>>> +
>>>>> +    for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
>>>>> +        if (!snd_utimer_ids[i]) {
>>>>> +            snd_utimer_ids[i] = true;
>>>>> +            return i;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return -EBUSY;
>>>>> +}
>>>>
>>>> Also the bitmap API could be useful here.
>>>>
>>>
>>> Awesome, will use it in V2.
>>
>> Hmm, maybe DEFINE_IDA(), ida_alloc_max() and ida_free() would be even 
>> better.
>>
> 
> It looks like IDA allocator uses XArrays under the hood to allocate ids 
> between 0 and INT_MAX... Considering the fact, that we currently could 
> have up to 128 userspace-driven timers in the system, using XArrays 
> seems a bit redundant, and I believe bitmap approach would be more 
> efficient. What do you think?
> 

I may be wrong but I think that ida allocates hunks for 1024 bits (128 
bytes * 8) at a time. (see [1])

So with this extra sape and the sapce for the xarray, it would waste a 
few bytes of memory, yes.

With ida, there is also some locking that may be unnecessary (but harmless)


Hoping, I got it right, here are a few numbers:

On a x86_64, with allmodconfig:

Your initial patch:
    text	   data	    bss	    dec	    hex	filename
   55020	   1783	    268	  57071	   deef	sound/core/timer.o

With ida:
   54763	   1631	    116	  56510	   dcbe	sound/core/timer.o
+ 128 bytes of runtime memory allocation

With bitmap:
   54805	   1535	    132	  56472	   dc98	sound/core/timer.o


I think that the code would be slightly more elegant with ida, but 
implementing it with a bitmap does not add that much complexity.

CJ


[1]: 
https://elixir.bootlin.com/linux/v6.10.2/source/include/linux/idr.h#L238
Ivan Orlov July 28, 2024, 11:54 a.m. UTC | #8
On 7/28/24 11:29, Christophe JAILLET wrote:
> 
> I may be wrong but I think that ida allocates hunks for 1024 bits (128 
> bytes * 8) at a time. (see [1])
> 
> So with this extra sape and the sapce for the xarray, it would waste a 
> few bytes of memory, yes.
> 
> With ida, there is also some locking that may be unnecessary (but harmless)
> 
> 
> Hoping, I got it right, here are a few numbers:
> 
> On a x86_64, with allmodconfig:
> 
> Your initial patch:
>     text       data        bss        dec        hex    filename
>    55020       1783        268      57071       deef    sound/core/timer.o
> 
> With ida:
>    54763       1631        116      56510       dcbe    sound/core/timer.o
> + 128 bytes of runtime memory allocation
> 
> With bitmap:
>    54805       1535        132      56472       dc98    sound/core/timer.o
> 
> 
> I think that the code would be slightly more elegant with ida, but 
> implementing it with a bitmap does not add that much complexity.
> 

Ah, alright, I agree that the code would be cleaner when using IDA, and 
such a small memory overhead won't be significant/noticeable. I'm going 
to use IDA in the V2 instead of bitmap API, thank you so much for 
pointing me to it (I was wondering if the Kernel has a generic ID 
allocator and now I finally know it does :) ).

Thank you!
diff mbox series

Patch

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 8bf7e8a0eb6f..ade952a54edd 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -894,6 +894,7 @@  enum {
 #define SNDRV_TIMER_GLOBAL_RTC		1	/* unused */
 #define SNDRV_TIMER_GLOBAL_HPET		2
 #define SNDRV_TIMER_GLOBAL_HRTIMER	3
+#define SNDRV_TIMER_GLOBAL_UDRIVEN	4
 
 /* info flags */
 #define SNDRV_TIMER_FLG_SLAVE		(1<<0)	/* cannot be controlled */
@@ -974,6 +975,20 @@  struct snd_timer_status {
 };
 #endif
 
+/*
+ * This structure describes the userspace-driven timer. Such timers are purely virtual,
+ * and can only be triggered from software (for instance, by userspace application).
+ */
+struct snd_utimer_info {
+	/*
+	 * To pretend being a normal timer, we need to know the frame rate and
+	 * the period size in frames.
+	 */
+	snd_pcm_uframes_t frame_rate;
+	snd_pcm_uframes_t period_size;
+	unsigned int id;
+};
+
 #define SNDRV_TIMER_IOCTL_PVERSION	_IOR('T', 0x00, int)
 #define SNDRV_TIMER_IOCTL_NEXT_DEVICE	_IOWR('T', 0x01, struct snd_timer_id)
 #define SNDRV_TIMER_IOCTL_TREAD_OLD	_IOW('T', 0x02, int)
@@ -990,6 +1005,8 @@  struct snd_timer_status {
 #define SNDRV_TIMER_IOCTL_CONTINUE	_IO('T', 0xa2)
 #define SNDRV_TIMER_IOCTL_PAUSE		_IO('T', 0xa3)
 #define SNDRV_TIMER_IOCTL_TREAD64	_IOW('T', 0xa4, int)
+#define SNDRV_TIMER_IOCTL_CREATE	_IOWR('T', 0xa5, struct snd_utimer_info)
+#define SNDRV_TIMER_IOCTL_TRIGGER	_IO('T', 0xa6)
 
 #if __BITS_PER_LONG == 64
 #define SNDRV_TIMER_IOCTL_TREAD SNDRV_TIMER_IOCTL_TREAD_OLD
diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index b970a1734647..3cf82641fc67 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -251,6 +251,17 @@  config SND_JACK_INJECTION_DEBUG
 	  Say Y if you are debugging via jack injection interface.
 	  If unsure select "N".
 
+config SND_UTIMER
+	bool "Enable support for userspace-controlled virtual timers"
+	depends on SND_TIMER
+	help
+	  Say Y to enable the support of userspace-controlled timers. These
+	  timers are purely virtual, and they are supposed to be triggered
+	  from userspace. They could be quite useful when synchronizing the
+	  sound timing with userspace applications (for instance, when sending
+	  data through snd-aloop).
+
+
 config SND_VMASTER
 	bool
 
diff --git a/sound/core/timer.c b/sound/core/timer.c
index d104adc75a8b..177bd06f7b60 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -13,6 +13,8 @@ 
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/sched/signal.h>
+#include <linux/anon_inodes.h>
+#include <linux/units.h>
 #include <sound/core.h>
 #include <sound/timer.h>
 #include <sound/control.h>
@@ -109,6 +111,17 @@  struct snd_timer_status64 {
 	unsigned char reserved[64];	/* reserved */
 };
 
+#ifdef CONFIG_SND_UTIMER
+#define SNDRV_UTIMERS_MAX_COUNT 128
+#define SNDRV_UTIMER_NAME_LEN 20
+/* Internal data structure for keeping the state of the userspace-driven timer */
+struct snd_utimer {
+	char *name;
+	struct snd_timer *timer;
+	unsigned int id;
+};
+#endif
+
 #define SNDRV_TIMER_IOCTL_STATUS64	_IOR('T', 0x14, struct snd_timer_status64)
 
 /* list of timers */
@@ -2009,6 +2022,217 @@  enum {
 	SNDRV_TIMER_IOCTL_PAUSE_OLD = _IO('T', 0x23),
 };
 
+#ifdef CONFIG_SND_UTIMER
+/*
+ * Since userspace-driven timers are passed to userspace, we need to have an identifier
+ * which will allow us to use them (basically, the subdevice number of udriven timer).
+ *
+ * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to (SNDRV_UTIMERS_MAX_COUNT - 1).
+ * When we take one of them, the corresponding entry in snd_utimer_ids becomes true.
+ */
+static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
+
+static void snd_utimer_put_id(struct snd_utimer *utimer)
+{
+	int timer_id = utimer->id;
+
+	snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
+	snd_utimer_ids[timer_id] = false;
+}
+
+static int snd_utimer_take_id(void)
+{
+	size_t i;
+
+	for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
+		if (!snd_utimer_ids[i]) {
+			snd_utimer_ids[i] = true;
+			return i;
+		}
+	}
+
+	return -EBUSY;
+}
+
+static void snd_utimer_free(struct snd_utimer *utimer)
+{
+	snd_timer_free(utimer->timer);
+	snd_utimer_put_id(utimer);
+	kfree(utimer);
+}
+
+static int snd_utimer_release(struct inode *inode, struct file *file)
+{
+	struct snd_utimer *utimer = (struct snd_utimer *)file->private_data;
+
+	snd_utimer_free(utimer);
+	return 0;
+}
+
+static int snd_utimer_trigger(struct file *file)
+{
+	struct snd_utimer *utimer = (struct snd_utimer *)file->private_data;
+
+	snd_timer_interrupt(utimer->timer, utimer->timer->sticks);
+	return 0;
+}
+
+static long snd_utimer_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
+{
+	switch (ioctl) {
+	case SNDRV_TIMER_IOCTL_TRIGGER:
+		return snd_utimer_trigger(file);
+	}
+
+	return -ENOTTY;
+}
+
+static const struct file_operations snd_utimer_fops = {
+	.llseek = noop_llseek,
+	.release = snd_utimer_release,
+	.unlocked_ioctl = snd_utimer_ioctl,
+};
+
+static int snd_utimer_start(struct snd_timer *t)
+{
+	return 0;
+}
+
+static int snd_utimer_stop(struct snd_timer *t)
+{
+	return 0;
+}
+
+static int snd_utimer_open(struct snd_timer *t)
+{
+	return 0;
+}
+
+static int snd_utimer_close(struct snd_timer *t)
+{
+	return 0;
+}
+
+static const struct snd_timer_hardware timer_hw = {
+	.flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_WORK,
+	.open = snd_utimer_open,
+	.close = snd_utimer_close,
+	.start = snd_utimer_start,
+	.stop = snd_utimer_stop,
+};
+
+static int snd_utimer_create(struct snd_utimer_info *utimer_info,
+			     struct snd_utimer **r_utimer)
+{
+	struct snd_utimer *utimer;
+	struct snd_timer *timer;
+	struct snd_timer_id tid;
+	int utimer_id;
+	int err = 0;
+	char *timer_name;
+
+	utimer = kzalloc(sizeof(*utimer), GFP_KERNEL);
+	if (!utimer)
+		return -ENOMEM;
+
+	timer_name = kzalloc(SNDRV_UTIMER_NAME_LEN, GFP_KERNEL);
+	if (!timer_name) {
+		kfree(utimer);
+		return -ENOMEM;
+	}
+
+	/* We hold the ioctl lock here so we won't get a race condition when allocating id */
+	utimer_id = snd_utimer_take_id();
+	if (utimer_id < 0) {
+		err = utimer_id;
+		goto err_take_id;
+	}
+
+	sprintf(timer_name, "snd-utimer%d", utimer_id);
+	utimer->name = timer_name;
+	utimer->id = utimer_id;
+
+	tid.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION;
+	tid.dev_class = SNDRV_TIMER_CLASS_GLOBAL;
+	tid.card = -1;
+	tid.device = SNDRV_TIMER_GLOBAL_UDRIVEN;
+	tid.subdevice = utimer_id;
+
+	err = snd_timer_new(NULL, timer_name, &tid, &timer);
+	if (err < 0) {
+		pr_err("Can't create userspace-driven timer\n");
+		goto err_timer_new;
+	}
+
+	timer->module = THIS_MODULE;
+	timer->hw = timer_hw;
+	timer->hw.resolution = NANO / utimer_info->frame_rate * utimer_info->period_size;
+	timer->hw.ticks = 1;
+	timer->max_instances = MAX_SLAVE_INSTANCES;
+
+	utimer->timer = timer;
+
+	err = snd_timer_global_register(timer);
+	if (err < 0) {
+		pr_err("Can't register a userspace-driven timer\n");
+		goto err_timer_reg;
+	}
+
+	*r_utimer = utimer;
+	return 0;
+
+err_timer_reg:
+	snd_timer_free(timer);
+err_timer_new:
+	snd_utimer_put_id(utimer);
+err_take_id:
+	kfree(timer_name);
+	kfree(utimer);
+
+	return err;
+}
+
+static int snd_utimer_ioctl_create(struct file *file,
+				   struct snd_utimer_info __user *_utimer_info)
+{
+	struct snd_utimer *utimer;
+	struct snd_utimer_info *utimer_info;
+	int err;
+
+	utimer_info = memdup_user(_utimer_info, sizeof(*utimer_info));
+	if (IS_ERR(utimer_info))
+		return PTR_ERR(no_free_ptr(utimer_info));
+
+	err = snd_utimer_create(utimer_info, &utimer);
+	if (err < 0) {
+		kfree(utimer_info);
+		return err;
+	}
+
+	utimer_info->id = utimer->id;
+
+	err = copy_to_user(_utimer_info, utimer_info, sizeof(*utimer_info));
+	if (err) {
+		snd_utimer_free(utimer);
+		kfree(utimer_info);
+		return -EFAULT;
+	}
+
+	kfree(utimer_info);
+
+	return anon_inode_getfd(utimer->name, &snd_utimer_fops, utimer, O_RDWR | O_CLOEXEC);
+}
+
+#else
+
+static int snd_utimer_ioctl_create(struct file *file,
+				   struct snd_utimer_info __user *_utimer_info)
+{
+	return -EINVAL;
+}
+
+#endif
+
 static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg, bool compat)
 {
@@ -2053,6 +2277,8 @@  static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 	case SNDRV_TIMER_IOCTL_PAUSE:
 	case SNDRV_TIMER_IOCTL_PAUSE_OLD:
 		return snd_timer_user_pause(file);
+	case SNDRV_TIMER_IOCTL_CREATE:
+		return snd_utimer_ioctl_create(file, argp);
 	}
 	return -ENOTTY;
 }