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

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > > +\begin{description}
> > > +\item[0] controlq
> > > +\item[1] pcmq
> > > +\end{description}
> > > +
> > > +The controlq virtqueue always exists, the pcmq virtqueue only exists if
> > > +the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
> > 
> > Why not use two queues, one for input and one for output?  I think most
> > virtio devices do it that way. It could simplify the implementations,
> > shave 32 bits off the buffers, and allow to quiesce notifications for
> > input and output streams independently. And in the future you can extend
> > the device with more than two streams.
> 
> This was discussed earlier (having two separate virtqueues vs multiplexing
> into single one). It's hard to say what is better, but at least one
> virtqueue = less maintenance work at runtime.

I'd suggest separate tx (playback) and rx (record) queues, the workflow
is quite different so that makes sense.  Multiplexing multiple playback
streams over a single tx queue is alot less problematic than handling
both rx and tx in a single queue.

> > > +\begin{lstlisting}
> > > +enum {
> > > +    /* PCM control request types */
> > > +    VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> > > +    VIRTIO_SND_R_PCM_SET_FORMAT,
> > > +    VIRTIO_SND_R_PCM_PREPARE,
> > > +    VIRTIO_SND_R_PCM_START,
> > > +    VIRTIO_SND_R_PCM_STOP,
> > > +    VIRTIO_SND_R_PCM_PAUSE,
> > > +    VIRTIO_SND_R_PCM_UNPAUSE,
> > > +
> > > +    /* generic status codes */
> > > +    VIRTIO_SND_S_OK = 0x8000,
> > > +    VIRTIO_SND_S_BAD_MSG,
> > > +    VIRTIO_SND_S_NOT_SUPP,
> > > +    VIRTIO_SND_S_IO_ERR
> > > +};
> > 
> > Any reason not to use two separate enums starting from 0?
> 
> It's funny, because initially there were exactly two separate enums. It was
> proposed to combine them into one to keep things in one place.

Having non-overlapping values for request types and status codes is more
robust, so IMO this is a good thing.  I'd also suggest to *not* start
from 0 (so a fresh, zero-initialized field is not a valid request type).

> > > +struct virtio_snd_ctl_msg {
> > > +    /* device-read-only data */
> > > +    le32 request_code;
> > > +    u8 request_payload[];
> > > +    /* device-writable data */
> > > +    le32 response_status;
> > > +    u8 response_payload[];
> > > +};
> > > +\end{lstlisting}

Must be splitted into out and in structs (see also virtqueue element
discussion in v1 thread).

I'd also suggest to specify a union for the payload.  Or different
structs for different request types (if they have a payload).

cheers,
  Gerd