Hi
Am 08.06.22 um 16:04 schrieb Alex Williamson:
You shouldn't have to copy any of the implementation of the aperture helpers.
If you call drm_aperture_remove_conflicting_pci_framebuffers() it should work correctly. The only reason why it requires a DRM driver structure as second argument is for the driver's name. [1] And that name is only used for printing an info message. [2]
vfio-pci is not dependent on CONFIG_DRM, therefore we need to open code this regardless. The only difference if we were to use the existing function would be something like:
#if IS_REACHABLE(CONFIG_DRM) ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &dummy_drm_driver); if (ret) return ret; #else #if IS_REACHABLE(CONFIG_FB) ret = remove_conflicting_pci_framebuffers(pdev, vdev->vdev.ops->name); if (ret) return ret; #endif ret = vga_remove_vgacon(pdev); if (ret) return ret; #endif
It's also bad practice to create a dummy DRM driver struct with some assumption which fields are used. If the usage is later expanded, we'd only discover it by users getting segfaults. If DRM wanted to make such an API guarantee, then we shouldn't have commit 97c9bfe3f660 ("drm/aperture: Pass DRM driver structure instead of driver name").
What you're open coding within vfio is legacy code and we want to remove it as much as possible from the aperture helpers.
Tying the helpers to DRM was in mistake in retrospective. We wanted something tailored to the needs of DRM. Now that we've seen quite a few corner cases in the interaction among graphics subsystems, we need something else. The order of creating devices and loading driver modules is crucial to making the hand-over between drivers work reliably. So far, this luckily has only been a problem in theory, but not practice.
The plan forward would be to drop patch 1 entirely.
For patch 2, the most trivial workaround is to instanciate struct drm_driver here and set the name field to 'vdev->vdev.ops->name'. In the longer term, the aperture helpers will be moved out of DRM and into a more prominent location. That workaround will be cleaned up then.
Trivial in execution, but as above, this is poor practice and should be avoided.
Alternatively, drm_aperture_remove_conflicting_pci_framebuffers() could be changed to accept the name string as second argument, but that's quite a bit of churn within the DRM code.
The series as presented was exactly meant to provide the most correct solution with the least churn to the DRM code. The above referenced commit precludes us from calling the existing DRM function directly without resorting to poor practices of assuming the usage of the DRM driver struct. Using the existing DRM function also does not prevent us from open coding the remainder of the function to avoid a CONFIG_DRM dependency. Thanks,
Please have a look at the attached patch. It moves the aperture helpers to a location common to the various possible users (DRM, fbdev, vfio). The DRM interfaces remain untouched for now. The patch should provide what you need in vfio and also serve our future use cases for graphics drivers. If possible, please create your patch on top of it.
Best regards Thomas
Alex