Re: [RFC 2/2] ASoC: rt5670: Add LED trigger support

[Takashi Iwai <tiwai@suse.de>]

On Wed, 24 Feb 2021 11:56:01 +0100,
Jaroslav Kysela wrote:
> 
> Dne 24. 02. 21 v 11:33 Takashi Iwai napsal(a):
> > On Wed, 24 Feb 2021 10:49:41 +0100,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a):
> >>
> >>>> It seems that you misunderstood the number of issues which my code is trying
> >>>> to resolve:
> >>>>
> >>>> 1) set LED based on state from multiple cards (so you cannot trigger LED
> >>>> inside single driver / single control element); we need one arbiter; this is
> >>>> the main argument
> >>>> 2) unifies the audio LED interface
> >>>> 3) reduce the hardware driver code
> >>>
> >>> Those purposes are all fine.  But they don't need to be exposed for
> >>> user controls that can be abused.  That's the very concern.
> >>
> >> So, how to handle this feature for AMD ACP without PA / PipeWire modifications?
> >>
> >> And if we add an user space channel to the audio LED arbiter code, how it
> >> differs from my proposed control API extension?
> > 
> > As the early patch does, creating a kernel control (but not a generic
> > "Capture" switch but specific to LED) and control it in UCM profile.
> > What's the practical problem there?  That's what I might have missed
> > as the starting point of the discussion.
> 
> UCM is just a database which does not do any state management for those
> controls. I've not found a simple way to create an arbiter for UCM without
> adding more layers to this API. Yes, we have enable/disable sequences, but for
> LED, you need to create "group" of devices and do the OR state management:
> 
> Current device enable/disable scheme:
> 
>   Device1 -> enable (LED off)
>   Device2 -> enable (LED off)
>   Device2 -> disable (LED on) --- but Device1 is on, so LED should be off
> 
>   ... LED off - set LED control to off
>   ... LED on - set LED control to on
> 
> Even the current mechanism fails here, we don't look into the mute switch
> value in UCM at the moment, so the LED will reflect only device use - not the
> mute switch. So, as you see, UCM does not cover this. It's just used to
> activate and deactivate paths, but there's no state management (except for the
> device on/off).
> 
> And, it will work only for UCM not for the standard/legacy ALSA setup.
> 
> Those are reasons for which I ruled out the UCM for the LED control.

Besides the lack of some features in UCM, having the binding of the
led trigger managed in the ALSA core is good, and I have no objection.
However, again, my concern is the user controls.

> >> We have already locking
> >> mechanism for the user control element to one task, so it's possible to create
> >> safe user space implementation (depending on the standard task priviledges) on
> >> demand even with my proposal.
> >>
> >> Could you elaborate the abuse you mean? Three bits?
> > 
> > You can create up to 1028 user controls per card and each of them can
> > fire the led trigger...  That's an interesting experiment :)
> > 
> > So far, a user control is merely storing the value, let read/write via
> > the control API.  That's all, and nothing wrong can happen just by
> > that.  Now if it interacts with other subsystem...
> > 
> > A more serious concern is rather the fragility of the setup; for
> > enabling the mute LED control, you'd have to create a new user-space
> > control, the function of the control has to be ignored by some
> > application and some not, etc.  This has to be done on each machine
> 
> You're using "ignore", but as I explained before, the user space switch will
> be used in the whole chain:
> 
> capture stream ->
>   alsa-lib mute switch / silence PCM stream ->
>   PA mute switch / silence PCM stream
> 
> So PA can use this switch like the traditional hardware mute switch.

Does it mean PA would work as of now without any change?  Or does it
need patching?

> And we cannot do much in the user space for a better mute support here.

Right, and I'm not asking about the mute control here.  It's all about
the LED control.

> > when the system got updated.  And, not everyone is using alsa-lib.
> > Does tiny ALSA and other existing backend support the user control
> > element management?  Some uncertainty here.
> 
> It's not an argument. Tiny alsa library does not have all features from
> alsa-lib. Nobody is restricted to follow the similar mechanism.

Yes, but OTOH, if it were implemented in a kernel control, there is no
need to worry about the extra setup.  So the easiness of the setup is
a significant key point.


thanks,

Takashi