[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification
Hi, > > qemu has rather small buffers backend buffers, to keep latencies low. > > So, yes it would copy data to backend buffers. No, it would most likely > > not copy over everything immediately. It will most likely leave buffers > > in the virtqueue, reading the data piecewise in the audio backend timer > > callback, complete the virtio request when it has read all data. So > > there wouldn't be a huge gap between completing the request and feeding > > the data to the host hardware. > > Yes, there is a gap, and this causes the following two concerns: > > 1. An application can rewind its pointer and rewrite part of this buffer. The > worst case is if the application rewrites the part currently being read by the > device. Depends on the driver implementation. If the driver will copy over the data to a buffer not visible to the userspace application this can't happen. If the driver allows userspace mmap the buffer, then queues virtio requests with pointers to those buffers, then yes, the application could modify buffers which are sitting in the queue waiting to be processed by the device. > > While talking about latencies: What happened to the idea to signal > > additional device latencies (backend buffering and processing ...) to > > the driver? > > Yeah, such an additional latency has a runtime nature, and we could report it > as part of an I/O request completion. We only need to choose units (time or > bytes). I'd stick to bytes, for consistency with period and buffer sizes. > Now we have only optional notifications (periods and xrun). But let's say, > that the device MAY send the xrun notifications and MUST send the period > notifications, and remove them from feature bits. > > Then we define requirements for message-based transport: > 1. the tx and rx queues are filled in with buffers having period_bytes size, > 2. buffer_bytes % period_bytes == 0, > 3. total buffers size in the queue is buffer_bytes. > > It is still not clear, when to complete playback buffers, but at least such > approach makes the driver being interrupt-driven. Period notification would be implicit (playback buffer completion) or explicit event queue message? On playback buffer completion: adding a latency field to the completion field could solve that nicely I think. The latency field would specify how long it will take until the device finishes playing that buffer. So in case the device completes the buffer when playback is finished it would fill in "0" there. In case the device completes the buffer after copying over the data to the internal buffer it would fill in the internal buffer size there (or maybe even the number of bytes which are currently used in the internal buffer). > > Completely different topic: I think we should consider adding tags to > > streams. Some background: In qemu we have two almost identical hda > > codecs: hda-duplex and hda-micro. The only difference is that > > hda-duplex has the input stream tagged as "line-in" and the output > > stream as "line-out", whereas hda-micro has the input channel tagged as > > "microphone" and the output channel as "speaker". hda-micro was created > > because some windows application refused to accept "line-in" as audio > > source. So applications clearly do care, and in case you have two input > > streams it would help applications pick a reasonable default. > > Should it be a fixed-size field in the capabilities structure? Yes, a simple enum would do the trick I think. Not sure if we want add more properties. hda codecs can also specify the color coding of physical jacks (line-in is red, ...) for example. Probably not useful for most use cases (i.e. when sound data lands @ pulseaudio where the user can re-route things as he wants), but it might be useful when a fixed physical host output is assigned to the guest. cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev][PATCH v1 2/2] virtio-mmio: Append version 2 interface
Version 2 needs to be appended together with version 1 as legacy
interface since we introduce the latest version.
Signed-off-by: Jing Liu
Signed-off-by: Chao Peng
Signed-off-by: Zha Bin
Signed-off-by: Liu Jiang
---
content.tex | 123 +---
1 file changed, 117 insertions(+), 6 deletions(-)
diff --git a/content.tex b/content.tex
index eaaffec..faf74de 100644
--- a/content.tex
+++ b/content.tex
@@ -2047,18 +2047,129 @@ \subsubsection{Driver Handling
Interrupts}\label{sec:Virtio Transport Options /
\subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over
MMIO / Legacy interface}
-The legacy MMIO transport used page-based addressing, resulting
+\subsubsection{Version 2}\label{sec:Virtio Transport Options / Virtio Over
MMIO / MMIO-specific Initialization And Device Operation / Version 2}
+
+The version 2 MMIO transport used single interrupt number and signel
notification address,
+resulting in a slightly different control register layout, the device
initialization and
+interrupt event configuration procedure from later version.
+
+Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / Version 2 MMIO
Device Register Layout}
+presents control registers layout, omitting
+descriptions of registers which did not change their function
+nor behaviour:
+
+\begin{longtable}{p{0.2\textwidth}p{0.7\textwidth}}
+ \caption {Version 2 MMIO Device Register Layout}
+ \label{tab:Virtio Trasport Options / Virtio Over MMIO / Version 2 MMIO
Device Register Layout} \\
+ \hline
+ \mmioreg{Name}{Function}{Offset from base}{Direction}{Description}
+ \hline
+ \hline
+ \endfirsthead
+ \hline
+ \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description}
+ \hline
+ \hline
+ \endhead
+ \endfoot
+ \endlastfoot
+ \mmioreg{MagicValue}{Magic value}{0x000}{R}{}
+ \hline
+ \mmioreg{Version}{Device version number}{0x004}{R}{Device returns value 0x2.}
+ \hline
+ \mmioreg{DeviceID}{Virtio Subsystem Device ID}{0x008}{R}{}
+ \hline
+ \mmioreg{VendorID}{Virtio Subsystem Vendor ID}{0x00c}{R}{}
+ \hline
+ \mmioreg{DeviceFeatures}{Flags representing features the device
supports}{0x010}{R}{}
+ \hline
+ \mmioreg{DeviceFeaturesSel}{Device (host) features word
selection.}{0x014}{W}{}
+ \hline
+ \mmioreg{DriverFeatures}{Flags representing device features understood and
activated by the driver}{0x020}{W}{}
+ \hline
+ \mmioreg{DriverFeaturesSel}{Activated (guest) features word
selection}{0x024}{W}{}
+ \hline
+ \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{}
+ \hline
+ \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{}
+ \hline
+ \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{}
+ \hline
+ \mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{}
+ \hline
+ \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
+Writing a value to this register notifies the device that
+there are new buffers to process in a queue.
+
+When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+the value written is the queue index.
+
+When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
+the \field{Notification data} value has the following format:
+
+\lstinputlisting{notifications-le.c}
+
+See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues /
Driver notifications}
+for the definition of the components.
+ }
+ \hline
+ \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
+Reading from this register returns a bit mask of events that
+caused the device interrupt to be asserted.
+The following events are possible:
+\begin{description}
+ \item[Used Buffer Notification] - bit 0 - the interrupt was asserted
+because the device has used a buffer
+in at least one of the active virtual queues.
+ \item [Configuration Change Notification] - bit 1 - the interrupt was
+asserted because the configuration of the device has changed.
+\end{description}
+ }
+ \hline
+ \mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
+Writing a value with bits set as defined in \field{InterruptStatus}
+to this register notifies the device that events causing
+the interrupt have been handled.
+ }
+ \hline
+ \mmioreg{Status}{Device status}{0x070}{RW}{}
+ \hline
+ \mmiodreg{QueueDescLow}{QueueDescHigh}{Virtual queue's Descriptor Area 64
bit long physical address}{0x080}{0x084}{W}{}
+ \hline
+ \mmiodreg{QueueDriverLow}{QueueDriverHigh}{Virtual queue's Driver Area 64
bit long physical address}{0x090}{0x094}{W}{}
+ \hline
+ \mmiodreg{QueueDeviceLow}{QueueDeviceHigh}{Virtual queue's Device Area 64
bit long physical address}{0x0a0}{0x0a4}{W}{}
+ \hline
+ \mmioreg{SHMSel}{Shared memory id}{0x0ac}{W}{}
+ \hline
+ \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long
length}{0x0b0}{0x0b4}{R}{}
+ \hline
+ \mmiodreg{SHMBaseLow}{SHMBaseHigh}{Shared memory region 64 bit long physical
[virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support
Upgrade virtio-mmio to version 3, with the abilities to support
MSI interrupt and notification features.
The details of version 2 will be appended as part of legacy interface
in next patch.
Signed-off-by: Jing Liu
Signed-off-by: Chao Peng
Signed-off-by: Zha Bin
Signed-off-by: Liu Jiang
---
content.tex | 191 ---
msi-status.c | 5 ++
2 files changed, 163 insertions(+), 33 deletions(-)
create mode 100644 msi-status.c
diff --git a/content.tex b/content.tex
index d68cfaf..eaaffec 100644
--- a/content.tex
+++ b/content.tex
@@ -1597,9 +1597,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio
Transport Options / Vi
}
\hline
\mmioreg{Version}{Device version number}{0x004}{R}{%
-0x2.
+0x3.
\begin{note}
- Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO
/ Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO /
Legacy interface}) used 0x1.
+ Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO
/ Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO /
Legacy interface}) used 0x1 or 0x2.
\end{note}
}
\hline
@@ -1671,25 +1671,23 @@ \subsection{MMIO Device Register
Layout}\label{sec:Virtio Transport Options / Vi
accesses apply to the queue selected by writing to \field{QueueSel}.
}
\hline
- \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
-Writing a value to this register notifies the device that
-there are new buffers to process in a queue.
+ \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
+Reading from the register returns the virtqueue notification configuration.
-When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-the value written is the queue index.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific
Initialization And Device Operation / Notification Address}
+for the configuration format.
-When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
-the \field{Notification data} value has the following format:
+Writing when the notification address calculated by the notification
configuration
+is just located at this register.
-\lstinputlisting{notifications-le.c}
-
-See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues /
Driver notifications}
-for the definition of the components.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific
Initialization And Device Operation / Available Buffer Notifications}
+to see the notification data format.
}
\hline
\mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
Reading from this register returns a bit mask of events that
-caused the device interrupt to be asserted.
+caused the device interrupt to be asserted. This is only used
+when MSI is not enabled.
The following events are possible:
\begin{description}
\item[Used Buffer Notification] - bit 0 - the interrupt was asserted
@@ -1703,7 +1701,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio
Transport Options / Vi
\mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
Writing a value with bits set as defined in \field{InterruptStatus}
to this register notifies the device that events causing
-the interrupt have been handled.
+the interrupt have been handled. This is only used when MSI is not enabled.
}
\hline
\mmioreg{Status}{Device status}{0x070}{RW}{%
@@ -1762,6 +1760,31 @@ \subsection{MMIO Device Register
Layout}\label{sec:Virtio Transport Options / Vi
\field{SHMSel} is unused) results in a base address of
0x.
}
+ \hline
+ \mmioreg{MsiStatus}{MSI status}{0x0c0}{R}{%
+Reading from this register returns the global MSI enable/disable status
and maximum
+number of virtqueues that device supports.
+\lstinputlisting{msi-status.c}
+ }
+ \hline
+ \mmioreg{MsiCmd}{MSI command}{0x0c2}{W}{%
+The driver writes to this register with appropriate operators and
arguments to
+execute MSI command to device.
+Operators supported is setting global MSI enable/disable status
+and updating MSI configuration to device.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific
Initialization And Device Operation / Device Initialization / MSI Vector
Configuration}
+for how to use this register.
+ }
+ \hline
+ \mmiodreg{MsiAddrLow}{MsiAddrHigh}{MSI 64 bit address}{0x0c4}{0x0c8}{W}{%
+Writing to these two registers (lower 32 bits of the address to
\field{MsiAddrLow},
+higher 32 bits to \field{MsiAddrHigh}) notifies the device about the
+MSI address.
+ }
+ \hline
+ \mmioreg{MsiData}{MSI 32 bit data}{0x0cc}{W}{%
+Writing to this register notifies the device about the MSI data.
+ }
\hline
\mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
Reading from this
[virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Keiichi,
Thank you for the update. Please see some comments below.
Also, we need to bring the virtio_video_control back as it is in fact used by
the driver to enumerate supported encoder controls. But yes, it still needs to
be documemnted, it's true.
On Mittwoch, 18. Dezember 2019 14:02:14 CET Keiichi Watanabe wrote:
> From: Dmitry Sepp
>
> The virtio video encoder device and decoder device provide functionalities
> to encode and decode video stream respectively.
> Though video encoder and decoder are provided as different devices, they use
> a same protocol.
>
> Signed-off-by: Dmitry Sepp
> Signed-off-by: Keiichi Watanabe
> ---
> content.tex | 1 +
> virtio-video.tex | 579 +++
> 2 files changed, 580 insertions(+)
> create mode 100644 virtio-video.tex
>
> diff --git a/content.tex b/content.tex
> index 556b373..9e56839 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5743,6 +5743,7 @@ \subsubsection{Legacy Interface: Framing
> Requirements}\label{sec:Device \input{virtio-vsock.tex}
> \input{virtio-fs.tex}
> \input{virtio-rpmb.tex}
> +\input{virtio-video.tex}
>
> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> diff --git a/virtio-video.tex b/virtio-video.tex
> new file mode 100644
> index 000..30e728d
> --- /dev/null
> +++ b/virtio-video.tex
> @@ -0,0 +1,579 @@
> +\section{Video Device}\label{sec:Device Types / Video Device}
> +
> +The virtio video encoder device and decoder device are virtual devices that
> +supports encoding and decoding respectively. Though the encoder and the
> decoder +are different devices, they use the same protocol.
> +
> +\subsection{Device ID}\label{sec:Device Types / Video Device / Device ID}
> +
> +\begin{description}
> +\item[30] encoder device
> +\item[31] decoder device
> +\end{description}
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Video Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] controlq - queue for sending control commands.
> +\item[1] eventq - queue for sending events happened in the device.
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / Video Device / Feature
> bits} +
> +\begin{description}
> +\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages can be used for
> video + buffers.
> +\end{description}
> +
> +\devicenormative{\subsubsection}{Feature bits}{Device Types / Video Device
> / Feature bits} +
> +The device MUST offer at least one of feature bits.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Video
> Device / Device configuration layout} +
> +Video device configuration uses the following layout structure:
> +
> +\begin{lstlisting}
> +struct virtio_video_config {
> +le32 max_cap_len;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{max_cap_len}] defines the maximum length of a descriptor
> + required to call VIRTIO_VIDEO_GET_CAPABILITY in bytes. The device
> + MUST set this value.
> +\end{description}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Video Device /
> Device Initialization} +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> Video Device / Device Initialization} +
> +The driver SHOULD query device capability by using the
> +VIRTIO_VIDEO_T_GET_CAPABILITY and use that information for the initial
> +setup.
> +
> +\subsection{Device Operation}\label{sec:Device Types / Video Device /
> Device Operation} +
> +The driver allocates input and output buffers and queues the buffers
> +to the device. The device performs operations on the buffers according
> +to the function in question.
> +
> +\subsubsection{Device Operation: Create stream}
> +
> +To process buffers, the device needs to associate them with a certain
> +video stream (essentially, a context). Streams are created by
> +VIRTIO_VIDEO_T_STREAM_CREATE with a default set of parameters
> +determined by the device.
> +
> +\subsubsection{Device Operation: Create buffers}
> +
> +Buffers are used to store the actual data as well as the relevant
> +metadata. Scatter lists are supported, so the buffer doesn't need to
> +be contiguous in guest physical memory.
> +
> +\begin{itemize*}
> +\item Use VIRTIO_VIDEO_T_RESOURCE_CREATE to create a virtio video
> + resource that is backed by a buffer allocated from the driver's
> + memory.
> +\item Use VIRTIO_VIDEO_T_RESOURCE_DESTROY to destroy a resource that
> + is no longer needed.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Stream parameter control}
> +
> +\begin{itemize*}
> +\item Use VIRTIO_VIDEO_T_GET_PARAMS to get the current stream parameters
> for + input and output streams from the device.
> +\item Use VIRTIO_VIDEO_T_SET_PARAMS to provide new stream parameters to the
> + device.
> +\item After setting stream parameters, the driver may issue
> + VIRTIO_VIDEO_T_GET_PARAMS as some parameters of both input and output can
> be + changed implicitly by the device during the set
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > However that still doesn't let the driver know which buffers will be > > dequeued when. A simple example of this scenario is when the guest is > > done displaying a frame and requeues the buffer back to the decoder. > > Then the decoder will not choose it for decoding next frames into as > > long as the frame in that buffer is still used as a reference frame, > > even if one sends the drain request. > It might be that I'm getting your point wrong, but do you mean some hardware > can mark a buffer as ready to be displayed yet still using the underlying > memory to decode other frames? Yes, this is how I understand Tomasz Figa. > This means, if you occasionally/intentionally > write to the buffer you mess up the whole decoding pipeline. And to avoid this the buffer handling aspect must be clarified in the specification. Is the device allowed to continue using the buffer after finishing decoding and completing the queue request? If so, how do we hand over buffer ownership back to the driver so it can free the pages? drain request? How do we handle re-using buffers? Can the driver simply re-queue them and expect the device figures by itself whenever it can use the buffer or whenever it is still needed as reference frame? cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > Not clearly defined in the spec: When is the decoder supposed to send > > the response for a queue request? When it finished decoding (i.e. frame > > is ready for playback), or when it doesn't need the buffer any more for > > decoding (i.e. buffer can be re-queued or pages can be released)? > In my eyes the both statements mean almost the same and both are valid. Well, no. When the device decoded a P-Frame it can notify the device, saying "here is your decoded frame". But the device might still need the buffer with the decoded frame to properly decode the following B/I Frames which reference the P-Frame. cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification
On 19.12.2019 08:17, Gerd Hoffmann wrote: Hi, The driver can still split the data from the application into a set of smaller virtio buffers. And this is how I would write a driver. Create a bunch of buffers, period_bytes each, enough to cover buffer_bytes. Then go submit them, re-use them robin-round (each time the device completes a buffer re-fill and re-submit it), without a special case with one big buffer for the (playback) stream start. Yes, but what about device side? Most likely, it will copy the contents of the buffer to another location and immediately complete the request (otherwise it is not clear when to complete). qemu has rather small buffers backend buffers, to keep latencies low. So, yes it would copy data to backend buffers. No, it would most likely not copy over everything immediately. It will most likely leave buffers in the virtqueue, reading the data piecewise in the audio backend timer callback, complete the virtio request when it has read all data. So there wouldn't be a huge gap between completing the request and feeding the data to the host hardware. Yes, there is a gap, and this causes the following two concerns: 1. An application can rewind its pointer and rewrite part of this buffer. The worst case is if the application rewrites the part currently being read by the device. 2. This approach looks like shared memory on a smaller scale, and the whole meaning of the messages is lost. Because sending a message assumes that what's gone is gone. Another reason for this (beside keeping latencies low) is that it is a good idea to avoid or at least limit guest-triggered allocations on the host for security reasons. Yes, that makes sense. While talking about latencies: What happened to the idea to signal additional device latencies (backend buffering and processing ...) to the driver? Yeah, such an additional latency has a runtime nature, and we could report it as part of an I/O request completion. We only need to choose units (time or bytes). In addition, the driver must advance the hardware position, and completing the request is the only possible place to do this. In most cases, all this will not coincide with the period notification. So, period notification should be triggered when the device has actually completed processing the data (i.e. host playback finished)? I can see that this can be useful. I think qemu wouldn't be able to support that with the current audio backend design, but that isn't a blocker given this is an optional feature. Also I think the spec should to clarify how period notification is supposed to work, how that relates to buffer completion and latencies. This means that the notification itself is an optional feature that may or may not be enabled for message-based transport. Which transports are expected to support that? If multiple transports: Can a device signal it supports these notifications for one transport but not for another? I think this does not depend on transport, but take a look at the comment below. I think, it's better not to set queued buffer size restrictions. Lets make it SHOULD then. Again, if periods are not used, then there's no period_bytes. And they are not used, for example, in most low-latency applications. (Well, technically speaking, low-latency applications in Linux must configure period size for ALSA. But then they usually suppress period notifications and send very small chunks of data, which usually do not correspond to the size of the period at all.) So you want pass the data on to the device in whatever chunks the userspace application hands them to the driver? Ok, reasonable. Should we define period_bytes as upper limit then, i.e. virtio buffers should be period_bytes or smaller? I think, it's becoming more and more complicated and we should revise everything. Now we have only optional notifications (periods and xrun). But let's say, that the device MAY send the xrun notifications and MUST send the period notifications, and remove them from feature bits. Then we define requirements for message-based transport: 1. the tx and rx queues are filled in with buffers having period_bytes size, 2. buffer_bytes % period_bytes == 0, 3. total buffers size in the queue is buffer_bytes. It is still not clear, when to complete playback buffers, but at least such approach makes the driver being interrupt-driven. Basically, it is what you said but with little changes. Completely different topic: I think we should consider adding tags to streams. Some background: In qemu we have two almost identical hda codecs: hda-duplex and hda-micro. The only difference is that hda-duplex has the input stream tagged as "line-in" and the output stream as "line-out", whereas hda-micro has the input channel tagged as "microphone" and the output channel as "speaker". hda-micro was created because some windows application refused to accept "line-in" as audio source. So applications clearly do
[virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > I also can't see why the flag is needed in the first place. The driver > > should know which buffers are queued still and be able to figure > > whenever the drain is complete or not without depending on that flag. > > So I'd suggest to simply drop it. > This flag is used not for drain only. In marks the completion of whatever > specific buffer sequence, like a full end-of-stream, resolution change, drain > etc. We also need this to handle nested sequences. For instance, a resolution > change event might happen while in drain. Ah, ok. That makes sense (please clarify this in the spec). cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Thu, Dec 19, 2019 at 7:55 PM Dmitry Sepp wrote: > > Hi Tomasz, > > On Donnerstag, 19. Dezember 2019 10:59:02 CET Tomasz Figa wrote: > > On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp > wrote: > > > Hi, > > > > > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann > wrote: > > > > > > Hi, > > > > > > > > > > > > > +The device MUST mark the last buffer with the > > > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > > > +sequence. > > > > > > > > > > > > No, that would build a race condition into the protocol. The device > > > > > > could complete the last buffer after the driver has sent the drain > > > > > > command but before the device saw it. So the flag would not be > > > > > > reliable. > > > > > > > > > > > > I also can't see why the flag is needed in the first place. The > > > > > > driver > > > > > > should know which buffers are queued still and be able to figure > > > > > > whenever the drain is complete or not without depending on that > > > > > > flag. > > > > > > So I'd suggest to simply drop it. > > > > > > > > > > Unfortunately video decoders are not that simple. There are always > > > > > going to be some buffers on the decoder side used as reference frames. > > > > > Only the decoder knows when to release them, as it continues decoding > > > > > the stream. > > > > > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > > the response for a queue request? When it finished decoding (i.e. frame > > > > is ready for playback), or when it doesn't need the buffer any more for > > > > decoding (i.e. buffer can be re-queued or pages can be released)? > > > > > > In my eyes the both statements mean almost the same and both are valid. I > > > think whatever underlying libraries are used for decoding on the device > > > side, they simply won't return us the buffer as long as the HW device > > > needs them to continue its normal operation. So your first sentence > > > applies to output buffers, the second - to input buffers. > > > > > > My understanding is as follows: we send the response for a queue request > > > as > > > soon as the HW device on the host side passes the buffer ownership back to > > > the client (like when VIDIOC_DQBUF has returned a buffer). > > > > That's how it's defined in V4L2 and what makes the most sense from the > > video decoding point of view, as one wants to display frames as soon > > as they are available. > > > > However that still doesn't let the driver know which buffers will be > > dequeued when. A simple example of this scenario is when the guest is > > done displaying a frame and requeues the buffer back to the decoder. > > Then the decoder will not choose it for decoding next frames into as > > long as the frame in that buffer is still used as a reference frame, > > even if one sends the drain request. > It might be that I'm getting your point wrong, but do you mean some hardware > can mark a buffer as ready to be displayed yet still using the underlying > memory to decode other frames? This means, if you occasionally/intentionally > write to the buffer you mess up the whole decoding pipeline. That would be > strange at least... That's correct. It depends on the hardware, but in principle we don't want to copy the frames decoded to temporary buffers for using them as reference frames, as that would waste bandwidth and increase latency. The contract between the kernel and the application is that it must not write to the frame buffers if it wants to get correct decoding results. But after all, I don't see a reason why the application would write to those buffers. Best regards, Tomasz - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Tomasz, On Donnerstag, 19. Dezember 2019 10:59:02 CET Tomasz Figa wrote: > On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp wrote: > > Hi, > > > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > > > Hi, > > > > > > > > > > > +The device MUST mark the last buffer with the > > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > > +sequence. > > > > > > > > > > No, that would build a race condition into the protocol. The device > > > > > could complete the last buffer after the driver has sent the drain > > > > > command but before the device saw it. So the flag would not be > > > > > reliable. > > > > > > > > > > I also can't see why the flag is needed in the first place. The > > > > > driver > > > > > should know which buffers are queued still and be able to figure > > > > > whenever the drain is complete or not without depending on that > > > > > flag. > > > > > So I'd suggest to simply drop it. > > > > > > > > Unfortunately video decoders are not that simple. There are always > > > > going to be some buffers on the decoder side used as reference frames. > > > > Only the decoder knows when to release them, as it continues decoding > > > > the stream. > > > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > the response for a queue request? When it finished decoding (i.e. frame > > > is ready for playback), or when it doesn't need the buffer any more for > > > decoding (i.e. buffer can be re-queued or pages can be released)? > > > > In my eyes the both statements mean almost the same and both are valid. I > > think whatever underlying libraries are used for decoding on the device > > side, they simply won't return us the buffer as long as the HW device > > needs them to continue its normal operation. So your first sentence > > applies to output buffers, the second - to input buffers. > > > > My understanding is as follows: we send the response for a queue request > > as > > soon as the HW device on the host side passes the buffer ownership back to > > the client (like when VIDIOC_DQBUF has returned a buffer). > > That's how it's defined in V4L2 and what makes the most sense from the > video decoding point of view, as one wants to display frames as soon > as they are available. > > However that still doesn't let the driver know which buffers will be > dequeued when. A simple example of this scenario is when the guest is > done displaying a frame and requeues the buffer back to the decoder. > Then the decoder will not choose it for decoding next frames into as > long as the frame in that buffer is still used as a reference frame, > even if one sends the drain request. It might be that I'm getting your point wrong, but do you mean some hardware can mark a buffer as ready to be displayed yet still using the underlying memory to decode other frames? This means, if you occasionally/intentionally write to the buffer you mess up the whole decoding pipeline. That would be strange at least... Regds, Dmitry. > > > Thanks for reviewing! > > > > Regards, > > Dmitry. > > > > > > How we defined this in the V4L2 stateful decoder interface is that if > > > > the decoder happened to return the last framebuffer before the drain > > > > request arrived, it would return one more, empty. > > > > > > Ok. That is not clear from the spec. I've read the drain request as as > > > "please stop decoding and complete all queue requests which are in the > > > virtqueue, even when you didn't process them yet". In which case > > > completing the last pending queue request would clearly indicate the > > > draining request is complete. Also completing the drain request should > > > only happen when the operation is finished. > > > > > > I think the various states a buffer can have and how queuing & deciding > > > & draining changes these states should be clarified in the > > > specification. > > > > > > cheers, > > > > > > Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp wrote: > > Hi, > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > > Hi, > > > > > > > > > +The device MUST mark the last buffer with the > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > +sequence. > > > > > > > > No, that would build a race condition into the protocol. The device > > > > could complete the last buffer after the driver has sent the drain > > > > command but before the device saw it. So the flag would not be > > > > reliable. > > > > > > > > I also can't see why the flag is needed in the first place. The driver > > > > should know which buffers are queued still and be able to figure > > > > whenever the drain is complete or not without depending on that flag. > > > > So I'd suggest to simply drop it. > > > > > > Unfortunately video decoders are not that simple. There are always > > > going to be some buffers on the decoder side used as reference frames. > > > Only the decoder knows when to release them, as it continues decoding > > > the stream. > > > > Not clearly defined in the spec: When is the decoder supposed to send > > the response for a queue request? When it finished decoding (i.e. frame > > is ready for playback), or when it doesn't need the buffer any more for > > decoding (i.e. buffer can be re-queued or pages can be released)? > In my eyes the both statements mean almost the same and both are valid. I > think whatever underlying libraries are used for decoding on the device side, > they simply won't return us the buffer as long as the HW device needs them to > continue its normal operation. So your first sentence applies to output > buffers, > the second - to input buffers. > > My understanding is as follows: we send the response for a queue request as > soon as the HW device on the host side passes the buffer ownership back to the > client (like when VIDIOC_DQBUF has returned a buffer). That's how it's defined in V4L2 and what makes the most sense from the video decoding point of view, as one wants to display frames as soon as they are available. However that still doesn't let the driver know which buffers will be dequeued when. A simple example of this scenario is when the guest is done displaying a frame and requeues the buffer back to the decoder. Then the decoder will not choose it for decoding next frames into as long as the frame in that buffer is still used as a reference frame, even if one sends the drain request. > > Thanks for reviewing! > > Regards, > Dmitry. > > > > > > How we defined this in the V4L2 stateful decoder interface is that if > > > the decoder happened to return the last framebuffer before the drain > > > request arrived, it would return one more, empty. > > > > Ok. That is not clear from the spec. I've read the drain request as as > > "please stop decoding and complete all queue requests which are in the > > virtqueue, even when you didn't process them yet". In which case > > completing the last pending queue request would clearly indicate the > > draining request is complete. Also completing the drain request should > > only happen when the operation is finished. > > > > I think the various states a buffer can have and how queuing & deciding > > & draining changes these states should be clarified in the > > specification. > > > > cheers, > > Gerd > > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Thu, Dec 19, 2019 at 6:26 PM Dmitry Sepp wrote: > > Hi Gerd, > > On Mittwoch, 18. Dezember 2019 14:40:37 CET Gerd Hoffmann wrote: > > Hi, > > > > > +The device MUST mark the last buffer with the > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > +sequence. > > > > No, that would build a race condition into the protocol. The device > > could complete the last buffer after the driver has sent the drain > > command but before the device saw it. So the flag would not be > > reliable. > No, then it means the device was not in drain, but, for example, hit a > resolution change in the stream and tells us that this is the last buffer with > the old resolution. > > > > > I also can't see why the flag is needed in the first place. The driver > > should know which buffers are queued still and be able to figure > > whenever the drain is complete or not without depending on that flag. > > So I'd suggest to simply drop it. > This flag is used not for drain only. In marks the completion of whatever > specific buffer sequence, like a full end-of-stream, resolution change, drain > etc. We also need this to handle nested sequences. For instance, a resolution > change event might happen while in drain. Good point. The resolution change event is in a different virtqueue, so it's not serialized with the request completions. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification
On 18.12.2019 17:54, Mark Brown wrote: On Wed, Dec 18, 2019 at 04:34:12PM +0100, Anton Yakovlev wrote: On 18.12.2019 12:52, Gerd Hoffmann wrote: Well, the driver has to manage buffers, so I can't see how a driver is not interested in completion notifications. If the driver isn't interested in keeping the latency low by using lots of small buffers it still needs to queue at least two (larger) buffers, so the device still has data to continue playback when it completed a buffer. And the driver needs to know when the device is done with a buffer so it can either recycle the buffer or free the pages. It seems that you have mixed up two different things: sending/completing a request containing a buffer, and period notifications. They are not the same. What do you expect a period notification to be? The default expecation would very much be that they're tied to what's going on with the buffers. Elapsed period notification was initially proposed as an optional feature. But then the discussion switched on to define the entire period-based operational sequence built in protocol. Well, we can go this way and make periods not an optional notification, but a full optional mode of operation. Then we can add all the requirements that Gerd spoke of. -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin Phone: +49 30 60 98 54 0 E-Mail: anton.yakov...@opensynergy.com - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > Hi, > > > > > > > +The device MUST mark the last buffer with the > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > +sequence. > > > > > > No, that would build a race condition into the protocol. The device > > > could complete the last buffer after the driver has sent the drain > > > command but before the device saw it. So the flag would not be > > > reliable. > > > > > > I also can't see why the flag is needed in the first place. The driver > > > should know which buffers are queued still and be able to figure > > > whenever the drain is complete or not without depending on that flag. > > > So I'd suggest to simply drop it. > > > > Unfortunately video decoders are not that simple. There are always > > going to be some buffers on the decoder side used as reference frames. > > Only the decoder knows when to release them, as it continues decoding > > the stream. > > Not clearly defined in the spec: When is the decoder supposed to send > the response for a queue request? When it finished decoding (i.e. frame > is ready for playback), or when it doesn't need the buffer any more for > decoding (i.e. buffer can be re-queued or pages can be released)? In my eyes the both statements mean almost the same and both are valid. I think whatever underlying libraries are used for decoding on the device side, they simply won't return us the buffer as long as the HW device needs them to continue its normal operation. So your first sentence applies to output buffers, the second - to input buffers. My understanding is as follows: we send the response for a queue request as soon as the HW device on the host side passes the buffer ownership back to the client (like when VIDIOC_DQBUF has returned a buffer). Thanks for reviewing! Regards, Dmitry. > > > How we defined this in the V4L2 stateful decoder interface is that if > > the decoder happened to return the last framebuffer before the drain > > request arrived, it would return one more, empty. > > Ok. That is not clear from the spec. I've read the drain request as as > "please stop decoding and complete all queue requests which are in the > virtqueue, even when you didn't process them yet". In which case > completing the last pending queue request would clearly indicate the > draining request is complete. Also completing the drain request should > only happen when the operation is finished. > > I think the various states a buffer can have and how queuing & deciding > & draining changes these states should be clarified in the > specification. > > cheers, > Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Gerd, On Mittwoch, 18. Dezember 2019 14:40:37 CET Gerd Hoffmann wrote: > Hi, > > > +The device MUST mark the last buffer with the > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > +sequence. > > No, that would build a race condition into the protocol. The device > could complete the last buffer after the driver has sent the drain > command but before the device saw it. So the flag would not be > reliable. No, then it means the device was not in drain, but, for example, hit a resolution change in the stream and tells us that this is the last buffer with the old resolution. > > I also can't see why the flag is needed in the first place. The driver > should know which buffers are queued still and be able to figure > whenever the drain is complete or not without depending on that flag. > So I'd suggest to simply drop it. This flag is used not for drain only. In marks the completion of whatever specific buffer sequence, like a full end-of-stream, resolution change, drain etc. We also need this to handle nested sequences. For instance, a resolution change event might happen while in drain. Regards, Dmitry. > > That is the only issue I've spotted in the protocol on a first quick > look. There are a few places where the spec text could be improved. > I'll try to set aside some time to go over this in detail, but I can't > promise I'll find the time to do that before xmas and new year. > > cheers, > Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
