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

[Gerd Hoffmann <kraxel@redhat.com>]

> Feature bits
> ------------
> 
> VIRTIO_SND_F_PCM_OUTPUT
> VIRTIO_SND_F_PCM_INPUT
> 
> and new ones would be
> 
> VIRTIO_SND_F_PCM_EVT_PERIOD - support elapsed period notifications
> VIRTIO_SND_F_PCM_EVT_XRUN - support underrun/overrun notifications
> VIRTIO_SND_F_PCM_MSG - support message-based data transport
> VIRTIO_SND_F_PCM_HOST_MEM - support sharing host memory with guest
> VIRTIO_SND_F_PCM_GUEST_MEM - support sharing guest memory with host
> 
> Not sure about correct naming here.

Hmm, I think at least parts of this should be device configuration not
feature bits ... 

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).

PCM_EVT_* probably makes sense to be a per-stream configuration bit.

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.

> By the way, Liam mentioned something about using PCI BARs for mapping host
> memory. Does it mean, that some features will be transport-specific?

In the latest spec revision virtio got support for shared memory
regions.  This should be used for VIRTIO_SND_F_PCM_HOST_MEM.

> What about virtio mmio?

Right now this is defined for the pci transport only.

> Virtqueues
> ----------
> 
> As Gerd explained, we need 4 virtqueues in total:
> 
> 1. control queue (guest->host: commands)
> 2. events queue (host->guest: notifications)
> 3. tx queue (guest->host: playback)
> 4. rx queue (host->guest: capture)
> 
> The first one always exists, but all other depend on feature bits. Should we
> check and assign these in some pre-defined order?

I'd suggest to make them all mandatory.

> 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 also affects how large configuration space should be.

I'd make the format bitfield 64bit no matter what.  It doesn't cost much
and it leaves room to extend things without hassle if needed.

> Common control message structure will be explicitly splitted into two
> 
> struct virtio_snd_ctl_req {
>     le32 type;
      le32 stream_id;
>     u8 payload[];
> };

> and the SET_FORMAT request will be replaced with the SET_PARAMS
> 
> struct virtio_snd_pcm_set_params {
>     struct virtio_snd_pcm_hdr hdr;
>     le32 channels;
>     le32 format;
>     le32 rate;
>     le32 period_size;
> };
> 
> We only need to decide units for the period_size field (frames or bytes).

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

> PCM I/O requests
> --------------------
> 
> As we get rid of multiplexing, we don't need a header anymore.

What if a device wants support two playback streams (possibly even with
different features, say one stereo and one 5:1)?

> And a message
> will have only status
> 
> 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?

cheers,
  Gerd