[virglrenderer-devel] multiprocess model and GL

[Chia-I Wu]

On Fri, Feb 14, 2020 at 6:23 PM Gurchetan Singh
<gurchetansingh at chromium.org> wrote:
>
> On Fri, Feb 14, 2020 at 3:20 PM Chia-I Wu <olvaffe at gmail.com> wrote:
> >
> > On Thu, Feb 13, 2020 at 7:43 PM Gurchetan Singh
> > <gurchetansingh at chromium.org> wrote:
> > >
> > > On Thu, Feb 13, 2020 at 1:40 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
> > > >
> > > >   Hi,
> > > >
> > > > > > > The kernel will tell virglrenderer the resource id.
> > > > > >
> > > > > > Which implies the resource_create_3d isn't an execbuffer but something
> > > > > > else.  It is not self-contained but depends on the given context,
> > > > > > specifically the resid generated by the kernel.  You can't do the same
> > > > > > via SUBMIT_3D.
> > > > >
> > > > > Typically, virgl_renderer_submit_cmd will take in
> > > > > VIRGL_CMD(ALLOCATE_WITH_OBJ_ID, size) since it needs to establish the
> > > > > [obj_id] --> [struct resource] mapping.
> > > > >
> > > > > virgl_renderer_resource_create_blob(resid, *resource_create_3d, ..)
> > > > > can take in both VIRGL_CMD(ALLOCATE_WITH_OBJ_ID, size) and
> > > > > VIRGL_CMD(ALLOCATE, size) [OpenGL/generic allocation contexts]).
> > > > > Internally, they'll go to the same allocation functions.  User-space
> > > > > will typically choose one or the other.
> > > > >
> > > > > Both VIRGL_CMD(ALLOCATE_WITH_OBJ_ID, size) and VIRGL_CMD(ALLOCATE,
> > > > > size) can be defined in the same header and same protocol namespace,
> > > > > if that's desired.  Does that address your concern about having two
> > > > > different protocols?
> > > >
> > > > No.  IMHO every execbuffer command should be submittable via
> > > > virgl_renderer_submit_cmd().  And ALLOCATE isn't, no matter in which
> > > > header file we define it, because processing it needs additional
> > > > information (the resid) which isn't in the execbuffer.
> > >
> > > Ah I see, the issue is both VIRGL_CMD(ALLOCATE, size) and
> > > VIRGL_CMD(GET_OBJ_ID, size) are only useful in the context of the
> > > kernel generated resource id.  Agreed there.
> > >
> > > I think it makes sense to ditch the detour to
> > > virgl_renderer_submit_cmd() for both allocation based and object id
> > > based flows?
> > >
> > > With the following virglrenderer API:
> > >
> > > virgl_renderer_resource_create_blob(resid, *void resource_create_3d, ..)
> > >
> > > Here's what I'm thinking:
> > >
> > > 1) One unified protocol per context for both resource creation and
> > > execbuffer.  Same protocol namespace, APIs.  This is different than
> > > the current status quo where resource creation is defined in
> > > virgl_hw.h and execbuffer is defined in virgl_protocol.h.
> > >
> > > 2) The capabilities define which commands are usable by
> > > virgl_renderer_resource_create_blob(resid, *void resource_create_3d,
> > > ..) and  virgl_renderer_submit_cmd(...).  A bitmask should do the
> > > trick.
> > >
> > > 3) VIRGL_CMD(ALLOCATE_WITH_OBJ_ID) will be usable by both,
> > > VIRGL_CMD(ALLOCATE, size) and VIRGL_CMD(GET_OBJ_ID, size) will only be
> > > usable by virgl_renderer_resource_create_blob(resid, *void
> > > resource_create_3d, ...).
> > >
> > > 4) All complex 3D operations commands will be reserved for
> > > virgl_renderer_submit_cmd(..).
> > >
> > > 5) Resource creation/management operations must be small (~ 64 bytes).
> > >
> > > 6) Only allow DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB with the
> > > predefined resource creation/management operations.
> > >
> > > I think this should satisfy all use cases, allow allocation during
> > > resource creation, avoid protocol collison, make sure
> > > virgl_renderer_submit_cmd(..) is self-contained, not require object
> > > ids for OpenGL/generic allocation contexts.  Style aside, is there any
> > > other concerns about this*?
> > >
> > > * for hostmem.  udmabuf TBD
> >
> > Using memory-v4 as the base, VIRTIO_GPU_RESOURCE_FLAG_ALLOC_EXECBUFFER
> > will continue to be supported.  You want to add
> > VIRTIO_GPU_RESOURCE_FLAG_ALLOC_ALLOCBUFFER that...
> >
> >  - supports the allocation subset of EXECBUFFER
> >  - supports other allocation commands that are not allowed in EXECBUFFER
> >  - is limited to 64 bytesOn the host side,
> >
> > I think we should pick either EXECBUFFER or ALLOCBUFFER, not providing
> > both.  And I think EXECBUFFER is enough.  (Note that there is the
> > option to add ALLOCBUFFER in the future in case I am wrong.)
>
> VIRTIO_GPU_RESOURCE_FLAG_ALLOC_EXECBUFFER can be merged into
> VIRTIO_GPU_RESOURCE_FLAG_3D_CREATE.
Exactly.  That was why I used the word "enough".

>
> virgl_renderer_resource_create_blob(resid, *void resource_create_3d,
> ...) can take in execbuffers if the protocol supports it.
>
> >
> > The concern over EXECBUFFER seems to be about requiring dummy object
> > ids to create resources.  How about switching virgl execbuffer to use
> > object ids?
>
> Ah, there's the issue.  Object IDs imply something -- that subsequent
> rendering commands will reference them.  Virgl contexts won't
> reference the object ID.  Generic allocation contexts don't need
> object IDs either.  So, they are only useful in the userspace that is
> designed around it.  Some userspace isn't.
virgl contexts do reference the allocations in the subsequent
rendering commands.  It is only that they use resids right now.  I
think generic allocation contexts usually need to reference their
allocations too, to get the metadata (stride, modifier, etc.) for
example.

It is true that ALLOC_EXECBUFFER encourages the use of object ids, in
the way that a context type using object ids won't need to generate
dummy object ids when creating resources.  virgl contexts can choose
to make the switch, or keep using resids (and generate dummy object
ids).  Either way is fine.  I suggested the switch because you seemed
to have concerns over dummy object ids.  But apparently you are not a
fan of making the switch either :|

I am not saying that object ids are the only way to do things.  There
are certainly other ways, and the kernel does allow them.  But I like
how the kernel has a bias toward object ids.

> >
> > There can be VIRGL_V1 and VIRGL_V2 context types.  Older guests with
> > lazy initialization will use VIRGL_V1 context type.  Newer guests can
> > choose between them, although there is no good reason to choose V1.
>
> I don't think there's much performance benefit in using object IDs for
> VIrgl V2.  Only compute and render targets potentially not getting
> assigned guest memory is the main theoretical benefit, right?  You can
> get that proposed benefit in other ways.  Is there other use cases you
> have in mind?
>
> >
> > VIRGL_V2 extends the virgl execbuffer with two commands
> >
> >  - a command to make object allocations and assign objids to them
> >  - a command to import resources as objects and assign objids to them
> >
> > But the key difference is that all existing commands will take objids
> > rather than resids.
>
> Philosophical difference -- if someone wants to do Virgl V2 and can
> prove the benefit, fine.  The API shouldn't force Virgl V2.
>
> >
> > >
> > >
> > > >
> > > > > > IMHO the workflow should be more like this:
> > > > > >
> > > > > >   (1) VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE
> > > > >
> > > > > Since resources will now be per context, doesn't
> > > > > VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB imply
> > > > > VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE for the creation context (if it's
> > > > > successful)?
> > > >
> > > > Yes (for EXECBUFFER allocations, DUMB resources are a different story of
> > > > course).
> > > >
> > > > > Does it make sense to only call
> > > > > VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE on importing into another context?
> > > >
> > > > Yes.
> > > >
> > > > cheers,
> > > >   Gerd
> > > >