| Message ID | 20210124165408.1122868-1-anton.yakovlev@opensynergy.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: add virtio sound driver | expand |
On Sun, 24 Jan 2021 17:53:59 +0100, Anton Yakovlev wrote: > > This series implements a driver part of the virtio sound device > specification v8 [1]. > > The driver supports PCM playback and capture substreams, jack and > channel map controls. A message-based transport is used to write/read > PCM frames to/from a device. > > The series is based (and was actually tested) on Linus's master > branch [2], on top of > > commit 1e2a199f6ccd ("Merge tag 'spi-fix-v5.11-rc4' of ...") > > As a device part was used OpenSynergy proprietary implementation. > > Any comments are very welcome. > > v1->v2 changes: > > 1. For some reason, in the previous patch series, several patches were > squashed. Fixed this issue to make the review easier. > 2. Added mst@redhat.com to the MAINTAINERS. > 3. When creating virtqueues, now only the event virtqueue is disabled. > It's enabled only after successful initialization of the device. > 4. Added additional comments to the reset worker function: > [2/9] virtio_card.c:virtsnd_reset_fn() > 5. Added check that VIRTIO_F_VERSION_1 feature bit is set. > 6. Added additional comments to the device removing function: > [2/9] virtio_card.c:virtsnd_remove() > 7. Added additional comments to the tx/rx interrupt handler: > [5/9] virtio_pcm_msg.c:virtsnd_pcm_msg_complete() > 8. Added additional comments to substream release wait function. > [6/9] virtio_pcm_ops.c:virtsnd_pcm_released() > > [1] https://lists.oasis-open.org/archives/virtio-dev/202003/msg00185.html > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > > Anton Yakovlev (9): > uapi: virtio_ids: add a sound device type ID from OASIS spec > ALSA: virtio: add virtio sound driver > ALSA: virtio: handling control messages > ALSA: virtio: build PCM devices and substream hardware descriptors > ALSA: virtio: handling control and I/O messages for the PCM device > ALSA: virtio: PCM substream operators > ALSA: virtio: introduce jack support > ALSA: virtio: introduce PCM channel map support > ALSA: virtio: introduce device suspend/resume support Through a quick glance over the new series, below are some comments (sorry not putting in each patch): - The PCM buffer management can be simplified with the recently introduced API, snd_pcm_set_managed_buffer() and *_all(). BTW, I wondered why you need to iterate do preallocation for each; can't the *_all() function work in a shot? - The PCM ops has sync_stop that is performed before prepare, hw_parms and else after a stream is stopped via trigger(STOP). This can be used for synchronizing things as found in your driver code. - I'm wondering whether you can use the non-atomic PCM ops. Is any interrupt handling (or spinlocked context) involved in the interface? If not, you can set nonatomic flag, and this would simply things. - Does the buffer type have to be CONTINUOUS? Can't the vmalloc buffer work? - Is the buffer supposed to be aligned with the period size? i.e. is the configuration like period=200 buffer=500 supposed to work? If the period-size alignment is needed, the driver requires an additional hw_constraint setup to make PERIODS integer. - In general, "stream", and "subtream" variables are used in mixed ways for both ALSA and virtio objects, and it's hard to follow. Maybe some prefix would help. - Don't PCM stream names need to be unique? They are all the same string. - Leave the card->id without changing unless you need to set it up explicitly by some extra reason. Also, card->shortname should be a bit more verbose and descriptive. It's a free-string while card->drier is an ID string that is used as a lookup key in alsa-lib. thanks, Takashi
Hi Takashi, Thank you for your hints, I actually applied them all. The only question that I have is... On 03.02.2021 19:07, Takashi Iwai wrote: [snip] > > - Don't PCM stream names need to be unique? They are all the same > string. What did you mean here? Substream names? > > thanks, > > Takashi >
On Mon, 08 Feb 2021 11:23:00 +0100, Anton Yakovlev wrote: > > Hi Takashi, > > Thank you for your hints, I actually applied them all. The only > question that I have is... > > On 03.02.2021 19:07, Takashi Iwai wrote: > > [snip] > > > > > - Don't PCM stream names need to be unique? They are all the same > > string. > > What did you mean here? Substream names? The driver set all PCM name as the same "VirtIO PCM", and snd_pcm_new() is always with "virtio_snd", no matter how many PCM streams are created. At least it would make sense to add a number suffix or such. thanks, Takashi
This series implements a driver part of the virtio sound device specification v8 [1]. The driver supports PCM playback and capture substreams, jack and channel map controls. A message-based transport is used to write/read PCM frames to/from a device. The series is based (and was actually tested) on Linus's master branch [2], on top of commit 1e2a199f6ccd ("Merge tag 'spi-fix-v5.11-rc4' of ...") As a device part was used OpenSynergy proprietary implementation. Any comments are very welcome. v1->v2 changes: 1. For some reason, in the previous patch series, several patches were squashed. Fixed this issue to make the review easier. 2. Added mst@redhat.com to the MAINTAINERS. 3. When creating virtqueues, now only the event virtqueue is disabled. It's enabled only after successful initialization of the device. 4. Added additional comments to the reset worker function: [2/9] virtio_card.c:virtsnd_reset_fn() 5. Added check that VIRTIO_F_VERSION_1 feature bit is set. 6. Added additional comments to the device removing function: [2/9] virtio_card.c:virtsnd_remove() 7. Added additional comments to the tx/rx interrupt handler: [5/9] virtio_pcm_msg.c:virtsnd_pcm_msg_complete() 8. Added additional comments to substream release wait function. [6/9] virtio_pcm_ops.c:virtsnd_pcm_released() [1] https://lists.oasis-open.org/archives/virtio-dev/202003/msg00185.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git Anton Yakovlev (9): uapi: virtio_ids: add a sound device type ID from OASIS spec ALSA: virtio: add virtio sound driver ALSA: virtio: handling control messages ALSA: virtio: build PCM devices and substream hardware descriptors ALSA: virtio: handling control and I/O messages for the PCM device ALSA: virtio: PCM substream operators ALSA: virtio: introduce jack support ALSA: virtio: introduce PCM channel map support ALSA: virtio: introduce device suspend/resume support MAINTAINERS | 9 + include/uapi/linux/virtio_ids.h | 1 + include/uapi/linux/virtio_snd.h | 361 ++++++++++++++++++++ sound/Kconfig | 2 + sound/Makefile | 3 +- sound/virtio/Kconfig | 10 + sound/virtio/Makefile | 13 + sound/virtio/virtio_card.c | 577 +++++++++++++++++++++++++++++++ sound/virtio/virtio_card.h | 121 +++++++ sound/virtio/virtio_chmap.c | 237 +++++++++++++ sound/virtio/virtio_ctl_msg.c | 293 ++++++++++++++++ sound/virtio/virtio_ctl_msg.h | 122 +++++++ sound/virtio/virtio_jack.c | 255 ++++++++++++++ sound/virtio/virtio_pcm.c | 582 ++++++++++++++++++++++++++++++++ sound/virtio/virtio_pcm.h | 132 ++++++++ sound/virtio/virtio_pcm_msg.c | 325 ++++++++++++++++++ sound/virtio/virtio_pcm_ops.c | 528 +++++++++++++++++++++++++++++ 17 files changed, 3570 insertions(+), 1 deletion(-) create mode 100644 include/uapi/linux/virtio_snd.h create mode 100644 sound/virtio/Kconfig create mode 100644 sound/virtio/Makefile create mode 100644 sound/virtio/virtio_card.c create mode 100644 sound/virtio/virtio_card.h create mode 100644 sound/virtio/virtio_chmap.c create mode 100644 sound/virtio/virtio_ctl_msg.c create mode 100644 sound/virtio/virtio_ctl_msg.h create mode 100644 sound/virtio/virtio_jack.c create mode 100644 sound/virtio/virtio_pcm.c create mode 100644 sound/virtio/virtio_pcm.h create mode 100644 sound/virtio/virtio_pcm_msg.c create mode 100644 sound/virtio/virtio_pcm_ops.c