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

[Ivan Orlov <ivan.orlov0322@gmail.com>]

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?

-- 
Kind regards,
Ivan Orlov