|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH v8 10/12] virtio-sound: implement audio output (TX) |
Date: | Mon, 4 Sep 2023 13:39:56 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 |
On 4/9/23 12:34, Manos Pitsidianakis wrote:
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`.
Ah OK. 'msg_processed'?
+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.
Hmm I'm too busy right now to check the C standard. Looking at QEMU code base, most of the time we use nothing or N * 0 to initialize N fields: $ git grep '{ }' | wc -l 536 $ git grep -E '{ ?0, 0 ?}' | wc -l 50 $ git grep '{ }' | wc -l 536 Anyway, not a blocker.
+/* + * 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.
Thanks.
+ 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.
Maybe this could do: /* * This is a flush event with a timeout which should complete ASAP. * TODO: Copy the data to a buffer in order not to lose any audio * bytes. (Meanwhile we just discard any write failure). */ Regards, Phil.
[Prev in Thread] | Current Thread | [Next in Thread] |