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

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
> +
> +None currently defined.

Flags for hostmem & guestmem here?

This could be an information the driver might want to know before
initializing the virtqueues.

> +\item START
> +\item PAUSE
> +\item UNPAUSE
> +\item STOP

Why pause/unpause?

I don't see a reason compelling for this given the driver can restart a
stream (i.e. set-params -> prepare -> start -> stop -> start -> stop ->
release).  Would a device handle stop and pause in a different way?

> +/* supported PCM stream features */
> +enum {
> +    VIRTIO_SND_PCM_F_HOST_MEM = 0,
> +    VIRTIO_SND_PCM_F_GUEST_MEM,

Is this useful as stream property?  I would expect when supported by a
device it would work for all streams.

> +    VIRTIO_SND_PCM_F_EVT_PERIOD,

I think this is only useful for hostmem/guestmem.

> +struct virtio_snd_pcm_set_params {
> +    struct virtio_snd_pcm_hdr hdr; /* .code = VIRTIO_SND_R_PCM_SET_PARAMS */
> +    u8 features;
> +    u8 channels;
> +    u8 format;
> +    u8 rate;
> +    le32 buffer_bytes;
> +    le32 period_bytes;
> +};
> +\end{lstlisting}
> +
> +The request contains the following fields:
> +
> +\begin{description}
> +\item[\field{features}] (device-readable) is a selected feature bit map:
> +\begin{itemize*}
> +\item VIRTIO_SND_PCM_F_HOST_MEM: use shared memory allocated by the host (TBD),
> +\item VIRTIO_SND_PCM_F_GUEST_MEM: use shared memory allocated by the guest (TBD),

I'd suggest to drop those bits here.  For hostmem + guestmem we probably
need a virtio_snd_pcm_set_params_{hostmem,guestmem} struct (and command)
with additional parameters anyway.

> +\item VIRTIO_SND_PCM_F_EVT_PERIOD: enable elapsed period notifications,

Same comment as above ;)

> +\begin{itemize*}
> +\item VIRTIO_SND_EVT_PCM_PERIOD: send notifications about each elapsed period,
> +the size of the period is specified in the \field{period_bytes} field.

Same comment as above ;)

> +\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
> +
> +An I/O message consists of the header part, followed by the buffer, and then
> +the status part.

Each buffer MUST (or SHOULD?) be period_bytes in size.

The driver SHOULD queue (buffer_bytes / period_bytes) virtio buffers, so
the device has buffer_bytes total buffer space available.

cheers,
  Gerd