Re: [PATCH v2 6/9] ALSA: virtio: PCM substream operators

On 25.01.2021 17:59, Guennadi Liakhovetski wrote:

On Sun, 24 Jan 2021, Anton Yakovlev wrote:

[snip]

...

+/**

• • virtsnd_pcm_release() - Release the PCM substream on the device

side.

...

• • @substream: VirtIO substream.
• • Context: Any context that permits to sleep.
• • Return: 0 on success, -errno on failure.
• */

+static inline bool virtsnd_pcm_released(struct virtio_pcm_substream *substream) +{

• /*
  

•  * The spec states that upon receipt of the RELEASE command "the
  

device

•  * MUST complete all pending I/O messages for the specified
  

stream ID".

•  * Thus, we consider the absence of I/O messages in the queue 
  

as an

...

•  * indication that the substream has been released.
  

•  */
  

• return atomic_read(&substream->msg_count) == 0;
  

Also here having it atomic doesn't really seem to help. This just means, that at some point of time it was == 0.

Technically, you're right. In practice, everything looks like this:

I/O messages are added to the virtqueue either at the start of the substream or in the interrupt handler (and only as long as .xfer_enabled is true). In general, this means that the .msg_count can only be incremented in the interrupt handler. As soon as the substream stops, the .xfer_enabled becomes false and the .msg_count no longer increases. This means that the .msg_count was either already 0, or we need to wait for it to become 0.

...

+}

+static int virtsnd_pcm_release(struct virtio_pcm_substream *substream)

kernel-doc missing

Yeap, thanks!

...

+{

• struct virtio_snd *snd = substream->snd;
  

• struct virtio_snd_msg *msg;
  

• unsigned int js = msecs_to_jiffies(msg_timeout_ms);
  

• int rc;
  

• msg = virtsnd_pcm_ctl_msg_alloc(substream,
  

VIRTIO_SND_R_PCM_RELEASE,

•                                 GFP_KERNEL);
  

• if (IS_ERR(msg))
  

•         return PTR_ERR(msg);
  

• rc = virtsnd_ctl_msg_send_sync(snd, msg);
  

• if (rc)
  

•         return rc;
  

• return wait_event_interruptible_timeout(substream->msg_empty,
  

virtsnd_pcm_released(substream),

•                                         js);
  

wait_event_interruptible_timeout() will return a positive number in success cases, 0 means a timeout and condition still false. Whereas when you call this function you interpret 0 as success and you expect any != 0 to be a negative error. Wondering how this worked during your tests?

Yeah, that's actually a bug. We haven't hit a timeout on that control path.

...

+}

+/**

• • virtsnd_pcm_open() - Open the PCM substream.
• • @substream: Kernel ALSA substream.
• • Context: Any context.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_pcm_open(struct snd_pcm_substream *substream) +{

• struct virtio_pcm *pcm = snd_pcm_substream_chip(substream);
  

• struct virtio_pcm_substream *ss = NULL;
  

• if (pcm) {
  

•         switch (substream->stream) {
  

•         case SNDRV_PCM_STREAM_PLAYBACK:
  

•         case SNDRV_PCM_STREAM_CAPTURE: {
  

•                 struct virtio_pcm_stream *stream =
  

•                         &pcm->streams[substream->stream];
  

•                 if (substream->number < stream->nsubstreams)
  

Can this condition ever be false?

Hard to tell. But there may be some bug. In general, I try to adhere to the rule that if an array element is referenced by index, it is better to check the index value first.

...

•                         ss = 
  

stream->substreams[substream->number];

...

•                 break;
  

•         }
  

•         }
  

• }
  

• if (!ss)
  

•         return -EBADFD;
  

• substream->runtime->hw = ss->hw;
  

• substream->private_data = ss;
  

• return 0;
  

+}

+/**

• • virtsnd_pcm_close() - Close the PCM substream.
• • @substream: Kernel ALSA substream.
• • Context: Any context.
• • Return: 0.
• */

+static int virtsnd_pcm_close(struct snd_pcm_substream *substream) +{

• return 0;
  

+}

+/**

• • virtsnd_pcm_hw_params() - Set the parameters of the PCM substream.
• • @substream: Kernel ALSA substream.
• • @hw_params: Hardware parameters (can be NULL).
• • The function can be called both from the upper level (in this case,
• • @hw_params is not NULL) or from the driver itself (in this case,

@hw_params

• • is NULL, and the parameter values are taken from the runtime

structure).

• • In all cases, the function:
• • 1. checks the state of the virtqueue and, if necessary, tries to

fix it,

• • 2. sets the parameters on the device side,
• • 3. allocates a hardware buffer and I/O messages.
• • Context: Any context that permits to sleep.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,

•                          struct snd_pcm_hw_params *hw_params)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
  

• struct virtio_pcm_substream *ss =
  

snd_pcm_substream_chip(substream);

• struct virtio_device *vdev = ss->snd->vdev;
  

• struct virtio_snd_msg *msg;
  

• struct virtio_snd_pcm_set_params *request;
  

• snd_pcm_format_t format;
  

• unsigned int channels;
  

• unsigned int rate;
  

• unsigned int buffer_bytes;
  

• unsigned int period_bytes;
  

• unsigned int periods;
  

• unsigned int i;
  

• int vformat = -1;
  

• int vrate = -1;
  

• int rc;
  

• /*
  

•  * If we got here after ops->trigger() was called, the queue may
  

•  * still contain messages. In this case, we need to release the
  

•  * substream first.
  

•  */
  

• if (atomic_read(&ss->msg_count)) {
  

•         rc = virtsnd_pcm_release(ss);
  

•         if (rc) {
  

•                 dev_err(&vdev->dev,
  

•                         "SID %u: invalid I/O queue state\n",
  

•                         ss->sid);
  

•                 return rc;
  

•         }
  

• }
  

• /* Set hardware parameters in device */
  

• if (hw_params) {
  

•         format = params_format(hw_params);
  

•         channels = params_channels(hw_params);
  

•         rate = params_rate(hw_params);
  

•         buffer_bytes = params_buffer_bytes(hw_params);
  

•         period_bytes = params_period_bytes(hw_params);
  

•         periods = params_periods(hw_params);
  

• } else {
  

•         format = runtime->format;
  

•         channels = runtime->channels;
  

•         rate = runtime->rate;
  

•         buffer_bytes = frames_to_bytes(runtime,
  

runtime->buffer_size);

•         period_bytes = frames_to_bytes(runtime,
  

runtime->period_size);

•         periods = runtime->periods;
  

• }
  

• for (i = 0; i < ARRAY_SIZE(g_a2v_format_map); ++i)
  

•         if (g_a2v_format_map[i].alsa_bit == format) {
  

•                 vformat = g_a2v_format_map[i].vio_bit;
  

•                 break;
  

•         }
  

• for (i = 0; i < ARRAY_SIZE(g_a2v_rate_map); ++i)
  

•         if (g_a2v_rate_map[i].rate == rate) {
  

•                 vrate = g_a2v_rate_map[i].vio_bit;
  

•                 break;
  

•         }
  

• if (vformat == -1 || vrate == -1)
  

•         return -EINVAL;
  

• msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_SET_PARAMS,
  

•                                 GFP_KERNEL);
  

• if (IS_ERR(msg))
  

•         return PTR_ERR(msg);
  

• request = sg_virt(&msg->sg_request);
  

• request->buffer_bytes = cpu_to_virtio32(vdev, buffer_bytes);
  

• request->period_bytes = cpu_to_virtio32(vdev, period_bytes);
  

• request->channels = channels;
  

• request->format = vformat;
  

• request->rate = vrate;
  

I presume the latter three fields don't have to be endienness-converted, perhaps they're 8-bit wide only.

Yes, these three values are u8.

...

• if (ss->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING))
  

•         request->features |=
  

•                 cpu_to_virtio32(vdev,
  

•                                 1U <<
  

VIRTIO_SND_PCM_F_MSG_POLLING);

• if (ss->features & (1U << VIRTIO_SND_PCM_F_EVT_XRUNS))
  

•         request->features |=
  

•                 cpu_to_virtio32(vdev,
  

•                                 1U << VIRTIO_SND_PCM_F_EVT_XRUNS);
  

• rc = virtsnd_ctl_msg_send_sync(ss->snd, msg);
  

Wouldn't it be better to only try to send the message after below allocations completed successfully?

I thought the reverse logic was better. This message asks the device to set a specific set of parameters. And if the device returned an error for some reason, then there is no point in allocating memory.

...

• if (rc)
  

•         return rc;
  

• /* If the buffer was already allocated earlier, do nothing. */
  

• if (runtime->dma_area)
  

•         return 0;
  

• /* Allocate hardware buffer */
  

• rc = snd_pcm_lib_malloc_pages(substream, buffer_bytes);
  

• if (rc < 0)
  

•         return rc;
  

• /* Allocate and initialize I/O messages */
  

• rc = virtsnd_pcm_msg_alloc(ss, periods, runtime->dma_area,
  

•                            period_bytes);
  

• if (rc)
  

•         snd_pcm_lib_free_pages(substream);
  

• return rc;
  

+}

+/**

• • virtsnd_pcm_hw_free() - Reset the parameters of the PCM substream.
• • @substream: Kernel ALSA substream.
• • The function does the following:
• • 1. tries to release the PCM substream on the device side,
• • 2. frees the hardware buffer.
• • Context: Any context that permits to sleep.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) +{

• struct virtio_pcm_substream *ss =
  

snd_pcm_substream_chip(substream);

• int rc;
  

• rc = virtsnd_pcm_release(ss);
  

• /*
  

•  * Even if we failed to send the RELEASE message or wait for the
  

queue

•  * flush to complete, we can safely delete the buffer. Because
  

after

•  * receiving the STOP command, the device must stop all I/O 
  

message

...

•  * processing. If there are still pending messages in the queue,
  

the

•  * next ops->hw_params() call should deal with this.
  

•  */
  

• snd_pcm_lib_free_pages(substream);
  

• return rc;
  

+}

+/**

• • virtsnd_pcm_hw_params() - Prepare the PCM substream.

copy-paste: this is virtsnd_pcm_prepare()

Oops... :)

...

• • @substream: Kernel ALSA substream.
• • The function can be called both from the upper level or from the

driver

• • itself.
• • In all cases, the function:
• • 1. checks the state of the virtqueue and, if necessary, tries to

fix it,

• • 2. prepares the substream on the device side.
• • Context: Any context that permits to sleep. May take and release

the tx/rx

• •      queue spinlock.
    

• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream) +{

• struct virtio_pcm_substream *ss =
  

snd_pcm_substream_chip(substream);

• struct virtio_snd_queue *queue = virtsnd_pcm_queue(ss);
  

• struct virtio_snd_msg *msg;
  

• unsigned long flags;
  

• int rc;
  

• /*
  

•  * If we got here after ops->trigger() was called, the queue may
  

•  * still contain messages. In this case, we need to reset the
  

•  * substream first.
  

•  */
  

• if (atomic_read(&ss->msg_count)) {
  

•         rc = virtsnd_pcm_hw_params(substream, NULL);
  

•         if (rc)
  

•                 return rc;
  

• }
  

• spin_lock_irqsave(&queue->lock, flags);
  

• ss->msg_last_enqueued = -1;
  

• spin_unlock_irqrestore(&queue->lock, flags);
  

• /*
  

•  * Since I/O messages are asynchronous, they can be completed
  

•  * when the runtime structure no longer exists. Since each
  

•  * completion implies incrementing the hw_ptr, we cache all the
  

•  * current values needed to compute the new hw_ptr value.
  

•  */
  

• ss->frame_bytes = substream->runtime->frame_bits >> 3;
  

• ss->period_size = substream->runtime->period_size;
  

• ss->buffer_size = substream->runtime->buffer_size;
  

• atomic_set(&ss->hw_ptr, 0);
  

• atomic_set(&ss->xfer_xrun, 0);
  

• atomic_set(&ss->msg_count, 0);
  

• msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_PREPARE,
  

•                                 GFP_KERNEL);
  

• if (IS_ERR(msg))
  

•         return PTR_ERR(msg);
  

• return virtsnd_ctl_msg_send_sync(ss->snd, msg);
  

+}

+/**

• • virtsnd_pcm_trigger() - Process command for the PCM substream.
• • @substream: Kernel ALSA substream.
• • @command: Substream command (SNDRV_PCM_TRIGGER_XXX).
• • Depending on the command, the function does the following:
• • 1. enables/disables data transmission,
• • 2. starts/stops the substream on the device side.
• • Context: Atomic context. May take and release the tx/rx queue

spinlock.

Really? Cannot .trigger() sleep? E.g. I see mdelay(25) in snd_es18xx_playback1_trigger()

Actually, you made a good point here. I didn't know, that it is possible to disable atomic mode for that callback. But, apparently, it is possible. And virtio pcm definetly is nonatomic. I need to redo this code, thanks!

...

• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command) +{

• struct virtio_pcm_substream *ss =
  

snd_pcm_substream_chip(substream);

• struct virtio_snd *snd = ss->snd;
  

• struct virtio_snd_queue *queue = virtsnd_pcm_queue(ss);
  

• struct virtio_snd_msg *msg;
  

• switch (command) {
  

• case SNDRV_PCM_TRIGGER_START:
  

• case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: {
  

•         int rc;
  

•         spin_lock(&queue->lock);
  

•         rc = virtsnd_pcm_msg_send(ss);
  

•         spin_unlock(&queue->lock);
  

Maybe it would be good to explain why locking is required here and isn't required in most other locations, where messages are sent?

There are two kinds of messages here: control messages and I/O. Functions for sending control message acquire and release the control virtqueue spinlock on their own. But we cannot do the same for I/O messages, since virtsnd_pcm_msg_send is also called from the interrupt handler, which is already grabbing the lock for the I/O virtqueue.

Thanks Guennadi

...

•         if (rc)
  

•                 return rc;
  

•         atomic_set(&ss->xfer_enabled, 1);
  

•         msg = virtsnd_pcm_ctl_msg_alloc(ss, 
  

VIRTIO_SND_R_PCM_START,

...

•                                         GFP_ATOMIC);
  

•         if (IS_ERR(msg))
  

•                 return PTR_ERR(msg);
  

•         return virtsnd_ctl_msg_send(snd, msg);
  

• }
  

• case SNDRV_PCM_TRIGGER_STOP:
  

• case SNDRV_PCM_TRIGGER_PAUSE_PUSH: {
  

•         atomic_set(&ss->xfer_enabled, 0);
  

•         msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_STOP,
  

•                                         GFP_ATOMIC);
  

•         if (IS_ERR(msg))
  

•                 return PTR_ERR(msg);
  

•         return virtsnd_ctl_msg_send(snd, msg);
  

• }
  

• default: {
  

•         return -EINVAL;
  

• }
  

• }
  

+}