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

[Dave Airlie <airlied@gmail.com>]

On Wed, Apr 9, 2014 at 6:04 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> I've just moved the implementation and guest driver here over to not
>> using a union anymore,
>> http://cgit.freedesktop.org/~airlied/qemu/log/?h=virtio-gpu
>> http://cgit.freedesktop.org/~airlied/linux/log/?h=virtio-vga-3d
>
> Finally found the time to continue working on the virtio-gpu spec.
>
> Adapted to the latest changes and to some suggestions in this thread.
> More to be done.  Current state is here:
>
>     http://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-gpu
>
> I've got some questions/suggestions while going over this:
>
>
> First:  How we'll go handle error reporting?  It's not really there at
> the moment ...
>
> I think we should drop the VIRTGPU_CMD_HAS_RESP bit and have all
> commands return a response.  Usually that would be just
> VIRTIO_GPU_RESP_SUCCESS.  VIRTIO_GPU_CMD_GET_DISPLAY_INFO will return
> VIRTIO_GPU_RESP_DISPLAY_CHANGE of course.  On error we can return
> VIRTIO_GPU_RESP_ERROR with an error information struct, like this:
>
> struct virtio_gpu_display_info {
>         struct virtio_gpu_resp_hdr hdr;
>         le32 error_code;
>         le32 error_id;
> };
>
> where error_id may (depending on error code) refer to the object the
> error applies to, i.e. for resource related errors that would be the
> resource id.
>
> For "can't continue until device reset" class of errors we can add a
> VIRTIO_GPU_EVENT_FATAL_ERROR event bit.

I looked into doing something like this, but had one of those what use
is this for the guest questions,

Resource creation is one of things I tried to model using return
values, however it ran into a number of
issues, currently the guest sends the create resource, then sends
attach backing store to resource, I'd rather
not make resource creation fully synchronous, since it will slow down
3D to a fairly measurable extent,

Now in this case the resource create will fail, the attach backing
store will fail due to illegal resource id, and
we'll get a cascade of failures all too late to do anything useful
with, so I'd really like to spec how we think
the guest can efficiently handle these sort of errors,

We also need some sort of async errors from config space interrupt,
since if something on the host side
happens that is fatal to the gpu it would be good to know.


>> > As I understand it:  You create one (or more) cursor resources using
>> > normal command queue commands.  Then you'll set cursor_id to the
>> > resource id you want use as cursor, fill the other fields.
>> > generation_count being incremented (relative to the previous command)
>> > implies the cursor sprite needs to be reloaded.  same generation_count
>> > indicates only the position (x & y) has changed.  Correct dave?
>>
>> Yes correct,
>
> Second:  Is the generation count used for anything but indicating the
> cursor should be reloaded?  I tend to think we should make the command
> and cursor queue pretty much identical (i.e. same format for commands
> and responses).  Then have two commands for the cursor, one to load it
> from a resource and one to simply move it.  command queue accepts all
> commands.  cursor queue accepts only cursor commands.

Just a reload cursor indicator, the reason I avoided identical queues is that
initially I didn't see the point of attaching a full command to each queue
entry for cursors, they are mostly handled in real hw by registers, and having
a queue of updates seems unnecessary, so I was treating the vq as an irq,
and attached the same status page to it,

> I guess we need a scanout_id field for the cursor updates so you can
> place the cursor on any head ... ?

Currently the cursor is relative to the crtc, so yes we probably need
to add scanout_id
for this to work properly.

>
> Do we want support for multiple cursors?

I suppose ideally we could support multiple cursors but I don't think
its a requirement as an initial feature.

> Third: ressource management.  Currently the guest can allocate unlimited
> ressources on the host using the VIRTIO_GPU_CMD_RESOURCE_CREATE_2D
> command.  We certainly want apply a limit here so the guest can't DoS
> the host.  And we should communicate that to the guest, either by
> providing a command to query it or in config space.

I've also contemplated this, again with 3D this gets rather messy,
since we've no actual idea
how much space an object takes up once the 3D driver is allocating it,
currently the guest
kernel is limiting things allocated by the guest userspace using the
TTM allocation tracking stuff,
but maybe we should have an option that people can disable if they don't care.

If this was to act like real hw VRAM, we'd have to start swapping
objects in/out unnecessarily
where we'd just be moving RAM around for no reason in a VM.

Dave.