Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev <anton.yakovlev@opensynergy.com>]

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