Clarify the purpose of "supports_dmabuf" in CrossDomainCapabilities

[valpackett]

The supports_dmabuf flag in CrossDomainCapabilities is set when rutabaga is built with (and successfully initializes) a "Gralloc" implementation based on GBM or Vulkan.

However none of that is necessary to import dma-bufs from the virglrenderer context into cross-domain. libkrun does not build (mini)gbm nor vulkano in its rutabaga, and GPU rendering with virglrenderer-provided services (Venus/vDRM) and passing the buffers to the host Wayland compositor over a cross-domain proxy works great.

Recently the initial version of a new, simpler proxy (wl-cross-domain-proxy) was published.. with a check for supports_dmabuf, and that was filtering out zwp_linux_dmabuf_v1 for no good reason in libkrun guests. So evidently that flag causes some confusion.

Can the purpose of that flag and the entire gralloc system be clarified in the comments/etc.?

[gurchetansingh]

The gralloc system is used in certain Android guests: https://android.googlesource.com/platform/external/minigbm/+/refs/heads/main/virtgpu_cross_domain.c. Definitely need to improve it, most notably use upstream GBM rather than minigbm.

For Linux, the flag was meant for cases where somebody didn't want to use anything GPU strategy (SR-IOV, any of the virtio-gpu contexts) for rendering, but would like less copies for display integration. It's an improvement over wl-shm when llvmpipe is used for rendering. I guess it makes sense if you're super-concerned about exploits.

Will add a CL with comments, but likely after the holidays. Hope that helps with your proxy design.

[valpackett]

Thanks!

most notably use upstream GBM rather than minigbm

I'm pretty sure I've seen many rutabaga builds (e.g. distro packages) straight up override minigbm with real gbm already… Not sure if anyone tried Android guests with that :) or knew that it's unnecessary for upstream Linux guests.

would like less copies for display integration

I'm not sure how exactly this was done in CrOS… (I've seen the "not upstreamed" flag) but for upstream, I'm planning to eliminate the need for the wl-shm copy in the proxies by using udmabuf on both sides. That would require at least a tiny kernel change as the import of dmabufs into virtio-gpu was conditioned on 3D being disabled, but we need that condition gone. But I have a proof-of-concept of rutabaga accessing a guest memfd and printing its contents. Might also need a patch for some extra indication that it's cross-device (?) as the PoC overrides the default_component to make the import happen in rutabaga and not in virglrenderer..

[zzyiwei]

I'm pretty sure I've seen many rutabaga builds (e.g. distro packages) straight up override minigbm with real gbm already… Not sure if anyone tried Android guests with that :) or knew that it's unnecessary for upstream Linux guests.

Upstream gbm works perfectly fine with virgl + venus via crosvm. I'd also vote for cleaning that up. iirc, for the related bits here, there're 2 minigbm specific thigns missed from upstream gbm:

1. The minigbm helper lib that picks the intended device for alloc
   • that's useful when vkms or cross-ip/multi-drm nodes are in the play
   • workaround can be copying the entire helper into rutabaga
2. The bo mapping cache mode query
   • this could sometimes be essential for host3d blob mem (could affect both venus and native context)
   • context is in this CL chain: https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/3777567
   • workaround can be ugly...

I'd say we can do (1) so to avoid hard-requiring minigbm, and use (2) if minigbm otherwise fallback to write-combine for the most correctness to play safe.

[valpackett]

Thanks for the info! (oops this probably should've been split off into a different issue but oh well)

mapping cache mode query

Looks like in libkrun we just have the "simplistic" is-i915 logic hardcoded into minigbm.rs get_image_memory_requirements() (and we just link to system gbm):

        // Intel GPUs typically only use cached memory buffers.  This will change with dGPUs, but
        // perhaps minigbm will be deprecated by then.  Other display drivers (rockchip, mediatek,
        // amdgpu) typically use write combine memory.  We can also consider use flags too if this
        // heuristic proves insufficient.
        if self.device_name == "i915" {
            reqs.map_info = RUTABAGA_MAP_CACHE_CACHED;
        } else {
            reqs.map_info = RUTABAGA_MAP_CACHE_WC;
        }

Works fine across i915, amdgpu, msm and asahi, but I haven't tested xe yet.

The minigbm helper lib that picks the intended device for alloc

Is that it (rendernode)?

impl MinigbmDevice {
    /// Returns a new `MinigbmDevice` if there is a rendernode in `/dev/dri/` that is accepted by
    /// the minigbm library.
    pub fn init() -> RutabagaResult<Box<dyn Gralloc>> {
        let undesired: &[&str] = &["vgem", "pvr"];
        let fd = rendernode::open_device(undesired)?;

        // gbm_create_device is safe to call with a valid fd, and we check that a valid one is
        // returned.  If the fd does not refer to a DRM device, gbm_create_device will reject it.
        let gbm = unsafe { gbm_create_device(fd.as_raw_descriptor()) };
        if gbm.is_null() {
            return Err(RutabagaError::IoError(Error::last_os_error()));
        }

        // Safe because a valid minigbm device has a statically allocated string associated with
        // it, which is valid for the lifetime of the process.
        let backend_name: *const c_char = unsafe { gbm_device_get_backend_name(gbm) };
        let c_str: &CStr = unsafe { CStr::from_ptr(backend_name) };
        let device_name: &str = c_str.to_str()?;

        Ok(Box::new(MinigbmDevice {
            minigbm_device: Arc::new(MinigbmDeviceInner { _fd: fd, gbm }),
            last_buffer: None,
            device_name,
        }))
    }
}

[valpackett]

So, coming back to this gbm issue (in order to test the libkrun migration to upstream rutabaga I needed to get it working at least as a hack first)…

One interesting thing wrt. picking the intended device, since e156297 there is RUTABAGA_PATH_TYPE_GPU so the consumer can actually explicitly pick a device for virglrenderer. That decision should probably be respected for gbm alloc as well.

Also what's annoying is that the rendernode::open_device helper thing is mostly based on get_drm_device_name which is already present in this repo but not easily usable since it's under mesa3d/magma! (Well I guess we can skip the whole "undesired" filtering though…(?))

[gurchetansingh]

@valpackett Yeah, if you could remove minigbm and use gbm exclusively, that would be terrific. Recommended steps:

1. roll-back https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5038006, which we can do now due to the ChromeOS maintenance branch. We can add minigbm-specific complexity (under a flag) in later, but most seem interested in upstream GBM anyways.

2. Move the rendernodes logic out of Magma, and into mesa3d_util in something called GpuDevice or GpuNode (or whatever name suits your fancy). I wouldn't move the d3dkmt logic out of it there, but GpuDevice should be good for Linux/Fuchsia "rendernode"-like access. Bonus points for using Rustix for android, part deux bytecodealliance/rustix#1577, since that'll allow use of "official" rustix ioctl wrappers and less hacky workarounds.

3. Use it to init GBM.

Hope that helps!