From: Volker Rümelin <[email protected]> So far, only rudimentary checks have been made to ensure that the guest only performs state transitions described in virtio-v1.2-csd01 5.14.6.6.1 PCM Command Lifecycle. While this is not a Device Requirement, let's add a state variable per audio stream and check all state transitions.
Because only permitted state transitions are now possible, only one copy of the audio stream parameters is required and these do not need to be initialised with default values. The state variable will also make it easier to restore the audio stream after migration. Signed-off-by: Volker Rümelin <[email protected]> [AM: there were too many conflicts, I did `git checkout --ours -- <.>` and then reimplemented the patch idea /AM] Signed-off-by: Alexander Mikhalitsyn <[email protected]> --- v4: - switched to enum virtio_snd_pcm_state (suggested by Manos Pitsidianakis) - call virtio_snd_pcm_close() when switching to VSND_PCMSTREAM_STATE_PARAMS_SET and VSND_PCMSTREAM_STATE_RELEASED states - added virtio_error() in case when stream->state is unexpected - fixed an error message text in virtio_snd_set_pcm_params (suggestions from Marc-André Lureau) v3: - explicitly call virtio_snd_pcm_close() from unrealize - remove a call to virtio_snd_pcm_flush() from virtio_snd_pcm_close() [ ^ this was my rebase mistake ] --- hw/audio/virtio-snd.c | 211 ++++++++++++++++++++-------------- include/hw/audio/virtio-snd.h | 17 +-- 2 files changed, 123 insertions(+), 105 deletions(-) diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c index d3fc4c30877..9505e478e91 100644 --- a/hw/audio/virtio-snd.c +++ b/hw/audio/virtio-snd.c @@ -30,11 +30,25 @@ #define VIRTIO_SOUND_CHMAP_DEFAULT 0 #define VIRTIO_SOUND_HDA_FN_NID 0 +typedef enum virtio_snd_pcm_state { + VIRTIO_SND_PCM_STATE_UNINIT = 0, + VIRTIO_SND_PCM_STATE_PARAMS_SET, + VIRTIO_SND_PCM_STATE_PREPARED, + VIRTIO_SND_PCM_STATE_STARTED, + VIRTIO_SND_PCM_STATE_STOPPED, + VIRTIO_SND_PCM_STATE_RELEASED, +} virtio_snd_pcm_state; + +static inline bool virtio_snd_pcm_state_prepared(virtio_snd_pcm_state s) { + return s > VIRTIO_SND_PCM_STATE_PARAMS_SET && + s < VIRTIO_SND_PCM_STATE_RELEASED; +} + static void virtio_snd_pcm_out_cb(void *data, int available); static void virtio_snd_process_cmdq(VirtIOSound *s); static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream); +static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream); static void virtio_snd_pcm_in_cb(void *data, int available); -static void virtio_snd_unrealize(DeviceState *dev); static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8) | BIT(VIRTIO_SND_PCM_FMT_U8) @@ -129,7 +143,7 @@ static VirtIOSoundPCMStream *virtio_snd_pcm_get_stream(VirtIOSound *s, uint32_t stream_id) { return stream_id >= s->snd_conf.streams ? NULL : - s->pcm.streams[stream_id]; + &s->streams[stream_id]; } /* @@ -141,8 +155,8 @@ static VirtIOSoundPCMStream *virtio_snd_pcm_get_stream(VirtIOSound *s, static virtio_snd_pcm_set_params *virtio_snd_pcm_get_params(VirtIOSound *s, uint32_t stream_id) { - return stream_id >= s->snd_conf.streams ? NULL - : &s->pcm.pcm_params[stream_id]; + return stream_id >= s->snd_conf.streams ? NULL : + &s->streams[stream_id].params; } /* @@ -245,11 +259,10 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s, /* * Set the given stream params. - * Called by both virtio_snd_handle_pcm_set_params and during device - * initialization. * Returns the response status code. (VIRTIO_SND_S_*). * * @s: VirtIOSound device + * @stream_id: stream id * @params: The PCM params as defined in the virtio specification */ static @@ -257,14 +270,27 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, uint32_t stream_id, virtio_snd_pcm_set_params *params) { + VirtIOSoundPCMStream *stream; virtio_snd_pcm_set_params *st_params; - if (stream_id >= s->snd_conf.streams || s->pcm.pcm_params == NULL) { - virtio_error(VIRTIO_DEVICE(s), "Streams have not been initialized.\n"); + stream = virtio_snd_pcm_get_stream(s, stream_id); + if (!stream) { + virtio_error(VIRTIO_DEVICE(s), "invalid stream id: %"PRIu32, + stream_id); return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); } - st_params = virtio_snd_pcm_get_params(s, stream_id); + switch (stream->state) { + case VIRTIO_SND_PCM_STATE_UNINIT: + case VIRTIO_SND_PCM_STATE_PARAMS_SET: + case VIRTIO_SND_PCM_STATE_PREPARED: + case VIRTIO_SND_PCM_STATE_RELEASED: + break; + default: + virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, + stream->state); + return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); + } if (params->channels < 1 || params->channels > AUDIO_MAX_CHANNELS) { error_report("Number of channels is not supported."); @@ -281,6 +307,8 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP); } + st_params = virtio_snd_pcm_get_params(s, stream_id); + st_params->buffer_bytes = le32_to_cpu(params->buffer_bytes); st_params->period_bytes = le32_to_cpu(params->period_bytes); st_params->features = le32_to_cpu(params->features); @@ -289,6 +317,15 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, st_params->format = params->format; st_params->rate = params->rate; + if (virtio_snd_pcm_state_prepared(stream->state)) { + /* implicit VIRTIO_SND_R_PCM_RELEASE */ + virtio_snd_pcm_flush(stream); + } + + virtio_snd_pcm_close(stream); + + stream->state = VIRTIO_SND_PCM_STATE_PARAMS_SET; + return cpu_to_le32(VIRTIO_SND_S_OK); } @@ -398,15 +435,12 @@ static void virtio_snd_get_qemu_audsettings(audsettings *as, */ static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream) { - if (stream) { - virtio_snd_pcm_flush(stream); - if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { - audio_be_close_out(stream->s->audio_be, stream->voice.out); - stream->voice.out = NULL; - } else if (stream->info.direction == VIRTIO_SND_D_INPUT) { - audio_be_close_in(stream->s->audio_be, stream->voice.in); - stream->voice.in = NULL; - } + if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { + audio_be_close_out(stream->s->audio_be, stream->voice.out); + stream->voice.out = NULL; + } else if (stream->info.direction == VIRTIO_SND_D_INPUT) { + audio_be_close_in(stream->s->audio_be, stream->voice.in); + stream->voice.in = NULL; } } @@ -423,33 +457,26 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id) virtio_snd_pcm_set_params *params; VirtIOSoundPCMStream *stream; - if (s->pcm.streams == NULL || - s->pcm.pcm_params == NULL || - stream_id >= s->snd_conf.streams) { + stream = virtio_snd_pcm_get_stream(s, stream_id); + if (!stream) { return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); } - params = virtio_snd_pcm_get_params(s, stream_id); - if (params == NULL) { + switch (stream->state) { + case VIRTIO_SND_PCM_STATE_PARAMS_SET: + case VIRTIO_SND_PCM_STATE_PREPARED: + case VIRTIO_SND_PCM_STATE_RELEASED: + break; + default: + virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, + stream->state); return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); } - stream = virtio_snd_pcm_get_stream(s, stream_id); - if (stream == NULL) { - stream = &s->streams[stream_id]; - stream->active = false; - stream->latency_bytes = 0; - - /* - * stream_id >= s->snd_conf.streams was checked before so this is - * in-bounds - */ - s->pcm.streams[stream_id] = stream; - } + params = virtio_snd_pcm_get_params(s, stream_id); virtio_snd_get_qemu_audsettings(&as, params); stream->info.channels_max = as.nchannels; - stream->params = *params; stream->as = as; @@ -471,6 +498,8 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id) audio_be_set_volume_in_lr(s->audio_be, stream->voice.in, 0, 255, 255); } + stream->state = VIRTIO_SND_PCM_STATE_PREPARED; + return cpu_to_le32(VIRTIO_SND_S_OK); } @@ -545,7 +574,31 @@ static uint32_t virtio_snd_pcm_start_stop(VirtIOSound *s, return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); } - stream->active = start; + if (start) { + switch (stream->state) { + case VIRTIO_SND_PCM_STATE_PREPARED: + case VIRTIO_SND_PCM_STATE_STOPPED: + break; + default: + virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, + stream->state); + return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); + } + + stream->state = VIRTIO_SND_PCM_STATE_STARTED; + } else { + switch (stream->state) { + case VIRTIO_SND_PCM_STATE_STARTED: + break; + default: + virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, + stream->state); + return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); + } + + stream->state = VIRTIO_SND_PCM_STATE_STOPPED; + } + if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { audio_be_set_active_out(s->audio_be, stream->voice.out, start); } else { @@ -639,6 +692,17 @@ static void virtio_snd_handle_pcm_release(VirtIOSound *s, return; } + switch (stream->state) { + case VIRTIO_SND_PCM_STATE_PREPARED: + case VIRTIO_SND_PCM_STATE_STOPPED: + break; + default: + virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, + stream->state); + cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); + return; + } + if (virtio_snd_pcm_get_io_msgs_count(stream)) { /* * virtio-v1.2-csd01, 5.14.6.6.5.1, @@ -653,6 +717,10 @@ static void virtio_snd_handle_pcm_release(VirtIOSound *s, virtio_snd_pcm_flush(stream); } + virtio_snd_pcm_close(stream); + + stream->state = VIRTIO_SND_PCM_STATE_RELEASED; + cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK); } @@ -874,12 +942,11 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) } stream_id = le32_to_cpu(hdr.stream_id); - if (stream_id >= vsnd->snd_conf.streams - || vsnd->pcm.streams[stream_id] == NULL) { + if (stream_id >= vsnd->snd_conf.streams) { goto tx_err; } - stream = vsnd->pcm.streams[stream_id]; + stream = &vsnd->streams[stream_id]; if (stream->info.direction != VIRTIO_SND_D_OUTPUT) { goto tx_err; } @@ -954,13 +1021,12 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) } stream_id = le32_to_cpu(hdr.stream_id); - if (stream_id >= vsnd->snd_conf.streams - || !vsnd->pcm.streams[stream_id]) { + if (stream_id >= vsnd->snd_conf.streams) { goto rx_err; } - stream = vsnd->pcm.streams[stream_id]; - if (stream == NULL || stream->info.direction != VIRTIO_SND_D_INPUT) { + stream = &vsnd->streams[stream_id]; + if (stream->info.direction != VIRTIO_SND_D_INPUT) { goto rx_err; } @@ -1019,8 +1085,6 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) ERRP_GUARD(); VirtIOSound *vsnd = VIRTIO_SND(dev); VirtIODevice *vdev = VIRTIO_DEVICE(dev); - virtio_snd_pcm_set_params default_params = { 0 }; - uint32_t status; trace_virtio_snd_realize(vsnd); @@ -1057,6 +1121,7 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { VirtIOSoundPCMStream *stream = &vsnd->streams[i]; + stream->state = VIRTIO_SND_PCM_STATE_UNINIT; stream->s = vsnd; QSIMPLEQ_INIT(&stream->queue); stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID; @@ -1070,21 +1135,9 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) stream->info.channels_max = 2; } - vsnd->pcm.streams = - g_new0(VirtIOSoundPCMStream *, vsnd->snd_conf.streams); - vsnd->pcm.pcm_params = - g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams); - virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config)); virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1); - /* set default params for all streams */ - default_params.features = 0; - default_params.buffer_bytes = cpu_to_le32(8192); - default_params.period_bytes = cpu_to_le32(2048); - default_params.channels = 2; - default_params.format = VIRTIO_SND_PCM_FMT_S16; - default_params.rate = VIRTIO_SND_PCM_RATE_48000; vsnd->queues[VIRTIO_SND_VQ_CONTROL] = virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl); vsnd->queues[VIRTIO_SND_VQ_EVENT] = @@ -1095,28 +1148,6 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer); QTAILQ_INIT(&vsnd->cmdq); QSIMPLEQ_INIT(&vsnd->invalid); - - for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { - status = virtio_snd_set_pcm_params(vsnd, i, &default_params); - if (status != cpu_to_le32(VIRTIO_SND_S_OK)) { - error_setg(errp, - "Can't initialize stream params, device responded with %s.", - print_code(status)); - goto error_cleanup; - } - status = virtio_snd_pcm_prepare(vsnd, i); - if (status != cpu_to_le32(VIRTIO_SND_S_OK)) { - error_setg(errp, - "Can't prepare streams, device responded with %s.", - print_code(status)); - goto error_cleanup; - } - } - - return; - -error_cleanup: - virtio_snd_unrealize(dev); } static inline void update_latency(VirtIOSoundPCMStream *s, size_t used) @@ -1165,7 +1196,7 @@ static void virtio_snd_pcm_out_cb(void *data, int available) if (!virtio_queue_ready(buffer->vq)) { return; } - if (!stream->active) { + if (stream->state != VIRTIO_SND_PCM_STATE_STARTED) { /* Stream has stopped, so do not perform audio_be_write. */ return_tx_buffer(stream, buffer); continue; @@ -1259,7 +1290,7 @@ static void virtio_snd_pcm_in_cb(void *data, int available) if (!virtio_queue_ready(buffer->vq)) { return; } - if (!stream->active) { + if (stream->state != VIRTIO_SND_PCM_STATE_STARTED) { /* Stream has stopped, so do not perform audio_be_read. */ return_rx_buffer(stream, buffer); continue; @@ -1332,17 +1363,16 @@ static void virtio_snd_unrealize(DeviceState *dev) qemu_del_vm_change_state_handler(vsnd->vmstate); trace_virtio_snd_unrealize(vsnd); - if (vsnd->pcm.streams) { + if (vsnd->streams) { + virtio_snd_process_cmdq(vsnd); for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { - stream = vsnd->pcm.streams[i]; - if (stream) { - virtio_snd_process_cmdq(stream->s); - virtio_snd_pcm_close(stream); + stream = &vsnd->streams[i]; + if (virtio_snd_pcm_state_prepared(stream->state)) { + virtio_snd_pcm_flush(stream); } + virtio_snd_pcm_close(stream); } - g_free(vsnd->pcm.streams); } - g_free(vsnd->pcm.pcm_params); g_free(vsnd->streams); vsnd->streams = NULL; virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]); @@ -1375,6 +1405,9 @@ static void virtio_snd_reset(VirtIODevice *vdev) VirtIOSoundPCMStream *stream = &vsnd->streams[i]; VirtIOSoundPCMBuffer *buffer; + virtio_snd_pcm_close(stream); + stream->state = VIRTIO_SND_PCM_STATE_UNINIT; + while ((buffer = QSIMPLEQ_FIRST(&stream->queue))) { QSIMPLEQ_REMOVE_HEAD(&stream->queue, entry); virtio_snd_pcm_buffer_free(buffer); diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h index 97ffaae18be..85d5d7c8619 100644 --- a/include/hw/audio/virtio-snd.h +++ b/include/hw/audio/virtio-snd.h @@ -75,8 +75,6 @@ typedef struct VirtIOSoundPCMStream VirtIOSoundPCMStream; typedef struct virtio_snd_ctrl_command virtio_snd_ctrl_command; -typedef struct VirtIOSoundPCM VirtIOSoundPCM; - typedef struct VirtIOSoundPCMBuffer VirtIOSoundPCMBuffer; /* @@ -121,28 +119,16 @@ struct VirtIOSoundPCMBuffer { uint8_t data[]; }; -struct VirtIOSoundPCM { - /* - * PCM parameters are a separate field instead of a VirtIOSoundPCMStream - * field, because the operation of PCM control requests is first - * VIRTIO_SND_R_PCM_SET_PARAMS and then VIRTIO_SND_R_PCM_PREPARE; this - * means that some times we get parameters without having an allocated - * stream yet. - */ - virtio_snd_pcm_set_params *pcm_params; - VirtIOSoundPCMStream **streams; -}; - struct VirtIOSoundPCMStream { virtio_snd_pcm_info info; virtio_snd_pcm_set_params params; + uint32_t state; VirtIOSound *s; audsettings as; union { SWVoiceIn *in; SWVoiceOut *out; } voice; - bool active; uint32_t latency_bytes; QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue; }; @@ -209,7 +195,6 @@ struct VirtIOSound { VirtQueue *queues[VIRTIO_SND_VQ_MAX]; uint64_t features; - VirtIOSoundPCM pcm; VirtIOSoundPCMStream *streams; AudioBackend *audio_be; VMChangeStateEntry *vmstate; -- 2.47.3
