Re: [PATCH 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh <gurchetansingh@chromium.org>]

On Tue, Apr 28, 2020 at 3:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

On Tue, Apr 21, 2020 at 05:55:30PM -0700, Gurchetan Singh wrote:
> Blob resources are size-based containers for host, guest, or
> host+guest allocations. These resources are designed with
> mulit-process 3D support in mind, but also usable in virtio-gpu 2d
> with system memory allocation (also known as dumb allocation, but
> since that's reserved in a DRM/KMS world, went with system).

Thanks for picking this up.

No problem, thanks for getting the ball rolling!

Â

> +Host allocations depend on whether VIRTIO_GPU_F_VIRGL is supported.
> +If VIRTIO_GPU_F_VIRGL is not supported, then:
> +
> +Â \begin{itemize*}
> +Â \item \field{blob_mem} MUST be VIRTIO_GPU_BLOB_MEM_HOSTSYS for host-only
> +Â blob resources
> +Â \item \field{blob_mem} MUST be VIRTIO_GPU_BLOB_MEM_HOSTSYS_GUEST for
> +Â default blob resources
> +Â \end{itemize*}
> +
> +For the above above cases, host system memory is allocated by the device.
> +
> +If VIRTIO_GPU_F_VIRGL is supported, then:
> +
> +Â \begin{itemize*}
> +Â \item \field{blob_mem} MUST be VIRTIO_GPU_BLOB_MEM_HOST3D for host-only
> +Â blob resources
> +Â \item \field{blob_mem} MUST be VIRTIO_GPU_BLOB_MEM_HOST3D_GUEST for
> +Â default resources
> +Â \end{itemize*}

Do we need separate items (HOSTSYS/HOST3D) for these? Shouldn't this be
a host implementation detail the guest doesn't need to worry about?

Yes, separate items are not needed and could indeed be an host implementation detail. Right now, the drm uapi has VIRTGPU_BLOB_MEM_HOST / VIRTIO_GPU_BLOB_MEM_HOST_GUEST and the kernel separates to HOST{SYS,3D} and does various checks.

https://gitlab.freedesktop.org/virgl/drm-misc-next/-/commit/0ccb47d5150a2978d179d0c44ef3285541c8964cÂ

The reasoning is since there's no RESOURCE_CREATE_BLOB2D orÂRESOURCE_CREATE_BLOB3D, the separation captures that difference in an explicit API level. That way, the blob id is only allowed with HOST3D.

But naming/enums is definitely mutable and open to suggestions if this reasoning doesn't seem right.

Also: Why HOST3D + HOST3D_GUEST? Isn't that covered by the use flags
(MAPPABLE) already?

A HOST3D blob has no guest memory associated with it, and can only be mapped through the shm region. Even then, a linear (mappable) view of a host3d blob may not be possible/desirable -- like shareable device local memory, or maybe even a protected content blob.

HOST3D_GUEST is always mappable, whileÂHOST3D isn't.ÂÂ

Â

> +If VIRTIO_GPU_F_VIRGL is set, both VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D
> +and VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D may be used to update the
> +resource. There is no restriction on the image/buffer view the driver
> +has on the blob resource.

I don't think VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D is going to work if
the resource has no format attached. I had a TRANSFER_BLOB command
because of that.

I was thinking about GL/VK interop here.

Say VK allocates the blob resource, but a GL texture is subsequently created from the blob resource using GL/VK interop extensions and a format is attached. For this reason, VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D is allowed on blob resources.

Similarly, virtgpu_kms always attaches XR24 and AB24 to resources[1], so VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D can be reused if size = 4 * width for dumb/sys blob resources (which I think will always be the case?).

TRANSFER_BLOB is definitely useful for things that can't fit neatly into the previous hypercalls (YUV formats in virtgpu_kms?), and could be useful for optimizing emulated coherent memory (if we everÂadd it). The current prototype doesn't really use TRANSFER_BLOB though, but if you have use cases in mind, it is definitely easy to add.ÂÂ

[1]Âhttps://github.com/torvalds/linux/blob/master/drivers/gpu/drm/virtio/virtgpu_plane.c#L33

Â

> +VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D, VIRTIO_GPU_CMD_SET_SCANOUT and
> +VIRTIO_GPU_CMD_RESOURCE_FLUSH MAY be used with blob resources as well,
> +subject to the following restrictions:

Same problem with VIRTIO_GPU_CMD_SET_SCANOUT, it expects the resource
has a format attached which isn't the case for blob resources ...Â

Ack. I definitely see the logic behindÂVIRTIO_GPU_CMD_SET_SCANOUT_BLOB, but current display integration (dpy_gl_scanout_texture(..) in QEMU) assumes the presence of a GL texture (which can be created from a blob resource). So we'll need to change the display integration to be Wayland based, and pass (offsets[4], strides[4], virtualized modifier?, a few other things) viaÂVIRTIO_GPU_CMD_SET_SCANOUT_BLOB. But I think that'll only work if only in a Zink + VK1.1-only world where GL never allocates.

That's a lot of changes, the current spec does what's needed for zero-copy VK/GL, and attempts to be forward looking for future changes. We may need additional feature bits for more advanced display integration.

Â

cheers,
 Gerd