Hi Takashi,
On 19.10.2023 03:07, Takashi Iwai wrote:
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@xxxxxxxxxxxxxxx>
Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@xxxxxxxxxx>
---
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://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flkml%2f20231016151000.GE119987%40fedora%2ft%2f&umid=2f305b77-83e7-47b6-a461-a8ca67d0bfe2&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2d5775265e7e1741ae8eb783a3cb78ed553093c1
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.
The driver uses a message queue to communicate with the device. Thus,
the audio buffer is sliced into several I/O requests (= number of
periods) of the same size (= period size).
Before this, all such requests were enqueued when the substream started,
and immediately re-enqueued once the request is completed. This approach
made it possible to add mmap support. But for mmap there are no explicit
notifications from the application how many frames were written or read.
Thus, it was assumed that the virtual device should read/write frames to
requests based on timings. And there are some problems here:
1. This was found to violate the virtio specification: if a request is
already in the queue, the device can safely read/write there at any
time.
2. It looks like this breaks the use case with swiotlb. Personally I'm
not sure how the application handles DMA ownership in the case of
mmaped buffer.
To correctly implement mmap support, instead of transferring data via a
message queue, the driver and device must have a shared memory region.
We can add mmap in the future when we expand the functionality of the
device to support such shared memory.
Best regards,
thanks,
Takashi
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
