From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web09.1524.1618682699392497999 for ; Sat, 17 Apr 2021 11:05:00 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=Zm/6PGn2; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 7E3862400FF for ; Sat, 17 Apr 2021 20:04:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1618682697; bh=HWpBAXDxjGFJKwT2o4KU2CFFBh1z1Lqslq/kRKxeu1I=; h=Subject:To:Cc:From:Date:From; b=Zm/6PGn2ufJ/9U2w6JqOu4WYvrEAkUr2wr+Phi61pBR151bDadMuV3Ti5N/kEhujn Oj4dQSODzCagCgW9l66gkGuGTPnmAID2zraTQA2dFGwoSfkPh0Y+c/GX1vpxjasfMi 2qqZ1rn2642/2N/2q6TZzC5pdgR/nOPdWWyHJbCkr3cMOpDte8isD61zQewtFtWfEr abLd4Th+lO7A9AD/x/MDDA2wJ+M514PpWiH2hC7Pks/QiI+X5Ht1nb5wHZhf7eCLKv A7USidAUmirxIqsqleGI6jf9Tb8bBjejVAOz4cST4mJqnZrGNYiyFg74z/d9plUZsO 946gF9XQpOoxw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4FN1FM5TgKz9rxR; Sat, 17 Apr 2021 20:04:55 +0200 (CEST) Subject: Re: [edk2-devel] VirtIO Sound Driver (GSoC 2021) To: devel@edk2.groups.io, afish@apple.com Cc: harlydavidsen@gmail.com, Leif Lindholm , Michael Brown , Mike Kinney , Laszlo Ersek , "Desimone, Nathaniel L" , Rafael Rodrigues Machado , Gerd Hoffmann References: <4AEC1784-99AF-47EF-B7DD-77F91EA3D7E9@apple.com> <309cc5ca-2ecd-79dd-b183-eec0572ea982@ipxe.org> <33e37977-2d27-36a0-89a6-36e513d06b2f@ipxe.org> <6F69BEA6-5B7A-42E5-B6DA-D819ECC85EE5@apple.com> <20210416113447.GG1664@vanye> <10E3436C-D743-4B2F-8E4B-7AD93B82FC92@apple.com> <7459B8C0-EDF0-4760-97E7-D3338312B3DF@apple.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <9b5f25d9-065b-257d-1d2d-7f80d14dec64@posteo.de> Date: Sat, 17 Apr 2021 18:04:55 +0000 MIME-Version: 1.0 In-Reply-To: <7459B8C0-EDF0-4760-97E7-D3338312B3DF@apple.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable On 17.04.21 19:31, Andrew Fish via groups.io wrote: > > >> On Apr 17, 2021, at 9:51 AM, Marvin H=C3=A4user > > wrote: >> >> On 16.04.21 19:45, Ethin Probst wrote: >>> Yes, three APIs (maybe like this) would work well: >>> - Start, Stop: begin playback of a stream >>> - SetVolume, GetVolume, Mute, Unmute: control volume of output and=20 >>> enable muting >>> - CreateStream, ReleaseStream, SetStreamSampleRate: Control sample >>> rate of stream (but not sample format since Signed 16-bit PCM is >>> enough) >>> Marvin, how do you suggest we make the events then? We need some way >>> of notifying the caller that the stream has concluded. We could make >>> the driver create the event and pass it back to the caller as an >>> event, but you'd still have dangling pointers (this is C, after all). >>> We could just make a IsPlaying() function and WaitForCompletion() >>> function and allow the driver to do the event handling -- would that >>> work? >> >> I do not know enough about the possible use-cases to tell. Aside from= =20 >> the two functions you already mentioned, you could also take in an=20 >> (optional) notification function. >> Which possible use-cases does determining playback end have? If it's=20 >> too much effort, just use EFI_EVENT I guess, just the less code can=20 >> mess it up, the better. >> > > In UEFI EFI_EVENT works much better. There is a gBS-WaitForEvent()=20 > function that lets a caller wait on an event. That is basically what=20 > the UEFI Shell is doing at the Shell prompt. A GUI in UEFI/C is=20 > basically an event loop. > > Fun fact: I ended up adding=C2=A0gIdleLoopEventGuid to the MdeModulePkg = so=20 > the DXE Core could signal gIdleLoopEventGuid if you are sitting in=20 > gBS-WaitForEvent() and no event is signaled. Basically in EFI nothing=20 > is going to happen until the next timer tick so the gIdleLoopEventGuid= =20 > lets you idle the CPU until the next timer tick. I was forced to do=20 > this as the 1st MacBook Air had a bad habit of thermal tripping when=20 > sitting at the UEFI Shell prompt. After all another name for a loop in= =20 > C code running on bare metal is a power virus. Mac EFI is one of the best implementations we know of, frankly. I'm=20 traumatised by Aptio 4 and alike, where (some issues are OEM-specific I=20 think) you can have timer events signalling after ExitBS, there is event= =20 clutter on IO polling to the point where everything lags no matter what=20 you do, and even in "smooth" scenarios there may be nothing worth the=20 description "granularity" (events scheduled to run every 10 ms may run=20 every 50 ms). Events are the last resort for us, if there really is no=20 other way. My first GUI implementation worked without events at all for=20 this reason, but as our workarounds got better, we did start using them=20 for keyboard and mouse polling. Timers do not apply here, but what does apply is resource management.=20 Using EFI_EVENT directly means (to the outside) the introduction of a=20 new resource to maintain, for each caller separately. On the other side,= =20 there is no resource to misuse or leak if none such is exposed. Yet, if=20 you argue with APIs like WaitForEvent, something has to signal it. In a=20 simple environment this would mean, some timer event is running and may=20 signal the event the main code waits for, where above's concern actually= =20 do apply. :) Again, the recommendation assumes the use-cases are simple=20 enough to easily avoid them. I think it would be best to sketch use-cases for audio and design the=20 solutions closely to the requirements. Why do we need to know when audio= =20 finished? What will happen when we queue audio twice? There are many=20 layers (UX, interface, implementation details) of questions to coming up= =20 with a pleasant and stable design. Best regards, Marvin > > Thanks, > > Andrew Fish. > >> If I remember correctly you mentioned the UEFI Talkbox before, if=20 >> that is more convenient for you, I'm there as mhaeuser. >> >> Best regards, >> Marvin >> >>> >>> On 4/16/21, Andrew Fish >=20 >>> wrote: >>>> >>>>> On Apr 16, 2021, at 4:34 AM, Leif Lindholm >>>> > wrote: >>>>> >>>>> Hi Ethin, >>>>> >>>>> I think we also want to have a SetMode function, even if we don't ge= t >>>>> around to implement proper support for it as part of GSoC (although = I >>>>> expect at least for virtio, that should be pretty straightforward). >>>>> >>>> Leif, >>>> >>>> I=E2=80=99m think if we have an API to load the buffer and a 2nd API = to=20 >>>> play the >>>> buffer an optional 3rd API could configure the streams. >>>> >>>>> It's quite likely that speech for UI would be stored as 8kHz (or >>>>> 20kHz) in some systems, whereas the example for playing a tune in GR= UB >>>>> would more likely be a 44.1 kHz mp3/wav/ogg/flac. >>>>> >>>>> For the GSoC project, I think it would be quite reasonable to >>>>> pre-generate pure PCM streams for testing rather than decoding >>>>> anything on the fly. >>>>> >>>>> Porting/writing decoders is really a separate task from enabling the >>>>> output. I would much rather see USB *and* HDA support able to play p= cm >>>>> streams before worrying about decoding. >>>>> >>>> I agree it might turn out it is easier to have the text to speech=20 >>>> code just >>>> encode a PCM directly. >>>> >>>> Thanks, >>>> >>>> Andrew Fish >>>> >>>>> / >>>>> =C2=A0=C2=A0=C2=A0Leif >>>>> >>>>> On Fri, Apr 16, 2021 at 00:33:06 -0500, Ethin Probst wrote: >>>>>> Thanks for that explanation (I missed Mike's message). Earlier I se= nt >>>>>> a summary of those things that we can agree on: mainly, that we hav= e >>>>>> mute, volume control, a load buffer, (maybe) an unload buffer, and = a >>>>>> start/stop stream function. Now that I fully understand the >>>>>> ramifications of this I don't mind settling for a specific format a= nd >>>>>> sample rate, and signed 16-bit PCM audio is, I think, the most wide= ly >>>>>> used one out there, besides 64-bit floating point samples, which I'= ve >>>>>> only seen used in DAWs, and that's something we don't need. >>>>>> Are you sure you want the firmware itself to handle the decoding of >>>>>> WAV audio? I can make a library class for that, but I'll definitely >>>>>> need help with the security aspect. >>>>>> >>>>>> On 4/16/21, Andrew Fish via groups.io =20 >>>>>> > = wrote: >>>>>>> >>>>>>>> On Apr 15, 2021, at 5:59 PM, Michael Brown >>>>>>> > wrote: >>>>>>>> >>>>>>>> On 16/04/2021 00:42, Ethin Probst wrote: >>>>>>>>> Forcing a particular channel mapping, sample rate and sample=20 >>>>>>>>> format >>>>>>>>> on >>>>>>>>> everyone would complicate application code. From an=20 >>>>>>>>> application point >>>>>>>>> of view, one would, with that type of protocol, need to do the >>>>>>>>> following: >>>>>>>>> 1) Load an audio file in any audio file format from any storage >>>>>>>>> mechanism. >>>>>>>>> 2) Decode the audio file format to extract the samples and audio >>>>>>>>> metadata. >>>>>>>>> 3) Resample the (now decoded) audio samples and convert (quantiz= e) >>>>>>>>> the >>>>>>>>> audio samples into signed 16-bit PCM audio. >>>>>>>>> 4) forward the samples onto the EFI audio protocol. >>>>>>>> You have made an incorrect assumption that there exists a=20 >>>>>>>> requirement >>>>>>>> to >>>>>>>> be able to play audio files in arbitrary formats. =C2=A0This requ= irement >>>>>>>> does >>>>>>>> not exist. >>>>>>>> >>>>>>>> With a protocol-mandated fixed baseline set of audio parameters >>>>>>>> (sample >>>>>>>> rate etc), what would happen in practice is that the audio=20 >>>>>>>> files would >>>>>>>> be >>>>>>>> encoded in that format at *build* time, using tools entirely=20 >>>>>>>> external >>>>>>>> to >>>>>>>> UEFI. =C2=A0The application code is then trivially simple: it jus= t does >>>>>>>> "load >>>>>>>> blob, pass blob to audio protocol". >>>>>>>> >>>>>>> >>>>>>> Ethin, >>>>>>> >>>>>>> Given the goal is an industry standard we value interoperability= =20 >>>>>>> more >>>>>>> that >>>>>>> flexibility. >>>>>>> >>>>>>> How about another use case. Lets say the Linux OS loader (Grub)=20 >>>>>>> wants >>>>>>> to >>>>>>> have an accessible UI so it decides to sore sound files on the EFI >>>>>>> System >>>>>>> Partition and use our new fancy UEFI Audio Protocol to add audio= =20 >>>>>>> to the >>>>>>> OS >>>>>>> loader GUI. So that version of Grub needs to work on 1,000 of=20 >>>>>>> different >>>>>>> PCs >>>>>>> and a wide range of UEFI Audio driver implementations. It is a muc= h >>>>>>> easier >>>>>>> world if Wave PCM 16 bit just works every place. You could add a= =20 >>>>>>> lot of >>>>>>> complexity and try to encode the audio on the fly, maybe even in= =20 >>>>>>> Linux >>>>>>> proper but that falls down if you are booting from read only=20 >>>>>>> media like >>>>>>> a >>>>>>> DVD or backup tape (yes people still do that in server land). >>>>>>> >>>>>>> The other problem with flexibility is you just made the test matri= x >>>>>>> very >>>>>>> large for every driver that needs to get implemented. For=20 >>>>>>> something as >>>>>>> complex as Intel HDA how you hook up the hardware and what=20 >>>>>>> CODECs you >>>>>>> use >>>>>>> may impact the quality of the playback for a given board. Your=20 >>>>>>> EFI is >>>>>>> likely >>>>>>> going to pick a single encoding at that will get tested all the=20 >>>>>>> time if >>>>>>> your >>>>>>> system has audio, but all 50 other things you support not so=20 >>>>>>> much. So >>>>>>> that >>>>>>> will required testing, and some one with audiophile ears (or an AI >>>>>>> program) >>>>>>> to test all the combinations. I=E2=80=99m not kidding I get BZs on= the=20 >>>>>>> quality >>>>>>> of >>>>>>> the boot bong on our systems. >>>>>>> >>>>>>> >>>>>>>>> typedef struct EFI_SIMPLE_AUDIO_PROTOCOL { >>>>>>>>> =C2=A0EFI_SIMPLE_AUDIO_PROTOCOL_RESET Reset; >>>>>>>>> =C2=A0EFI_SIMPLE_AUDIO_PROTOCOL_START Start; >>>>>>>>> =C2=A0EFI_SIMPLE_AUDIO_PROTOCOL_STOP Stop; >>>>>>>>> } EFI_SIMPLE_AUDIO_PROTOCOL; >>>>>>>> This is now starting to look like something that belongs in=20 >>>>>>>> boot-time >>>>>>>> firmware. =C2=A0:) >>>>>>>> >>>>>>> I think that got a little too simple I=E2=80=99d go back and look = at the=20 >>>>>>> example >>>>>>> I >>>>>>> posted to the thread but add an API to load the buffer, and then= =20 >>>>>>> play >>>>>>> the >>>>>>> buffer (that way we can an API in the future to twiddle knobs). Th= at >>>>>>> API >>>>>>> also implements the async EFI interface. Trust me the 1st thing=20 >>>>>>> that is >>>>>>> going to happen when we add audio is some one is going to=20 >>>>>>> complain in >>>>>>> xyz >>>>>>> state we should mute audio, or we should honer audio volume and mu= te >>>>>>> settings from setup, or from values set in the OS. Or some one=20 >>>>>>> is going >>>>>>> to >>>>>>> want the volume keys on the keyboard to work in EFI. >>>>>>> >>>>>>> Also if you need to pick apart the Wave PCM 16 byte file to feed= =20 >>>>>>> it into >>>>>>> the >>>>>>> audio hardware that probably means we should have a library that= =20 >>>>>>> does >>>>>>> that >>>>>>> work, so other Audio drivers can share that code. Also having a=20 >>>>>>> library >>>>>>> makes it easier to write a unit test. We need to be security=20 >>>>>>> conscious >>>>>>> as we >>>>>>> need to treat the Audo file as attacker controlled data. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Andrew Fish >>>>>>> >>>>>>>> Michael >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> -- >>>>>> Signed, >>>>>> Ethin D. Probst >>>>> >>>>> >>>>> >>>>> >>>> >>> >> >> >> > >=20