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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]