Re: [virtio-dev] Re: [PATCH v2 4/9] ALSA: virtio: build PCM devices and substream hardware descriptors
On 25.01.2021 16:44, Guennadi Liakhovetski wrote:
> On Sun, 24 Jan 2021, Anton Yakovlev wrote:
>
...[snip]...
>>
>> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
>> index 955eadc2d858..39fe13b43dd1 100644
>> --- a/sound/virtio/virtio_card.c
>> +++ b/sound/virtio/virtio_card.c
>> @@ -92,6 +92,17 @@ static void virtsnd_event_notify_cb(struct
>> virtqueue *vqueue)
>> if (!event)
>> break;
>>
>> + switch (le32_to_cpu(event->hdr.code)) {
>> + case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED:
>> + case VIRTIO_SND_EVT_PCM_XRUN: {
>
> In the previous patch you had a switch-case statement complying to the
> common kernel coding style. It isn't specified in coding-style.rst, but
> these superfluous braces really don't seem to be good for anything - in
> this and multiple other switch-case statements in the series.
I will fix this. Thanks!
...[snip]...
>> @@ -359,6 +384,8 @@ static int virtsnd_probe(struct virtio_device *vdev)
>> static void virtsnd_remove(struct virtio_device *vdev)
>> {
>> struct virtio_snd *snd = vdev->priv;
>> + struct virtio_pcm *pcm;
>> + struct virtio_pcm *pcm_next;
>>
>> if (!snd)
>> return;
>> @@ -376,6 +403,24 @@ static void virtsnd_remove(struct virtio_device
>> *vdev)
>> vdev->config->reset(vdev);
>> vdev->config->del_vqs(vdev);
>>
>> + list_for_each_entry_safe(pcm, pcm_next, >pcm_list, list) {
>> + unsigned int i;
>> +
>> + list_del(>list);
>> +
>> + for (i = 0; i < ARRAY_SIZE(pcm->streams); ++i) {
>> + struct virtio_pcm_stream *stream =
>> >streams[i];
>> +
>> + if (stream->substreams)
>> + devm_kfree(>dev,
stream->substreams);
>> + }
>> +
>> + devm_kfree(>dev, pcm);
>
> Please double-check both devm_kfree() calls above. Probably they aren't
> needed in the .remove() method.
Then I will redo these parts, and the parts that you noticed in the rest
of the comments to this file.
...[snip]...
>
> Thanks
> Guennadi
>
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
>
>
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
www.opensynergy.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/9] ALSA: virtio: build PCM devices and substream hardware descriptors
On Sun, 24 Jan 2021, Anton Yakovlev wrote:
Like the HDA specification, the virtio sound device specification links
PCM substreams, jacks and PCM channel maps into functional groups. For
each discovered group, a PCM device is created, the number of which
coincides with the group number.
Introduce the module parameters for setting the hardware buffer
parameters:
pcm_buffer_ms [=160]
pcm_periods_min [=2]
pcm_periods_max [=16]
pcm_period_ms_min [=10]
pcm_period_ms_max [=80]
Signed-off-by: Anton Yakovlev
---
sound/virtio/Makefile | 3 +-
sound/virtio/virtio_card.c | 45
sound/virtio/virtio_card.h | 9 +
sound/virtio/virtio_pcm.c | 536 +
sound/virtio/virtio_pcm.h | 89 ++
5 files changed, 681 insertions(+), 1 deletion(-)
create mode 100644 sound/virtio/virtio_pcm.c
create mode 100644 sound/virtio/virtio_pcm.h
diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile
index dc551e637441..69162a545a41 100644
--- a/sound/virtio/Makefile
+++ b/sound/virtio/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o
virtio_snd-objs := \
virtio_card.o \
- virtio_ctl_msg.o
+ virtio_ctl_msg.o \
+ virtio_pcm.o
diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
index 955eadc2d858..39fe13b43dd1 100644
--- a/sound/virtio/virtio_card.c
+++ b/sound/virtio/virtio_card.c
@@ -92,6 +92,17 @@ static void virtsnd_event_notify_cb(struct virtqueue *vqueue)
if (!event)
break;
+ switch (le32_to_cpu(event->hdr.code)) {
+ case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED:
+ case VIRTIO_SND_EVT_PCM_XRUN: {
In the previous patch you had a switch-case statement complying to the
common kernel coding style. It isn't specified in coding-style.rst, but
these superfluous braces really don't seem to be good for anything - in
this and multiple other switch-case statements in the series.
+ virtsnd_pcm_event(snd, event);
+ break;
+ }
+ default: {
+ break;
An empty default doesn't seem very useful either. So the above could've
just been
+ switch (le32_to_cpu(event->hdr.code)) {
+ case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED:
+ case VIRTIO_SND_EVT_PCM_XRUN:
+ virtsnd_pcm_event(snd, event);
+ }
+ }
+ }
+
virtsnd_event_send(queue->vqueue, event, true,
GFP_ATOMIC);
}
@@ -274,6 +285,16 @@ static int virtsnd_build_devs(struct virtio_snd *snd)
strscpy(snd->card->longname, "VirtIO Sound Card",
sizeof(snd->card->longname));
+ rc = virtsnd_pcm_parse_cfg(snd);
+ if (rc)
+ return rc;
+
+ if (snd->nsubstreams) {
+ rc = virtsnd_pcm_build_devs(snd);
+ if (rc)
+ return rc;
+ }
+
return snd_card_register(snd->card);
}
@@ -302,6 +323,9 @@ static int virtsnd_validate(struct virtio_device *vdev)
return -EINVAL;
}
+ if (virtsnd_pcm_validate(vdev))
+ return -EINVAL;
+
return 0;
}
@@ -325,6 +349,7 @@ static int virtsnd_probe(struct virtio_device *vdev)
snd->vdev = vdev;
INIT_WORK(>reset_work, virtsnd_reset_fn);
INIT_LIST_HEAD(>ctl_msgs);
+ INIT_LIST_HEAD(>pcm_list);
vdev->priv = snd;
@@ -359,6 +384,8 @@ static int virtsnd_probe(struct virtio_device *vdev)
static void virtsnd_remove(struct virtio_device *vdev)
{
struct virtio_snd *snd = vdev->priv;
+ struct virtio_pcm *pcm;
+ struct virtio_pcm *pcm_next;
if (!snd)
return;
@@ -376,6 +403,24 @@ static void virtsnd_remove(struct virtio_device *vdev)
vdev->config->reset(vdev);
vdev->config->del_vqs(vdev);
+ list_for_each_entry_safe(pcm, pcm_next, >pcm_list, list) {
+ unsigned int i;
+
+ list_del(>list);
+
+ for (i = 0; i < ARRAY_SIZE(pcm->streams); ++i) {
+ struct virtio_pcm_stream *stream = >streams[i];
+
+ if (stream->substreams)
+ devm_kfree(>dev, stream->substreams);
+ }
+
+ devm_kfree(>dev, pcm);
Please double-check both devm_kfree() calls above. Probably they aren't
needed in the .remove() method.
+ }
+
+ if (snd->substreams)
+ devm_kfree(>dev, snd->substreams);
+
devm_kfree(>dev, snd);
vdev->priv = NULL;
diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h
index 37b734a92134..be6651a6aaf8 100644
--- a/sound/virtio/virtio_card.h
[PATCH v2 4/9] ALSA: virtio: build PCM devices and substream hardware descriptors
Like the HDA specification, the virtio sound device specification links
PCM substreams, jacks and PCM channel maps into functional groups. For
each discovered group, a PCM device is created, the number of which
coincides with the group number.
Introduce the module parameters for setting the hardware buffer
parameters:
pcm_buffer_ms [=160]
pcm_periods_min [=2]
pcm_periods_max [=16]
pcm_period_ms_min [=10]
pcm_period_ms_max [=80]
Signed-off-by: Anton Yakovlev
---
sound/virtio/Makefile | 3 +-
sound/virtio/virtio_card.c | 45
sound/virtio/virtio_card.h | 9 +
sound/virtio/virtio_pcm.c | 536 +
sound/virtio/virtio_pcm.h | 89 ++
5 files changed, 681 insertions(+), 1 deletion(-)
create mode 100644 sound/virtio/virtio_pcm.c
create mode 100644 sound/virtio/virtio_pcm.h
diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile
index dc551e637441..69162a545a41 100644
--- a/sound/virtio/Makefile
+++ b/sound/virtio/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o
virtio_snd-objs := \
virtio_card.o \
- virtio_ctl_msg.o
+ virtio_ctl_msg.o \
+ virtio_pcm.o
diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
index 955eadc2d858..39fe13b43dd1 100644
--- a/sound/virtio/virtio_card.c
+++ b/sound/virtio/virtio_card.c
@@ -92,6 +92,17 @@ static void virtsnd_event_notify_cb(struct virtqueue *vqueue)
if (!event)
break;
+ switch (le32_to_cpu(event->hdr.code)) {
+ case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED:
+ case VIRTIO_SND_EVT_PCM_XRUN: {
+ virtsnd_pcm_event(snd, event);
+ break;
+ }
+ default: {
+ break;
+ }
+ }
+
virtsnd_event_send(queue->vqueue, event, true,
GFP_ATOMIC);
}
@@ -274,6 +285,16 @@ static int virtsnd_build_devs(struct virtio_snd *snd)
strscpy(snd->card->longname, "VirtIO Sound Card",
sizeof(snd->card->longname));
+ rc = virtsnd_pcm_parse_cfg(snd);
+ if (rc)
+ return rc;
+
+ if (snd->nsubstreams) {
+ rc = virtsnd_pcm_build_devs(snd);
+ if (rc)
+ return rc;
+ }
+
return snd_card_register(snd->card);
}
@@ -302,6 +323,9 @@ static int virtsnd_validate(struct virtio_device *vdev)
return -EINVAL;
}
+ if (virtsnd_pcm_validate(vdev))
+ return -EINVAL;
+
return 0;
}
@@ -325,6 +349,7 @@ static int virtsnd_probe(struct virtio_device *vdev)
snd->vdev = vdev;
INIT_WORK(>reset_work, virtsnd_reset_fn);
INIT_LIST_HEAD(>ctl_msgs);
+ INIT_LIST_HEAD(>pcm_list);
vdev->priv = snd;
@@ -359,6 +384,8 @@ static int virtsnd_probe(struct virtio_device *vdev)
static void virtsnd_remove(struct virtio_device *vdev)
{
struct virtio_snd *snd = vdev->priv;
+ struct virtio_pcm *pcm;
+ struct virtio_pcm *pcm_next;
if (!snd)
return;
@@ -376,6 +403,24 @@ static void virtsnd_remove(struct virtio_device *vdev)
vdev->config->reset(vdev);
vdev->config->del_vqs(vdev);
+ list_for_each_entry_safe(pcm, pcm_next, >pcm_list, list) {
+ unsigned int i;
+
+ list_del(>list);
+
+ for (i = 0; i < ARRAY_SIZE(pcm->streams); ++i) {
+ struct virtio_pcm_stream *stream = >streams[i];
+
+ if (stream->substreams)
+ devm_kfree(>dev, stream->substreams);
+ }
+
+ devm_kfree(>dev, pcm);
+ }
+
+ if (snd->substreams)
+ devm_kfree(>dev, snd->substreams);
+
devm_kfree(>dev, snd);
vdev->priv = NULL;
diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h
index 37b734a92134..be6651a6aaf8 100644
--- a/sound/virtio/virtio_card.h
+++ b/sound/virtio/virtio_card.h
@@ -24,6 +24,9 @@
#include
#include "virtio_ctl_msg.h"
+#include "virtio_pcm.h"
+
+struct virtio_pcm_substream;
/**
* struct virtio_snd_queue - Virtqueue wrapper structure.
@@ -43,6 +46,9 @@ struct virtio_snd_queue {
* @card: ALSA sound card.
* @ctl_msgs: Pending control request list.
* @event_msgs: Device events.
+ * @pcm_list: VirtIO PCM device list.
+ * @substreams: VirtIO PCM substreams.
+ * @nsubstreams: Number of PCM substreams.
*/
struct virtio_snd {
struct virtio_device *vdev;
@@ -51,6 +57,9 @@ struct virtio_snd {
struct snd_card *card;
struct list_head ctl_msgs;
struct virtio_snd_event *event_msgs;
+ struct list_head pcm_list;
+ struct virtio_pcm_substream
