Re: [PATCH v2 6/9] ALSA: virtio: PCM substream operators

From: Guennadi Liakhovetski
Date: Mon Jan 25 2021 - 12:06:25 EST

Next message: Tudor.Ambarus: "Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default"
Previous message: Petr Mladek: "[GIT PULL] printk urgent fix for 5.11-rc6"
In reply to: Anton Yakovlev: "[PATCH v2 6/9] ALSA: virtio: PCM substream operators"
Next in thread: Guennadi Liakhovetski: "Re: [PATCH v2 6/9] ALSA: virtio: PCM substream operators"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, 24 Jan 2021, Anton Yakovlev wrote:

Introduce the operators required for the operation of substreams.

Signed-off-by: Anton Yakovlev <anton.yakovlev@xxxxxxxxxxxxxxx>
---
sound/virtio/Makefile | 3 +-
sound/virtio/virtio_pcm.c | 5 +-
sound/virtio/virtio_pcm.h | 2 +
sound/virtio/virtio_pcm_ops.c | 513 ++++++++++++++++++++++++++++++++++
4 files changed, 521 insertions(+), 2 deletions(-)
create mode 100644 sound/virtio/virtio_pcm_ops.c

[snip]

diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
new file mode 100644
index 000000000000..19882777fcd6
--- /dev/null
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -0,0 +1,513 @@

[snip]

+/**
+ * virtsnd_pcm_release() - Release the PCM substream on the device side.
+ * @substream: VirtIO substream.
+ *
+ * Context: Any context that permits to sleep.
+ * Return: 0 on success, -errno on failure.
+ */
+static inline bool virtsnd_pcm_released(struct virtio_pcm_substream *substream)
+{
+ /*
+ * The spec states that upon receipt of the RELEASE command "the device
+ * MUST complete all pending I/O messages for the specified stream ID".
+ * Thus, we consider the absence of I/O messages in the queue as an
+ * indication that the substream has been released.
+ */
+ return atomic_read(&substream->msg_count) == 0;

Also here having it atomic doesn't really seem to help. This just means, that at some point of time it was == 0.

+}
+
+static int virtsnd_pcm_release(struct virtio_pcm_substream *substream)

kernel-doc missing

+{
+ struct virtio_snd *snd = substream->snd;
+ struct virtio_snd_msg *msg;
+ unsigned int js = msecs_to_jiffies(msg_timeout_ms);
+ int rc;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(substream, VIRTIO_SND_R_PCM_RELEASE,
+ GFP_KERNEL);
+ if (IS_ERR(msg))
+ return PTR_ERR(msg);
+
+ rc = virtsnd_ctl_msg_send_sync(snd, msg);
+ if (rc)
+ return rc;
+
+ return wait_event_interruptible_timeout(substream->msg_empty,
+ virtsnd_pcm_released(substream),
+ js);
+}
+
+/**
+ * virtsnd_pcm_open() - Open the PCM substream.
+ * @substream: Kernel ALSA substream.
+ *
+ * Context: Any context.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm *pcm = snd_pcm_substream_chip(substream);
+ struct virtio_pcm_substream *ss = NULL;
+
+ if (pcm) {
+ switch (substream->stream) {
+ case SNDRV_PCM_STREAM_PLAYBACK:
+ case SNDRV_PCM_STREAM_CAPTURE: {
+ struct virtio_pcm_stream *stream =
+ &pcm->streams[substream->stream];
+
+ if (substream->number < stream->nsubstreams)

Can this condition ever be false?

+ ss = stream->substreams[substream->number];
+ break;
+ }
+ }
+ }
+
+ if (!ss)
+ return -EBADFD;
+
+ substream->runtime->hw = ss->hw;
+ substream->private_data = ss;
+
+ return 0;
+}
+
+/**
+ * virtsnd_pcm_close() - Close the PCM substream.
+ * @substream: Kernel ALSA substream.
+ *
+ * Context: Any context.
+ * Return: 0.
+ */
+static int virtsnd_pcm_close(struct snd_pcm_substream *substream)
+{
+ return 0;
+}
+
+/**
+ * virtsnd_pcm_hw_params() - Set the parameters of the PCM substream.
+ * @substream: Kernel ALSA substream.
+ * @hw_params: Hardware parameters (can be NULL).
+ *
+ * The function can be called both from the upper level (in this case,
+ * @hw_params is not NULL) or from the driver itself (in this case, @hw_params
+ * is NULL, and the parameter values are taken from the runtime structure).
+ *
+ * In all cases, the function:
+ * 1. checks the state of the virtqueue and, if necessary, tries to fix it,
+ * 2. sets the parameters on the device side,
+ * 3. allocates a hardware buffer and I/O messages.
+ *
+ * Context: Any context that permits to sleep.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *hw_params)
+{
+ struct snd_pcm_runtime *runtime = substream->runtime;
+ struct virtio_pcm_substream *ss = snd_pcm_substream_chip(substream);
+ struct virtio_device *vdev = ss->snd->vdev;
+ struct virtio_snd_msg *msg;
+ struct virtio_snd_pcm_set_params *request;
+ snd_pcm_format_t format;
+ unsigned int channels;
+ unsigned int rate;
+ unsigned int buffer_bytes;
+ unsigned int period_bytes;
+ unsigned int periods;
+ unsigned int i;
+ int vformat = -1;
+ int vrate = -1;
+ int rc;
+
+ /*
+ * If we got here after ops->trigger() was called, the queue may
+ * still contain messages. In this case, we need to release the
+ * substream first.
+ */
+ if (atomic_read(&ss->msg_count)) {
+ rc = virtsnd_pcm_release(ss);
+ if (rc) {
+ dev_err(&vdev->dev,
+ "SID %u: invalid I/O queue state\n",
+ ss->sid);
+ return rc;
+ }
+ }
+
+ /* Set hardware parameters in device */
+ if (hw_params) {
+ format = params_format(hw_params);
+ channels = params_channels(hw_params);
+ rate = params_rate(hw_params);
+ buffer_bytes = params_buffer_bytes(hw_params);
+ period_bytes = params_period_bytes(hw_params);
+ periods = params_periods(hw_params);
+ } else {
+ format = runtime->format;
+ channels = runtime->channels;
+ rate = runtime->rate;
+ buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
+ period_bytes = frames_to_bytes(runtime, runtime->period_size);
+ periods = runtime->periods;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(g_a2v_format_map); ++i)
+ if (g_a2v_format_map[i].alsa_bit == format) {
+ vformat = g_a2v_format_map[i].vio_bit;
+
+ break;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(g_a2v_rate_map); ++i)
+ if (g_a2v_rate_map[i].rate == rate) {
+ vrate = g_a2v_rate_map[i].vio_bit;
+
+ break;
+ }
+
+ if (vformat == -1 || vrate == -1)
+ return -EINVAL;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_SET_PARAMS,
+ GFP_KERNEL);
+ if (IS_ERR(msg))
+ return PTR_ERR(msg);
+
+ request = sg_virt(&msg->sg_request);
+
+ request->buffer_bytes = cpu_to_virtio32(vdev, buffer_bytes);
+ request->period_bytes = cpu_to_virtio32(vdev, period_bytes);
+ request->channels = channels;
+ request->format = vformat;
+ request->rate = vrate;

I presume the latter three fields don't have to be endienness-converted, perhaps they're 8-bit wide only.

+
+ if (ss->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING))
+ request->features |=
+ cpu_to_virtio32(vdev,
+ 1U << VIRTIO_SND_PCM_F_MSG_POLLING);
+
+ if (ss->features & (1U << VIRTIO_SND_PCM_F_EVT_XRUNS))
+ request->features |=
+ cpu_to_virtio32(vdev,
+ 1U << VIRTIO_SND_PCM_F_EVT_XRUNS);
+
+ rc = virtsnd_ctl_msg_send_sync(ss->snd, msg);

Wouldn't it be better to only try to send the message after below allocations completed successfully?

+ if (rc)
+ return rc;
+
+ /* If the buffer was already allocated earlier, do nothing. */
+ if (runtime->dma_area)
+ return 0;
+
+ /* Allocate hardware buffer */
+ rc = snd_pcm_lib_malloc_pages(substream, buffer_bytes);
+ if (rc < 0)
+ return rc;
+
+ /* Allocate and initialize I/O messages */
+ rc = virtsnd_pcm_msg_alloc(ss, periods, runtime->dma_area,
+ period_bytes);
+ if (rc)
+ snd_pcm_lib_free_pages(substream);
+
+ return rc;
+}
+
+/**
+ * virtsnd_pcm_hw_free() - Reset the parameters of the PCM substream.
+ * @substream: Kernel ALSA substream.
+ *
+ * The function does the following:
+ * 1. tries to release the PCM substream on the device side,
+ * 2. frees the hardware buffer.
+ *
+ * Context: Any context that permits to sleep.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *ss = snd_pcm_substream_chip(substream);
+ int rc;
+
+ rc = virtsnd_pcm_release(ss);
+
+ /*
+ * Even if we failed to send the RELEASE message or wait for the queue
+ * flush to complete, we can safely delete the buffer. Because after
+ * receiving the STOP command, the device must stop all I/O message
+ * processing. If there are still pending messages in the queue, the
+ * next ops->hw_params() call should deal with this.
+ */
+ snd_pcm_lib_free_pages(substream);
+
+ return rc;
+}
+
+/**
+ * virtsnd_pcm_hw_params() - Prepare the PCM substream.

copy-paste: this is virtsnd_pcm_prepare()

+ * @substream: Kernel ALSA substream.
+ *
+ * The function can be called both from the upper level or from the driver
+ * itself.
+ *
+ * In all cases, the function:
+ * 1. checks the state of the virtqueue and, if necessary, tries to fix it,
+ * 2. prepares the substream on the device side.
+ *
+ * Context: Any context that permits to sleep. May take and release the tx/rx
+ * queue spinlock.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *ss = snd_pcm_substream_chip(substream);
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(ss);
+ struct virtio_snd_msg *msg;
+ unsigned long flags;
+ int rc;
+
+ /*
+ * If we got here after ops->trigger() was called, the queue may
+ * still contain messages. In this case, we need to reset the
+ * substream first.
+ */
+ if (atomic_read(&ss->msg_count)) {
+ rc = virtsnd_pcm_hw_params(substream, NULL);
+ if (rc)
+ return rc;
+ }
+
+ spin_lock_irqsave(&queue->lock, flags);
+ ss->msg_last_enqueued = -1;
+ spin_unlock_irqrestore(&queue->lock, flags);
+
+ /*
+ * Since I/O messages are asynchronous, they can be completed
+ * when the runtime structure no longer exists. Since each
+ * completion implies incrementing the hw_ptr, we cache all the
+ * current values needed to compute the new hw_ptr value.
+ */
+ ss->frame_bytes = substream->runtime->frame_bits >> 3;
+ ss->period_size = substream->runtime->period_size;
+ ss->buffer_size = substream->runtime->buffer_size;
+
+ atomic_set(&ss->hw_ptr, 0);
+ atomic_set(&ss->xfer_xrun, 0);
+ atomic_set(&ss->msg_count, 0);
+
+ msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_PREPARE,
+ GFP_KERNEL);
+ if (IS_ERR(msg))
+ return PTR_ERR(msg);
+
+ return virtsnd_ctl_msg_send_sync(ss->snd, msg);
+}
+
+/**
+ * virtsnd_pcm_trigger() - Process command for the PCM substream.
+ * @substream: Kernel ALSA substream.
+ * @command: Substream command (SNDRV_PCM_TRIGGER_XXX).
+ *
+ * Depending on the command, the function does the following:
+ * 1. enables/disables data transmission,
+ * 2. starts/stops the substream on the device side.
+ *
+ * Context: Atomic context. May take and release the tx/rx queue spinlock.

Really? Cannot .trigger() sleep? E.g. I see mdelay(25) in snd_es18xx_playback1_trigger()

+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
+{
+ struct virtio_pcm_substream *ss = snd_pcm_substream_chip(substream);
+ struct virtio_snd *snd = ss->snd;
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(ss);
+ struct virtio_snd_msg *msg;
+
+ switch (command) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: {
+ int rc;
+
+ spin_lock(&queue->lock);
+ rc = virtsnd_pcm_msg_send(ss);
+ spin_unlock(&queue->lock);

Maybe it would be good to explain why locking is required here and isn't required in most other locations, where messages are sent?

Thanks
Guennadi

+ if (rc)
+ return rc;
+
+ atomic_set(&ss->xfer_enabled, 1);
+
+ msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_START,
+ GFP_ATOMIC);
+ if (IS_ERR(msg))
+ return PTR_ERR(msg);
+
+ return virtsnd_ctl_msg_send(snd, msg);
+ }
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH: {
+ atomic_set(&ss->xfer_enabled, 0);
+
+ msg = virtsnd_pcm_ctl_msg_alloc(ss, VIRTIO_SND_R_PCM_STOP,
+ GFP_ATOMIC);
+ if (IS_ERR(msg))
+ return PTR_ERR(msg);
+
+ return virtsnd_ctl_msg_send(snd, msg);
+ }
+ default: {
+ return -EINVAL;
+ }
+ }
+}
+
+/**
+ * virtsnd_pcm_pointer() - Get the current hardware position for the PCM
+ * substream.
+ * @substream: Kernel ALSA substream.
+ *
+ * Context: Atomic context.
+ * Return: Hardware position in frames inside [0 ... buffer_size) range.
+ */
+static snd_pcm_uframes_t
+virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *ss = snd_pcm_substream_chip(substream);
+
+ if (atomic_read(&ss->xfer_xrun))
+ return SNDRV_PCM_POS_XRUN;
+
+ return (snd_pcm_uframes_t)atomic_read(&ss->hw_ptr);
+}
+
+/* PCM substream operators map. */
+const struct snd_pcm_ops virtsnd_pcm_ops = {
+ .open = virtsnd_pcm_open,
+ .close = virtsnd_pcm_close,
+ .ioctl = snd_pcm_lib_ioctl,
+ .hw_params = virtsnd_pcm_hw_params,
+ .hw_free = virtsnd_pcm_hw_free,
+ .prepare = virtsnd_pcm_prepare,
+ .trigger = virtsnd_pcm_trigger,
+ .pointer = virtsnd_pcm_pointer,
+};
--
2.30.0

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Next message: Tudor.Ambarus: "Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default"
Previous message: Petr Mladek: "[GIT PULL] printk urgent fix for 5.11-rc6"
In reply to: Anton Yakovlev: "[PATCH v2 6/9] ALSA: virtio: PCM substream operators"
Next in thread: Guennadi Liakhovetski: "Re: [PATCH v2 6/9] ALSA: virtio: PCM substream operators"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]