On Thu, Sep 14, 2023 at 6:56 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
> On 2023/09/14 17:29, Albert Esteve wrote: > > > > > > On Thu, Sep 14, 2023 at 9:17 AM Akihiko Odaki <akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>> wrote: > > > > On 2023/09/13 23:18, Albert Esteve wrote: > > > > > > > > > On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki > > <akihiko.od...@daynix.com <mailto:akihiko.od...@daynix.com> > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>>> wrote: > > > > > > On 2023/09/13 21:58, Albert Esteve wrote: > > > > > > > > > > > > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki > > > <akihiko.od...@daynix.com <mailto:akihiko.od...@daynix.com> > > <mailto:akihiko.od...@daynix.com <mailto:akihiko.od...@daynix.com>> > > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com> > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>>>> wrote: > > > > > > > > On 2023/09/13 20:34, Albert Esteve wrote: > > > > > > > > > > > > > > > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki > > > > <akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com> <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>> > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com> <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>>> > > > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com> > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>> > > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com> > > > <mailto:akihiko.od...@daynix.com > > <mailto:akihiko.od...@daynix.com>>>>> 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>>>> > > > > > > > > > > > > > > > > > > > > < > 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/>>> > > > <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>> > > > > > > > > > < > 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. > > > > It is not a gpu-specific feature. It is a generic cross-device sharing > > mechanism for virtio objects. In this case, virtio objects happen to be > > dmabufs in this first iteration. Hence, the name. > > > > virtio-gpu (and vhost-user-gpu) will use this feature only with virgl, > > that is > > fine, and transversal to the object-sharing mechanism. It allows > > to share dmabufs in the host following how they are shared in the guest. > > The virtgpu driver may call ASSIGN_UUID for other types of resources > > (not sure, > > but could be), but they will never be shared with other virtio devices. > > So they are not too relevant. Also, the shared objects table could > > potentially > > be accessed from any virtio device, not only virtio-gpu or virtio-video. > > The virtgpu driver will call ASSIGN_UUID for resources that are backed > with pixman buffer. What is used as the backing store for resources is > an implementation detail of VMM and the guest cannot know it. For the > guest, they are same kind of resources (2D images). > > Ok, got it. In any case, those resources that are actually backed by a dmabuf ought to be shared using virtio_dmabuf. I think that is the key point of this discussion. That support can be added once both this patch and the virtio_dmabuf land.
