Re: [PATCH v8 10/12] virtio-sound: implement audio output (TX)

[Philippe Mathieu-Daudé]

On 28/8/23 21:55, Emmanouil Pitsidianakis wrote:
> Handle output IO messages in the transmit (TX) virtqueue.
>
> It allocates a VirtIOSoundPCMBlock for each IO message and copies the
> data buffer to it. When the IO buffer is written to the host's sound
> card, the guest will be notified that it has been consumed.
>
> The lifetime of an IO message is:
>
> 1. Guest sends IO message to TX virtqueue.
> 2. QEMU adds it to the appropriate stream's IO buffer queue.
> 3. Sometime later, the host audio backend calls the output callback,
>     virtio_snd_pcm_out_cb(), which is defined with an AUD_open_out()
>     call. The callback gets an available number of bytes the backend can
>     receive. Then it writes data from the IO buffer queue to the backend.
>     If at any time a buffer is exhausted, it is returned to the guest as
>     completed.
> 4. If the guest releases the stream, its buffer queue is flushed by
>     attempting to write any leftover data to the audio backend and
>     releasing all IO messages back to the guest. This is how according to
>     the spec the guest knows the release was successful.
>
> Based-on: 
> https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
> Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
> Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/virtio/trace-events         |   2 +
>   hw/virtio/virtio-snd.c         | 262 ++++++++++++++++++++++++++++++++-
>   include/hw/virtio/virtio-snd.h |  11 ++
>   3 files changed, 271 insertions(+), 4 deletions(-)


>   /*
> - * Handles VIRTIO_SND_R_PCM_RELEASE. Releases the buffer resources allocated to
> - * a stream.
> + * Returns the number of I/O messages that are being processed.
> + *
> + * @stream: VirtIOSoundPCMStream
> + */
> +static size_t virtio_snd_pcm_get_pending_io_msgs(VirtIOSoundPCMStream *stream)
> +{
> +    VirtIOSoundPCMBlock *block;
> +    VirtIOSoundPCMBlock *next;
> +    size_t size = 0;
> +
> +    WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> +        QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
> +            size += 1;

Can you add a comment explaining this magic size?

> +        }
> +    }
> +    return size;
> +}


> +/*
> + * The tx virtqueue handler. Makes the buffers available to their respective
> + * streams for consumption.
> + *
> + * @vdev: VirtIOSound device
> + * @vq: tx virtqueue
> + */
> +static void virtio_snd_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOSound *s = VIRTIO_SND(vdev);
> +    VirtIOSoundPCMStream *stream = NULL;
> +    VirtQueueElement *elem;
> +    size_t sz;
> +    virtio_snd_pcm_xfer hdr;
> +    virtio_snd_pcm_status resp = { 0 };

virtio_snd_pcm_status has multiple fields, so better zero-initialize
all of them with '{ }'.

> +
> +    trace_virtio_snd_handle_xfer();
> +
> +    for (;;) {
> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +        if (!elem) {
> +            break;
> +        }
> +        /* get the message hdr object */
> +        sz = iov_to_buf(elem->out_sg,
> +                        elem->out_num,
> +                        0,
> +                        &hdr,
> +                        sizeof(hdr));
> +        if (sz != sizeof(hdr)
> +            || hdr.stream_id >= s->snd_conf.streams
> +            || !s->pcm->streams[hdr.stream_id]) {
> +            goto tx_err;
> +        }
> +
> +        stream = s->pcm->streams[hdr.stream_id];
> +        if (stream->direction != VIRTIO_SND_D_OUTPUT) {
> +            goto tx_err;
> +        }
> +
> +        WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> +            virtio_snd_pcm_read_write(stream,
> +                    vq,
> +                    elem,
> +                    hdr.stream_id == VIRTIO_SND_D_INPUT);
> +
> +            resp.status = VIRTIO_SND_S_OK;
> +            iov_from_buf(elem->in_sg,
> +                         elem->in_num,
> +                         0,
> +                         &resp,
> +                         sizeof(resp));
> +        }
> +        continue;
> +
> +tx_err:
> +        WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> +            resp.status = VIRTIO_SND_S_BAD_MSG;
> +            iov_from_buf(elem->in_sg,
> +                         elem->in_num,
> +                         0,
> +                         &resp,
> +                         sizeof(resp));
> +        }
> +    }
> +
> +    /*
> +     * Notify vq about virtio_snd_pcm_status responses.
> +     * Buffer responses must be notified separately later.
> +     */
> +    virtio_notify(VIRTIO_DEVICE(s), vq);
> +}


> +/*
> + * AUD_* output callback.
> + *
> + * @data: VirtIOSoundPCMStream stream
> + * @available: number of bytes that can be written with AUD_write()
> + */
> +static void virtio_snd_pcm_out_cb(void *data, int available)
> +{
> +    VirtIOSoundPCMStream *stream = data;
> +    VirtIOSoundPCMBlock *block;
> +    VirtIOSoundPCMBlock *next;
> +    size_t size;
> +
> +    WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> +        QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
> +            for (;;) {
> +                size = MIN(block->size, available);
> +                size = AUD_write(stream->voice.out,
> +                        block->data + block->offset,
> +                        size);

If AUD_write() returns 0, is this an infinite loop?

> +                block->size -= size;
> +                block->offset += size;
> +                if (!block->size) {
> +                    virtqueue_push(block->vq,
> +                            block->elem,
> +                            sizeof(block->elem));
> +                    virtio_notify(VIRTIO_DEVICE(stream->s),
> +                            block->vq);
> +                    QSIMPLEQ_REMOVE_HEAD(&stream->queue, entry);
> +                    g_free(block);
> +                    available -= size;
> +                    break;
> +                }
> +
> +                available -= size;
> +                if (!available) {
> +                    break;
> +                }
> +            }
> +            if (!available) {
> +                break;
> +            }
> +        }
> +    }
> +}
> +
> +/*
> + * Flush all buffer data from this stream's queue into the driver's virtual
> + * queue.
> + *
> + * @stream: VirtIOSoundPCMStream *stream
> + */
> +static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
> +{
> +    VirtIOSoundPCMBlock *block;
> +    VirtIOSoundPCMBlock *next;
> +
> +    WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> +        QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
> +            AUD_write(stream->voice.out, block->data + block->offset, 
> block->size);

Is it OK to ignore AUD_write() returning < block->size?
If so, can you add a comment please?

> +            virtqueue_push(block->vq, block->elem, sizeof(block->elem));
> +            virtio_notify(VIRTIO_DEVICE(stream->s), block->vq);
> +            QSIMPLEQ_REMOVE(&stream->queue, block, VirtIOSoundPCMBlock, entry);
> +        }
> +    }
> +}