This commit uses the ack() callback to determine when a buffer has been
updated, then exposes it to guest.
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.
When the ack() callback is invoked, the driver exposes only the buffers
that have already been updated, i.e., enqueued in the available ring.
Thus, the device always picks up a buffer that is updated.
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 ack() 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: Anton Yakovlev <[email protected]>
Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
---
Changelog:
v2 -> v3:
* Use ack() callback instead of copy()/fill_silence()
* Change commit name
* Rewrite commit message
* Remove virtsnd_pcm_msg_send_locked()
* Use single callback for both capture and playback
* Fix kernel test robot warnings regarding documentation
* v2 patch at:
https://lore.kernel.org/lkml/[email protected]/T/
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 | 1 +
sound/virtio/virtio_pcm.h | 6 ++-
sound/virtio/virtio_pcm_msg.c | 80 ++++++++++++++++++++---------------
sound/virtio/virtio_pcm_ops.c | 30 +++++++++++--
4 files changed, 79 insertions(+), 38 deletions(-)
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index c10d91fff2fb..9cc5a95b4913 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -124,6 +124,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
values = le64_to_cpu(info->formats);
vss->hw.formats = 0;
+ vss->appl_ptr = 0;
for (i = 0; i < ARRAY_SIZE(g_v2a_format_map); ++i)
if (values & (1ULL << i)) {
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 062eb8e8f2cf..ea3c2845ae9b 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -27,6 +27,7 @@ struct virtio_pcm_msg;
* substream operators.
* @buffer_bytes: Current buffer size in bytes.
* @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
+ * @appl_ptr: Substream application pointer value in bytes [0 ... buffer_bytes).
* @xfer_enabled: Data transfer state (0 - off, 1 - on).
* @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
* @stopped: True if the substream is stopped and must be released on the device
@@ -51,13 +52,13 @@ struct virtio_pcm_substream {
spinlock_t lock;
size_t buffer_bytes;
size_t hw_ptr;
+ size_t appl_ptr;
bool xfer_enabled;
bool xfer_xrun;
bool stopped;
bool suspended;
struct virtio_pcm_msg **msgs;
unsigned int nmsgs;
- int msg_last_enqueued;
unsigned int msg_count;
wait_queue_head_t msg_empty;
};
@@ -117,7 +118,8 @@ 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);
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..106e8e847746 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);
@@ -181,66 +180,81 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
vss->msgs = NULL;
vss->nmsgs = 0;
+ vss->appl_ptr = 0;
}
/**
* virtsnd_pcm_msg_send() - Send asynchronous I/O messages.
* @vss: VirtIO PCM substream.
+ * @offset: starting position that has been updated
+ * @bytes: number of bytes that has been updated
*
* 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);
@@ -309,6 +323,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 +336,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..21cde37ecfa3 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,6 +450,29 @@ virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
return hw_ptr;
}
+static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ unsigned long flags;
+ struct snd_pcm_runtime *runtime = vss->substream->runtime;
+ ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
+ ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
+ int rc;
+
+ spin_lock_irqsave(&queue->lock, flags);
+ spin_lock(&vss->lock);
+
+ ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
+
+ rc = virtsnd_pcm_msg_send(vss, vss->appl_ptr % buf_size, bytes);
+ vss->appl_ptr += bytes;
+
+ spin_unlock(&vss->lock);
+ spin_unlock_irqrestore(&queue->lock, flags);
+ return rc;
+}
+
/* PCM substream operators map. */
const struct snd_pcm_ops virtsnd_pcm_ops = {
.open = virtsnd_pcm_open,
@@ -461,4 +484,5 @@ const struct snd_pcm_ops virtsnd_pcm_ops = {
.trigger = virtsnd_pcm_trigger,
.sync_stop = virtsnd_pcm_sync_stop,
.pointer = virtsnd_pcm_pointer,
+ .ack = virtsnd_pcm_ack,
};
base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
--
2.41.0
On Mon, 23 Oct 2023 17:06:57 +0200,
Matias Ezequiel Vara Larsen wrote:
>
> +static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
> +{
> + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> + unsigned long flags;
> + struct snd_pcm_runtime *runtime = vss->substream->runtime;
> + ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> + ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> + int rc;
> +
> + spin_lock_irqsave(&queue->lock, flags);
> + spin_lock(&vss->lock);
> +
> + ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
The variable declaration should be moved to the beginning of the
function.
Also, there can be a overlap beyond runtime->boundary (which easily
happens for 32bit apps), so the calculation can be a bit more complex
with conditional.
thanks,
Takashi
Hi Matias,
Thanks for the new patch!
On 24.10.2023 00:06, Matias Ezequiel Vara Larsen wrote:
> This commit uses the ack() callback to determine when a buffer has been
> updated, then exposes it to guest.
>
> 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.
>
> When the ack() callback is invoked, the driver exposes only the buffers
> that have already been updated, i.e., enqueued in the available ring.
> Thus, the device always picks up a buffer that is updated.
>
> 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 ack() 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: Anton Yakovlev <[email protected]>
> Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> ---
> Changelog:
> v2 -> v3:
> * Use ack() callback instead of copy()/fill_silence()
> * Change commit name
> * Rewrite commit message
> * Remove virtsnd_pcm_msg_send_locked()
> * Use single callback for both capture and playback
> * Fix kernel test robot warnings regarding documentation
> * v2 patch at:
> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f87y1fzkq8c.wl%2dtiwai%40suse.de%2fT%2f&umid=6a6586e6-1bcb-48d2-8c0c-75ef6bb15df9&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-090fe82db9a03f0dc8c4f214d4d2e2bf916e7f1f
> 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=6a6586e6-1bcb-48d2-8c0c-75ef6bb15df9&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d4d809544c877beff1a631a29db01290c65d879
>
> sound/virtio/virtio_pcm.c | 1 +
> sound/virtio/virtio_pcm.h | 6 ++-
> sound/virtio/virtio_pcm_msg.c | 80 ++++++++++++++++++++---------------
> sound/virtio/virtio_pcm_ops.c | 30 +++++++++++--
> 4 files changed, 79 insertions(+), 38 deletions(-)
>
> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index c10d91fff2fb..9cc5a95b4913 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -124,6 +124,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> values = le64_to_cpu(info->formats);
>
> vss->hw.formats = 0;
> + vss->appl_ptr = 0;
>
> for (i = 0; i < ARRAY_SIZE(g_v2a_format_map); ++i)
> if (values & (1ULL << i)) {
> diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
> index 062eb8e8f2cf..ea3c2845ae9b 100644
> --- a/sound/virtio/virtio_pcm.h
> +++ b/sound/virtio/virtio_pcm.h
> @@ -27,6 +27,7 @@ struct virtio_pcm_msg;
> * substream operators.
> * @buffer_bytes: Current buffer size in bytes.
> * @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
> + * @appl_ptr: Substream application pointer value in bytes [0 ... buffer_bytes).
> * @xfer_enabled: Data transfer state (0 - off, 1 - on).
> * @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
> * @stopped: True if the substream is stopped and must be released on the device
> @@ -51,13 +52,13 @@ struct virtio_pcm_substream {
> spinlock_t lock;
> size_t buffer_bytes;
> size_t hw_ptr;
> + size_t appl_ptr;
> bool xfer_enabled;
> bool xfer_xrun;
> bool stopped;
> bool suspended;
> struct virtio_pcm_msg **msgs;
> unsigned int nmsgs;
> - int msg_last_enqueued;
> unsigned int msg_count;
> wait_queue_head_t msg_empty;
> };
> @@ -117,7 +118,8 @@ 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);
>
> 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..106e8e847746 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);
>
> @@ -181,66 +180,81 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
>
> vss->msgs = NULL;
> vss->nmsgs = 0;
> + vss->appl_ptr = 0;
> }
>
> /**
> * virtsnd_pcm_msg_send() - Send asynchronous I/O messages.
> * @vss: VirtIO PCM substream.
> + * @offset: starting position that has been updated
> + * @bytes: number of bytes that has been updated
> *
> * 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);
>
> @@ -309,6 +323,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 +336,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..21cde37ecfa3 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,6 +450,29 @@ virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
> return hw_ptr;
> }
>
> +static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
> +{
> + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> + unsigned long flags;
> + struct snd_pcm_runtime *runtime = vss->substream->runtime;
> + ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> + ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> + int rc;
> +
> + spin_lock_irqsave(&queue->lock, flags);
> + spin_lock(&vss->lock);
> +
> + ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
> +
> + rc = virtsnd_pcm_msg_send(vss, vss->appl_ptr % buf_size, bytes);
> + vss->appl_ptr += bytes;
I think it makes sense to store vss->appl_ptr in frames (type
snd_pcm_uframes_t instead of size_t), and do all calculations in frames.
You could do convertion before invoking virtsnd_pcm_msg_send().
We also need to either disable rewinds (SNDRV_PCM_INFO_NO_REWINDS), or take
into account the case when the new runtime->control->appl_ptr value is less
than the old one.
Best regards,
> +
> + spin_unlock(&vss->lock);
> + spin_unlock_irqrestore(&queue->lock, flags);
> + return rc;
> +}
> +
> /* PCM substream operators map. */
> const struct snd_pcm_ops virtsnd_pcm_ops = {
> .open = virtsnd_pcm_open,
> @@ -461,4 +484,5 @@ const struct snd_pcm_ops virtsnd_pcm_ops = {
> .trigger = virtsnd_pcm_trigger,
> .sync_stop = virtsnd_pcm_sync_stop,
> .pointer = virtsnd_pcm_pointer,
> + .ack = virtsnd_pcm_ack,
> };
>
> base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
On Mon, Oct 23, 2023 at 05:50:00PM +0200, Takashi Iwai wrote:
> On Mon, 23 Oct 2023 17:06:57 +0200,
> Matias Ezequiel Vara Larsen wrote:
> >
> > +static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
> > +{
> > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> > + unsigned long flags;
> > + struct snd_pcm_runtime *runtime = vss->substream->runtime;
> > + ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> > + ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> > + int rc;
> > +
> > + spin_lock_irqsave(&queue->lock, flags);
> > + spin_lock(&vss->lock);
> > +
> > + ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
>
> The variable declaration should be moved to the beginning of the
> function.
>
> Also, there can be a overlap beyond runtime->boundary (which easily
> happens for 32bit apps), so the calculation can be a bit more complex
> with conditional.
>
Should I use as an example `cs46xx_playback/capture_transfer()` which relies on
the `snd_pcm_indirect_playback/capture_transfer()`? It looks like it
does already that calculation.
Thanks, Matias.
On Tue, Oct 24, 2023 at 09:11:10AM +0900, Anton Yakovlev wrote:
> Hi Matias,
>
> Thanks for the new patch!
>
>
>
> On 24.10.2023 00:06, Matias Ezequiel Vara Larsen wrote:
> > This commit uses the ack() callback to determine when a buffer has been
> > updated, then exposes it to guest.
> >
> > 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.
> >
> > When the ack() callback is invoked, the driver exposes only the buffers
> > that have already been updated, i.e., enqueued in the available ring.
> > Thus, the device always picks up a buffer that is updated.
> >
> > 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 ack() 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: Anton Yakovlev <[email protected]>
> > Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
> > ---
> > Changelog:
> > v2 -> v3:
> > * Use ack() callback instead of copy()/fill_silence()
> > * Change commit name
> > * Rewrite commit message
> > * Remove virtsnd_pcm_msg_send_locked()
> > * Use single callback for both capture and playback
> > * Fix kernel test robot warnings regarding documentation
> > * v2 patch at:
> > https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f87y1fzkq8c.wl%2dtiwai%40suse.de%2fT%2f&umid=6a6586e6-1bcb-48d2-8c0c-75ef6bb15df9&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-090fe82db9a03f0dc8c4f214d4d2e2bf916e7f1f
> > 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=6a6586e6-1bcb-48d2-8c0c-75ef6bb15df9&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d4d809544c877beff1a631a29db01290c65d879
> >
> > sound/virtio/virtio_pcm.c | 1 +
> > sound/virtio/virtio_pcm.h | 6 ++-
> > sound/virtio/virtio_pcm_msg.c | 80 ++++++++++++++++++++---------------
> > sound/virtio/virtio_pcm_ops.c | 30 +++++++++++--
> > 4 files changed, 79 insertions(+), 38 deletions(-)
> >
> > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> > index c10d91fff2fb..9cc5a95b4913 100644
> > --- a/sound/virtio/virtio_pcm.c
> > +++ b/sound/virtio/virtio_pcm.c
> > @@ -124,6 +124,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss,
> > values = le64_to_cpu(info->formats);
> > vss->hw.formats = 0;
> > + vss->appl_ptr = 0;
> > for (i = 0; i < ARRAY_SIZE(g_v2a_format_map); ++i)
> > if (values & (1ULL << i)) {
> > diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
> > index 062eb8e8f2cf..ea3c2845ae9b 100644
> > --- a/sound/virtio/virtio_pcm.h
> > +++ b/sound/virtio/virtio_pcm.h
> > @@ -27,6 +27,7 @@ struct virtio_pcm_msg;
> > * substream operators.
> > * @buffer_bytes: Current buffer size in bytes.
> > * @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
> > + * @appl_ptr: Substream application pointer value in bytes [0 ... buffer_bytes).
> > * @xfer_enabled: Data transfer state (0 - off, 1 - on).
> > * @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
> > * @stopped: True if the substream is stopped and must be released on the device
> > @@ -51,13 +52,13 @@ struct virtio_pcm_substream {
> > spinlock_t lock;
> > size_t buffer_bytes;
> > size_t hw_ptr;
> > + size_t appl_ptr;
> > bool xfer_enabled;
> > bool xfer_xrun;
> > bool stopped;
> > bool suspended;
> > struct virtio_pcm_msg **msgs;
> > unsigned int nmsgs;
> > - int msg_last_enqueued;
> > unsigned int msg_count;
> > wait_queue_head_t msg_empty;
> > };
> > @@ -117,7 +118,8 @@ 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);
> > 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..106e8e847746 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);
> > @@ -181,66 +180,81 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
> > vss->msgs = NULL;
> > vss->nmsgs = 0;
> > + vss->appl_ptr = 0;
> > }
> > /**
> > * virtsnd_pcm_msg_send() - Send asynchronous I/O messages.
> > * @vss: VirtIO PCM substream.
> > + * @offset: starting position that has been updated
> > + * @bytes: number of bytes that has been updated
> > *
> > * 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);
> > @@ -309,6 +323,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 +336,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..21cde37ecfa3 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,6 +450,29 @@ virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
> > return hw_ptr;
> > }
> > +static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
> > +{
> > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> > + unsigned long flags;
> > + struct snd_pcm_runtime *runtime = vss->substream->runtime;
> > + ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> > + ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> > + int rc;
> > +
> > + spin_lock_irqsave(&queue->lock, flags);
> > + spin_lock(&vss->lock);
> > +
> > + ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
> > +
> > + rc = virtsnd_pcm_msg_send(vss, vss->appl_ptr % buf_size, bytes);
> > + vss->appl_ptr += bytes;
>
> I think it makes sense to store vss->appl_ptr in frames (type
> snd_pcm_uframes_t instead of size_t), and do all calculations in frames.
> You could do convertion before invoking virtsnd_pcm_msg_send().
>
Will do, Thanks.
> We also need to either disable rewinds (SNDRV_PCM_INFO_NO_REWINDS), or take
> into account the case when the new runtime->control->appl_ptr value is less
> than the old one.
>
I am planning to disable rewinds for the moment.
Thanks, Matias.
On Tue, 24 Oct 2023 13:12:35 +0200,
Matias Ezequiel Vara Larsen wrote:
>
> On Mon, Oct 23, 2023 at 05:50:00PM +0200, Takashi Iwai wrote:
> > On Mon, 23 Oct 2023 17:06:57 +0200,
> > Matias Ezequiel Vara Larsen wrote:
> > >
> > > +static int virtsnd_pcm_ack(struct snd_pcm_substream *substream)
> > > +{
> > > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> > > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> > > + unsigned long flags;
> > > + struct snd_pcm_runtime *runtime = vss->substream->runtime;
> > > + ssize_t appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> > > + ssize_t buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> > > + int rc;
> > > +
> > > + spin_lock_irqsave(&queue->lock, flags);
> > > + spin_lock(&vss->lock);
> > > +
> > > + ssize_t bytes = (appl_pos - vss->appl_ptr) % buf_size;
> >
> > The variable declaration should be moved to the beginning of the
> > function.
> >
> > Also, there can be a overlap beyond runtime->boundary (which easily
> > happens for 32bit apps), so the calculation can be a bit more complex
> > with conditional.
> >
>
> Should I use as an example `cs46xx_playback/capture_transfer()` which relies on
> the `snd_pcm_indirect_playback/capture_transfer()`? It looks like it
> does already that calculation.
Yes, using the existing helper is a good idea.
The only problem is that this helper isn't well documented ;-<
thanks,
Takashi