Anton Yakovlev
2023-Oct-24 00:17 UTC
[PATCH v2] ALSA: virtio: use copy and fill_silence callbacks
Hi Takashi, On 19.10.2023 16:48, Takashi Iwai wrote:> On Thu, 19 Oct 2023 03:20:19 +0200, > Anton Yakovlev wrote: >> >> 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 at opensynergy.com> >>>> Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar at 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://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=323acbff-2d10-45a8-bbe1-78fc8583abec&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-6526c5e588ae6e2c247d0c967e0504e4fc7f307a >>>> >>>> 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. > > Ah, then this implementation might be an overkill. You're still using > the (intermediate) vmalloc buffer allocated via PCM managed mode, and > the actual data is copied from/to there. So it doesn't conflict with > the mmap operation at all. > > I guess that the problem you're trying to solve (the immediate data > transfer to the queue) can be implemented rather via PCM ack callback > instead. ALSA PCM core notifies the possible data transfer via PCM > ack callback right after each change of appl_ptr or hw_ptr, including > each read/write op or mmap commit. Then the driver can check the > change of appl_ptr (or hw_ptr for capture), fetch the newly available > data, and queue it immediately. > > Usually together with the use of ack callback, the driver sets > SNDRV_PCM_INFO_SYNC_APPLPTR flag. This prevents the mmap of the PCM > control record (not the audio data) and enforces the use of > SNDRV_PCM_IOCTL_SYNC_PTR ioctl instead (so that the driver always gets > the ack callback).Thanks for pointing out this possibility! I'm wondering if TinyALSA works correctly with this flag. Best regards,> > > thanks, > > Takashi > > >> >> >> Best regards, >> >>> >>> >>> thanks, >>> >>> Takashi >> >> -- >> Anton Yakovlev >> Senior Software Engineer >> >> OpenSynergy GmbH >> Rotherstr. 20, 10245 Berlin-- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin