This commit replaces the mmap mechanism with the copy() and
fill_silence() callbacks for both capturing and playback for the
virtio-sound driver. This change is required to prevent the updating of
the content of a buffer that is already in the available ring.
The current mechanism splits a dma buffer into descriptors that are
exposed to the device. This dma buffer is shared with the user
application. When the device consumes a buffer, the driver moves the
request from the used ring to available ring.
The driver exposes the buffer to the device without knowing if the
content has been updated from the user. The section 2.8.21.1 of the
virtio spec states that: "The device MAY access the descriptor chains
the driver created and the memory they refer to immediately". If the
device picks up buffers from the available ring just after it is
notified, it happens that the content may be old.
By providing the copy() callback, the driver first updates the content
of the buffer, and then, exposes the buffer to the device by enqueuing
it in the available ring. Thus, device always picks up a buffer that is
updated. During copy(), the number of requests enqueued depends on the
"pos" and "bytes" arguments. The length of each request is period_size
bytes.
For capturing, the driver starts by exposing all the available buffers
to device. After device updates the content of a buffer, it enqueues it
in the used ring. It is only after the copy() for capturing is issued
that the driver re-enqueues the buffer in the available ring.
Co-developed-by: Anton Yakovlev <[email protected]>
Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
---
Changelog:
v1 -> v2:
* Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
* Make virtsnd_pcm_msg_send() generic by specifying the offset and size
for the modified part of the buffer; this way no assumptions need to
be made.
* Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
reading/writing of frames is supported.
* Correct comment at virtsnd_pcm_msg_send().
* v1 patch at:
https://lore.kernel.org/lkml/20231016151000.GE119987@fedora/t/
sound/virtio/virtio_pcm.c | 7 ++-
sound/virtio/virtio_pcm.h | 9 ++--
sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
4 files changed, 137 insertions(+), 53 deletions(-)
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index c10d91fff2fb..66d67eef1bcc 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
* only message-based transport.
*/
vss->hw.info =
- SNDRV_PCM_INFO_MMAP |
- SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_BATCH |
SNDRV_PCM_INFO_BLOCK_TRANSFER |
SNDRV_PCM_INFO_INTERLEAVED |
- SNDRV_PCM_INFO_PAUSE;
+ SNDRV_PCM_INFO_PAUSE |
+ SNDRV_PCM_INFO_NO_REWINDS;
if (!info->channels_min || info->channels_min > info->channels_max) {
dev_err(&vdev->dev,
@@ -471,7 +470,7 @@ int virtsnd_pcm_build_devs(struct virtio_snd *snd)
for (kss = ks->substream; kss; kss = kss->next)
vs->substreams[kss->number]->substream = kss;
- snd_pcm_set_ops(vpcm->pcm, i, &virtsnd_pcm_ops);
+ snd_pcm_set_ops(vpcm->pcm, i, &virtsnd_pcm_ops[i]);
}
snd_pcm_set_managed_buffer_all(vpcm->pcm,
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 062eb8e8f2cf..8b42928a8e01 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -57,7 +57,6 @@ struct virtio_pcm_substream {
bool suspended;
struct virtio_pcm_msg **msgs;
unsigned int nmsgs;
- int msg_last_enqueued;
unsigned int msg_count;
wait_queue_head_t msg_empty;
};
@@ -90,7 +89,7 @@ struct virtio_pcm {
struct virtio_pcm_stream streams[SNDRV_PCM_STREAM_LAST + 1];
};
-extern const struct snd_pcm_ops virtsnd_pcm_ops;
+extern const struct snd_pcm_ops virtsnd_pcm_ops[];
int virtsnd_pcm_validate(struct virtio_device *vdev);
@@ -117,7 +116,11 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss);
-int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss);
+int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
+ unsigned long bytes);
+
+int virtsnd_pcm_msg_send_locked(struct virtio_pcm_substream *vss,
+ unsigned long offset, unsigned long bytes);
unsigned int virtsnd_pcm_msg_pending_num(struct virtio_pcm_substream *vss);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index aca2dc1989ba..c3d471088d9e 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -155,7 +155,6 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
sizeof(msg->xfer));
sg_init_one(&msg->sgs[PCM_MSG_SG_STATUS], &msg->status,
sizeof(msg->status));
- msg->length = period_bytes;
virtsnd_pcm_sg_from(&msg->sgs[PCM_MSG_SG_DATA], sg_num, data,
period_bytes);
@@ -189,58 +188,70 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
*
* 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.
+ * virtqueue. For this, the function uses offset and bytes to calculate the
+ * messages that need to be added.
*
* Context: Any context. Expects the tx/rx queue and the VirtIO substream
* spinlocks to be held by caller.
* Return: 0 on success, -errno on failure.
*/
-int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss)
+int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
+ unsigned long bytes)
{
- struct snd_pcm_runtime *runtime = vss->substream->runtime;
struct virtio_snd *snd = vss->snd;
struct virtio_device *vdev = snd->vdev;
struct virtqueue *vqueue = virtsnd_pcm_queue(vss)->vqueue;
- int i;
- int n;
+ unsigned long period_bytes = snd_pcm_lib_period_bytes(vss->substream);
+ unsigned long start, end, i;
+ unsigned int msg_count = vss->msg_count;
bool notify = false;
+ int rc;
- i = (vss->msg_last_enqueued + 1) % runtime->periods;
- n = runtime->periods - vss->msg_count;
+ start = offset / period_bytes;
+ end = (offset + bytes - 1) / period_bytes;
- for (; n; --n, i = (i + 1) % runtime->periods) {
+ for (i = start; i <= end; i++) {
struct virtio_pcm_msg *msg = vss->msgs[i];
struct scatterlist *psgs[] = {
&msg->sgs[PCM_MSG_SG_XFER],
&msg->sgs[PCM_MSG_SG_DATA],
&msg->sgs[PCM_MSG_SG_STATUS]
};
- int rc;
-
- msg->xfer.stream_id = cpu_to_le32(vss->sid);
- memset(&msg->status, 0, sizeof(msg->status));
-
- if (vss->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) {
- dev_err(&vdev->dev,
- "SID %u: failed to send I/O message\n",
- vss->sid);
- return rc;
+ unsigned long n;
+
+ n = period_bytes - (offset % period_bytes);
+ if (n > bytes)
+ n = bytes;
+
+ msg->length += n;
+ if (msg->length == period_bytes) {
+ msg->xfer.stream_id = cpu_to_le32(vss->sid);
+ memset(&msg->status, 0, sizeof(msg->status));
+
+ if (vss->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) {
+ dev_err(&vdev->dev,
+ "SID %u: failed to send I/O message\n",
+ vss->sid);
+ return rc;
+ }
+
+ vss->msg_count++;
}
- vss->msg_last_enqueued = i;
- vss->msg_count++;
+ offset = 0;
+ bytes -= n;
}
+ if (msg_count == vss->msg_count)
+ return 0;
+
if (!(vss->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING)))
notify = virtqueue_kick_prepare(vqueue);
@@ -250,6 +261,22 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss)
return 0;
}
+int virtsnd_pcm_msg_send_locked(struct virtio_pcm_substream *vss,
+ unsigned long offset, unsigned long bytes)
+{
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ unsigned long flags;
+ int rc;
+
+ spin_lock_irqsave(&queue->lock, flags);
+ spin_lock(&vss->lock);
+ rc = virtsnd_pcm_msg_send(vss, offset, bytes);
+ spin_unlock(&vss->lock);
+ spin_unlock_irqrestore(&queue->lock, flags);
+
+ return rc;
+}
+
/**
* virtsnd_pcm_msg_pending_num() - Returns the number of pending I/O messages.
* @vss: VirtIO substream.
@@ -309,6 +336,8 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
if (vss->hw_ptr >= vss->buffer_bytes)
vss->hw_ptr -= vss->buffer_bytes;
+ msg->length = 0;
+
vss->xfer_xrun = false;
vss->msg_count--;
@@ -320,8 +349,6 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
le32_to_cpu(msg->status.latency_bytes));
schedule_work(&vss->elapsed_period);
-
- virtsnd_pcm_msg_send(vss);
} else if (!vss->msg_count) {
wake_up_all(&vss->msg_empty);
}
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index f8bfb87624be..4569b285f520 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -282,7 +282,6 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
vss->hw_ptr = 0;
- vss->msg_last_enqueued = -1;
} else {
struct snd_pcm_runtime *runtime = substream->runtime;
unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
@@ -324,7 +323,7 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
struct virtio_snd_queue *queue;
struct virtio_snd_msg *msg;
unsigned long flags;
- int rc;
+ int rc = 0;
switch (command) {
case SNDRV_PCM_TRIGGER_START:
@@ -333,7 +332,8 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
spin_lock_irqsave(&queue->lock, flags);
spin_lock(&vss->lock);
- rc = virtsnd_pcm_msg_send(vss);
+ if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
+ rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
if (!rc)
vss->xfer_enabled = true;
spin_unlock(&vss->lock);
@@ -450,15 +450,70 @@ virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
return hw_ptr;
}
+static int virtsnd_pcm_pb_silence(struct snd_pcm_substream *substream,
+ int channel, unsigned long pos,
+ unsigned long bytes)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct snd_pcm_runtime *runtime = substream->runtime;
+
+ snd_pcm_format_set_silence(runtime->format, runtime->dma_area + pos,
+ bytes_to_samples(runtime, bytes));
+
+ return virtsnd_pcm_msg_send_locked(vss, pos, bytes);
+}
+
+static int virtsnd_pcm_pb_copy(struct snd_pcm_substream *substream, int channel,
+ unsigned long pos, struct iov_iter *iter,
+ unsigned long bytes)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct snd_pcm_runtime *runtime = substream->runtime;
+
+ if (copy_from_iter(runtime->dma_area + pos, bytes, iter) != bytes)
+ return -EFAULT;
+
+ return virtsnd_pcm_msg_send_locked(vss, pos, bytes);
+}
+
+static int virtsnd_pcm_cp_copy(struct snd_pcm_substream *substream, int channel,
+ unsigned long pos, struct iov_iter *iter,
+ unsigned long bytes)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct snd_pcm_runtime *runtime = substream->runtime;
+
+ if (copy_to_iter(runtime->dma_area + pos, bytes, iter) != bytes)
+ return -EFAULT;
+
+ return virtsnd_pcm_msg_send_locked(vss, pos, bytes);
+}
+
/* PCM substream operators map. */
-const struct snd_pcm_ops virtsnd_pcm_ops = {
- .open = virtsnd_pcm_open,
- .close = virtsnd_pcm_close,
- .ioctl = snd_pcm_lib_ioctl,
- .hw_params = virtsnd_pcm_hw_params,
- .hw_free = virtsnd_pcm_hw_free,
- .prepare = virtsnd_pcm_prepare,
- .trigger = virtsnd_pcm_trigger,
- .sync_stop = virtsnd_pcm_sync_stop,
- .pointer = virtsnd_pcm_pointer,
+const struct snd_pcm_ops virtsnd_pcm_ops[] = {
+ {
+ .open = virtsnd_pcm_open,
+ .close = virtsnd_pcm_close,
+ .ioctl = snd_pcm_lib_ioctl,
+ .hw_params = virtsnd_pcm_hw_params,
+ .hw_free = virtsnd_pcm_hw_free,
+ .prepare = virtsnd_pcm_prepare,
+ .trigger = virtsnd_pcm_trigger,
+ .sync_stop = virtsnd_pcm_sync_stop,
+ .pointer = virtsnd_pcm_pointer,
+ .copy = virtsnd_pcm_pb_copy,
+ .fill_silence = virtsnd_pcm_pb_silence,
+ },
+ {
+ .open = virtsnd_pcm_open,
+ .close = virtsnd_pcm_close,
+ .ioctl = snd_pcm_lib_ioctl,
+ .hw_params = virtsnd_pcm_hw_params,
+ .hw_free = virtsnd_pcm_hw_free,
+ .prepare = virtsnd_pcm_prepare,
+ .trigger = virtsnd_pcm_trigger,
+ .sync_stop = virtsnd_pcm_sync_stop,
+ .pointer = virtsnd_pcm_pointer,
+ .copy = virtsnd_pcm_cp_copy,
+ },
};
base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
--
2.41.0
Hi Matias,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa]
url: https://github.com/intel-lab-lkp/linux/commits/Matias-Ezequiel-Vara-Larsen/ALSA-virtio-use-copy-and-fill_silence-callbacks/20231018-185108
base: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
patch link: https://lore.kernel.org/r/ZS%2B392ZzVIoEyv8n%40fedora
patch subject: [PATCH v2] ALSA: virtio: use copy and fill_silence callbacks
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231018/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231018/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> sound/virtio/virtio_pcm_msg.c:200: warning: Function parameter or member 'offset' not described in 'virtsnd_pcm_msg_send'
>> sound/virtio/virtio_pcm_msg.c:200: warning: Function parameter or member 'bytes' not described in 'virtsnd_pcm_msg_send'
2 warnings as Errors
vim +200 sound/virtio/virtio_pcm_msg.c
f40a28679e0b7c Anton Yakovlev 2021-03-02 184
f40a28679e0b7c Anton Yakovlev 2021-03-02 185 /**
f40a28679e0b7c Anton Yakovlev 2021-03-02 186 * virtsnd_pcm_msg_send() - Send asynchronous I/O messages.
f40a28679e0b7c Anton Yakovlev 2021-03-02 187 * @vss: VirtIO PCM substream.
f40a28679e0b7c Anton Yakovlev 2021-03-02 188 *
f40a28679e0b7c Anton Yakovlev 2021-03-02 189 * All messages are organized in an ordered circular list. Each time the
f40a28679e0b7c Anton Yakovlev 2021-03-02 190 * function is called, all currently non-enqueued messages are added to the
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 191 * virtqueue. For this, the function uses offset and bytes to calculate the
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 192 * messages that need to be added.
f40a28679e0b7c Anton Yakovlev 2021-03-02 193 *
f40a28679e0b7c Anton Yakovlev 2021-03-02 194 * Context: Any context. Expects the tx/rx queue and the VirtIO substream
f40a28679e0b7c Anton Yakovlev 2021-03-02 195 * spinlocks to be held by caller.
f40a28679e0b7c Anton Yakovlev 2021-03-02 196 * Return: 0 on success, -errno on failure.
f40a28679e0b7c Anton Yakovlev 2021-03-02 197 */
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 198 int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 199 unsigned long bytes)
f40a28679e0b7c Anton Yakovlev 2021-03-02 @200 {
f40a28679e0b7c Anton Yakovlev 2021-03-02 201 struct virtio_snd *snd = vss->snd;
f40a28679e0b7c Anton Yakovlev 2021-03-02 202 struct virtio_device *vdev = snd->vdev;
f40a28679e0b7c Anton Yakovlev 2021-03-02 203 struct virtqueue *vqueue = virtsnd_pcm_queue(vss)->vqueue;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 204 unsigned long period_bytes = snd_pcm_lib_period_bytes(vss->substream);
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 205 unsigned long start, end, i;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 206 unsigned int msg_count = vss->msg_count;
f40a28679e0b7c Anton Yakovlev 2021-03-02 207 bool notify = false;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 208 int rc;
f40a28679e0b7c Anton Yakovlev 2021-03-02 209
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 210 start = offset / period_bytes;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 211 end = (offset + bytes - 1) / period_bytes;
f40a28679e0b7c Anton Yakovlev 2021-03-02 212
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 213 for (i = start; i <= end; i++) {
f40a28679e0b7c Anton Yakovlev 2021-03-02 214 struct virtio_pcm_msg *msg = vss->msgs[i];
f40a28679e0b7c Anton Yakovlev 2021-03-02 215 struct scatterlist *psgs[] = {
f40a28679e0b7c Anton Yakovlev 2021-03-02 216 &msg->sgs[PCM_MSG_SG_XFER],
f40a28679e0b7c Anton Yakovlev 2021-03-02 217 &msg->sgs[PCM_MSG_SG_DATA],
f40a28679e0b7c Anton Yakovlev 2021-03-02 218 &msg->sgs[PCM_MSG_SG_STATUS]
f40a28679e0b7c Anton Yakovlev 2021-03-02 219 };
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 220 unsigned long n;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 221
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 222 n = period_bytes - (offset % period_bytes);
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 223 if (n > bytes)
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 224 n = bytes;
f40a28679e0b7c Anton Yakovlev 2021-03-02 225
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 226 msg->length += n;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 227 if (msg->length == period_bytes) {
f40a28679e0b7c Anton Yakovlev 2021-03-02 228 msg->xfer.stream_id = cpu_to_le32(vss->sid);
f40a28679e0b7c Anton Yakovlev 2021-03-02 229 memset(&msg->status, 0, sizeof(msg->status));
f40a28679e0b7c Anton Yakovlev 2021-03-02 230
f40a28679e0b7c Anton Yakovlev 2021-03-02 231 if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK)
f40a28679e0b7c Anton Yakovlev 2021-03-02 232 rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg,
f40a28679e0b7c Anton Yakovlev 2021-03-02 233 GFP_ATOMIC);
f40a28679e0b7c Anton Yakovlev 2021-03-02 234 else
f40a28679e0b7c Anton Yakovlev 2021-03-02 235 rc = virtqueue_add_sgs(vqueue, psgs, 1, 2, msg,
f40a28679e0b7c Anton Yakovlev 2021-03-02 236 GFP_ATOMIC);
f40a28679e0b7c Anton Yakovlev 2021-03-02 237
f40a28679e0b7c Anton Yakovlev 2021-03-02 238 if (rc) {
f40a28679e0b7c Anton Yakovlev 2021-03-02 239 dev_err(&vdev->dev,
f40a28679e0b7c Anton Yakovlev 2021-03-02 240 "SID %u: failed to send I/O message\n",
f40a28679e0b7c Anton Yakovlev 2021-03-02 241 vss->sid);
f40a28679e0b7c Anton Yakovlev 2021-03-02 242 return rc;
f40a28679e0b7c Anton Yakovlev 2021-03-02 243 }
f40a28679e0b7c Anton Yakovlev 2021-03-02 244
f40a28679e0b7c Anton Yakovlev 2021-03-02 245 vss->msg_count++;
f40a28679e0b7c Anton Yakovlev 2021-03-02 246 }
f40a28679e0b7c Anton Yakovlev 2021-03-02 247
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 248 offset = 0;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 249 bytes -= n;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 250 }
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 251
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 252 if (msg_count == vss->msg_count)
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 253 return 0;
10ad52116c3a46 Matias Ezequiel Vara Larsen 2023-10-18 254
f40a28679e0b7c Anton Yakovlev 2021-03-02 255 if (!(vss->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING)))
f40a28679e0b7c Anton Yakovlev 2021-03-02 256 notify = virtqueue_kick_prepare(vqueue);
f40a28679e0b7c Anton Yakovlev 2021-03-02 257
f40a28679e0b7c Anton Yakovlev 2021-03-02 258 if (notify)
f40a28679e0b7c Anton Yakovlev 2021-03-02 259 virtqueue_notify(vqueue);
f40a28679e0b7c Anton Yakovlev 2021-03-02 260
f40a28679e0b7c Anton Yakovlev 2021-03-02 261 return 0;
f40a28679e0b7c Anton Yakovlev 2021-03-02 262 }
f40a28679e0b7c Anton Yakovlev 2021-03-02 263
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 18 Oct 2023 12:48:23 +0200,
Matias Ezequiel Vara Larsen wrote:
>
> This commit replaces the mmap mechanism with the copy() and
> fill_silence() callbacks for both capturing and playback for the
> virtio-sound driver. This change is required to prevent the updating of
> the content of a buffer that is already in the available ring.
>
> The current mechanism splits a dma buffer into descriptors that are
> exposed to the device. This dma buffer is shared with the user
> application. When the device consumes a buffer, the driver moves the
> request from the used ring to available ring.
>
> The driver exposes the buffer to the device without knowing if the
> content has been updated from the user. The section 2.8.21.1 of the
> virtio spec states that: "The device MAY access the descriptor chains
> the driver created and the memory they refer to immediately". If the
> device picks up buffers from the available ring just after it is
> notified, it happens that the content may be old.
>
> By providing the copy() callback, the driver first updates the content
> of the buffer, and then, exposes the buffer to the device by enqueuing
> it in the available ring. Thus, device always picks up a buffer that is
> updated. During copy(), the number of requests enqueued depends on the
> "pos" and "bytes" arguments. The length of each request is period_size
> bytes.
>
> For capturing, the driver starts by exposing all the available buffers
> to device. After device updates the content of a buffer, it enqueues it
> in the used ring. It is only after the copy() for capturing is issued
> that the driver re-enqueues the buffer in the available ring.
>
> Co-developed-by: Anton Yakovlev <[email protected]>
> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> ---
> Changelog:
> v1 -> v2:
> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
> for the modified part of the buffer; this way no assumptions need to
> be made.
> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
> reading/writing of frames is supported.
> * Correct comment at virtsnd_pcm_msg_send().
> * v1 patch at:
> https://lore.kernel.org/lkml/20231016151000.GE119987@fedora/t/
>
> sound/virtio/virtio_pcm.c | 7 ++-
> sound/virtio/virtio_pcm.h | 9 ++--
> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
> 4 files changed, 137 insertions(+), 53 deletions(-)
Most of the code changes look good, but I wonder:
>
> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index c10d91fff2fb..66d67eef1bcc 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> * only message-based transport.
> */
> vss->hw.info =
> - SNDRV_PCM_INFO_MMAP |
> - SNDRV_PCM_INFO_MMAP_VALID |
Do we need the removal of those MMAP features inevitably?
Usually mmap can still work even if the driver implements the copy
ops. Those aren't always mutual exclusive.
thanks,
Takashi
Hi Takashi,
On 19.10.2023 03:07, Takashi Iwai wrote:
> On Wed, 18 Oct 2023 12:48:23 +0200,
> Matias Ezequiel Vara Larsen wrote:
>>
>> This commit replaces the mmap mechanism with the copy() and
>> fill_silence() callbacks for both capturing and playback for the
>> virtio-sound driver. This change is required to prevent the updating of
>> the content of a buffer that is already in the available ring.
>>
>> The current mechanism splits a dma buffer into descriptors that are
>> exposed to the device. This dma buffer is shared with the user
>> application. When the device consumes a buffer, the driver moves the
>> request from the used ring to available ring.
>>
>> The driver exposes the buffer to the device without knowing if the
>> content has been updated from the user. The section 2.8.21.1 of the
>> virtio spec states that: "The device MAY access the descriptor chains
>> the driver created and the memory they refer to immediately". If the
>> device picks up buffers from the available ring just after it is
>> notified, it happens that the content may be old.
>>
>> By providing the copy() callback, the driver first updates the content
>> of the buffer, and then, exposes the buffer to the device by enqueuing
>> it in the available ring. Thus, device always picks up a buffer that is
>> updated. During copy(), the number of requests enqueued depends on the
>> "pos" and "bytes" arguments. The length of each request is period_size
>> bytes.
>>
>> For capturing, the driver starts by exposing all the available buffers
>> to device. After device updates the content of a buffer, it enqueues it
>> in the used ring. It is only after the copy() for capturing is issued
>> that the driver re-enqueues the buffer in the available ring.
>>
>> Co-developed-by: Anton Yakovlev <[email protected]>
>> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
>> ---
>> Changelog:
>> v1 -> v2:
>> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
>> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
>> for the modified part of the buffer; this way no assumptions need to
>> be made.
>> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
>> reading/writing of frames is supported.
>> * Correct comment at virtsnd_pcm_msg_send().
>> * v1 patch at:
>> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=2f305b77-83e7-47b6-a461-a8ca67d0bfe2&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d5775265e7e1741ae8eb783a3cb78ed553093c1
>>
>> sound/virtio/virtio_pcm.c | 7 ++-
>> sound/virtio/virtio_pcm.h | 9 ++--
>> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
>> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
>> 4 files changed, 137 insertions(+), 53 deletions(-)
>
> Most of the code changes look good, but I wonder:
>
>>
>> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
>> index c10d91fff2fb..66d67eef1bcc 100644
>> --- a/sound/virtio/virtio_pcm.c
>> +++ b/sound/virtio/virtio_pcm.c
>> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
>> * only message-based transport.
>> */
>> vss->hw.info =
>> - SNDRV_PCM_INFO_MMAP |
>> - SNDRV_PCM_INFO_MMAP_VALID |
>
> Do we need the removal of those MMAP features inevitably?
> Usually mmap can still work even if the driver implements the copy
> ops. Those aren't always mutual exclusive.
The driver uses a message queue to communicate with the device. Thus,
the audio buffer is sliced into several I/O requests (= number of
periods) of the same size (= period size).
Before this, all such requests were enqueued when the substream started,
and immediately re-enqueued once the request is completed. This approach
made it possible to add mmap support. But for mmap there are no explicit
notifications from the application how many frames were written or read.
Thus, it was assumed that the virtual device should read/write frames to
requests based on timings. And there are some problems here:
1. This was found to violate the virtio specification: if a request is
already in the queue, the device can safely read/write there at any
time.
2. It looks like this breaks the use case with swiotlb. Personally I'm
not sure how the application handles DMA ownership in the case of
mmaped buffer.
To correctly implement mmap support, instead of transferring data via a
message queue, the driver and device must have a shared memory region.
We can add mmap in the future when we expand the functionality of the
device to support such shared memory.
Best regards,
>
>
> thanks,
>
> Takashi
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
On Thu, 19 Oct 2023 03:20:19 +0200,
Anton Yakovlev wrote:
>
> Hi Takashi,
>
> On 19.10.2023 03:07, Takashi Iwai wrote:
> > On Wed, 18 Oct 2023 12:48:23 +0200,
> > Matias Ezequiel Vara Larsen wrote:
> >>
> >> This commit replaces the mmap mechanism with the copy() and
> >> fill_silence() callbacks for both capturing and playback for the
> >> virtio-sound driver. This change is required to prevent the updating of
> >> the content of a buffer that is already in the available ring.
> >>
> >> The current mechanism splits a dma buffer into descriptors that are
> >> exposed to the device. This dma buffer is shared with the user
> >> application. When the device consumes a buffer, the driver moves the
> >> request from the used ring to available ring.
> >>
> >> The driver exposes the buffer to the device without knowing if the
> >> content has been updated from the user. The section 2.8.21.1 of the
> >> virtio spec states that: "The device MAY access the descriptor chains
> >> the driver created and the memory they refer to immediately". If the
> >> device picks up buffers from the available ring just after it is
> >> notified, it happens that the content may be old.
> >>
> >> By providing the copy() callback, the driver first updates the content
> >> of the buffer, and then, exposes the buffer to the device by enqueuing
> >> it in the available ring. Thus, device always picks up a buffer that is
> >> updated. During copy(), the number of requests enqueued depends on the
> >> "pos" and "bytes" arguments. The length of each request is period_size
> >> bytes.
> >>
> >> For capturing, the driver starts by exposing all the available buffers
> >> to device. After device updates the content of a buffer, it enqueues it
> >> in the used ring. It is only after the copy() for capturing is issued
> >> that the driver re-enqueues the buffer in the available ring.
> >>
> >> Co-developed-by: Anton Yakovlev <[email protected]>
> >> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> >> ---
> >> Changelog:
> >> v1 -> v2:
> >> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
> >> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
> >> for the modified part of the buffer; this way no assumptions need to
> >> be made.
> >> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
> >> reading/writing of frames is supported.
> >> * Correct comment at virtsnd_pcm_msg_send().
> >> * v1 patch at:
> >> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=2f305b77-83e7-47b6-a461-a8ca67d0bfe2&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d5775265e7e1741ae8eb783a3cb78ed553093c1
> >>
> >> sound/virtio/virtio_pcm.c | 7 ++-
> >> sound/virtio/virtio_pcm.h | 9 ++--
> >> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
> >> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
> >> 4 files changed, 137 insertions(+), 53 deletions(-)
> >
> > Most of the code changes look good, but I wonder:
> >
> >>
> >> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> >> index c10d91fff2fb..66d67eef1bcc 100644
> >> --- a/sound/virtio/virtio_pcm.c
> >> +++ b/sound/virtio/virtio_pcm.c
> >> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> >> * only message-based transport.
> >> */
> >> vss->hw.info =
> >> - SNDRV_PCM_INFO_MMAP |
> >> - SNDRV_PCM_INFO_MMAP_VALID |
> >
> > Do we need the removal of those MMAP features inevitably?
> > Usually mmap can still work even if the driver implements the copy
> > ops. Those aren't always mutual exclusive.
>
> The driver uses a message queue to communicate with the device. Thus,
> the audio buffer is sliced into several I/O requests (= number of
> periods) of the same size (= period size).
>
> Before this, all such requests were enqueued when the substream started,
> and immediately re-enqueued once the request is completed. This approach
> made it possible to add mmap support. But for mmap there are no explicit
> notifications from the application how many frames were written or read.
> Thus, it was assumed that the virtual device should read/write frames to
> requests based on timings. And there are some problems here:
>
> 1. This was found to violate the virtio specification: if a request is
> already in the queue, the device can safely read/write there at any
> time.
> 2. It looks like this breaks the use case with swiotlb. Personally I'm
> not sure how the application handles DMA ownership in the case of
> mmaped buffer.
>
> To correctly implement mmap support, instead of transferring data via a
> message queue, the driver and device must have a shared memory region.
> We can add mmap in the future when we expand the functionality of the
> device to support such shared memory.
Ah, then this implementation might be an overkill. You're still using
the (intermediate) vmalloc buffer allocated via PCM managed mode, and
the actual data is copied from/to there. So it doesn't conflict with
the mmap operation at all.
I guess that the problem you're trying to solve (the immediate data
transfer to the queue) can be implemented rather via PCM ack callback
instead. ALSA PCM core notifies the possible data transfer via PCM
ack callback right after each change of appl_ptr or hw_ptr, including
each read/write op or mmap commit. Then the driver can check the
change of appl_ptr (or hw_ptr for capture), fetch the newly available
data, and queue it immediately.
Usually together with the use of ack callback, the driver sets
SNDRV_PCM_INFO_SYNC_APPLPTR flag. This prevents the mmap of the PCM
control record (not the audio data) and enforces the use of
SNDRV_PCM_IOCTL_SYNC_PTR ioctl instead (so that the driver always gets
the ack callback).
thanks,
Takashi
>
>
> Best regards,
>
> >
> >
> > thanks,
> >
> > Takashi
>
> --
> Anton Yakovlev
> Senior Software Engineer
>
> OpenSynergy GmbH
> Rotherstr. 20, 10245 Berlin
>
Hello Takashi,
On Thu, Oct 19, 2023 at 09:48:03AM +0200, Takashi Iwai wrote:
> On Thu, 19 Oct 2023 03:20:19 +0200,
> Anton Yakovlev wrote:
> >
> > Hi Takashi,
> >
> > On 19.10.2023 03:07, Takashi Iwai wrote:
> > > On Wed, 18 Oct 2023 12:48:23 +0200,
> > > Matias Ezequiel Vara Larsen wrote:
> > >>
> > >> This commit replaces the mmap mechanism with the copy() and
> > >> fill_silence() callbacks for both capturing and playback for the
> > >> virtio-sound driver. This change is required to prevent the updating of
> > >> the content of a buffer that is already in the available ring.
> > >>
> > >> The current mechanism splits a dma buffer into descriptors that are
> > >> exposed to the device. This dma buffer is shared with the user
> > >> application. When the device consumes a buffer, the driver moves the
> > >> request from the used ring to available ring.
> > >>
> > >> The driver exposes the buffer to the device without knowing if the
> > >> content has been updated from the user. The section 2.8.21.1 of the
> > >> virtio spec states that: "The device MAY access the descriptor chains
> > >> the driver created and the memory they refer to immediately". If the
> > >> device picks up buffers from the available ring just after it is
> > >> notified, it happens that the content may be old.
> > >>
> > >> By providing the copy() callback, the driver first updates the content
> > >> of the buffer, and then, exposes the buffer to the device by enqueuing
> > >> it in the available ring. Thus, device always picks up a buffer that is
> > >> updated. During copy(), the number of requests enqueued depends on the
> > >> "pos" and "bytes" arguments. The length of each request is period_size
> > >> bytes.
> > >>
> > >> For capturing, the driver starts by exposing all the available buffers
> > >> to device. After device updates the content of a buffer, it enqueues it
> > >> in the used ring. It is only after the copy() for capturing is issued
> > >> that the driver re-enqueues the buffer in the available ring.
> > >>
> > >> Co-developed-by: Anton Yakovlev <[email protected]>
> > >> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> > >> ---
> > >> Changelog:
> > >> v1 -> v2:
> > >> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
> > >> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
> > >> for the modified part of the buffer; this way no assumptions need to
> > >> be made.
> > >> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
> > >> reading/writing of frames is supported.
> > >> * Correct comment at virtsnd_pcm_msg_send().
> > >> * v1 patch at:
> > >> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=2f305b77-83e7-47b6-a461-a8ca67d0bfe2&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d5775265e7e1741ae8eb783a3cb78ed553093c1
> > >>
> > >> sound/virtio/virtio_pcm.c | 7 ++-
> > >> sound/virtio/virtio_pcm.h | 9 ++--
> > >> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
> > >> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
> > >> 4 files changed, 137 insertions(+), 53 deletions(-)
> > >
> > > Most of the code changes look good, but I wonder:
> > >
> > >>
> > >> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> > >> index c10d91fff2fb..66d67eef1bcc 100644
> > >> --- a/sound/virtio/virtio_pcm.c
> > >> +++ b/sound/virtio/virtio_pcm.c
> > >> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> > >> * only message-based transport.
> > >> */
> > >> vss->hw.info =
> > >> - SNDRV_PCM_INFO_MMAP |
> > >> - SNDRV_PCM_INFO_MMAP_VALID |
> > >
> > > Do we need the removal of those MMAP features inevitably?
> > > Usually mmap can still work even if the driver implements the copy
> > > ops. Those aren't always mutual exclusive.
> >
> > The driver uses a message queue to communicate with the device. Thus,
> > the audio buffer is sliced into several I/O requests (= number of
> > periods) of the same size (= period size).
> >
> > Before this, all such requests were enqueued when the substream started,
> > and immediately re-enqueued once the request is completed. This approach
> > made it possible to add mmap support. But for mmap there are no explicit
> > notifications from the application how many frames were written or read.
> > Thus, it was assumed that the virtual device should read/write frames to
> > requests based on timings. And there are some problems here:
> >
> > 1. This was found to violate the virtio specification: if a request is
> > already in the queue, the device can safely read/write there at any
> > time.
> > 2. It looks like this breaks the use case with swiotlb. Personally I'm
> > not sure how the application handles DMA ownership in the case of
> > mmaped buffer.
> >
> > To correctly implement mmap support, instead of transferring data via a
> > message queue, the driver and device must have a shared memory region.
> > We can add mmap in the future when we expand the functionality of the
> > device to support such shared memory.
>
> Ah, then this implementation might be an overkill. You're still using
> the (intermediate) vmalloc buffer allocated via PCM managed mode, and
> the actual data is copied from/to there. So it doesn't conflict with
> the mmap operation at all.
>
> I guess that the problem you're trying to solve (the immediate data
> transfer to the queue) can be implemented rather via PCM ack callback
> instead. ALSA PCM core notifies the possible data transfer via PCM
> ack callback right after each change of appl_ptr or hw_ptr, including
> each read/write op or mmap commit. Then the driver can check the
> change of appl_ptr (or hw_ptr for capture), fetch the newly available
> data, and queue it immediately.
>
> Usually together with the use of ack callback, the driver sets
> SNDRV_PCM_INFO_SYNC_APPLPTR flag. This prevents the mmap of the PCM
> control record (not the audio data) and enforces the use of
> SNDRV_PCM_IOCTL_SYNC_PTR ioctl instead (so that the driver always gets
> the ack callback).
>
>
Thanks for your comments. If I understand correctly, we have two
options:
1. Use copy/fill_silence callbacks and use my own buffers thus disabling
mmap.
2. Use mmap and the ack callback to track when appl_ptr changes thus
moving the content to the queues after it has been updated.
Am I right?
Thanks, Matias.
> thanks,
>
> Takashi
>
>
> >
> >
> > Best regards,
> >
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > --
> > Anton Yakovlev
> > Senior Software Engineer
> >
> > OpenSynergy GmbH
> > Rotherstr. 20, 10245 Berlin
> >
>
On Fri, 20 Oct 2023 11:45:39 +0200,
Matias Ezequiel Vara Larsen wrote:
>
> Hello Takashi,
>
> On Thu, Oct 19, 2023 at 09:48:03AM +0200, Takashi Iwai wrote:
> > On Thu, 19 Oct 2023 03:20:19 +0200,
> > Anton Yakovlev wrote:
> > >
> > > Hi Takashi,
> > >
> > > On 19.10.2023 03:07, Takashi Iwai wrote:
> > > > On Wed, 18 Oct 2023 12:48:23 +0200,
> > > > Matias Ezequiel Vara Larsen wrote:
> > > >>
> > > >> This commit replaces the mmap mechanism with the copy() and
> > > >> fill_silence() callbacks for both capturing and playback for the
> > > >> virtio-sound driver. This change is required to prevent the updating of
> > > >> the content of a buffer that is already in the available ring.
> > > >>
> > > >> The current mechanism splits a dma buffer into descriptors that are
> > > >> exposed to the device. This dma buffer is shared with the user
> > > >> application. When the device consumes a buffer, the driver moves the
> > > >> request from the used ring to available ring.
> > > >>
> > > >> The driver exposes the buffer to the device without knowing if the
> > > >> content has been updated from the user. The section 2.8.21.1 of the
> > > >> virtio spec states that: "The device MAY access the descriptor chains
> > > >> the driver created and the memory they refer to immediately". If the
> > > >> device picks up buffers from the available ring just after it is
> > > >> notified, it happens that the content may be old.
> > > >>
> > > >> By providing the copy() callback, the driver first updates the content
> > > >> of the buffer, and then, exposes the buffer to the device by enqueuing
> > > >> it in the available ring. Thus, device always picks up a buffer that is
> > > >> updated. During copy(), the number of requests enqueued depends on the
> > > >> "pos" and "bytes" arguments. The length of each request is period_size
> > > >> bytes.
> > > >>
> > > >> For capturing, the driver starts by exposing all the available buffers
> > > >> to device. After device updates the content of a buffer, it enqueues it
> > > >> in the used ring. It is only after the copy() for capturing is issued
> > > >> that the driver re-enqueues the buffer in the available ring.
> > > >>
> > > >> Co-developed-by: Anton Yakovlev <[email protected]>
> > > >> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> > > >> ---
> > > >> Changelog:
> > > >> v1 -> v2:
> > > >> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
> > > >> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
> > > >> for the modified part of the buffer; this way no assumptions need to
> > > >> be made.
> > > >> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
> > > >> reading/writing of frames is supported.
> > > >> * Correct comment at virtsnd_pcm_msg_send().
> > > >> * v1 patch at:
> > > >> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=2f305b77-83e7-47b6-a461-a8ca67d0bfe2&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d5775265e7e1741ae8eb783a3cb78ed553093c1
> > > >>
> > > >> sound/virtio/virtio_pcm.c | 7 ++-
> > > >> sound/virtio/virtio_pcm.h | 9 ++--
> > > >> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
> > > >> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
> > > >> 4 files changed, 137 insertions(+), 53 deletions(-)
> > > >
> > > > Most of the code changes look good, but I wonder:
> > > >
> > > >>
> > > >> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> > > >> index c10d91fff2fb..66d67eef1bcc 100644
> > > >> --- a/sound/virtio/virtio_pcm.c
> > > >> +++ b/sound/virtio/virtio_pcm.c
> > > >> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> > > >> * only message-based transport.
> > > >> */
> > > >> vss->hw.info =
> > > >> - SNDRV_PCM_INFO_MMAP |
> > > >> - SNDRV_PCM_INFO_MMAP_VALID |
> > > >
> > > > Do we need the removal of those MMAP features inevitably?
> > > > Usually mmap can still work even if the driver implements the copy
> > > > ops. Those aren't always mutual exclusive.
> > >
> > > The driver uses a message queue to communicate with the device. Thus,
> > > the audio buffer is sliced into several I/O requests (= number of
> > > periods) of the same size (= period size).
> > >
> > > Before this, all such requests were enqueued when the substream started,
> > > and immediately re-enqueued once the request is completed. This approach
> > > made it possible to add mmap support. But for mmap there are no explicit
> > > notifications from the application how many frames were written or read.
> > > Thus, it was assumed that the virtual device should read/write frames to
> > > requests based on timings. And there are some problems here:
> > >
> > > 1. This was found to violate the virtio specification: if a request is
> > > already in the queue, the device can safely read/write there at any
> > > time.
> > > 2. It looks like this breaks the use case with swiotlb. Personally I'm
> > > not sure how the application handles DMA ownership in the case of
> > > mmaped buffer.
> > >
> > > To correctly implement mmap support, instead of transferring data via a
> > > message queue, the driver and device must have a shared memory region.
> > > We can add mmap in the future when we expand the functionality of the
> > > device to support such shared memory.
> >
> > Ah, then this implementation might be an overkill. You're still using
> > the (intermediate) vmalloc buffer allocated via PCM managed mode, and
> > the actual data is copied from/to there. So it doesn't conflict with
> > the mmap operation at all.
> >
> > I guess that the problem you're trying to solve (the immediate data
> > transfer to the queue) can be implemented rather via PCM ack callback
> > instead. ALSA PCM core notifies the possible data transfer via PCM
> > ack callback right after each change of appl_ptr or hw_ptr, including
> > each read/write op or mmap commit. Then the driver can check the
> > change of appl_ptr (or hw_ptr for capture), fetch the newly available
> > data, and queue it immediately.
> >
> > Usually together with the use of ack callback, the driver sets
> > SNDRV_PCM_INFO_SYNC_APPLPTR flag. This prevents the mmap of the PCM
> > control record (not the audio data) and enforces the use of
> > SNDRV_PCM_IOCTL_SYNC_PTR ioctl instead (so that the driver always gets
> > the ack callback).
> >
> >
>
> Thanks for your comments. If I understand correctly, we have two
> options:
> 1. Use copy/fill_silence callbacks and use my own buffers thus disabling
> mmap.
> 2. Use mmap and the ack callback to track when appl_ptr changes thus
> moving the content to the queues after it has been updated.
>
> Am I right?
Not really. The ack callback is always effective no matter whether
the stream is transferred via mmap or read/write. So, when you
implement a la packet transfer via ack callback, it works for both
mmap and read/write. It's used typically for drivers doing a
packet-based transfer, e.g. FireWire drivers or USB-audio.
OTOH, if you'd like to drop the intermediate ring buffer (enabled by
snd_pcm_set_managed_buffer_all() and allocated/freed automatically at
hw_params/hw_free in PCM core), you can implement PCM copy and
fill_ops methods. But in this case, you can't copy to
runtime->dma_area like your patch; you may need a temporary buffer of
your own, or handle directly the user pointer.
And, even with copy/fill_ops implementations, mmap itself isn't
prohibited when the driver provides its own mmap call. This can be
seen, for example, when a driver uses the iomem region as the data
ring buffer.
HTH,
Takashi
> Thanks, Matias.
>
> > thanks,
> >
> > Takashi
> >
> >
> > >
> > >
> > > Best regards,
> > >
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > >
> > > --
> > > Anton Yakovlev
> > > Senior Software Engineer
> > >
> > > OpenSynergy GmbH
> > > Rotherstr. 20, 10245 Berlin
> > >
> >
>
Hi Takashi,
On 19.10.2023 16:48, Takashi Iwai wrote:
> On Thu, 19 Oct 2023 03:20:19 +0200,
> Anton Yakovlev wrote:
>>
>> Hi Takashi,
>>
>> On 19.10.2023 03:07, Takashi Iwai wrote:
>>> On Wed, 18 Oct 2023 12:48:23 +0200,
>>> Matias Ezequiel Vara Larsen wrote:
>>>>
>>>> This commit replaces the mmap mechanism with the copy() and
>>>> fill_silence() callbacks for both capturing and playback for the
>>>> virtio-sound driver. This change is required to prevent the updating of
>>>> the content of a buffer that is already in the available ring.
>>>>
>>>> The current mechanism splits a dma buffer into descriptors that are
>>>> exposed to the device. This dma buffer is shared with the user
>>>> application. When the device consumes a buffer, the driver moves the
>>>> request from the used ring to available ring.
>>>>
>>>> The driver exposes the buffer to the device without knowing if the
>>>> content has been updated from the user. The section 2.8.21.1 of the
>>>> virtio spec states that: "The device MAY access the descriptor chains
>>>> the driver created and the memory they refer to immediately". If the
>>>> device picks up buffers from the available ring just after it is
>>>> notified, it happens that the content may be old.
>>>>
>>>> By providing the copy() callback, the driver first updates the content
>>>> of the buffer, and then, exposes the buffer to the device by enqueuing
>>>> it in the available ring. Thus, device always picks up a buffer that is
>>>> updated. During copy(), the number of requests enqueued depends on the
>>>> "pos" and "bytes" arguments. The length of each request is period_size
>>>> bytes.
>>>>
>>>> For capturing, the driver starts by exposing all the available buffers
>>>> to device. After device updates the content of a buffer, it enqueues it
>>>> in the used ring. It is only after the copy() for capturing is issued
>>>> that the driver re-enqueues the buffer in the available ring.
>>>>
>>>> Co-developed-by: Anton Yakovlev <[email protected]>
>>>> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
>>>> ---
>>>> Changelog:
>>>> v1 -> v2:
>>>> * Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
>>>> * Make virtsnd_pcm_msg_send() generic by specifying the offset and size
>>>> for the modified part of the buffer; this way no assumptions need to
>>>> be made.
>>>> * Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential
>>>> reading/writing of frames is supported.
>>>> * Correct comment at virtsnd_pcm_msg_send().
>>>> * v1 patch at:
>>>> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=323acbff-2d10-45a8-bbe1-78fc8583abec&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-6526c5e588ae6e2c247d0c967e0504e4fc7f307a
>>>>
>>>> sound/virtio/virtio_pcm.c | 7 ++-
>>>> sound/virtio/virtio_pcm.h | 9 ++--
>>>> sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++-------------
>>>> sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++-----
>>>> 4 files changed, 137 insertions(+), 53 deletions(-)
>>>
>>> Most of the code changes look good, but I wonder:
>>>
>>>>
>>>> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
>>>> index c10d91fff2fb..66d67eef1bcc 100644
>>>> --- a/sound/virtio/virtio_pcm.c
>>>> +++ b/sound/virtio/virtio_pcm.c
>>>> @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
>>>> * only message-based transport.
>>>> */
>>>> vss->hw.info =
>>>> - SNDRV_PCM_INFO_MMAP |
>>>> - SNDRV_PCM_INFO_MMAP_VALID |
>>>
>>> Do we need the removal of those MMAP features inevitably?
>>> Usually mmap can still work even if the driver implements the copy
>>> ops. Those aren't always mutual exclusive.
>>
>> The driver uses a message queue to communicate with the device. Thus,
>> the audio buffer is sliced into several I/O requests (= number of
>> periods) of the same size (= period size).
>>
>> Before this, all such requests were enqueued when the substream started,
>> and immediately re-enqueued once the request is completed. This approach
>> made it possible to add mmap support. But for mmap there are no explicit
>> notifications from the application how many frames were written or read.
>> Thus, it was assumed that the virtual device should read/write frames to
>> requests based on timings. And there are some problems here:
>>
>> 1. This was found to violate the virtio specification: if a request is
>> already in the queue, the device can safely read/write there at any
>> time.
>> 2. It looks like this breaks the use case with swiotlb. Personally I'm
>> not sure how the application handles DMA ownership in the case of
>> mmaped buffer.
>>
>> To correctly implement mmap support, instead of transferring data via a
>> message queue, the driver and device must have a shared memory region.
>> We can add mmap in the future when we expand the functionality of the
>> device to support such shared memory.
>
> Ah, then this implementation might be an overkill. You're still using
> the (intermediate) vmalloc buffer allocated via PCM managed mode, and
> the actual data is copied from/to there. So it doesn't conflict with
> the mmap operation at all.
>
> I guess that the problem you're trying to solve (the immediate data
> transfer to the queue) can be implemented rather via PCM ack callback
> instead. ALSA PCM core notifies the possible data transfer via PCM
> ack callback right after each change of appl_ptr or hw_ptr, including
> each read/write op or mmap commit. Then the driver can check the
> change of appl_ptr (or hw_ptr for capture), fetch the newly available
> data, and queue it immediately.
>
> Usually together with the use of ack callback, the driver sets
> SNDRV_PCM_INFO_SYNC_APPLPTR flag. This prevents the mmap of the PCM
> control record (not the audio data) and enforces the use of
> SNDRV_PCM_IOCTL_SYNC_PTR ioctl instead (so that the driver always gets
> the ack callback).
Thanks for pointing out this possibility!
I'm wondering if TinyALSA works correctly with this flag.
Best regards,
>
>
> thanks,
>
> Takashi
>
>
>>
>>
>> Best regards,
>>
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>
>> --
>> Anton Yakovlev
>> Senior Software Engineer
>>
>> OpenSynergy GmbH
>> Rotherstr. 20, 10245 Berlin
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin