On Thu, May 30, 2024 at 12:21 AM Kasireddy, Vivek <vivek.kasire...@intel.com> wrote:
> Hi Gurchetan, > > > > > On Fri, May 24, 2024 at 11:33 AM Kasireddy, Vivek > > <vivek.kasire...@intel.com <mailto:vivek.kasire...@intel.com> > wrote: > > > > > > Hi, > > > > Sorry, my previous reply got messed up as a result of HTML > > formatting. This is > > a plain text version of the same reply. > > > > > > > > > > > Having virtio-gpu import scanout buffers (via prime) from > other > > > devices means that we'd be adding a head to headless GPUs > > assigned > > > to a Guest VM or additional heads to regular GPU devices > that > > are > > > passthrough'd to the Guest. In these cases, the Guest > > compositor > > > can render into the scanout buffer using a primary GPU and > has > > the > > > secondary GPU (virtio-gpu) import it for display purposes. > > > > > > The main advantage with this is that the imported scanout > > buffer can > > > either be displayed locally on the Host (e.g, using Qemu + > GTK > > UI) > > > or encoded and streamed to a remote client (e.g, Qemu + > Spice > > UI). > > > Note that since Qemu uses udmabuf driver, there would be no > > > copies > > > made of the scanout buffer as it is displayed. This should > be > > > possible even when it might reside in device memory such > has > > > VRAM. > > > > > > The specific use-case that can be supported with this > series is > > when > > > running Weston or other guest compositors with "additional- > > devices" > > > feature (./weston --drm-device=card1 --additional- > > devices=card0). > > > More info about this feature can be found at: > > > https://gitlab.freedesktop.org/wayland/weston/- > > > /merge_requests/736 > > > > > > In the above scenario, card1 could be a dGPU or an iGPU and > > card0 > > > would be virtio-gpu in KMS only mode. However, the case > > where this > > > patch series could be particularly useful is when card1 is > a GPU > > VF > > > that needs to share its scanout buffer (in a zero-copy > way) with > > the > > > GPU PF on the Host. Or, it can also be useful when the > scanout > > buffer > > > needs to be shared between any two GPU devices (assuming > > one of > > > them > > > is assigned to a Guest VM) as long as they are P2P DMA > > compatible. > > > > > > > > > > > > Is passthrough iGPU-only or passthrough dGPU-only something you > > intend to > > > use? > > Our main use-case involves passthrough’g a headless dGPU VF device > > and sharing > > the Guest compositor’s scanout buffer with dGPU PF device on the > > Host. Same goal for > > headless iGPU VF to iGPU PF device as well. > > > > > > > > Just to check my understanding: the same physical {i, d}GPU is > partitioned > > into the VF and PF, but the PF handles host-side display integration and > > rendering? > Yes, that is mostly right. In a nutshell, the same physical GPU is > partitioned > into one PF device and multiple VF devices. Only the PF device has access > to > the display hardware and can do KMS (on the Host). The VF devices are > headless with no access to display hardware (cannot do KMS but can do > render/ > encode/decode) and are generally assigned (or passthrough'd) to the Guest > VMs. > Some more details about this model can be found here: > > https://lore.kernel.org/dri-devel/20231110182231.1730-1-michal.wajdec...@intel.com/ > > > > > > > However, using a combination of iGPU and dGPU where either of > > them can be passthrough’d > > to the Guest is something I think can be supported with this patch > > series as well. > > > > > > > > If it's a dGPU + iGPU setup, then the way other people seem to > do it > > is a > > > "virtualized" iGPU (via virgl/gfxstream/take your pick) and pass- > > through the > > > dGPU. > > > > > > For example, AMD seems to use virgl to allocate and import into > > the dGPU. > > > > > > https://gitlab.freedesktop.org/mesa/mesa/- > > /merge_requests/23896 > > > > > > https://lore.kernel.org/all/20231221100016.4022353-1- > > > julia.zh...@amd.com/ <http://julia.zh...@amd.com/> > > > > > > > > > ChromeOS also uses that method (see crrev.com/c/3764931 > > <http://crrev.com/c/3764931> > > > <http://crrev.com/c/3764931> ) [cc: dGPU architect +Dominik Behr > > > <mailto:db...@google.com <mailto:db...@google.com> > ] > > > > > > So if iGPU + dGPU is the primary use case, you should be able to > > use these > > > methods as well. The model would "virtualized iGPU" + > > passthrough dGPU, > > > not split SoCs. > > In our use-case, the goal is to have only one primary GPU > > (passthrough’d iGPU/dGPU) > > do all the rendering (using native DRI drivers) for > clients/compositor > > and all the outputs > > and share the scanout buffers with the secondary GPU (virtio-gpu). > > Since this is mostly > > how Mutter (and also Weston) work in a multi-GPU setup, I am not > > sure if virgl is needed. > > > > > > > > I think you can probably use virgl with the PF and others probably will, > but > > supporting multiple methods in Linux is not unheard of. > In our case, we have an alternative SR-IOV based GPU > virtualization/partitioning > model (as described above) where a Guest VM will have access to a > hardware-accelerated > GPU VF device for its rendering/encode/decode needs. So, in this > situation, using > virgl will become redundant and unnecessary. > > And, in this model, we intend to use virtio-gpu for KMS in the Guest VM > (since the > GPU VF device cannot do KMS) with the addition of this patchset. However, > note that, > since not all GPU SKUs/versions have the SRIOV capability, we plan on > using virgl in > those cases where it becomes necessary. > > > > > Does your patchset need the Mesa kmsro patchset to function correctly? > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592 > This patchset is an alternative proposal. So, KMSRO would not be needed. > AFAICS, the above MR is mainly stalled because KMSRO uses dumb buffers > which are not suitable for hardware-based rendering in all cases. And, > KMSRO > is not really helpful performance-wise with dGPUs, as it forces most > buffers to > be allocated from system memory. > Previously, it was recommended when exploring VDMABUF: "the /important/ thing is that the driver which exports the dma-buf (and thus handles the mappings) must be aware of the virtualization so it can properly coordinate things with the host side." https://patchwork.kernel.org/project/linux-media/patch/20210203073517.1908882-3-vivek.kasire...@intel.com/#23975915 So that's why the KMSRO approach was tried (virtio-gpu dumb allocations, not i915). But as you point out, nobody uses dumb buffers for hardware-based rendering. So, if you are going with i915 allocates + virtio-gpu imports, it should be fine if you fixed all the issues with i915 allocates + VDMABUF imports. It seems your fixes add complexity in VFIO and other places, but having virtio-gpu 3d + virgl allocate adds complexity to Mesa-based allocation paths (i915, amdgpu would all have to open virtio-gpu render node, and pick a context type etc.). I would just do virtio-gpu allocates, since it only requires user-space patches and no extra ioctls, but that reflects my preferences. If the mm/VFIO/QEMU people are fine with your approach, I see nothing wrong with merging it. The one caveat is if someone uses a non-GTK/EGL host path, we'll have to pin memory for the lifetime of the import, since knowing RESOURCE_FLUSH is done is not sufficient. But if you're only using it, it shouldn't be an issue right now. > > > > > > > If so, I would try to get that reviewed first to meet DRM requirements > > (https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source- > > userspace-requirements). You might explicitly call out the design > decision > > you're making: ("We can probably use virgl as the virtualized iGPU via > PF, but > > that adds unnecessary complexity b/c ______"). > As I described above, what we have is an alternative GPU virtualization > scheme > where virgl is not necessary if SRIOV capability is available. And, as > mentioned > earlier, I have tested this series with Mutter/Gnome-shell (upstream > master) > (plus one small patch: > https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3745) > and no other changes to any other userspace components on Host and Guest. > > > > > > > And, doing it this way means that no other userspace components > > need to be modified > > on both the Guest and the Host. > > > > > > > > > > > > > > As part of the import, the virtio-gpu driver shares the dma > > > addresses and lengths with Qemu which then determines > > whether > > > the > > > memory region they belong to is owned by a PCI device or > > whether it > > > is part of the Guest's system ram. If it is the former, it > identifies > > > the devid (or bdf) and bar and provides this info (along > with > > offsets > > > and sizes) to the udmabuf driver. In the latter case, > instead of > > the > > > the devid and bar it provides the memfd. The udmabuf driver > > then > > > creates a dmabuf using this info that Qemu shares with > Spice > > for > > > encode via Gstreamer. > > > > > > Note that the virtio-gpu driver registers a move_notify() > callback > > > to track location changes associated with the scanout > buffer and > > > sends attach/detach backing cmds to Qemu when appropriate. > > And, > > > synchronization (that is, ensuring that Guest and Host are > not > > > using the scanout buffer at the same time) is ensured by > > pinning/ > > > unpinning the dmabuf as part of plane update and using a > fence > > > in resource_flush cmd. > > > > > > > > > I'm not sure how QEMU's display paths work, but with crosvm if > > you share > > > the guest-created dmabuf with the display, and the guest moves > > the backing > > > pages, the only recourse is the destroy the surface and show a > > black screen > > > to the user: not the best thing experience wise. > > Since Qemu GTK UI uses EGL, there is a blit done from the guest’s > > scanout buffer onto an EGL > > backed buffer on the Host. So, this problem would not happen as of > > now. > > > > > > > > The guest kernel doesn't know you're using the QEMU GTK UI + EGL host- > > side. > So, with blob=true, there is a dma fence in resource_flush() that gets > associated > with the Blit/Encode on the Host. This guest dma fence should eventually > be signalled > only when the Host is done using guest's scanout buffer. > > > > > If somebody wants to use the virtio-gpu import mechanism with lower-level > > Wayland-based display integration, then the problem would occur. > Right, one way to address this issue is to prevent the Guest compositor > from > reusing the scanout buffer (until the Host is done) and forcing it to pick > a new > buffer (since Mesa GBM allows 4 backbuffers). > I have tried this experiment with KMSRO and Wayland-based Qemu UI > previously > on iGPUs (and Weston) and noticed that the Guest FPS was getting halved: > > https://lore.kernel.org/qemu-devel/20210913222036.3193732-1-vivek.kasire...@intel.com/ > > and also discussed and proposed a solution which did not go anywhere: > > https://lore.kernel.org/dri-devel/20210913233529.3194401-1-vivek.kasire...@intel.com/ > > > > > Perhaps, do that just to be safe unless you have performance concerns. > If you meant pinning the imported scanout buffer in the Guest, then yes, > that is something I am already doing in this patchset. > > > > > > > > > > > Only amdgpu calls dma_buf_move_notfiy(..), and you're probably > > testing on > > > Intel only, so you may not be hitting that code path anyways. > > I have tested with the Xe driver in the Guest which also calls > > dma_buf_move_notfiy(). But > > note that for dGPUs, both Xe and amdgpu migrate the scanout buffer > > from vram to system > > memory as part of export, because virtio-gpu is not P2P compatible. > > However, I am hoping > > to relax this (p2p check against virtio-gpu) in Xe driver if it > detects > > that it is running in > > VF mode once the following patch series is merged: > > https://lore.kernel.org/dri-devel/20240422063602.3690124-1- > > vivek.kasire...@intel.com/ > > > > > I forgot the > > > exact reason, but apparently udmabuf may not work with amdgpu > > displays > > > and it seems the virtualized iGPU + dGPU is the way to go for > > amdgpu > > > anyways. > > I would really like to know why udmabuf would not work with > > amdgpu? > > > > > > > > It's just a rumor I heard, but the idea is udmabuf would be imported into > > AMDGPU_GEM_DOMAIN_CPU only. > > > > https://cgit.freedesktop.org/drm/drm- > > misc/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c#n333 > > > > "AMDGPU_GEM_DOMAIN_CPU: System memory that is not GPU accessible. > > Memory in this pool could be swapped out to disk if there is pressure." > > > > https://dri.freedesktop.org/docs/drm/gpu/amdgpu.html > > > > > > Perhaps that limitation is artificial and unnecessary, and it may indeed > work. > > I don't think anybody has tried... > Since udmabuf driver properly pins the backing pages (from memfd) for DMA, > I don't see any reason why amdgpu would not be able to import. > > Thanks, > Vivek > > > > > > > > > > > > So I recommend just pinning the buffer for the lifetime of the > > > import for simplicity and correctness. > > Yeah, in this patch series, the dmabuf is indeed pinned, but only > for a > > short duration in the Guest – > > just until the Host is done using it (blit or encode). > > > > Thanks, > > Vivek > > > > > > > > > > > This series is available at: > > > https://gitlab.freedesktop.org/Vivek/drm-tip/- > > > /commits/virtgpu_import_rfc > > > > > > along with additional patches for Qemu and Spice here: > > > https://gitlab.freedesktop.org/Vivek/qemu/- > > > /commits/virtgpu_dmabuf_pcidev > > > https://gitlab.freedesktop.org/Vivek/spice/- > > > /commits/encode_dmabuf_v4 > > > > > > Patchset overview: > > > > > > Patch 1: Implement > > > VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING cmd > > > Patch 2-3: Helpers to initalize, import, free imported > object > > > Patch 4-5: Import and use buffers from other devices for > > scanout > > > Patch 6-7: Have udmabuf driver create dmabuf from PCI bars > > for P2P > > > DMA > > > > > > This series is tested using the following method: > > > - Run Qemu with the following relevant options: > > > qemu-system-x86_64 -m 4096m .... > > > -device vfio-pci,host=0000:03:00.0 > > > -device virtio- > > vga,max_outputs=1,blob=true,xres=1920,yres=1080 > > > -spice port=3001,gl=on,disable-ticketing=on,preferred- > > > codec=gstreamer:h264 > > > -object memory-backend-memfd,id=mem1,size=4096M > > > -machine memory-backend=mem1 ... > > > - Run upstream Weston with the following options in the > Guest > > VM: > > > ./weston --drm-device=card1 --additional-devices=card0 > > > > > > where card1 is a DG2 dGPU (passthrough'd and using xe > driver > > in > > > Guest VM), > > > card0 is virtio-gpu and the Host is using a RPL iGPU. > > > > > > Cc: Gerd Hoffmann <kra...@redhat.com > > <mailto:kra...@redhat.com> > > > <mailto:kra...@redhat.com <mailto:kra...@redhat.com> > > > > > Cc: Dongwon Kim <dongwon....@intel.com > > <mailto:dongwon....@intel.com> > > > <mailto:dongwon....@intel.com > > <mailto:dongwon....@intel.com> > > > > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch > > <mailto:daniel.vet...@ffwll.ch> > > > <mailto:daniel.vet...@ffwll.ch <mailto:daniel.vet...@ffwll.ch> > > > > > > Cc: Christian Koenig <christian.koe...@amd.com > > <mailto:christian.koe...@amd.com> > > > <mailto:christian.koe...@amd.com > > <mailto:christian.koe...@amd.com> > > > > > Cc: Dmitry Osipenko <dmitry.osipe...@collabora.com > > <mailto:dmitry.osipe...@collabora.com> > > > <mailto:dmitry.osipe...@collabora.com > > <mailto:dmitry.osipe...@collabora.com> > > > > > Cc: Rob Clark <robdcl...@chromium.org > > <mailto:robdcl...@chromium.org> > > > <mailto:robdcl...@chromium.org > > <mailto:robdcl...@chromium.org> > > > > > Cc: Thomas Hellström <thomas.hellst...@linux.intel.com > > <mailto:thomas.hellst...@linux.intel.com> > > > <mailto:thomas.hellst...@linux.intel.com > > <mailto:thomas.hellst...@linux.intel.com> > > > > > Cc: Oded Gabbay <ogab...@kernel.org > > <mailto:ogab...@kernel.org> > > > <mailto:ogab...@kernel.org <mailto:ogab...@kernel.org> > > > > > Cc: Michal Wajdeczko <michal.wajdec...@intel.com > > <mailto:michal.wajdec...@intel.com> > > > <mailto:michal.wajdec...@intel.com > > <mailto:michal.wajdec...@intel.com> > > > > > Cc: Michael Tretter <m.tret...@pengutronix.de > > <mailto:m.tret...@pengutronix.de> > > > <mailto:m.tret...@pengutronix.de > > <mailto:m.tret...@pengutronix.de> > > > > > > > > Vivek Kasireddy (7): > > > drm/virtio: Implement > > > VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING cmd > > > drm/virtio: Add a helper to map and note the dma addrs > and > > > lengths > > > drm/virtio: Add helpers to initialize and free the > imported > > object > > > drm/virtio: Import prime buffers from other devices as > guest > > blobs > > > drm/virtio: Ensure that bo's backing store is valid while > > updating > > > plane > > > udmabuf/uapi: Add new ioctl to create a dmabuf from PCI > bar > > > regions > > > udmabuf: Implement UDMABUF_CREATE_LIST_FOR_PCIDEV > > ioctl > > > > > > drivers/dma-buf/udmabuf.c | 122 > ++++++++++++++++-- > > > drivers/gpu/drm/virtio/virtgpu_drv.h | 8 ++ > > > drivers/gpu/drm/virtio/virtgpu_plane.c | 56 ++++++++- > > > drivers/gpu/drm/virtio/virtgpu_prime.c | 167 > > > ++++++++++++++++++++++++- > > > drivers/gpu/drm/virtio/virtgpu_vq.c | 15 +++ > > > include/uapi/linux/udmabuf.h | 11 +- > > > 6 files changed, 368 insertions(+), 11 deletions(-) > > > > > > -- > > > 2.43.0 > > > > > > > > > > > >
