Re: virtio-sound linux driver conformance to spec

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

On Mon, Sep 25, 2023 at 10:04:33AM +0900, Anton Yakovlev wrote:
> 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.
> 
> 
Hello Anton,

Thanks, I will try to propose a better fix in the next few weeks. I
am not familiar with sound drivers so I may require some help. I'll let
you know if I have any question.

Thanks, Matias.