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

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

On Mon, Mar 24, 2014 at 08:31:47AM +1000, Dave Airlie wrote:
> On Mon, Mar 24, 2014 at 7:04 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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.
> 
> I'm also not sure we should be going to such lengths yet to
> standardise this, I'm not sure the design is ready to be set in stone
> I'd really prefer if the design had a bit more review before then :-),

Okay sure, we can keep this out of tree for now.
Making the spec a bit more explicit will only help review though.

> but I'll reply inline below to a few bits.
> 
> I think we should at least handle OOM type situations on resource
> allocations and maybe some other error conditions a little bit, as
> well of course as catching overflows etc.
> 
> Like I think removing the event VQ is probably a good idea so it will
> mean rewriting a lot of this stuff.
> 
> some replies inline below,
> 
> >> +++ 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.
> 
> 3 VQs in the current one, command, cursor and event. the event one
> filled with buffers, maybe it'll be removed.
> 
> >
> >> +
> >> +\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?
> 
> The 3D commands will be at the same level as the 2D ones, I'm a bit
> worried about how well we can document them in a standard,
> particularly the submit userspace commands one.

Hmm are they part of some other standard?
If yes - we can just refer to that.


> 
> >> +
> >> +\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?
> 
> Only commands that have the resp required bit have a response, so
> currently only the display info query.
> 
> >> +This sends a response in the same queue slot.
> >
> > what does this mean exactly?
> 
> I meant it doesn't use another queue, it just puts the reply into the
> first out scatter gather.


Ah ok, so I would just say this:

	Each command has an out buffer with the command parameters,
	optionally followed by in buffer for the response.
	Only commands that have the resp required bit set have a response.



> >
> >> 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?
> 
> The guest shouldn't create resources with the same id as one it created already.
> >
> >
> >> +
> >> +\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 ...
> 
> No from guest memory to host backing, the other way around is only
> required for 3D.
> Probably just reword that sentence.
> 
> >> 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 ...
> 
> Its a cursor, but yes from guest to host.
> >> +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?
> 
> Yes each set of cursor info should be atomic wrt the generation_count.
> >> +             struct virtgpu_display_info display_info;
> >> +     } u;
> >> +};
> >> +\end{lstlisting}
> >> +
> >> +\begin{description}
> >> +
> >> +\item[VIRTGPU_EVENT_ERROR]
> >> +3D only ?
> 
> No we can have errors on 2D like too big resource allocation requests,
> or asking to scanout a surface outside a resource bounday, or trying
> to buffer overflow a transfer, though the guest kernel should be in
> error if any of these occur, and maybe a single bit, you did something
> bad is good enough.
> 
> >> +
> >> +\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?
> 
> When something external to the guest resizes, so the SDL or spice
> client resizes its screen, you get the events.


Right - I really meant what happens with these structures?
Above makes me think there's a VQ pre-filled with in buffers,
and events go there using this format.

> >> +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?
> 
> At the moment we switch on the first scanout setting, as otherwise
> we'd have nothing on the screen for a long period of time during
> driver init.
> 
> Dave.