[RFC RESEND] virtio-video: Add virtio video device specification

[RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

[Resend after fixing an issue with the virtio-dev mailing list]

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 <dmitry.sepp@opensynergy.com>
---
 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 0000000..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.
+
+\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}
+
+\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}
+
+\subsubsection{Function description}\label{sec:Device Types / Video Device / Supported functions / Function description}
+
+The device reports its configuration using descriptors. A descriptor is a data
+structure with a defined format. Each descriptor begins with a generic virtio
+video descriptor header that contains information about the descriptor type and
+its length.
+
+\begin{lstlisting}
+enum virtio_video_desc_type {
+	VIRTIO_VIDEO_DESC_UNDEFINED = 0,
+
+	VIRTIO_VIDEO_DESC_FRAME_RATE = 0x0100,
+	VIRTIO_VIDEO_DESC_FRAME_SIZE,
+	VIRTIO_VIDEO_DESC_PIX_FORMAT,
+	VIRTIO_VIDEO_DESC_PLANE_FORMAT,
+	VIRTIO_VIDEO_DESC_EXTRAS,
+	VIRTIO_VIDEO_DESC_CAP,
+	VIRTIO_VIDEO_DESC_FUNC,
+	VIRTIO_VIDEO_DESC_PARAMS,
+};
+
+struct virtio_video_desc {
+	__le32 type; /* One of VIRTIO_VIDEO_DESC_* types */
+	__le16 length;
+	__u8 padding[2];
+};
+\end{lstlisting}
+
+Functions describe a set of services that the device offers. In general, the
+device can transform data from one format to another
+(encoder/decoder/processor) or produce/consume data (capture/output).
+
+Functions have the descriptor type VIRTIO_VIDEO_DESC_FUNC. Each
+encoder/decoder/processor function has 2 pins, input and output. Capture
+function has only one input pin, output has only an output pin. Each pin has a
+capability that describes formats this pin can handle. Also, a function can
+have a set of control capabilities that control the process of data
+transformation/production/consumption.
+
+The capability that describes the data format consists of:
+
+\begin{itemize*}
+\item a set of pixel formats that contains one or more:
+\item supported frame sizes that contains one or more:
+\item supported frame rates.
+\end{itemize*}
+
+The description of each function looks as follows:
+
+\begin{itemize*}
+  \item capability (input pin)
+  \begin{itemize*}
+    \item pixel format
+    \begin{itemize*}
+      \item frame size
+        \begin{itemize*}
+          \item frame rate
+          \item ... (other possible frame rates)
+        \end{itemize*}
+      \item ... (other possible frame sizes) 
+      \end{itemize*}
+    \item ... (other possible pixel formats)
+    \end{itemize*}
+  \item capability (output pin)
+  \begin{itemize*}
+    \item ...
+  \end{itemize*}
+  \item ... (other capabilities, control)
+\end{itemize*}
+
+\subsubsection{Encoder specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Encoder specific capabilities}
+
+TBD.
+
+\subsubsection{Decoder specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Decoder specific capabilities}
+
+TBD.
+
+\subsubsection{Processor specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Processor specific capabilities}
+
+TBD.
+
+\subsubsection{Capture specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Capture specific capabilities}
+
+TBD.
+
+\subsubsection{Output specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Output specific capabilities}
+
+TBD.
+
+\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 num_functions;
+	__le32 total_functions_size;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{num_functions}] specifies how many functions are supported by the
+  device.
+\item[\field{total_functions_size}] defines the maximum size of a buffer
+  required to fetch function capabilities.
+\end{description}
+
+\subsection{Device Initialization}\label{sec:Device Types / Video Device / Device Initialization}
+
+\devicenormative{\subsubsection}{Device Initialization}{Device Types / Video Device / Device Initialization}
+
+\begin{itemize*}
+\item The driver SHOULD query information about supported functions from the
+  device using the VIRTIO_VIDEO_T_GET_FUNCS command and use that information
+  for the initial setup.
+\end{itemize*}
+
+\subsection{Device Operation}\label{sec:Device Types / Video Device / Device Operation}
+
+The driver allocates input and/or output buffers (depending on the function)
+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 with a default set of
+parameters determined by the device.
+
+\begin{itemize*}
+\item Create a stream using VIRTIO_VIDEO_T_STREAM_CREATE.
+\end{itemize*}
+
+\subsubsection{Device Operation: Create buffers}
+
+Buffers are used to store the actual data as well as the relevant metadata.
+
+\begin{itemize*}
+\item create a device resource using VIRTIO_VIDEO_T_RESOURCE_CREATE.
+\item create a buffer from the driver's memory, and attach it as backing
+  storage to the resource just created using
+  VIRTIO_VIDEO_REQ_RESOURCE_ATTACH_BACKING. Scatter lists are supported, so the
+  buffer doesn't need to be contiguous in guest physical memory.
+\item set buffer metadata.
+\item create a stream using VIRTIO_VIDEO_T_STREAM_CREATE.
+\end{itemize*}
+
+\subsubsection{Device Operation: Process buffers}
+
+\begin{itemize*}
+\item if the function and the buffer type require so, write data to the buffer memory.
+\item use VIRTIO_VIDEO_T_RESOURCE_QUEUE to queue the buffer for processing in the device.
+\item the request completes asynchronously when the device has finished with the buffer.
+\end{itemize*}
+
+\subsubsection{Device Operation: Stream parameter control}
+
+\begin{itemize*}
+\item use VIRTIO_VIDEO_T_GET_PARAMS to get the current stream input or output
+  pin parameters from the device.
+\item use VIRTIO_VIDEO_T_SET_PARAMS to provide new stream input or output pin
+  parameters to the device.
+\item after setting stream parameters, the driver may issue VIRTIO_VIDEO_T_GET_PARAMS
+  as some parameters can be changed implicitly by the device during the set
+  operation.
+\end{itemize*}
+
+\subsubsection{Device Operation: Buffer processing control}
+
+\begin{itemize*}
+\item use VIRTIO_VIDEO_T_STREAM_START to start buffer processing in the device.
+\item use VIRTIO_VIDEO_T_STREAM_STOP to stop buffer processing in the device.
+\item use VIRTIO_VIDEO_T_STREAM_DRAIN to ask the device to process and return
+  all of the already queued buffers.
+\item use VIRTIO_VIDEO_T_QUEUE_CLEAR to ask the device to return back already
+  queued buffers from the input or the output queue. This also includes input or
+  output buffers that can be currently owned by the device's processing pipeline.
+\end{itemize*}
+
+\subsubsection{Device Operation: Asynchronous events}
+
+While processing buffers, the device can send asynchronous event notifications
+to the driver. The behaviour depends on the exact stream. For example, the
+decoder device sends a resolution change event when it encounters new
+resolution metadata in the stream.
+
+\subsubsection{Device Operation: Request header}
+
+All requests and responses on the control virt queue have a fixed header using
+the following layout structure and definitions:
+
+\begin{lstlisting}
+enum virtio_video_ctrl_type {
+	VIRTIO_VIDEO_CTRL_UNDEFINED = 0,
+
+	/* request */
+	VIRTIO_VIDEO_T_GET_FUNCS = 0x0100,
+	VIRTIO_VIDEO_T_STREAM_CREATE,
+	VIRTIO_VIDEO_T_STREAM_DESTROY,
+	VIRTIO_VIDEO_T_STREAM_START,
+	VIRTIO_VIDEO_T_STREAM_STOP,
+	VIRTIO_VIDEO_T_STREAM_DRAIN,
+	VIRTIO_VIDEO_T_RESOURCE_CREATE,
+	VIRTIO_VIDEO_T_RESOURCE_DESTROY,
+	VIRTIO_VIDEO_T_RESOURCE_ATTACH_BACKING,
+	VIRTIO_VIDEO_T_RESOURCE_DETACH_BACKING,
+	VIRTIO_VIDEO_T_RESOURCE_QUEUE,
+	VIRTIO_VIDEO_T_QUEUE_CLEAR,
+	VIRTIO_VIDEO_T_SET_PARAMS,
+	VIRTIO_VIDEO_T_GET_PARAMS,
+
+	/* response */
+	VIRTIO_VIDEO_S_OK = 0x0200,
+	VIRTIO_VIDEO_S_OK_RESOURCE_QUEUE,
+	VIRTIO_VIDEO_S_OK_GET_PARAMS,
+
+	VIRTIO_VIDEO_S_ERR_UNSPEC = 0x0300,
+	VIRTIO_VIDEO_S_ERR_OUT_OF_MEMORY,
+	VIRTIO_VIDEO_S_ERR_INVALID_FUNCTION_ID,
+	VIRTIO_VIDEO_S_ERR_INVALID_RESOURCE_ID,
+	VIRTIO_VIDEO_S_ERR_INVALID_STREAM_ID,
+	VIRTIO_VIDEO_S_ERR_INVALID_PARAMETER,
+};
+
+struct virtio_video_ctrl_hdr {
+	__le32 type;
+	__le32 stream_id;
+	__le32 function_id;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{type}] the type of the driver request or the device response.
+\item[\field{stream_id}] specifies a target stream.
+\item[\field{function_id}] specifies a function the stream belongs to.
+\end{description}
+
+\subsubsection{Device Operation: controlq}
+
+\begin{description}
+
+\item[VIRTIO_VIDEO_T_GET_FUNCS] Retrieve information about supported functions.
+  No request data (just bare \field{struct virtio_video_ctrl_hdr}). The driver
+  SHOULD ignore the value of the \field{stream_id} parameter. Response type is
+  VIRTIO_VIDEO_S_OK_GET_PARAMS, response data is an array of \field{struct
+  virtio_video_function}.
+
+\begin{lstlisting}
+struct virtio_video_frame_rate {
+	struct virtio_video_desc desc;
+	__le32 min_rate;
+	__le32 max_rate;
+	__le32 step;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_FRAME_RATE.
+\item[\field{min_rate}] minimum supported frame rate.
+\item[\field{max_rate}] maximum supported frame rate.
+\item[\field{step}] frame rate step size, if applicable. Otherwise the device
+  MUST provide \field{min_rate} equal to \field{max_rate}.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_frame_size {
+	struct virtio_video_desc desc;
+	__le32 min_width;
+	__le32 max_width;
+	__le32 step_width;
+	__le32 min_height;
+	__le32 max_height;
+	__le32 step_height;
+	__le32 num_rates;
+	__u8 padding[4];
+	/* Followed by struct virtio_video_frame_rate frame_rates[]; */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_FRAME_SIZE.
+\item[\field{min_width}] minimum supported frame width.
+\item[\field{max_width}] maximum supported frame width..
+\item[\field{step_width}] frame width step size, if applicable. Otherwise the
+  device MUST provide \field{min_width} equal to \field{max_width}.
+\item[\field{min_height}] minimum supported frame height.
+\item[\field{max_height}] maximum supported frame height.
+\item[\field{step_height}] frame height step size, if applicable. Otherwise the
+  device MUST provide \field{min_height} equal to \field{max_height}.
+\item[\field{num_rates}] number of frame rates supported for the given frame size.
+\end{description}
+
+\begin{lstlisting}
+enum virtio_video_pixel_format {
+	VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
+
+	VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
+	VIRTIO_VIDEO_PIX_FMT_NV12,
+	VIRTIO_VIDEO_PIX_FMT_NV21,
+	VIRTIO_VIDEO_PIX_FMT_I420,
+	VIRTIO_VIDEO_PIX_FMT_I422,
+	VIRTIO_VIDEO_PIX_FMT_XBGR,
+};
+
+struct virtio_video_pix_format {
+	struct virtio_video_desc desc;
+	__le32 pixel_format;
+	__le32 num_sizes;
+	/* Followed by struct virtio_video_frame_size frame_sizes[]; */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_PIX_FORMAT.
+\item[\field{pixel_format}] data format for the pin.
+\item[\field{num_sizes}] number of frame rates supported for the given data
+  format.
+\end{description}
+
+\begin{lstlisting}
+enum virtio_video_pin_type {
+	VIRTIO_VIDEO_PIN_UNDEFINED = 0,
+
+	VIRTIO_VIDEO_PIN_INPUT = 0x0100,
+	VIRTIO_VIDEO_PIN_OUTPUT,
+};
+
+struct virtio_video_frame_format {
+	__le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
+	__le32 num_formats;
+	/* Followed by struct virtio_video_pix_format pix_formats[]; */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{pin_type}] pin direction. The device MUST set the value to either
+  VIRTIO_VIDEO_PIN_INPUT or VIRTIO_VIDEO_PIN_OUTPUT.
+\item[\field{num_formats}] number of frame formats supported for the pin.
+\end{description}
+
+\begin{lstlisting}
+enum virtio_video_cap_type {
+	VIRTIO_VIDEO_CAP_UNDEFINED = 0,
+
+	VIRTIO_VIDEO_CAP_PIN_FORMATS = 0x0100,
+	VIRTIO_VIDEO_CAP_CONTROL,
+};
+
+struct virtio_video_capability {
+	struct virtio_video_desc desc;
+	__le32 cap_type; /* One of VIRTIO_VIDEO_CAP_* types */
+	__le32 cap_id;
+	union {
+		struct virtio_video_frame_format frame_format;
+		struct virtio_video_control control;
+	} u;
+};
+\end{lstlisting}
+
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_CAP.
+\item[\field{cap_type}] specifies the type of the device capability.
+\item[\field{cap_id}] internal id of the capability.
+\item[\field{u}] the actual capability description.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_function {
+	struct virtio_video_desc desc;
+	__le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
+	__le32 function_id;
+	struct virtio_video_params in_params;
+	struct virtio_video_params out_params;
+	__le32 num_caps;
+	__u8 padding[4];
+	/* Followed by struct virtio_video_capability video_caps[]; */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_FUNC.
+\item[\field{function_type}] one of the supported function types.
+\item[\field{function_id}] id of the function.
+\item[\field{in_params}] default parameters for the input pin, if applicable.
+  See VIRTIO_VIDEO_T_SET_PARAMS.
+\item[\field{out_params}] default parameters for the output pin, if applicable.
+  See VIRTIO_VIDEO_T_SET_PARAMS.
+\item[\field{num_caps}] number of capabilities (i.e. formats, controls)
+  supported by the function.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_STREAM_CREATE] create a video stream (context) within the
+  device.
+
+\begin{lstlisting}
+/* VIRTIO_VIDEO_T_STREAM_CREATE */
+struct virtio_video_stream_create {
+	struct virtio_video_ctrl_hdr hdr;
+	char debug_name[64];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{debug_name}] a text string for a debug purpose.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_STREAM_DESTROY] destroy a video stream (context) within the
+  device.
+
+\begin{lstlisting}
+struct virtio_video_stream_destroy {
+	struct virtio_video_ctrl_hdr hdr;
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_T_STREAM_START] start processing buffers in the device's
+  queue(s).
+
+\begin{lstlisting}
+struct virtio_video_stream_start {
+	struct virtio_video_ctrl_hdr hdr;
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_T_STREAM_STOP] stop processing buffers in the device's
+  queue(s).
+
+\begin{lstlisting}
+struct virtio_video_stream_stop {
+	struct virtio_video_ctrl_hdr hdr;
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_T_STREAM_DRAIN] ask the device to push all the queued
+  buffers through the pipeline.
+
+\begin{lstlisting}
+struct virtio_video_stream_drain {
+	struct virtio_video_ctrl_hdr hdr;
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_T_RESOURCE_CREATE] create a resource descriptor within the
+  device.
+
+\begin{lstlisting}
+struct virtio_video_resource_create {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 resource_id;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] internal id of the resource.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_RESOURCE_DESTROY] destroy a resource descriptor within the
+  device.
+
+\begin{lstlisting}
+struct virtio_video_resource_destroy {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 resource_id;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] internal id of the resource.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_RESOURCE_ATTACH_BACKING] associate backing pages with a
+  resource.
+
+\begin{lstlisting}
+struct virtio_video_mem_entry {
+	__le64 addr;
+	__le32 length;
+	__u8 padding[4];
+};
+
+struct virtio_video_resource_attach_backing {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 resource_id;
+	__le32 nr_entries;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] internal id of the resource.
+\item[\field{nr_entries}] number of \field{struct virtio_video_mem_entry}
+  memory entries.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_RESOURCE_DETACH_BACKING] Dissociate backing pages from a
+  resource.
+
+\begin{lstlisting}
+struct virtio_video_resource_detach_backing {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 resource_id;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] internal id of the resource.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_RESOURCE_QUEUE] Add a buffer to the device's queue.
+
+\begin{lstlisting}
+#define VIRTIO_VIDEO_MAX_PLANES 8
+
+struct virtio_video_resource_queue {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 resource_id;
+	__le32 function_id;
+	__le64 timestamp;
+	__le32 data_size[VIRTIO_VIDEO_MAX_PLANES];
+	__le32 pin_type;
+	__u8 nr_data_size;
+	__u8 padding[3];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] internal id of the resource.
+\item[\field{function_id}] internal id of the function.
+\item[\field{timestamp}] an abstract sequence counter that can be used for
+  synchronisation.
+\item[\field{data_size}] number of data bytes within a plane.
+\item[\field{pin_type}] pin direction.
+\item[\field{nr_data_size}] number of \field{data_size} entries.
+\end{description}
+
+\begin{lstlisting}
+enum virtio_video_buffer_flag {
+	VIRTIO_VIDEO_BUFFER_F_ERR	= 0x0001,
+	VIRTIO_VIDEO_BUFFER_F_EOS	= 0x0002,
+	/* Encoder only */
+	VIRTIO_VIDEO_BUFFER_IFRAME	= 0x0004,
+	VIRTIO_VIDEO_BUFFER_PFRAME	= 0x0008,
+	VIRTIO_VIDEO_BUFFER_BFRAME	= 0x0010,
+};
+
+struct virtio_video_resource_queue_resp {
+	struct virtio_video_ctrl_hdr hdr;
+	__le64 timestamp;
+	__le32 flags; /* One of VIRTIO_VIDEO_BUFFER_* flags */
+	__le32 size;  /* Encoded size */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{timestamp}] an abstract sequence counter that can be used for
+  synchronisation.
+\item[\field{flags}] mark specific buffers in the sequence.
+\item[\field{size}] data size in the buffer (encoder only).
+\end{description}
+
+The device sends a response to the queue request asynchronously when it has
+finished processing the buffer.
+
+The device SHOULD mark a buffer that triggered a processing error with the
+VIRTIO_VIDEO_BUFFER_F_ERR flag.
+
+The device MUST mark the last buffer with the VIRTIO_VIDEO_BUFFER_F_EOS flag to
+denote completion of the drain sequence.
+
+In case of encoder, to denote a particular frame type the devie MUST mark the
+respective buffer with VIRTIO_VIDEO_BUFFER_IFRAME, VIRTIO_VIDEO_BUFFER_PFRAME,
+VIRTIO_VIDEO_BUFFER_BFRAME.
+
+\item[VIRTIO_VIDEO_T_RESOURCE_QUEUE_CLEAR] Return already queued buffers back
+  from the input or the output queue of the device. The device SHOULD return
+  all of the buffers from the respective queue as soon as possible without
+  pushing the buffers through the processing pipeline.
+
+\begin{lstlisting}
+struct virtio_video_queue_clear {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 pin_type;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{pin_type}] pin direction.
+\end{description}
+
+\item[VIRTIO_VIDEO_T_SET_PARAMS] Change parameters of the input or the output
+  pin of a stream.
+
+\begin{lstlisting}
+enum virtio_video_channel_type {
+	VIRTIO_VIDEO_CHANNEL_UNDEFINED = 0,
+
+	VIRTIO_VIDEO_CHANNEL_Y = 0x0100,
+	VIRTIO_VIDEO_CHANNEL_U,
+	VIRTIO_VIDEO_CHANNEL_V,
+	VIRTIO_VIDEO_CHANNEL_UV,
+	VIRTIO_VIDEO_CHANNEL_VU,
+	VIRTIO_VIDEO_CHANNEL_YUV,
+	VIRTIO_VIDEO_CHANNEL_YVU,
+	VIRTIO_VIDEO_CHANNEL_BGR,
+	VIRTIO_VIDEO_CHANNEL_BGRX,
+};
+
+struct virtio_video_plane_format {
+	struct virtio_video_desc desc;
+	__le32 channel; /* One of VIRTIO_VIDEO_CHANNEL_* types */
+	__le32 plane_size;
+	__le32 stride;
+	__le32 padding;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{desc}] a virtio video descriptor header. The device MUST set the
+  \field{type} to VIRTIO_VIDEO_DESC_PLANE_FORMAT.
+\item[\field{channel}] describes the image channel(s) stored by the plane.
+\item[\field{plane_size}] size of the plane in bytes.
+\item[\field{stride}] stride used for the plane in bytes.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_params {
+	struct virtio_video_desc desc;
+	__le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
+	__le32 frame_rate;
+	__le32 frame_width;
+	__le32 frame_height;
+	__le32 pixel_format;
+	__le32 min_buffers;
+	__le32 num_planes;
+	struct virtio_video_plane_format plane_formats[VIRTIO_VIDEO_MAX_PLANES];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{pin_type}] pin direction.
+\item[\field{frame_rate}] the value to get/set.
+\item[\field{frame_width}] the value to get/set.
+\item[\field{frame_height}] the value to get/set.
+\item[\field{pixel_format}] the value to get/set.
+\item[\field{min_buffers}] minimum buffers required to handle the format (r/o).
+\item[\field{num_planes}] number of planes used to store pixel data (r/o).
+\item[\field{plane_formats}] description of each plane.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_set_params {
+	struct virtio_video_ctrl_hdr hdr;
+	struct virtio_video_params params;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{params}] parameters to set.
+\end{description}
+
+Setting stream parameters might have side effects within the device. For
+example, the device MAY perform alignment of width and height, change the
+number of planes it uses for the format, or do whatever changes that are
+required to continue normal operation using the updated parameters. It is up to
+the driver to check the parameter set after the VIRTIO_VIDEO_T_SET_PARAMS
+request has been issued.
+
+\item[VIRTIO_VIDEO_T_GET_PARAMS] Get parameters of the input or the output pin
+  of a stream.
+
+\begin{lstlisting}
+struct virtio_video_get_params {
+	struct virtio_video_ctrl_hdr hdr;
+	__le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
+};
+
+struct virtio_video_get_params_resp {
+	struct virtio_video_ctrl_hdr hdr;
+	struct virtio_video_params params;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{pin_type}] pin direction.
+\item[\field{params}] parameter values.
+\end{description}
+
+\end{description}
+
+\subsubsection{Device Operation: eventq}
+
+The device can report events on the event queue. The driver initially populates
+the queue with device-writeable buffers. When the device needs to report an
+event, it fills a buffer and notifies the driver. The driver consumes the
+report and adds a new buffer to the virtqueue.
+
+\begin{lstlisting}
+enum virtio_video_event_type {
+	VIRTIO_VIDEO_EVENT_T_UNDEFINED = 0,
+	/* Decoder only */
+	VIRTIO_VIDEO_EVENT_T_RESOLUTION_CHANGED = 0x0100,
+};
+
+struct virtio_video_event {
+	__le32 event_type; /* One of VIRTIO_VIDEO_EVENT_T_* types */
+	__le32 function_id;
+	__le32 stream_id;
+	__u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{event_type}] type of the triggered event .
+\item[\field{function_id}] id of the source function.
+\item[\field{stream_id}] id of the source stream.
+\end{description}
+
+The device MUST send VIRTIO_VIDEO_EVENT_T_RESOLUTION_CHANGED whenever it
+encounters new resolution data in the stream. This includes the case of the
+initial device configuration after metadata has been parsed and the case of
+dynamic resolution change.
+
-- 
2.23.0

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

On Tue, Nov 05, 2019 at 08:19:19PM +0100, Dmitry Sepp wrote:
> [Resend after fixing an issue with the virtio-dev mailing list]
> 
> This patch proposes a virtio specification for a new virtio video
> device.

Hmm, quickly looking over this, it looks simliar to the vdec draft
posted a few weeks ago, with other device variants added but not
fully specified yet.

So, can you clarify the relationship between the two?

thanks,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hello Gerd,

Thank you for your feedback.

There is no relationship between those. As I mentioned earlier. we have also 
been working on a virtio video device at the same time. And there is no 
relationship between the two specs.

I can point you to the differences I see:

virtio-vdec:
1. Both the device and the driver submit requests to each other. For each 
request the response is sent as a separate request.
2. No support for getting/setting video stream parameters. For example 
(decoder): output format (NV12, I420), so the driver cannot really select the 
output format after headers have been parsed.
3. No support for getting plane requirements from the device (sg vs contig, 
size, stride alignment, plane count).
4. In the vdec device Drain and Flush are not separate for each buffer queue. 
So seek and dynamic resolution change (adaptive playback) cannot be 
implemented because 'flush' can have different meaning for a resolution change 
and a seek.
5. Decoder only: new devices will be needed to support encoder, processor or 
capture. Currently input is always a bitstream, output is always a video 
frame. No way set input format (needed for encoder, see 2).

virtio-video:
1. Uses the 'driver requests - device responses' model.
2. Does not  have the logical split of bitstreams and framebuffers, has only a 
generic buffer object.
3. Generic: can support any type of video device right away due to flexibility 
of stream configuration. Both input and output buffer queues can accept 
bitstream and frame buffers and run independently. (more controls need to be 
defined for e.g. camera)
4. Supports seek, drain, dynamic resolution change using an API agnostic 
command set (no v4l2/vaapi or so on remoting).
5. Complex configuration (most likely cannot be simplified for such a complex 
device type).

Best regards,
Dmitry.

On Donnerstag, 7. November 2019 10:56:57 CET Gerd Hoffmann wrote:
> On Tue, Nov 05, 2019 at 08:19:19PM +0100, Dmitry Sepp wrote:
> > [Resend after fixing an issue with the virtio-dev mailing list]
> > 
> > This patch proposes a virtio specification for a new virtio video
> > device.
> 
> Hmm, quickly looking over this, it looks simliar to the vdec draft
> posted a few weeks ago, with other device variants added but not
> fully specified yet.
> 
> So, can you clarify the relationship between the two?
> 
> thanks,
>   Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

On Thu, Nov 07, 2019 at 02:09:34PM +0100, Dmitry Sepp wrote:
> Hello Gerd,
> 
> Thank you for your feedback.
> 
> There is no relationship between those. As I mentioned earlier. we have also 
> been working on a virtio video device at the same time. And there is no 
> relationship between the two specs.

Keiichi, have you looked at the spec?

I think it is useful to have a single device specification for all video
functions given that there is a bunch of common stuff.  Both encoder and
decoder must negotiate video frame and video stream parameters for
example.  Also the virtio-video spec looks like a superset of
virtio-vdec.

Is there any important feature in video-vdec which is missing in
virtio-video?

> virtio-video:
> 1. Uses the 'driver requests - device responses' model.
> 2. Does not  have the logical split of bitstreams and framebuffers, has only a 
> generic buffer object.

Can you give a quick summary on buffer object management?

thanks,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Tomasz Figa]

On Thu, Nov 7, 2019 at 10:09 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hello Gerd,
>
> Thank you for your feedback.
>
> There is no relationship between those. As I mentioned earlier. we have also
> been working on a virtio video device at the same time. And there is no
> relationship between the two specs.
>
> I can point you to the differences I see:
>
> virtio-vdec:
> 1. Both the device and the driver submit requests to each other. For each
> request the response is sent as a separate request.

To be more precise, in vdec there are no responses. The guest sends
commands to the host using one virtqueue. The host signals
asynchronous events, which might not have the exact earlier guest
request associated to them. An example of such special case could be
H.264 framebuffer reordering, where one might end up with a few decode
requests not resulting in any frames being output and then one decode
request that would trigger multiple accumulated frames to be returned.

> 2. No support for getting/setting video stream parameters. For example
> (decoder): output format (NV12, I420), so the driver cannot really select the
> output format after headers have been parsed.

Getting video stream parameters is there, but they are currently left
fully in control of the host video decoder. Ability to select between
multiple possible formats could be worth adding, though.

> 3. No support for getting plane requirements from the device (sg vs contig,
> size, stride alignment, plane count).

There is actually a bigger difference that results in that. Vdec
assumes host-allocated buffers coming from a different device, e.g.
virtio-gpu and the host having the right knowledge to allocate the
buffers correctly. This is related to the fact that it's generally
difficult to convey all the allocation constraints in a generic
manner.

> 4. In the vdec device Drain and Flush are not separate for each buffer queue.
> So seek and dynamic resolution change (adaptive playback) cannot be
> implemented because 'flush' can have different meaning for a resolution change
> and a seek.

That's not true. Drain and flush can be defined very precisely for a
stateful video decoder.

For example, V4L2 defines drain as follows:
https://www.kernel.org/doc/html/latest/media/uapi/v4l/dev-decoder.html#drain
and it's modeled after that in vdec.

There is no flush explicitly defined in V4L2, but that corresponds to
the behavior of STREAMOFF, which drops all the buffers on given queue.
There is no practical use for flushing just one queue in case of a
video decoder, so we decided to simplify it down to a single flush
that fully resets the decoder, which is useful for instantaneous seek.

There is also already infrastructure existing for dynamic resolution
change too, using the stream information host request and drain flow,
which is very similar to how this is done in V4L2:
https://www.kernel.org/doc/html/latest/media/uapi/v4l/dev-decoder.html#dynamic-resolution-change

> 5. Decoder only: new devices will be needed to support encoder, processor or
> capture. Currently input is always a bitstream, output is always a video
> frame. No way set input format (needed for encoder, see 2).

The rationale for this was that this is a point of contact with the
host and a possible attack surface, so having a protocol as specific
as possible makes the attack surface smaller and is easier to validate
in the device implementation.

>
> virtio-video:
> 1. Uses the 'driver requests - device responses' model.
> 2. Does not  have the logical split of bitstreams and framebuffers, has only a
> generic buffer object.
> 3. Generic: can support any type of video device right away due to flexibility
> of stream configuration. Both input and output buffer queues can accept
> bitstream and frame buffers and run independently. (more controls need to be
> defined for e.g. camera)

To fully support real cameras, not just simple UVC webcams, some
mechanism to have multiple output and capture streams synchronized
together would be needed, because Android Camera HALv3 heavily relies
on multiple stream support.

For example, a simple camera application with ZSL (zero shutter lag)
could setup following streams:
1) YUV preview
2) RAW capture
3) RAW output
4) JPEG capture

1) and 2) would operate at camera frame rate, while 3) and 4) would be
given on demand whenever the user presses the shutter button. Presence
of 3) and 4) must not affect 1) and 2), i.e. the preview and raw
capture must continue at camera frame rate.

> 4. Supports seek, drain, dynamic resolution change using an API agnostic
> command set (no v4l2/vaapi or so on remoting).
> 5. Complex configuration (most likely cannot be simplified for such a complex
> device type).
>
> Best regards,
> Dmitry.
>
> On Donnerstag, 7. November 2019 10:56:57 CET Gerd Hoffmann wrote:
> > On Tue, Nov 05, 2019 at 08:19:19PM +0100, Dmitry Sepp wrote:
> > > [Resend after fixing an issue with the virtio-dev mailing list]
> > >
> > > This patch proposes a virtio specification for a new virtio video
> > > device.
> >
> > Hmm, quickly looking over this, it looks simliar to the vdec draft
> > posted a few weeks ago, with other device variants added but not
> > fully specified yet.
> >
> > So, can you clarify the relationship between the two?
> >
> > thanks,
> >   Gerd
>
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Tomasz Figa]

On Fri, Nov 8, 2019 at 4:50 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Nov 07, 2019 at 02:09:34PM +0100, Dmitry Sepp wrote:
> > Hello Gerd,
> >
> > Thank you for your feedback.
> >
> > There is no relationship between those. As I mentioned earlier. we have also
> > been working on a virtio video device at the same time. And there is no
> > relationship between the two specs.
>
> Keiichi, have you looked at the spec?
>
> I think it is useful to have a single device specification for all video
> functions given that there is a bunch of common stuff.  Both encoder and
> decoder must negotiate video frame and video stream parameters for
> example.  Also the virtio-video spec looks like a superset of
> virtio-vdec.
>
> Is there any important feature in video-vdec which is missing in
> virtio-video?
>

I just replied to Dmitry's email with further clarification on some
vdec aspects and rationale behind some of the design decisions. Please
take a look.

I think it should be possible to build one protocol for both decoding
and encoding. Actually virtio-vdec shouldn't need too much
modification to handle encoding. The ability to set operating mode
(decoder vs encoder) and set frame buffer format should be enough.

However I believe that making it as generic as virtio-video adds too
much complexity, increasing the possible attack surface and making it
difficult to validate.

Best regards,
Tomasz

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

> > 1. Both the device and the driver submit requests to each other. For each
> > request the response is sent as a separate request.
> 
> To be more precise, in vdec there are no responses. The guest sends
> commands to the host using one virtqueue. The host signals
> asynchronous events, which might not have the exact earlier guest
> request associated to them.

How do you report errors?  Is there an error event for that?

> An example of such special case could be
> H.264 framebuffer reordering, where one might end up with a few decode
> requests not resulting in any frames being output and then one decode
> request that would trigger multiple accumulated frames to be returned.

Note: this can be done with a request/response model too, by simply
completing the decode request when there is frame data, so in that case
multiple decode requests simply complete at the same time.  Yes, you can
have multiple outstanding requests in virtio.  Out-of-order completion
is also allowed btw.

> > 2. No support for getting/setting video stream parameters. For example
> > (decoder): output format (NV12, I420), so the driver cannot really select the
> > output format after headers have been parsed.
> 
> Getting video stream parameters is there, but they are currently left
> fully in control of the host video decoder. Ability to select between
> multiple possible formats could be worth adding, though.

Nice to see you agree on that one ;)

> > 3. No support for getting plane requirements from the device (sg vs contig,
> > size, stride alignment, plane count).
> 
> There is actually a bigger difference that results in that. Vdec
> assumes host-allocated buffers coming from a different device, e.g.
> virtio-gpu and the host having the right knowledge to allocate the
> buffers correctly. This is related to the fact that it's generally
> difficult to convey all the allocation constraints in a generic
> manner.

Yep, buffer handling is tricky, especially when it comes to decoding
directly to gpu buffers and also when supporting playback of
drm-protected streams where the guest might not be allowed to access
the stream data.

> > 5. Decoder only: new devices will be needed to support encoder, processor or
> > capture. Currently input is always a bitstream, output is always a video
> > frame. No way set input format (needed for encoder, see 2).
> 
> The rationale for this was that this is a point of contact with the
> host and a possible attack surface, so having a protocol as specific
> as possible makes the attack surface smaller and is easier to validate
> in the device implementation.

I think it certainly makes sense to support both video encoding and
video decoding with a single device spec.

For capture (especially camera) and processor things are less clear,
although there is at least some overlap too.  IIRC most of the spec is
"TBD" for those anyway, so I'd suggest to drop them from the spec for
now and focus on the video parts.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

First of all, thanks Dmitry for sharing your protocol here.
I hope we can have a productive discussion to establish a nice protocol by
comparing with virtio-vdec and virtio-video.

On Fri, Nov 8, 2019 at 6:05 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > 1. Both the device and the driver submit requests to each other. For each
> > > request the response is sent as a separate request.
> >
> > To be more precise, in vdec there are no responses. The guest sends
> > commands to the host using one virtqueue. The host signals
> > asynchronous events, which might not have the exact earlier guest
> > request associated to them.
>
> How do you report errors?  Is there an error event for that?

We use a field |result| in virtio_vdec_host_req struct. That's to say
an error is
associated with a host request, not a guest request.
Also, we have VIRTIO_VDEC_HOST_REQ_NOTIFY_GLOBAL_ERROR
event to report an error associated with no host request.

>
> > An example of such special case could be
> > H.264 framebuffer reordering, where one might end up with a few decode
> > requests not resulting in any frames being output and then one decode
> > request that would trigger multiple accumulated frames to be returned.
>
> Note: this can be done with a request/response model too, by simply
> completing the decode request when there is frame data, so in that case
> multiple decode requests simply complete at the same time.  Yes, you can
> have multiple outstanding requests in virtio.  Out-of-order completion
> is also allowed btw.
>
> > > 2. No support for getting/setting video stream parameters. For example
> > > (decoder): output format (NV12, I420), so the driver cannot really select the
> > > output format after headers have been parsed.
> >
> > Getting video stream parameters is there, but they are currently left
> > fully in control of the host video decoder. Ability to select between
> > multiple possible formats could be worth adding, though.
>
> Nice to see you agree on that one ;)
>
> > > 3. No support for getting plane requirements from the device (sg vs contig,
> > > size, stride alignment, plane count).
> >
> > There is actually a bigger difference that results in that. Vdec
> > assumes host-allocated buffers coming from a different device, e.g.
> > virtio-gpu and the host having the right knowledge to allocate the
> > buffers correctly. This is related to the fact that it's generally
> > difficult to convey all the allocation constraints in a generic
> > manner.
>
> Yep, buffer handling is tricky, especially when it comes to decoding
> directly to gpu buffers and also when supporting playback of
> drm-protected streams where the guest might not be allowed to access
> the stream data.
>
> > > 5. Decoder only: new devices will be needed to support encoder, processor or
> > > capture. Currently input is always a bitstream, output is always a video
> > > frame. No way set input format (needed for encoder, see 2).
> >
> > The rationale for this was that this is a point of contact with the
> > host and a possible attack surface, so having a protocol as specific
> > as possible makes the attack surface smaller and is easier to validate
> > in the device implementation.
>
> I think it certainly makes sense to support both video encoding and
> video decoding with a single device spec.
>
> For capture (especially camera) and processor things are less clear,
> although there is at least some overlap too.  IIRC most of the spec is
> "TBD" for those anyway, so I'd suggest to drop them from the spec for
> now and focus on the video parts.
>

I agree that having video codec feature and camera feature in one
protocol sounds too complex.
Also, if we decide to have a buffer sharing device as Gerd suggested
in a different thread,
we'll get less overlaps between video codec feature and camera feature.
e.g. VIRTIO_VIDEO_T_RESOURCE_* would be simplified. (or removed?)


As Tomasz said, I think virtio-vdec can be modified to support encoding as well
without big changes.
I'm happy to update our protocol and driver implementation to support
encoding if
needed.

Best regards,
 Keiichi

> cheers,
>   Gerd
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hello Gerd,

Please see the answer below.

On Freitag, 8. November 2019 08:49:55 CET Gerd Hoffmann wrote:
> On Thu, Nov 07, 2019 at 02:09:34PM +0100, Dmitry Sepp wrote:
> > Hello Gerd,
> > 
> > Thank you for your feedback.
> > 
> > There is no relationship between those. As I mentioned earlier. we have
> > also been working on a virtio video device at the same time. And there is
> > no relationship between the two specs.
> 
> Keiichi, have you looked at the spec?
> 
> I think it is useful to have a single device specification for all video
> functions given that there is a bunch of common stuff.  Both encoder and
> decoder must negotiate video frame and video stream parameters for
> example.  Also the virtio-video spec looks like a superset of
> virtio-vdec.
> 
> Is there any important feature in video-vdec which is missing in
> virtio-video?
> 
> > virtio-video:
> > 1. Uses the 'driver requests - device responses' model.
> > 2. Does not  have the logical split of bitstreams and framebuffers, has
> > only a generic buffer object.
> 
> Can you give a quick summary on buffer object management?
Buffer objects are implemented in a similar way to how it was done for virtio-
gpu. The driver allocates a resource (descriptor) on the device.  Than the 
driver allocates/imports backing pages and attaches those to the resource.

When a buffer is ready for processing from the driver's point of view, the 
driver queues it to the device (please see struct 
virtio_video_resource_queue). Within this structure the driver provides 
required metadata (number of data bytes in the buffer, timestamp/counter and so 
on).

The nice thing about this is that there is no real decode or encode requests. 
The device just fetches the buffer from the input queue, applies some 
transformation according to its function and settings, fetches an output buffer 
from the respective queue, and stores the result in the output buffer (or -s, 
if needed).

The device keeps buffer requests until the respective buffer has been processed 
(i.e. input is consumed or output is written). When the buffer is not needed 
anymore, the virtio_video_resource_queue request completes asynchronously. 
This also makes it possible to easily handle different frame types that are 
decoded out-of-order.

Regards,
Dmitry.

> 
> thanks,
>   Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> > > > 3. No support for getting plane requirements from the device (sg vs contig,
> > > > size, stride alignment, plane count).
> > >
> > > There is actually a bigger difference that results in that. Vdec
> > > assumes host-allocated buffers coming from a different device, e.g.
> > > virtio-gpu and the host having the right knowledge to allocate the
> > > buffers correctly. This is related to the fact that it's generally
> > > difficult to convey all the allocation constraints in a generic
> > > manner.
> >
> > Yep, buffer handling is tricky, especially when it comes to decoding
> > directly to gpu buffers and also when supporting playback of
> > drm-protected streams where the guest might not be allowed to access
> > the stream data.

> Also, if we decide to have a buffer sharing device as Gerd suggested
> in a different thread,
> we'll get less overlaps between video codec feature and camera feature.
> e.g. VIRTIO_VIDEO_T_RESOURCE_* would be simplified. (or removed?)

Disclaimer: Havn't found the time yet to go over both virtio-video and
virtio-vdec in detail.

> As Tomasz said, I think virtio-vdec can be modified to support
> encoding as well without big changes.  I'm happy to update our
> protocol and driver implementation to support encoding if needed.

I think it makes sense to have a rough plan first ;)
Is there a virtio-video implementation too?

When it comes to buffer handling:  I don't like that virtio-vdec depends
on virtio-gpu for buffer handling.  Allowing sharing buffers between
virtio-vdec and virtio-gpu (and possibly other devices) makes sense as
an optional extension.  But IMO the encoder/decoder device should be
able to operate as stand-alone device.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi Gerd,

On Mittwoch, 20. November 2019 12:29:29 CET Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > 3. No support for getting plane requirements from the device (sg vs
> > > > > contig,
> > > > > size, stride alignment, plane count).
> > > > 
> > > > There is actually a bigger difference that results in that. Vdec
> > > > assumes host-allocated buffers coming from a different device, e.g.
> > > > virtio-gpu and the host having the right knowledge to allocate the
> > > > buffers correctly. This is related to the fact that it's generally
> > > > difficult to convey all the allocation constraints in a generic
> > > > manner.
> > > 
> > > Yep, buffer handling is tricky, especially when it comes to decoding
> > > directly to gpu buffers and also when supporting playback of
> > > drm-protected streams where the guest might not be allowed to access
> > > the stream data.
> > 
> > Also, if we decide to have a buffer sharing device as Gerd suggested
> > in a different thread,
> > we'll get less overlaps between video codec feature and camera feature.
> > e.g. VIRTIO_VIDEO_T_RESOURCE_* would be simplified. (or removed?)
> 
> Disclaimer: Havn't found the time yet to go over both virtio-video and
> virtio-vdec in detail.
> 
> > As Tomasz said, I think virtio-vdec can be modified to support
> > encoding as well without big changes.  I'm happy to update our
> > protocol and driver implementation to support encoding if needed.
> 
> I think it makes sense to have a rough plan first ;)
> Is there a virtio-video implementation too?
Yes, of course. We will be ready to share the code very soon.

Regards,
Dmitry.

> 
> When it comes to buffer handling:  I don't like that virtio-vdec depends
> on virtio-gpu for buffer handling.  Allowing sharing buffers between
> virtio-vdec and virtio-gpu (and possibly other devices) makes sense as
> an optional extension.  But IMO the encoder/decoder device should be
> able to operate as stand-alone device.
> 
> cheers,
>   Gerd
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

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 <dmitry.sepp@opensynergy.com> 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 <dmitry.sepp@opensynergy.com>
> ---
>  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 0000000..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}
> +
> +\subsubsection{Function description}\label{sec:Device Types / Video Device / Supported functions / Function description}
> +
> +The device reports its configuration using descriptors. A descriptor is a data
> +structure with a defined format. Each descriptor begins with a generic virtio
> +video descriptor header that contains information about the descriptor type and
> +its length.
> +
> +\begin{lstlisting}
> +enum virtio_video_desc_type {
> +       VIRTIO_VIDEO_DESC_UNDEFINED = 0,
> +
> +       VIRTIO_VIDEO_DESC_FRAME_RATE = 0x0100,
> +       VIRTIO_VIDEO_DESC_FRAME_SIZE,
> +       VIRTIO_VIDEO_DESC_PIX_FORMAT,
> +       VIRTIO_VIDEO_DESC_PLANE_FORMAT,
> +       VIRTIO_VIDEO_DESC_EXTRAS,
> +       VIRTIO_VIDEO_DESC_CAP,
> +       VIRTIO_VIDEO_DESC_FUNC,
> +       VIRTIO_VIDEO_DESC_PARAMS,
> +};
> +
> +struct virtio_video_desc {
> +       __le32 type; /* One of VIRTIO_VIDEO_DESC_* types */
> +       __le16 length;
> +       __u8 padding[2];
> +};
> +\end{lstlisting}
> +
> +Functions describe a set of services that the device offers. In general, the
> +device can transform data from one format to another
> +(encoder/decoder/processor) or produce/consume data (capture/output).
> +
> +Functions have the descriptor type VIRTIO_VIDEO_DESC_FUNC. Each
> +encoder/decoder/processor function has 2 pins, input and output. Capture
> +function has only one input pin, output has only an output pin. Each pin has a
> +capability that describes formats this pin can handle. Also, a function can
> +have a set of control capabilities that control the process of data
> +transformation/production/consumption.
> +
> +The capability that describes the data format consists of:
> +
> +\begin{itemize*}
> +\item a set of pixel formats that contains one or more:
> +\item supported frame sizes that contains one or more:
> +\item supported frame rates.
> +\end{itemize*}
> +
> +The description of each function looks as follows:
> +
> +\begin{itemize*}
> +  \item capability (input pin)
> +  \begin{itemize*}
> +    \item pixel format
> +    \begin{itemize*}
> +      \item frame size
> +        \begin{itemize*}
> +          \item frame rate
> +          \item ... (other possible frame rates)
> +        \end{itemize*}
> +      \item ... (other possible frame sizes)
> +      \end{itemize*}
> +    \item ... (other possible pixel formats)
> +    \end{itemize*}
> +  \item capability (output pin)
> +  \begin{itemize*}
> +    \item ...
> +  \end{itemize*}
> +  \item ... (other capabilities, control)
> +\end{itemize*}
> +
> +\subsubsection{Encoder specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Encoder specific capabilities}
> +
> +TBD.
> +
> +\subsubsection{Decoder specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Decoder specific capabilities}
> +
> +TBD.
> +
> +\subsubsection{Processor specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Processor specific capabilities}
> +
> +TBD.
> +
> +\subsubsection{Capture specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Capture specific capabilities}
> +
> +TBD.
> +
> +\subsubsection{Output specific capabilities}\label{sec:Device Types / Video Device / Supported functions / Output specific capabilities}
> +
> +TBD.
> +
> +\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 num_functions;
> +       __le32 total_functions_size;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{num_functions}] specifies how many functions are supported by the
> +  device.
> +\item[\field{total_functions_size}] defines the maximum size of a buffer
> +  required to fetch function capabilities.
> +\end{description}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Video Device / Device Initialization}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Video Device / Device Initialization}
> +
> +\begin{itemize*}
> +\item The driver SHOULD query information about supported functions from the
> +  device using the VIRTIO_VIDEO_T_GET_FUNCS command and use that information
> +  for the initial setup.
> +\end{itemize*}

Once we have a decoder device and encoder device separately, T_GET_FUNCS is no
longer needed. Rather, we want to have a control to get virtio_video_capability
instead of virtio_video_function. Probably, we can call it T_GET_CAPS.

> +
> +\subsection{Device Operation}\label{sec:Device Types / Video Device / Device Operation}
> +
> +The driver allocates input and/or output buffers (depending on the function)
> +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 with a default set of
> +parameters determined by the device.
> +
> +\begin{itemize*}
> +\item Create a stream using VIRTIO_VIDEO_T_STREAM_CREATE.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Create buffers}
> +
> +Buffers are used to store the actual data as well as the relevant metadata.
> +
> +\begin{itemize*}
> +\item create a device resource using VIRTIO_VIDEO_T_RESOURCE_CREATE.
> +\item create a buffer from the driver's memory, and attach it as backing
> +  storage to the resource just created using
> +  VIRTIO_VIDEO_REQ_RESOURCE_ATTACH_BACKING. Scatter lists are supported, so the
> +  buffer doesn't need to be contiguous in guest physical memory.
> +\item set buffer metadata.
> +\item create a stream using VIRTIO_VIDEO_T_STREAM_CREATE.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Process buffers}
> +
> +\begin{itemize*}
> +\item if the function and the buffer type require so, write data to the buffer memory.
> +\item use VIRTIO_VIDEO_T_RESOURCE_QUEUE to queue the buffer for processing in the device.
> +\item the request completes asynchronously when the device has finished with the buffer.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Stream parameter control}
> +
> +\begin{itemize*}
> +\item use VIRTIO_VIDEO_T_GET_PARAMS to get the current stream input or output
> +  pin parameters from the device.
> +\item use VIRTIO_VIDEO_T_SET_PARAMS to provide new stream input or output pin
> +  parameters to the device.
> +\item after setting stream parameters, the driver may issue VIRTIO_VIDEO_T_GET_PARAMS
> +  as some parameters can be changed implicitly by the device during the set
> +  operation.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Buffer processing control}
> +
> +\begin{itemize*}
> +\item use VIRTIO_VIDEO_T_STREAM_START to start buffer processing in the device.
> +\item use VIRTIO_VIDEO_T_STREAM_STOP to stop buffer processing in the device.
> +\item use VIRTIO_VIDEO_T_STREAM_DRAIN to ask the device to process and return
> +  all of the already queued buffers.
> +\item use VIRTIO_VIDEO_T_QUEUE_CLEAR to ask the device to return back already
> +  queued buffers from the input or the output queue. This also includes input or
> +  output buffers that can be currently owned by the device's processing pipeline.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: Asynchronous events}
> +
> +While processing buffers, the device can send asynchronous event notifications
> +to the driver. The behaviour depends on the exact stream. For example, the
> +decoder device sends a resolution change event when it encounters new
> +resolution metadata in the stream.
> +
> +\subsubsection{Device Operation: Request header}
> +
> +All requests and responses on the control virt queue have a fixed header using
> +the following layout structure and definitions:
> +
> +\begin{lstlisting}
> +enum virtio_video_ctrl_type {
> +       VIRTIO_VIDEO_CTRL_UNDEFINED = 0,
> +
> +       /* request */
> +       VIRTIO_VIDEO_T_GET_FUNCS = 0x0100,
> +       VIRTIO_VIDEO_T_STREAM_CREATE,
> +       VIRTIO_VIDEO_T_STREAM_DESTROY,
> +       VIRTIO_VIDEO_T_STREAM_START,
> +       VIRTIO_VIDEO_T_STREAM_STOP,
> +       VIRTIO_VIDEO_T_STREAM_DRAIN,
> +       VIRTIO_VIDEO_T_RESOURCE_CREATE,
> +       VIRTIO_VIDEO_T_RESOURCE_DESTROY,
> +       VIRTIO_VIDEO_T_RESOURCE_ATTACH_BACKING,
> +       VIRTIO_VIDEO_T_RESOURCE_DETACH_BACKING,
> +       VIRTIO_VIDEO_T_RESOURCE_QUEUE,
> +       VIRTIO_VIDEO_T_QUEUE_CLEAR,
> +       VIRTIO_VIDEO_T_SET_PARAMS,
> +       VIRTIO_VIDEO_T_GET_PARAMS,
> +
> +       /* response */
> +       VIRTIO_VIDEO_S_OK = 0x0200,
> +       VIRTIO_VIDEO_S_OK_RESOURCE_QUEUE,
> +       VIRTIO_VIDEO_S_OK_GET_PARAMS,
> +
> +       VIRTIO_VIDEO_S_ERR_UNSPEC = 0x0300,
> +       VIRTIO_VIDEO_S_ERR_OUT_OF_MEMORY,
> +       VIRTIO_VIDEO_S_ERR_INVALID_FUNCTION_ID,
> +       VIRTIO_VIDEO_S_ERR_INVALID_RESOURCE_ID,
> +       VIRTIO_VIDEO_S_ERR_INVALID_STREAM_ID,
> +       VIRTIO_VIDEO_S_ERR_INVALID_PARAMETER,
> +};
> +
> +struct virtio_video_ctrl_hdr {
> +       __le32 type;
> +       __le32 stream_id;
> +       __le32 function_id;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{type}] the type of the driver request or the device response.
> +\item[\field{stream_id}] specifies a target stream.
> +\item[\field{function_id}] specifies a function the stream belongs to.
> +\end{description}
> +
> +\subsubsection{Device Operation: controlq}
> +
> +\begin{description}
> +
> +\item[VIRTIO_VIDEO_T_GET_FUNCS] Retrieve information about supported functions.
> +  No request data (just bare \field{struct virtio_video_ctrl_hdr}). The driver
> +  SHOULD ignore the value of the \field{stream_id} parameter. Response type is
> +  VIRTIO_VIDEO_S_OK_GET_PARAMS, response data is an array of \field{struct
> +  virtio_video_function}.
> +
> +\begin{lstlisting}
> +struct virtio_video_frame_rate {
> +       struct virtio_video_desc desc;
> +       __le32 min_rate;
> +       __le32 max_rate;
> +       __le32 step;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_FRAME_RATE.
> +\item[\field{min_rate}] minimum supported frame rate.
> +\item[\field{max_rate}] maximum supported frame rate.
> +\item[\field{step}] frame rate step size, if applicable. Otherwise the device
> +  MUST provide \field{min_rate} equal to \field{max_rate}.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_frame_size {
> +       struct virtio_video_desc desc;
> +       __le32 min_width;
> +       __le32 max_width;
> +       __le32 step_width;
> +       __le32 min_height;
> +       __le32 max_height;
> +       __le32 step_height;
> +       __le32 num_rates;
> +       __u8 padding[4];
> +       /* Followed by struct virtio_video_frame_rate frame_rates[]; */
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_FRAME_SIZE.
> +\item[\field{min_width}] minimum supported frame width.
> +\item[\field{max_width}] maximum supported frame width..
> +\item[\field{step_width}] frame width step size, if applicable. Otherwise the
> +  device MUST provide \field{min_width} equal to \field{max_width}.
> +\item[\field{min_height}] minimum supported frame height.
> +\item[\field{max_height}] maximum supported frame height.
> +\item[\field{step_height}] frame height step size, if applicable. Otherwise the
> +  device MUST provide \field{min_height} equal to \field{max_height}.
> +\item[\field{num_rates}] number of frame rates supported for the given frame size.
> +\end{description}
> +
> +\begin{lstlisting}
> +enum virtio_video_pixel_format {
> +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> +
> +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> +       VIRTIO_VIDEO_PIX_FMT_NV12,
> +       VIRTIO_VIDEO_PIX_FMT_NV21,
> +       VIRTIO_VIDEO_PIX_FMT_I420,
> +       VIRTIO_VIDEO_PIX_FMT_I422,
> +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> +};

I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
mapping from formats to integers.
Also, I suppose the word "pixel formats" means only raw (decoded) formats.
But, it can be encoded format like H.264. So, I guess "image format" or
"fourcc" is a better word choice.

> +
> +struct virtio_video_pix_format {
> +       struct virtio_video_desc desc;
> +       __le32 pixel_format;
> +       __le32 num_sizes;
> +       /* Followed by struct virtio_video_frame_size frame_sizes[]; */
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_PIX_FORMAT.
> +\item[\field{pixel_format}] data format for the pin.
> +\item[\field{num_sizes}] number of frame rates supported for the given data
> +  format.
> +\end{description}
> +
> +\begin{lstlisting}
> +enum virtio_video_pin_type {
> +       VIRTIO_VIDEO_PIN_UNDEFINED = 0,
> +
> +       VIRTIO_VIDEO_PIN_INPUT = 0x0100,
> +       VIRTIO_VIDEO_PIN_OUTPUT,
> +};

Just curious, what type of direction can possibly be added in the future?

> +
> +struct virtio_video_frame_format {
> +       __le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
> +       __le32 num_formats;
> +       /* Followed by struct virtio_video_pix_format pix_formats[]; */
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{pin_type}] pin direction. The device MUST set the value to either
> +  VIRTIO_VIDEO_PIN_INPUT or VIRTIO_VIDEO_PIN_OUTPUT.
> +\item[\field{num_formats}] number of frame formats supported for the pin.
> +\end{description}
> +
> +\begin{lstlisting}
> +enum virtio_video_cap_type {
> +       VIRTIO_VIDEO_CAP_UNDEFINED = 0,
> +
> +       VIRTIO_VIDEO_CAP_PIN_FORMATS = 0x0100,
> +       VIRTIO_VIDEO_CAP_CONTROL,
> +};
> +
> +struct virtio_video_capability {
> +       struct virtio_video_desc desc;
> +       __le32 cap_type; /* One of VIRTIO_VIDEO_CAP_* types */
> +       __le32 cap_id;
> +       union {
> +               struct virtio_video_frame_format frame_format;
> +               struct virtio_video_control control;
> +       } u;
> +};
> +\end{lstlisting}

What is virtio_video_control for? I couldn't find the definition of this struct.

> +
> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_CAP.
> +\item[\field{cap_type}] specifies the type of the device capability.
> +\item[\field{cap_id}] internal id of the capability.
> +\item[\field{u}] the actual capability description.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_function {
> +       struct virtio_video_desc desc;
> +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> +       __le32 function_id;
> +       struct virtio_video_params in_params;
> +       struct virtio_video_params out_params;
> +       __le32 num_caps;
> +       __u8 padding[4];
> +       /* Followed by struct virtio_video_capability video_caps[]; */
> +};
> +\end{lstlisting}

If one device only has one functionality, virtio_video_function's fields will be
no longer needed except in_params and out_params. So, we'd be able to remove
virtio_video_function and have in_params and out_params in
virtio_video_capability instead.

In addition, we should add a field to represent supported combinations of source
format and destination format. In virtio-vdec, we have it as |mask| field in
virtio_vdec_format_desc. So, we can add a le32 field here in
virtio-video for the
same purpose.

> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_FUNC.
> +\item[\field{function_type}] one of the supported function types.
> +\item[\field{function_id}] id of the function.
> +\item[\field{in_params}] default parameters for the input pin, if applicable.
> +  See VIRTIO_VIDEO_T_SET_PARAMS.
> +\item[\field{out_params}] default parameters for the output pin, if applicable.
> +  See VIRTIO_VIDEO_T_SET_PARAMS.
> +\item[\field{num_caps}] number of capabilities (i.e. formats, controls)
> +  supported by the function.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_STREAM_CREATE] create a video stream (context) within the
> +  device.
> +
> +\begin{lstlisting}
> +/* VIRTIO_VIDEO_T_STREAM_CREATE */
> +struct virtio_video_stream_create {
> +       struct virtio_video_ctrl_hdr hdr;
> +       char debug_name[64];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{debug_name}] a text string for a debug purpose.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_STREAM_DESTROY] destroy a video stream (context) within the
> +  device.
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_destroy {
> +       struct virtio_video_ctrl_hdr hdr;
> +};
> +\end{lstlisting}
> +
> +\item[VIRTIO_VIDEO_T_STREAM_START] start processing buffers in the device's
> +  queue(s).
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_start {
> +       struct virtio_video_ctrl_hdr hdr;
> +};
> +\end{lstlisting}
> +
> +\item[VIRTIO_VIDEO_T_STREAM_STOP] stop processing buffers in the device's
> +  queue(s).
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_stop {
> +       struct virtio_video_ctrl_hdr hdr;
> +};
> +\end{lstlisting}

I doubt whether T_STREAM_START and T_STREAM_STOP provide any useful
functionalities. Mem-to-mem devices process data when input or output buffers
are available. So, I don't think the notion of "start" and "stop" make
much sense.

> +
> +\item[VIRTIO_VIDEO_T_STREAM_DRAIN] ask the device to push all the queued
> +  buffers through the pipeline.
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_drain {
> +       struct virtio_video_ctrl_hdr hdr;
> +};
> +\end{lstlisting}
> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_CREATE] create a resource descriptor within the
> +  device.
> +
> +\begin{lstlisting}
> +struct virtio_video_resource_create {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 resource_id;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] internal id of the resource.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_DESTROY] destroy a resource descriptor within the
> +  device.
> +
> +\begin{lstlisting}
> +struct virtio_video_resource_destroy {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 resource_id;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] internal id of the resource.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_ATTACH_BACKING] associate backing pages with a
> +  resource.
> +
> +\begin{lstlisting}
> +struct virtio_video_mem_entry {
> +       __le64 addr;
> +       __le32 length;
> +       __u8 padding[4];
> +};
> +
> +struct virtio_video_resource_attach_backing {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 resource_id;
> +       __le32 nr_entries;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] internal id of the resource.
> +\item[\field{nr_entries}] number of \field{struct virtio_video_mem_entry}
> +  memory entries.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_DETACH_BACKING] Dissociate backing pages from a
> +  resource.
> +
> +\begin{lstlisting}
> +struct virtio_video_resource_detach_backing {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 resource_id;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] internal id of the resource.
> +\end{description}

I suppose that it'd be better not to have the above series of T_RESOURCE
controls at least until we reach a conclusion in the thread of buffer-sharing
device. If we end up concluding this type of controls is the best way, we'll be
able to revisit here.

> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_QUEUE] Add a buffer to the device's queue.
> +
> +\begin{lstlisting}
> +#define VIRTIO_VIDEO_MAX_PLANES 8
> +
> +struct virtio_video_resource_queue {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 resource_id;
> +       __le32 function_id;
> +       __le64 timestamp;
> +       __le32 data_size[VIRTIO_VIDEO_MAX_PLANES];
> +       __le32 pin_type;
> +       __u8 nr_data_size;
> +       __u8 padding[3];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] internal id of the resource.
> +\item[\field{function_id}] internal id of the function.
> +\item[\field{timestamp}] an abstract sequence counter that can be used for
> +  synchronisation.
> +\item[\field{data_size}] number of data bytes within a plane.
> +\item[\field{pin_type}] pin direction.
> +\item[\field{nr_data_size}] number of \field{data_size} entries.
> +\end{description}
> +
> +\begin{lstlisting}
> +enum virtio_video_buffer_flag {
> +       VIRTIO_VIDEO_BUFFER_F_ERR       = 0x0001,
> +       VIRTIO_VIDEO_BUFFER_F_EOS       = 0x0002,
> +       /* Encoder only */
> +       VIRTIO_VIDEO_BUFFER_IFRAME      = 0x0004,
> +       VIRTIO_VIDEO_BUFFER_PFRAME      = 0x0008,
> +       VIRTIO_VIDEO_BUFFER_BFRAME      = 0x0010,
> +};
> +
> +struct virtio_video_resource_queue_resp {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le64 timestamp;
> +       __le32 flags; /* One of VIRTIO_VIDEO_BUFFER_* flags */
> +       __le32 size;  /* Encoded size */
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{timestamp}] an abstract sequence counter that can be used for
> +  synchronisation.
> +\item[\field{flags}] mark specific buffers in the sequence.
> +\item[\field{size}] data size in the buffer (encoder only).
> +\end{description}
> +
> +The device sends a response to the queue request asynchronously when it has
> +finished processing the buffer.
> +
> +The device SHOULD mark a buffer that triggered a processing error with the
> +VIRTIO_VIDEO_BUFFER_F_ERR flag.
> +
> +The device MUST mark the last buffer with the VIRTIO_VIDEO_BUFFER_F_EOS flag to
> +denote completion of the drain sequence.
> +
> +In case of encoder, to denote a particular frame type the devie MUST mark the
> +respective buffer with VIRTIO_VIDEO_BUFFER_IFRAME, VIRTIO_VIDEO_BUFFER_PFRAME,
> +VIRTIO_VIDEO_BUFFER_BFRAME.
> +
> +\item[VIRTIO_VIDEO_T_RESOURCE_QUEUE_CLEAR] Return already queued buffers back
> +  from the input or the output queue of the device. The device SHOULD return
> +  all of the buffers from the respective queue as soon as possible without
> +  pushing the buffers through the processing pipeline.
> +
> +\begin{lstlisting}
> +struct virtio_video_queue_clear {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 pin_type;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{pin_type}] pin direction.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_T_SET_PARAMS] Change parameters of the input or the output
> +  pin of a stream.
> +
> +\begin{lstlisting}
> +enum virtio_video_channel_type {
> +       VIRTIO_VIDEO_CHANNEL_UNDEFINED = 0,
> +
> +       VIRTIO_VIDEO_CHANNEL_Y = 0x0100,
> +       VIRTIO_VIDEO_CHANNEL_U,
> +       VIRTIO_VIDEO_CHANNEL_V,
> +       VIRTIO_VIDEO_CHANNEL_UV,
> +       VIRTIO_VIDEO_CHANNEL_VU,
> +       VIRTIO_VIDEO_CHANNEL_YUV,
> +       VIRTIO_VIDEO_CHANNEL_YVU,
> +       VIRTIO_VIDEO_CHANNEL_BGR,
> +       VIRTIO_VIDEO_CHANNEL_BGRX,
> +};
> +
> +struct virtio_video_plane_format {
> +       struct virtio_video_desc desc;
> +       __le32 channel; /* One of VIRTIO_VIDEO_CHANNEL_* types */
> +       __le32 plane_size;
> +       __le32 stride;
> +       __le32 padding;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{desc}] a virtio video descriptor header. The device MUST set the
> +  \field{type} to VIRTIO_VIDEO_DESC_PLANE_FORMAT.
> +\item[\field{channel}] describes the image channel(s) stored by the plane.
> +\item[\field{plane_size}] size of the plane in bytes.
> +\item[\field{stride}] stride used for the plane in bytes.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_params {
> +       struct virtio_video_desc desc;
> +       __le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
> +       __le32 frame_rate;
> +       __le32 frame_width;
> +       __le32 frame_height;
> +       __le32 pixel_format;
> +       __le32 min_buffers;
> +       __le32 num_planes;
> +       struct virtio_video_plane_format plane_formats[VIRTIO_VIDEO_MAX_PLANES];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{pin_type}] pin direction.
> +\item[\field{frame_rate}] the value to get/set.
> +\item[\field{frame_width}] the value to get/set.
> +\item[\field{frame_height}] the value to get/set.
> +\item[\field{pixel_format}] the value to get/set.
> +\item[\field{min_buffers}] minimum buffers required to handle the format (r/o).
> +\item[\field{num_planes}] number of planes used to store pixel data (r/o).
> +\item[\field{plane_formats}] description of each plane.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_set_params {
> +       struct virtio_video_ctrl_hdr hdr;
> +       struct virtio_video_params params;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{params}] parameters to set.
> +\end{description}
> +
> +Setting stream parameters might have side effects within the device. For
> +example, the device MAY perform alignment of width and height, change the
> +number of planes it uses for the format, or do whatever changes that are
> +required to continue normal operation using the updated parameters. It is up to
> +the driver to check the parameter set after the VIRTIO_VIDEO_T_SET_PARAMS
> +request has been issued.

Just to confirm, setting parameters can have side effects on the opposite
direction of stream, right? In other words, SET_PARAMS for input should affect
the result of GET_PARAMS for output. If so, I'd write it explicitly.

> +
> +\item[VIRTIO_VIDEO_T_GET_PARAMS] Get parameters of the input or the output pin
> +  of a stream.
> +
> +\begin{lstlisting}
> +struct virtio_video_get_params {
> +       struct virtio_video_ctrl_hdr hdr;
> +       __le32 pin_type; /* One of VIRTIO_VIDEO_PIN_* types */
> +};
> +
> +struct virtio_video_get_params_resp {
> +       struct virtio_video_ctrl_hdr hdr;
> +       struct virtio_video_params params;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{pin_type}] pin direction.
> +\item[\field{params}] parameter values.
> +\end{description}
> +
> +\end{description}
> +
> +\subsubsection{Device Operation: eventq}
> +
> +The device can report events on the event queue. The driver initially populates
> +the queue with device-writeable buffers. When the device needs to report an
> +event, it fills a buffer and notifies the driver. The driver consumes the
> +report and adds a new buffer to the virtqueue.
> +
> +\begin{lstlisting}
> +enum virtio_video_event_type {
> +       VIRTIO_VIDEO_EVENT_T_UNDEFINED = 0,
> +       /* Decoder only */
> +       VIRTIO_VIDEO_EVENT_T_RESOLUTION_CHANGED = 0x0100,
> +};

It'd be good to add an event to report a host-side error which is associated
with no request. Probably, we can add an event like
VIRTIO_VIDEO_EVENT_T_ERR_UNSPEC for now.

> +
> +struct virtio_video_event {
> +       __le32 event_type; /* One of VIRTIO_VIDEO_EVENT_T_* types */
> +       __le32 function_id;
> +       __le32 stream_id;
> +       __u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{event_type}] type of the triggered event .
> +\item[\field{function_id}] id of the source function.
> +\item[\field{stream_id}] id of the source stream.
> +\end{description}
> +
> +The device MUST send VIRTIO_VIDEO_EVENT_T_RESOLUTION_CHANGED whenever it
> +encounters new resolution data in the stream. This includes the case of the
> +initial device configuration after metadata has been parsed and the case of
> +dynamic resolution change.
> +
> --
> 2.23.0
>

Best regards,
Keiichi


On Thu, Nov 21, 2019 at 7:55 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Gerd,
>
> On Mittwoch, 20. November 2019 12:29:29 CET Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > > > 3. No support for getting plane requirements from the device (sg vs
> > > > > > contig,
> > > > > > size, stride alignment, plane count).
> > > > >
> > > > > There is actually a bigger difference that results in that. Vdec
> > > > > assumes host-allocated buffers coming from a different device, e.g.
> > > > > virtio-gpu and the host having the right knowledge to allocate the
> > > > > buffers correctly. This is related to the fact that it's generally
> > > > > difficult to convey all the allocation constraints in a generic
> > > > > manner.
> > > >
> > > > Yep, buffer handling is tricky, especially when it comes to decoding
> > > > directly to gpu buffers and also when supporting playback of
> > > > drm-protected streams where the guest might not be allowed to access
> > > > the stream data.
> > >
> > > Also, if we decide to have a buffer sharing device as Gerd suggested
> > > in a different thread,
> > > we'll get less overlaps between video codec feature and camera feature.
> > > e.g. VIRTIO_VIDEO_T_RESOURCE_* would be simplified. (or removed?)
> >
> > Disclaimer: Havn't found the time yet to go over both virtio-video and
> > virtio-vdec in detail.
> >
> > > As Tomasz said, I think virtio-vdec can be modified to support
> > > encoding as well without big changes.  I'm happy to update our
> > > protocol and driver implementation to support encoding if needed.
> >
> > I think it makes sense to have a rough plan first ;)
> > Is there a virtio-video implementation too?
> Yes, of course. We will be ready to share the code very soon.
>
> Regards,
> Dmitry.
>
> >
> > When it comes to buffer handling:  I don't like that virtio-vdec depends
> > on virtio-gpu for buffer handling.  Allowing sharing buffers between
> > virtio-vdec and virtio-gpu (and possibly other devices) makes sense as
> > an optional extension.  But IMO the encoder/decoder device should be
> > able to operate as stand-alone device.
> >
> > cheers,
> >   Gerd
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> 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.

Agree.

> 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.

Hmm, modern GPUs support both encoding and decoding ...

I don't think we should bake that restriction into the specification.
It probably makes sense to use one virtqueue per function though, that
will simplify dispatching in both host and guest.

> 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.

I think virtio-video should be able to work as stand-alone device,
so we need some way to allocate buffers ...

Buffer sharing with other devices can be added later.

> > +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?

Grab the next free, update the spec accordingly, submit the one-line
patch.

> > +\begin{lstlisting}
> > +enum virtio_video_pixel_format {
> > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > +
> > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > +};
> 
> I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> mapping from formats to integers.
> Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> But, it can be encoded format like H.264. So, I guess "image format" or
> "fourcc" is a better word choice.

Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?

> > +\begin{lstlisting}
> > +struct virtio_video_function {
> > +       struct virtio_video_desc desc;
> > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > +       __le32 function_id;
> > +       struct virtio_video_params in_params;
> > +       struct virtio_video_params out_params;
> > +       __le32 num_caps;
> > +       __u8 padding[4];
> > +       /* Followed by struct virtio_video_capability video_caps[]; */
> > +};
> > +\end{lstlisting}
> 
> If one device only has one functionality, virtio_video_function's fields will be
> no longer needed except in_params and out_params. So, we'd be able to remove
> virtio_video_function and have in_params and out_params in
> virtio_video_capability instead.

Same goes for per-function virtqueues (used virtqueue implies function).

> > +\begin{lstlisting}
> > +struct virtio_video_resource_detach_backing {
> > +       struct virtio_video_ctrl_hdr hdr;
> > +       __le32 resource_id;
> > +       __u8 padding[4];
> > +};
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > +\item[\field{resource_id}] internal id of the resource.
> > +\end{description}
> 
> I suppose that it'd be better not to have the above series of T_RESOURCE
> controls at least until we reach a conclusion in the thread of buffer-sharing
> device. If we end up concluding this type of controls is the best way, we'll be
> able to revisit here.

Well.  For buffer management there are a bunch of options.

 (1) Simply stick the buffers (well, pointers to the buffer pages) into
     the virtqueue.  This is the standard virtio way.

 (2) Create resources, then put the resource ids into the virtqueue.
     virtio-gpu uses that model.  First, because virtio-gpu needs an id
     to reference resources in the rendering command stream
     (virtio-video doesn't need this).  Also because (some kinds of)
     resources are around for a long time and the guest-physical ->
     host-virtual mapping needs to be done only once that way (which
     I think would be the case for virtio-video too because v4l2
     re-uses buffers in robin-round fashion).  Drawback is this
     assumes shared memory between host and guest (which is the case
     in typical use cases but it is not mandated by the virtio spec).

 (3) Import external resources (from virtio-gpu for example).
     Out of scope for now, will probably added as optional feature
     later.

I guess long-term we want support either (1)+(3) or (2)+(3).

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Enrico Granata]

On Wed, Dec 4, 2019 at 1:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > 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.
>
> Agree.
>
> > 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.
>
> Hmm, modern GPUs support both encoding and decoding ...
>
> I don't think we should bake that restriction into the specification.
> It probably makes sense to use one virtqueue per function though, that
> will simplify dispatching in both host and guest.
>
> > 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.
>
> I think virtio-video should be able to work as stand-alone device,
> so we need some way to allocate buffers ...
>
> Buffer sharing with other devices can be added later.
>
> > > +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?
>
> Grab the next free, update the spec accordingly, submit the one-line
> patch.
>
> > > +\begin{lstlisting}
> > > +enum virtio_video_pixel_format {
> > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > +
> > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > +};
> >
> > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > mapping from formats to integers.
> > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > But, it can be encoded format like H.264. So, I guess "image format" or
> > "fourcc" is a better word choice.
>
> Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
>
> > > +\begin{lstlisting}
> > > +struct virtio_video_function {
> > > +       struct virtio_video_desc desc;
> > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > +       __le32 function_id;
> > > +       struct virtio_video_params in_params;
> > > +       struct virtio_video_params out_params;
> > > +       __le32 num_caps;
> > > +       __u8 padding[4];
> > > +       /* Followed by struct virtio_video_capability video_caps[]; */
> > > +};
> > > +\end{lstlisting}
> >
> > If one device only has one functionality, virtio_video_function's fields will be
> > no longer needed except in_params and out_params. So, we'd be able to remove
> > virtio_video_function and have in_params and out_params in
> > virtio_video_capability instead.
>
> Same goes for per-function virtqueues (used virtqueue implies function).
>
> > > +\begin{lstlisting}
> > > +struct virtio_video_resource_detach_backing {
> > > +       struct virtio_video_ctrl_hdr hdr;
> > > +       __le32 resource_id;
> > > +       __u8 padding[4];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +\item[\field{resource_id}] internal id of the resource.
> > > +\end{description}
> >
> > I suppose that it'd be better not to have the above series of T_RESOURCE
> > controls at least until we reach a conclusion in the thread of buffer-sharing
> > device. If we end up concluding this type of controls is the best way, we'll be
> > able to revisit here.
>

As an interim solution, this form of "manual resource backing-store
management" could be specified as a feature flag.
Once there is an established solution for buffer sharing, we would
then go ahead and introduce a new feature flag for "works with the
buffer sharing mechanism", as an alternative to this manual mechanism.

wdyt?

> Well.  For buffer management there are a bunch of options.
>
>  (1) Simply stick the buffers (well, pointers to the buffer pages) into
>      the virtqueue.  This is the standard virtio way.
>
>  (2) Create resources, then put the resource ids into the virtqueue.
>      virtio-gpu uses that model.  First, because virtio-gpu needs an id
>      to reference resources in the rendering command stream
>      (virtio-video doesn't need this).  Also because (some kinds of)
>      resources are around for a long time and the guest-physical ->
>      host-virtual mapping needs to be done only once that way (which
>      I think would be the case for virtio-video too because v4l2
>      re-uses buffers in robin-round fashion).  Drawback is this
>      assumes shared memory between host and guest (which is the case
>      in typical use cases but it is not mandated by the virtio spec).
>
>  (3) Import external resources (from virtio-gpu for example).
>      Out of scope for now, will probably added as optional feature
>      later.
>
> I guess long-term we want support either (1)+(3) or (2)+(3).
>
> cheers,
>   Gerd
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

Hi,

On Thu, Dec 5, 2019 at 4:12 AM Enrico Granata <egranata@google.com> wrote:
>
> On Wed, Dec 4, 2019 at 1:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > 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.
> >
> > Agree.
> >
> > > 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.
> >
> > Hmm, modern GPUs support both encoding and decoding ...
> >
> > I don't think we should bake that restriction into the specification.
> > It probably makes sense to use one virtqueue per function though, that
> > will simplify dispatching in both host and guest.
> >

It'd be a better idea to have a set of virtqueues for each function.
So, we will have 2 * (# of features provided) virtqueues, as one
function requires controlq
and eventq.

> > > 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.
> >
> > I think virtio-video should be able to work as stand-alone device,
> > so we need some way to allocate buffers ...
> >
> > Buffer sharing with other devices can be added later.

Good point. Then, we should have T_RESOURCE_CREATE and T_RESOURCE_DESTROY
at least.

> >
> > > > +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?
> >
> > Grab the next free, update the spec accordingly, submit the one-line
> > patch.

Thanks. I will do so at the next version of the patch.

> >
> > > > +\begin{lstlisting}
> > > > +enum virtio_video_pixel_format {
> > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > +
> > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > +};
> > >
> > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > mapping from formats to integers.
> > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > "fourcc" is a better word choice.
> >
> > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> >

Fourcc is used for both raw and coded formats.
I'm not sure if it makes much sense to define different enums for raw
and coded formats, as
we reuse any other structs for both types of formats.

What I'd suggest is like this:

#define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))

enum virtio_video_fourcc {
    VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,

    /* Coded formats */
    VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
    ...

    /* Raw formats */
    VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
    ...
}

> > > > +\begin{lstlisting}
> > > > +struct virtio_video_function {
> > > > +       struct virtio_video_desc desc;
> > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > > +       __le32 function_id;
> > > > +       struct virtio_video_params in_params;
> > > > +       struct virtio_video_params out_params;
> > > > +       __le32 num_caps;
> > > > +       __u8 padding[4];
> > > > +       /* Followed by struct virtio_video_capability video_caps[]; */
> > > > +};
> > > > +\end{lstlisting}
> > >
> > > If one device only has one functionality, virtio_video_function's fields will be
> > > no longer needed except in_params and out_params. So, we'd be able to remove
> > > virtio_video_function and have in_params and out_params in
> > > virtio_video_capability instead.
> >
> > Same goes for per-function virtqueues (used virtqueue implies function).
> >
> > > > +\begin{lstlisting}
> > > > +struct virtio_video_resource_detach_backing {
> > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > +       __le32 resource_id;
> > > > +       __u8 padding[4];
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +\begin{description}
> > > > +\item[\field{resource_id}] internal id of the resource.
> > > > +\end{description}
> > >
> > > I suppose that it'd be better not to have the above series of T_RESOURCE
> > > controls at least until we reach a conclusion in the thread of buffer-sharing
> > > device. If we end up concluding this type of controls is the best way, we'll be
> > > able to revisit here.
> >
>
> As an interim solution, this form of "manual resource backing-store
> management" could be specified as a feature flag.
> Once there is an established solution for buffer sharing, we would
> then go ahead and introduce a new feature flag for "works with the
> buffer sharing mechanism", as an alternative to this manual mechanism.
>
> wdyt?

It'd be a good idea to change buffer management method by a feature flag.

>
> > Well.  For buffer management there are a bunch of options.
> >
> >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> >      the virtqueue.  This is the standard virtio way.
> >
> >  (2) Create resources, then put the resource ids into the virtqueue.
> >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> >      to reference resources in the rendering command stream
> >      (virtio-video doesn't need this).  Also because (some kinds of)
> >      resources are around for a long time and the guest-physical ->
> >      host-virtual mapping needs to be done only once that way (which
> >      I think would be the case for virtio-video too because v4l2
> >      re-uses buffers in robin-round fashion).  Drawback is this
> >      assumes shared memory between host and guest (which is the case
> >      in typical use cases but it is not mandated by the virtio spec).
> >
> >  (3) Import external resources (from virtio-gpu for example).
> >      Out of scope for now, will probably added as optional feature
> >      later.
> >
> > I guess long-term we want support either (1)+(3) or (2)+(3).
> >

In the first version of spec, we might want to support the minimal workable set
of controls. Then, we'll be able to add additional controls with a new feature
flag as Enrico suggested.

So, the problem is what's the simplest scenario and which types of controls are
required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
and T_RESOURCE_DESTROY. We might need to add some fields in the structs, though.
If so, we should add only these two controls first.

Best regards,
Keiichi

> > cheers,
> >   Gerd
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> > > > > +\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?
> > >
> > > Grab the next free, update the spec accordingly, submit the one-line
> > > patch.
> 
> Thanks. I will do so at the next version of the patch.

No.  Submit as separate one-liner patch which does nothing but grabbing
an ID.

> > > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > > mapping from formats to integers.
> > > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > > "fourcc" is a better word choice.
> > >
> > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > >
> 
> Fourcc is used for both raw and coded formats.
> I'm not sure if it makes much sense to define different enums for raw
> and coded formats, as
> we reuse any other structs for both types of formats.
> 
> What I'd suggest is like this:
> 
> #define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
> 
> enum virtio_video_fourcc {
>     VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,
> 
>     /* Coded formats */
>     VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
>     ...
> 
>     /* Raw formats */
>     VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
>     ...
> }

Ok, that'll work.

I've linked fourcc to drm fourcc codes in my head, and drm hasn't codes
for the compressed formats.

When defining things this way we should of course make sure that the raw
format codes are identical to the ones drm uses.

Is there a formal standard for these codes btw?

> > As an interim solution, this form of "manual resource backing-store
> > management" could be specified as a feature flag.
> > Once there is an established solution for buffer sharing, we would
> > then go ahead and introduce a new feature flag for "works with the
> > buffer sharing mechanism", as an alternative to this manual mechanism.
> >
> > wdyt?
> 
> It'd be a good idea to change buffer management method by a feature flag.

I don't think so.  A device might want support multiple kinds of buffer
management, most notably both their own buffers and imported buffers.
Indicating which methods are available can be done with feature flags,
but actually picking one not.

> > > Well.  For buffer management there are a bunch of options.
> > >
> > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > >      the virtqueue.  This is the standard virtio way.
> > >
> > >  (2) Create resources, then put the resource ids into the virtqueue.
> > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > >      to reference resources in the rendering command stream
> > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > >      resources are around for a long time and the guest-physical ->
> > >      host-virtual mapping needs to be done only once that way (which
> > >      I think would be the case for virtio-video too because v4l2
> > >      re-uses buffers in robin-round fashion).  Drawback is this
> > >      assumes shared memory between host and guest (which is the case
> > >      in typical use cases but it is not mandated by the virtio spec).
> > >
> > >  (3) Import external resources (from virtio-gpu for example).
> > >      Out of scope for now, will probably added as optional feature
> > >      later.
> > >
> > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > >
> 
> In the first version of spec, we might want to support the minimal workable set
> of controls. Then, we'll be able to add additional controls with a new feature
> flag as Enrico suggested.
> 
> So, the problem is what's the simplest scenario and which types of controls are
> required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
> and T_RESOURCE_DESTROY.

For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
to the buffer pages.  No resource management needed.

For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
where RESOURCE_CREATE passes the scatter list of buffer pages to the
host and QUEUE_RESOURCE will carry just the resource id.

For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

Hi,

On Fri, Dec 6, 2019 at 4:32 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > > > +\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?
> > > >
> > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > patch.
> >
> > Thanks. I will do so at the next version of the patch.
>
> No.  Submit as separate one-liner patch which does nothing but grabbing
> an ID.

Thanks. I sent https://markmail.org/message/jpjekiq7c4gkp6jt

>
> > > > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > > > mapping from formats to integers.
> > > > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > > > "fourcc" is a better word choice.
> > > >
> > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > > >
> >
> > Fourcc is used for both raw and coded formats.
> > I'm not sure if it makes much sense to define different enums for raw
> > and coded formats, as
> > we reuse any other structs for both types of formats.
> >
> > What I'd suggest is like this:
> >
> > #define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
> >
> > enum virtio_video_fourcc {
> >     VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,
> >
> >     /* Coded formats */
> >     VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
> >     ...
> >
> >     /* Raw formats */
> >     VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
> >     ...
> > }
>
> Ok, that'll work.
>
> I've linked fourcc to drm fourcc codes in my head, and drm hasn't codes
> for the compressed formats.
>
> When defining things this way we should of course make sure that the raw
> format codes are identical to the ones drm uses.
>

Yes. For Linux, though we have different constants for drm foucc and
video fourcc,
actual values are identical.
(e.g. DRM_FORMAT_NV12 and V4L2_PIX_FMT_NV12)
Then, we will have one more constant representing the same format for virtio.

> Is there a formal standard for these codes btw?

RFC2361 defines FOURCCs for several formats, but it's not a formal
standard iiuc.
https://tools.ietf.org/html/rfc2361

>
> > > As an interim solution, this form of "manual resource backing-store
> > > management" could be specified as a feature flag.
> > > Once there is an established solution for buffer sharing, we would
> > > then go ahead and introduce a new feature flag for "works with the
> > > buffer sharing mechanism", as an alternative to this manual mechanism.
> > >
> > > wdyt?
> >
> > It'd be a good idea to change buffer management method by a feature flag.
>
> I don't think so.  A device might want support multiple kinds of buffer
> management, most notably both their own buffers and imported buffers.
> Indicating which methods are available can be done with feature flags,
> but actually picking one not.
>

Ah, you're right. Then, we might want to extend SET_PARAM or add a new control
to specify a way of buffer management.

> > > > Well.  For buffer management there are a bunch of options.
> > > >
> > > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > > >      the virtqueue.  This is the standard virtio way.
> > > >
> > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > > >      to reference resources in the rendering command stream
> > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > >      resources are around for a long time and the guest-physical ->
> > > >      host-virtual mapping needs to be done only once that way (which
> > > >      I think would be the case for virtio-video too because v4l2
> > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > >      assumes shared memory between host and guest (which is the case
> > > >      in typical use cases but it is not mandated by the virtio spec).
> > > >
> > > >  (3) Import external resources (from virtio-gpu for example).
> > > >      Out of scope for now, will probably added as optional feature
> > > >      later.
> > > >
> > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > >
> >
> > In the first version of spec, we might want to support the minimal workable set
> > of controls. Then, we'll be able to add additional controls with a new feature
> > flag as Enrico suggested.
> >
> > So, the problem is what's the simplest scenario and which types of controls are
> > required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
> > and T_RESOURCE_DESTROY.
>
> For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> to the buffer pages.  No resource management needed.
>
> For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> where RESOURCE_CREATE passes the scatter list of buffer pages to the
> host and QUEUE_RESOURCE will carry just the resource id.
>
> For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
>

Thanks for the clarification.
On second thought, however, I'm wondering if we should keep all
RESOURCE controls.
This should be an option (4).
Even though (1) and (2) may be simpler, nobody wants to implement it so far.
Then, (4) would be the only scenario which virtio-video can work as a
stand alone and we have users of.

As a result, the initial version or virtio-video will
* keep 6 RESOURCE commands as this patch originally proposed,
* add a feature flag to show supported buffer management method, and
* add a field to specify buffer management method in
virtio_video_params, but only one method is defined.

WDYT?

Best regards,
Keiichi


Best regards,
Keiichi

> cheers,
>   Gerd
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Enrico Granata]

On Fri, Dec 6, 2019 at 4:30 AM Keiichi Watanabe <keiichiw@chromium.org> wrote:
>
> Hi,
>
> On Fri, Dec 6, 2019 at 4:32 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > > > +\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?
> > > > >
> > > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > > patch.
> > >
> > > Thanks. I will do so at the next version of the patch.
> >
> > No.  Submit as separate one-liner patch which does nothing but grabbing
> > an ID.
>
> Thanks. I sent https://markmail.org/message/jpjekiq7c4gkp6jt
>
> >
> > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > > > > mapping from formats to integers.
> > > > > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > > > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > > > > "fourcc" is a better word choice.
> > > > >
> > > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > > > >
> > >
> > > Fourcc is used for both raw and coded formats.
> > > I'm not sure if it makes much sense to define different enums for raw
> > > and coded formats, as
> > > we reuse any other structs for both types of formats.
> > >
> > > What I'd suggest is like this:
> > >
> > > #define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
> > >
> > > enum virtio_video_fourcc {
> > >     VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,
> > >
> > >     /* Coded formats */
> > >     VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
> > >     ...
> > >
> > >     /* Raw formats */
> > >     VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
> > >     ...
> > > }
> >
> > Ok, that'll work.
> >
> > I've linked fourcc to drm fourcc codes in my head, and drm hasn't codes
> > for the compressed formats.
> >
> > When defining things this way we should of course make sure that the raw
> > format codes are identical to the ones drm uses.
> >
>
> Yes. For Linux, though we have different constants for drm foucc and
> video fourcc,
> actual values are identical.
> (e.g. DRM_FORMAT_NV12 and V4L2_PIX_FMT_NV12)
> Then, we will have one more constant representing the same format for virtio.
>
> > Is there a formal standard for these codes btw?
>
> RFC2361 defines FOURCCs for several formats, but it's not a formal
> standard iiuc.
> https://tools.ietf.org/html/rfc2361
>
> >
> > > > As an interim solution, this form of "manual resource backing-store
> > > > management" could be specified as a feature flag.
> > > > Once there is an established solution for buffer sharing, we would
> > > > then go ahead and introduce a new feature flag for "works with the
> > > > buffer sharing mechanism", as an alternative to this manual mechanism.
> > > >
> > > > wdyt?
> > >
> > > It'd be a good idea to change buffer management method by a feature flag.
> >
> > I don't think so.  A device might want support multiple kinds of buffer
> > management, most notably both their own buffers and imported buffers.
> > Indicating which methods are available can be done with feature flags,
> > but actually picking one not.
> >
>
> Ah, you're right. Then, we might want to extend SET_PARAM or add a new control
> to specify a way of buffer management.
>

I think we need both. Feature flag(s) would give out the list of
supported mechanism for a given device and driver.
Out of the intersection, a new control can be used in order to pick
the one actually being used for a given transaction.

> > > > > Well.  For buffer management there are a bunch of options.
> > > > >
> > > > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > > > >      the virtqueue.  This is the standard virtio way.
> > > > >
> > > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > > > >      to reference resources in the rendering command stream
> > > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > > >      resources are around for a long time and the guest-physical ->
> > > > >      host-virtual mapping needs to be done only once that way (which
> > > > >      I think would be the case for virtio-video too because v4l2
> > > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > > >      assumes shared memory between host and guest (which is the case
> > > > >      in typical use cases but it is not mandated by the virtio spec).
> > > > >
> > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > >      Out of scope for now, will probably added as optional feature
> > > > >      later.
> > > > >
> > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > >
> > >
> > > In the first version of spec, we might want to support the minimal workable set
> > > of controls. Then, we'll be able to add additional controls with a new feature
> > > flag as Enrico suggested.
> > >
> > > So, the problem is what's the simplest scenario and which types of controls are
> > > required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
> > > and T_RESOURCE_DESTROY.
> >
> > For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> > to the buffer pages.  No resource management needed.
> >
> > For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> > where RESOURCE_CREATE passes the scatter list of buffer pages to the
> > host and QUEUE_RESOURCE will carry just the resource id.
> >
> > For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
> >
>
> Thanks for the clarification.
> On second thought, however, I'm wondering if we should keep all
> RESOURCE controls.
> This should be an option (4).
> Even though (1) and (2) may be simpler, nobody wants to implement it so far.
> Then, (4) would be the only scenario which virtio-video can work as a
> stand alone and we have users of.
>
> As a result, the initial version or virtio-video will
> * keep 6 RESOURCE commands as this patch originally proposed,
> * add a feature flag to show supported buffer management method, and
> * add a field to specify buffer management method in
> virtio_video_params, but only one method is defined.
>
> WDYT?
>
> Best regards,
> Keiichi
>
>
> Best regards,
> Keiichi
>
> > cheers,
> >   Gerd
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> > For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> > to the buffer pages.  No resource management needed.
> >
> > For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> > where RESOURCE_CREATE passes the scatter list of buffer pages to the
> > host and QUEUE_RESOURCE will carry just the resource id.
> >
> > For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
> >
> 
> Thanks for the clarification.
> On second thought, however, I'm wondering if we should keep all
> RESOURCE controls.
> This should be an option (4).

Well, that's actually (2), aka "we use RESOURCE_* commands to manage
resources".  With the commands in the latest draft that would be:

  (1) RESOURCE_CREATE
  (2) RESOURCE_ATTACH_BACKING
  (3) RESOURCE_QUEUE (resource can be used multiple times here)
  (4) RESOURCE_DETACH_BACKING
  (5) RESOURCE_DESTROY

I'm not sure we need the separate *_BACKING commands.  I think we could
also have RESOURCE_CREATE pass the buffer pages scatter list instead.

Why it is done this way?  Just because the design was copied from
virtio-gpu?  Or is there some specific reason?

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi,

On Montag, 9. Dezember 2019 11:46:15 CET Gerd Hoffmann wrote:
>   Hi,
> 
> > > For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> > > to the buffer pages.  No resource management needed.
> > > 
> > > For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> > > where RESOURCE_CREATE passes the scatter list of buffer pages to the
> > > host and QUEUE_RESOURCE will carry just the resource id.
> > > 
> > > For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
> > 
> > Thanks for the clarification.
> > On second thought, however, I'm wondering if we should keep all
> > RESOURCE controls.
> > This should be an option (4).
> 
> Well, that's actually (2), aka "we use RESOURCE_* commands to manage
> resources".  With the commands in the latest draft that would be:
> 
>   (1) RESOURCE_CREATE
>   (2) RESOURCE_ATTACH_BACKING
>   (3) RESOURCE_QUEUE (resource can be used multiple times here)
>   (4) RESOURCE_DETACH_BACKING
>   (5) RESOURCE_DESTROY
> 
> I'm not sure we need the separate *_BACKING commands.  I think we could
> also have RESOURCE_CREATE pass the buffer pages scatter list instead.
> 
> Why it is done this way?  Just because the design was copied from
> virtio-gpu?  Or is there some specific reason?

Yes, the design was just derived from virtio-gpu at early stages.

I do agree we should merge the two steps.

> 
> cheers,
>   Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

Hi,

On Mon, Dec 9, 2019 at 8:38 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi,
>
> On Montag, 9. Dezember 2019 11:46:15 CET Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> > > > to the buffer pages.  No resource management needed.
> > > >
> > > > For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> > > > where RESOURCE_CREATE passes the scatter list of buffer pages to the
> > > > host and QUEUE_RESOURCE will carry just the resource id.
> > > >
> > > > For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
> > >
> > > Thanks for the clarification.
> > > On second thought, however, I'm wondering if we should keep all
> > > RESOURCE controls.
> > > This should be an option (4).
> >
> > Well, that's actually (2), aka "we use RESOURCE_* commands to manage
> > resources".  With the commands in the latest draft that would be:
> >
> >   (1) RESOURCE_CREATE
> >   (2) RESOURCE_ATTACH_BACKING
> >   (3) RESOURCE_QUEUE (resource can be used multiple times here)
> >   (4) RESOURCE_DETACH_BACKING
> >   (5) RESOURCE_DESTROY
> >
> > I'm not sure we need the separate *_BACKING commands.  I think we could
> > also have RESOURCE_CREATE pass the buffer pages scatter list instead.
> >
> > Why it is done this way?  Just because the design was copied from
> > virtio-gpu?  Or is there some specific reason?
>
> Yes, the design was just derived from virtio-gpu at early stages.
>
> I do agree we should merge the two steps.

Thanks for the explanation.  Merging them sounds good to me.

-Keiichi

>
> >
> > cheers,
> >   Gerd
>
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

On Sat, Dec 7, 2019 at 12:50 AM Enrico Granata <egranata@google.com> wrote:
>
> On Fri, Dec 6, 2019 at 4:30 AM Keiichi Watanabe <keiichiw@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, Dec 6, 2019 at 4:32 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > > > > +\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?
> > > > > >
> > > > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > > > patch.
> > > >
> > > > Thanks. I will do so at the next version of the patch.
> > >
> > > No.  Submit as separate one-liner patch which does nothing but grabbing
> > > an ID.
> >
> > Thanks. I sent https://markmail.org/message/jpjekiq7c4gkp6jt
> >
> > >
> > > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > > > > > mapping from formats to integers.
> > > > > > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > > > > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > > > > > "fourcc" is a better word choice.
> > > > > >
> > > > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > > > > >
> > > >
> > > > Fourcc is used for both raw and coded formats.
> > > > I'm not sure if it makes much sense to define different enums for raw
> > > > and coded formats, as
> > > > we reuse any other structs for both types of formats.
> > > >
> > > > What I'd suggest is like this:
> > > >
> > > > #define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
> > > >
> > > > enum virtio_video_fourcc {
> > > >     VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,
> > > >
> > > >     /* Coded formats */
> > > >     VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
> > > >     ...
> > > >
> > > >     /* Raw formats */
> > > >     VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
> > > >     ...
> > > > }
> > >
> > > Ok, that'll work.
> > >
> > > I've linked fourcc to drm fourcc codes in my head, and drm hasn't codes
> > > for the compressed formats.
> > >
> > > When defining things this way we should of course make sure that the raw
> > > format codes are identical to the ones drm uses.
> > >
> >
> > Yes. For Linux, though we have different constants for drm foucc and
> > video fourcc,
> > actual values are identical.
> > (e.g. DRM_FORMAT_NV12 and V4L2_PIX_FMT_NV12)
> > Then, we will have one more constant representing the same format for virtio.
> >
> > > Is there a formal standard for these codes btw?
> >
> > RFC2361 defines FOURCCs for several formats, but it's not a formal
> > standard iiuc.
> > https://tools.ietf.org/html/rfc2361
> >
> > >
> > > > > As an interim solution, this form of "manual resource backing-store
> > > > > management" could be specified as a feature flag.
> > > > > Once there is an established solution for buffer sharing, we would
> > > > > then go ahead and introduce a new feature flag for "works with the
> > > > > buffer sharing mechanism", as an alternative to this manual mechanism.
> > > > >
> > > > > wdyt?
> > > >
> > > > It'd be a good idea to change buffer management method by a feature flag.
> > >
> > > I don't think so.  A device might want support multiple kinds of buffer
> > > management, most notably both their own buffers and imported buffers.
> > > Indicating which methods are available can be done with feature flags,
> > > but actually picking one not.
> > >
> >
> > Ah, you're right. Then, we might want to extend SET_PARAM or add a new control
> > to specify a way of buffer management.
> >
>
> I think we need both. Feature flag(s) would give out the list of
> supported mechanism for a given device and driver.
> Out of the intersection, a new control can be used in order to pick
> the one actually being used for a given transaction.

Ah, I meant that we need to extend a control in addition to a feature
flag. Sorry for the
unclear reply.

On second thought, I wonder if we can set a type of buffer management methods in
STREAM_CREATE by adding fields like this:

enum virtio_video_buffer_type {
        VIRTIO_VIDEO_BUF_TYPE_ALLOCATE,
        /* VIRTIO_VIDEO_BUF_TYPE_IMPORT will be added */
};

struct virtio_video_stream_create {
        struct virtio_video_ctrl_hdr hdr;
        le32 in_buf_type;  /* One of VIRTIO_VIDEO_BUF_TYPE_* */
        le32 out_buf_type;
        char debug_name[64];
};

wdyt?

-Keiichi

>
> > > > > > Well.  For buffer management there are a bunch of options.
> > > > > >
> > > > > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > > > > >      the virtqueue.  This is the standard virtio way.
> > > > > >
> > > > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > > > > >      to reference resources in the rendering command stream
> > > > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > > > >      resources are around for a long time and the guest-physical ->
> > > > > >      host-virtual mapping needs to be done only once that way (which
> > > > > >      I think would be the case for virtio-video too because v4l2
> > > > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > > > >      assumes shared memory between host and guest (which is the case
> > > > > >      in typical use cases but it is not mandated by the virtio spec).
> > > > > >
> > > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > > >      Out of scope for now, will probably added as optional feature
> > > > > >      later.
> > > > > >
> > > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > >
> > > >
> > > > In the first version of spec, we might want to support the minimal workable set
> > > > of controls. Then, we'll be able to add additional controls with a new feature
> > > > flag as Enrico suggested.
> > > >
> > > > So, the problem is what's the simplest scenario and which types of controls are
> > > > required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
> > > > and T_RESOURCE_DESTROY.
> > >
> > > For (1) you'll simply do a QUEUE_BUFFER.  The command carries references
> > > to the buffer pages.  No resource management needed.
> > >
> > > For (2) you'll have RESOURCE_CREATE + RESOURCE_DESTROY + QUEUE_RESOURCE,
> > > where RESOURCE_CREATE passes the scatter list of buffer pages to the
> > > host and QUEUE_RESOURCE will carry just the resource id.
> > >
> > > For (3) you'll have RESOURCE_IMPORT + RESOURCE_DESTROY + QUEUE_RESOURCE.
> > >
> >
> > Thanks for the clarification.
> > On second thought, however, I'm wondering if we should keep all
> > RESOURCE controls.
> > This should be an option (4).
> > Even though (1) and (2) may be simpler, nobody wants to implement it so far.
> > Then, (4) would be the only scenario which virtio-video can work as a
> > stand alone and we have users of.
> >
> > As a result, the initial version or virtio-video will
> > * keep 6 RESOURCE commands as this patch originally proposed,
> > * add a feature flag to show supported buffer management method, and
> > * add a field to specify buffer management method in
> > virtio_video_params, but only one method is defined.
> >
> > WDYT?
> >
> > Best regards,
> > Keiichi
> >
> >
> > Best regards,
> > Keiichi
> >
> > > cheers,
> > >   Gerd
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hello,

I'd like to invite everyone to share ideas regarding required encoder features 
in this separate sub-tree.

In general, encoder devices are more complex compared to decoders. So the 
question I'd like to rise is in what way we define the minimal subset of 
features to be implemented by the virtio-video.

We may look at the following to define the set of features:
1. USB video, 2.3.6 Encoding Unit [1].
2. Android 10 Compatibility Definition [2].

Would be nice to hear about any specific requirements from the Chromium team as 
well.

[1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
[2] https://source.android.com/compatibility/android-cdd#5_2_video_encoding

Thank you.

Best regards,
Dmitry.

On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
>   Hi,
> 
> > 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.
> Agree.
> 
> > 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.
> 
> Hmm, modern GPUs support both encoding and decoding ...
> 
> I don't think we should bake that restriction into the specification.
> It probably makes sense to use one virtqueue per function though, that
> will simplify dispatching in both host and guest.
> 
> > 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.
> 
> I think virtio-video should be able to work as stand-alone device,
> so we need some way to allocate buffers ...
> 
> Buffer sharing with other devices can be added later.
> 
> > > +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?
> 
> Grab the next free, update the spec accordingly, submit the one-line
> patch.
> 
> > > +\begin{lstlisting}
> > > +enum virtio_video_pixel_format {
> > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > +
> > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > +};
> > 
> > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > mapping from formats to integers.
> > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > But, it can be encoded format like H.264. So, I guess "image format" or
> > "fourcc" is a better word choice.
> 
> Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> 
> > > +\begin{lstlisting}
> > > +struct virtio_video_function {
> > > +       struct virtio_video_desc desc;
> > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > +       __le32 function_id;
> > > +       struct virtio_video_params in_params;
> > > +       struct virtio_video_params out_params;
> > > +       __le32 num_caps;
> > > +       __u8 padding[4];
> > > +       /* Followed by struct virtio_video_capability video_caps[]; */
> > > +};
> > > +\end{lstlisting}
> > 
> > If one device only has one functionality, virtio_video_function's fields
> > will be no longer needed except in_params and out_params. So, we'd be
> > able to remove virtio_video_function and have in_params and out_params in
> > virtio_video_capability instead.
> 
> Same goes for per-function virtqueues (used virtqueue implies function).
> 
> > > +\begin{lstlisting}
> > > +struct virtio_video_resource_detach_backing {
> > > +       struct virtio_video_ctrl_hdr hdr;
> > > +       __le32 resource_id;
> > > +       __u8 padding[4];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +\item[\field{resource_id}] internal id of the resource.
> > > +\end{description}
> > 
> > I suppose that it'd be better not to have the above series of T_RESOURCE
> > controls at least until we reach a conclusion in the thread of
> > buffer-sharing device. If we end up concluding this type of controls is
> > the best way, we'll be able to revisit here.
> 
> Well.  For buffer management there are a bunch of options.
> 
>  (1) Simply stick the buffers (well, pointers to the buffer pages) into
>      the virtqueue.  This is the standard virtio way.
> 
>  (2) Create resources, then put the resource ids into the virtqueue.
>      virtio-gpu uses that model.  First, because virtio-gpu needs an id
>      to reference resources in the rendering command stream
>      (virtio-video doesn't need this).  Also because (some kinds of)
>      resources are around for a long time and the guest-physical ->
>      host-virtual mapping needs to be done only once that way (which
>      I think would be the case for virtio-video too because v4l2
>      re-uses buffers in robin-round fashion).  Drawback is this
>      assumes shared memory between host and guest (which is the case
>      in typical use cases but it is not mandated by the virtio spec).
> 
>  (3) Import external resources (from virtio-gpu for example).
>      Out of scope for now, will probably added as optional feature
>      later.
> 
> I guess long-term we want support either (1)+(3) or (2)+(3).
> 
> cheers,
>   Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi,

Just to start, let's consider this v4l2 control: 
V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
As I can see, this control is referenced as a mandatory one in the Chromium 
sources [1].

So could someone from the Chromium team please explain why it is mandatory? 
(YouTube?) In fact, almost no encoders implement this control. Do we need it 
in virtio-video?

[1] https://chromium.googlesource.com/chromium/src/media/+/refs/heads/master/
gpu/v4l2/v4l2_video_encode_accelerator.cc#1500

Regards,
Dmitry.

On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> +Changyeon Jo <changyeon@google.com> for his awareness
> 
> Thanks,
> - Enrico
> 
> 
> On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> 
> wrote:
> > Hello,
> > 
> > I'd like to invite everyone to share ideas regarding required encoder
> > features
> > in this separate sub-tree.
> > 
> > In general, encoder devices are more complex compared to decoders. So the
> > question I'd like to rise is in what way we define the minimal subset of
> > features to be implemented by the virtio-video.
> > 
> > We may look at the following to define the set of features:
> > 1. USB video, 2.3.6 Encoding Unit [1].
> > 2. Android 10 Compatibility Definition [2].
> > 
> > Would be nice to hear about any specific requirements from the Chromium
> > team as
> > well.
> > 
> > [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > [2]
> > https://source.android.com/compatibility/android-cdd#5_2_video_encoding
> > 
> > Thank you.
> > 
> > Best regards,
> > Dmitry.
> > 
> > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > >   Hi,
> > >   
> > > > 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.
> > > 
> > > Agree.
> > > 
> > > > 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.
> > > 
> > > Hmm, modern GPUs support both encoding and decoding ...
> > > 
> > > I don't think we should bake that restriction into the specification.
> > > It probably makes sense to use one virtqueue per function though, that
> > > will simplify dispatching in both host and guest.
> > > 
> > > > 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.
> > > 
> > > I think virtio-video should be able to work as stand-alone device,
> > > so we need some way to allocate buffers ...
> > > 
> > > Buffer sharing with other devices can be added later.
> > > 
> > > > > +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?
> > > 
> > > Grab the next free, update the spec accordingly, submit the one-line
> > > patch.
> > > 
> > > > > +\begin{lstlisting}
> > > > > +enum virtio_video_pixel_format {
> > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > +
> > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > +};
> > > > 
> > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > 
> > reinventing a
> > 
> > > > mapping from formats to integers.
> > > > Also, I suppose the word "pixel formats" means only raw (decoded)
> > 
> > formats.
> > 
> > > > But, it can be encoded format like H.264. So, I guess "image format"
> > > > or
> > > > "fourcc" is a better word choice.
> > > 
> > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > > 
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_video_function {
> > > > > +       struct virtio_video_desc desc;
> > > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > > > +       __le32 function_id;
> > > > > +       struct virtio_video_params in_params;
> > > > > +       struct virtio_video_params out_params;
> > > > > +       __le32 num_caps;
> > > > > +       __u8 padding[4];
> > > > > +       /* Followed by struct virtio_video_capability video_caps[];
> > 
> > */
> > 
> > > > > +};
> > > > > +\end{lstlisting}
> > > > 
> > > > If one device only has one functionality, virtio_video_function's
> > 
> > fields
> > 
> > > > will be no longer needed except in_params and out_params. So, we'd be
> > > > able to remove virtio_video_function and have in_params and out_params
> > 
> > in
> > 
> > > > virtio_video_capability instead.
> > > 
> > > Same goes for per-function virtqueues (used virtqueue implies function).
> > > 
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_video_resource_detach_backing {
> > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > +       __le32 resource_id;
> > > > > +       __u8 padding[4];
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +\begin{description}
> > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > +\end{description}
> > > > 
> > > > I suppose that it'd be better not to have the above series of
> > 
> > T_RESOURCE
> > 
> > > > controls at least until we reach a conclusion in the thread of
> > > > buffer-sharing device. If we end up concluding this type of controls
> > > > is
> > > > the best way, we'll be able to revisit here.
> > > 
> > > Well.  For buffer management there are a bunch of options.
> > > 
> > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > >  
> > >      the virtqueue.  This is the standard virtio way.
> > >  
> > >  (2) Create resources, then put the resource ids into the virtqueue.
> > >  
> > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > >      to reference resources in the rendering command stream
> > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > >      resources are around for a long time and the guest-physical ->
> > >      host-virtual mapping needs to be done only once that way (which
> > >      I think would be the case for virtio-video too because v4l2
> > >      re-uses buffers in robin-round fashion).  Drawback is this
> > >      assumes shared memory between host and guest (which is the case
> > >      in typical use cases but it is not mandated by the virtio spec).
> > >  
> > >  (3) Import external resources (from virtio-gpu for example).
> > >  
> > >      Out of scope for now, will probably added as optional feature
> > >      later.
> > > 
> > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > 
> > > cheers,
> > > 
> > >   Gerd
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
<dmitry.sepp@opensynergy.com> wrote:
>
> Hi,
>
> Just to start, let's consider this v4l2 control:
> V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> As I can see, this control is referenced as a mandatory one in the Chromium
> sources [1].
>
> So could someone from the Chromium team please explain why it is mandatory?
> (YouTube?) In fact, almost no encoders implement this control. Do we need it
> in virtio-video?

That control is used to encode videos in constant bitrate (CBR) mode,
which is critical for
real-time use case like video conferencing.

However, that Chromium source code just requires *host-side* drivers
to have the v4l2
control. Also, I guess it's rare that a guest app wants to use CQP
instead of CBR from our
experience.
So, I suppose we can omit this control in virtio spec for now by
assuming that guests
always use CBR mode.

Best regards,
Keiichi

>
> [1] https://chromium.googlesource.com/chromium/src/media/+/refs/heads/master/
> gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
>
> Regards,
> Dmitry.
>
> On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > +Changyeon Jo <changyeon@google.com> for his awareness
> >
> > Thanks,
> > - Enrico
> >
> >
> > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> >
> > wrote:
> > > Hello,
> > >
> > > I'd like to invite everyone to share ideas regarding required encoder
> > > features
> > > in this separate sub-tree.
> > >
> > > In general, encoder devices are more complex compared to decoders. So the
> > > question I'd like to rise is in what way we define the minimal subset of
> > > features to be implemented by the virtio-video.
> > >
> > > We may look at the following to define the set of features:
> > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > 2. Android 10 Compatibility Definition [2].
> > >
> > > Would be nice to hear about any specific requirements from the Chromium
> > > team as
> > > well.
> > >
> > > [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > [2]
> > > https://source.android.com/compatibility/android-cdd#5_2_video_encoding
> > >
> > > Thank you.
> > >
> > > Best regards,
> > > Dmitry.
> > >
> > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > >   Hi,
> > > >
> > > > > 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.
> > > >
> > > > Agree.
> > > >
> > > > > 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.
> > > >
> > > > Hmm, modern GPUs support both encoding and decoding ...
> > > >
> > > > I don't think we should bake that restriction into the specification.
> > > > It probably makes sense to use one virtqueue per function though, that
> > > > will simplify dispatching in both host and guest.
> > > >
> > > > > 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.
> > > >
> > > > I think virtio-video should be able to work as stand-alone device,
> > > > so we need some way to allocate buffers ...
> > > >
> > > > Buffer sharing with other devices can be added later.
> > > >
> > > > > > +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?
> > > >
> > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > patch.
> > > >
> > > > > > +\begin{lstlisting}
> > > > > > +enum virtio_video_pixel_format {
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > +
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > +};
> > > > >
> > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > >
> > > reinventing a
> > >
> > > > > mapping from formats to integers.
> > > > > Also, I suppose the word "pixel formats" means only raw (decoded)
> > >
> > > formats.
> > >
> > > > > But, it can be encoded format like H.264. So, I guess "image format"
> > > > > or
> > > > > "fourcc" is a better word choice.
> > > >
> > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> > > >
> > > > > > +\begin{lstlisting}
> > > > > > +struct virtio_video_function {
> > > > > > +       struct virtio_video_desc desc;
> > > > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > > > > +       __le32 function_id;
> > > > > > +       struct virtio_video_params in_params;
> > > > > > +       struct virtio_video_params out_params;
> > > > > > +       __le32 num_caps;
> > > > > > +       __u8 padding[4];
> > > > > > +       /* Followed by struct virtio_video_capability video_caps[];
> > >
> > > */
> > >
> > > > > > +};
> > > > > > +\end{lstlisting}
> > > > >
> > > > > If one device only has one functionality, virtio_video_function's
> > >
> > > fields
> > >
> > > > > will be no longer needed except in_params and out_params. So, we'd be
> > > > > able to remove virtio_video_function and have in_params and out_params
> > >
> > > in
> > >
> > > > > virtio_video_capability instead.
> > > >
> > > > Same goes for per-function virtqueues (used virtqueue implies function).
> > > >
> > > > > > +\begin{lstlisting}
> > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > +       __le32 resource_id;
> > > > > > +       __u8 padding[4];
> > > > > > +};
> > > > > > +\end{lstlisting}
> > > > > > +
> > > > > > +\begin{description}
> > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > +\end{description}
> > > > >
> > > > > I suppose that it'd be better not to have the above series of
> > >
> > > T_RESOURCE
> > >
> > > > > controls at least until we reach a conclusion in the thread of
> > > > > buffer-sharing device. If we end up concluding this type of controls
> > > > > is
> > > > > the best way, we'll be able to revisit here.
> > > >
> > > > Well.  For buffer management there are a bunch of options.
> > > >
> > > >  (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > > >
> > > >      the virtqueue.  This is the standard virtio way.
> > > >
> > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > >
> > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an id
> > > >      to reference resources in the rendering command stream
> > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > >      resources are around for a long time and the guest-physical ->
> > > >      host-virtual mapping needs to be done only once that way (which
> > > >      I think would be the case for virtio-video too because v4l2
> > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > >      assumes shared memory between host and guest (which is the case
> > > >      in typical use cases but it is not mandated by the virtio spec).
> > > >
> > > >  (3) Import external resources (from virtio-gpu for example).
> > > >
> > > >      Out of scope for now, will probably added as optional feature
> > > >      later.
> > > >
> > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > >
> > > > cheers,
> > > >
> > > >   Gerd
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi Keiichi,

Thank you for your comment.
What do you thing about selection/crop rectangles? Should this be defined as a 
must have? Or we just assume the device always sticks to the stream 
resolution.

Regards,
Dmitry.

On Donnerstag, 12. Dezember 2019 06:39:11 CET Keiichi Watanabe wrote:
> On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
> 
> <dmitry.sepp@opensynergy.com> wrote:
> > Hi,
> > 
> > Just to start, let's consider this v4l2 control:
> > V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> > As I can see, this control is referenced as a mandatory one in the
> > Chromium
> > sources [1].
> > 
> > So could someone from the Chromium team please explain why it is
> > mandatory?
> > (YouTube?) In fact, almost no encoders implement this control. Do we need
> > it in virtio-video?
> 
> That control is used to encode videos in constant bitrate (CBR) mode,
> which is critical for
> real-time use case like video conferencing.
> 
> However, that Chromium source code just requires *host-side* drivers
> to have the v4l2
> control. Also, I guess it's rare that a guest app wants to use CQP
> instead of CBR from our
> experience.
> So, I suppose we can omit this control in virtio spec for now by
> assuming that guests
> always use CBR mode.
> 
> Best regards,
> Keiichi
> 
> > [1]
> > https://chromium.googlesource.com/chromium/src/media/+/refs/heads/master/
> > gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
> > 
> > Regards,
> > Dmitry.
> > 
> > On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > > +Changyeon Jo <changyeon@google.com> for his awareness
> > > 
> > > Thanks,
> > > - Enrico
> > > 
> > > 
> > > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> > > 
> > > wrote:
> > > > Hello,
> > > > 
> > > > I'd like to invite everyone to share ideas regarding required encoder
> > > > features
> > > > in this separate sub-tree.
> > > > 
> > > > In general, encoder devices are more complex compared to decoders. So
> > > > the
> > > > question I'd like to rise is in what way we define the minimal subset
> > > > of
> > > > features to be implemented by the virtio-video.
> > > > 
> > > > We may look at the following to define the set of features:
> > > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > > 2. Android 10 Compatibility Definition [2].
> > > > 
> > > > Would be nice to hear about any specific requirements from the
> > > > Chromium
> > > > team as
> > > > well.
> > > > 
> > > > [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > > [2]
> > > > https://source.android.com/compatibility/android-cdd#5_2_video_encodin
> > > > g
> > > > 
> > > > Thank you.
> > > > 
> > > > Best regards,
> > > > Dmitry.
> > > > 
> > > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > >   
> > > > > > 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.
> > > > > 
> > > > > Agree.
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > Hmm, modern GPUs support both encoding and decoding ...
> > > > > 
> > > > > I don't think we should bake that restriction into the
> > > > > specification.
> > > > > It probably makes sense to use one virtqueue per function though,
> > > > > that
> > > > > will simplify dispatching in both host and guest.
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > I think virtio-video should be able to work as stand-alone device,
> > > > > so we need some way to allocate buffers ...
> > > > > 
> > > > > Buffer sharing with other devices can be added later.
> > > > > 
> > > > > > > +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?
> > > > > 
> > > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > > patch.
> > > > > 
> > > > > > > +\begin{lstlisting}
> > > > > > > +enum virtio_video_pixel_format {
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > > +
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > > +};
> > > > > > 
> > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > > > 
> > > > reinventing a
> > > > 
> > > > > > mapping from formats to integers.
> > > > > > Also, I suppose the word "pixel formats" means only raw (decoded)
> > > > 
> > > > formats.
> > > > 
> > > > > > But, it can be encoded format like H.264. So, I guess "image
> > > > > > format"
> > > > > > or
> > > > > > "fourcc" is a better word choice.
> > > > > 
> > > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.)
> > > > > enums?
> > > > > 
> > > > > > > +\begin{lstlisting}
> > > > > > > +struct virtio_video_function {
> > > > > > > +       struct virtio_video_desc desc;
> > > > > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_*
> > > > > > > types */
> > > > > > > +       __le32 function_id;
> > > > > > > +       struct virtio_video_params in_params;
> > > > > > > +       struct virtio_video_params out_params;
> > > > > > > +       __le32 num_caps;
> > > > > > > +       __u8 padding[4];
> > > > > > > +       /* Followed by struct virtio_video_capability
> > > > > > > video_caps[];
> > > > 
> > > > */
> > > > 
> > > > > > > +};
> > > > > > > +\end{lstlisting}
> > > > > > 
> > > > > > If one device only has one functionality, virtio_video_function's
> > > > 
> > > > fields
> > > > 
> > > > > > will be no longer needed except in_params and out_params. So, we'd
> > > > > > be
> > > > > > able to remove virtio_video_function and have in_params and
> > > > > > out_params
> > > > 
> > > > in
> > > > 
> > > > > > virtio_video_capability instead.
> > > > > 
> > > > > Same goes for per-function virtqueues (used virtqueue implies
> > > > > function).
> > > > > 
> > > > > > > +\begin{lstlisting}
> > > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > > +       __le32 resource_id;
> > > > > > > +       __u8 padding[4];
> > > > > > > +};
> > > > > > > +\end{lstlisting}
> > > > > > > +
> > > > > > > +\begin{description}
> > > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > > +\end{description}
> > > > > > 
> > > > > > I suppose that it'd be better not to have the above series of
> > > > 
> > > > T_RESOURCE
> > > > 
> > > > > > controls at least until we reach a conclusion in the thread of
> > > > > > buffer-sharing device. If we end up concluding this type of
> > > > > > controls
> > > > > > is
> > > > > > the best way, we'll be able to revisit here.
> > > > > 
> > > > > Well.  For buffer management there are a bunch of options.
> > > > > 
> > > > >  (1) Simply stick the buffers (well, pointers to the buffer pages)
> > > > >  into
> > > > >  
> > > > >      the virtqueue.  This is the standard virtio way.
> > > > >  
> > > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > > >  
> > > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an
> > > > >      id
> > > > >      to reference resources in the rendering command stream
> > > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > > >      resources are around for a long time and the guest-physical ->
> > > > >      host-virtual mapping needs to be done only once that way (which
> > > > >      I think would be the case for virtio-video too because v4l2
> > > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > > >      assumes shared memory between host and guest (which is the case
> > > > >      in typical use cases but it is not mandated by the virtio
> > > > >      spec).
> > > > >  
> > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > >  
> > > > >      Out of scope for now, will probably added as optional feature
> > > > >      later.
> > > > > 
> > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > 
> > > > > cheers,
> > > > > 
> > > > >   Gerd
> > > > 
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

On Thu, Dec 12, 2019 at 7:34 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> Thank you for your comment.
> What do you thing about selection/crop rectangles? Should this be defined as a
> must have? Or we just assume the device always sticks to the stream
> resolution.
>

For decoding, the device needs to tell the driver crop rectangle when
a resolution change happens, as virtio-vdec has
virtio_vdec_host_req_stream_crop.
So, we should add a new field in virtio_video_param for it.

For what it's worth, we have C-headers that exposes Chrome's video
codec functionalities. So, our virtio-video device will do
decoding/encoding by calling these functions.
Decoder: https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h
Encoder: https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h

BTW, I'm preparing the next version of virtio-video spec by addressing
points we discussed in this thread. I'll send it here next week,
hopefully.

Best,
Keiichi



> Regards,
> Dmitry.
>
> On Donnerstag, 12. Dezember 2019 06:39:11 CET Keiichi Watanabe wrote:
> > On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
> >
> > <dmitry.sepp@opensynergy.com> wrote:
> > > Hi,
> > >
> > > Just to start, let's consider this v4l2 control:
> > > V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> > > As I can see, this control is referenced as a mandatory one in the
> > > Chromium
> > > sources [1].
> > >
> > > So could someone from the Chromium team please explain why it is
> > > mandatory?
> > > (YouTube?) In fact, almost no encoders implement this control. Do we need
> > > it in virtio-video?
> >
> > That control is used to encode videos in constant bitrate (CBR) mode,
> > which is critical for
> > real-time use case like video conferencing.
> >
> > However, that Chromium source code just requires *host-side* drivers
> > to have the v4l2
> > control. Also, I guess it's rare that a guest app wants to use CQP
> > instead of CBR from our
> > experience.
> > So, I suppose we can omit this control in virtio spec for now by
> > assuming that guests
> > always use CBR mode.
> >
> > Best regards,
> > Keiichi
> >
> > > [1]
> > > https://chromium.googlesource.com/chromium/src/media/+/refs/heads/master/
> > > gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
> > >
> > > Regards,
> > > Dmitry.
> > >
> > > On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > > > +Changyeon Jo <changyeon@google.com> for his awareness
> > > >
> > > > Thanks,
> > > > - Enrico
> > > >
> > > >
> > > > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> > > >
> > > > wrote:
> > > > > Hello,
> > > > >
> > > > > I'd like to invite everyone to share ideas regarding required encoder
> > > > > features
> > > > > in this separate sub-tree.
> > > > >
> > > > > In general, encoder devices are more complex compared to decoders. So
> > > > > the
> > > > > question I'd like to rise is in what way we define the minimal subset
> > > > > of
> > > > > features to be implemented by the virtio-video.
> > > > >
> > > > > We may look at the following to define the set of features:
> > > > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > > > 2. Android 10 Compatibility Definition [2].
> > > > >
> > > > > Would be nice to hear about any specific requirements from the
> > > > > Chromium
> > > > > team as
> > > > > well.
> > > > >
> > > > > [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > > > [2]
> > > > > https://source.android.com/compatibility/android-cdd#5_2_video_encodin
> > > > > g
> > > > >
> > > > > Thank you.
> > > > >
> > > > > Best regards,
> > > > > Dmitry.
> > > > >
> > > > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > > > >   Hi,
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Agree.
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Hmm, modern GPUs support both encoding and decoding ...
> > > > > >
> > > > > > I don't think we should bake that restriction into the
> > > > > > specification.
> > > > > > It probably makes sense to use one virtqueue per function though,
> > > > > > that
> > > > > > will simplify dispatching in both host and guest.
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > I think virtio-video should be able to work as stand-alone device,
> > > > > > so we need some way to allocate buffers ...
> > > > > >
> > > > > > Buffer sharing with other devices can be added later.
> > > > > >
> > > > > > > > +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?
> > > > > >
> > > > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > > > patch.
> > > > > >
> > > > > > > > +\begin{lstlisting}
> > > > > > > > +enum virtio_video_pixel_format {
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > > > +
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > > > +};
> > > > > > >
> > > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > > > >
> > > > > reinventing a
> > > > >
> > > > > > > mapping from formats to integers.
> > > > > > > Also, I suppose the word "pixel formats" means only raw (decoded)
> > > > >
> > > > > formats.
> > > > >
> > > > > > > But, it can be encoded format like H.264. So, I guess "image
> > > > > > > format"
> > > > > > > or
> > > > > > > "fourcc" is a better word choice.
> > > > > >
> > > > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.)
> > > > > > enums?
> > > > > >
> > > > > > > > +\begin{lstlisting}
> > > > > > > > +struct virtio_video_function {
> > > > > > > > +       struct virtio_video_desc desc;
> > > > > > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_*
> > > > > > > > types */
> > > > > > > > +       __le32 function_id;
> > > > > > > > +       struct virtio_video_params in_params;
> > > > > > > > +       struct virtio_video_params out_params;
> > > > > > > > +       __le32 num_caps;
> > > > > > > > +       __u8 padding[4];
> > > > > > > > +       /* Followed by struct virtio_video_capability
> > > > > > > > video_caps[];
> > > > >
> > > > > */
> > > > >
> > > > > > > > +};
> > > > > > > > +\end{lstlisting}
> > > > > > >
> > > > > > > If one device only has one functionality, virtio_video_function's
> > > > >
> > > > > fields
> > > > >
> > > > > > > will be no longer needed except in_params and out_params. So, we'd
> > > > > > > be
> > > > > > > able to remove virtio_video_function and have in_params and
> > > > > > > out_params
> > > > >
> > > > > in
> > > > >
> > > > > > > virtio_video_capability instead.
> > > > > >
> > > > > > Same goes for per-function virtqueues (used virtqueue implies
> > > > > > function).
> > > > > >
> > > > > > > > +\begin{lstlisting}
> > > > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > > > +       __le32 resource_id;
> > > > > > > > +       __u8 padding[4];
> > > > > > > > +};
> > > > > > > > +\end{lstlisting}
> > > > > > > > +
> > > > > > > > +\begin{description}
> > > > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > > > +\end{description}
> > > > > > >
> > > > > > > I suppose that it'd be better not to have the above series of
> > > > >
> > > > > T_RESOURCE
> > > > >
> > > > > > > controls at least until we reach a conclusion in the thread of
> > > > > > > buffer-sharing device. If we end up concluding this type of
> > > > > > > controls
> > > > > > > is
> > > > > > > the best way, we'll be able to revisit here.
> > > > > >
> > > > > > Well.  For buffer management there are a bunch of options.
> > > > > >
> > > > > >  (1) Simply stick the buffers (well, pointers to the buffer pages)
> > > > > >  into
> > > > > >
> > > > > >      the virtqueue.  This is the standard virtio way.
> > > > > >
> > > > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > > > >
> > > > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an
> > > > > >      id
> > > > > >      to reference resources in the rendering command stream
> > > > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > > > >      resources are around for a long time and the guest-physical ->
> > > > > >      host-virtual mapping needs to be done only once that way (which
> > > > > >      I think would be the case for virtio-video too because v4l2
> > > > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > > > >      assumes shared memory between host and guest (which is the case
> > > > > >      in typical use cases but it is not mandated by the virtio
> > > > > >      spec).
> > > > > >
> > > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > > >
> > > > > >      Out of scope for now, will probably added as optional feature
> > > > > >      later.
> > > > > >
> > > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > >
> > > > > > cheers,
> > > > > >
> > > > > >   Gerd
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Christophe de Dinechin]

+spice-devel for awareness

Context: there is a lot of work there on video streaming for SPICE, mostly
done ATM through proprietary APIs.

> On 9 Dec 2019, at 15:19, Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> 
> Hello,
> 
> I'd like to invite everyone to share ideas regarding required encoder features 
> in this separate sub-tree.
> 
> In general, encoder devices are more complex compared to decoders. So the 
> question I'd like to rise is in what way we define the minimal subset of 
> features to be implemented by the virtio-video.
> 
> We may look at the following to define the set of features:
> 1. USB video, 2.3.6 Encoding Unit [1].
> 2. Android 10 Compatibility Definition [2].
> 
> Would be nice to hear about any specific requirements from the Chromium team as 
> well.
> 
> [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> [2] https://source.android.com/compatibility/android-cdd#5_2_video_encoding
> 
> Thank you.
> 
> Best regards,
> Dmitry.
> 
> On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
>>  Hi,
>> 
>>> 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.
>> Agree.
>> 
>>> 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.
>> 
>> Hmm, modern GPUs support both encoding and decoding ...
>> 
>> I don't think we should bake that restriction into the specification.
>> It probably makes sense to use one virtqueue per function though, that
>> will simplify dispatching in both host and guest.
>> 
>>> 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.
>> 
>> I think virtio-video should be able to work as stand-alone device,
>> so we need some way to allocate buffers ...
>> 
>> Buffer sharing with other devices can be added later.
>> 
>>>> +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?
>> 
>> Grab the next free, update the spec accordingly, submit the one-line
>> patch.
>> 
>>>> +\begin{lstlisting}
>>>> +enum virtio_video_pixel_format {
>>>> +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
>>>> +
>>>> +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
>>>> +       VIRTIO_VIDEO_PIX_FMT_NV12,
>>>> +       VIRTIO_VIDEO_PIX_FMT_NV21,
>>>> +       VIRTIO_VIDEO_PIX_FMT_I420,
>>>> +       VIRTIO_VIDEO_PIX_FMT_I422,
>>>> +       VIRTIO_VIDEO_PIX_FMT_XBGR,
>>>> +};
>>> 
>>> I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
>>> mapping from formats to integers.
>>> Also, I suppose the word "pixel formats" means only raw (decoded) formats.
>>> But, it can be encoded format like H.264. So, I guess "image format" or
>>> "fourcc" is a better word choice.
>> 
>> Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
>> 
>>>> +\begin{lstlisting}
>>>> +struct virtio_video_function {
>>>> +       struct virtio_video_desc desc;
>>>> +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
>>>> +       __le32 function_id;
>>>> +       struct virtio_video_params in_params;
>>>> +       struct virtio_video_params out_params;
>>>> +       __le32 num_caps;
>>>> +       __u8 padding[4];
>>>> +       /* Followed by struct virtio_video_capability video_caps[]; */
>>>> +};
>>>> +\end{lstlisting}
>>> 
>>> If one device only has one functionality, virtio_video_function's fields
>>> will be no longer needed except in_params and out_params. So, we'd be
>>> able to remove virtio_video_function and have in_params and out_params in
>>> virtio_video_capability instead.
>> 
>> Same goes for per-function virtqueues (used virtqueue implies function).
>> 
>>>> +\begin{lstlisting}
>>>> +struct virtio_video_resource_detach_backing {
>>>> +       struct virtio_video_ctrl_hdr hdr;
>>>> +       __le32 resource_id;
>>>> +       __u8 padding[4];
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\begin{description}
>>>> +\item[\field{resource_id}] internal id of the resource.
>>>> +\end{description}
>>> 
>>> I suppose that it'd be better not to have the above series of T_RESOURCE
>>> controls at least until we reach a conclusion in the thread of
>>> buffer-sharing device. If we end up concluding this type of controls is
>>> the best way, we'll be able to revisit here.
>> 
>> Well.  For buffer management there are a bunch of options.
>> 
>> (1) Simply stick the buffers (well, pointers to the buffer pages) into
>>     the virtqueue.  This is the standard virtio way.
>> 
>> (2) Create resources, then put the resource ids into the virtqueue.
>>     virtio-gpu uses that model.  First, because virtio-gpu needs an id
>>     to reference resources in the rendering command stream
>>     (virtio-video doesn't need this).  Also because (some kinds of)
>>     resources are around for a long time and the guest-physical ->
>>     host-virtual mapping needs to be done only once that way (which
>>     I think would be the case for virtio-video too because v4l2
>>     re-uses buffers in robin-round fashion).  Drawback is this
>>     assumes shared memory between host and guest (which is the case
>>     in typical use cases but it is not mandated by the virtio spec).
>> 
>> (3) Import external resources (from virtio-gpu for example).
>>     Out of scope for now, will probably added as optional feature
>>     later.
>> 
>> I guess long-term we want support either (1)+(3) or (2)+(3).
>> 
>> cheers,
>>  Gerd
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

On Fri, Dec 13, 2019 at 11:20 PM Keiichi Watanabe <keiichiw@chromium.org> wrote:
>
> On Thu, Dec 12, 2019 at 7:34 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> >
> > Hi Keiichi,
> >
> > Thank you for your comment.
> > What do you thing about selection/crop rectangles? Should this be defined as a
> > must have? Or we just assume the device always sticks to the stream
> > resolution.
> >
>
> For decoding, the device needs to tell the driver crop rectangle when
> a resolution change happens, as virtio-vdec has
> virtio_vdec_host_req_stream_crop.
> So, we should add a new field in virtio_video_param for it.
>
> For what it's worth, we have C-headers that exposes Chrome's video
> codec functionalities. So, our virtio-video device will do
> decoding/encoding by calling these functions.
> Decoder: https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h
> Encoder: https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h
>

Oops, the links were not accesible...
The correct links are:
Decoder: https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h
Encoder: https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h

Best,
Keiichi

> BTW, I'm preparing the next version of virtio-video spec by addressing
> points we discussed in this thread. I'll send it here next week,
> hopefully.
>
> Best,
> Keiichi
>
>
>
> > Regards,
> > Dmitry.
> >
> > On Donnerstag, 12. Dezember 2019 06:39:11 CET Keiichi Watanabe wrote:
> > > On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
> > >
> > > <dmitry.sepp@opensynergy.com> wrote:
> > > > Hi,
> > > >
> > > > Just to start, let's consider this v4l2 control:
> > > > V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> > > > As I can see, this control is referenced as a mandatory one in the
> > > > Chromium
> > > > sources [1].
> > > >
> > > > So could someone from the Chromium team please explain why it is
> > > > mandatory?
> > > > (YouTube?) In fact, almost no encoders implement this control. Do we need
> > > > it in virtio-video?
> > >
> > > That control is used to encode videos in constant bitrate (CBR) mode,
> > > which is critical for
> > > real-time use case like video conferencing.
> > >
> > > However, that Chromium source code just requires *host-side* drivers
> > > to have the v4l2
> > > control. Also, I guess it's rare that a guest app wants to use CQP
> > > instead of CBR from our
> > > experience.
> > > So, I suppose we can omit this control in virtio spec for now by
> > > assuming that guests
> > > always use CBR mode.
> > >
> > > Best regards,
> > > Keiichi
> > >
> > > > [1]
> > > > https://chromium.googlesource.com/chromium/src/media/+/refs/heads/master/
> > > > gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
> > > >
> > > > Regards,
> > > > Dmitry.
> > > >
> > > > On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > > > > +Changyeon Jo <changyeon@google.com> for his awareness
> > > > >
> > > > > Thanks,
> > > > > - Enrico
> > > > >
> > > > >
> > > > > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> > > > >
> > > > > wrote:
> > > > > > Hello,
> > > > > >
> > > > > > I'd like to invite everyone to share ideas regarding required encoder
> > > > > > features
> > > > > > in this separate sub-tree.
> > > > > >
> > > > > > In general, encoder devices are more complex compared to decoders. So
> > > > > > the
> > > > > > question I'd like to rise is in what way we define the minimal subset
> > > > > > of
> > > > > > features to be implemented by the virtio-video.
> > > > > >
> > > > > > We may look at the following to define the set of features:
> > > > > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > > > > 2. Android 10 Compatibility Definition [2].
> > > > > >
> > > > > > Would be nice to hear about any specific requirements from the
> > > > > > Chromium
> > > > > > team as
> > > > > > well.
> > > > > >
> > > > > > [1] https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > > > > [2]
> > > > > > https://source.android.com/compatibility/android-cdd#5_2_video_encodin
> > > > > > g
> > > > > >
> > > > > > Thank you.
> > > > > >
> > > > > > Best regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > > > > >   Hi,
> > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Agree.
> > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Hmm, modern GPUs support both encoding and decoding ...
> > > > > > >
> > > > > > > I don't think we should bake that restriction into the
> > > > > > > specification.
> > > > > > > It probably makes sense to use one virtqueue per function though,
> > > > > > > that
> > > > > > > will simplify dispatching in both host and guest.
> > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > I think virtio-video should be able to work as stand-alone device,
> > > > > > > so we need some way to allocate buffers ...
> > > > > > >
> > > > > > > Buffer sharing with other devices can be added later.
> > > > > > >
> > > > > > > > > +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?
> > > > > > >
> > > > > > > Grab the next free, update the spec accordingly, submit the one-line
> > > > > > > patch.
> > > > > > >
> > > > > > > > > +\begin{lstlisting}
> > > > > > > > > +enum virtio_video_pixel_format {
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > > > > +
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > > > > >
> > > > > > reinventing a
> > > > > >
> > > > > > > > mapping from formats to integers.
> > > > > > > > Also, I suppose the word "pixel formats" means only raw (decoded)
> > > > > >
> > > > > > formats.
> > > > > >
> > > > > > > > But, it can be encoded format like H.264. So, I guess "image
> > > > > > > > format"
> > > > > > > > or
> > > > > > > > "fourcc" is a better word choice.
> > > > > > >
> > > > > > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.)
> > > > > > > enums?
> > > > > > >
> > > > > > > > > +\begin{lstlisting}
> > > > > > > > > +struct virtio_video_function {
> > > > > > > > > +       struct virtio_video_desc desc;
> > > > > > > > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_*
> > > > > > > > > types */
> > > > > > > > > +       __le32 function_id;
> > > > > > > > > +       struct virtio_video_params in_params;
> > > > > > > > > +       struct virtio_video_params out_params;
> > > > > > > > > +       __le32 num_caps;
> > > > > > > > > +       __u8 padding[4];
> > > > > > > > > +       /* Followed by struct virtio_video_capability
> > > > > > > > > video_caps[];
> > > > > >
> > > > > > */
> > > > > >
> > > > > > > > > +};
> > > > > > > > > +\end{lstlisting}
> > > > > > > >
> > > > > > > > If one device only has one functionality, virtio_video_function's
> > > > > >
> > > > > > fields
> > > > > >
> > > > > > > > will be no longer needed except in_params and out_params. So, we'd
> > > > > > > > be
> > > > > > > > able to remove virtio_video_function and have in_params and
> > > > > > > > out_params
> > > > > >
> > > > > > in
> > > > > >
> > > > > > > > virtio_video_capability instead.
> > > > > > >
> > > > > > > Same goes for per-function virtqueues (used virtqueue implies
> > > > > > > function).
> > > > > > >
> > > > > > > > > +\begin{lstlisting}
> > > > > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > > > > +       __le32 resource_id;
> > > > > > > > > +       __u8 padding[4];
> > > > > > > > > +};
> > > > > > > > > +\end{lstlisting}
> > > > > > > > > +
> > > > > > > > > +\begin{description}
> > > > > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > > > > +\end{description}
> > > > > > > >
> > > > > > > > I suppose that it'd be better not to have the above series of
> > > > > >
> > > > > > T_RESOURCE
> > > > > >
> > > > > > > > controls at least until we reach a conclusion in the thread of
> > > > > > > > buffer-sharing device. If we end up concluding this type of
> > > > > > > > controls
> > > > > > > > is
> > > > > > > > the best way, we'll be able to revisit here.
> > > > > > >
> > > > > > > Well.  For buffer management there are a bunch of options.
> > > > > > >
> > > > > > >  (1) Simply stick the buffers (well, pointers to the buffer pages)
> > > > > > >  into
> > > > > > >
> > > > > > >      the virtqueue.  This is the standard virtio way.
> > > > > > >
> > > > > > >  (2) Create resources, then put the resource ids into the virtqueue.
> > > > > > >
> > > > > > >      virtio-gpu uses that model.  First, because virtio-gpu needs an
> > > > > > >      id
> > > > > > >      to reference resources in the rendering command stream
> > > > > > >      (virtio-video doesn't need this).  Also because (some kinds of)
> > > > > > >      resources are around for a long time and the guest-physical ->
> > > > > > >      host-virtual mapping needs to be done only once that way (which
> > > > > > >      I think would be the case for virtio-video too because v4l2
> > > > > > >      re-uses buffers in robin-round fashion).  Drawback is this
> > > > > > >      assumes shared memory between host and guest (which is the case
> > > > > > >      in typical use cases but it is not mandated by the virtio
> > > > > > >      spec).
> > > > > > >
> > > > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > > > >
> > > > > > >      Out of scope for now, will probably added as optional feature
> > > > > > >      later.
> > > > > > >
> > > > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > > >
> > > > > > > cheers,
> > > > > > >
> > > > > > >   Gerd
> > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
> >

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Tomasz Figa]

On Wed, Dec 4, 2019 at 6:16 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > 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.
>
> Agree.
>
> > 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.
>
> Hmm, modern GPUs support both encoding and decoding ...
>

Many SoC architectures have completely separate IP blocks for encoding
and decoding. Similarly, in GPUs those are usually completely separate
parts of the pipeline.

> I don't think we should bake that restriction into the specification.
> It probably makes sense to use one virtqueue per function though, that
> will simplify dispatching in both host and guest.
>

Wouldn't it make the handling easier if we had one virtio device per function?

[snip]

> > > +\begin{lstlisting}
> > > +enum virtio_video_pixel_format {
> > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > +
> > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > +};
> >
> > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > mapping from formats to integers.
> > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > But, it can be encoded format like H.264. So, I guess "image format" or
> > "fourcc" is a better word choice.
>
> Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
>

I'd specifically avoid FourCC here, as it's very loosely defined and
could introduce confusion. A separate enum for "image formats",
including both  sounds good to me.

> > > +\begin{lstlisting}
> > > +struct virtio_video_function {
> > > +       struct virtio_video_desc desc;
> > > +       __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > +       __le32 function_id;
> > > +       struct virtio_video_params in_params;
> > > +       struct virtio_video_params out_params;
> > > +       __le32 num_caps;
> > > +       __u8 padding[4];
> > > +       /* Followed by struct virtio_video_capability video_caps[]; */
> > > +};
> > > +\end{lstlisting}
> >
> > If one device only has one functionality, virtio_video_function's fields will be
> > no longer needed except in_params and out_params. So, we'd be able to remove
> > virtio_video_function and have in_params and out_params in
> > virtio_video_capability instead.
>
> Same goes for per-function virtqueues (used virtqueue implies function).
>
> > > +\begin{lstlisting}
> > > +struct virtio_video_resource_detach_backing {
> > > +       struct virtio_video_ctrl_hdr hdr;
> > > +       __le32 resource_id;
> > > +       __u8 padding[4];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +\item[\field{resource_id}] internal id of the resource.
> > > +\end{description}
> >
> > I suppose that it'd be better not to have the above series of T_RESOURCE
> > controls at least until we reach a conclusion in the thread of buffer-sharing
> > device. If we end up concluding this type of controls is the best way, we'll be
> > able to revisit here.
>
> Well.  For buffer management there are a bunch of options.
>
>  (1) Simply stick the buffers (well, pointers to the buffer pages) into
>      the virtqueue.  This is the standard virtio way.
>
>  (2) Create resources, then put the resource ids into the virtqueue.
>      virtio-gpu uses that model.  First, because virtio-gpu needs an id
>      to reference resources in the rendering command stream
>      (virtio-video doesn't need this).  Also because (some kinds of)
>      resources are around for a long time and the guest-physical ->
>      host-virtual mapping needs to be done only once that way (which
>      I think would be the case for virtio-video too because v4l2
>      re-uses buffers in robin-round fashion).  Drawback is this
>      assumes shared memory between host and guest (which is the case
>      in typical use cases but it is not mandated by the virtio spec).
>
>  (3) Import external resources (from virtio-gpu for example).
>      Out of scope for now, will probably added as optional feature
>      later.
>
> I guess long-term we want support either (1)+(3) or (2)+(3).

What this protocol has been proposing is a twist of (1), where there
is a "resource create" call that generates a local "resource ID" for
the given list of guest pages. I think that's a sane approach, given
that the number of pages to describe a buffer worth of 4K video would
be more than 3000. We don't want to send so long lists of pages every
frame, especially since we normally recycle the buffers.

Best regards,
Tomasz

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> > Hmm, modern GPUs support both encoding and decoding ...
> 
> Many SoC architectures have completely separate IP blocks for encoding
> and decoding. Similarly, in GPUs those are usually completely separate
> parts of the pipeline.

In the OS there is one driver per GPU handling both ...

> > I don't think we should bake that restriction into the specification.
> > It probably makes sense to use one virtqueue per function though, that
> > will simplify dispatching in both host and guest.
> >
> 
> Wouldn't it make the handling easier if we had one virtio device per function?

I don't think there is much of a difference between one device per
function and one virtqueue per function.  You'll probably have a single
driver for both anyway, to share common code for buffer management etc.

> > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> 
> I'd specifically avoid FourCC here, as it's very loosely defined and
> could introduce confusion.

I don't think using fourcc is a problem, and given that both drm and
v4l2 use fourcc already this would be a good choice I think.

But the definition should be more specific than just "fourcc".  Best
would be to explicitly list and define each format supported by the
spec.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Tomasz Figa]

On Mon, Dec 16, 2019 at 7:32 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Hmm, modern GPUs support both encoding and decoding ...
> >
> > Many SoC architectures have completely separate IP blocks for encoding
> > and decoding. Similarly, in GPUs those are usually completely separate
> > parts of the pipeline.
>
> In the OS there is one driver per GPU handling both ...
>

That's usually only the case for the desktop PC-style GPUs. That said,
it probably doesn't matter from the protocol point of view how it's
handled in the host. At least it is definitely safe to assume that the
host provides functionality for independently decoding and encoding
things from different processes, which is enough to implement virtio
decoder and encoder as separate devices.

> > > I don't think we should bake that restriction into the specification.
> > > It probably makes sense to use one virtqueue per function though, that
> > > will simplify dispatching in both host and guest.
> > >
> >
> > Wouldn't it make the handling easier if we had one virtio device per function?
>
> I don't think there is much of a difference between one device per
> function and one virtqueue per function.  You'll probably have a single
> driver for both anyway, to share common code for buffer management etc.
>

The semantics of the encoder and decoder on the driver side, at least
on Linux where V4L2 is used, are different enough for much of the code
to be separated.

That said, even with separate devices, the same driver can be
instantiated multiple times, which is a common case in Linux, handled
by the driver core. That basically gives us the ability to have as
many instances of whatever function we want for free, without the need
to explicitly handle multiple functions in one device in the driver.
So that clearly equals simpler driver code.

On the host side, the encode and decode APIs are different as well, so
having separate implementation decoder and encoder, possibly just
sharing some helper code, would make much more sense.

> > > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> >
> > I'd specifically avoid FourCC here, as it's very loosely defined and
> > could introduce confusion.
>
> I don't think using fourcc is a problem, and given that both drm and
> v4l2 use fourcc already this would be a good choice I think.

Both DRM and V4L2 use two mutually incompatible sets of FourCCs, so
I'm not sure how it could be a good choice. At least unless we decide
to pick a specific set of FourCC. It doesn't help that Windows/DirectX
has its own set of FourCCs that's again slightly different than the
two mentioned before.

>
> But the definition should be more specific than just "fourcc".  Best
> would be to explicitly list and define each format supported by the
> spec.

Why not be consistent with virtio-gpu and just define new formats as
we add support for them as sequential integers?

Best regards,
Tomasz

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

  Hi,

> On the host side, the encode and decode APIs are different as well, so
> having separate implementation decoder and encoder, possibly just
> sharing some helper code, would make much more sense.

When going down that route I'd suggest to use two device ids (even when
specifying both variants in one spec section and one header file due to
the overlaps) instead of feature flags.

> > I don't think using fourcc is a problem, and given that both drm and
> > v4l2 use fourcc already this would be a good choice I think.
> 
> Both DRM and V4L2 use two mutually incompatible sets of FourCCs, so
> I'm not sure how it could be a good choice. At least unless we decide
> to pick a specific set of FourCC. It doesn't help that Windows/DirectX
> has its own set of FourCCs that's again slightly different than the
> two mentioned before.

Ouch, wasn't aware of that.  That makes reusing fourcc codes much less
useful.

> > But the definition should be more specific than just "fourcc".  Best
> > would be to explicitly list and define each format supported by the
> > spec.
> 
> Why not be consistent with virtio-gpu and just define new formats as
> we add support for them as sequential integers?

Yes, lets do that.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

Hi,

Thanks Tomasz and Gerd for the suggestions and information.

On Tue, Dec 17, 2019 at 10:39 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > On the host side, the encode and decode APIs are different as well, so
> > having separate implementation decoder and encoder, possibly just
> > sharing some helper code, would make much more sense.
>
> When going down that route I'd suggest to use two device ids (even when
> specifying both variants in one spec section and one header file due to
> the overlaps) instead of feature flags.

Sounds good. It makes sense to use different device IDs for different devices.

>
> > > I don't think using fourcc is a problem, and given that both drm and
> > > v4l2 use fourcc already this would be a good choice I think.
> >
> > Both DRM and V4L2 use two mutually incompatible sets of FourCCs, so
> > I'm not sure how it could be a good choice. At least unless we decide
> > to pick a specific set of FourCC. It doesn't help that Windows/DirectX
> > has its own set of FourCCs that's again slightly different than the
> > two mentioned before.
>
> Ouch, wasn't aware of that.  That makes reusing fourcc codes much less
> useful.
>
> > > But the definition should be more specific than just "fourcc".  Best
> > > would be to explicitly list and define each format supported by the
> > > spec.
> >
> > Why not be consistent with virtio-gpu and just define new formats as
> > we add support for them as sequential integers?
>
> Yes, lets do that.
>

It makes sense. I seems to have overestimated FourCC.

Best,
Keiichi

> cheers,
>   Gerd
>

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi,

On Dienstag, 17. Dezember 2019 15:09:16 CET Keiichi Watanabe wrote:
> Hi,
> 
> Thanks Tomasz and Gerd for the suggestions and information.
> 
> On Tue, Dec 17, 2019 at 10:39 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   Hi,
> >   
> > > On the host side, the encode and decode APIs are different as well, so
> > > having separate implementation decoder and encoder, possibly just
> > > sharing some helper code, would make much more sense.
> > 
> > When going down that route I'd suggest to use two device ids (even when
> > specifying both variants in one spec section and one header file due to
> > the overlaps) instead of feature flags.
> 
> Sounds good. It makes sense to use different device IDs for different
> devices.
Does this mean one driver handles both? Or we have two separate drivers?

> > > > I don't think using fourcc is a problem, and given that both drm and
> > > > v4l2 use fourcc already this would be a good choice I think.
> > > 
> > > Both DRM and V4L2 use two mutually incompatible sets of FourCCs, so
> > > I'm not sure how it could be a good choice. At least unless we decide
> > > to pick a specific set of FourCC. It doesn't help that Windows/DirectX
> > > has its own set of FourCCs that's again slightly different than the
> > > two mentioned before.
> > 
> > Ouch, wasn't aware of that.  That makes reusing fourcc codes much less
> > useful.
> > 
> > > > But the definition should be more specific than just "fourcc".  Best
> > > > would be to explicitly list and define each format supported by the
> > > > spec.
> > > 
> > > Why not be consistent with virtio-gpu and just define new formats as
> > > we add support for them as sequential integers?
> > 
> > Yes, lets do that.
> 
> It makes sense. I seems to have overestimated FourCC.
This is what was actually done in the driver code already (it is a bit ahead 
of the spec, but I guess no one has looked at it so far).

Regards,
Dmitry.

> 
> Best,
> Keiichi
> 
> > cheers,
> > 
> >   Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Gerd Hoffmann]

On Tue, Dec 17, 2019 at 05:13:59PM +0100, Dmitry Sepp wrote:
> Hi,
> 
> On Dienstag, 17. Dezember 2019 15:09:16 CET Keiichi Watanabe wrote:
> > Hi,
> > 
> > Thanks Tomasz and Gerd for the suggestions and information.
> > 
> > On Tue, Dec 17, 2019 at 10:39 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >   Hi,
> > >   
> > > > On the host side, the encode and decode APIs are different as well, so
> > > > having separate implementation decoder and encoder, possibly just
> > > > sharing some helper code, would make much more sense.
> > > 
> > > When going down that route I'd suggest to use two device ids (even when
> > > specifying both variants in one spec section and one header file due to
> > > the overlaps) instead of feature flags.
> > 
> > Sounds good. It makes sense to use different device IDs for different
> > devices.
> Does this mean one driver handles both? Or we have two separate drivers?

That is the driver writers choice.  You can have a single kernel module
registering as driver for both IDs.  Or you can have two kernel modules,
each registering for one of the IDs, possibly with a third library
module with helper code for common bits like buffer management.

cheers,
  Gerd

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Dmitry Sepp]

Hi Keiichi,

Could you please explain in a more detailed way what exactly CROP is used for? 
Is it equivalent to G/S_CROP or G/S_SELECTION in v4l2? Do you only need to get 
it, or to set as well? Can it also be useful for the encoder?

Regrds,
Dmitry.

On Freitag, 13. Dezember 2019 17:31:24 CET Keiichi Watanabe wrote:
> On Fri, Dec 13, 2019 at 11:20 PM Keiichi Watanabe <keiichiw@chromium.org> 
wrote:
> > On Thu, Dec 12, 2019 at 7:34 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 
wrote:
> > > Hi Keiichi,
> > > 
> > > Thank you for your comment.
> > > What do you thing about selection/crop rectangles? Should this be
> > > defined as a must have? Or we just assume the device always sticks to
> > > the stream resolution.
> > 
> > For decoding, the device needs to tell the driver crop rectangle when
> > a resolution change happens, as virtio-vdec has
> > virtio_vdec_host_req_stream_crop.
> > So, we should add a new field in virtio_video_param for it.
> > 
> > For what it's worth, we have C-headers that exposes Chrome's video
> > codec functionalities. So, our virtio-video device will do
> > decoding/encoding by calling these functions.
> > Decoder:
> > https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c0
> > 18c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h Encoder:
> > https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c0
> > 18c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h
> Oops, the links were not accesible...
> The correct links are:
> Decoder:
> https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d
> 014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h Encoder:
> https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d
> 014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h
> 
> Best,
> Keiichi
> 
> > BTW, I'm preparing the next version of virtio-video spec by addressing
> > points we discussed in this thread. I'll send it here next week,
> > hopefully.
> > 
> > Best,
> > Keiichi
> > 
> > > Regards,
> > > Dmitry.
> > > 
> > > On Donnerstag, 12. Dezember 2019 06:39:11 CET Keiichi Watanabe wrote:
> > > > On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
> > > > 
> > > > <dmitry.sepp@opensynergy.com> wrote:
> > > > > Hi,
> > > > > 
> > > > > Just to start, let's consider this v4l2 control:
> > > > > V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> > > > > As I can see, this control is referenced as a mandatory one in the
> > > > > Chromium
> > > > > sources [1].
> > > > > 
> > > > > So could someone from the Chromium team please explain why it is
> > > > > mandatory?
> > > > > (YouTube?) In fact, almost no encoders implement this control. Do we
> > > > > need
> > > > > it in virtio-video?
> > > > 
> > > > That control is used to encode videos in constant bitrate (CBR) mode,
> > > > which is critical for
> > > > real-time use case like video conferencing.
> > > > 
> > > > However, that Chromium source code just requires *host-side* drivers
> > > > to have the v4l2
> > > > control. Also, I guess it's rare that a guest app wants to use CQP
> > > > instead of CBR from our
> > > > experience.
> > > > So, I suppose we can omit this control in virtio spec for now by
> > > > assuming that guests
> > > > always use CBR mode.
> > > > 
> > > > Best regards,
> > > > Keiichi
> > > > 
> > > > > [1]
> > > > > https://chromium.googlesource.com/chromium/src/media/+/refs/heads/ma
> > > > > ster/
> > > > > gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
> > > > > 
> > > > > Regards,
> > > > > Dmitry.
> > > > > 
> > > > > On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > > > > > +Changyeon Jo <changyeon@google.com> for his awareness
> > > > > > 
> > > > > > Thanks,
> > > > > > - Enrico
> > > > > > 
> > > > > > 
> > > > > > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp
> > > > > > <dmitry.sepp@opensynergy.com>
> > > > > > 
> > > > > > wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > I'd like to invite everyone to share ideas regarding required
> > > > > > > encoder
> > > > > > > features
> > > > > > > in this separate sub-tree.
> > > > > > > 
> > > > > > > In general, encoder devices are more complex compared to
> > > > > > > decoders. So
> > > > > > > the
> > > > > > > question I'd like to rise is in what way we define the minimal
> > > > > > > subset
> > > > > > > of
> > > > > > > features to be implemented by the virtio-video.
> > > > > > > 
> > > > > > > We may look at the following to define the set of features:
> > > > > > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > > > > > 2. Android 10 Compatibility Definition [2].
> > > > > > > 
> > > > > > > Would be nice to hear about any specific requirements from the
> > > > > > > Chromium
> > > > > > > team as
> > > > > > > well.
> > > > > > > 
> > > > > > > [1]
> > > > > > > https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > > > > > [2]
> > > > > > > https://source.android.com/compatibility/android-cdd#5_2_video_e
> > > > > > > ncodin
> > > > > > > g
> > > > > > > 
> > > > > > > Thank you.
> > > > > > > 
> > > > > > > Best regards,
> > > > > > > Dmitry.
> > > > > > > 
> > > > > > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > > > > > >   Hi,
> > > > > > > >   
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > Agree.
> > > > > > > > 
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > Hmm, modern GPUs support both encoding and decoding ...
> > > > > > > > 
> > > > > > > > I don't think we should bake that restriction into the
> > > > > > > > specification.
> > > > > > > > It probably makes sense to use one virtqueue per function
> > > > > > > > though,
> > > > > > > > that
> > > > > > > > will simplify dispatching in both host and guest.
> > > > > > > > 
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > I think virtio-video should be able to work as stand-alone
> > > > > > > > device,
> > > > > > > > so we need some way to allocate buffers ...
> > > > > > > > 
> > > > > > > > Buffer sharing with other devices can be added later.
> > > > > > > > 
> > > > > > > > > > +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?
> > > > > > > > 
> > > > > > > > Grab the next free, update the spec accordingly, submit the
> > > > > > > > one-line
> > > > > > > > patch.
> > > > > > > > 
> > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > +enum virtio_video_pixel_format {
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > > > > > +
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > > > > > +};
> > > > > > > > > 
> > > > > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > > > > > > 
> > > > > > > reinventing a
> > > > > > > 
> > > > > > > > > mapping from formats to integers.
> > > > > > > > > Also, I suppose the word "pixel formats" means only raw
> > > > > > > > > (decoded)
> > > > > > > 
> > > > > > > formats.
> > > > > > > 
> > > > > > > > > But, it can be encoded format like H.264. So, I guess "image
> > > > > > > > > format"
> > > > > > > > > or
> > > > > > > > > "fourcc" is a better word choice.
> > > > > > > > 
> > > > > > > > Use separate pixel_format (fourcc) and stream_format (H.264
> > > > > > > > etc.)
> > > > > > > > enums?
> > > > > > > > 
> > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > +struct virtio_video_function {
> > > > > > > > > > +       struct virtio_video_desc desc;
> > > > > > > > > > +       __le32 function_type; /* One of
> > > > > > > > > > VIRTIO_VIDEO_FUNC_*
> > > > > > > > > > types */
> > > > > > > > > > +       __le32 function_id;
> > > > > > > > > > +       struct virtio_video_params in_params;
> > > > > > > > > > +       struct virtio_video_params out_params;
> > > > > > > > > > +       __le32 num_caps;
> > > > > > > > > > +       __u8 padding[4];
> > > > > > > > > > +       /* Followed by struct virtio_video_capability
> > > > > > > > > > video_caps[];
> > > > > > > 
> > > > > > > */
> > > > > > > 
> > > > > > > > > > +};
> > > > > > > > > > +\end{lstlisting}
> > > > > > > > > 
> > > > > > > > > If one device only has one functionality,
> > > > > > > > > virtio_video_function's
> > > > > > > 
> > > > > > > fields
> > > > > > > 
> > > > > > > > > will be no longer needed except in_params and out_params.
> > > > > > > > > So, we'd
> > > > > > > > > be
> > > > > > > > > able to remove virtio_video_function and have in_params and
> > > > > > > > > out_params
> > > > > > > 
> > > > > > > in
> > > > > > > 
> > > > > > > > > virtio_video_capability instead.
> > > > > > > > 
> > > > > > > > Same goes for per-function virtqueues (used virtqueue implies
> > > > > > > > function).
> > > > > > > > 
> > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > > > > > +       __le32 resource_id;
> > > > > > > > > > +       __u8 padding[4];
> > > > > > > > > > +};
> > > > > > > > > > +\end{lstlisting}
> > > > > > > > > > +
> > > > > > > > > > +\begin{description}
> > > > > > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > > > > > +\end{description}
> > > > > > > > > 
> > > > > > > > > I suppose that it'd be better not to have the above series
> > > > > > > > > of
> > > > > > > 
> > > > > > > T_RESOURCE
> > > > > > > 
> > > > > > > > > controls at least until we reach a conclusion in the thread
> > > > > > > > > of
> > > > > > > > > buffer-sharing device. If we end up concluding this type of
> > > > > > > > > controls
> > > > > > > > > is
> > > > > > > > > the best way, we'll be able to revisit here.
> > > > > > > > 
> > > > > > > > Well.  For buffer management there are a bunch of options.
> > > > > > > > 
> > > > > > > >  (1) Simply stick the buffers (well, pointers to the buffer
> > > > > > > >  pages)
> > > > > > > >  into
> > > > > > > >  
> > > > > > > >      the virtqueue.  This is the standard virtio way.
> > > > > > > >  
> > > > > > > >  (2) Create resources, then put the resource ids into the
> > > > > > > >  virtqueue.
> > > > > > > >  
> > > > > > > >      virtio-gpu uses that model.  First, because virtio-gpu
> > > > > > > >      needs an
> > > > > > > >      id
> > > > > > > >      to reference resources in the rendering command stream
> > > > > > > >      (virtio-video doesn't need this).  Also because (some
> > > > > > > >      kinds of)
> > > > > > > >      resources are around for a long time and the
> > > > > > > >      guest-physical ->
> > > > > > > >      host-virtual mapping needs to be done only once that way
> > > > > > > >      (which
> > > > > > > >      I think would be the case for virtio-video too because
> > > > > > > >      v4l2
> > > > > > > >      re-uses buffers in robin-round fashion).  Drawback is
> > > > > > > >      this
> > > > > > > >      assumes shared memory between host and guest (which is
> > > > > > > >      the case
> > > > > > > >      in typical use cases but it is not mandated by the virtio
> > > > > > > >      spec).
> > > > > > > >  
> > > > > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > > > > >  
> > > > > > > >      Out of scope for now, will probably added as optional
> > > > > > > >      feature
> > > > > > > >      later.
> > > > > > > > 
> > > > > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > > > > 
> > > > > > > > cheers,
> > > > > > > > 
> > > > > > > >   Gerd
> > > > > > > 
> > > > > > > ----------------------------------------------------------------
> > > > > > > -----
> > > > > > > To unsubscribe, e-mail:
> > > > > > > virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > For additional commands, e-mail:
> > > > > > > virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [RFC RESEND] virtio-video: Add virtio video device specification

[Keiichi Watanabe]

Hi Dmitry,

On Fri, Dec 20, 2019 at 11:25 PM Dmitry Sepp
<dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> Could you please explain in a more detailed way what exactly CROP is used for?
> Is it equivalent to G/S_CROP or G/S_SELECTION in v4l2? Do you only need to get
> it, or to set as well? Can it also be useful for the encoder?

It corresponds to struct v4l2_rect in V4L2, which is used in
G/S_SELECTION. IIUC, G/S_SELECTION is a kind of super set of G/S_CROP.
Both decoder and encoder can utilize both G_SELECTION and S_SELECTION.
Their usages are documented in the specs of V4L2 stateful
decoder/encoder API.

stateful decoder spec:
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-decoder.html
stateful encoder spec (in maintainer's tree):
https://hverkuil.home.xs4all.nl/codec-api/uapi/v4l/dev-encoder.html

Best regards,
Keiichi

>
> Regrds,
> Dmitry.
>
> On Freitag, 13. Dezember 2019 17:31:24 CET Keiichi Watanabe wrote:
> > On Fri, Dec 13, 2019 at 11:20 PM Keiichi Watanabe <keiichiw@chromium.org>
> wrote:
> > > On Thu, Dec 12, 2019 at 7:34 PM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> wrote:
> > > > Hi Keiichi,
> > > >
> > > > Thank you for your comment.
> > > > What do you thing about selection/crop rectangles? Should this be
> > > > defined as a must have? Or we just assume the device always sticks to
> > > > the stream resolution.
> > >
> > > For decoding, the device needs to tell the driver crop rectangle when
> > > a resolution change happens, as virtio-vdec has
> > > virtio_vdec_host_req_stream_crop.
> > > So, we should add a new field in virtio_video_param for it.
> > >
> > > For what it's worth, we have C-headers that exposes Chrome's video
> > > codec functionalities. So, our virtio-video device will do
> > > decoding/encoding by calling these functions.
> > > Decoder:
> > > https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c0
> > > 18c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h Encoder:
> > > https://chromium.git.corp.google.com/chromiumos/platform2/+/00ad71fa640c0
> > > 18c1d014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h
> > Oops, the links were not accesible...
> > The correct links are:
> > Decoder:
> > https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d
> > 014f467c50b9a7dc962a10/arc/vm/libvda/libvda_decode.h Encoder:
> > https://chromium.googlesource.com/chromiumos/platform2/+/00ad71fa640c018c1d
> > 014f467c50b9a7dc962a10/arc/vm/libvda/libvda_encode.h
> >
> > Best,
> > Keiichi
> >
> > > BTW, I'm preparing the next version of virtio-video spec by addressing
> > > points we discussed in this thread. I'll send it here next week,
> > > hopefully.
> > >
> > > Best,
> > > Keiichi
> > >
> > > > Regards,
> > > > Dmitry.
> > > >
> > > > On Donnerstag, 12. Dezember 2019 06:39:11 CET Keiichi Watanabe wrote:
> > > > > On Tue, Dec 10, 2019 at 10:16 PM Dmitry Sepp
> > > > >
> > > > > <dmitry.sepp@opensynergy.com> wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Just to start, let's consider this v4l2 control:
> > > > > > V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> > > > > > As I can see, this control is referenced as a mandatory one in the
> > > > > > Chromium
> > > > > > sources [1].
> > > > > >
> > > > > > So could someone from the Chromium team please explain why it is
> > > > > > mandatory?
> > > > > > (YouTube?) In fact, almost no encoders implement this control. Do we
> > > > > > need
> > > > > > it in virtio-video?
> > > > >
> > > > > That control is used to encode videos in constant bitrate (CBR) mode,
> > > > > which is critical for
> > > > > real-time use case like video conferencing.
> > > > >
> > > > > However, that Chromium source code just requires *host-side* drivers
> > > > > to have the v4l2
> > > > > control. Also, I guess it's rare that a guest app wants to use CQP
> > > > > instead of CBR from our
> > > > > experience.
> > > > > So, I suppose we can omit this control in virtio spec for now by
> > > > > assuming that guests
> > > > > always use CBR mode.
> > > > >
> > > > > Best regards,
> > > > > Keiichi
> > > > >
> > > > > > [1]
> > > > > > https://chromium.googlesource.com/chromium/src/media/+/refs/heads/ma
> > > > > > ster/
> > > > > > gpu/v4l2/v4l2_video_encode_accelerator.cc#1500
> > > > > >
> > > > > > Regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > On Montag, 9. Dezember 2019 22:12:28 CET Enrico Granata wrote:
> > > > > > > +Changyeon Jo <changyeon@google.com> for his awareness
> > > > > > >
> > > > > > > Thanks,
> > > > > > > - Enrico
> > > > > > >
> > > > > > >
> > > > > > > On Mon, Dec 9, 2019 at 6:20 AM Dmitry Sepp
> > > > > > > <dmitry.sepp@opensynergy.com>
> > > > > > >
> > > > > > > wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > I'd like to invite everyone to share ideas regarding required
> > > > > > > > encoder
> > > > > > > > features
> > > > > > > > in this separate sub-tree.
> > > > > > > >
> > > > > > > > In general, encoder devices are more complex compared to
> > > > > > > > decoders. So
> > > > > > > > the
> > > > > > > > question I'd like to rise is in what way we define the minimal
> > > > > > > > subset
> > > > > > > > of
> > > > > > > > features to be implemented by the virtio-video.
> > > > > > > >
> > > > > > > > We may look at the following to define the set of features:
> > > > > > > > 1. USB video, 2.3.6 Encoding Unit [1].
> > > > > > > > 2. Android 10 Compatibility Definition [2].
> > > > > > > >
> > > > > > > > Would be nice to hear about any specific requirements from the
> > > > > > > > Chromium
> > > > > > > > team as
> > > > > > > > well.
> > > > > > > >
> > > > > > > > [1]
> > > > > > > > https://www.usb.org/sites/default/files/USB_Video_Class_1_5.zip
> > > > > > > > [2]
> > > > > > > > https://source.android.com/compatibility/android-cdd#5_2_video_e
> > > > > > > > ncodin
> > > > > > > > g
> > > > > > > >
> > > > > > > > Thank you.
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Dmitry.
> > > > > > > >
> > > > > > > > On Mittwoch, 4. Dezember 2019 10:16:20 CET Gerd Hoffmann wrote:
> > > > > > > > >   Hi,
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Agree.
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Hmm, modern GPUs support both encoding and decoding ...
> > > > > > > > >
> > > > > > > > > I don't think we should bake that restriction into the
> > > > > > > > > specification.
> > > > > > > > > It probably makes sense to use one virtqueue per function
> > > > > > > > > though,
> > > > > > > > > that
> > > > > > > > > will simplify dispatching in both host and guest.
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > I think virtio-video should be able to work as stand-alone
> > > > > > > > > device,
> > > > > > > > > so we need some way to allocate buffers ...
> > > > > > > > >
> > > > > > > > > Buffer sharing with other devices can be added later.
> > > > > > > > >
> > > > > > > > > > > +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?
> > > > > > > > >
> > > > > > > > > Grab the next free, update the spec accordingly, submit the
> > > > > > > > > one-line
> > > > > > > > > patch.
> > > > > > > > >
> > > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > > +enum virtio_video_pixel_format {
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > > > > > > > > +
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I420,
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_I422,
> > > > > > > > > > > +       VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > > > > > > > > +};
> > > > > > > > > >
> > > > > > > > > > I'm wondering if we can use FOURCC instead. So, we can avoid
> > > > > > > >
> > > > > > > > reinventing a
> > > > > > > >
> > > > > > > > > > mapping from formats to integers.
> > > > > > > > > > Also, I suppose the word "pixel formats" means only raw
> > > > > > > > > > (decoded)
> > > > > > > >
> > > > > > > > formats.
> > > > > > > >
> > > > > > > > > > But, it can be encoded format like H.264. So, I guess "image
> > > > > > > > > > format"
> > > > > > > > > > or
> > > > > > > > > > "fourcc" is a better word choice.
> > > > > > > > >
> > > > > > > > > Use separate pixel_format (fourcc) and stream_format (H.264
> > > > > > > > > etc.)
> > > > > > > > > enums?
> > > > > > > > >
> > > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > > +struct virtio_video_function {
> > > > > > > > > > > +       struct virtio_video_desc desc;
> > > > > > > > > > > +       __le32 function_type; /* One of
> > > > > > > > > > > VIRTIO_VIDEO_FUNC_*
> > > > > > > > > > > types */
> > > > > > > > > > > +       __le32 function_id;
> > > > > > > > > > > +       struct virtio_video_params in_params;
> > > > > > > > > > > +       struct virtio_video_params out_params;
> > > > > > > > > > > +       __le32 num_caps;
> > > > > > > > > > > +       __u8 padding[4];
> > > > > > > > > > > +       /* Followed by struct virtio_video_capability
> > > > > > > > > > > video_caps[];
> > > > > > > >
> > > > > > > > */
> > > > > > > >
> > > > > > > > > > > +};
> > > > > > > > > > > +\end{lstlisting}
> > > > > > > > > >
> > > > > > > > > > If one device only has one functionality,
> > > > > > > > > > virtio_video_function's
> > > > > > > >
> > > > > > > > fields
> > > > > > > >
> > > > > > > > > > will be no longer needed except in_params and out_params.
> > > > > > > > > > So, we'd
> > > > > > > > > > be
> > > > > > > > > > able to remove virtio_video_function and have in_params and
> > > > > > > > > > out_params
> > > > > > > >
> > > > > > > > in
> > > > > > > >
> > > > > > > > > > virtio_video_capability instead.
> > > > > > > > >
> > > > > > > > > Same goes for per-function virtqueues (used virtqueue implies
> > > > > > > > > function).
> > > > > > > > >
> > > > > > > > > > > +\begin{lstlisting}
> > > > > > > > > > > +struct virtio_video_resource_detach_backing {
> > > > > > > > > > > +       struct virtio_video_ctrl_hdr hdr;
> > > > > > > > > > > +       __le32 resource_id;
> > > > > > > > > > > +       __u8 padding[4];
> > > > > > > > > > > +};
> > > > > > > > > > > +\end{lstlisting}
> > > > > > > > > > > +
> > > > > > > > > > > +\begin{description}
> > > > > > > > > > > +\item[\field{resource_id}] internal id of the resource.
> > > > > > > > > > > +\end{description}
> > > > > > > > > >
> > > > > > > > > > I suppose that it'd be better not to have the above series
> > > > > > > > > > of
> > > > > > > >
> > > > > > > > T_RESOURCE
> > > > > > > >
> > > > > > > > > > controls at least until we reach a conclusion in the thread
> > > > > > > > > > of
> > > > > > > > > > buffer-sharing device. If we end up concluding this type of
> > > > > > > > > > controls
> > > > > > > > > > is
> > > > > > > > > > the best way, we'll be able to revisit here.
> > > > > > > > >
> > > > > > > > > Well.  For buffer management there are a bunch of options.
> > > > > > > > >
> > > > > > > > >  (1) Simply stick the buffers (well, pointers to the buffer
> > > > > > > > >  pages)
> > > > > > > > >  into
> > > > > > > > >
> > > > > > > > >      the virtqueue.  This is the standard virtio way.
> > > > > > > > >
> > > > > > > > >  (2) Create resources, then put the resource ids into the
> > > > > > > > >  virtqueue.
> > > > > > > > >
> > > > > > > > >      virtio-gpu uses that model.  First, because virtio-gpu
> > > > > > > > >      needs an
> > > > > > > > >      id
> > > > > > > > >      to reference resources in the rendering command stream
> > > > > > > > >      (virtio-video doesn't need this).  Also because (some
> > > > > > > > >      kinds of)
> > > > > > > > >      resources are around for a long time and the
> > > > > > > > >      guest-physical ->
> > > > > > > > >      host-virtual mapping needs to be done only once that way
> > > > > > > > >      (which
> > > > > > > > >      I think would be the case for virtio-video too because
> > > > > > > > >      v4l2
> > > > > > > > >      re-uses buffers in robin-round fashion).  Drawback is
> > > > > > > > >      this
> > > > > > > > >      assumes shared memory between host and guest (which is
> > > > > > > > >      the case
> > > > > > > > >      in typical use cases but it is not mandated by the virtio
> > > > > > > > >      spec).
> > > > > > > > >
> > > > > > > > >  (3) Import external resources (from virtio-gpu for example).
> > > > > > > > >
> > > > > > > > >      Out of scope for now, will probably added as optional
> > > > > > > > >      feature
> > > > > > > > >      later.
> > > > > > > > >
> > > > > > > > > I guess long-term we want support either (1)+(3) or (2)+(3).
> > > > > > > > >
> > > > > > > > > cheers,
> > > > > > > > >
> > > > > > > > >   Gerd
> > > > > > > >
> > > > > > > > ----------------------------------------------------------------
> > > > > > > > -----
> > > > > > > > To unsubscribe, e-mail:
> > > > > > > > virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > > For additional commands, e-mail:
> > > > > > > > virtio-dev-help@lists.oasis-open.org
>
>