[virglrenderer-devel] multiprocess model and GL

[Chia-I Wu]

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

The concern over EXECBUFFER seems to be about requiring dummy object
ids to create resources.  How about switching virgl execbuffer to use
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.

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.

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