Re: [virtio-dev] [PATCH 2/2] Add GPU device type.

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Mar 21, 2014 at 03:53:17PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Looks good, thanks.
Generally this needs to be tightened up some more,
adding compliance statements where appropriate.

> ---
>  content.tex    |   1 +
>  virtio-gpu.tex | 276 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 277 insertions(+)
>  create mode 100644 virtio-gpu.tex
> 
> diff --git a/content.tex b/content.tex
> index 196950d..f7e3e83 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -4887,6 +4887,7 @@ descriptor for the \field{sense_len}, \field{residual},
>  \field{status_qualifier}, \field{status}, \field{response} and
>  \field{sense} fields.
>  
> +\input{virtio-gpu.tex}
>  \input{virtio-input.tex}
>  
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> diff --git a/virtio-gpu.tex b/virtio-gpu.tex
> new file mode 100644
> index 0000000..c9b4d62
> --- /dev/null
> +++ b/virtio-gpu.tex
> @@ -0,0 +1,276 @@
> +\section{GPU Device}\label{sec:Device Types / GPU Device}
> +
> +virtio-gpu is a virtio based graphics adapter.  Supported features:
> +
> +\begin{enumerate}
> +\item ARGB Hardware cursors
> +\item multiple outputs
> +\end{enumerate}
> +
> +\subsection{Device ID}\label{sec:Device Types / GPU Device / Device ID}
> +
> +16
> +
> +\subsection{Virtqueues}\label{sec:Device Types / GPU Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] controlq
> +\item[1] cursorq
> +\item[2] eventq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / GPU Device / Feature bits}
> +
> +None yet.  Upcoming 3D support will add one or more.

Should 2D commands have a feature bit?
Presumably 2D can be implemented on top of 3D?

> +
> +\subsection{Device configuration layout}\label{sec:Device Types / GPU Device / Device configuration layout}
> +
> +None.
> +
> +\subsection{Device Initialization}\label{sec:Device Types / GPU Device / Device Initialization}
> +
> +TODO.

Need some text here?
E.g. does driver create some VQs? Fill them with buffers? Etc.

> +
> +\subsection{Device Operation}\label{sec:Device Types / GPU Device / Device Operation}
> +
> +The virtio-gpu is based around the concept of resources private to the
> +host, the guest must DMA transfer into these resources. This is a
> +design requirement in order to interface with future 3D rendering.

Need to mark up 3D stubs somehow so they stand out.
Just make them comments?

> In
> +the unaccelerated 

word missing?

>there is no support for DMA transfers from
> +resources, just to them.
> +
> +Resources are initially simple 2D resources, consisting of a width,
> +height and format along with an identifier. The guest must then attach
> +backing store to the resources in order for DMA transfers to
> +work. This is like a GART in a real GPU.
> +
> +A typical guest user would create a 2D resource using
> +VIRTGPU_CMD_RESOURCE_CREATE_2D, attach backing store using
> +VIRTGPU_CMD_RESOURCE_ATTACH_BACKING, then attach the resource to a
> +scanout using VIRTGPU_CMD_SET_SCANOUT, then use
> +VIRTGPU_CMD_TRANSFER_SEND_2D to send updates to the resource, and
> +finally VIRTGPU_CMD_RESOURCE_FLUSH to flush the scanout buffers to
> +screen.
> +
> +\subsubsection{Device Operation: controlq}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: controlq}
> +
> +\begin{lstlisting}
> +struct virtgpu_command {

virtio_gpu_ for consistency with other devices?

> +	le32 type;
> +	le32 flags;
> +	le64 fence_id;
> +	union virtgpu_cmds {
> +		struct virtgpu_cmd_get_cap get_cap;
> +		struct virtgpu_resource_create_2d resource_create_2d;
> +		struct virtgpu_resource_unref resource_unref;
> +		struct virtgpu_resource_flush resource_flush;
> +		struct virtgpu_set_scanout set_scanout;
> +		struct virtgpu_transfer_to_host_2d transfer_to_host_2d;
> +		struct virtgpu_resource_attach_backing resource_attach_backing;
> +		struct virtgpu_resource_inval_backing resource_inval_backing;
> +	} u;
> +};

unions have some unfortunate side effects.
For example hosts will need to assume buffer size is max of all commands.
And if we later add a command that needs a different buffer size, that
will change.
Using unions also makes it too easy to have uninitialized fields
laying around.

for this reason, I think it's best to do:

struct virtgpu_command_hdr {
	 +     le32 type;
 	 +     le32 flags;
 	 +     le64 fence_id;
}

and have each command include that implicitly or explicitly.


> +
> +struct virtgpu_response {
> +	le32 type;
> +	le32 flags;
> +	union virtgpu_resps {
> +		struct virtgpu_display_info display_info;
> +		union virtgpu_caps caps;

virtgpu_caps seems undefined

> +	} u;

same comment about the use of unions.

> +};
> +\end{lstlisting}


So I assume each command has an out buffer
using virtgpu_cmds format, followed by in buffer
using virtgpu_response?


> +
> +\begin{description}
> +
> +\item[VIRTGPU_CMD_GET_DISPLAY_INFO]
> +Retrieve the current output configuration.
> +
> +\begin{lstlisting}
> +#define VIRTGPU_MAX_SCANOUTS 16
> +struct virtgpu_display_info {
> +	le32 num_scanouts;
> +	struct {
> +		le32 enabled;
> +		le32 width;
> +		le32 height;
> +		le32 x;
> +		le32 y;
> +		le32 flags;
> +	} pmodes[VIRTGPU_MAX_SCANOUTS];
> +};
> +\end{lstlisting}
> +
> +This sends a response in the same queue slot.

what does this mean exactly?

> The response contains
> +the max number of scanouts the host can support, along with a list of
> +per-scanout information. The info contains whether the scanout is
> +enabled, what its preferred x, y, width and height are and some future
> +flags.
> +
> +\item[VIRTGPU_CMD_GET_CAPS]
> +FIXME: seems there are changes (new struct virtgpu_cmd_get_cap)here
> +for 3D ...
> +
> +\item[VIRTGPU_CMD_RESOURCE_CREATE_2D]
> +Create a 2D resource on the host.
> +
> +\begin{lstlisting}
> +struct virtgpu_resource_create_2d {
> +	le32 resource_id;
> +	le32 format;
> +	le32 width;
> +	le32 height;
> +};
> +\end{lstlisting}
> +
> +This creates a 2D resource on the host with the specified width,
> +height and format. Only a small subset of formats are support.

supported

> The
> +resource ids are generated by the guest.

Are these some uniquness assumptions?


> +
> +\item[VIRTGPU_CMD_RESOURCE_UNREF]
> +Destroy a resource.
> +
> +\begin{lstlisting}
> +struct virtgpu_resource_unref {
> +	le32 resource_id;
> +};
> +\end{lstlisting}
> +
> +This informs the host that a resource is no longer required by the
> +guest.
> +
> +\item[VIRTGPU_CMD_SET_SCANOUT]
> +Set the scanout parameters for a single output.
> +
> +\begin{lstlisting}
> +struct virtgpu_set_scanout {
> +	le32 scanout_id;
> +	le32 resource_id;
> +	le32 width;
> +	le32 height;
> +	le32 x;
> +	le32 y;
> +};
> +\end{lstlisting}
> +
> +This sets the scanout parameters for a single scanout. The resource_id
> +is the resource to be scanned out from, along with a rectangle
> +specified by x, y, width and height.
> +
> +\item[VIRTGPU_CMD_RESOURCE_FLUSH]
> +Flush a scanout resource
> +
> +\begin{lstlisting}
> +struct virtgpu_resource_flush {
> +	le32 resource_id;
> +	le32 width;
> +	le32 height;
> +	le32 x;
> +	le32 y;
> +};
> +\end{lstlisting}
> +
> +This flushes a resource to screen, it takes a rectangle and a resource
> +id, and flushes any scanouts the resource is being used on.
> +
> +\item[VIRTGPU_CMD_TRANSFER_TO_HOST_2D]
> +Transfer from guest memory to host resource.
> +
> +\begin{lstlisting}
> +struct virtgpu_transfer_to_host_2d {
> +	le32 resource_id;
> +	le32 offset;
> +	le32 width;
> +	le32 height;
> +	le32 x;
> +	le32 y;
> +};
> +\end{lstlisting}
> +
> +This takes a resource id along with an destination offset

a destination offset

> into the
> +resource, and a box to transfer from the host backing for the
> +resource.

so are we doing transfer from host backing to memory then?
above text says to host resource ...

> +
> +\item[VIRTGPU_CMD_RESOURCE_ATTACH_BACKING]
> +Assign backing pages to a resource.
> +
> +\begin{lstlisting}
> +struct virtgpu_resource_attach_backing {
> +	le32 resource_id;
> +	le32 nr_entries;
> +};
> +\end{lstlisting}
> +
> +This assign

assigns

> an array of guest pages as the backing store for a
> +resource. These pages are then used for the transfer operations for
> +that resource from that point on.
> +
> +\item[VIRTGPU_CMD_RESOURCE_INVAL_BACKING]
> +Detach backing pages from a resource.
> +
> +\begin{lstlisting}
> +struct virtgpu_resource_inval_backing {
> +	le32 resource_id;
> +};
> +\end{lstlisting}
> +
> +This detaches any backing pages from a resource, to be used

s/to be used/this is useful/

> in case of
> +guest swapping or object destruction.
> +
> +\end{description}
> +
> +\subsubsection{Device Operation: cursorq}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: cursorq}
> +
> +\begin{lstlisting}
> +struct virtgpu_hw_cursor_page {
> +	le32 cursor_x, cursor_y;
> +	le32 cursor_hot_x, cursor_hot_y;
> +	le32 cursor_id;
> +	le32 generation_count;
> +};
> +\end{lstlisting}
> +
> +The cursor queue accepts on structure

one structure?

> per vq entry. The cursor is just
> +a standard 2D resource that is 64x64 sized. Transfers are used to
> +upload the cursor contents.

which way is up? just say to load from ... to ...

> +
> +For cursor movement, only \field{cursor_x} and \field{cursor_y} need to be
> +updated.
> +
> +For a cursor update where the guest has transferred a new cursor into
> +the resource, \field{cursor_hot_x}, \field{cursor_hot_y} and
> +\field{cursor_id} should be updated and \field{generation_count}
> +should be increased by one,

you mean each successive command as compared to the previous one?

> so the host knows to refresh its copy of
> +the cursor data from the resource.
> +
> +\subsubsection{Device Operation: eventq}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: eventq}
> +
> +\begin{lstlisting}
> +struct virtgpu_event {
> +	le32 type;
> +	le32 err_code;
> +	union virtgpu_events {
> +		struct virtgpu_display_info display_info;
> +	} u;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +
> +\item[VIRTGPU_EVENT_ERROR]
> +3D only ?
> +
> +\item[VIRTGPU_EVENT_DISPLAY_CHANGE]
> +Contains the same info as the response to VIRTGPU_CMD_GET_DISPLAY_INFO.

Presumably this should read "type: one of VIRTGPU_EVENT_ERROR
VIRTGPU_EVENT_DISPLAY_CHANGE"?

How are these events used?


> +
> +\subsection{VGA Compatibility}\label{sec:Device Types / GPU Device / VGA Compatibility}
> +
> +Applies to Virtio Over PCI only.  The GPU device can come with and
> +without VGA compatibility.  The PCI class should be DISPLAY_VGA if VGA
> +compatibility is present and DISPLAY_OTHER otherwise.
> +
> +VGA compatibility:  PCI region 2 has a linear framebuffer, standard vga
> +and bochs dispi interface registers are present.  Enabling virtio
> +switches the device from vga compat into native virtio mode.

what does enabling virtio mean?
when do we switch?
probably at driver_ok?

> +
> +\end{description}
> -- 
> 1.8.3.1
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org