Re: virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

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