[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH] snd: Add virtio sound device specification
Hi, On Mon, Nov 11, 2019 at 03:20:55PM +0000, Liam Girdwood wrote: > > > > +struct virtio_snd_ctl_msg { > > > > + /* device-read-only data */ > > > > + le32 request_code; > > > > + u8 request_payload[]; > > > > > > How do I know how much data to read here ? Is size embedded in > > > request_code ? > > > > Yes, you are right. Actual request/response length can be easily > > derived from > > a request_code. > > > > Nak, please use a separate size and type code otherwise we will have > real problems in the future when we come to add new features (with new > codes or bigger sizes). Other virtio devices rely on the buffer length already provided by virtio for this. For example see the virtio-net control virtqueue (virtio_net_ctrl*), which is extended with new commands from time to time. Most recently, the "receive-side scaling" feature introduces a large structure as payload to virtio_net_ctrl. [...] > > > > +\begin{lstlisting} > > > > +struct virtio_snd_pcm_xfer { > > > > + le32 stream; > > > > + u8 data[]; > > > > + le32 status; > > > > + le32 actual_length; > > > > > > Not following this, is actual_length the size of data[]. If so, it > > > must > > > preceed data[]. > > > > No, the actual_length field is supposed to be used by device side to > > report > > actual amount of bytes read from/written to a buffer. By the way the written size is already provided by virtio (field len in the used descriptor), but not the read size. > > In real world > > scenario, > > if an I/O request contains N bytes, a device can play/capture *up to* > > N bytes. > > Thus, it's required to report this length back to a driver. > > > > This is not how PCM audio buffering works. I don't copy extra padding > if I dont need to, I only copy when my buffer is full. See the need for > period size in earlier comments. [...] > > > 2) Zero copy of audio data and stream positions. Essential for any > > > low > > > latency audio use cases (if supported by HW, hypervisor, VM > > > framework). > > > > What do you mean by zero-copy? Shared memory between device and > > driver sides? > > Yes, but I see this is now a separate thread as it used by others too. The virtio transport already permits zero-copy, because one buffer can be described with a chain of descriptors, each pointing to a different area in memory. The driver can split the virtio_snd_pcm_xfer into header and data: virtqueue desc ring | | +-------+ | |---------> header (copied) +- - - -+ | |---------> PCM data (mapped) +-------+ | | So it might be a good idea to put all the metadata (stream, status, actual_length) at the beginning of the virtio_snd_pcm_xfer struct, otherwise the driver will need three descriptors instead of two to achieve this. Thanks, Jean
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]