Pin encode device to a DRM render node - #23
Conversation
Select the Vulkan device whose VK_EXT_physical_device_drm render major:minor matches a caller-supplied node, so the encoder shares the compositor physical GPU. vendor:device ids cannot disambiguate identical GPUs, and importing a DMA-BUF across GPUs corrupts frames. Adds VideoContextBuilder::drm_render_node and a verify_drm_pin example.
hgaiser
left a comment
There was a problem hiding this comment.
I like the idea, but I have some thoughts on how to improve it further. Thanks for the input!
| // Resolve the preferred DRM render node to (major, minor). Used to pin | ||
| // the encoder to the same physical GPU as the compositor, since importing | ||
| // a DMA-BUF allocated on one GPU into an encoder on another corrupts the | ||
| // image and vendor:device filtering can't disambiguate identical GPUs. | ||
| let preferred_drm = builder.preferred_drm_render_node.as_deref().and_then(|path| { | ||
| match drm_render_major_minor(path) { | ||
| Some(mm) => { | ||
| info!( | ||
| "Encoder will prefer the GPU backing {} (drm {}:{})", | ||
| path.display(), | ||
| mm.0, | ||
| mm.1 | ||
| ); | ||
| Some(mm) | ||
| }, | ||
| None => { | ||
| warn!( | ||
| "Could not resolve DRM major/minor for {}; encoder will pick the first suitable GPU", | ||
| path.display() | ||
| ); | ||
| None | ||
| }, | ||
| } | ||
| }); |
There was a problem hiding this comment.
I would prefer to return an error when the requested render node could not be used. It seems counterintuitive to continue with only a warning logged. If the caller wanted to try again with "any" render node, it could just create a new builder without a given render node.
This would also mean dropping the preferred in the variable naming.
Small nitpick: you mention compositor a few times, but technically the source can be anything. A video player, a game, etc.
| drm_render: Option<(i64, i64)>, | ||
| } | ||
| let mut candidates: Vec<DeviceCandidate> = Vec::new(); |
There was a problem hiding this comment.
Given that the provided drm render node becomes a hard requirement, the rest of the file changes significantly because we don't need to collect all candidates.
| /// GPU into an encoder on another corrupts the image, and identical GPUs | ||
| /// can't be disambiguated by vendor:device id. Falls back to the first | ||
| /// suitable device when unset or unmatched. No-op on non-Linux. | ||
| pub fn drm_render_node(mut self, node: Option<std::path::PathBuf>) -> Self { |
There was a problem hiding this comment.
Why make it an Option ? The user can simply not call it when it's None, right?
| use ash::vk::TaggedStructure; | ||
| use pixelforge::VideoContextBuilder; | ||
| use std::ffi::CStr; | ||
| use std::os::unix::fs::MetadataExt; |
There was a problem hiding this comment.
This won't compile on macos/windows.
| #[cfg(not(target_os = "linux"))] | ||
| fn drm_render_major_minor(_path: &std::path::Path) -> Option<(i64, i64)> { | ||
| None | ||
| } |
There was a problem hiding this comment.
I don't know if it makes sense to implement this function on non-linux systems. Might be better to just gate the drm_render_major_minor (as is already done) and its usage.
There was a problem hiding this comment.
I understand the need to test the new feature, but I don't think this example makes sense to keep in pixelforge.
| /// Prefer the physical device backing the given DRM render node | ||
| /// (e.g. `/dev/dri/renderD128`). | ||
| /// | ||
| /// Device selection picks the encode-capable Vulkan device whose DRM | ||
| /// render major/minor (via `VK_EXT_physical_device_drm`) matches this node, | ||
| /// so the encoder runs on the same physical GPU as the compositor that | ||
| /// produced the DMA-BUFs. Without this, importing a buffer allocated on one | ||
| /// GPU into an encoder on another corrupts the image, and identical GPUs | ||
| /// can't be disambiguated by vendor:device id. Falls back to the first | ||
| /// suitable device when unset or unmatched. No-op on non-Linux. |
There was a problem hiding this comment.
| /// Prefer the physical device backing the given DRM render node | |
| /// (e.g. `/dev/dri/renderD128`). | |
| /// | |
| /// Device selection picks the encode-capable Vulkan device whose DRM | |
| /// render major/minor (via `VK_EXT_physical_device_drm`) matches this node, | |
| /// so the encoder runs on the same physical GPU as the compositor that | |
| /// produced the DMA-BUFs. Without this, importing a buffer allocated on one | |
| /// GPU into an encoder on another corrupts the image, and identical GPUs | |
| /// can't be disambiguated by vendor:device id. Falls back to the first | |
| /// suitable device when unset or unmatched. No-op on non-Linux. | |
| /// Pin the encoder to the GPU identified by a DRM render node (e.g. `/dev/dri/renderD128`). | |
| /// | |
| /// Only available on Linux. |
Select the Vulkan device whose VK_EXT_physical_device_drm render major:minor matches a caller-supplied node, so the encoder shares the compositor physical GPU. vendor:device ids cannot disambiguate identical GPUs, and importing a DMA-BUF across GPUs corrupts frames. Adds VideoContextBuilder::drm_render_node and a verify_drm_pin example.
Tested on a multi-gpu amd machine.