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

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

On 28.02.2020 12:14, Gerd Hoffmann wrote:
> On Thu, Feb 27, 2020 at 03:08:59PM +0100, Anton Yakovlev wrote:
> > Hello all,
> >
> > We have completed the implementation of the PoC specification v5. Below are
> > our comments on what might or should be improved in the specification.
> >
> > Initialization:
> >
> > 1. I think we should add the direction of the jack (in/out) to the
> > configuration of the jack. Because some properties (for example, a label for
> > some jack types) are based on this direction. The Linux HDA subsystem takes
> > this information from a pin/complex configuration. But we don't have (and
> > don't need) such entities.
>
> Sounds good.
>
> > 2. The stream configuration contains âfeaturesâ that partially work as normal
> > virtio features, and partially do not. Maybe it is better to rename the field
> > to âflagsâ (as well as in the set_params request) and enter two sets of flags:
> > one is reported in the stream configuration, and the other is used in the
> > set_params request? This should make things less confusing.
>
> Maybe have both features and flags fields?

Yes, that's a good idea.


> > 1. In the latest version of the specification, we resized the âfeaturesâ field
> > in the stream configuration (u8->le32), but forgot to do the same in the
> > set_params request. This should be fixed.
>
> Sure.
>
> > 2. The device may report polling support for message-based transport. In this
> > case, the driver can be optimized so that it does not kick virtqueue for each
> > message.
>
> Makes sense too, especially given that the packets should arrive at
> regular intervals and not (unlike for example network packets) at
> unpredictable times.

Then this will be an additional feature.


> > 3. For the input stream (capture), we decided to report the actual filled size
> > using the len field in the virtqueue descriptor. Should the specification
> > clearly indicate that the value contains sizeof(struct virtio_snd_pcm_status)
> > + the size of the recorded frames?
>
> Doesn't hurt to explicitly say so even though it should be clear that
> the descriptor size covers the complete payload and not only the
> recorded frames.
>
> > 4. We also need to add a device requirement to complete all pending messages
> > for a stream on a RELEASE request. Otherwise, they may become stuck in the
> > virtqueue.
>
> Yes, that makes sense.  Maybe also explicitly say that the RELEASE
> request should be completed last (only after all other pending messages
> are completed).

Makes sense, I will add this as well.


> 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

--
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin