Thomas Zimmermann
2023-Feb-28 11:37 UTC
[PATCH] drm/virtio: Add option to disable KMS support
Hi Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:> Gerd Hoffmann <kraxel at redhat.com> 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 at redhat.com> > 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 at redhat.com> > --- > 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230228/6d3012ef/attachment-0001.sig>