-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement VirtIO sound device #53
base: master
Are you sure you want to change the base?
Conversation
4a8a6fc
to
0902227
Compare
Please provide testing procedures once the change is ready. |
0902227
to
ebba2c7
Compare
2b1009d
to
be7ad90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run clang-format
before submitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consult https://github.com/cntools/cnfa/blob/master/.github/workflows/build-cnfa.yml and mention the build dependency in top-level README.md
.
a67216b
to
1fc471c
Compare
862cb51
to
da29ce5
Compare
af8f287
to
bcb3795
Compare
2830a0f
to
5251ec6
Compare
a7a8782
to
ecb2614
Compare
58ecbaa
to
3e42d75
Compare
uint32_t Status; | ||
uint32_t InterruptStatus; | ||
/* supplied by environment */ | ||
uint32_t *ram; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure ram
is proper here. Should it be mem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copy-and-paste from virtio-net.c
. Maybe we can make a difference in this commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the content of queue.h
to utils.h
as rv32emu does. Then, enforce the Linux-like naming scheme.
uint32_t *ram = vsnd->ram; | ||
void *priv = vsnd->priv; | ||
uint32_t jacks = PRIV(vsnd)->jacks; | ||
uint32_t streams = PRIV(vsnd)->streams; | ||
uint32_t chmaps = PRIV(vsnd)->chmaps; | ||
uint32_t controls = PRIV(vsnd)->controls; | ||
memset(vsnd, 0, sizeof(*vsnd)); | ||
vsnd->ram = ram; | ||
vsnd->priv = priv; | ||
PRIV(vsnd)->jacks = jacks; | ||
PRIV(vsnd)->streams = streams; | ||
PRIV(vsnd)->chmaps = chmaps; | ||
PRIV(vsnd)->controls = controls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use memcpy
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jserv , it seems that copying certain members of a struct using dst->foo = src->foo
will be the sane way according this StackOverflow answer.
} | ||
|
||
/* Control the callback to prepare the buffer */ | ||
/* TODO: add lock to avoid race condition */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may consolidate the testing for producer-consumer implementation in advance.
This comment was marked as resolved.
This comment was marked as resolved.
2e9b605
to
5b3011e
Compare
void semu_timer_rebase(semu_timer_t *timer, uint64_t time); | ||
|
||
/* Linux-like queue API */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's enforce the Linux kernel style naming scheme.
virtio-snd.c
Outdated
memcpy(props->ring.buffer + idx, payload, size - idx); | ||
memcpy(props->ring.buffer, payload, n - (size - idx)); | ||
} | ||
asm("" ::: "memory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create a compiler barrier to make sure the data has to been written to ring buffer thus prevent race condition (see the DPDK counterpart).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is necessary, I will suggest wrapping it with macro for readability.
I can build on macOS with CoreAudio support now. What is missing to complete PCM functionality? |
Hi @jserv , I list the messing parts for completing PCM functionality:
For testing, I come up with the following two methods and the corresponding expected results:
|
88676c4
to
80b61e7
Compare
Enable ALSA driver and System V IPC in Linux Kernel. For ALSA, the debug is enabled and will be removed once this commit is ready. Add description of descriptor chaining, yet need refactoring the description as well as code (exists uncertainty of the query struct). Succeed to initialize virtio-snd. Handle requests in control and TX queue, and print the address and length of each virtq element to check the validness of self-implement queue. For macOS, an experimental core audio library check is implemented. As the driver sends the PCM frames asynchronously, use a dedicated thread for receiving frames from driver. The reason that pcm_release state hangs is because of PulseAudio, need to address this later. Add ring buffer struct so that we don't need to allocate space frequently. Can play guest sound. However, the sound is lagging.
80b61e7
to
56a79dd
Compare
Implement VirtIO sound device supporting these operations (the item with checked box checked means it is implemented right now):
VIRTIO_SND_R_JACK_INFO
VIRTIO_SND_R_PCM_INFO
VIRTIO_SND_R_CHMAP_INFO
VIRTIO_SND_R_PCM_SET_PARAMS
VIRTIO_SND_R_PCM_PREPARE
VIRTIO_SND_R_PCM_RELEASE
VIRTIO_SND_R_PCM_START
VIRTIO_SND_R_PCM_STOP
Test Cases
The test cases are subjected to be altered.
boot up test
test procedures
make check
to runsemu
.dmesg
).expected results
check driver configuration
test procedures
aplay -l
in emulator.expected results
play sound
test procedures
speaker-test
in emulator.expected results
speaker-test
is executing.