Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification
Hi everyone,
After comparing this virtio-video and virtio-vdec we proposed in a different
thread, I suppose that it's good to use the virtio-video protocol as a basis for
discussion. Then, we can improve this protocol together to merge both
requirements and establish one protocol.
Here, let me share my ideas to improve virtio-video. If my direction looks
reasonable to you, I'm going to update this virtio-video RFC patch as v2 by
adding "Co-authored-by" line in its commit message.
(Or, if there is a better way for this kind of collaboration, please
let me know.)
--
First, I'd like to suggest the following three changes for the general design:
1. Focus on only decoder/encoder functionalities first.
As Tomasz said earlier in this thread, it'd be too complicated to support camera
usage at the same time. So, I'd suggest to make it just a generic mem-to-mem
video processing device protocol for now.
If we finally decide to support camera in this protocol, we can add it later.
2. Only one feature bit can be specified for one device.
I'd like to have a decoder device and encoder device separately.
It'd be natural to assume it because a decoder and an encoder are provided as
different hardware. Also, this assumption will make the protocol and
implementation simpler.
e.g. my inline comments about VIRTIO_VIDEO_T_GET_FUNCS and
virtio_video_function below
3. Separate buffer allocation functionalities from virtio-video protocol.
To support various ways of guest/host buffer sharing, we might want to have a
dedicated buffer sharing device as we're discussing in another thread. Until we
reach consensus there, it'd be good not to have buffer allocation
functionalities in virtio-video.
--
For each control, please see my inline comments for the protocol below.
(I pasted the original RFC content below, as it was lost during the
previous conversation.)
On Wed, Nov 6, 2019 at 1:23 AM Dmitry Sepp wrote:
>
> This patch proposes a virtio specification for a new virtio video
> device.
>
> The virtio video device is an abstract video streaming device that
> operates input and/or output data buffers to share video devices with
> several guests. The device uses descriptor structures to advertise and
> negotiate stream formats and controls. This allows the driver to modify
> the processing logic of the device on a per stream basis. The generic
> nature of the device makes it possible to support the following video
> functions: encoder, decoder, processor, capture, output.
>
> Thank you in advance for any feedback.
>
> Signed-off-by: Dmitry Sepp
> ---
> content.tex | 1 +
> virtio-video.tex | 776 +++
> 2 files changed, 777 insertions(+)
> create mode 100644 virtio-video.tex
>
> diff --git a/content.tex b/content.tex
> index b1ea9b9..6990b5d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5698,6 +5698,7 @@ \subsubsection{Legacy Interface: Framing
> Requirements}\label{sec:Device
> \input{virtio-crypto.tex}
> \input{virtio-vsock.tex}
> \input{virtio-fs.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..84aade8
> --- /dev/null
> +++ b/virtio-video.tex
> @@ -0,0 +1,776 @@
> +\section{Video Device}\label{sec:Device Types / Video Device}
> +
> +The virtio video device is a virtual video streaming device that supports the
> +following functions: encoder, decoder, capture, output.
> +
> +\subsection{Device ID}\label{sec:Device Types / Video Device / Device ID}
> +
> +TBD.
I'm wondering how and when we can determine and reserve this ID?
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Video Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] controlq
> +\item[1] eventq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / Video Device / Feature
> bits}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_F_ENCODER (0)] Video encoder function supported.
> +\item[VIRTIO_VIDEO_F_DECODER (1)] Video decoder function supported.
> +\item[VIRTIO_VIDEO_F_PROCESSOR (2)] Video processor function supported.
> +\item[VIRTIO_VIDEO_F_CAPTURE (3)] Video capture function supported.
> +\item[VIRTIO_VIDEO_F_OUTPUT (4)] Video output function supported.
> +\end{description}
As I suggested above, it'd be good to have only VIRTIO_VIDEO_F_DECODER and
VIRTIO_VIDEO_F_ENCODER for now and prohibit specifying both at the same time.
> +
> +\subsection{Supported functions}\label{sec:Device Types / Video Device /
> Supported video functions}
> +
> +The following video functions are defined:
> +
> +\begin{lstlisting}
> +enum virtio_video_func_type {
> + VIRTIO_VIDEO_FUNC_UNDEFINED = 0,
> +
> + VIRTIO_VIDEO_FUNC_ENCODER = 0x0100,
> + VIRTIO_VIDEO_FUNC_DECODER,
> + VIRTIO_VIDEO_FUNC_PROCESSOR,
> + VIRTIO_VIDEO_FUNC_CAPTURE,
> + VIRTIO_VIDEO_FUNC_OUTPUT,
> +};
> +\end{lstlisting}
> +
>
Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature
Hi, > > > Do we already have a mechanism > > > to import memory to virtio-gpu via importing a bo handle from somewhere > > > else? > > > > Inside the guest? Import is not working yet, that needs another update > > (blob resources) due to current virtio-gpu resources requiring a format > > at creation time and dmabufs not having one. > > > > > Gotcha, I was just poking around related code and it looked like I could > use a resource w/ R8 format and width = bytes. Yep, that trick might work for some use cases. But mapping such an imported dma-buf to a scanout isn't going to work for example ... 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] [PATCH] virtio-gpu: add shared resource feature
On Tue, Dec 3, 2019 at 1:33 AM Gerd Hoffmann wrote: > On Mon, Dec 02, 2019 at 09:24:31AM -0800, Frank Yang wrote: > > Interesting; what's the use case for this? > > Mostly reduce data copying. > > > Do we already have a mechanism > > to import memory to virtio-gpu via importing a bo handle from somewhere > > else? > > Inside the guest? Import is not working yet, that needs another update > (blob resources) due to current virtio-gpu resources requiring a format > at creation time and dmabufs not having one. > > Gotcha, I was just poking around related code and it looked like I could use a resource w/ R8 format and width = bytes. Blob resources would be a more proper way then :) > See https://www.kraxel.org/blog/2019/11/virtio-gpu-status-and-plans/ for > some background. > > Exporting virtio-gpu resources works in the drm-misc-next branch, should > land in mainline this merge window (i.e. 5.5-rc1). > > cheers, > Gerd > >
[virtio-dev] Re: [PATCH v3] content: document speed, duplex
On Tue, 3 Dec 2019 05:39:23 -0500 "Michael S. Tsirkin" wrote: > Document as used by Linux. > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/59 > Signed-off-by: Michael S. Tsirkin > Reviewed-by: Cornelia Huck > --- > > changes from v2: > fix up units > actually there's no way for driver to know that link up > changed: it might have changed back. > make the device requirement a weaker "SHOULD" and instead > ask that driver re-read on each config change notification > > content.tex | 39 +++ > 1 file changed, 39 insertions(+) Looks good to me. - 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 v3] content: document speed, duplex
Document as used by Linux.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/59
Signed-off-by: Michael S. Tsirkin
Reviewed-by: Cornelia Huck
---
changes from v2:
fix up units
actually there's no way for driver to know that link up
changed: it might have changed back.
make the device requirement a weaker "SHOULD" and instead
ask that driver re-read on each config change notification
content.tex | 39 +++
1 file changed, 39 insertions(+)
diff --git a/content.tex b/content.tex
index d68cfaf..f7a6971 100644
--- a/content.tex
+++ b/content.tex
@@ -2823,6 +2823,7 @@ \subsection{Feature bits}\label{sec:Device Types /
Network Device / Feature bits
\item[VIRTIO_NET_F_STANDBY(62)] Device may act as a standby for a primary
device with the same MAC address.
+\item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
\end{description}
\subsubsection{Feature bit requirements}\label{sec:Device Types / Network
Device / Feature bits / Feature bit requirements}
@@ -2882,12 +2883,28 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Network Device
VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
use.
+The following two fields, \field{speed} and \field{duplex}, only
+exist if VIRTIO_NET_F_SPEED_DUPLEX is set.
+
+\field{speed} contains the device speed, in units of 1 MBit per
+second, 0 to 0x7, or 0xf for unknown speed.
+
+\field{duplex} has the values of 0x00 for full duplex, 0x01 for
+half duplex and 0xff for unknown duplex state.
+
+Both \field{speed} and \field{duplex} can change, thus the driver
+is expected to re-read these values after receiving a
+configuration change notification.
+
\begin{lstlisting}
struct virtio_net_config {
u8 mac[6];
le16 status;
le16 max_virtqueue_pairs;
le16 mtu;
+le32 speed;
+u8 duplex;
+
};
\end{lstlisting}
@@ -2916,6 +2933,19 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Network Device
If the driver negotiates the VIRTIO_NET_F_STANDBY feature, the device MAY act
as a standby device for a primary device with the same MAC address.
+If VIRTIO_NET_F_SPEED_DUPLEX has been negotiated, \field{speed}
+MUST contain the device speed, in units of 1 MBit per second, 0 to
+0x7, or 0xf for unknown.
+
+If VIRTIO_NET_F_SPEED_DUPLEX has been negotiated, \field{duplex}
+MUST have the values of 0x00 for full duplex, 0x01 for half
+duplex, or 0xff for unknown.
+
+If VIRTIO_NET_F_SPEED_DUPLEX and VIRTIO_NET_F_STATUS have both
+been negotiated, the device SHOULD NOT change the \field{speed} and
+\field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
+the \field{status}.
+
\drivernormative{\subsubsection}{Device configuration layout}{Device Types /
Network Device / Device configuration layout}
A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
@@ -2940,6 +2970,15 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Network Device
A driver SHOULD negotiate the VIRTIO_NET_F_STANDBY feature if the device
offers it.
+If VIRTIO_NET_F_SPEED_DUPLEX has been negotiated,
+the driver MUST treat any value of \field{speed} above
+0x7fff as well as any value of \field{duplex} not
+matching 0x00 or 0x01 as an unknown value.
+
+If VIRTIO_NET_F_SPEED_DUPLEX has been negotiated, the driver
+SHOULD re-read \field{speed} and \field{duplex} after a
+configuration change notification.
+
\subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device
Types / Network Device / Device configuration layout / Legacy Interface: Device
configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration
layout / Legacy Interface: Device configuration layout}
When using the legacy interface, transitional devices and drivers
--
MST
-
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] virtio-gpu: some edid clarifications
On Thu, Nov 28, 2019 at 12:42:43PM +0100, Gerd Hoffmann wrote: > Add some notes about fetching the EDID information. https://github.com/oasis-tcs/virtio-spec/issues/64 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] [PATCH] virtio-gpu: add shared resource feature
On Mon, Dec 02, 2019 at 09:24:31AM -0800, Frank Yang wrote: > Interesting; what's the use case for this? Mostly reduce data copying. > Do we already have a mechanism > to import memory to virtio-gpu via importing a bo handle from somewhere > else? Inside the guest? Import is not working yet, that needs another update (blob resources) due to current virtio-gpu resources requiring a format at creation time and dmabufs not having one. See https://www.kraxel.org/blog/2019/11/virtio-gpu-status-and-plans/ for some background. Exporting virtio-gpu resources works in the drm-misc-next branch, should land in mainline this merge window (i.e. 5.5-rc1). 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] [PATCH] snd: Add virtio sound device specification
On Mon, Dec 02, 2019 at 02:06:53PM +0100, Anton Yakovlev wrote:
> Hi,
>
> Sorry for the late reply, I was not available for a week.
>
>
> On 25.11.2019 10:31, Gerd Hoffmann wrote:
> >Hi,
> >
> > > > Instead of PCM_OUTPUT and PCM_INPUT I would put the number of input and
> > > > output streams into the device config space (and explicitly allow count
> > > > being zero).
> > >
> > > There are several options for the number of streams:
> > >
> > > 1. The device can provide one input and/or output stream.
> > > 2. The device can provide several input and/or output streams, and all of
> > > them
> > > share the same capabilities.
> > > 3. The device can provide several input and/or output streams, and they
> > > can
> > > have different set of capabilities.
> > >
> > > Overall design depends on chosen option. Current draft is based on p.1.
> > > But we
> > > have never discussed what is the best solution here.
> >
> > (3) looks most useful to me.
>
> Then we need to refactor device configuration. I would propose to put into
> configuration space only a total number of all available PCM streams and
> introduce special control request to query per-stream configuration. A
> response should contain a stream type (input/output) and all its capabilities.
Yes, that will fine work too.
> > > > PCM_MSG -- I would drop the feature bit and make that mandatory, so we
> > > > have a common baseline on which all drivers and devices can agree on.
> > >
> > > Then we need to inform device what "transport" will be in use (I assumed
> > > it
> > > would be feature negotiation).
> >
> > Whenever other transports (i.e. via shared memory) are supported: yes,
> > that should be a feature bit.
> >
> > Not sure about choosing the transport. If both msg (i.e. via virtqueue)
> > and shared memory are available, does it make sense to allow the driver
> > choose the transport each time it starts a stream?
>
> Shared memory based transport in any case will require some additional
> actions. For HOST_MEM case the driver will need to get an access to host
> buffer somehow. In GUEST_MEM case the driver will need to provide a buffer for
> the host.
>
> At first sight, we could extend the set_params request with the
> transport_type field and some additional information.
Or have a per-transport set_params request command.
> For example, in case
> of GUEST_MEM the request could be followed by a buffer sg-list.
I'm not convinced guest_mem is that useful. host_mem allows to give the
guest access to the buffers used by the hosts sound hardware, which is
probably what you need if the MSG transport can't handle the latency
requirements you have.
> > > > > struct virtio_snd_pcm_status {
> > > > > le32 status;
> > > > > le32 actual_length;
> > > > > };
> > > >
> > > > Hmm. Why actual_length? Do we want allow short transfers? Why?
> > > > Wouldn't it be more useful to just fill the buffer completely?
> > >
> > > In current design we have no buffer size requirements. It means, that in
> > > theory the device and the driver could have buffers with different sizes.
> >
> > Should we define that the (virtio) buffer size must be period_bytes then?
>
> It will have no sense, since the driver chooses period_bytes at runtime.
Yep, the driver will choose period_bytes when starting a stream.
The driver will also queue the buffers (for the MSG transport),
so it should be no problem for the driver to make the two match.
> If we gonna introduce any buffer constrains, it must be set by the
> device in a stream configuration.
If we want allow the device specify min/max period_bytes which it can
handle, then yes, that should go into the stream configuration.
Or we use negotiation: driver asks for period_bytes in set-params, the
driver picks the closest period_bytes value it can handle and returns
that.
> > > Also, the capture stream is a special case. Now we don't state explicitly
> > > whether read request is blockable or not.
> >
> > The concept of "blockable" doesn't exist at that level. The driver
> > submits buffers to the device, the device fills them and notifies the
> > driver when the buffer is full. It simply doesn't work like a read(2)
> > syscall.
>
> But you described exactly "blockable" case: an I/O request is completed not
> immediately but upon some condition (the buffer is full). In case of message-
> based transport, both the device and the driver will have its own buffers.
Well, no. The device doesn't need any buffers, it can use the buffers
submitted by the driver. Typical workflow:
(1) The driver puts a bunch of empty buffers into the rx (record/read)
virtqueue (each being period_bytes in size).
(2) The driver starts recording.
(3) The device fills the first buffer with recorded sound data.
(4) When the buffer is full the device returns it to the driver,
takes the next from the virtqueue to continue recording.
(5) The driver takes the filled buffer and does
