From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JpCJD-0006Sa-JB for qemu-devel@nongnu.org; Thu, 24 Apr 2008 21:01:03 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JpCJB-0006SL-By for qemu-devel@nongnu.org; Thu, 24 Apr 2008 21:01:02 -0400 Received: from [199.232.76.173] (port=44932 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JpCJB-0006SI-7b for qemu-devel@nongnu.org; Thu, 24 Apr 2008 21:01:01 -0400 Received: from fmmailgate02.web.de ([217.72.192.227]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JpCJA-0008Om-HJ for qemu-devel@nongnu.org; Thu, 24 Apr 2008 21:01:00 -0400 Received: from smtp08.web.de (fmsmtp08.dlan.cinetic.de [172.20.5.216]) by fmmailgate02.web.de (Postfix) with ESMTP id 9488BDA208E0 for ; Fri, 25 Apr 2008 03:00:59 +0200 (CEST) Received: from [88.65.242.3] (helo=[192.168.1.198]) by smtp08.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1JpCJ9-0000Xv-00 for qemu-devel@nongnu.org; Fri, 25 Apr 2008 03:00:59 +0200 Message-ID: <48112D47.6050500@web.de> Date: Fri, 25 Apr 2008 03:00:55 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] [4249] Improve audio api use in WM8750. References: <4810FBF9.1040308@web.de> <48111E61.9050002@web.de> <4811213C.7020507@web.de> <48112992.2000602@web.de> In-Reply-To: <48112992.2000602@web.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig80D0D4F681AD9E0162083FDC" Sender: jan.kiszka@web.de Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig80D0D4F681AD9E0162083FDC Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Jan Kiszka wrote: > andrzej zaborowski wrote: >> On 25/04/2008, Jan Kiszka wrote: >>> Jan Kiszka wrote: >>> > Nothing is crawling here, just use a reasonable threshold for flus= hing >>> > _after_ the callback. Need to check, but maybe we can even wait th= e a >>> > full buffer. >>> >>> >>> Of course not the full buffer, but its half is fine as it generally >>> leaves enough time to the guest to refill the other half: >>> >>> Index: hw/wm8750.c >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- hw/wm8750.c (Revision 4250) >>> +++ hw/wm8750.c (Arbeitskopie) >>> @@ -73,14 +73,10 @@ static void wm8750_audio_out_cb(void *op >>> >>> { >>> struct wm8750_s *s =3D (struct wm8750_s *) opaque; >>> >>> >>> - if (s->idx_out >=3D free_b) { >>> - s->idx_out =3D free_b; >>> - s->req_out =3D 0; >>> >>> + s->req_out =3D free_b; >>> + s->data_req(s->opaque, free_b >> 2, s->req_in >> 2); >>> >>> + if (s->idx_out >=3D sizeof(s->data_out)/2) >> The checking of whether the guest filled enough data happens in >> wm8750_dac_dat(), I don't see why do it second time here. The only >> place we need an additional check is before s->dat_req call, which you= >> remove. >=20 > True, that's redundant, let's fix it this way (this even obsoletes the > flush in out_cb): >=20 > Index: hw/wm8750.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- hw/wm8750.c (Revision 4250) > +++ hw/wm8750.c (Arbeitskopie) > @@ -73,14 +73,8 @@ static void wm8750_audio_out_cb(void *op > { > struct wm8750_s *s =3D (struct wm8750_s *) opaque; > =20 > - if (s->idx_out >=3D free_b) { > - s->idx_out =3D free_b; > - s->req_out =3D 0; > - wm8750_out_flush(s); > - } else > - s->req_out =3D free_b - s->idx_out; > -=20 > - s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2); > + s->req_out =3D free_b; > + s->data_req(s->opaque, free_b >> 2, s->req_in >> 2); > } > =20 > struct wm_rate_s { > @@ -623,7 +617,7 @@ void wm8750_dac_dat(void *opaque, uint32 > *data =3D sample & s->outmask; > s->req_out -=3D 4; > s->idx_out +=3D 4; > - if (s->idx_out >=3D sizeof(s->data_out) || s->req_out <=3D 0) > + if (s->idx_out >=3D sizeof(s->data_out)/2 || s->req_out <=3D 0) OK, it's late... The real issue here is that the wm8750's internal cache correlates with the MusicPal guest's internal buffering threshold - it happens to be 4K as well. Thus, the line above just pushes the problem to guests that play with 2K buffers. And this demonstrates nicely that the current caching is fragile, only suitable for a subset of guests. Back to square #1, only cache what piles up during controllable periods: inside the callback. In my case, I _depend_ on flushing after the callback, because this is where data gets transfered to the Wolfson, and it gets transferred in larger chunks as well. Thus, flushing later easily causes buffer underruns. And for those scenarios where data arrives asynchronously in smaller chunks between the callbacks, we may also flush before entering the subordinated callback. But, frankly, how may cycle does all this caching actually save us? Did you measure it? I doubt it is relevant. Jan --------------enig80D0D4F681AD9E0162083FDC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4-svn0 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iD8DBQFIES1KniDOoMHTA+kRAhhVAJ91iCS9Ud8h2av5Q3ZrAHHUlp56zgCeOTTy k09/fhqFl4u8x6p+zQ3WVic= =sJDD -----END PGP SIGNATURE----- --------------enig80D0D4F681AD9E0162083FDC--