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

[Liam Girdwood <liam.r.girdwood@linux.intel.com>]

On Wed, 2019-11-06 at 10:28 +0100, Mikhail Golubev wrote:
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/54
> 
> Request for a vote.

Sorry, I think there are still too many opens. I would also recommend
that this proposal be sent to ALSA devel list (subscribers only) to get
a more thorough review since it will be difficult to fix once deployed.

Can you also provide more details on the use case testing carried out
here ? 

> 
> BR,
> Mikhail.
> 
> On 11/5/19 4:06 PM, Mikhail Golubev wrote:
> > From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
> > 

snip

> > 
> > +\subsection{Device Operation}\label{sec:Device Types / Sound
> > Device / Device Operation}
> > +
> > +All control messages are placed into the controlq virtqueue and
> > use the following
> > +layout structure and definitions:
> > +
> > +\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,

Where do I tear down the PCM when I'm finished with it ? i.e. to free
resources.

> > +
> > +    /* generic status codes */
> > +    VIRTIO_SND_S_OK = 0x8000,
> > +    VIRTIO_SND_S_BAD_MSG,
> > +    VIRTIO_SND_S_NOT_SUPP,
> > +    VIRTIO_SND_S_IO_ERR
> > +};
> > +
> > +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}
> > +

Using request_code for payload size is just asking for trouble.

> > 
> > +
> > +\subsubsection{Device Operation: PCM I/O requests}
> > +
> > +All I/O requests are placed into the pcmq virtqueue. Each request
> > is in the
> > +following form:
> > +
> > +\begin{lstlisting}
> > +struct virtio_snd_pcm_xfer {
> > +    le32 stream;
> > +    u8 data[];
> > +    le32 status;
> > +    le32 actual_length;
> > +};

Using period/fragment/block size for PCM means we only copy the minimum
data necessary and dont have to pad. 

Thanks

Liam