On Wed, 18 Oct 2023 12:48:23 +0200, Matias Ezequiel Vara Larsen wrote:
This commit replaces the mmap mechanism with the copy() and fill_silence() callbacks for both capturing and playback for the virtio-sound driver. This change is required to prevent the updating of the content of a buffer that is already in the available ring.
The current mechanism splits a dma buffer into descriptors that are exposed to the device. This dma buffer is shared with the user application. When the device consumes a buffer, the driver moves the request from the used ring to available ring.
The driver exposes the buffer to the device without knowing if the content has been updated from the user. The section 2.8.21.1 of the virtio spec states that: "The device MAY access the descriptor chains the driver created and the memory they refer to immediately". If the device picks up buffers from the available ring just after it is notified, it happens that the content may be old.
By providing the copy() callback, the driver first updates the content of the buffer, and then, exposes the buffer to the device by enqueuing it in the available ring. Thus, device always picks up a buffer that is updated. During copy(), the number of requests enqueued depends on the "pos" and "bytes" arguments. The length of each request is period_size bytes.
For capturing, the driver starts by exposing all the available buffers to device. After device updates the content of a buffer, it enqueues it in the used ring. It is only after the copy() for capturing is issued that the driver re-enqueues the buffer in the available ring.
Co-developed-by: Anton Yakovlev anton.yakovlev@opensynergy.com Signed-off-by: Matias Ezequiel Vara Larsen mvaralar@redhat.com
Changelog: v1 -> v2:
- Use snd_pcm_set_managed_buffer_all()for buffer allocation/freeing.
- Make virtsnd_pcm_msg_send() generic by specifying the offset and size for the modified part of the buffer; this way no assumptions need to be made.
- Disable SNDRV_PCM_INFO_NO_REWINDS since now only sequential reading/writing of frames is supported.
- Correct comment at virtsnd_pcm_msg_send().
- v1 patch at: https://lore.kernel.org/lkml/20231016151000.GE119987@fedora/t/
sound/virtio/virtio_pcm.c | 7 ++- sound/virtio/virtio_pcm.h | 9 ++-- sound/virtio/virtio_pcm_msg.c | 93 ++++++++++++++++++++++------------- sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++----- 4 files changed, 137 insertions(+), 53 deletions(-)
Most of the code changes look good, but I wonder:
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c index c10d91fff2fb..66d67eef1bcc 100644 --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -104,12 +104,11 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss, * only message-based transport. */ vss->hw.info =
SNDRV_PCM_INFO_MMAP |SNDRV_PCM_INFO_MMAP_VALID |
Do we need the removal of those MMAP features inevitably? Usually mmap can still work even if the driver implements the copy ops. Those aren't always mutual exclusive.
thanks,
Takashi