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

[Gerd Hoffmann <kraxel@redhat.com>]

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?

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

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

cheers,
  Gerd