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

[Anton Yakovlev <anton.yakovlev@opensynergy.com>]

Hi,

On 17.12.2019 15:45, Gerd Hoffmann wrote:
>    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.

Yes, it might be. Yet this means that there will be two sets of features. And
current draft makes things more consistent (or at least looks so). Like, the
choice of transport looks more natural (just usual "feature negotiation").


> > +\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?

Well, usually restarting a paused stream means a lower start delay. But you
have a point here. I think it's worth asking other people's opinions on this.


> > +/* 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.

Since we allowed different capabilities for different streams, now it's
possible to have different backends for them. Iâm not sure how much this can
be a real scenario, but it is theoretically possible to have one backend that
is able to work with shared memory, and the other is not. The same can be said
for other stream feature bits.


> > +    VIRTIO_SND_PCM_F_EVT_PERIOD,
>
> I think this is only useful for hostmem/guestmem.

Using message-based transport, an application can send the entire playback 
buffer at the beginning, and then send period-sized parts on each period 
notification.


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

See comment at the beginning.


> > +\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.

I think, it's better not to set queued buffer size restrictions. Periods are
an optional feature. If the driver is not interested in period notifications,
it's unclear what period_bytes means here.

Although, for the input stream we could say, that the total size of buffers in
the rx queue should be buffer_bytes.


> cheers,
>    Gerd

--
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 30 60 98 54 0
E-Mail: anton.yakovlev@opensynergy.com