Re: [virtio-dev] [PATCH] snd: Add virtio sound device specification

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > I don't think there is a use case which guestmem can handle but hostmem
> > can not.
> 
> There may be a special security requirement like "a guest must not have any
> access to a host memory" (especially in case of type 1 hv).

Hmm, ok.  Point.

> Also, sharing memory implies sharing of two things: buffer itself and a page
> containing position counter. And it goes in both directions (a host shares
> buffer and hardware position or a guest shares buffer and application
> position). And it seems, that overall solution can benefit if there will be
> four independent entities. For example, message-based transport can have
> some benefits, if a host will provide its hardware pointer in a shared page.

Hmm, I'm not sure mixing things is a good idea.  When using shared
memory I would use it for both data and pointer.

> > > > So, it's not a "here is a buffer, fill it please", "here is the next,
> > > > ..." ping pong game between driver and device.  There is a queue with
> > > > multiple buffers instead, and the device fills them one by one.
> > > 
> > > Then, should we make this pattern to be mandatory?
> > 
> > It's pretty standard for virtio.  The network receive queue works
> > fundamentally the same way for example, except that network buffers
> > actually might be half-filled only because you get network packets
> > where you don't know the size beforehand instead of a constant stream
> > of sound samples which you can easily pack into buffers as you like.
> 
> Except the end of the capture stream case. Can we use the len field in
> virtq_used_elem for storing actual captured size then?

Yes.

> > Note that it depends on the use case whenever long latencies are
> > actually worse.  Some use cases don't care much (music playback).  For
> > some use cases it is good enough to know the exact latency so you can
> > take it into account for synchronization (video playback).  Some
> > usecases are pretty much impossible with long latencies (games, voip).
> 
> By the way, there is a good point here. Maybe it makes sense for the device
> somehow reports its current/additionally latency value? Like, as a part of a
> response to the set_params request?

You mean for latencies additional to the ones implied by buffer size?
Yes, that sounds useful.

> > > There's always a trade-off and an implementer might choose
> > > whatever fits better.
> > 
> > Sure.  But you choose once, when configuring the stream, not for each
> > individual buffer, right?  So the driver will:
> > 
> >    (1) set-params (including period_bytes), taking into account what
> >        userspace asked for.
> >    (2) queue buffers for the stream, each being period_bytes in size.
> >    (3) start stream.
> 
> Originally, we introduced period_bytes for enabling period notification
> feature (which is optional). At least, it was not supposed to be used for
> slicing a buffer into pieces (although, the driver may chose to work in this
> way).

Ah, I've missed the detail that you've decoupled buffers and period
notification.  Guess we talked past each other because of that.  So I'm
trying to outline things again, just to be sure we are on the same page:

The driver picks total buffer size (say 16k for example) and period size
(say 2k).  The driver allocates 8 virtio buffers with 2k each.
Allocating one big buffer and slice it works too of course.  Then the
driver goes queue up these 8 virtio buffers to the virtqueue.  The
device returns the filled virtio buffers to the driver.  Then you don't
separate period notification in the first place, you can simply use the
buffer completion notifications for that.

cheers,
  Gerd