[Qemu-devel] [4249] Improve audio api use in WM8750.

[Qemu-devel] [4249] Improve audio api use in WM8750.

[Andrzej Zaborowski]

Revision: 4249
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4249
Author:   balrog
Date:     2008-04-24 21:01:40 +0000 (Thu, 24 Apr 2008)

Log Message:
-----------
Improve audio api use in WM8750.

These are changes from
http://svn.openmoko.org/trunk/src/host/qemu-neo1973/hw/wm8753.c that I
forgot to push to WM8750.  Some were suggested by malc.

Modified Paths:
--------------
    trunk/hw/wm8750.c

Modified: trunk/hw/wm8750.c
===================================================================
--- trunk/hw/wm8750.c	2008-04-24 19:21:53 UTC (rev 4248)
+++ trunk/hw/wm8750.c	2008-04-24 21:01:40 UTC (rev 4249)
@@ -55,10 +55,10 @@
 
 static inline void wm8750_out_flush(struct wm8750_s *s)
 {
-    int sent;
-    if (!s->idx_out)
-        return;
-    sent = AUD_write(*s->out[0], s->data_out, s->idx_out);
+    int sent = 0;
+    while (sent < s->idx_out)
+        sent += AUD_write(*s->out[0], s->data_out + sent, s->idx_out - sent)
+                ?: s->idx_out;
     s->idx_out = 0;
 }
 
@@ -67,19 +67,20 @@
     struct wm8750_s *s = (struct wm8750_s *) opaque;
     s->req_in = avail_b;
     s->data_req(s->opaque, s->req_out >> 2, avail_b >> 2);
-
-#if 0
-    wm8750_in_load(s);
-#endif
 }
 
 static void wm8750_audio_out_cb(void *opaque, int free_b)
 {
     struct wm8750_s *s = (struct wm8750_s *) opaque;
 
-    s->req_out = free_b;
-    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
-    wm8750_out_flush(s);
+    if (s->idx_out >= free_b) {
+        s->idx_out = free_b;
+        s->req_out = 0;
+        wm8750_out_flush(s);
+    } else
+        s->req_out = free_b - s->idx_out;
+ 
+    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
 }
 
 struct wm_rate_s {
@@ -121,7 +122,7 @@
     {  512, 24000,  512, 24000 },	/* SR: 11100 */
     {  768, 24000,  768, 24000 },	/* SR: 11101 */
     {  128, 88200,  128, 88200 },	/* SR: 11110 */
-    {  192, 88200,  128, 88200 },	/* SR: 11111 */
+    {  192, 88200,  192, 88200 },	/* SR: 11111 */
 };
 
 static void wm8750_set_format(struct wm8750_s *s)
@@ -597,6 +598,7 @@
     return &s->i2c;
 }
 
+#if 0
 static void wm8750_fini(i2c_slave *i2c)
 {
     struct wm8750_s *s = (struct wm8750_s *) i2c;
@@ -604,6 +606,7 @@
     AUD_remove_card(&s->card);
     qemu_free(s);
 }
+#endif
 
 void wm8750_data_req_set(i2c_slave *i2c,
                 void (*data_req)(void *, int, int), void *opaque)

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

Hi Andrzej,

Andrzej Zaborowski wrote:
> Revision: 4249
...
>  static void wm8750_audio_out_cb(void *opaque, int free_b)
>  {
>      struct wm8750_s *s = (struct wm8750_s *) opaque;
>  
> -    s->req_out = free_b;
> -    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
> -    wm8750_out_flush(s);
> +    if (s->idx_out >= free_b) {
> +        s->idx_out = free_b;
> +        s->req_out = 0;
> +        wm8750_out_flush(s);
> +    } else
> +        s->req_out = free_b - s->idx_out;
> + 
> +    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);

Please make sure that the callback is always issued _before_ the flush
(keep in mind: it may increase the amount of data that has to be flushed
ASAP!). And this change also leaves the MusicPal broken.

As I haven't fully understood what the hunk above is supposed to improve
(and a quick fix of mine failed), I cannot provide a patch yet.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[andrzej zaborowski]

On 24/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
> Hi Andrzej,
>
>  Andrzej Zaborowski wrote:
>  > Revision: 4249
>  ...
>  >  static void wm8750_audio_out_cb(void *opaque, int free_b)
>  >  {
>  >      struct wm8750_s *s = (struct wm8750_s *) opaque;
>  >
>  > -    s->req_out = free_b;
>  > -    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>  > -    wm8750_out_flush(s);
>  > +    if (s->idx_out >= free_b) {
>  > +        s->idx_out = free_b;
>  > +        s->req_out = 0;
>  > +        wm8750_out_flush(s);
>  > +    } else
>  > +        s->req_out = free_b - s->idx_out;
>  > +
>  > +    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
>
>  Please make sure that the callback is always issued _before_ the flush
>  (keep in mind: it may increase the amount of data that has to be flushed
>  ASAP!). And this change also leaves the MusicPal broken.

The idea is to output free_b bytes immediately if we have that many in
the buffer (it could happen assuming that free_b value can change
between callbacks).  I'm not sure how this can break something: if
*inside* the data_req() callback we receive enough bytes to fill the
the whole buffer then dac_dat() will call out_flush().

Without that all buffering becomes useless because we flush every
sample we receive and we start to crawl.

Regards
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

andrzej zaborowski wrote:
> On 24/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Hi Andrzej,
>>
>>  Andrzej Zaborowski wrote:
>>  > Revision: 4249
>>  ...
>>  >  static void wm8750_audio_out_cb(void *opaque, int free_b)
>>  >  {
>>  >      struct wm8750_s *s = (struct wm8750_s *) opaque;
>>  >
>>  > -    s->req_out = free_b;
>>  > -    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>>  > -    wm8750_out_flush(s);
>>  > +    if (s->idx_out >= free_b) {
>>  > +        s->idx_out = free_b;
>>  > +        s->req_out = 0;
>>  > +        wm8750_out_flush(s);
>>  > +    } else
>>  > +        s->req_out = free_b - s->idx_out;
>>  > +
>>  > +    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
>>
>>  Please make sure that the callback is always issued _before_ the flush
>>  (keep in mind: it may increase the amount of data that has to be flushed
>>  ASAP!). And this change also leaves the MusicPal broken.
> 
> The idea is to output free_b bytes immediately if we have that many in
> the buffer (it could happen assuming that free_b value can change

Wrong ordering: If there has been a _relevant_ amount of data (not a few
10 bytes) hanging around in our internal buffer between the guest has
filled it during the last callbacks and the host reports demand via the
new callback, that data was missing in the host's audio buffer! That's
what I saw (heard) with the old version (the current one even causes
total silence).

> between callbacks).  I'm not sure how this can break something: if
> *inside* the data_req() callback we receive enough bytes to fill the
> the whole buffer then dac_dat() will call out_flush().
> 
> Without that all buffering becomes useless because we flush every
> sample we receive and we start to crawl.

Nothing is crawling here, just use a reasonable threshold for flushing
_after_ the callback. Need to check, but maybe we can even wait the a
full buffer.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

Jan Kiszka wrote:
> andrzej zaborowski wrote:
>> On 24/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Hi Andrzej,
>>>
>>>  Andrzej Zaborowski wrote:
>>>  > Revision: 4249
>>>  ...
>>>  >  static void wm8750_audio_out_cb(void *opaque, int free_b)
>>>  >  {
>>>  >      struct wm8750_s *s = (struct wm8750_s *) opaque;
>>>  >
>>>  > -    s->req_out = free_b;
>>>  > -    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>>>  > -    wm8750_out_flush(s);
>>>  > +    if (s->idx_out >= free_b) {
>>>  > +        s->idx_out = free_b;
>>>  > +        s->req_out = 0;
>>>  > +        wm8750_out_flush(s);
>>>  > +    } else
>>>  > +        s->req_out = free_b - s->idx_out;
>>>  > +
>>>  > +    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
>>>
>>>  Please make sure that the callback is always issued _before_ the flush
>>>  (keep in mind: it may increase the amount of data that has to be flushed
>>>  ASAP!). And this change also leaves the MusicPal broken.
>> The idea is to output free_b bytes immediately if we have that many in
>> the buffer (it could happen assuming that free_b value can change
> 
> Wrong ordering: If there has been a _relevant_ amount of data (not a few
> 10 bytes) hanging around in our internal buffer between the guest has
> filled it during the last callbacks and the host reports demand via the
> new callback, that data was missing in the host's audio buffer! That's
> what I saw (heard) with the old version (the current one even causes
> total silence).
> 
>> between callbacks).  I'm not sure how this can break something: if
>> *inside* the data_req() callback we receive enough bytes to fill the
>> the whole buffer then dac_dat() will call out_flush().
>>
>> Without that all buffering becomes useless because we flush every
>> sample we receive and we start to crawl.
> 
> Nothing is crawling here, just use a reasonable threshold for flushing
> _after_ the callback. Need to check, but maybe we can even wait the 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
===================================================================
--- 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 = (struct wm8750_s *) opaque;

-    if (s->idx_out >= free_b) {
-        s->idx_out = free_b;
-        s->req_out = 0;
+    s->req_out = free_b;
+    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
+    if (s->idx_out >= sizeof(s->data_out)/2)
         wm8750_out_flush(s);
-    } else
-        s->req_out = free_b - s->idx_out;
-
-    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
 }

 struct wm_rate_s {


Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[andrzej zaborowski]

On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
> andrzej zaborowski wrote:
>  > On 24/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>  >> Hi Andrzej,
>  >>
>  >>  Andrzej Zaborowski wrote:
>  >>  > Revision: 4249
>  >>  ...
>  >>  >  static void wm8750_audio_out_cb(void *opaque, int free_b)
>  >>  >  {
>  >>  >      struct wm8750_s *s = (struct wm8750_s *) opaque;
>  >>  >
>  >>  > -    s->req_out = free_b;
>  >>  > -    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>  >>  > -    wm8750_out_flush(s);
>  >>  > +    if (s->idx_out >= free_b) {
>  >>  > +        s->idx_out = free_b;
>  >>  > +        s->req_out = 0;
>  >>  > +        wm8750_out_flush(s);
>  >>  > +    } else
>  >>  > +        s->req_out = free_b - s->idx_out;
>  >>  > +
>  >>  > +    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
>  >>
>  >>  Please make sure that the callback is always issued _before_ the flush
>  >>  (keep in mind: it may increase the amount of data that has to be flushed
>  >>  ASAP!). And this change also leaves the MusicPal broken.
>  >
>  > The idea is to output free_b bytes immediately if we have that many in
>  > the buffer (it could happen assuming that free_b value can change
>
>
> Wrong ordering: If there has been a _relevant_ amount of data (not a few
>  10 bytes) hanging around in our internal buffer between the guest has
>  filled it during the last callbacks and the host reports demand via the
>  new callback, that data was missing in the host's audio buffer! That's
>  what I saw (heard) with the old version (the current one even causes
>  total silence).

Not sure I understand what you mean by missing data.  The host reports
that it can accept a write of free_b bytes.  If we have free_b bytes
in s->data_out then we write it.  Then we tell the guest how many more
samples we need (if any).  When the guest provides us with that many
samples then we write the whole buffer to host (no matter if that
happened inside the callback or whenever that data became ready).

>
>
>  > between callbacks).  I'm not sure how this can break something: if
>  > *inside* the data_req() callback we receive enough bytes to fill the
>  > the whole buffer then dac_dat() will call out_flush().
>  >
>  > Without that all buffering becomes useless because we flush every
>  > sample we receive and we start to crawl.
>
>
> Nothing is crawling here, just use a reasonable threshold for flushing
>  _after_ the callback.

We don't really care what happened in the callback.  We need to write
when we reach the threshold, and this is what is implemented.
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[andrzej zaborowski]

On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
> Jan Kiszka wrote:
>  > Nothing is crawling here, just use a reasonable threshold for flushing
>  > _after_ the callback. Need to check, but maybe we can even wait the 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
>  ===================================================================
>  --- 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 = (struct wm8750_s *) opaque;
>
>
> -    if (s->idx_out >= free_b) {
>  -        s->idx_out = free_b;
>  -        s->req_out = 0;
>
> +    s->req_out = free_b;
>  +    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>
> +    if (s->idx_out >= 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.

Regards
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

andrzej zaborowski wrote:
> On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Jan Kiszka wrote:
>>  > Nothing is crawling here, just use a reasonable threshold for flushing
>>  > _after_ the callback. Need to check, but maybe we can even wait the 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
>>  ===================================================================
>>  --- 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 = (struct wm8750_s *) opaque;
>>
>>
>> -    if (s->idx_out >= free_b) {
>>  -        s->idx_out = free_b;
>>  -        s->req_out = 0;
>>
>> +    s->req_out = free_b;
>>  +    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>>
>> +    if (s->idx_out >= 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.

True, that's redundant, let's fix it this way (this even obsoletes the
flush in out_cb):

Index: hw/wm8750.c
===================================================================
--- 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 = (struct wm8750_s *) opaque;
 
-    if (s->idx_out >= free_b) {
-        s->idx_out = free_b;
-        s->req_out = 0;
-        wm8750_out_flush(s);
-    } else
-        s->req_out = free_b - s->idx_out;
- 
-    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
+    s->req_out = free_b;
+    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
 }
 
 struct wm_rate_s {
@@ -623,7 +617,7 @@ void wm8750_dac_dat(void *opaque, uint32
     *data = sample & s->outmask;
     s->req_out -= 4;
     s->idx_out += 4;
-    if (s->idx_out >= sizeof(s->data_out) || s->req_out <= 0)
+    if (s->idx_out >= sizeof(s->data_out)/2 || s->req_out <= 0)
         wm8750_out_flush(s);
 }
 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

Jan Kiszka wrote:
> andrzej zaborowski wrote:
>> On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Jan Kiszka wrote:
>>>  > Nothing is crawling here, just use a reasonable threshold for flushing
>>>  > _after_ the callback. Need to check, but maybe we can even wait the 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
>>>  ===================================================================
>>>  --- 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 = (struct wm8750_s *) opaque;
>>>
>>>
>>> -    if (s->idx_out >= free_b) {
>>>  -        s->idx_out = free_b;
>>>  -        s->req_out = 0;
>>>
>>> +    s->req_out = free_b;
>>>  +    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>>>
>>> +    if (s->idx_out >= 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.
> 
> True, that's redundant, let's fix it this way (this even obsoletes the
> flush in out_cb):
> 
> Index: hw/wm8750.c
> ===================================================================
> --- 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 = (struct wm8750_s *) opaque;
>  
> -    if (s->idx_out >= free_b) {
> -        s->idx_out = free_b;
> -        s->req_out = 0;
> -        wm8750_out_flush(s);
> -    } else
> -        s->req_out = free_b - s->idx_out;
> - 
> -    s->data_req(s->opaque, s->req_out >> 2, s->req_in >> 2);
> +    s->req_out = free_b;
> +    s->data_req(s->opaque, free_b >> 2, s->req_in >> 2);
>  }
>  
>  struct wm_rate_s {
> @@ -623,7 +617,7 @@ void wm8750_dac_dat(void *opaque, uint32
>      *data = sample & s->outmask;
>      s->req_out -= 4;
>      s->idx_out += 4;
> -    if (s->idx_out >= sizeof(s->data_out) || s->req_out <= 0)
> +    if (s->idx_out >= sizeof(s->data_out)/2 || s->req_out <= 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[andrzej zaborowski]

On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>  > 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.
>
> True, that's redundant, let's fix it this way (this even obsoletes the
>  flush in out_cb):

Ok, although I insist that it doesn't obsolete the check in out_cb
(hence why I added it).  Imagine s->req_out was 5000, and the guest
already wrote 1000 bytes. Now suddenly audio API calls out_cb with
free_b == 1500. In this case we have half of the buffer ready to
write, *before* we call s->data_req.

BTW I just committed a change to the MusicPal LCD, please check if it
still works.  Thanks.
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

andrzej zaborowski wrote:
> On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>>  > 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.
>>
>> True, that's redundant, let's fix it this way (this even obsoletes the
>>  flush in out_cb):
> 
> Ok, although I insist that it doesn't obsolete the check in out_cb
> (hence why I added it).  Imagine s->req_out was 5000, and the guest
> already wrote 1000 bytes. Now suddenly audio API calls out_cb with
> free_b == 1500. In this case we have half of the buffer ready to
> write, *before* we call s->data_req.

Yes, buffer/2 was attacking the symptom, wrongly, see my other mail.

> BTW I just committed a change to the MusicPal LCD, please check if it
> still works.  Thanks.

Still fine!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [4249] Improve audio api use in WM8750.

[andrzej zaborowski]

On 25/04/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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.

It's not really caching but rather trying to emulate the hardware's
FIFOs so that we get the same behavior.  But I see the problem: the
FIFO on wm8750 side was used to avoid buffering data more than once
for the Spitz and the Neo1973 machines. But they use a I2S interface
which is totally different than that of MusicPal (they both have a hw
register through which all samples have to go, rather than a kind of
DMA).  What we need is an api to let the cpu explicitely flush the
data, because in the model with DMA, only the CPU knows when it's good
moment to do that (e.g at the end of audio_callback in hw/musicpal.c),
I'll try to come up with something like that.  An arbitrary threshold
in wm8750 won't work for all machines.

Regards