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

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

Hi,

Sorry for the late reply, I was not available for a week.


On 25.11.2019 10:31, Gerd Hoffmann wrote:
>    Hi,
>
> > > Instead of PCM_OUTPUT and PCM_INPUT I would put the number of input and
> > > output streams into the device config space (and explicitly allow count
> > > being zero).
> >
> > There are several options for the number of streams:
> >
> > 1. The device can provide one input and/or output stream.
> > 2. The device can provide several input and/or output streams, and all of them
> > share the same capabilities.
> > 3. The device can provide several input and/or output streams, and they can
> > have different set of capabilities.
> >
> > Overall design depends on chosen option. Current draft is based on p.1. But we
> > have never discussed what is the best solution here.
>
> (3) looks most useful to me.

Then we need to refactor device configuration. I would propose to put into
configuration space only a total number of all available PCM streams and
introduce special control request to query per-stream configuration. A
response should contain a stream type (input/output) and all its capabilities.


> > > PCM_MSG -- I would drop the feature bit and make that mandatory, so we
> > > have a common baseline on which all drivers and devices can agree on.
> >
> > Then we need to inform device what "transport" will be in use (I assumed it
> > would be feature negotiation).
>
> Whenever other transports (i.e. via shared memory) are supported: yes,
> that should be a feature bit.
>
> Not sure about choosing the transport.  If both msg (i.e. via virtqueue)
> and shared memory are available, does it make sense to allow the driver
> choose the transport each time it starts a stream?

Shared memory based transport in any case will require some additional
actions. For HOST_MEM case the driver will need to get an access to host
buffer somehow. In GUEST_MEM case the driver will need to provide a buffer for
the host.

At first sight, we could extend the set_params request with the transport_type 
field and some additional information. For example, in case of GUEST_MEM the 
request could be followed by a buffer sg-list. This way it will work like you 
said: the driver chooses the transport each time it starts a stream.


> > > Right now this is defined for the pci transport only.
> >
> > And now we only declare a feature bit, right?
>
> Not fully sure what you mean here.
>
> Having the first revision define only the virtqueue transport, reserve a
> feature bit for the shared memory transport and hash out the details of
> that later is possible.

Yes, that's what I wanted to clarify.


> > > > Configuration space
> > > > -------------------
> > > >
> > > > We talked about PCM formats defined in ALSA. In Linux kernel v5.4-rc8 we have
> > > >
> > > > Do we really need all of them? And I skipped endianess, but should we care
> > > > about it as well?
> > >
> > > I'd keep the list as short as possible.  The formats listed in the patch
> > > looked like a good start.
> >
> > It was proposed to have all non-compressed formats.
> >
> > And... do we care about cross-endian cases?
>
> I'd start with little endian formats only.  virtio byte order is little
> endian, and the world is moving to little endian anyway.  We can add big
> endian formats later should this turn out to be needed, but I don't
> expect that to be the case.

Great!


> > > I'd suggest bytes (and rename the field to period_bytes).
>
> > > > struct virtio_snd_pcm_status {
> > > >       le32 status;
> > > >       le32 actual_length;
> > > > };
> > >
> > > Hmm.  Why actual_length?  Do we want allow short transfers?  Why?
> > > Wouldn't it be more useful to just fill the buffer completely?
> >
> > In current design we have no buffer size requirements. It means, that in
> > theory the device and the driver could have buffers with different sizes.
>
> Should we define that the (virtio) buffer size must be period_bytes then?

It will have no sense, since the driver chooses period_bytes at runtime. If we
gonna introduce any buffer constrains, it must be set by the device in a
stream configuration.


> > Also, the capture stream is a special case. Now we don't state explicitly
> > whether read request is blockable or not.
>
> The concept of "blockable" doesn't exist at that level.  The driver
> submits buffers to the device, the device fills them and notifies the
> driver when the buffer is full.  It simply doesn't work like a read(2)
> syscall.

But you described exactly "blockable" case: an I/O request is completed not
immediately but upon some condition (the buffer is full). In case of message-
based transport, both the device and the driver will have its own buffers. And
for capturing these buffers might be filled at different speed. For example,
in order to improve latency, the device could complete requests immediately
and fill in buffers with whatever it has at the moment.


> Buffers not being filled completely could possibly happen when stopping
> a stream with a half-filled buffer, if we want allow that in the spec.
>
> Another possible case would be compressed streams with variable bitrate,
> where the number of bytes needed for a millisecond of sound data isn't
> fixed.

Compressed streams are not part of PCM framework anyway.


> 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

www.opensynergy.com

Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
GeschÃftsfÃhrer/Managing Director: Regis Adjamah