On Fri, May 10, 2019 at 02:15:00PM +0000, Anton Yakovlev wrote:
> Hi Gerd! My name is Anton and I'm the original author of this draft. Thanks for comments!
>
> >> 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?
>
> Including the control queue.
Ok. Please say so explicitly in the specs.
> >> 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.
> > ...
> > (maybe just drop the single-element structs and use the enums directly).
>
> IMO, both options are quite fine. We decided to choose separate
> namespaces because it's more flexible approach.
I fail to see why it is more flexible. If you want go with the separate
function/request fields I'd suggest to do the same for responses.
> >> ### Device Configuration
> >> ...
> >> struct virtio_snd_generic_desc
> >> {
> >> u8 length;
> >> u8 type;
> >> u16 padding;
> >
> > I'd make length and type u16 and drop the padding.
>
> Well, it's a possible option. We just didn't want to waste additional space here.
Ok, saw later in the patch that other descriptors _replace_ padding
instead of _appending_ to the struct. So padding isn't there to align
fields appended, but round up the descriptor size for descriptor
alignment, correct?
I think it's fine then.
> >> 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.
>
> Yeah, an ability to define some experimental extensions was one of the
> targets here (like adding sound controls, non-PCM streams, MIDI and so
> on). Also, this spec targeted the widest possible range of cases.
> Like, some hardware has very tricky features which can not be (or just
> has no point to be) defined in common specification. One example is to
> allow switching between speaker and bluetooth output. Ususally you
> don't need it, but if you want to provide such feature to the guest,
> it's better to implement this as a custom extension.
Why? I think it makes perfect sense to have a standard function for
stream routing.
Even when allowing vendor specific descriptors/functions a sub-range of
"type" must be reserved for that and a way to identify the vendor is
needed.
> >> ### 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?
>
> You are right.
Good. Please clarify the specs.
> >> #### Suspend
> >> #### Resume
> >
> > Why these two are needed?
>
> I would say, that some virtualization platforms either do not notify
> your backend about guest suspend/resume or it's very tricky to
> distinguish such conditions from other shutdown/initialization types.
> According to my practice, it's always better to notify device side in
> explicit way, because the driver always know what is going now.
So the idea is the driver doesn't stop streams on suspend, but sends a
suspend notification to the device and expects the device stop the
streams?
> > Also restoring state as optional device feature doesn't look useful to
> > me.
>
> Well, restoring active playback or capturing is not impossible thing
> at all. From other side, maybe some device implementors might not
> want/need such functionality. So I'm not sure, what is better here.
I think we should have *one* way to handle suspend/resume. So either
make suspend/resume commands mandatory, or expect the driver to
stop/restart streams on suspend/resume.
I think the latter is the better way. If you want know on the device
side why a stream was stopped (why do you need to know btw?) you can add
a "reason" field to the stop command.
> >> - *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"?
>
> This memory is physically continuous from the driver's point of view.
So continuous in guest physical memory.
> > Why do you think this is needed?
>
> When you implement an audio device, you usually start with what we
> call "manual mode". It works, but only for simple use cases. There're
> two major challenges here.
>
> Thus, no requests/traps/hypercalls to the device while a stream is
> running - no problems here.
As already mentioned by Stefan you can run the virtqueue in polling mode
to avoid trapping into the hypervisor.
> 2) Everybody wants to reduce a latency as much as possible. One of the
> most logical way is to allow to mmap data buffer into the user space
> memory.
mmap() doesn't conflict with virtqueues. Userspace must notify the
kernel driver about buffer updates though.
> (Actually, in some cases you even have no choice here, like in
> case of the WaveRT interface in Windows.) But memory mapped buffer
> means, that you either have a very little or have not at all an idea
> what is going on with buffer content.
So how do you maintain virtio_snd_pcm_hw_regs.dma_position? Do you
allow userspace mmap the whole virtio_snd_pcm_shmem struct and update
it directly, without calling into the kernel driver?
> > Why both microseconds and bytes? Isn't that redundant?
>
> Just to be sure that both ends have exactly the same ideas.
I don't think this is needed. If one end gets the microseconds <=>
bytes transformation wrong you'll have bigger problems anyway ...
> > Are 8-bit formats are still used in practice?
>
> Actually, yeah. I saw quite recent chipsets with the U8 format
> support. Anyway, is it a big issue? It costs zero to support
> additional formats, so why not?
Well, the cost is not zero. Some more code (not that much indeed).
More testing (more significant cost). So I would try to avoid carrying
forward old stuff which is not used in practice any more just because
you can.
> > 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?
>
> Some format's physical width is not multiple of 8. Such samples are
> kept in "a storage" which width is multiple of 8 (like 20-bit samples
> are kept either in 24-bit or in 32-bit storage).
So VIRTIO_SND_PCM_FMT_U20_LE == VIRTIO_SND_PCM_FMT_U32_LE, except that
with U20 the sound hardware doesn't use the 12 least significant bits?
> Both Linux (ALSA) and Windows (WaveXXX interfaces) just expect from
> you to report support of these formats. You don't need to do anything
> special in order to implemented them. So, yes, we can support them in
> the prototype.
I guess I would have used the storage format and the number of
significant bits instead, but if sound APIs use different format
constants for these being consistent makes sense to me.
> > 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?
>
> Same answer: why not? :)
Same reply as above ;)
> >> /* 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?
>
> Basically, we were inspired how ALSA handles these and reused the same
> approach here. How: a hardware can have internal router/mixer to allow
> flexible configuration, there's nothing special.
I mean in the virtio api. Devices can report the mapping capability but
the driver can't pick one. Or did I miss something (if so then this is
an indication that the spec should be more clear on how channel mapping
is supposed to work ...) ?
> >> #### Prepare ... #### Start ...
> >
> > So I guess the way to start a stream is (a) prepare, (b) queue some
> > buffers, (c) start. Correct?
>
> Yeap!
Doesn't hurt to explicitly say so in the specs ;)
cheers,
Gerd
