Re: [PATCH v4 00/17] ALSA: hda: cirrus: Add initial DSP support and firmware loading

[Takashi Iwai <tiwai@suse.de>]

On Wed, 01 Jun 2022 18:43:31 +0200,
Richard Fitzgerald wrote:
> 
> On 30/05/2022 11:45, Takashi Iwai wrote:
> > On Mon, 30 May 2022 12:34:15 +0200,
> > Charles Keepax wrote:
> >> 
> >> On Mon, May 30, 2022 at 12:14:26PM +0200, Takashi Iwai wrote:
> >>> On Mon, 30 May 2022 11:36:39 +0200,
> >>> Charles Keepax wrote:
> >>>> On Mon, May 30, 2022 at 11:18:43AM +0200, Takashi Iwai wrote:
> >>>>> On Mon, 30 May 2022 11:08:46 +0200,
> >>>>> Charles Keepax wrote:
> >>>>>> On Fri, May 27, 2022 at 06:13:38PM +0200, Takashi Iwai wrote:
> >>>>>>> On Wed, 25 May 2022 15:16:21 +0200,
> >>>>>>> Vitaly Rodionov wrote:
> >>>> Yeah that should be what is happening here. Although it looks
> >>>> like this code might be removing all the controls if the firmware
> >>>> is unloaded. I will discuss that with the guys, we normal just
> >>>> disable the controls on the wm_adsp stuff.
> >>> 
> >>> OK, that sounds good.  Basically my concern came up from the code
> >>> snippet doing asynchronous addition/removal via work.  This showed
> >>> some yellow signal, as such a pattern doesn't appear in the normal
> >>> implementation.  If this is (still) really necessary, it has to be
> >>> clarified as an exception.
> >>> 
> >> 
> >> Hm... ok we will think about that. I think that part will
> >> probably still be necessary. Because there is an ALSA control
> >> that selects the firmware, then it is necesarry to defer creating
> >> the controls to some work, since you are already holding the
> >> lock.
> > 
> > Well, if an ALSA control can trigger the firmware loading, that's
> > already fragile.  A firmware loading is a heavy task, which should
> > happen only at probing and/or resuming in general.  Do we have other
> > drivers doing the f/w loading triggered by a kctl...?
> > 
> 
> On Wolfson/Cirrus codecs the firmware isn't to "make the chip work".
> The DSP is programmable to allow for additional audio processing
> algorithms. Which algorithm you need depends on the audio use case(s)
> you are running, and can change as you change use-case. Many of the
> codecs don't have enough DSP memory to hold all possible algorithms.
> Which is why the firmware load has always been triggered from ALSA
> controls in the ASoC code. It's not something that can be loaded
> once in probe().

But it's still a question whether such an easily triggerable interface
should be used for a heavy procedure like the DSP firmware load.
e.g. calling the standard request_firmware() API directly from a
control callback doesn't sound like a good idea at all.  The similar
argument is applied to the dynamic addition/removal of other controls
followed by a kctl callback.


thanks,

Takashi