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 <anton.yakovlev@xxxxxxxxxxxxxxx> > > Signed-off-by: Anton Yakovlev <anton.yakovlev@xxxxxxxxxxxxxxx> > > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@xxxxxxxxxx> > > --- > > 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.
