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

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

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

Maybe we should add a status command, with two purposes:  First to
return the status, second to group commands.  So, to stick with resource
creation, the guest will queue these commands:

  RESOURCE_CREATE
  RESOURCE_ATTACH_BACKING
  [ possibly more ops on the just creates resource ]
  GET_STATUS

When a command fails, the device will skip all commands until it finds a
GET_STATUS command in the ring, fills the error information there.  We
probably want assign IDs to the commands so GET_STATUS can easily
indicate which of the commands did fail.

Commands possibly queued after GET_STATUS will be processed normally, so
the guest can pipeline commands to create+fill a bunch of resources at
once, to reduce the number of context switches.

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

We could add two error event bits.  One meaning "everything is f*cked
up, please reset + reinitialize device", the other meaning "something
went wrong, please use GET_STATUS to figure details".

> >> > 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 see.  In case multiple updates are queued this model allows you to
simply throw away everything but the most recent one, then treat it as
update to the cursor register file.

Main advantage of a full command is that it is easier to extend as you
can simply add new commands for new features then.

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

Yep, not a priority, but something to keep in mind when designing
things.  I think sticking a reserved field into the cursor struct(s) is
enough for now.

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

Hmm.  Maybe account in pixels not memory then?  And limit the number of
objects too?

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

The host must not trust the guest here for security reasons.  So I think
virtio-gpu device should have a limit (making that user-configurable is
fine), communicate that limit to the guest so the driver can configure
ttm accordingly.

cheers,
  Gerd