Matias Ezequiel Vara Larsen
2023-Sep-20 13:18 UTC
virtio-sound linux driver conformance to spec
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: > > > > Hello, > > > > > > > > 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. > > > > Matias > > 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 do: ``` schedule_work(&vss->elapsed_period); virtsnd_pcm_msg_send(vss); ``` 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); snd_pcm_period_elapsed(vss->substream); + 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) { wake_up_all(&vss->msg_empty); } 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. Matias
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: >>>>> Hello, >>>>> >>>>> 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. >>> >>> Matias >> >> 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 > do: > ``` > schedule_work(&vss->elapsed_period); > > virtsnd_pcm_msg_send(vss); > ``` > 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); > > snd_pcm_period_elapsed(vss->substream); > + 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) { > wake_up_all(&vss->msg_empty); > } > > > 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