Message-ID: <87a60yywo0.fsf@minerva.mail-host-address-is-not-set>
Date:   Tue, 28 Feb 2023 10:19:27 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Gerd Hoffmann <kraxel@...hat.com>, Rob Clark <robdclark@...il.com>
Cc:     Rob Clark <robdclark@...omium.org>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        open list <linux-kernel@...r.kernel.org>,
        dri-devel@...ts.freedesktop.org,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Ryan Neph <ryanneph@...omium.org>,
        David Airlie <airlied@...hat.com>,
        "open list:VIRTIO GPU DRIVER" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH] drm/virtio: Add option to disable KMS support

Gerd Hoffmann <kraxel@...hat.com> writes:

Hello Gerd,

> On Mon, Feb 27, 2023 at 07:40:11AM -0800, Rob Clark wrote:
>> On Sun, Feb 26, 2023 at 10:38 PM Gerd Hoffmann <kraxel@...hat.com> wrote:
>> >
>> > On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
>> > > From: Rob Clark <robdclark@...omium.org>
>> > >
>> > > Add a build option to disable modesetting support.  This is useful in
>> > > cases where the guest only needs to use the GPU in a headless mode, or
>> > > (such as in the CrOS usage) window surfaces are proxied to a host
>> > > compositor.
>> >
>> > Why make that a compile time option?  There is a config option for the
>> > number of scanouts (aka virtual displays) a device has.  Just set that
>> > to zero (and fix the driver to not consider that configuration an
>> > error).
>> 
>> The goal is to not advertise DRIVER_MODESET (and DRIVER_ATOMIC).. I
>> guess that could be done based on whether there are any scanouts, but
>> it would mean making the drm_driver struct non-const.
>
> Apparently there is a drm_device->driver_features override,
> (amdgpu uses that).  The driver could simply drop the DRIVER_MODESET and
> DRIVER_ATOMIC bits in case no scanout is present instead of throwing an
> error.
>
>> And I think it is legitimate to allow the guest to make this choice,
>> regardless of what the host decides to expose, since it is about the
>> ioctl surface area that the guest kernel exposes to guest userspace.
>
> 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.

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@...hat.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@...hat.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 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:
-- 
2.39.2

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Powered by blists - more mailing lists