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

[malc]

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)

> > > +        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.


> > 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.

> > > +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.

[..snip..]

> Thanks a lot for your review!

You are welcome.

--
mailto:address@hidden