| Message ID | 20210124165408.1122868-6-anton.yakovlev@opensynergy.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/9] uapi: virtio_ids: add a sound device type ID from OASIS spec | expand |
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
On 25.01.2021 17:25, Guennadi Liakhovetski wrote: ...[snip]... >> 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. > Yes, maybe I was not very good at breaking the code into patches. .xfer_enabled and .xfer_xrun are used in callback functions for operators (next patch). Basically, these two contain boolean values. ...[snip]... >> + >> +/** >> + * 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? Yes, this is a part of the spec. But the spec only defines a "layout" of the message, and does not limit or in any way define the number of descriptors to transmit each of the parts of the message. Hence, this enum cannot be defined as part of the ABI. However, since this driver uses only one descriptor for each part, it is more convenient to use an enum to make the code more readable. >> + >> +/** >> + * 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? Hm, good point! I forgot to delete this line, thanks. >> + 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? Yes it is possible. The virtio device may ask the driver to reset itself. This can happen at any time, including when the device is actively used. In such case, we disable the use of virtqueues by setting the .vqueue values to NULL. >> + >> + 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. It is not necessary here. As virtqueue_add_sgs requires the virtqueue to be protected by the caller using an external lock, so all calls to virtsnd_pcm_msg_send are wrapped with spinlocks (with disabled interrupts for the current core) for the tx/rx virtqueues. >> + >> + 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." The general situation looks like this: .hw_ptr and .xfer_xrun written in the virtsnd_pcm_msg_complete() read in the pointer() substream operator .xfer_enabled written in the trigger() substream operator read in the virtsnd_pcm_msg_complete() ALSA takes some substream locks while calling for trigger/pointer(). Unfortunately, we cannot use the same substream locks here, as it opens up many control paths leading to deadlock. And all that remains is either to use atomic fields, or to introduce our own spinlock for each substream (to protect these fields). Personally, I don't know which would be better. But the code with atomic fields looks at least simpler. >> + >> + /* >> + * 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 >
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)) + atomic_set(&substream->xfer_xrun, 1); 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 +}; + +/** + * 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); + 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; + + 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); + + 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); + + /* + * 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); + } +} + +/** + * virtsnd_pcm_notify_cb() - Process all completed I/O messages. + * @vqueue: Underlying tx/rx virtqueue. + * + * If transmission is allowed, then each completed message is immediately placed + * back at the end of the queue. + * + * Context: Interrupt context. Takes and releases the tx/rx queue spinlock. + */ +static inline void virtsnd_pcm_notify_cb(struct virtio_snd_queue *queue) +{ + unsigned long flags; + + spin_lock_irqsave(&queue->lock, flags); + while (queue->vqueue) { + virtqueue_disable_cb(queue->vqueue); + + for (;;) { + struct virtio_pcm_msg *msg; + u32 length; + + msg = virtqueue_get_buf(queue->vqueue, &length); + if (!msg) + break; + + virtsnd_pcm_msg_complete(msg, length); + } + + if (unlikely(virtqueue_is_broken(queue->vqueue))) + break; + + if (virtqueue_enable_cb(queue->vqueue)) + break; + } + spin_unlock_irqrestore(&queue->lock, flags); +} + +/** + * virtsnd_pcm_tx_notify_cb() - Process all completed TX messages. + * @vqueue: Underlying tx virtqueue. + * + * Context: Interrupt context. + */ +void virtsnd_pcm_tx_notify_cb(struct virtqueue *vqueue) +{ + struct virtio_snd *snd = vqueue->vdev->priv; + + virtsnd_pcm_notify_cb(virtsnd_tx_queue(snd)); +} + +/** + * virtsnd_pcm_rx_notify_cb() - Process all completed RX messages. + * @vqueue: Underlying rx virtqueue. + * + * Context: Interrupt context. + */ +void virtsnd_pcm_rx_notify_cb(struct virtqueue *vqueue) +{ + struct virtio_snd *snd = vqueue->vdev->priv; + + virtsnd_pcm_notify_cb(virtsnd_rx_queue(snd)); +} + +/** + * virtsnd_pcm_ctl_msg_alloc() - Allocate and initialize the PCM device control + * message for the specified substream. + * @substream: VirtIO PCM substream. + * @command: Control request code (VIRTIO_SND_R_PCM_XXX). + * @gfp: Kernel flags for memory allocation. + * + * Context: Any context. May sleep if @gfp flags permit. + * Return: Allocated message on success, ERR_PTR(-errno) on failure. + */ +struct virtio_snd_msg * +virtsnd_pcm_ctl_msg_alloc(struct virtio_pcm_substream *substream, + unsigned int command, gfp_t gfp) +{ + struct virtio_device *vdev = substream->snd->vdev; + size_t request_size = sizeof(struct virtio_snd_pcm_hdr); + size_t response_size = sizeof(struct virtio_snd_hdr); + struct virtio_snd_msg *msg; + + switch (command) { + case VIRTIO_SND_R_PCM_SET_PARAMS: { + request_size = sizeof(struct virtio_snd_pcm_set_params); + break; + } + } + + msg = virtsnd_ctl_msg_alloc(vdev, request_size, response_size, gfp); + if (!IS_ERR(msg)) { + struct virtio_snd_pcm_hdr *hdr = sg_virt(&msg->sg_request); + + hdr->hdr.code = cpu_to_virtio32(vdev, command); + hdr->stream_id = cpu_to_virtio32(vdev, substream->sid); + } + + return msg; +}
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