From: Rob Clark <[email protected]>
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.
Signed-off-by: Rob Clark <[email protected]>
---
drivers/gpu/drm/virtio/Kconfig | 11 +++++++++++
drivers/gpu/drm/virtio/Makefile | 5 ++++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++++-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 ++++++++++
drivers/gpu/drm/virtio/virtgpu_kms.c | 6 ++++++
5 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..293e6f0bf133 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,9 +161,11 @@ 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 defined(CONFIG_DRM_VIRTIO_GPU_KMS)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
+#endif
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
vgdev->has_indirect = true;
}
@@ -218,6 +220,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
goto err_vbufs;
}
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
/* get display info */
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_scanouts, &num_scanouts);
@@ -229,6 +232,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
goto err_scanouts;
}
DRM_INFO("number of scanouts: %d\n", num_scanouts);
+#endif
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_capsets, &num_capsets);
@@ -246,10 +250,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
virtio_gpu_get_capsets(vgdev, num_capsets);
if (vgdev->has_edid)
virtio_gpu_cmd_get_edids(vgdev);
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
virtio_gpu_cmd_get_display_info(vgdev);
virtio_gpu_notify(vgdev);
wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
5 * HZ);
+#endif
return 0;
err_scanouts:
--
2.39.1
On 2/24/23 21:02, Rob Clark wrote:
> obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index ae97b98750b6..9cb7d6dd3da6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
> DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
>
> static const struct drm_driver driver = {
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> + .driver_features =
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
Could you please replace s/defined/IS_ENABLED/ and also the #ifdefs with
"if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS))" in the code? The ifdefs are
always not nice to have in the code. The IS_ENABLED usage will also make
code compile-tested regardless of the CONFIG_DRM_VIRTIO_GPU_KMS option
state.
Otherwise looks good!
--
Best regards,
Dmitry
On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> 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).
take care,
Gerd
On Sun, Feb 26, 2023 at 10:38 PM Gerd Hoffmann <[email protected]> wrote:
>
> On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > 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. 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.
BR,
-R
On Mon, Feb 27, 2023 at 07:40:11AM -0800, Rob Clark wrote:
> On Sun, Feb 26, 2023 at 10:38 PM Gerd Hoffmann <[email protected]> wrote:
> >
> > On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
> > > From: Rob Clark <[email protected]>
> > >
> > > 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.
dev.driver_features is a thing.
--
Ville Syrj?l?
Intel
On Mon, Feb 27, 2023 at 07:40:11AM -0800, Rob Clark wrote:
> On Sun, Feb 26, 2023 at 10:38 PM Gerd Hoffmann <[email protected]> wrote:
> >
> > On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
> > > From: Rob Clark <[email protected]>
> > >
> > > 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.
take care,
Gerd
Gerd Hoffmann <[email protected]> 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 <[email protected]> wrote:
>> >
>> > On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
>> > > From: Rob Clark <[email protected]>
>> > >
>> > > 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 <[email protected]>
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 <[email protected]>
---
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
On 2/28/23 12:19, Javier Martinez Canillas wrote:
> Gerd Hoffmann <[email protected]> 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 <[email protected]> wrote:
>>>> On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote:
>>>>> From: Rob Clark <[email protected]>
>>>>>
>>>>> 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.
IIUC, Rob's main point for having a config option is solely for security
reasons. The config option eliminates possibility of accidentally (or
intentionally) enabling KMS from software, which is better to have in
case of shipping a product (Chromebook) on which multiple teams are
working on.
--
Best regards,
Dmitry
Hi
Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
> Gerd Hoffmann <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
Thomas Zimmermann <[email protected]> writes:
> Hi
>
> Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
>> Gerd Hoffmann <[email protected]> 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.
>
AFAIK i915 and nouveau do this. But also on the rpi4 only the vc4 display
driver is disabled but the v3d driver used for rendering is not disabled.
So the 'nomodeset' semantics are not consistent across all DRM drivers.
[...]
>> - 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.
>
Right. Or the DRM aperture helper shouldn't attempt to remove the firmware
provided framebuffer if the DRM driver doesn't have the DRIVER_MODESET set.
> 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.
>
Yes, that could work too. Dmitry mentioned that Rob wanted the compile-time
option to reduce the attack surface area. I don't have a strong opinion on
this, but just wanted to point out that there wasn't a need for a new param
and that the existing module's 'modeset' could be repurposed for this case.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi
Am 24.02.23 um 19:02 schrieb Rob Clark:
> From: Rob Clark <[email protected]>
>
> 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.
We've just been discussing this on IRC, but failed to see the practical
benefit. It's not like the modesetting code takes up lots of memory.
What's the real-world use case?
Best regards
Thomas
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
> drivers/gpu/drm/virtio/Kconfig | 11 +++++++++++
> drivers/gpu/drm/virtio/Makefile | 5 ++++-
> drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++++-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 10 ++++++++++
> drivers/gpu/drm/virtio/virtgpu_kms.c | 6 ++++++
> 5 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> index 51ec7c3240c9..ea06ff2aa4b4 100644
> --- a/drivers/gpu/drm/virtio/Kconfig
> +++ b/drivers/gpu/drm/virtio/Kconfig
> @@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
> QEMU based VMMs (like KVM or Xen).
>
> If unsure say M.
> +
> +config DRM_VIRTIO_GPU_KMS
> + bool "Virtio GPU driver modesetting support"
> + depends on DRM_VIRTIO_GPU
> + default y
> + help
> + Enable modesetting support for virtio GPU driver. This can be
> + disabled in cases where only "headless" usage of the GPU is
> + required.
> +
> + If unsure, say Y.
> diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
> index b99fa4a73b68..24c7ebe87032 100644
> --- a/drivers/gpu/drm/virtio/Makefile
> +++ b/drivers/gpu/drm/virtio/Makefile
> @@ -4,8 +4,11 @@
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
> virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
> - virtgpu_display.o virtgpu_vq.o \
> + virtgpu_vq.o \
> virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
> virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
>
> +virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
> + virtgpu_display.o
> +
> obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index ae97b98750b6..9cb7d6dd3da6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
> DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
>
> static const struct drm_driver driver = {
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> + .driver_features =
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> + DRIVER_MODESET | DRIVER_ATOMIC |
> +#endif
> + DRIVER_GEM | DRIVER_RENDER,
> .open = virtio_gpu_driver_open,
> .postclose = virtio_gpu_driver_postclose,
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index af6ffb696086..ffe8faf67247 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
> uint32_t x, uint32_t y);
>
> /* virtgpu_display.c */
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> +#else
> +static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
> +{
> + return 0;
> +}
> +static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
> +{
> +}
> +#endif
>
> /* virtgpu_plane.c */
> uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 27b7f14dae89..293e6f0bf133 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -161,9 +161,11 @@ 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 defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
> vgdev->has_edid = true;
> }
> +#endif
> if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
> vgdev->has_indirect = true;
> }
> @@ -218,6 +220,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> goto err_vbufs;
> }
>
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> /* get display info */
> virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> num_scanouts, &num_scanouts);
> @@ -229,6 +232,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> goto err_scanouts;
> }
> DRM_INFO("number of scanouts: %d\n", num_scanouts);
> +#endif
>
> virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> num_capsets, &num_capsets);
> @@ -246,10 +250,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> virtio_gpu_get_capsets(vgdev, num_capsets);
> if (vgdev->has_edid)
> virtio_gpu_cmd_get_edids(vgdev);
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> virtio_gpu_cmd_get_display_info(vgdev);
> virtio_gpu_notify(vgdev);
> wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
> 5 * HZ);
> +#endif
> return 0;
>
> err_scanouts:
--
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