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

[Dave Airlie <airlied@gmail.com>]

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 :-),
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.


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

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

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