Hi Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
Gerd Hoffmann <kraxel@xxxxxxxxxx> writes:
[...]
I think it is a bad idea to make that a compile time option, I'd suggest a runtime switch instead, for example a module parameter to ask the driver to ignore any scanouts.I don't think there's a need for a new module parameter, there's already the virtio-gpu 'modeset' module parameter to enable/disable modsetting and the global 'nomodeset' kernel cmdline parameter to do it for all DRM drivers. Currently, many drivers just fail to probe when 'nomodeset' is present, but others only disable modsetting but keep the rendering part. In fact, most DRM only drivers just ignore the 'nomodeset' parameter.
Do you have a list of these drivers? Maybe we need to adjust semantics slightly. Please see my comment below.
We could make virtio-gpu driver to only disable KMS with these params, something like the following (untested) patch: From 9cddee7f696f37c34d80d6160e87827f3d7a0237 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas <javierm@xxxxxxxxxx> Date: Tue, 28 Feb 2023 10:09:11 +0100 Subject: [PATCH] drm/virtio: Only disable KMS with nomodeset The virtio-gpu driver currently fails to probe if either the "nomodeset" kernel cmdline parameter is used or the module "modeset" parameter used. But there may be cases where the rendering part of the driver is needed and only the mode setting part needs to be disabled. So let's change the logic to only disable the KMS part but still keep the DRM side of it. Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> --- drivers/gpu/drm/virtio/virtgpu_display.c | 16 +++++++++++++++ drivers/gpu/drm/virtio/virtgpu_drv.c | 23 ++++++++++++++-------- drivers/gpu/drm/virtio/virtgpu_kms.c | 25 +----------------------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 9ea7611a9e0f..e176e5e8c1a0 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -335,6 +335,22 @@ static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) { int i, ret; + u32 num_scanouts; + + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) { + vgdev->has_edid = true; + } + + /* get display info */ + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config, + num_scanouts, &num_scanouts); + vgdev->num_scanouts = min_t(uint32_t, num_scanouts, + VIRTIO_GPU_MAX_SCANOUTS); + if (!vgdev->num_scanouts) { + DRM_ERROR("num_scanouts is zero\n"); + return -EINVAL; + } + DRM_INFO("number of scanouts: %d\n", num_scanouts);ret = drmm_mode_config_init(vgdev->ddev);if (ret) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index ae97b98750b6..979b5b177f49 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -40,7 +40,7 @@#include "virtgpu_drv.h" -static const struct drm_driver driver;+static struct drm_driver driver;static int virtio_gpu_modeset = -1; @@ -69,13 +69,12 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)static int virtio_gpu_probe(struct virtio_device *vdev) { struct drm_device *dev; + struct virtio_gpu_device *vgdev; int ret;- if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)- return -EINVAL; - - if (virtio_gpu_modeset == 0) - return -EINVAL; + if ((drm_firmware_drivers_only() && virtio_gpu_modeset == -1) || + (virtio_gpu_modeset == 0)) + driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
The kernel-wide option 'nomodeset' affects system behavior. It's a misnomer, as it actually means 'don't replace the firmware-provided framebuffer.' So if you just set these flags here, virtio-gpu would later remove the firmware driver via aperture helpers. Therefore, if drm_formware_drivers_only() returns true, we should fail probing with -ENODEV.
But we could try to repurpose the module's 'modeset' option. It's already obsoleted by nomodeset anyway. I'd try to make modeset it a boolean that controls modesetting vs render-only. It will then be about the driver's feature set, rather than system behavior.
Best regards Thomas
/** The virtio-gpu device is a virtual device that doesn't have DMA @@ -98,11 +97,19 @@ static int virtio_gpu_probe(struct virtio_device *vdev) if (ret) goto err_free;+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {+ vgdev = dev->dev_private; + ret = virtio_gpu_modeset_init(vgdev); + if (ret) + goto err_deinit; + } + ret = drm_dev_register(dev, 0); if (ret) goto err_deinit;- drm_fbdev_generic_setup(vdev->priv, 32);+ if (drm_core_check_feature(dev, DRIVER_MODESET)) + drm_fbdev_generic_setup(vdev->priv, 32); return 0;err_deinit:@@ -171,7 +178,7 @@ MODULE_AUTHOR("Alon Levy");DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops); -static const struct drm_driver driver = {+static struct drm_driver driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC, .open = virtio_gpu_driver_open, .postclose = virtio_gpu_driver_postclose, diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c index 27b7f14dae89..2f5f2aac6b71 100644 --- a/drivers/gpu/drm/virtio/virtgpu_kms.c +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c @@ -122,7 +122,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) struct virtio_gpu_device *vgdev; /* this will expand later */ struct virtqueue *vqs[2]; - u32 num_scanouts, num_capsets; + u32 num_capsets; int ret = 0;if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))@@ -161,9 +161,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL)) vgdev->has_virgl_3d = true; #endif - if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) { - vgdev->has_edid = true; - } if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) { vgdev->has_indirect = true; } @@ -218,28 +215,10 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) goto err_vbufs; }- /* get display info */- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config, - num_scanouts, &num_scanouts); - vgdev->num_scanouts = min_t(uint32_t, num_scanouts, - VIRTIO_GPU_MAX_SCANOUTS); - if (!vgdev->num_scanouts) { - DRM_ERROR("num_scanouts is zero\n"); - ret = -EINVAL; - goto err_scanouts; - } - DRM_INFO("number of scanouts: %d\n", num_scanouts); - virtio_cread_le(vgdev->vdev, struct virtio_gpu_config, num_capsets, &num_capsets); DRM_INFO("number of cap sets: %d\n", num_capsets);- ret = virtio_gpu_modeset_init(vgdev);- if (ret) { - DRM_ERROR("modeset init failed\n"); - goto err_scanouts; - } - virtio_device_ready(vgdev->vdev);if (num_capsets)@@ -252,8 +231,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) 5 * HZ); return 0;-err_scanouts:- virtio_gpu_free_vbufs(vgdev); err_vbufs: vgdev->vdev->config->del_vqs(vgdev->vdev); err_vqs:
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
