[virglrenderer-devel] multiprocess model and GL

[Gerd Hoffmann]

On Fri, Jan 24, 2020 at 10:08:39AM -0800, Chia-I Wu wrote:
> On Fri, Jan 24, 2020 at 1:29 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
> >
> > > Unless we have a strong feeling
> > > that requiring two virtio commands to create a vk resource are too
> > > many, we can worry about it after we have a prototype and some
> > > numbers.
> >
> > Well, performance-wise the number of virtio commands doesn't matter
> > much.  The number of context switches does.
> Yeah, two virtio commands here imply two ioctls and two vq kicks.

Latest kernel driver has support for enabling and disabling vq kicks.
Right now it's used only to batch page-flip virtio commands (i.e. submit
multiple virtio commands, notify only after the last one so they all are
processed with a single vmexit).

Doing the same for multiple virtio commands triggered by a single ioctl
is trivial, i.e. a LIST ioctl could easily submit a bunch of virtio
commands (one for each resource) and notify the host only after
submitting the last one so they all are processed in one go.

Doing it across multiple ioctls is a bit more effort but
should be doable too.

> > So, for INIT_BLOB, we might consider to use an *array*.  So you can
> > initialize a bunch of resources with a single ioctl.  Then pack all
> > allocation commands into a single execbuffer and run that.  Voila,
> > you can initialize a bunch of resources with only two system calls.
> The userspace does not usually allocate buffers in batches though.
> The array version can be useful if the userspace can create a bunch of
> size-less resources first and provide the sizes later.

Hmm, then we would need a separate ioctl to initialize the gem object,
i.e. the workflow would be:

  GETID_BLOB
  alloc via execbuffer
  INIT_BLOB(resource-id,size)

One more context switch.  On the other hand we don't need to know the
size beforehand, so we should be able to skip the extra metadata query
step then.  Also INIT_BLOB can be done lazily.  Not sure how much of the
win that actually would be in practice, we would avoid the ioctl only in
case userspace creates exportable memory without actually exporting it.

> > > gbm resources here, and dumb resources, udmabuf resources, and classic
> > > resources belong to main process allocations.
> >
> > Not sure about gbm.  But, yes, dumb + classic are global.
> Yeah, gbm objects can use the init_blob+execbuffer trick as well.  The
> host does not necessarily need to run the gbm context in a per-context
> process.

What would be in the execbuffer then for gbm?

Right now execbuffer is defined as "commands which virglrenderer
understands", but that wouldn't work for gbm allocations ...

> That implies yet another kind of contexts.  I guess the kernel can
> provide a SET_FEATURE (or INIT_CONTEXT) ioctl with an opaque `__u32
> kind' parameter.  The parameter decides the wire format of execbuffer
> which the kernel does not care.

Not fully sure about that.  I had the impression that Gurchetan Singh
wants gbm allocations in gl/vk contexts too.

> > >  - ALLOC_BLOB: allocate a blob storage in host main process
> > >  - ALLOC_TYPED: allocate a typed storage in host main process
> > >  - ALLOC_IOVEC: allocate a udmabuf from guest iovec in host main process
> >
> > size is needed for INIT.
> > storage too (the kernel needs to know whenever it should allocate guest
> > memory pages or reserve some address space in the vram bar).
> If the kernel can provide CREATE_BLOB_LIST, to create a list of
> id-only gem bos first, and INIT_BLOB, to initialize an individual gem
> bo, it can be very useful.  CREATE_BLOB_LIST generate a virtio
> command, but the overhead is amortized.  INIT_BLOB generates the
> ATTACH_BACKING command only when a shmem is requested.
> 
> > ALLOC will trigger *host* allocations.
> Exactly.
> 
> If we are ok with using execbuffer for gbm objects, we might as well
> say that when a ALLOC_FOO is needed and requires any argument,
> execbuffer should be used instead.  Then ALLOC_BLOB or ALLOC_IOVEC can
> be merged into INIT_BLOB
> 
> struct drm_virtgpu_resource_init_blob {
> __u64 size;         /* in */
> __u32 flags;        /* in */
> __u32 host_flags; /* in, no host flag defined yet */
> __u32 host_alloc; /* in, NONE, ALLOC_BLOB or ALLOC_IOVEC */
> __u32 bo_handle;    /* out */
> __u32 res_handle;   /* out */
> __u32 pad;
> };

Yep, for the simple case we can do it in a single ioctl.

Execbuffer allocations still need three: GETID + execbuffer + INIT.
Where GETID can have a LIST variant and execbuffer can probably do
multiple allocations in one go too.  INIT needs to wait for execbuffer
to finish to get the object size.

> > >  - INIT_BLOB+shmem+ALLOC_TYPED is equivalent to the classic RESOURCE_CREATE
> > >  - INIT_BLOB+shmem+ALLOC_IOVEC is equivalent to RESOURCE_CREATE_SHARED
> >
> > BTW: I'm not sure any more RESOURCE_CREATE_SHARED is that a great plan.
> Not exactly RESOURCE_CREATE_SHARED, but the mechanism can be used to
> achieve something similar to DRM_IOCTL_I915_GEM_USERPTR in the future.

Is this what it sounds like, i.e. create a gem object from (guest)
userspace allocated memory?  If so this should be easy to do, virtio-gpu
objects are are just guest memory pages after all.  So we should be able
to pin them via get_user_pages() and run with them ...

> > I think it's more useful to integrate that into to new INIT/ALLOC BLOB
> > ioctls/virtio commands.
> Sure, as long as the host can distinguish them (to create a new host
> storage for dumb or to create a udmabuf from iovec).

Why does the host need to know?  The host does need (a) the list of
pages (sent via attach_backing), and (b) a bool which tells whenever it
is ok to directly access the backing storage without explicit transfer
commands send by the guest.

> > > Yeah, it makes more sense for the guest kernel to manage the address
> > > space.  Thanks.  I mixed this up with shared memfd heap above where I
> > > think guests need to get offsets from host (maybe I am wrong there
> > > too?)
> >
> > It's the hosts job to manage the heap and the offsets, the guest doesn't
> > need to know.  The guest just asks for this (host) resource being mapped
> > at that place (in guest address space).
> It is for COQOS hypervisor which cannot remap allocations to the
> guest-requested addresses, as I was told.  The guests must get the
> offsets (into this dedicated heap) from the host.

Hmm.  I'm wondering how it'll go manage dynamic gpu allocations at
all ...

cheers,
  Gerd