mbox series

[v5,00/12] Add VIRTIO sound card

Message ID cover.1690626150.git.manos.pitsidianakis@linaro.org
Headers show
Series Add VIRTIO sound card | expand

Message

Manos Pitsidianakis July 29, 2023, 10:36 a.m. UTC 
This patch series adds an audio device implementing the recent virtio 
sound spec (1.2) and a corresponding PCI wrapper device.

https://github.com/epilys/qemu-virtio-snd/tree/sound-v6

Main differences with v4 patch [^v4]
<cover.1689857559.git.manos.pitsidianakis@linaro.org>:
- Use ERRP_GUARD() to propagate errors.
- Use virtio_add_feature() instead of XORing constants.
- Use %zu format specifier for size_t.

[^v4]: 
https://lore.kernel.org/qemu-devel/cover.1689857559.git.manos.pitsidianakis@linaro.org/

Previously:

[^v3]: 
https://lore.kernel.org/qemu-devel/cover.1689692765.git.manos.pitsidianakis@linaro.org/

Emmanouil Pitsidianakis (12):
  Add virtio-sound device stub
  Add virtio-sound-pci device
  virtio-sound: handle control messages and streams
  virtio-sound: set PCM stream parameters
  virtio-sound: prepare PCM streams
  virtio-sound: handle VIRTIO_SND_R_PCM_INFO request
  virtio-sound: handle VIRTIO_SND_R_PCM_{START,STOP}
  virtio-sound: handle VIRTIO_SND_PCM_SET_PARAMS
  virtio-sound: handle VIRTIO_SND_R_PCM_PREPARE
  virtio-sound: handle VIRTIO_SND_PCM_RELEASE
  virtio-sound: implement audio output (TX)
  virtio-sound: implement audio capture (RX)

 MAINTAINERS                    |    6 +
 hw/virtio/Kconfig              |    5 +
 hw/virtio/meson.build          |    2 +
 hw/virtio/trace-events         |   20 +
 hw/virtio/virtio-snd-pci.c     |   91 +++
 hw/virtio/virtio-snd.c         | 1300 ++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h           |    1 +
 include/hw/virtio/virtio-snd.h |  158 ++++
 softmmu/qdev-monitor.c         |    1 +
 9 files changed, 1584 insertions(+)
 create mode 100644 hw/virtio/virtio-snd-pci.c
 create mode 100644 hw/virtio/virtio-snd.c
 create mode 100644 include/hw/virtio/virtio-snd.h

Range-diff against v4:
 1:  ae372de565 !  1:  899b9a06bb Add virtio-sound device stub
    @@ hw/virtio/virtio-snd.c (new)
     +     * Feature Bits
     +     * None currently defined.
     +     */
    ++    VirtIOSound *s = VIRTIO_SND(vdev);
    ++    features |= s->features;
    ++
     +    trace_virtio_snd_get_features(vdev, features);
    -+    return features | 1UL << VIRTIO_F_VERSION_1 | 1UL << VIRTIO_F_IN_ORDER;
    ++
    ++    return features;
     +}
     +
     +static void virtio_snd_common_realize(DeviceState *dev,
    @@ hw/virtio/virtio-snd.c (new)
     +    VirtIOSound *vsnd = VIRTIO_SND(dev);
     +
     +    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
    ++    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
     +
     +    /* set number of jacks and streams */
     +    if (vsnd->snd_conf.jacks > 8) {
    @@ hw/virtio/virtio-snd.c (new)
     +
     +static void virtio_snd_realize(DeviceState *dev, Error **errp)
     +{
    ++    ERRP_GUARD();
     +    VirtIOSound *vsnd = VIRTIO_SND(dev);
    -+    Error *err = NULL;
     +
     +    vsnd->vmstate =
     +        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
    @@ hw/virtio/virtio-snd.c (new)
     +                              virtio_snd_handle_queue,
     +                              virtio_snd_handle_queue,
     +                              virtio_snd_handle_queue,
    -+                              &err);
    -+    if (err != NULL) {
    -+        error_propagate(errp, err);
    -+        return;
    -+    }
    ++                              errp);
     +}
     +
     +static void virtio_snd_unrealize(DeviceState *dev)
    @@ include/hw/virtio/virtio-snd.h (new)
     +typedef struct VirtIOSound {
     +    VirtIODevice parent_obj;
     +    VirtQueue *queues[VIRTIO_SND_VQ_MAX];
    ++    uint64_t features;
     +    QEMUSoundCard card;
     +    VMChangeStateEntry *vmstate;
     +    virtio_snd_config snd_conf;
 2:  5cde5472ea =  2:  035be510f8 Add virtio-sound-pci device
 3:  dc65cac2f4 !  3:  e6a3624f89 virtio-sound: handle control messages and streams
    @@ hw/virtio/virtio-snd.c: virtio_snd_set_config(VirtIODevice *vdev, const uint8_t
      static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
                                   Error **errp)
     @@ hw/virtio/virtio-snd.c: static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
    -     return features | 1UL << VIRTIO_F_VERSION_1 | 1UL << VIRTIO_F_IN_ORDER;
    +     return features;
      }
      
     +static void virtio_snd_set_pcm(VirtIOSound *snd)
    @@ hw/virtio/virtio-snd.c: static void virtio_snd_common_realize(DeviceState *dev,
     +    virtio_snd_set_pcm(vsnd);
     +
          virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
    +     virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
      
    -     /* set number of jacks and streams */
     @@ hw/virtio/virtio-snd.c: static void virtio_snd_common_realize(DeviceState *dev,
          vsnd->queues[VIRTIO_SND_VQ_EVENT] = virtio_add_queue(vdev, 64, evt);
          vsnd->queues[VIRTIO_SND_VQ_TX] = virtio_add_queue(vdev, 64, txq);
    @@ hw/virtio/virtio-snd.c: static void virtio_snd_common_realize(DeviceState *dev,
      
      static void
     @@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, Error **errp)
    +     ERRP_GUARD();
          VirtIOSound *vsnd = VIRTIO_SND(dev);
    -     Error *err = NULL;
      
     +    vsnd->pcm = NULL;
          vsnd->vmstate =
    @@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, Error *
     +                              virtio_snd_handle_event,
     +                              virtio_snd_handle_xfer,
     +                              virtio_snd_handle_xfer,
    -                               &err);
    -     if (err != NULL) {
    -         error_propagate(errp, err);
    --        return;
    -     }
    +                               errp);
      }
      
     +/*
    @@ include/hw/virtio/virtio-snd.h: typedef struct virtio_snd_pcm_xfer virtio_snd_pc
     +struct VirtIOSound {
          VirtIODevice parent_obj;
          VirtQueue *queues[VIRTIO_SND_VQ_MAX];
    +     uint64_t features;
     +    VirtIOSoundPCM *pcm;
          QEMUSoundCard card;
          VMChangeStateEntry *vmstate;
 4:  bbd1799fc9 =  4:  82c80e2ae4 virtio-sound: set PCM stream parameters
 5:  68a13c4385 =  5:  19d95e8411 virtio-sound: prepare PCM streams
 6:  f267d41957 !  6:  78777fb46f virtio-sound: handle VIRTIO_SND_R_PCM_INFO request
    @@ hw/virtio/virtio-snd.c: static VirtIOSoundPCMParams *virtio_snd_pcm_get_params(V
     +
     +    if (iov_size(cmd->elem->in_sg, cmd->elem->in_num) <
     +        sizeof(virtio_snd_hdr) + req.size * req.count) {
    -+        error_report("pcm info: buffer too small, got: %lu, needed: %lu",
    ++        error_report("pcm info: buffer too small, got: %zu, needed: %zu",
     +                iov_size(cmd->elem->in_sg, cmd->elem->in_num),
     +                sizeof(virtio_snd_pcm_info));
     +        cmd->resp.code = VIRTIO_SND_S_BAD_MSG;
 7:  5939a6161e =  7:  7641e21ee8 virtio-sound: handle VIRTIO_SND_R_PCM_{START,STOP}
 8:  8f78d3a132 =  8:  48f9b776f5 virtio-sound: handle VIRTIO_SND_PCM_SET_PARAMS
 9:  d2b3854084 =  9:  cdb8ab3ee2 virtio-sound: handle VIRTIO_SND_R_PCM_PREPARE
10:  4e14b2d129 = 10:  bbc11d0348 virtio-sound: handle VIRTIO_SND_PCM_RELEASE
11:  65442a1cd1 ! 11:  d5fb4b058c virtio-sound: implement audio output (TX)
    @@ Metadata
      ## Commit message ##
         virtio-sound: implement audio output (TX)
     
    -    Handle output IO messages in the receive (RX) virtqueue.
    +    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
    @@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, Error *
     -                              virtio_snd_handle_xfer,
     +                              virtio_snd_handle_tx,
                                    virtio_snd_handle_xfer,
    -                               &err);
    -     if (err != NULL) {
    -@@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, Error **errp)
    -     }
    +                               errp);
      }
      
     +/*
12:  aeb0caf7dc ! 12:  e2c30e2a21 virtio-sound: implement audio capture (RX)
    @@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, Error *
     -                              virtio_snd_handle_xfer,
     +                              virtio_snd_handle_tx_xfer,
     +                              virtio_snd_handle_rx_xfer,
    -                               &err);
    -     if (err != NULL) {
    -         error_propagate(errp, err);
    +                               errp);
    + }
    + 
     @@ hw/virtio/virtio-snd.c: static void virtio_snd_pcm_out_cb(void *data, int available)
      }
      

base-commit: ccb86f079a9e4d94918086a9df18c1844347aff8

Comments

Marc-André Lureau July 29, 2023, 11 a.m. UTC | #1 
Hi

On Sat, Jul 29, 2023 at 2:39 PM Emmanouil Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> +
> +/*
> + * Prepares a VirtIOSound card stream.
> + * Returns the response status code. (VIRTIO_SND_S_*).
> + *
> + * @s: VirtIOSound device
> + * @stream_id: stream id
> + */
> +static uint32_t virtio_snd_pcm_prepare_impl(VirtIOSound *s, uint32_t stream_id)
> +{
> +    audsettings as;
> +    VirtIOSoundPCMParams *params;
> +    VirtIOSoundPCMStream *stream;
> +
> +    if (!s->pcm->streams ||
> +        !s->pcm->pcm_params ||
> +        !s->pcm->pcm_params[stream_id]) {
> +        return VIRTIO_SND_S_BAD_MSG;
> +    }
> +
> +    params = virtio_snd_pcm_get_params(s, stream_id);
> +    if (!params) {
> +        return VIRTIO_SND_S_BAD_MSG;
> +    }
> +
> +    virtio_snd_get_qemu_audsettings(&as, params);
> +
> +    stream = g_new0(VirtIOSoundPCMStream, 1);
> +
> +    stream->id = stream_id;
> +    stream->pcm = s->pcm;
> +    stream->direction = stream_id < s->snd_conf.streams / 2 +
> +        (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
> +    stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
> +    stream->features = 0;
> +    stream->channels_min = 1;
> +    stream->channels_max = as.nchannels;
> +    stream->formats = supported_formats;
> +    stream->rates = supported_rates;
> +    stream->s = s;
> +
> +    stream->buffer_bytes = params->buffer_bytes;
> +    stream->period_bytes = params->period_bytes;
> +
> +    stream->positions[0] = VIRTIO_SND_CHMAP_FL;
> +    stream->positions[1] = VIRTIO_SND_CHMAP_FR;
> +
> +    stream->as = as;
> +    stream->desired_as = stream->as;
> +    qemu_mutex_init(&stream->queue_mutex);
> +    QSIMPLEQ_INIT(&stream->queue);
> +
> +    s->pcm->streams[stream_id] = stream;

Same remark as v4:
Shouldn't it close & free the existing stream? Or return an error?
Manos Pitsidianakis July 29, 2023, 11:27 a.m. UTC | #2 
On Sat, 29 Jul 2023 14:00, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> +
>> +    s->pcm->streams[stream_id] = stream;
>
>Same remark as v4:
>Shouldn't it close & free the existing stream? Or return an error?
>

Hello, yes you are correct. I missed that comment on the v4 reviews :)

Will send a v6 with a fix, I will wait a bit in case any other patch 
gets a Reviewed-By this time.