Re: [RFC PATCH 0/4] Add support for detection

[Takashi Iwai <tiwai@suse.de>]

On Wed, 16 Oct 2024 15:29:35 +0200,
Amadeusz Sławiński wrote:
> 
> On 10/16/2024 3:11 PM, Takashi Iwai wrote:
> > On Wed, 16 Oct 2024 15:02:24 +0200,
> > Amadeusz Sławiński wrote:
> >> 
> >> There are some scenarios when using DSP where one may want to have
> >> partially active stream and fully enable it after some event occurs.
> >> 
> >> Following patchset adds new "detect" state to ALSA state machine to
> >> allow waiting for condition to occur before fully starting a stream. In
> >> further patches the state is propagated through ASoC components to allow
> >> them to handling the state as necessary.
> >> 
> >> Main goal of this patchset is to allow handling scenarios like keyphrase
> >> detection - where DSP analyses incoming signal and wakes userspace to
> >> consume stream only when keyphrase is detected.
> >> 
> >> I'm sending this as RFC so we can discuss if this is the way to go or if
> >> there is perhaps another preferred way of adding such interface.
> >> Userspace part of implementation is available at
> >> https://github.com/amadeuszslawinski-intel/alsa-lib/tree/rfc_detect
> >> 
> >> Amadeusz Sławiński (4):
> >>    ALSA: core: Add support for running detect on capture stream
> >>    ALSA: core: Allow polling for detection
> >>    ASoC: pcm: Add support for running detect on capture stream
> >>    ASoC: Propagate DETECT trigger
> > 
> > Generally speaking, the addition of a new PCM state should be avoided.
> > It'll influence too badly on all user-space programs.  e.g. if an old
> > user-space program receives such a new state, what should it do?
> > How can it know it's a fatal error or it can be ignored / skipped?
> 
> In this case it should not get into new state unless specifically
> requested from userspace, unless I'm missing something?

Hmm, if the state is exclusive only for the requested program and
influences only on that program itself, why does it have to be a
global PCM state that essentially every program code has to deal with?

> Goal is to allow something along the lines of following in arecord or
> similar:
> 
>         ret = snd_pcm_detect(handle); // here only parts of DSP FW
> needed for detection are running
>         c = snd_pcm_poll_descriptors_count(handle);
> 
>         pfds = malloc(sizeof(struct pollfd) * c);
> 
>         snd_pcm_poll_descriptors(handle, pfds, c); // polling for
> detect event to happen
> 
>         while (!detected) {
>                 ret = poll(pfds, c, -1);
>                 snd_pcm_poll_descriptors_revents(handle, pfds, c,
> &revents);
> 
>                 if (revents == POLLERR) {
>                         error(_("poll, revents == |POLLERR"));
>                 }
>                 if (revents == POLLIN) {
>                         error(_("poll, revents == |POLLIN"));
>                         detected = 1;
>                 }
>         }

It's too complex if it's needed for each program.
If any, it'd be easier to implement an ioctl() for triggering the
detect and the sync...

>         ret = snd_pcm_start(handle); // starts whatever else is needed
> for PCM to work
> 
> > 
> > And, if it's about the synchronization of the DSP readiness, can't it
> > be rather synced in each PCM open or prepare instead?
> > 
> 
> It's too early. We need to do it after hw_params as it needs to create
> all paths needed for full stream.

It can be done in hw_params, too.  I don't mind where to put, but my
point is that the sync can be implemented internally without changing
the external API to user-space.

> During detection it just activates
> ones needed for detection and only after receiving detection event we
> want to start ones needed for draining.

So if the driver needs the sync, it can be done in hw_params or
prepare, too.  Something like stop_sync stuff we introduced.
In the case of stop_sync, hw_params waits for the pending task by the
previous stop trigger.  In this case, the sync can be performed after
hw_params (if requested) instead.


thanks,

Takashi