[virtio-dev] virtio-snd comments/questions

[virtio-dev] virtio-snd comments/questions

[Radu Ocica]

[-- Attachment #1: Type: text/plain, Size: 19796 bytes --]

Hi,
I am working on implementing a virtio-snd vdev. During my work on different aspects of the implementation I stumbled across the following issues with the virtio sound device specification (that is part of the virtio 1.3 draft). In some cases additional clarifications might be enough to address the issue, in others additional features might be required. Below is a list of the issues, with some suggestions on how the issues could be addressed.
PCM playback and capture
Unclear semantics of PCM_STOP/PCM_START operations
The virtio-snd specification is not clear on the expectations on how the PCM_STOP control request is to be handled by the vdev, in terms of whether to:

  *   drop the remaining buffered audio in the host pcm stream and return pending io messages to the guest driver
  *   drop the remaining buffered audio in the host pcm stream and keep pending io messages in order to be able to prime the host pcm stream in case PCM_STOP is followed by PCM_START; return pending io messages only when receiving PCM_RELEASE
  *   drain the remaining buffered audio in the host pcm stream and send a PCM_STOP response upon drain completion, return pending io messages to the guest driver as they are completed (before PCM_STOP response); drain capability might or might not be available for the host pcm stream
  *   start draining the remaining buffered audio in the host pcm stream and send a PCM_STOP response immediately, return pending io messages to the guest driver as they are completed (after PCM_STOP response); if receiving PCM_RELEASE before drain completion hold off the response until all pending io messages are completed and returned to the guest driver; drain capability might or might not be available for the host pcm stream
  *   pause the host pcm stream and maintain buffered audio / pending io messages as is; pause capability might or might not be available for the host pcm stream

If the thinking is that the vdev on the host can decide on how to implement PCM_STOP based on the host stream capabilities, the guest driver has no information about the choice made by the vdev. In our opinion the choice made by the vdev in implementing PCM_STOP dictates what the possible transitions after PCM_STOP can be (if PCM_STOP is implemented as pause, PCM_START and PCM_RELEASE are possible transitions, but if PCM_STOP is implemented as drain, the only valid transition is PCM_RELEASE and if PCM_STOP is implemented as drop and all pending io messages have been returned to the guest driver, again the only valid transition seems to be PCM_RELEASE), but the guest driver is the one issuing the next request and it might at the best be able to make an educated guess how the vdev implemented PCM_STOP based on whether all the pending io messages are returned or not to the guest driver. To make such a guess possible, more clarifications would be required in the spec to identify which of the scenarios listed above are considered by the spec.
Note 1:  The reference virtio-snd linux/android guest driver explicitly doesn't support the ALSA SNDRV_PCM_INFO_DRAIN_TRIGGER feature flag, which might indicate there was no intent to support drain semantics.
Note 2: The reference virtio-snd linux/android guest driver explicitly does support the ALSA SNDRV_PCM_INFO_PAUSE feature flag, which seems to indicate intent to support pause semantics, along with drop semantics.
Note 3: The Android Audio HAL and tinyalsa/tinyplay don't have any notion of drain or pause. At the end of a playback session there is a wait for previously buffered data to complete playing using the tinyalsa pcm_wait() function, and then a PCM_STOP request is issued after the buffered audio has completed playing. Drop semantics are assumed for PCM_STOP in this context.
Information on period alignment and max audio buffer size
No information about period alignment and min/max audio buffer/period size is currently available in struct virtio_snd_pcm_info. Ideally the period size used for io messages should match the period size used on the host. That will guarantee consistency in timing, precise accounting of available room to write more data or available data to read, across host and guest. A workaround is possible, where the device will fail a PCM_SET_PARAMETERS request that specifies an unsupported period alignment or audio buffer/period size and the guest is forced to use a period and buffer size that is known to be supported by the host device (this can be done in an Android guest, by hard coding the android audio HAL period size and the number of periods and thus effectively the audio buffer size that is used for the virtio sound devices in the guest; in android and linux guests it can be done also by setting the virtio_snd module parameters, to the effect of locking down the period and buffer size).
General Issues

  *   There is no statement regarding the endianness of PCM data exchanged via txq and rxq queues and the virtio sound PCM format definitions do not include endianness.
  *   There is currently no requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture). The linux/android guest driver currently enqueues an io message for playback immediately after calling snd_pcm_period_elapsed(). snd_pcm_period_elapsed() makes it possible for the guest client app to write new data, but upon the return of this function there is no guarantee that the guest client app has already written new data. If the vdev dequeues the io message immediately and copies the PCM data for that io request before the guest client app has actually written new data to it, it results in out-of-sequence/stale data being played back.
  *   There is no statement how the guest driver should react to a control request failing.

     *   For PCM control requests the spec details what the "possible transitions" are after each control request. It doesn't specify whether this applies only to successful control requests, or irrespective of the success of the control requests. For instance for PCM_START only PCM_STOP is listed as a valid transition. What happens if PCM_START fails though? Is PCM_STOP still a valid transition? Is a repeat of PCM_START valid, since the previous PCM_START failed? Is a PCM_RELEASE required in order for the guest driver to recover the io request descriptors that were sent to the vdev before PCM_START? Is a PCM_STOP required in this case, in order to be able to send PCM_RELEASE (because of valid sequences considerations)? It was seen that the reference linux/android guest driver ignores a PCM_START failure and still sends io requests after the failed PCM_START.
     *   Should a failed control request be retried with a maximum retry count? Here the danger is that retrying forever without any wait in between retries might result in a high CPU load. Should there be a dedicated status value that tells the guest not to retry a failed request (or to use a error handling sequence involving RELEASE and PREPARE)?

  *   There is no statement how the guest driver should react to an io message failing

     *   Should it stop sending io messages and just send PCM_STOP? Or retry sending io requests hoping that they will succeed? Should there be a maximum retry count? Here the danger is that retrying forever without any wait in between might result in a high CPU load.

  *   The spec seems to indicate that PCM_PREPARE after PCM_STOP or PCM_SET_PARAMS after PCM_STOP would be invalid, still the linux/android guest driver are sending control  requests in these sequences (seen when playing back from android UI and PCM_START handling is modified to fail for the sake of testing the behavior). Is the spec out of date? Is the reference linux/guest android driver not compliant to the spec? Are these sequences made possible by the fact that the PCM_START previous to PCM_STOP actually failed? If these are valid sequences, the virtio spec should be updated to show them as valid.
  *   The virtio spec mentions that for playback, between PCM_PREPARE and PCM_START the guest driver should send io messages to prime the host playback stream. There is no statement what shall be done wrt io messages, in the following situations:

     *   between PCM_STOP and PCM_START: this goes back to the semantics of PCM_STOP: if PCM_STOP had drop semantics, all outstanding io messages could be returned to the guest; if PCM_STOP had pause semantics, outstanding io messages could be kept on the device side and returned to the guest only upon the receipt of a PCM_RELEASE request from the guest; if PCM_STOP had drain semantics, the outstanding io messages could be returned to the guest as they are completed
     *   after a failed PCM_START: since PCM_START failed, all io messages sent by the guest after PCM_PREPARE could be returned to the guest; or this could be deferred to a PCM_STOP or PCM_RELEASE request

  *   There is no way for the vdev to notify the guest driver that there is an error condition that requires a new PCM_PREPARE in order to clear. The vdev could send the VIRTIO_SND_EVT_PCM_XRUN, which requires PCM_PREPARE to clear, but this would be abusing the spec, as VIRTIO_SND_EVT_PCM_XRUN is meant exclusively for XRUN errors.
  *   There is no way for the vdev to notify the guest driver that a stream is disconnected (e.g. host audio device has been removed - think USB audio device - or reinserted).


Recommendations:

  *   Add explicit statement about endianness of PCM data exchanged via txq and rxq queues.
  *   Add explicit requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture)
  *   Add explicit requirement for control and io message failures to be handled on the guest driver side. It would be expected that if PCM_START fails, for instance, the guest driver doesn't send any further io messages. For io messages, at the minimum the guest driver should use a count of successive failures and stop retrying. If the handling for a failed control request is retrying the same request, there should be a limited number of retries. Specific to PCM_START failure, it should be specified whether PCM_STOP and maybe PCM_RELEASE and PCM_PREPARE is required before attempting any PCM_START retry. The behavior could be specified as dependent on whether the vdev has returned the pending io messages when handling PCM_STOP (if all pending io messages returned at PCM_STOP, perform PCM_RELEASE and PCM_PREPARE before retrying).
  *   Add clarification for handling of oustanding io messages on the device side upon a failed PCM_START control request, or upon a PCM_STOP control request
  *   Update valid transitions for each PCM control request, if specification out of date (see above for control request sequences seen with the reference linux/android guest driver, that are currently not specified as valid in the virtio sound specification)
  *   Add explicit statement that the documented valid transitions for PCM control requests are for successful control requests and in the case of failed requests, the valid transitions are same as before the failed control request (if this statement is correct, if not clarify what the expectation is)
  *   It would be beneficial if the virtio-snd specification did not leave the interpretation of PCM_STOP semantics to the vdev (drop vs pause vs drain), without a clear contract between vdev and guest driver
If the spec intends to allow the host to use both pause/resume and drop/go (and maybe even drain/go) semantics for the START/STOP operations, then for clarity it should use a separate request (PCM_PAUSE) for pause and if drain semantics are to be included as well, then a separate request (PCM_DRAIN) for drain as well. PCM_START could be used to restart/resume in all cases. Ideally there should be the following optional stream features:

     *   VIRTIO_SND_PCM_F_PAUSE - if offered by the vdev it indicates support for pause-resume both in the vdev and the host pcm stream; new request VIRTIO_SND_R_PCM_PAUSE with valid transitions START or RELEASE; START would have a valid transition to PAUSE;
     *   VIRTIO_SND_PCM_F_DRAIN - if offered by the vdev it indicates support for drain both in the vdev and the host pcm stream, new request VIRTIO_SND_R_PCM_DRAIN with valid transitions RELEASE; START would have a valid transition to DRAIN;
     *   VIRTIO_SND_R_PCM_STOP would retain drop semantics and have possible transitions RELEASE;
     *   A failed VIRTIO_SND_R_PCM_START would have the only possible transition RELEASE;
     *   Pending io messages would only need to be returned to the guest driver when handling VIRTIO_SND_R_PCM_RELEASE
     *   For priming the host pcm stream, io messages would only need to be sent by the guest driver between VIRTIO_SND_R_PCM_PREPARE and VIRTIO_SND_R_PCM_START
     *   The above approach would highly simplify the spec in terms of documenting possible transitions and when to send io messages to prime the host pcm stream and when to return pending io messages to the guest driver, as opposed to overloading the use of PCM_STOP for drop/pause(/drain), when the behavior wrt io messages would highly depend on the host interpretation of PCM_STOP.

  *   It would be beneficial to add two new PCM notifications, via the following stream features:

     *   VIRTIO_SND_PCM_F_EVT_ERR - indicates support for new notification VIRTIO_SND_EVT_PCM_ERROR, that signals to the guest driver that en error occurred (other than structly underrun/overrun) while handling io messages, requiring a sequence of PCM_RELEASE, PCM_PREPARE, PCM_START to recover
     *   VIRTIO_SND_PCM_F_EVT_CONN_STATE - indicates support for new notifications VIRTIO_SND_EVT_PCM_DISCONNECTED and VIRTIO_SND_EVT_PCM_CONNECTED, to indicate to the guest driver pcm stream disconnected (e.g. USB device removed) and pcm stream connected (e.g. USB device re-inserted)

  *   It would be beneficial to include Information on period alignment and max buffer size in struct virtio_snd_pcm_info, available via optional feature flag, e.g. VIRTIO_SND_PCM_F_PERIOD_INFO and the additional info would be added at the end of struct struct virtio_snd_pcm_info, replacing the padding bytes and adding to the length of the struct - new le32 fields period_align_bytes, min_period_bytes, max_period_bytes, max_buffer_bytes, where min_period_bytes, max_period_bytes and max_buffer_bytes are all multiples of period_align_bytes. With this addition, the guest driver would have the information required to match the period/buffer size capabilities of the host pcm stream and advertise them to clients in the guest space, without the need to hard code these using the guest virtio_snd module parameters.
  *   Alternatively a new control request to query the refined PCM parameters could be added (e.g. PCM_GET_PARAMS, via PCM feature flag VIRTIO_SND_PCM_F_GET_PARAMS), to be used by the guest driver after it has successfully sent PCM_SET_PARAMS, in order to query the PCM parameters effectively used by the device.

Audio Controls
The original virtio-snd device specification as added to virtio 1.1 did not include audio control support, however the virtio 1.3 draft includes this.
Audio Control Roles
The roles describe standard uses for controls, like volume, mute and gain. There is also an undefined role definition that according to the spec must not be used.

  *   VIRTIO_SND_CTL_ROLE_UNDEFINED
  *   VIRTIO_SND_CTL_ROLE_VOLUME
  *   VIRTIO_SND_CTL_ROLE_MUTE
  *   VIRTIO_SND_CTL_ROLE_GAIN

Problems identified with the role definitions:

  *   Not all audio controls can be described as volume, mute or gain. The undefined role must be allowed for any control that is not for volume, mute or gain.
  *   What is the difference between VIRTIO_SND_CTL_ROLE_VOLUME and VIRTIO_SND_CTL_ROLE_GAIN, is the first supposed to be used for playback and the second for capture? Or is the first supposed to be used for digital volume and the second for analog gain? Is a distinction between volume and gain necessary?
  *   If certain data types are assumed for controls with role VIRTIO_SND_CTL_ROLE_VOLUME, VIRTIO_SND_CTL_ROLE_MUTE or VIRTIO_SND_CTL_ROLE_GAIN, this should be explicitly specified (e.g. BOOLEAN for VIRTIO_SND_CTL_ROLE_MUTE, INTEGER for VIRTIO_SND_CTL_ROLE_VOLUME and VIRTIO_SND_CTL_ROLE_GAIN?).

Mute control value meaning
The virtio spec is not clear whether a value of 1 for an audio control with role VIRTIO_SND_CTL_ROLE_MUTE means muted, or rather switched on (unmuted). Testing end-to-end with the reference virtio-snd linux/android guest driver it was inferred that 1 means switched on, i.e. unmuted. In our opinion this should be clarified within the spec.
TLV support
The TLV struct and possible type values are considered well known from ALSA.
There are only a few common/standard TLV type definitions (for specific meta data types, like db scale, db min/max plus a handful more or the container TLV type that allows packing multiple TLVs into one containing TLV); they should be defined in virtio-snd.h for the completeness of the specification. Otherwise a non ALSA based host that supports reporting metadata would need to redefine these to match the ALSA definitions
The audio controls extension proposal draft includes the definition of struct virtio_snd_ctl_tlv. It is removed however in the virtio_snd.h header file of the linux/android virtio-snd guest driver implementation for this extension - the guest driver just performs memcpy to/from the equivalent ALSA struct. For the completeness of the specification all data structures used for data exchanges within the virtio spec should be part of the public header file. This is particularly important when the host or guest don't use ALSA and don't have access to the ALSA TLV struct definition.
Other than for metadata of controls for volume/gain, in ALSA audio controls can use TLVs for representing the actual control data (such controls use type VIRTIO_SND_CTL_TYPE_BYTES and access VIRTIO_SND_CTL_ACCESS_TLV_READ, VIRTIO_SND_CTL_ACCESS_TLV_WRITE or a combination of VIRTIO_SND_CTL_ACCESS_TLV_READ and VIRTIO_SND_CTL_ACCESS_TLV_WRITE). Since this use seems to be supported by the reference linux/android virtio-snd guest driver, it should be documented in the spec.
virtio_snd_ctl_info struct alignment
Field "name" of the virtio_snd_ctl_info struct has a length of 44 bytes. With this field length, the alignment of the following value union field with __le64 data members depends on packing and/or alignment compiler directives that can differ between host and guest and thus result in a different alignment of "value". Klocwork detects this issue as a PORTING.STORAGE.STRUCT guideline violation for this struct, which can be fixed by modifying the length of field "name" to 48, or by adding a padding field of length 4 bytes between the "name" and "value" fields (e.g. __u8 padding[4]).
Looking forward to clarifications and/or responses.
Radu Ocica

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

[-- Attachment #2: Type: text/html, Size: 31954 bytes --]

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

Hello Radu, 

I CC Anton too. This email responds part of the questions.

On Wed, Oct 18, 2023 at 03:32:21PM +0000, Radu Ocica wrote:
> Hi,
> I am working on implementing a virtio-snd vdev. During my work on different aspects of the implementation I stumbled across the following issues with the virtio sound device specification (that is part of the virtio 1.3 draft). In some cases additional clarifications might be enough to address the issue, in others additional features might be required. Below is a list of the issues, with some suggestions on how the issues could be addressed.
> PCM playback and capture
> Unclear semantics of PCM_STOP/PCM_START operations
> The virtio-snd specification is not clear on the expectations on how the PCM_STOP control request is to be handled by the vdev, in terms of whether to:
> 
>   *   drop the remaining buffered audio in the host pcm stream and return pending io messages to the guest driver
>   *   drop the remaining buffered audio in the host pcm stream and keep pending io messages in order to be able to prime the host pcm stream in case PCM_STOP is followed by PCM_START; return pending io messages only when receiving PCM_RELEASE
>   *   drain the remaining buffered audio in the host pcm stream and send a PCM_STOP response upon drain completion, return pending io messages to the guest driver as they are completed (before PCM_STOP response); drain capability might or might not be available for the host pcm stream
>   *   start draining the remaining buffered audio in the host pcm stream and send a PCM_STOP response immediately, return pending io messages to the guest driver as they are completed (after PCM_STOP response); if receiving PCM_RELEASE before drain completion hold off the response until all pending io messages are completed and returned to the guest driver; drain capability might or might not be available for the host pcm stream
>   *   pause the host pcm stream and maintain buffered audio / pending io messages as is; pause capability might or might not be available for the host pcm stream
> 

I agree that the STOP command could be better explain it. In our
implementation[1], we implemented as your last point defines it, i.e.,
stop host's pcm stream and keep messages in tx/rx. The STOP control
command is marked as completed immediately. The virtio spec does not
indicate that STOP requires to mark pending io messages as completed
thus we do not do that. A note about this in the spec would help.

> If the thinking is that the vdev on the host can decide on how to implement PCM_STOP based on the host stream capabilities, the guest driver has no information about the choice made by the vdev. In our opinion the choice made by the vdev in implementing PCM_STOP dictates what the possible transitions after PCM_STOP can be (if PCM_STOP is implemented as pause, PCM_START and PCM_RELEASE are possible transitions, but if PCM_STOP is implemented as drain, the only valid transition is PCM_RELEASE and if PCM_STOP is implemented as drop and all pending io messages have been returned to the guest driver, again the only valid transition seems to be PCM_RELEASE), but the guest driver is the one issuing the next request and it might at the best be able to make an educated guess how the vdev implemented PCM_STOP based on whether all the pending io messages are returned or not to the guest driver. To make such a guess possible, more clarifications would be required in the spec to identify which of the scenarios listed above are considered by the spec.

Checking the Linux kernel driver, the STOP cmd is issued when getting
SNDRV_PCM_TRIGGER_PAUSE_PUSH. Documentation does not indicate when this
msg is sent though. The RELEASE cmd is issued only during
virtsnd_pcm_sync_stop(). In this sense, I would guess that STOP behaves
like a PAUSE whereas RELEASE behaves like a STOP and dropping all
pending IO messages. The spec left for the implementation if device
would still playback those buffer or just drop them. I think you may
find interesting the discussion we started at this thread[2] regarding
the RELEASE control cmd semantics.

> Note 1:  The reference virtio-snd linux/android guest driver explicitly doesn't support the ALSA SNDRV_PCM_INFO_DRAIN_TRIGGER feature flag, which might indicate there was no intent to support drain semantics.
> Note 2: The reference virtio-snd linux/android guest driver explicitly does support the ALSA SNDRV_PCM_INFO_PAUSE feature flag, which seems to indicate intent to support pause semantics, along with drop semantics.
> Note 3: The Android Audio HAL and tinyalsa/tinyplay don't have any notion of drain or pause. At the end of a playback session there is a wait for previously buffered data to complete playing using the tinyalsa pcm_wait() function, and then a PCM_STOP request is issued after the buffered audio has completed playing. Drop semantics are assumed for PCM_STOP in this context.
> Information on period alignment and max audio buffer size
> No information about period alignment and min/max audio buffer/period size is currently available in struct virtio_snd_pcm_info. Ideally the period size used for io messages should match the period size used on the host. That will guarantee consistency in timing, precise accounting of available room to write more data or available data to read, across host and guest. A workaround is possible, where the device will fail a PCM_SET_PARAMETERS request that specifies an unsupported period alignment or audio buffer/period size and the guest is forced to use a period and buffer size that is known to be supported by the host device (this can be done in an Android guest, by hard coding the android audio HAL period size and the number of periods and thus effectively the audio buffer size that is used for the virtio sound devices in the guest; in android and linux guests it can be done also by setting the virtio_snd module parameters, to the effect of locking down the period and buffer size).
> General Issues
> 
>   *   There is no statement regarding the endianness of PCM data exchanged via txq and rxq queues and the virtio sound PCM format definitions do not include endianness.
>   *   There is currently no requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture). The linux/android guest driver currently enqueues an io message for playback immediately after calling snd_pcm_period_elapsed(). snd_pcm_period_elapsed() makes it possible for the guest client app to write new data, but upon the return of this function there is no guarantee that the guest client app has already written new data. If the vdev dequeues the io message immediately and copies the PCM data for that io request before the guest client app has actually written new data to it, it results in out-of-sequence/stale data being played back.

Right, we discussed about this issue at [3]. I've recently proposed the
following patch to fix that: '[PATCH v3] ALSA: virtio: use ack callback'

>   *   There is no statement how the guest driver should react to a control request failing.
> 
>      *   For PCM control requests the spec details what the "possible transitions" are after each control request. It doesn't specify whether this applies only to successful control requests, or irrespective of the success of the control requests. For instance for PCM_START only PCM_STOP is listed as a valid transition. What happens if PCM_START fails though? Is PCM_STOP still a valid transition? Is a repeat of PCM_START valid, since the previous PCM_START failed? Is a PCM_RELEASE required in order for the guest driver to recover the io request descriptors that were sent to the vdev before PCM_START? Is a PCM_STOP required in this case, in order to be able to send PCM_RELEASE (because of valid sequences considerations)? It was seen that the reference linux/android guest driver ignores a PCM_START failure and still sends io requests after the failed PCM_START.

We've also spotted that the spec does not specify what to do with
invalid transitions. I think in those cases the transition cannot be
taken and thus spec only refers to successful transitions. IIUC, in
Crosvm, for example, if a cmd is executed in a invalid state, the device
returns error to the guest but the transition does not happen. 

I will send a second email to address the remainder points.

Thanks, Matias.

[1] https://github.com/rust-vmm/vhost-device/tree/main/staging/vhost-device-sound
[2] https://lore.kernel.org/all/1a54feab-5de9-4b39-a4ce-7ff22e23cf52@opensynergy.com/T/
[3] https://lore.kernel.org/all/f28d4604-b169-4583-b9ab-d53e6d08ce63@opensynergy.com/T/.


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

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

On Wed, Oct 18, 2023 at 03:32:21PM +0000, Radu Ocica wrote:
> Hi,
> I am working on implementing a virtio-snd vdev. During my work on different aspects of the implementation I stumbled across the following issues with the virtio sound device specification (that is part of the virtio 1.3 draft). In some cases additional clarifications might be enough to address the issue, in others additional features might be required. Below is a list of the issues, with some suggestions on how the issues could be addressed.
> PCM playback and capture
> Unclear semantics of PCM_STOP/PCM_START operations
> The virtio-snd specification is not clear on the expectations on how the PCM_STOP control request is to be handled by the vdev, in terms of whether to:
> 
>   *   drop the remaining buffered audio in the host pcm stream and return pending io messages to the guest driver
>   *   drop the remaining buffered audio in the host pcm stream and keep pending io messages in order to be able to prime the host pcm stream in case PCM_STOP is followed by PCM_START; return pending io messages only when receiving PCM_RELEASE
>   *   drain the remaining buffered audio in the host pcm stream and send a PCM_STOP response upon drain completion, return pending io messages to the guest driver as they are completed (before PCM_STOP response); drain capability might or might not be available for the host pcm stream
>   *   start draining the remaining buffered audio in the host pcm stream and send a PCM_STOP response immediately, return pending io messages to the guest driver as they are completed (after PCM_STOP response); if receiving PCM_RELEASE before drain completion hold off the response until all pending io messages are completed and returned to the guest driver; drain capability might or might not be available for the host pcm stream
>   *   pause the host pcm stream and maintain buffered audio / pending io messages as is; pause capability might or might not be available for the host pcm stream
> 
> If the thinking is that the vdev on the host can decide on how to implement PCM_STOP based on the host stream capabilities, the guest driver has no information about the choice made by the vdev. In our opinion the choice made by the vdev in implementing PCM_STOP dictates what the possible transitions after PCM_STOP can be (if PCM_STOP is implemented as pause, PCM_START and PCM_RELEASE are possible transitions, but if PCM_STOP is implemented as drain, the only valid transition is PCM_RELEASE and if PCM_STOP is implemented as drop and all pending io messages have been returned to the guest driver, again the only valid transition seems to be PCM_RELEASE), but the guest driver is the one issuing the next request and it might at the best be able to make an educated guess how the vdev implemented PCM_STOP based on whether all the pending io messages are returned or not to the guest driver. To make such a guess possible, more clarifications would be required in the spec to identify which of the scenarios listed above are considered by the spec.
> Note 1:  The reference virtio-snd linux/android guest driver explicitly doesn't support the ALSA SNDRV_PCM_INFO_DRAIN_TRIGGER feature flag, which might indicate there was no intent to support drain semantics.
> Note 2: The reference virtio-snd linux/android guest driver explicitly does support the ALSA SNDRV_PCM_INFO_PAUSE feature flag, which seems to indicate intent to support pause semantics, along with drop semantics.
> Note 3: The Android Audio HAL and tinyalsa/tinyplay don't have any notion of drain or pause. At the end of a playback session there is a wait for previously buffered data to complete playing using the tinyalsa pcm_wait() function, and then a PCM_STOP request is issued after the buffered audio has completed playing. Drop semantics are assumed for PCM_STOP in this context.
> Information on period alignment and max audio buffer size
> No information about period alignment and min/max audio buffer/period size is currently available in struct virtio_snd_pcm_info. Ideally the period size used for io messages should match the period size used on the host. That will guarantee consistency in timing, precise accounting of available room to write more data or available data to read, across host and guest. A workaround is possible, where the device will fail a PCM_SET_PARAMETERS request that specifies an unsupported period alignment or audio buffer/period size and the guest is forced to use a period and buffer size that is known to be supported by the host device (this can be done in an Android guest, by hard coding the android audio HAL period size and the number of periods and thus effectively the audio buffer size that is used for the virtio sound devices in the guest; in android and linux guests it can be done also by setting the virtio_snd module parameters, to the effect of locking down the period and buffer size).
> General Issues
> 
>   *   There is no statement regarding the endianness of PCM data exchanged via txq and rxq queues and the virtio sound PCM format definitions do not include endianness.
>   *   There is currently no requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture). The linux/android guest driver currently enqueues an io message for playback immediately after calling snd_pcm_period_elapsed(). snd_pcm_period_elapsed() makes it possible for the guest client app to write new data, but upon the return of this function there is no guarantee that the guest client app has already written new data. If the vdev dequeues the io message immediately and copies the PCM data for that io request before the guest client app has actually written new data to it, it results in out-of-sequence/stale data being played back.
>   *   There is no statement how the guest driver should react to a control request failing.
> 
>      *   For PCM control requests the spec details what the "possible transitions" are after each control request. It doesn't specify whether this applies only to successful control requests, or irrespective of the success of the control requests. For instance for PCM_START only PCM_STOP is listed as a valid transition. What happens if PCM_START fails though? Is PCM_STOP still a valid transition? Is a repeat of PCM_START valid, since the previous PCM_START failed? Is a PCM_RELEASE required in order for the guest driver to recover the io request descriptors that were sent to the vdev before PCM_START? Is a PCM_STOP required in this case, in order to be able to send PCM_RELEASE (because of valid sequences considerations)? It was seen that the reference linux/android guest driver ignores a PCM_START failure and still sends io requests after the failed PCM_START.

I tested this and I observed that when START fails, the guest still
sends the STOP cmd. This cmd is only valid if the stream has transited
to the START state, i.e., the spec only allows STOP in the START state.
I did not obser that the RELEASE cmd is issued. I think the driver shall
not send the STOP msg in that situation. What do you think?

Regards, Matias.


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

Re: [virtio-dev] virtio-snd comments/questions

[Radu Ocica]

[-- Attachment #1: Type: text/plain, Size: 9086 bytes --]

I also think that the driver should not send a STOP command after a failed START command. There is no explicit description of any states in the virtio sound device specification. We have both adopted an interpretation that the state is determined by the last successful pcm control request and the transitions specified for pcm control requests assume success of these control requests. I wish this was clarified explicitly in the spec, to not allow room for interpretation.


Regards,

Radu

________________________________
From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> on behalf of Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
Sent: Tuesday, October 24, 2023 9:30:41 AM
To: Radu Ocica
Cc: virtio-dev@lists.oasis-open.org; virtio-comment@lists.oasis-open.org; anton.yakovlev@opensynergy.com
Subject: Re: [virtio-dev] virtio-snd comments/questions

CAUTION - This email is from an external source. Please be cautious with links and attachments. (go/taginfo)

On Wed, Oct 18, 2023 at 03:32:21PM +0000, Radu Ocica wrote:
> Hi,
> I am working on implementing a virtio-snd vdev. During my work on different aspects of the implementation I stumbled across the following issues with the virtio sound device specification (that is part of the virtio 1.3 draft). In some cases additional clarifications might be enough to address the issue, in others additional features might be required. Below is a list of the issues, with some suggestions on how the issues could be addressed.
> PCM playback and capture
> Unclear semantics of PCM_STOP/PCM_START operations
> The virtio-snd specification is not clear on the expectations on how the PCM_STOP control request is to be handled by the vdev, in terms of whether to:
>
>   *   drop the remaining buffered audio in the host pcm stream and return pending io messages to the guest driver
>   *   drop the remaining buffered audio in the host pcm stream and keep pending io messages in order to be able to prime the host pcm stream in case PCM_STOP is followed by PCM_START; return pending io messages only when receiving PCM_RELEASE
>   *   drain the remaining buffered audio in the host pcm stream and send a PCM_STOP response upon drain completion, return pending io messages to the guest driver as they are completed (before PCM_STOP response); drain capability might or might not be available for the host pcm stream
>   *   start draining the remaining buffered audio in the host pcm stream and send a PCM_STOP response immediately, return pending io messages to the guest driver as they are completed (after PCM_STOP response); if receiving PCM_RELEASE before drain completion hold off the response until all pending io messages are completed and returned to the guest driver; drain capability might or might not be available for the host pcm stream
>   *   pause the host pcm stream and maintain buffered audio / pending io messages as is; pause capability might or might not be available for the host pcm stream
>
> If the thinking is that the vdev on the host can decide on how to implement PCM_STOP based on the host stream capabilities, the guest driver has no information about the choice made by the vdev. In our opinion the choice made by the vdev in implementing PCM_STOP dictates what the possible transitions after PCM_STOP can be (if PCM_STOP is implemented as pause, PCM_START and PCM_RELEASE are possible transitions, but if PCM_STOP is implemented as drain, the only valid transition is PCM_RELEASE and if PCM_STOP is implemented as drop and all pending io messages have been returned to the guest driver, again the only valid transition seems to be PCM_RELEASE), but the guest driver is the one issuing the next request and it might at the best be able to make an educated guess how the vdev implemented PCM_STOP based on whether all the pending io messages are returned or not to the guest driver. To make such a guess possible, more clarifications would be required in the spec to identify which of the scenarios listed above are considered by the spec.
> Note 1:  The reference virtio-snd linux/android guest driver explicitly doesn't support the ALSA SNDRV_PCM_INFO_DRAIN_TRIGGER feature flag, which might indicate there was no intent to support drain semantics.
> Note 2: The reference virtio-snd linux/android guest driver explicitly does support the ALSA SNDRV_PCM_INFO_PAUSE feature flag, which seems to indicate intent to support pause semantics, along with drop semantics.
> Note 3: The Android Audio HAL and tinyalsa/tinyplay don't have any notion of drain or pause. At the end of a playback session there is a wait for previously buffered data to complete playing using the tinyalsa pcm_wait() function, and then a PCM_STOP request is issued after the buffered audio has completed playing. Drop semantics are assumed for PCM_STOP in this context.
> Information on period alignment and max audio buffer size
> No information about period alignment and min/max audio buffer/period size is currently available in struct virtio_snd_pcm_info. Ideally the period size used for io messages should match the period size used on the host. That will guarantee consistency in timing, precise accounting of available room to write more data or available data to read, across host and guest. A workaround is possible, where the device will fail a PCM_SET_PARAMETERS request that specifies an unsupported period alignment or audio buffer/period size and the guest is forced to use a period and buffer size that is known to be supported by the host device (this can be done in an Android guest, by hard coding the android audio HAL period size and the number of periods and thus effectively the audio buffer size that is used for the virtio sound devices in the guest; in android and linux guests it can be done also by setting the virtio_snd module parameters, to the effect of locking down the period and buffer size).
> General Issues
>
>   *   There is no statement regarding the endianness of PCM data exchanged via txq and rxq queues and the virtio sound PCM format definitions do not include endianness.
>   *   There is currently no requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture). The linux/android guest driver currently enqueues an io message for playback immediately after calling snd_pcm_period_elapsed(). snd_pcm_period_elapsed() makes it possible for the guest client app to write new data, but upon the return of this function there is no guarantee that the guest client app has already written new data. If the vdev dequeues the io message immediately and copies the PCM data for that io request before the guest client app has actually written new data to it, it results in out-of-sequence/stale data being played back.
>   *   There is no statement how the guest driver should react to a control request failing.
>
>      *   For PCM control requests the spec details what the "possible transitions" are after each control request. It doesn't specify whether this applies only to successful control requests, or irrespective of the success of the control requests. For instance for PCM_START only PCM_STOP is listed as a valid transition. What happens if PCM_START fails though? Is PCM_STOP still a valid transition? Is a repeat of PCM_START valid, since the previous PCM_START failed? Is a PCM_RELEASE required in order for the guest driver to recover the io request descriptors that were sent to the vdev before PCM_START? Is a PCM_STOP required in this case, in order to be able to send PCM_RELEASE (because of valid sequences considerations)? It was seen that the reference linux/android guest driver ignores a PCM_START failure and still sends io requests after the failed PCM_START.

I tested this and I observed that when START fails, the guest still
sends the STOP cmd. This cmd is only valid if the stream has transited
to the START state, i.e., the spec only allows STOP in the START state.
I did not obser that the RELEASE cmd is issued. I think the driver shall
not send the STOP msg in that situation. What do you think?

Regards, Matias.


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

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

[-- Attachment #2: Type: text/html, Size: 10493 bytes --]

Re: [virtio-dev] virtio-snd comments/questions

[Radu Ocica]

[-- Attachment #1: Type: text/plain, Size: 13934 bytes --]

Hi Matias,


I will answer to your comments:


> I agree that the STOP command could be better explain it. In our
implementation[1], we implemented as your last point defines it, i.e.,
stop host's pcm stream and keep messages in tx/rx. The STOP control
command is marked as completed immediately. The virtio spec does not
indicate that STOP requires to mark pending io messages as completed
thus we do not do that. A note about this in the spec would help.


In our implementation we made the assumption that STOP is meant to be implemented as pause, thus if the host stream supports the pause operation we pause it and keep io messages in tx/rx queue; the host stream is stopped (via the drop operation) and io messages are marked in this case as completed and returned to the guest driver only when receiving the RELEASE control request.

However, if the host stream does not support pause the only option left is to perform a drop operation on the host stream and in this case all pending messages in the tx/rx queue need to be returned to the guest as completed, because at the next start possibly all messages will be needed to prime the host stream (in the case of playback). A host stream prepare will also be needed to be issued internally by the device if a START is issued by the guest right after the STOP. And there is some uncertainty whether a guest driver will send new io messages before or after the START control request.

I think the difficulty of describing a clear behavior for STOP stems from attempting a minimal spec, with a minimal number of control requests. If dedicated STOP vs PAUSE requests were used, it would be more straightforward to specify an unambiguous behavior for each.


> Checking the Linux kernel driver, the STOP cmd is issued when getting
SNDRV_PCM_TRIGGER_PAUSE_PUSH. Documentation does not indicate when this
msg is sent though. The RELEASE cmd is issued only during
virtsnd_pcm_sync_stop(). In this sense, I would guess that STOP behaves
like a PAUSE whereas RELEASE behaves like a STOP and dropping all
pending IO messages. The spec left for the implementation if device
would still playback those buffer or just drop them. I think you may
find interesting the discussion we started at this thread[2] regarding
the RELEASE control cmd semantics.


We have made the call to never have the device drain the host pcm stream. If the host pcm stream supports pause, then STOP pauses it and RELEASE performs a drop and returns immediately all io messages as completed to the guest driver. If the host pcm stream does not support pause, then STOP results in a drop operation and return of all io messages as completed to the guest driver, and the RELEASE is a no-op. A drain like behavior for playback can be obtained if the guest driver stops writing new data and waits to underrun, time at which it sends STOP and RELEASE. This seems to work fine with the existing reference linux/android guest driver for success path handling. There are questions regarding failure path handling (failure of control or io requests) and what variability can/should be expected in terms of guest driver behavior.


> Right, we discussed about this issue at [3]. I've recently proposed the
following patch to fix that: '[PATCH v3] ALSA: virtio: use ack callback'


This looks promising. I found it already at https://github.com/torvalds/linux/blob/master/sound/virtio/ and will give it a try. Is there a repository for this driver code where both this fix and the audio control support is available?


Thank you for your comments and the very valuable information on the ack callback patch.


Regards,

Radu

________________________________
From: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
Sent: Tuesday, October 24, 2023 6:18:17 AM
To: Radu Ocica
Cc: virtio-comment@lists.oasis-open.org; virtio-dev@lists.oasis-open.org; anton.yakovlev@opensynergy.com
Subject: Re: [virtio-dev] virtio-snd comments/questions

CAUTION - This email is from an external source. Please be cautious with links and attachments. (go/taginfo)

Hello Radu,

I CC Anton too. This email responds part of the questions.

On Wed, Oct 18, 2023 at 03:32:21PM +0000, Radu Ocica wrote:
> Hi,
> I am working on implementing a virtio-snd vdev. During my work on different aspects of the implementation I stumbled across the following issues with the virtio sound device specification (that is part of the virtio 1.3 draft). In some cases additional clarifications might be enough to address the issue, in others additional features might be required. Below is a list of the issues, with some suggestions on how the issues could be addressed.
> PCM playback and capture
> Unclear semantics of PCM_STOP/PCM_START operations
> The virtio-snd specification is not clear on the expectations on how the PCM_STOP control request is to be handled by the vdev, in terms of whether to:
>
>   *   drop the remaining buffered audio in the host pcm stream and return pending io messages to the guest driver
>   *   drop the remaining buffered audio in the host pcm stream and keep pending io messages in order to be able to prime the host pcm stream in case PCM_STOP is followed by PCM_START; return pending io messages only when receiving PCM_RELEASE
>   *   drain the remaining buffered audio in the host pcm stream and send a PCM_STOP response upon drain completion, return pending io messages to the guest driver as they are completed (before PCM_STOP response); drain capability might or might not be available for the host pcm stream
>   *   start draining the remaining buffered audio in the host pcm stream and send a PCM_STOP response immediately, return pending io messages to the guest driver as they are completed (after PCM_STOP response); if receiving PCM_RELEASE before drain completion hold off the response until all pending io messages are completed and returned to the guest driver; drain capability might or might not be available for the host pcm stream
>   *   pause the host pcm stream and maintain buffered audio / pending io messages as is; pause capability might or might not be available for the host pcm stream
>

I agree that the STOP command could be better explain it. In our
implementation[1], we implemented as your last point defines it, i.e.,
stop host's pcm stream and keep messages in tx/rx. The STOP control
command is marked as completed immediately. The virtio spec does not
indicate that STOP requires to mark pending io messages as completed
thus we do not do that. A note about this in the spec would help.

> If the thinking is that the vdev on the host can decide on how to implement PCM_STOP based on the host stream capabilities, the guest driver has no information about the choice made by the vdev. In our opinion the choice made by the vdev in implementing PCM_STOP dictates what the possible transitions after PCM_STOP can be (if PCM_STOP is implemented as pause, PCM_START and PCM_RELEASE are possible transitions, but if PCM_STOP is implemented as drain, the only valid transition is PCM_RELEASE and if PCM_STOP is implemented as drop and all pending io messages have been returned to the guest driver, again the only valid transition seems to be PCM_RELEASE), but the guest driver is the one issuing the next request and it might at the best be able to make an educated guess how the vdev implemented PCM_STOP based on whether all the pending io messages are returned or not to the guest driver. To make such a guess possible, more clarifications would be required in the spec to identify which of the scenarios listed above are considered by the spec.

Checking the Linux kernel driver, the STOP cmd is issued when getting
SNDRV_PCM_TRIGGER_PAUSE_PUSH. Documentation does not indicate when this
msg is sent though. The RELEASE cmd is issued only during
virtsnd_pcm_sync_stop(). In this sense, I would guess that STOP behaves
like a PAUSE whereas RELEASE behaves like a STOP and dropping all
pending IO messages. The spec left for the implementation if device
would still playback those buffer or just drop them. I think you may
find interesting the discussion we started at this thread[2] regarding
the RELEASE control cmd semantics.

> Note 1:  The reference virtio-snd linux/android guest driver explicitly doesn't support the ALSA SNDRV_PCM_INFO_DRAIN_TRIGGER feature flag, which might indicate there was no intent to support drain semantics.
> Note 2: The reference virtio-snd linux/android guest driver explicitly does support the ALSA SNDRV_PCM_INFO_PAUSE feature flag, which seems to indicate intent to support pause semantics, along with drop semantics.
> Note 3: The Android Audio HAL and tinyalsa/tinyplay don't have any notion of drain or pause. At the end of a playback session there is a wait for previously buffered data to complete playing using the tinyalsa pcm_wait() function, and then a PCM_STOP request is issued after the buffered audio has completed playing. Drop semantics are assumed for PCM_STOP in this context.
> Information on period alignment and max audio buffer size
> No information about period alignment and min/max audio buffer/period size is currently available in struct virtio_snd_pcm_info. Ideally the period size used for io messages should match the period size used on the host. That will guarantee consistency in timing, precise accounting of available room to write more data or available data to read, across host and guest. A workaround is possible, where the device will fail a PCM_SET_PARAMETERS request that specifies an unsupported period alignment or audio buffer/period size and the guest is forced to use a period and buffer size that is known to be supported by the host device (this can be done in an Android guest, by hard coding the android audio HAL period size and the number of periods and thus effectively the audio buffer size that is used for the virtio sound devices in the guest; in android and linux guests it can be done also by setting the virtio_snd module parameters, to the effect of locking down the period and buffer size).
> General Issues
>
>   *   There is no statement regarding the endianness of PCM data exchanged via txq and rxq queues and the virtio sound PCM format definitions do not include endianness.
>   *   There is currently no requirement for the guest driver to enqueue an io message only after it has completed writing data to it (playback) or reading data from it (capture). The linux/android guest driver currently enqueues an io message for playback immediately after calling snd_pcm_period_elapsed(). snd_pcm_period_elapsed() makes it possible for the guest client app to write new data, but upon the return of this function there is no guarantee that the guest client app has already written new data. If the vdev dequeues the io message immediately and copies the PCM data for that io request before the guest client app has actually written new data to it, it results in out-of-sequence/stale data being played back.

Right, we discussed about this issue at [3]. I've recently proposed the
following patch to fix that: '[PATCH v3] ALSA: virtio: use ack callback'

>   *   There is no statement how the guest driver should react to a control request failing.
>
>      *   For PCM control requests the spec details what the "possible transitions" are after each control request. It doesn't specify whether this applies only to successful control requests, or irrespective of the success of the control requests. For instance for PCM_START only PCM_STOP is listed as a valid transition. What happens if PCM_START fails though? Is PCM_STOP still a valid transition? Is a repeat of PCM_START valid, since the previous PCM_START failed? Is a PCM_RELEASE required in order for the guest driver to recover the io request descriptors that were sent to the vdev before PCM_START? Is a PCM_STOP required in this case, in order to be able to send PCM_RELEASE (because of valid sequences considerations)? It was seen that the reference linux/android guest driver ignores a PCM_START failure and still sends io requests after the failed PCM_START.

We've also spotted that the spec does not specify what to do with
invalid transitions. I think in those cases the transition cannot be
taken and thus spec only refers to successful transitions. IIUC, in
Crosvm, for example, if a cmd is executed in a invalid state, the device
returns error to the guest but the transition does not happen.

I will send a second email to address the remainder points.

Thanks, Matias.

[1] https://urldefense.com/v3/__https://github.com/rust-vmm/vhost-device/tree/main/staging/vhost-device-sound__;!!JoeW-IhCUkS0Jg!bYrKkBHywrzvbKqKlnT9sd2r8yt3hdxcPMpipjtExqsI3lIusGdbwOM5n1sT_BE1-GB4xdvBalxdvUzFVHM$
[2] https://urldefense.com/v3/__https://lore.kernel.org/all/1a54feab-5de9-4b39-a4ce-7ff22e23cf52@opensynergy.com/T/__;!!JoeW-IhCUkS0Jg!bYrKkBHywrzvbKqKlnT9sd2r8yt3hdxcPMpipjtExqsI3lIusGdbwOM5n1sT_BE1-GB4xdvBalxdg5pM9y4$
[3] https://urldefense.com/v3/__https://lore.kernel.org/all/f28d4604-b169-4583-b9ab-d53e6d08ce63@opensynergy.com/T/__;!!JoeW-IhCUkS0Jg!bYrKkBHywrzvbKqKlnT9sd2r8yt3hdxcPMpipjtExqsI3lIusGdbwOM5n1sT_BE1-GB4xdvBalxdHbWqCWs$ .

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

[-- Attachment #2: Type: text/html, Size: 17731 bytes --]

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

Hello,

On Fri, Nov 03, 2023 at 07:05:30PM +0000, Radu Ocica wrote:
> Hi Matias,
>
>
> I will answer to your comments:
>
>
> > I agree that the STOP command could be better explain it. In our
> implementation[1], we implemented as your last point defines it, i.e.,
> stop host's pcm stream and keep messages in tx/rx. The STOP control
> command is marked as completed immediately. The virtio spec does not
> indicate that STOP requires to mark pending io messages as completed
> thus we do not do that. A note about this in the spec would help.
>
>
> In our implementation we made the assumption that STOP is meant to be implemented as pause, thus if the host stream supports the pause operation we pause it and keep io messages in tx/rx queue; the host stream is stopped (via the drop operation) and io messages are marked in this case as completed and returned to the guest driver only when receiving the RELEASE control request.
>
> However, if the host stream does not support pause the only option left is to perform a drop operation on the host stream and in this case all pending messages in the tx/rx queue need to be returned to the guest as completed, because at the next start possibly all messages will be needed to prime the host stream (in the case of playback). A host stream prepare will also be needed to be issued internally by the device if a START is issued by the guest right after the STOP. And there is some uncertainty whether a guest driver will send new io messages before or after the START control request.
>

Could you elaborate on this? Which audio backend would not support
PAUSE? I am not very familiar with audio engines.


> I think the difficulty of describing a clear behavior for STOP stems from attempting a minimal spec, with a minimal number of control requests. If dedicated STOP vs PAUSE requests were used, it would be more straightforward to specify an unambiguous behavior for each.
>
>
> > Checking the Linux kernel driver, the STOP cmd is issued when getting
> SNDRV_PCM_TRIGGER_PAUSE_PUSH. Documentation does not indicate when this
> msg is sent though. The RELEASE cmd is issued only during
> virtsnd_pcm_sync_stop(). In this sense, I would guess that STOP behaves
> like a PAUSE whereas RELEASE behaves like a STOP and dropping all
> pending IO messages. The spec left for the implementation if device
> would still playback those buffer or just drop them. I think you may
> find interesting the discussion we started at this thread[2] regarding
> the RELEASE control cmd semantics.
>
>
> We have made the call to never have the device drain the host pcm stream. If the host pcm stream supports pause, then STOP pauses it and RELEASE performs a drop and returns immediately all io messages as completed to the guest driver. If the host pcm stream does not support pause, then STOP results in a drop operation and return of all io messages as completed to the guest driver, and the RELEASE is a no-op. A drain like behavior for playback can be obtained if the guest driver stops writing new data and waits to underrun, time at which it sends STOP and RELEASE. This seems to work fine with the existing reference linux/android guest driver for success path handling. There are questions regarding failure path handling (failure of control or io requests) and what variability can/should be expected in terms of guest driver behavior.
>
>
> > Right, we discussed about this issue at [3]. I've recently proposed the
> following patch to fix that: '[PATCH v3] ALSA: virtio: use ack callback'
>
>
> This looks promising. I found it already at https://github.com/torvalds/linux/blob/master/sound/virtio/ and will give it a try. Is there a repository for this driver code where both this fix and the audio control support is available?
>
>

What do you mean with audio control support? Are you talking about the
device? The device implementation is at
https://github.com/rust-vmm/vhost-device/tree/main/staging.


Matias


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

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

On Fri, Nov 03, 2023 at 06:17:17PM +0000, Radu Ocica wrote:
> I also think that the driver should not send a STOP command after a failed START command. There is no explicit description of any states in the virtio sound device specification. We have both adopted an interpretation that the state is determined by the last successful pcm control request and the transitions specified for pcm control requests assume success of these control requests. I wish this was clarified explicitly in the spec, to not allow room for interpretation.
>
>
I agree. I think what is missed in the spec is to define the behavior of
the PCM command cycle in case a command fails. I worked on a RFC to
improve that section, feel free to comment at
https://lists.oasis-open.org/archives/virtio-comment/202307/msg00158.html.
I think an overall solution would be to implement the PCM state machine
in the driver thus control messages would be sent only if the stream is
in the correct state.

Matias


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

Re: [virtio-dev] virtio-snd comments/questions

[Radu Ocica]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]


> Could you elaborate on this? Which audio backend would not support
> PAUSE? I am not very familiar with audio engines.

For instance in linux, alsa-lib has a flag that specifies pause support by a PCM stream:
#define SNDRV_PCM_INFO_PAUSE                       (1UL<<19)  /* pause ioctl is supported */
The snd_pcm_pause API can only be used successfully if the PCM stream has this bit set in its PCM info.
A similar concept exists in QNX sound.



>> This looks promising. I found it already at https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/sound/virtio/__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw6FiWxDY$  and will give it a try. Is there a repository for this driver code where both this fix and the audio control support is available?
>>
>>

> What do you mean with audio control support? Are you talking about the
> device? The device implementation is at
https://urldefense.com/v3/__https://github.com/rust-vmm/vhost-device/tree/main/staging__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw1sNq6IY$ .

I meant the support for feature VIRTIO_SND_F_CTLS in the virtio_snd guest driver. A patch to add this support is available here: https://lists.oasis-open.org/archives/virtio-comment/202104/msg00013.html. However this patch is not present in the virtio_snd guest driver version here: https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/sound/virtio/__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw6FiWxDY$ . I was wondering whether there is a repository where the virtio_snd driver has both the VIRTIO_SND_F_CTLS support and the ack callback patch.

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

[-- Attachment #2: Type: text/html, Size: 5077 bytes --]

Re: [virtio-dev] virtio-snd comments/questions

[Manos Pitsidianakis]

On Thu, 9 Nov 2023 at 18:46, Radu Ocica <rocica@blackberry.com> wrote:
> >> This looks promising. I found it already at https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/sound/virtio/__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw6FiWxDY$  and will give it a try. Is there a repository for this driver code where both this fix and the audio control support is available?

The patch will most likely be part of the next kernel release, 6.7. As
for the control support, read on:

> > What do you mean with audio control support? Are you talking about the
> > device? The device implementation is at
> https://urldefense.com/v3/__https://github.com/rust-vmm/vhost-device/tree/main/staging__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw1sNq6IY$ .
>
> I meant the support for feature VIRTIO_SND_F_CTLS in the virtio_snd guest driver. A patch to add this support is available here: https://lists.oasis-open.org/archives/virtio-comment/202104/msg00013.html. However this patch is not present in the virtio_snd guest driver version here: https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/sound/virtio/__;!!JoeW-IhCUkS0Jg!bBq1USPrJoN1_CBJkW-XBknkCSEOUP40V1ctr_OU0tsYyEr9pXjSEu7n5ibc5BL4fKT3Utk-cpGw6FiWxDY$ . I was wondering whether there is a repository where the virtio_snd driver has both the VIRTIO_SND_F_CTLS support and the ack callback patch.


The feature is part of the 1.3 version of the VIRTIO spec which is
still in a Committee Specification Draft phase. You can find that the
latest version of the VIRTIO spec is 1.2 here:
https://www.oasis-open.org/committees/tc_home.php

I believe it will take some time for both 1.3 to be approved and
afterwards patches for the kernel must be published and reviewed,
which will take some cycles as well. So I'd suggest you not to count
on control capabilities at the moment, unfortunately.

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

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

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

Hello, 

On Thu, Nov 09, 2023 at 04:46:45PM +0000, Radu Ocica wrote:
> 
> > Could you elaborate on this? Which audio backend would not support
> > PAUSE? I am not very familiar with audio engines.
> 
> For instance in linux, alsa-lib has a flag that specifies pause support by a PCM stream:
> #define SNDRV_PCM_INFO_PAUSE                       (1UL<<19)  /* pause ioctl is supported */
> The snd_pcm_pause API can only be used successfully if the PCM stream has this bit set in its PCM info.
> A similar concept exists in QNX sound.
> 

Oh, I see. I was unaware of this flag. Not supporting PAUSE in the host
sound card may not be a problem. In our case, the pw backend in the host
stops playing when a guest sends STOP immediately. As soon as START is
sent again, pw begins consuming from the tx queue. Tx buffers are only
signaled as complete after START is reissued. In the meantime, the msgs
are in the available ring of the tx queue.

> However, if the host stream does not support pause the only option
> left is to perform a drop operation on the host stream and in this
> case all pending messages in the tx/rx queue need to be returned to
> the guest as completed, because at the next start possibly all
> messages will be needed to prime the host stream (in the case of
> playback).

If STOP is sent and there is still msgs in the TX queue, can't the
device just stop playing until START is issued again? Why does the
device require to drop the msgs in this case?

Thanks, Matias.


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

Re: [virtio-dev] virtio-snd comments/questions

[Radu Ocica]

[-- Attachment #1: Type: text/plain, Size: 3761 bytes --]

Hi Matias,


> >
> > > Could you elaborate on this? Which audio backend would not support
> > > PAUSE? I am not very familiar with audio engines.
> >
> > For instance in linux, alsa-lib has a flag that specifies pause support by a PCM stream:
> > #define SNDRV_PCM_INFO_PAUSE                       (1UL<<19)  /* pause ioctl is supported */
> > The snd_pcm_pause API can only be used successfully if the PCM stream has this bit set in its PCM info.
> > A similar concept exists in QNX sound.
> >

> Oh, I see. I was unaware of this flag. Not supporting PAUSE in the host
> sound card may not be a problem. In our case, the pw backend in the host
> stops playing when a guest sends STOP immediately. As soon as START is
> sent again, pw begins consuming from the tx queue. Tx buffers are only
> signaled as complete after START is reissued. In the meantime, the msgs
> are in the available ring of the tx queue.

> > However, if the host stream does not support pause the only option
> > left is to perform a drop operation on the host stream and in this
> > case all pending messages in the tx/rx queue need to be returned to
> > the guest as completed, because at the next start possibly all
> > messages will be needed to prime the host stream (in the case of
> > playback).

> If STOP is sent and there is still msgs in the TX queue, can't the
> device just stop playing until START is issued again? Why does the
> device require to drop the msgs in this case?
Invoking the pcm drop API on the host has been the modality chosen by us to stop playing, with immediate effect. Whatever io messages were buffered at the time by the host pcm device need then to be returned to the guest driver as completed. Messages in the TX queue at the time of the STOP request can be left untouched. Depending on the relationship between number of periods used by the guest device vs number of periods used by the host device, the TX queue might be normally empty though (for instance if host and guest device use the same number of periods, all pcm data received from the guest driver would end up immediately in the backend buffer).
The way you describe STOP it sounds like you just stop writing new data to the host pcm device, without invoking any API to explicitly stop it (if pause API not available for the host pcm device). This allows for currently backend buffered data to continue playing and correspondingly io messages need to be returned to the guest driver as they are consumed. If no START is received before the backend buffered data completes playing, the host device then underruns. At this point it either continues running by injecting silence or stops, depending on setup options. The effective stop of sound playback occurs when the backend buffered data completes playing, which might be after a considerable delay, depending on backend buffer size. Keeping the host device running and injecting silence sounds like the easiest way to avoid the need to re-prepare the host pcm device when restarting. Have I captured your approach accurately?

Cheers,
Radu

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

[-- Attachment #2: Type: text/html, Size: 6250 bytes --]

Re: [virtio-dev] virtio-snd comments/questions

[Matias Ezequiel Vara Larsen]

On Wed, Nov 22, 2023 at 06:59:15PM +0000, Radu Ocica wrote:
> Hi Matias,
> 
> 
> > >
> > > > Could you elaborate on this? Which audio backend would not support
> > > > PAUSE? I am not very familiar with audio engines.
> > >
> > > For instance in linux, alsa-lib has a flag that specifies pause support by a PCM stream:
> > > #define SNDRV_PCM_INFO_PAUSE                       (1UL<<19)  /* pause ioctl is supported */
> > > The snd_pcm_pause API can only be used successfully if the PCM stream has this bit set in its PCM info.
> > > A similar concept exists in QNX sound.
> > >
> 
> > Oh, I see. I was unaware of this flag. Not supporting PAUSE in the host
> > sound card may not be a problem. In our case, the pw backend in the host
> > stops playing when a guest sends STOP immediately. As soon as START is
> > sent again, pw begins consuming from the tx queue. Tx buffers are only
> > signaled as complete after START is reissued. In the meantime, the msgs
> > are in the available ring of the tx queue.
> 
> > > However, if the host stream does not support pause the only option
> > > left is to perform a drop operation on the host stream and in this
> > > case all pending messages in the tx/rx queue need to be returned to
> > > the guest as completed, because at the next start possibly all
> > > messages will be needed to prime the host stream (in the case of
> > > playback).
> 
> > If STOP is sent and there is still msgs in the TX queue, can't the
> > device just stop playing until START is issued again? Why does the
> > device require to drop the msgs in this case?
> Invoking the pcm drop API on the host has been the modality chosen by us to stop playing, with immediate effect. Whatever io messages were buffered at the time by the host pcm device need then to be returned to the guest driver as completed. Messages in the TX queue at the time of the STOP request can be left untouched. Depending on the relationship between number of periods used by the guest device vs number of periods used by the host device, the TX queue might be normally empty though (for instance if host and guest device use the same number of periods, all pcm data received from the guest driver would end up immediately in the backend buffer).
> The way you describe STOP it sounds like you just stop writing new data to the host pcm device, without invoking any API to explicitly stop it (if pause API not available for the host pcm device). This allows for currently backend buffered data to continue playing and correspondingly io messages need to be returned to the guest driver as they are consumed. If no START is received before the backend buffered data completes playing, the host device then underruns. At this point it either continues running by injecting silence or stops, depending on setup options. The effective stop of sound playback occurs when the backend buffered data completes playing, which might be after a considerable delay, depending on backend buffer size. Keeping the host device running and injecting silence sounds like the easiest way to avoid the need to re-prepare the host pcm device when restarting. Have I captured your approach accurately?

For example, in the audio backend for pipewire, the STOP cmd stops
playing, i.e., the device stops to consume from the tx queue. This cmd
translates to something like `stream.set_active(false)`. In the pw
backend, the consumption happens from a thread named the threadloop.
When setting `set_active(false)`, such a thread stops to execute. There
is no need to insert silence from the audio backend. The threadloop
starts to execute again and thus consuming from tx when issuing
`set_active(true)` during the START cmd.

Matias


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