Re: [QEMU PATCH v4 10/13] virtio-gpu: Resource UUID

[Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>]

On 2023/09/13 23:18, Albert Esteve wrote:
> On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx 
> <mailto:akihiko.odaki@xxxxxxxxxx>> wrote:
>     On 2023/09/13 21:58, Albert Esteve wrote:
>      >
>      >
>      > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki
>     <akihiko.odaki@xxxxxxxxxx <mailto:akihiko.odaki@xxxxxxxxxx>
>      > <mailto:akihiko.odaki@xxxxxxxxxx
>     <mailto:akihiko.odaki@xxxxxxxxxx>>> wrote:
>      >
>      >     On 2023/09/13 20:34, Albert Esteve wrote:
>      >      >
>      >      >
>      >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
>      >     <akihiko.odaki@xxxxxxxxxx <mailto:akihiko.odaki@xxxxxxxxxx>
>     <mailto:akihiko.odaki@xxxxxxxxxx <mailto:akihiko.odaki@xxxxxxxxxx>>
>      >      > <mailto:akihiko.odaki@xxxxxxxxxx
>     <mailto:akihiko.odaki@xxxxxxxxxx>
>      >     <mailto:akihiko.odaki@xxxxxxxxxx
>     <mailto:akihiko.odaki@xxxxxxxxxx>>>> wrote:
>      >      >
>      >      >     On 2023/09/13 16:55, Albert Esteve wrote:
>      >      >      > Hi Antonio,
>      >      >      >
>      >      >      > If I'm not mistaken, this patch is related with:
>      >      >      >
>      >      >
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>      >      >      >
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>
>      >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both,
>     would
>      >     use the
>      >      >      > infrastructure from the patch I linked to store the
>      >      >      > virtio objects, so that they can be later shared with
>      >     other devices.
>      >      >
>      >      >     I don't think such sharing is possible because the
>     resources are
>      >      >     identified by IDs that are local to the device. That also
>      >     complicates
>      >      >     migration.
>      >      >
>      >      >     Regards,
>      >      >     Akihiko Odaki
>      >      >
>      >      > Hi Akihiko,
>      >      >
>      >      > As far as I understand, the feature to export
>     dma-bufs from the
>      >      > virtgpu was introduced as part of the virtio cross-device
>     sharing
>      >      > proposal [1]. Thus, it shall be posible. When
>     virtgpu ASSING_UUID,
>      >      > it exports and identifies the dmabuf resource, so that
>     when the
>      >     dmabuf gets
>      >      > shared inside the guest (e.g., with virtio-video), we can
>     use the
>      >     assigned
>      >      > UUID to find the dmabuf in the host (using the patch that I
>      >     linked above),
>      >      > and import it.
>      >      >
>      >      > [1] - https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>
>     <https://lwn.net/Articles/828988/ <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>>
>      >
>      >     The problem is that virtio-gpu can have other kind of
>     resources like
>      >     pixman and OpenGL textures and manage them and DMA-BUFs with
>     unified
>      >     resource ID.
>      >
>      >
>      > I see.
>      >
>      >
>      >     So you cannot change:
>      >     g_hash_table_insert(g->resource_uuids,
>      >     GUINT_TO_POINTER(assign.resource_id), uuid);
>      >     by:
>      >     virtio_add_dmabuf(uuid, assign.resource_id);
>      >
>      >     assign.resource_id is not DMA-BUF file descriptor, and the
>     underlying
>      >     resource my not be DMA-BUF at first place.
>      >
>      >
>      > I didn't really look into the patch in-depth, so the code was
>     intended
>      > to give an idea of how the implementation would look like with
>      > the cross-device patch API. Indeed, it is not the resource_id,
>      > (I just took a brief look at the virtio specificacion 1.2), but the
>      > underlying
>      > resource what we want to use here.
>      >
>      >
>      >     Also, since this lives in the common code that is not used
>     only by
>      >     virtio-gpu-gl but also virtio-gpu, which supports migration,
>     we also
>      >     need to take care of that. It is not a problem for DMA-BUF as
>      >     DMA-BUF is
>      >     not migratable anyway, but the situation is different in this
>     case.
>      >
>      >     Implementing cross-device sharing is certainly a possibility,
>     but that
>      >     requires more than dealing with DMA-BUFs.
>      >
>      >
>      > So, if I understood correctly, dmabufs are just a subset of the
>     resources
>      > that the gpu manages, or can assign UUIDs to. I am not sure why
>      > the virt gpu driver would want to send a ASSIGN_UUID for anything
>      > that is not a dmabuf (are we sure it does?), but I am not super
>     familiarized
>      > with virtgpu to begin with.
>
>     In my understanding, an resource will be first created as OpenGL or
>     Vulkan textures and then exported as a DMA-BUF file descriptor. For
>     these resource types exporting/importing code is mandatory.
>
>     For pixman buffers (i.e., non-virgl device), I don't see a compelling
>     reason to have cross-device sharing. It is possible to omit resource
>     UUID feature from non-virgl device to avoid implementing complicated
>     migration.
>
>
> I see, thanks for the clarification.
> I would assume you could avoid the UUID feature for those resources, but
> I will need to check the driver implementation. It is worth checking 
> though, if
> that would simplify the implementation.
>
>
>      > But I see that internally, the GPU specs relate a UUID with a
>     resource_id,
>      > so we still need both tables:
>      > - one to relate UUID with resource_id to be able to locate the
>      > underlying resource
>      > - the table that holds the dmabuf with the UUID for cross-device
>     sharing
>      >
>      > With that in mind, sounds to me that the support for cross-device
>      > sharing could
>      > be added on top of this patch, once
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
>      >
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>
>      > lands.
>
>     That is possible, but I think it's better to implement cross-device
>     sharing at the same time introducing virtio-dmabuf.
>
>     The current design of virtio-dmabuf looks somewhat inconsistent; it's
>     named "dmabuf", but internally the UUIDs are stored into something
>     named
>     "resource_uuids" and it has SharedObjectType so it's more like a
>     generic
>     resource sharing mechanism. If you actually have an implementation of
>     cross-device sharing using virtio-dmabuf, it will be clear what kind of
>     feature is truly necessary.
>
>
> Yeah, the file was named as virtio-dmabuf following the kernel
> implementation. Also, because for the moment it only aims to share
> dmabufs. However, virtio specs leave the virtio object defintion vague [1]
> (I guess purposely). It is up to the specific devices to define what an 
> object
> means for them. So the implementation tries to follow that, and
> leave the contents of the table generic. The table can hold any kind of 
> object,
> and the API exposes type-specific functions (for dmabufs, or others).
In the guest kernel, the name "virtio_dma_buf" represents the interface 
between the *guest* kernel and *guest* user-space. It makes sense since 
the cross-device resource sharing is managed by the userspace and 
DMA-BUF is the only interface between them for this purpose.
The situation is different for QEMU; QEMU interacts with backends using 
backend-specific interfaces (OpenGL/pixman) and virgl is capable to 
export textures as DMA-BUF. DMA-BUF is not universal in this sense. As 
such, we cannot just borrow the kernel-side naming but invent one.