Messages in this thread | | | Date | Mon, 25 Jan 2021 17:25:11 +0100 (CET) | From | Guennadi Liakhovetski <> | Subject | Re: [PATCH v2 5/9] ALSA: virtio: handling control and I/O messages for the PCM device |
| |
On Sun, 24 Jan 2021, Anton Yakovlev wrote:
> The driver implements a message-based transport for I/O substream > operations. Before the start of the substream, the hardware buffer is > sliced into I/O messages, the number of which is equal to the current > number of periods. The size of each message is equal to the current > size of one period. > > I/O messages are organized in an ordered queue. The completion of the > I/O message indicates an elapsed period (the only exception is the end > of the stream for the capture substream). Upon completion, the message > is automatically re-added to the end of the queue. > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > --- > sound/virtio/Makefile | 3 +- > sound/virtio/virtio_card.c | 10 ++ > sound/virtio/virtio_card.h | 9 + > sound/virtio/virtio_pcm.c | 3 + > sound/virtio/virtio_pcm.h | 31 ++++ > sound/virtio/virtio_pcm_msg.c | 325 ++++++++++++++++++++++++++++++++++ > 6 files changed, 380 insertions(+), 1 deletion(-) > create mode 100644 sound/virtio/virtio_pcm_msg.c > > diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile > index 69162a545a41..626af3cc3ed7 100644 > --- a/sound/virtio/Makefile > +++ b/sound/virtio/Makefile > @@ -5,5 +5,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o > virtio_snd-objs := \ > virtio_card.o \ > virtio_ctl_msg.o \ > - virtio_pcm.o > + virtio_pcm.o \ > + virtio_pcm_msg.o > > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index 39fe13b43dd1..11d025ee77c2 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c > @@ -143,6 +143,12 @@ static int virtsnd_find_vqs(struct virtio_snd *snd) > callbacks[VIRTIO_SND_VQ_CONTROL] = virtsnd_ctl_notify_cb; > callbacks[VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb; > > + virtio_cread(vdev, struct virtio_snd_config, streams, &n); > + if (n) { > + callbacks[VIRTIO_SND_VQ_TX] = virtsnd_pcm_tx_notify_cb; > + callbacks[VIRTIO_SND_VQ_RX] = virtsnd_pcm_rx_notify_cb; > + } > + > rc = virtio_find_vqs(vdev, VIRTIO_SND_VQ_MAX, vqs, callbacks, names, > NULL); > if (rc) { > @@ -177,6 +183,10 @@ static int virtsnd_find_vqs(struct virtio_snd *snd) > * virtsnd_enable_event_vq() - Enable the event virtqueue. > * @snd: VirtIO sound device. > * > + * The tx queue is enabled only if the device supports playback stream(s). > + * > + * The rx queue is enabled only if the device supports capture stream(s). > + * > * Context: Any context. > */ > static void virtsnd_enable_event_vq(struct virtio_snd *snd) > diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h > index be6651a6aaf8..b11c09984882 100644 > --- a/sound/virtio/virtio_card.h > +++ b/sound/virtio/virtio_card.h > @@ -89,4 +89,13 @@ virtsnd_rx_queue(struct virtio_snd *snd) > return &snd->queues[VIRTIO_SND_VQ_RX]; > } > > +static inline struct virtio_snd_queue * > +virtsnd_pcm_queue(struct virtio_pcm_substream *substream) > +{ > + if (substream->direction == SNDRV_PCM_STREAM_PLAYBACK) > + return virtsnd_tx_queue(substream->snd); > + else > + return virtsnd_rx_queue(substream->snd); > +} > + > #endif /* VIRTIO_SND_CARD_H */ > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index 036990b7b78a..1ab50dcc88c8 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c > @@ -376,6 +376,7 @@ int virtsnd_pcm_parse_cfg(struct virtio_snd *snd) > > substream->snd = snd; > substream->sid = i; > + init_waitqueue_head(&substream->msg_empty); > > rc = virtsnd_pcm_build_hw(substream, &info[i]); > if (rc) > @@ -530,6 +531,8 @@ void virtsnd_pcm_event(struct virtio_snd *snd, struct virtio_snd_event *event) > break; > } > case VIRTIO_SND_EVT_PCM_XRUN: { > + if (atomic_read(&substream->xfer_enabled))
Why does .xfer_enabled have to be atomic? It only takes two values - 0 and 1, I don't see any incrementing, or test-and-set type operations or anything similar. Also I don't see .xfer_enabled being set to 1 anywhere in this patch, presumably that happens in one of later patches.
> + atomic_set(&substream->xfer_xrun, 1);
Ditto.
> break; > } > } > diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h > index 73fb4d9dc524..d011b7e1d18d 100644 > --- a/sound/virtio/virtio_pcm.h > +++ b/sound/virtio/virtio_pcm.h > @@ -24,6 +24,7 @@ > #include <sound/pcm.h> > > struct virtio_pcm; > +struct virtio_pcm_msg; > > /** > * struct virtio_pcm_substream - VirtIO PCM substream. > @@ -34,6 +35,16 @@ struct virtio_pcm; > * @features: Stream VirtIO feature bit map (1 << VIRTIO_SND_PCM_F_XXX). > * @substream: Kernel ALSA substream. > * @hw: Kernel ALSA substream hardware descriptor. > + * @frame_bytes: Current frame size in bytes. > + * @period_size: Current period size in frames. > + * @buffer_size: Current buffer size in frames. > + * @hw_ptr: Substream hardware pointer value in frames [0 ... buffer_size). > + * @xfer_enabled: Data transfer state (0 - off, 1 - on). > + * @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun). > + * @msgs: I/O messages. > + * @msg_last_enqueued: Index of the last I/O message added to the virtqueue. > + * @msg_count: Number of pending I/O messages in the virtqueue. > + * @msg_empty: Notify when msg_count is zero. > */ > struct virtio_pcm_substream { > struct virtio_snd *snd; > @@ -43,6 +54,16 @@ struct virtio_pcm_substream { > u32 features; > struct snd_pcm_substream *substream; > struct snd_pcm_hardware hw; > + unsigned int frame_bytes; > + snd_pcm_uframes_t period_size; > + snd_pcm_uframes_t buffer_size; > + atomic_t hw_ptr; > + atomic_t xfer_enabled; > + atomic_t xfer_xrun; > + struct virtio_pcm_msg *msgs; > + int msg_last_enqueued; > + atomic_t msg_count; > + wait_queue_head_t msg_empty; > }; > > /** > @@ -86,4 +107,14 @@ struct virtio_pcm *virtsnd_pcm_find(struct virtio_snd *snd, unsigned int nid); > struct virtio_pcm *virtsnd_pcm_find_or_create(struct virtio_snd *snd, > unsigned int nid); > > +struct virtio_snd_msg * > +virtsnd_pcm_ctl_msg_alloc(struct virtio_pcm_substream *substream, > + unsigned int command, gfp_t gfp); > + > +int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *substream, > + unsigned int nmsg, u8 *dma_area, > + unsigned int period_bytes); > + > +int virtsnd_pcm_msg_send(struct virtio_pcm_substream *substream); > + > #endif /* VIRTIO_SND_PCM_H */ > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > new file mode 100644 > index 000000000000..d524e7cbe43f > --- /dev/null > +++ b/sound/virtio/virtio_pcm_msg.c > @@ -0,0 +1,325 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Sound card driver for virtio > + * Copyright (C) 2020 OpenSynergy GmbH > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > +#include <sound/pcm_params.h> > + > +#include "virtio_card.h" > + > +/** > + * enum pcm_msg_sg_index - Scatter-gather element indexes for an I/O message. > + * @PCM_MSG_SG_XFER: Element containing a virtio_snd_pcm_xfer structure. > + * @PCM_MSG_SG_DATA: Element containing a data buffer. > + * @PCM_MSG_SG_STATUS: Element containing a virtio_snd_pcm_status structure. > + * @PCM_MSG_SG_MAX: The maximum number of elements in the scatter-gather table. > + * > + * These values are used as the index of the payload scatter-gather table. > + */ > +enum pcm_msg_sg_index { > + PCM_MSG_SG_XFER = 0, > + PCM_MSG_SG_DATA, > + PCM_MSG_SG_STATUS, > + PCM_MSG_SG_MAX > +};
If I understand correctly, messages are sent to the back-end driver in this specific order, so this is a part of the ABI, isn't it? Is it also a part of the spec? If so this should be defined in your ABI header?
> + > +/** > + * struct virtio_pcm_msg - VirtIO I/O message. > + * @substream: VirtIO PCM substream. > + * @xfer: Request header payload. > + * @status: Response header payload. > + * @sgs: Payload scatter-gather table. > + */ > +struct virtio_pcm_msg { > + struct virtio_pcm_substream *substream; > + struct virtio_snd_pcm_xfer xfer; > + struct virtio_snd_pcm_status status; > + struct scatterlist sgs[PCM_MSG_SG_MAX]; > +}; > + > +/** > + * virtsnd_pcm_msg_alloc() - Allocate I/O messages. > + * @substream: VirtIO PCM substream. > + * @nmsg: Number of messages (equal to the number of periods). > + * @dma_area: Pointer to used audio buffer. > + * @period_bytes: Period (message payload) size. > + * > + * The function slices the buffer into nmsg parts (each with the size of > + * period_bytes), and creates nmsg corresponding I/O messages. > + * > + * Context: Any context that permits to sleep. > + * Return: 0 on success, -ENOMEM on failure. > + */ > +int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *substream, > + unsigned int nmsg, u8 *dma_area, > + unsigned int period_bytes) > +{ > + struct virtio_device *vdev = substream->snd->vdev; > + unsigned int i; > + > + if (substream->msgs) > + devm_kfree(&vdev->dev, substream->msgs); > + > + substream->msgs = devm_kcalloc(&vdev->dev, nmsg, > + sizeof(*substream->msgs), GFP_KERNEL); > + if (!substream->msgs) > + return -ENOMEM; > + > + for (i = 0; i < nmsg; ++i) { > + struct virtio_pcm_msg *msg = &substream->msgs[i]; > + > + msg->substream = substream; > + > + sg_init_table(msg->sgs, PCM_MSG_SG_MAX);
Why do you need to initialise a table of 3 meddages if you then initialise each of them separately immediately below?
> + sg_init_one(&msg->sgs[PCM_MSG_SG_XFER], &msg->xfer, > + sizeof(msg->xfer)); > + sg_init_one(&msg->sgs[PCM_MSG_SG_DATA], > + dma_area + period_bytes * i, period_bytes); > + sg_init_one(&msg->sgs[PCM_MSG_SG_STATUS], &msg->status, > + sizeof(msg->status)); > + } > + > + return 0; > +} > + > +/** > + * virtsnd_pcm_msg_send() - Send asynchronous I/O messages. > + * @substream: VirtIO PCM substream. > + * > + * All messages are organized in an ordered circular list. Each time the > + * function is called, all currently non-enqueued messages are added to the > + * virtqueue. For this, the function keeps track of two values: > + * > + * msg_last_enqueued = index of the last enqueued message, > + * msg_count = # of pending messages in the virtqueue. > + * > + * Context: Any context. > + * Return: 0 on success, -EIO on failure. > + */ > +int virtsnd_pcm_msg_send(struct virtio_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->substream->runtime; > + struct virtio_snd *snd = substream->snd; > + struct virtio_device *vdev = snd->vdev; > + struct virtqueue *vqueue = virtsnd_pcm_queue(substream)->vqueue; > + int i; > + int n; > + bool notify = false; > + > + if (!vqueue) > + return -EIO;
Is this actually possible? That would mean a data corruption or a bug in the driver, right? In either case it can be NULL or 1 or any other invalid value, so checking for NULL doesn't seem to help a lot?
> + > + i = (substream->msg_last_enqueued + 1) % runtime->periods; > + n = runtime->periods - atomic_read(&substream->msg_count); > + > + for (; n; --n, i = (i + 1) % runtime->periods) { > + struct virtio_pcm_msg *msg = &substream->msgs[i]; > + struct scatterlist *psgs[PCM_MSG_SG_MAX] = { > + [PCM_MSG_SG_XFER] = &msg->sgs[PCM_MSG_SG_XFER], > + [PCM_MSG_SG_DATA] = &msg->sgs[PCM_MSG_SG_DATA], > + [PCM_MSG_SG_STATUS] = &msg->sgs[PCM_MSG_SG_STATUS] > + }; > + int rc; > + > + msg->xfer.stream_id = cpu_to_virtio32(vdev, substream->sid); > + memset(&msg->status, 0, sizeof(msg->status)); > + > + atomic_inc(&substream->msg_count);
.msg_count is also accessed in virtsnd_pcm_msg_complete() which is why presumably you use atomic access. But here you already increment the count before you even begin adding the message to the virtqueue. So if virtsnd_pcm_msg_complete() preempts you here the .msg_count will be inconsistent? Possibly you need to protect both operations together: incrementing the counter and adding messages to queues.
> + > + if (substream->direction == SNDRV_PCM_STREAM_PLAYBACK) > + rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg, > + GFP_ATOMIC); > + else > + rc = virtqueue_add_sgs(vqueue, psgs, 1, 2, msg, > + GFP_ATOMIC); > + > + if (rc) { > + atomic_dec(&substream->msg_count); > + return -EIO; > + } > + > + substream->msg_last_enqueued = i; > + } > + > + if (!(substream->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING))) > + notify = virtqueue_kick_prepare(vqueue); > + > + if (notify) > + if (!virtqueue_notify(vqueue)) > + return -EIO; > + > + return 0; > +} > + > +/** > + * virtsnd_pcm_msg_complete() - Complete an I/O message. > + * @msg: I/O message. > + * @size: Number of bytes written. > + * > + * Completion of the message means the elapsed period. > + * > + * The interrupt handler modifies three fields of the substream structure > + * (hw_ptr, xfer_xrun, msg_count) that are used in operator functions. These > + * values are atomic to avoid frequent interlocks with the interrupt handler. > + * This becomes especially important in the case of multiple running substreams > + * that share both the virtqueue and interrupt handler. > + * > + * Context: Interrupt context. > + */ > +static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg, size_t size) > +{ > + struct virtio_pcm_substream *substream = msg->substream; > + snd_pcm_uframes_t hw_ptr; > + unsigned int msg_count; > + > + /* > + * hw_ptr always indicates the buffer position of the first I/O message > + * in the virtqueue. Therefore, on each completion of an I/O message, > + * the hw_ptr value is unconditionally advanced. > + */ > + hw_ptr = (snd_pcm_uframes_t)atomic_read(&substream->hw_ptr);
Also unclear why this has to be atomic, especially taking into account that it's only accessed in "interrupt context."
> + > + /* > + * If the capture substream returned an incorrect status, then just > + * increase the hw_ptr by the period size. > + */ > + if (substream->direction == SNDRV_PCM_STREAM_PLAYBACK || > + size <= sizeof(msg->status)) { > + hw_ptr += substream->period_size; > + } else { > + size -= sizeof(msg->status); > + hw_ptr += size / substream->frame_bytes; > + } > + > + atomic_set(&substream->hw_ptr, (u32)(hw_ptr % substream->buffer_size)); > + atomic_set(&substream->xfer_xrun, 0); > + > + msg_count = atomic_dec_return(&substream->msg_count); > + > + if (atomic_read(&substream->xfer_enabled)) { > + struct snd_pcm_runtime *runtime = substream->substream->runtime; > + > + runtime->delay = > + bytes_to_frames(runtime, > + le32_to_cpu(msg->status.latency_bytes)); > + > + snd_pcm_period_elapsed(substream->substream); > + > + virtsnd_pcm_msg_send(substream); > + } else if (!msg_count) { > + wake_up_all(&substream->msg_empty); > + } > +}
Thanks Guennadi
| |