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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.