Re: virtio-sound linux driver conformance to spec

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Hello Matias,

On 20.09.2023 22:18, Matias Ezequiel Vara Larsen wrote:
On Tue, Sep 19, 2023 at 11:52:27AM -0400, Michael S. Tsirkin wrote:
On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:

This email is to report a behavior of the Linux virtio-sound driver that
looks like it is not conforming to the VirtIO specification. The kernel
driver is moving buffers from the used ring to the available ring
without knowing if the content has been updated from the user. If the
device picks up buffers from the available ring just after it is
notified, it happens that the content is old.

Then, what happens, exactly? Do things still work?

We are currently developing a vhost-user backend for virtio-sound and
what happens is that if the backend implementation decides to copy the
content of a buffer from a request that just arrived to the available
ring, it gets the old content thus reproducing some sections two times.
For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
`Front, front left...`. To fix this issue, our current implementation
delays reading from guest memory just until the audio engine requires.
However, the first implementation shall also work since it is conforming
to the specification.


Sounds like it. How hard is it to change the behaviour though?
Does it involve changing userspace?

AFAIU, a fix for the driver may be to somehow wait until userspace
updates the buffer before add it in the available ring.
So far, when the device notifies the driver that a new buffer is in the
used ring, the driver calls the virtsnd_pcm_msg_complete() function to

It first defers the notification to userspace regarding an elapse period
and then it enqueues the request again in the available
ring.`schedule_work()` defers the calling to the
`virtsnd_pcm_period_elapsed()` function that issues
`snd_pcm_period_elapsed(vss->substream);` to notify userspace.
My proposal would be that the driver could also defer
`virtsnd_pcm_msg_send(vss)` to execute just after
`snd_pcm_period_elapsed(vss->substream)`. Something like this:

diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index c10d91fff2fb..41f1e74c8478 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -309,6 +309,7 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
                 container_of(work, struct virtio_pcm_substream, elapsed_period);
+       virtsnd_pcm_msg_send(vss);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index aca2dc1989ba..43f0078b1152 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -321,7 +321,6 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
schedule_work(&vss->elapsed_period); - virtsnd_pcm_msg_send(vss);
         } else if (!vss->msg_count) {

I tested it and it looks it fixes the issue. However, I am not sure if
this is enough since I do not know if when `snd_pcm_period_elapsed()`
returns, the buffers have been already updated.

It's just a lucky coincidence that this change helped in your case.

Instead, to solve the problem, it's necessary to implement read/write()
operations for the substream, and disable MMAP access mode.

I'll try, but I'm not sure I'll have enough time for this in the near future.

Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Virtualization mailing list

[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux