[Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal

[Jan Kiszka]

malc wrote:
> On Mon, 14 Apr 2008, Jan Kiszka wrote:
> 
>> malc wrote:
>>> On Sun, 13 Apr 2008, Jan Kiszka wrote:
> 
> [..snip..]
> 
>>>> +static void sound_fill_mixer_buffer(mv88w8618_sound_state *s,
>>>> unsigned int length)
>>>> +{
>>>> +    unsigned int pos;
>>>> +    double val;
>>>> +
>>>> +    if (s->mute) {
>>>> +        memset(s->mixer_buffer, 0, length);
>>>
>>> Zero is not correct "mute" value for signed formats.
>>
>> Hmm, maybe it's still too early for me - but what else??
> 
> Well.. 0x80 for S8 and 0x8000 for S16 (audio_pcm_info_clear_buf in
> audio.c)

void audio_pcm_info_clear_buf (struct audio_pcm_info *info, ...
{
...
    if (info->sign) {
        memset (buf, 0x00, len << info->shift);
    }

>>>
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (s->playback_mode & 1)
>>>> +        for (pos = 0; pos < length; pos += 2) {
>>>> +            val = *(int16_t *)(s->target_buffer + s->play_pos + pos);
>>>> +            val = val * pow(10.0, s->volume/20.0);
>>>> +            *(int16_t *)(s->mixer_buffer + pos) = val;
>>>> +        }
>>>
>>> On the surface it's sounds like endianess problems are ahead.
>>
>> Oh, yes.
>>
>>>
>>>> +    else
>>>> +        for (pos = 0; pos < length; pos += 1) {
>>>> +            val = *(int8_t *)(s->target_buffer + s->play_pos + pos);
>>>> +            val = val * pow(10.0, s->volume/20.0);
>>>> +            *(int8_t *)(s->mixer_buffer + pos) = val;
>>>> +        }
>>>> +}
>>>
>>> There's actually a generic framework for volume manipulation in QEMUs
>>> audio, unfortunatelly it's unconditionally disabled (define NOVOL in
>>> audio/mixeng.c) for the reasons i can't recall now.
>>
>> Yeah, looked around a bit before hacking those loops but indeed found
>> nothing ready to use.
> 
> It's just a question of adding(and using) AUD_set_volume[_in/out],
> which will set vol field of respective soft voice and removing
> uncoditional #define NOVOL from mixeng.c.

Well, I was (and still am) deterred by the plain fact that there are
some dead AUD_set_volume spots in ac97.c. If it is that simple, why do
we still lack an implementation? My feeling is that you are far deeper
into this than I am at this point. So maybe you would like me to test
some AUD_set_volume-providing patch of yours? :->

>>>
>>> Also adlib emulation is only conditionally available due to
>>> usage of floating point arithmetics, maybe rules have changed now
>>> though, but "fixing" this particular issue in this particular case
>>> seems easy enough.
>>
>> Sorry, didn't get what you mean here.
> 
> Adlib is not built by default, requires explicit --enable-adlib switch
> to configure, because fmopl.c uses FPU and Fabrice didn't like the idea.
> Fixing it here is just a matter of switching to fixed point.
> 
> Then again if volume manipulation in main audio proper are brought up
> to date this all can be scrapped and it will also "magically" solve the
> endianness issue.

I do agree that such stuff should be solved generically, but for now
adding a simple le16_to_cpu does not compare to fixing QEMU's
soft-volume management for me (given limited time).

>>>
>>>> +static void sound_callback(void *opaque, int free)
>>>> +{
>>>> +    mv88w8618_sound_state *s = opaque;
>>>> +    uint32_t old_status = s->status;
>>>> +    int64_t now = qemu_get_clock(rt_clock);
>>>> +    unsigned int n;
>>>> +
>>>> +    if (now - s->last_callback < (1000 * s->threshold/2) / (44100*2))
>>>> +        return;
>>>> +    s->last_callback = now;
>>>
>>> Here two clocks are being used for things - the audio one which
>>> influences
>>> `free' paramter and rt_clock, even if there are reasons why free
>>> could not
>>> be used choice of rt_clock over vm_clock seems wrong.
>>
>> Yes, using the monotonic vm_clock is better. Will change.
> 
> Why is it needed at all? `free' already supplies you with an audio clock
> source.

The callback mechanism fires far more often than the emulated hardware
expects (and can handle), I need throttling.

The sad truth is that my original version was still off my an order of
magnitude, causing overload for the guest while decoding obviously more
complex 128kbit-WMA streams. Now it runs smoothly. :)

Maybe there is a way to customize the callback rate, I haven't looked
that close, but I'm open for suggestions.

Thanks again,
Jan

signature.asc
Description: OpenPGP digital signature