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 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);
>    }

Apparently i should get some sleep..

[..snip..]

> > 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? :->

Heh. The reason why it's in this state is probably that i has only one 
(disabled) user, namely ac97, and this one is relative newcomer. But this 
are indeed that simple, one must provide this AUD_set_volume_out thing 
that would fill in the vol structure of the soft voice, there are 3 fields 
to set (left, right and mute) two of them being 32.32 fixed point (if my 
memory serves me). The above blunder, however, shows that i'm in no shape 
of doing that, at least not untill i get some sleep.

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

And not forgetting to set aud_settings endiannes to AUDIO_HOST_ENDIANESS

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

So just leave the callback until free get's big enough, you will get
dropouts either way, it's just that. I guess you might also use the
AUD_init_time_stamp_out/AUD_get_elapsed_usec_out mechanism (implemenation
is right at the end of audio/audio_template.h), but i still think that
just going with free is saner approach.

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

Great.

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

No, you can't customize that.

--
mailto:address@hidden