Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > qemu has rather small buffers backend buffers, to keep latencies low.
> > So, yes it would copy data to backend buffers.  No, it would most likely
> > not copy over everything immediately.  It will most likely leave buffers
> > in the virtqueue, reading the data piecewise in the audio backend timer
> > callback, complete the virtio request when it has read all data.  So
> > there wouldn't be a huge gap between completing the request and feeding
> > the data to the host hardware.
> 
> Yes, there is a gap, and this causes the following two concerns:
> 
> 1. An application can rewind its pointer and rewrite part of this buffer. The
> worst case is if the application rewrites the part currently being read by the
> device.

Depends on the driver implementation.  If the driver will copy over the
data to a buffer not visible to the userspace application this can't
happen.  If the driver allows userspace mmap the buffer, then queues
virtio requests with pointers to those buffers, then yes, the
application could modify buffers which are sitting in the queue waiting
to be processed by the device.

> > While talking about latencies:  What happened to the idea to signal
> > additional device latencies (backend buffering and processing ...) to
> > the driver?
> 
> Yeah, such an additional latency has a runtime nature, and we could report it
> as part of an I/O request completion. We only need to choose units (time or
> bytes).

I'd stick to bytes, for consistency with period and buffer sizes.

> Now we have only optional notifications (periods and xrun). But let's say,
> that the device MAY send the xrun notifications and MUST send the period
> notifications, and remove them from feature bits.
> 
> Then we define requirements for message-based transport:
> 1. the tx and rx queues are filled in with buffers having period_bytes size,
> 2. buffer_bytes % period_bytes == 0,
> 3. total buffers size in the queue is buffer_bytes.
> 
> It is still not clear, when to complete playback buffers, but at least such
> approach makes the driver being interrupt-driven.

Period notification would be implicit (playback buffer completion) or
explicit event queue message?

On playback buffer completion: adding a latency field to the completion
field could solve that nicely I think.  The latency field would specify
how long it will take until the device finishes playing that buffer.  So
in case the device completes the buffer when playback is finished it
would fill in "0" there.  In case the device completes the buffer after
copying over the data to the internal buffer it would fill in the
internal buffer size there (or maybe even the number of bytes which are
currently used in the internal buffer).

> > Completely different topic:  I think we should consider adding tags to
> > streams.  Some background:  In qemu we have two almost identical hda
> > codecs: hda-duplex and hda-micro.  The only difference is that
> > hda-duplex has the input stream tagged as "line-in" and the output
> > stream as "line-out", whereas hda-micro has the input channel tagged as
> > "microphone" and the output channel as "speaker".  hda-micro was created
> > because some windows application refused to accept "line-in" as audio
> > source.  So applications clearly do care, and in case you have two input
> > streams it would help applications pick a reasonable default.
> 
> Should it be a fixed-size field in the capabilities structure?

Yes, a simple enum would do the trick I think.

Not sure if we want add more properties.  hda codecs can also specify
the color coding of physical jacks (line-in is red, ...) for example.
Probably not useful for most use cases (i.e. when sound data lands @
pulseaudio where the user can re-route things as he wants), but it might
be useful when a fixed physical host output is assigned to the guest.

cheers,
  Gerd