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

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 13:26, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >   /*
> > - * 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?

It's not magic, it's simply how many messages there are as explained in 
the function doc comment. This was previously bytes hence `size`. I will 
change the variable name to `count`.

> > +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 '{ }'.

I don't understand why, virtio_snd_pcm_status has two int fields hence { 
0 } zero-initializes all of them.

> > +/*
> > + * 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?

Hm since we have available > 0 bytes this wouldn't theoretically happen, 
but I see there are code paths that return 0 on bugs/failures, I will 
add the check.

> > +                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?

This is a flush event with a timeout so it should complete asap. As 
mentioned in another reply it might be better to copy the data to a 
buffer in order not to lose any audio bytes.

Thank you for the feedback,
Manos