Re: [virtio-dev] Request for a new device number for a virtio-audio device.

[Gerd Hoffmann <kraxel@redhat.com>]

On Fri, May 10, 2019 at 11:45:24AM +0200, Mikhail Golubev wrote:
> Hi all!
> 
> Sorry for breaking in the middle of the virtio audio driver and device
> development discussion. But we are developing a virtio sound card prototype for
> a while and we would like to share our specification draft and public header
> file (attaching 'virtio_snd.h' and 'virtio-snd-spec-draft.md'). The PoC for
> proposed approach was tested with virtio audio driver in Linux and in Windows 10
> as well.
> 
> It would be really great if we can collaborate on this topic. I would be happy
> to answer any questions.

Wow.  That looks pretty complete already ...

> Configuration space provides a maximum amount of available virtual queues. The
> nqueues value MUST be at least one.
> 
> ```
> struct virtio_snd_config
> {
>     le32 nqueues;
> };
> ```

Including or excluding the control queue?

> Each request begins with a 2-byte field that identifies a recipient function
> followed by a 2-byte field that identifies a function-specific request type.
> 
> Each response begins with a 4-byte field that contains a status code. The values
> 0-31 are reserved for common status codes, a function can define function-specific
> status codes starting from 32.

I'd suggest to use a common scheme with a single le32 field for both
requests and responses.

#define VIRTIO_SND_BASE_OFFSET 0x10000
#define VIRTIO_SND_PCM_OFFSET  0x20000

enum virtio_sound_request {
	VIRTIO_SND_BASE_REQ1 = VIRTIO_SND_BASE_OFFSET,
        VIRTIO_SND_BASE_REQ2,
	[ ... ]
	VIRTIO_SND_PCM_REQ1 = VIRTIO_SND_PCM_OFFSET,
	VIRTIO_SND_PCM_REQ2,
	[ ... ]
};

enum virtio_sound_response {
	VIRTIO_SND_E_SUCCESS,

	VIRTIO_SND_E_GENERAL = VIRTIO_SND_BASE_OFFSET,
	VIRTIO_SND_E_NOTSUPPORTED,
	[ ... ]
	VIRTIO_SND_PCM_E_NOT_READY = VIRTIO_SND_PCM_OFFSET,
	[ ... ]
};

> struct virtio_snd_req
> {
	enum virtio_sound_request;

> struct virtio_snd_rsp
> {
	enum virtio_sound_response;

(maybe just drop the single-element structs and use the enums directly).

> ### Device Configuration
> 
> The device reports its configuration using descriptors. A descriptor is a data
> structure with a defined format. Each descriptor begins with a byte-wide field
> that contains the total number of bytes in the descriptor followed by a byte-wide
> field that identifies the descriptor type.
> 
> ```
> struct virtio_snd_generic_desc
> {
>     u8 length;
>     u8 type;
>     u16 padding;

I'd make length and type u16 and drop the padding.

> With the exception of the base function, all implemented functions MUST define
> its own descriptor(s) format (either presented in this specification or
> vendor-specific) and MUST includes these in configuration only if a particular
> function (or part of its functionality) is enabled in the device.

I don't think it is a good idea to allow vendor-specific descriptors
here.  It should all be in the virtio spec.  We might want reserve a
sub-range of "type" for experimental/development purposes though.

> ### Function Operation
> 
> #### Get Device Configuration
> 
> The driver sends the VIRTIO_SND_BASE_R_GET_CFG generic request, the device
> answers with a response containing device configuration.

Which is a list of descriptors I assume?

> #### Suspend
> 
> The driver sends the VIRTIO_SND_BASE_R_SUSPEND generic request, the device
> answers with a generic response. For each initialized function, the device SHOULD
> be able to preserve its runtime state and MUST put it into function-specific
> suspended state.
> 
> #### Resume
> 
> The driver sends the VIRTIO_SND_BASE_R_RESUME generic request, the device
> answers with a generic response. For each initialized function, the device SHOULD
> be able to restore its runtime state and MUST put it into function-specific
> non-suspended state.

Why these two are needed?

Also restoring state as optional device feature doesn't look useful to
me.

> - *Manual mode*: the driver assigns a dedicated virtual queue for a PCM substream
> and use it to transmit data buffers to the device. The device writes/reads PCM
> frames only upon receiving a buffer from the driver.

That is how I expect a virtio-audio device work.

> - *Automatic mode*: the driver allocates and shares with the device a pseudophysically
> continuous memory area containing runtime control registers and data buffer itself.

What is "pseudophysically continuous memory"?
Why do you think this is needed?

> Regardless of the mode of operation, the device MAY require to specify an
> approximate data buffer update frequency. In such case, the driver MUST specify
> an approximate update frequency (expressed in both microseconds and bytes units)
> before starting a PCM substream.

Why both microseconds and bytes?  Isn't that redundant?

> /* a PCM function descriptor */
> struct virtio_snd_pcm_desc
> {
>     /* sizeof(struct virtio_snd_pcm_desc) */
>     u8 length;
>     /* VIRTIO_SND_DESC_PCM */
>     u8 type;
>     /* a PCM function ID (assigned by the device) */
>     u8 pcm_id;
>     /* # of PCM stream descriptors in the configuration (one per supported PCM stream type) */
>     u8 nstreams;
> };

> /* supported PCM sample formats */
> #define VIRTIO_SND_PCM_FMT_MU_LAW       0
> #define VIRTIO_SND_PCM_FMT_A_LAW        1
> #define VIRTIO_SND_PCM_FMT_S8           2
> #define VIRTIO_SND_PCM_FMT_U8           3
> #define VIRTIO_SND_PCM_FMT_S16_LE       4
> #define VIRTIO_SND_PCM_FMT_S16_BE       5
> #define VIRTIO_SND_PCM_FMT_U16_LE       6
> #define VIRTIO_SND_PCM_FMT_U16_BE       7
> #define VIRTIO_SND_PCM_FMT_S24_LE       8
> #define VIRTIO_SND_PCM_FMT_S24_BE       9
> #define VIRTIO_SND_PCM_FMT_U24_LE       10
> #define VIRTIO_SND_PCM_FMT_U24_BE       11
> #define VIRTIO_SND_PCM_FMT_S32_LE       12
> #define VIRTIO_SND_PCM_FMT_S32_BE       13
> #define VIRTIO_SND_PCM_FMT_U32_LE       14
> #define VIRTIO_SND_PCM_FMT_U32_BE       15
> #define VIRTIO_SND_PCM_FMT_FLOAT_LE     16
> #define VIRTIO_SND_PCM_FMT_FLOAT_BE     17
> #define VIRTIO_SND_PCM_FMT_FLOAT64_LE   18
> #define VIRTIO_SND_PCM_FMT_FLOAT64_BE   19
> #define VIRTIO_SND_PCM_FMT_S20_LE       20
> #define VIRTIO_SND_PCM_FMT_S20_BE       21
> #define VIRTIO_SND_PCM_FMT_U20_LE       22
> #define VIRTIO_SND_PCM_FMT_U20_BE       23
> #define VIRTIO_SND_PCM_FMT_S24_3LE      24
> #define VIRTIO_SND_PCM_FMT_S24_3BE      25
> #define VIRTIO_SND_PCM_FMT_U24_3LE      26
> #define VIRTIO_SND_PCM_FMT_U24_3BE      27
> #define VIRTIO_SND_PCM_FMT_S20_3LE      28
> #define VIRTIO_SND_PCM_FMT_S20_3BE      29
> #define VIRTIO_SND_PCM_FMT_U20_3LE      30
> #define VIRTIO_SND_PCM_FMT_U20_3BE      31
> #define VIRTIO_SND_PCM_FMT_S18_3LE      32
> #define VIRTIO_SND_PCM_FMT_U18_3LE      33
> #define VIRTIO_SND_PCM_FMT_S18_3BE      34
> #define VIRTIO_SND_PCM_FMT_U18_3BE      35

That looks a bit over-engineered to me.

First, virtio uses little endian.  How about dropping all bigendian
formats?

Are 8-bit formats are still used in practice?

What are VIRTIO_SND_PCM_FMT_{S,U}20_LE ?

What are VIRTIO_SND_PCM_FMT_*_3BE ?

Which of these formats are actually implemented in your prototype?

> /* supported PCM frame rates */
> #define VIRTIO_SND_PCM_RATE_5512        0
> #define VIRTIO_SND_PCM_RATE_8000        1
> #define VIRTIO_SND_PCM_RATE_11025       2
> #define VIRTIO_SND_PCM_RATE_16000       3
> #define VIRTIO_SND_PCM_RATE_22050       4
> #define VIRTIO_SND_PCM_RATE_32000       5
> #define VIRTIO_SND_PCM_RATE_44100       6
> #define VIRTIO_SND_PCM_RATE_48000       7
> #define VIRTIO_SND_PCM_RATE_64000       8
> #define VIRTIO_SND_PCM_RATE_88200       9
> #define VIRTIO_SND_PCM_RATE_96000       10
> #define VIRTIO_SND_PCM_RATE_176400      11
> #define VIRTIO_SND_PCM_RATE_192000      12

Same question: Do we actually need all those?  My impression is that
these days 48000 is pretty much standard.  96000 + 192000 for higher
quality.  44100 sometimes still because it happens to be the CD sample
rate.  Maybe 32000/16000/8000 for low bandwidth codecs.

But 5512, 11025 & friends?

> The driver gets the VIRTIO_SND_PCM_FEAT_CHANNEL_MAP feature value in order to obtain
> a channel map with specified index, the device answers with the virtio_snd_pcm_chmap
> PCM response.
> 
> If channel map type allows to swap channels, the driver can set the VIRTIO_SND_PCM_FEAT_CHANNEL_MAP
> feature value specifying selected channel map data as a request argument. The
> device answers with a generic response.
> 
> ```
> /* a PCM channel map definitions */
> 
> /* All channels have fixed channel positions */
> #define VIRTIO_SND_PCM_CHMAP_FIXED      0
> /* All channels are swappable (e.g. {FL/FR/RL/RR} -> {RR/RL/FR/FL}) */
> #define VIRTIO_SND_PCM_CHMAP_VARIABLE   1
> /* Only pair-wise channels are swappable (e.g. {FL/FR/RL/RR} -> {RL/RR/FL/FR}) */
> #define VIRTIO_SND_PCM_CHMAP_PAIRED     2

What this is good for?  How would a driver ask the device to actually
swap the channels?  And isn't it easier to have the device provide a
complete list of possible channel maps and let the driver simply pick
one?

> /* Standard channel position definition */
> #define VIRTIO_SND_PCM_CH_NONE          0   /* undefined */
> #define VIRTIO_SND_PCM_CH_NA            1   /* silent */
> #define VIRTIO_SND_PCM_CH_MONO          2   /* mono stream */
> #define VIRTIO_SND_PCM_CH_FL            3   /* front left */
> #define VIRTIO_SND_PCM_CH_FR            4   /* front right */
> #define VIRTIO_SND_PCM_CH_RL            5   /* rear left */
> #define VIRTIO_SND_PCM_CH_RR            6   /* rear right */
> #define VIRTIO_SND_PCM_CH_FC            7   /* front center */
> #define VIRTIO_SND_PCM_CH_LFE           8   /* low frequency (LFE) */
> #define VIRTIO_SND_PCM_CH_SL            9   /* side left */
> #define VIRTIO_SND_PCM_CH_SR            10  /* side right */
> #define VIRTIO_SND_PCM_CH_RC            11  /* rear center */
> #define VIRTIO_SND_PCM_CH_FLC           12  /* front left center */
> #define VIRTIO_SND_PCM_CH_FRC           13  /* front right center */
> #define VIRTIO_SND_PCM_CH_RLC           14  /* rear left center */
> #define VIRTIO_SND_PCM_CH_RRC           15  /* rear right center */
> #define VIRTIO_SND_PCM_CH_FLW           16  /* front left wide */
> #define VIRTIO_SND_PCM_CH_FRW           17  /* front right wide */
> #define VIRTIO_SND_PCM_CH_FLH           18  /* front left high */
> #define VIRTIO_SND_PCM_CH_FCH           19  /* front center high */
> #define VIRTIO_SND_PCM_CH_FRH           20  /* front right high */
> #define VIRTIO_SND_PCM_CH_TC            21  /* top center */
> #define VIRTIO_SND_PCM_CH_TFL           22  /* top front left */
> #define VIRTIO_SND_PCM_CH_TFR           23  /* top front right */
> #define VIRTIO_SND_PCM_CH_TFC           24  /* top front center */
> #define VIRTIO_SND_PCM_CH_TRL           25  /* top rear left */
> #define VIRTIO_SND_PCM_CH_TRR           26  /* top rear right */
> #define VIRTIO_SND_PCM_CH_TRC           27  /* top rear center */
> #define VIRTIO_SND_PCM_CH_TFLC          28  /* top front left center */
> #define VIRTIO_SND_PCM_CH_TFRC          29  /* top front right center */
> #define VIRTIO_SND_PCM_CH_TSL           30  /* top side left */
> #define VIRTIO_SND_PCM_CH_TSR           31  /* top side right */
> #define VIRTIO_SND_PCM_CH_LLFE          32  /* left LFE */
> #define VIRTIO_SND_PCM_CH_RLFE          33  /* right LFE */
> #define VIRTIO_SND_PCM_CH_BC            34  /* bottom center */
> #define VIRTIO_SND_PCM_CH_BLC           35  /* bottom left center */
> #define VIRTIO_SND_PCM_CH_BRC           36  /* bottom right center */

Wow.  What is the use case for all those channels?  cinema sound?

> #### Prepare
> 
> The driver MUST send the VIRTIO_SND_PCM_R_PREPARE generic PCM request to prepare
> a substream for running, the device answers with a generic response.
> 
> #### Start
> 
> The driver sends the VIRTIO_SND_PCM_R_START generic PCM request, the device
> answers with a generic response.

So I guess the way to start a stream is (a) prepare, (b) queue some
buffers, (c) start.  Correct?

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org