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

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > Instead of PCM_OUTPUT and PCM_INPUT I would put the number of input and
> > output streams into the device config space (and explicitly allow count
> > being zero).
> 
> There are several options for the number of streams:
> 
> 1. The device can provide one input and/or output stream.
> 2. The device can provide several input and/or output streams, and all of them
> share the same capabilities.
> 3. The device can provide several input and/or output streams, and they can
> have different set of capabilities.
> 
> Overall design depends on chosen option. Current draft is based on p.1. But we
> have never discussed what is the best solution here.

(3) looks most useful to me.

> > PCM_MSG -- I would drop the feature bit and make that mandatory, so we
> > have a common baseline on which all drivers and devices can agree on.
> 
> Then we need to inform device what "transport" will be in use (I assumed it
> would be feature negotiation).

Whenever other transports (i.e. via shared memory) are supported: yes,
that should be a feature bit.

Not sure about choosing the transport.  If both msg (i.e. via virtqueue)
and shared memory are available, does it make sense to allow the driver
choose the transport each time it starts a stream?

> > Right now this is defined for the pci transport only.
> 
> And now we only declare a feature bit, right?

Not fully sure what you mean here.

Having the first revision define only the virtqueue transport, reserve a
feature bit for the shared memory transport and hash out the details of
that later is possible.

> > > Configuration space
> > > -------------------
> > > 
> > > We talked about PCM formats defined in ALSA. In Linux kernel v5.4-rc8 we have
> > > 
> > > Do we really need all of them? And I skipped endianess, but should we care
> > > about it as well?
> > 
> > I'd keep the list as short as possible.  The formats listed in the patch
> > looked like a good start.
> 
> It was proposed to have all non-compressed formats.
> 
> And... do we care about cross-endian cases?

I'd start with little endian formats only.  virtio byte order is little
endian, and the world is moving to little endian anyway.  We can add big
endian formats later should this turn out to be needed, but I don't
expect that to be the case.


> > I'd suggest bytes (and rename the field to period_bytes).

> > > struct virtio_snd_pcm_status {
> > >      le32 status;
> > >      le32 actual_length;
> > > };
> > 
> > Hmm.  Why actual_length?  Do we want allow short transfers?  Why?
> > Wouldn't it be more useful to just fill the buffer completely?
> 
> In current design we have no buffer size requirements. It means, that in
> theory the device and the driver could have buffers with different sizes.

Should we define that the (virtio) buffer size must be period_bytes then?

> Also, the capture stream is a special case. Now we don't state explicitly
> whether read request is blockable or not.

The concept of "blockable" doesn't exist at that level.  The driver
submits buffers to the device, the device fills them and notifies the
driver when the buffer is full.  It simply doesn't work like a read(2)
syscall.

Buffers not being filled completely could possibly happen when stopping
a stream with a half-filled buffer, if we want allow that in the spec.

Another possible case would be compressed streams with variable bitrate,
where the number of bytes needed for a millisecond of sound data isn't
fixed.

cheers,
  Gerd