[PATCH v2 0/5] drm/nouveau: Enable atomic modesetting by default

[PATCH v2 0/5] drm/nouveau: Enable atomic modesetting by default

[Lyude Paul]

Nouveau is one of the very few modern hardware drivers in the kernel that
doesn't have atomic modesetting enabled by default, in part because when it
was originally written by Ben there wasn't much in the way of good atomic
modesetting clients to actually test things out with.

Nowadays however, atomic modesetting is very much the norm - and support in
userspace for non-atomic drivers is starting to bitrot a bit - leading to
its own set of issues. At the same time, many of those issues are fixed by
just turning on atomic in nouveau. Plus, I've been running nouveau with
atomic modesetting on by default for most of the machines I work on, and
I've already fixed quite a number of issues to the point where things seem
quite stable (excluding a single screen flashing bug on my desktop, which I
am not particularly convinced has anything to do with atomic modesetting).

This patch series enables atomic modesetting by default, at least for
NV50 and newer. NV04 is left unchanged, as atomic modesetting was never
implemented for it. This series also enforces leaving atomic modesetting
disabled on <NV50, and adds the value of nouveau.atomic to our kernel
debug output.

Previous version of this patch series:

	https://patchwork.freedesktop.org/series/171310/

Lyude Paul (5):
  drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()
  drm/nouveau: Print the nouveau.atomic parameter in
    nouveau_display_options()
  drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling
  drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+
  drm/nouveau/kms/nv50-: Enable atomic modesetting by default

 drivers/gpu/drm/nouveau/nouveau_drm.c | 49 ++++++++++++++-------------
 1 file changed, 26 insertions(+), 23 deletions(-)


base-commit: 35b6cf07851a2b430b0eefbcb7b5a856f9030264
-- 
2.55.0

[PATCH v2 0/5] drm/nouveau: Enable atomic modesetting by default

[Lyude Paul]

Nouveau is one of the very few modern hardware drivers in the kernel that
doesn't have atomic modesetting enabled by default, in part because when it
was originally written by Ben there wasn't much in the way of good atomic
modesetting clients to actually test things out with.

Nowadays however, atomic modesetting is very much the norm - and support in
userspace for non-atomic drivers is starting to bitrot a bit - leading to
its own set of issues. At the same time, many of those issues are fixed by
just turning on atomic in nouveau. Plus, I've been running nouveau with
atomic modesetting on by default for most of the machines I work on, and
I've already fixed quite a number of issues to the point where things seem
quite stable (excluding a single screen flashing bug on my desktop, which I
am not particularly convinced has anything to do with atomic modesetting).

This patch series enables atomic modesetting by default, at least for
NV50 and newer. NV04 is left unchanged, as atomic modesetting was never
implemented for it. This series also enforces leaving atomic modesetting
disabled on <NV50, and adds the value of nouveau.atomic to our kernel
debug output.

Previous version of this patch series:

	https://patchwork.freedesktop.org/series/171310/

Lyude Paul (5):
  drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()
  drm/nouveau: Print the nouveau.atomic parameter in
    nouveau_display_options()
  drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling
  drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+
  drm/nouveau/kms/nv50-: Enable atomic modesetting by default

 drivers/gpu/drm/nouveau/nouveau_drm.c | 49 ++++++++++++++-------------
 1 file changed, 26 insertions(+), 23 deletions(-)


base-commit: 35b6cf07851a2b430b0eefbcb7b5a856f9030264
-- 
2.55.0

[PATCH v2 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

[Lyude Paul]

Sashiko caught this while reviewing the patches for enabling atomic by
default. Trying to fix this properly would be a bit of a pain, and we're
going to be adding another pre-DRM allocation here anyway. So, let's
instead just convert the initial struct nouveau_drm allocation over to
using devm so it is freed automatically.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4d1ad718e09b7..9570850656126 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -723,7 +723,6 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
 	nvif_parent_dtor(&drm->parent);
 
 	mutex_destroy(&drm->client_mutex);
-	kfree(drm);
 }
 
 static struct nouveau_drm *
@@ -740,17 +739,15 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 	struct nouveau_drm *drm;
 	int ret;
 
-	drm = kzalloc_obj(*drm);
+	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);
 	if (!drm)
 		return ERR_PTR(-ENOMEM);
 
 	drm->nvkm = device;
 
 	drm->dev = drm_dev_alloc(drm_driver, parent);
-	if (IS_ERR(drm->dev)) {
-		ret = PTR_ERR(drm->dev);
-		goto done;
-	}
+	if (IS_ERR(drm->dev))
+		return ERR_CAST(drm->dev);
 
 	drm->dev->dev_private = drm;
 	dev_set_drvdata(parent, drm);
-- 
2.55.0

[PATCH v2 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

[Lyude Paul]

Sashiko caught this while reviewing the patches for enabling atomic by
default. Trying to fix this properly would be a bit of a pain, and we're
going to be adding another pre-DRM allocation here anyway. So, let's
instead just convert the initial struct nouveau_drm allocation over to
using devm so it is freed automatically.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4d1ad718e09b7..9570850656126 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -723,7 +723,6 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
 	nvif_parent_dtor(&drm->parent);
 
 	mutex_destroy(&drm->client_mutex);
-	kfree(drm);
 }
 
 static struct nouveau_drm *
@@ -740,17 +739,15 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 	struct nouveau_drm *drm;
 	int ret;
 
-	drm = kzalloc_obj(*drm);
+	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);
 	if (!drm)
 		return ERR_PTR(-ENOMEM);
 
 	drm->nvkm = device;
 
 	drm->dev = drm_dev_alloc(drm_driver, parent);
-	if (IS_ERR(drm->dev)) {
-		ret = PTR_ERR(drm->dev);
-		goto done;
-	}
+	if (IS_ERR(drm->dev))
+		return ERR_CAST(drm->dev);
 
 	drm->dev->dev_private = drm;
 	dev_set_drvdata(parent, drm);
-- 
2.55.0

[PATCH v2 2/5] drm/nouveau: Print the nouveau.atomic parameter in nouveau_display_options()

[Lyude Paul]

Seems like we never remembered to start printing the value for this, so
let's start to aid in troubleshooting in case we run into any issues with
atomic.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 9570850656126..5565cf2367aa9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1413,6 +1413,7 @@ static void nouveau_display_options(void)
 	DRM_DEBUG_DRIVER("... debug        : %s\n", nouveau_debug);
 	DRM_DEBUG_DRIVER("... noaccel      : %d\n", nouveau_noaccel);
 	DRM_DEBUG_DRIVER("... modeset      : %d\n", nouveau_modeset);
+	DRM_DEBUG_DRIVER("... atomic       : %d\n", nouveau_atomic);
 	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
 	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
 	DRM_DEBUG_DRIVER("... hdmimhz      : %d\n", nouveau_hdmimhz);
-- 
2.55.0

[PATCH v2 2/5] drm/nouveau: Print the nouveau.atomic parameter in nouveau_display_options()

[Lyude Paul]

Seems like we never remembered to start printing the value for this, so
let's start to aid in troubleshooting in case we run into any issues with
atomic.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 9570850656126..5565cf2367aa9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1413,6 +1413,7 @@ static void nouveau_display_options(void)
 	DRM_DEBUG_DRIVER("... debug        : %s\n", nouveau_debug);
 	DRM_DEBUG_DRIVER("... noaccel      : %d\n", nouveau_noaccel);
 	DRM_DEBUG_DRIVER("... modeset      : %d\n", nouveau_modeset);
+	DRM_DEBUG_DRIVER("... atomic       : %d\n", nouveau_atomic);
 	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
 	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
 	DRM_DEBUG_DRIVER("... hdmimhz      : %d\n", nouveau_hdmimhz);
-- 
2.55.0

[PATCH v2 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

[Lyude Paul]

The way we handled the nouveau.atomic module parameter before was fairly
broken, and had a number of issues:

- It was only ever actually parsed in the case of PCI devices.
- When nouveau.atomic was enabled, it would add the cap for atomic
  modesetting to the global driver_pci structure. This meant that if one
  GPU on a system supported atomic and another didn't, it would still get
  enabled for both.

Looking into this exposed further silliness in the way that we actually
handle the drm_driver struct. We have one global structure for platform
devices, and another for PCI devices - both of which are literally
identical.

So before we start preparing to enable atomic modesetting by default, let's
fix this. Instead of sharing driver_pci and driver_platform, we move over
to dynamically allocating a drm_driver structure with devm to use, which
copies the contents of driver_stub. We then convert driver_stub to a const,
and move the handling of the nouveau.atomic module parameter into
nouveau_drm_device_new(), and conditionally add the atomic modesetting
capability to the dynamically allocated drm_driver struct.

Doing this is also preferable, as the next step for enabling atomic
modesetting by default will be ensuring that we don't enable it for legacy
devices that still don't support it. This requires only checking the atomic
modesetting module parameter after the NVIF device is ready, as this allows
us to check the GPU family that nouveau is running on.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V2:
* s/driver_pci/drm_driver/
* Dynamically allocate drm_driver struct, get rid of duplicate global
  driver structs to fix another Sashiko issue.

 drivers/gpu/drm/nouveau/nouveau_drm.c | 30 +++++++++++++--------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 5565cf2367aa9..ae489dc1b1943 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -111,9 +111,7 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1
 static int nouveau_runtime_pm = -1;
 module_param_named(runpm, nouveau_runtime_pm, int, 0400);
 
-static struct drm_driver driver_stub;
-static struct drm_driver driver_pci;
-static struct drm_driver driver_platform;
+static const struct drm_driver driver_stub;
 
 #ifdef CONFIG_DEBUG_FS
 struct dentry *nouveau_debugfs_root;
@@ -726,8 +724,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
 }
 
 static struct nouveau_drm *
-nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
-		       struct nvkm_device *device)
+nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 {
 	static const struct nvif_mclass
 	mmus[] = {
@@ -737,12 +734,18 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		{}
 	};
 	struct nouveau_drm *drm;
+	struct drm_driver *drm_driver;
 	int ret;
 
 	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);
 	if (!drm)
 		return ERR_PTR(-ENOMEM);
 
+	drm_driver = devm_kzalloc(parent, sizeof(*drm_driver), GFP_KERNEL);
+	if (!drm_driver)
+		return ERR_PTR(-ENOMEM);
+	*drm_driver = driver_stub;
+
 	drm->nvkm = device;
 
 	drm->dev = drm_dev_alloc(drm_driver, parent);
@@ -767,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		goto done;
 	}
 
+	if (nouveau_atomic)
+		drm_driver->driver_features |= DRIVER_ATOMIC;
+
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
 		NV_ERROR(drm, "Failed to map PRI: %d\n", ret);
@@ -870,16 +876,13 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 		return ret;
 
 	/* Remove conflicting drivers (vesafb, efifb etc). */
-	ret = aperture_remove_conflicting_pci_devices(pdev, driver_pci.name);
+	ret = aperture_remove_conflicting_pci_devices(pdev, driver_stub.name);
 	if (ret)
 		goto fail_nvkm;
 
 	pci_set_master(pdev);
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
-
-	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
+	drm = nouveau_drm_device_new(&pdev->dev, device);
 	if (IS_ERR(drm)) {
 		ret = PTR_ERR(drm);
 		goto fail_nvkm;
@@ -1356,7 +1359,7 @@ nouveau_driver_fops = {
 	.fop_flags = FOP_UNSIGNED_OFFSET,
 };
 
-static struct drm_driver
+static const struct drm_driver
 driver_stub = {
 	.driver_features = DRIVER_GEM |
 			   DRIVER_SYNCOBJ | DRIVER_SYNCOBJ_TIMELINE |
@@ -1453,7 +1456,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 	if (err)
 		goto err_free;
 
-	drm = nouveau_drm_device_new(&driver_platform, &pdev->dev, *pdevice);
+	drm = nouveau_drm_device_new(&pdev->dev, *pdevice);
 	if (IS_ERR(drm)) {
 		err = PTR_ERR(drm);
 		goto err_free;
@@ -1478,9 +1481,6 @@ nouveau_drm_init(void)
 {
 	int ret;
 
-	driver_pci = driver_stub;
-	driver_platform = driver_stub;
-
 	nouveau_display_options();
 
 	if (nouveau_modeset == -1) {
-- 
2.55.0

[PATCH v2 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

[Lyude Paul]

The way we handled the nouveau.atomic module parameter before was fairly
broken, and had a number of issues:

- It was only ever actually parsed in the case of PCI devices.
- When nouveau.atomic was enabled, it would add the cap for atomic
  modesetting to the global driver_pci structure. This meant that if one
  GPU on a system supported atomic and another didn't, it would still get
  enabled for both.

Looking into this exposed further silliness in the way that we actually
handle the drm_driver struct. We have one global structure for platform
devices, and another for PCI devices - both of which are literally
identical.

So before we start preparing to enable atomic modesetting by default, let's
fix this. Instead of sharing driver_pci and driver_platform, we move over
to dynamically allocating a drm_driver structure with devm to use, which
copies the contents of driver_stub. We then convert driver_stub to a const,
and move the handling of the nouveau.atomic module parameter into
nouveau_drm_device_new(), and conditionally add the atomic modesetting
capability to the dynamically allocated drm_driver struct.

Doing this is also preferable, as the next step for enabling atomic
modesetting by default will be ensuring that we don't enable it for legacy
devices that still don't support it. This requires only checking the atomic
modesetting module parameter after the NVIF device is ready, as this allows
us to check the GPU family that nouveau is running on.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V2:
* s/driver_pci/drm_driver/
* Dynamically allocate drm_driver struct, get rid of duplicate global
  driver structs to fix another Sashiko issue.

 drivers/gpu/drm/nouveau/nouveau_drm.c | 30 +++++++++++++--------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 5565cf2367aa9..ae489dc1b1943 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -111,9 +111,7 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1
 static int nouveau_runtime_pm = -1;
 module_param_named(runpm, nouveau_runtime_pm, int, 0400);
 
-static struct drm_driver driver_stub;
-static struct drm_driver driver_pci;
-static struct drm_driver driver_platform;
+static const struct drm_driver driver_stub;
 
 #ifdef CONFIG_DEBUG_FS
 struct dentry *nouveau_debugfs_root;
@@ -726,8 +724,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
 }
 
 static struct nouveau_drm *
-nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
-		       struct nvkm_device *device)
+nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 {
 	static const struct nvif_mclass
 	mmus[] = {
@@ -737,12 +734,18 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		{}
 	};
 	struct nouveau_drm *drm;
+	struct drm_driver *drm_driver;
 	int ret;
 
 	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);
 	if (!drm)
 		return ERR_PTR(-ENOMEM);
 
+	drm_driver = devm_kzalloc(parent, sizeof(*drm_driver), GFP_KERNEL);
+	if (!drm_driver)
+		return ERR_PTR(-ENOMEM);
+	*drm_driver = driver_stub;
+
 	drm->nvkm = device;
 
 	drm->dev = drm_dev_alloc(drm_driver, parent);
@@ -767,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		goto done;
 	}
 
+	if (nouveau_atomic)
+		drm_driver->driver_features |= DRIVER_ATOMIC;
+
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
 		NV_ERROR(drm, "Failed to map PRI: %d\n", ret);
@@ -870,16 +876,13 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 		return ret;
 
 	/* Remove conflicting drivers (vesafb, efifb etc). */
-	ret = aperture_remove_conflicting_pci_devices(pdev, driver_pci.name);
+	ret = aperture_remove_conflicting_pci_devices(pdev, driver_stub.name);
 	if (ret)
 		goto fail_nvkm;
 
 	pci_set_master(pdev);
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
-
-	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
+	drm = nouveau_drm_device_new(&pdev->dev, device);
 	if (IS_ERR(drm)) {
 		ret = PTR_ERR(drm);
 		goto fail_nvkm;
@@ -1356,7 +1359,7 @@ nouveau_driver_fops = {
 	.fop_flags = FOP_UNSIGNED_OFFSET,
 };
 
-static struct drm_driver
+static const struct drm_driver
 driver_stub = {
 	.driver_features = DRIVER_GEM |
 			   DRIVER_SYNCOBJ | DRIVER_SYNCOBJ_TIMELINE |
@@ -1453,7 +1456,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 	if (err)
 		goto err_free;
 
-	drm = nouveau_drm_device_new(&driver_platform, &pdev->dev, *pdevice);
+	drm = nouveau_drm_device_new(&pdev->dev, *pdevice);
 	if (IS_ERR(drm)) {
 		err = PTR_ERR(drm);
 		goto err_free;
@@ -1478,9 +1481,6 @@ nouveau_drm_init(void)
 {
 	int ret;
 
-	driver_pci = driver_stub;
-	driver_platform = driver_stub;
-
 	nouveau_display_options();
 
 	if (nouveau_modeset == -1) {
-- 
2.55.0

[PATCH v2 4/5] drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+

[Lyude Paul]

Atomic modesetting support was never added for pre-nv50 chipsets, so make
sure we don't allow it to be forced on. Additionally, print a small warning
when it's not supported.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V2:
* Check against info.family, not info.chipset

 drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ae489dc1b1943..33f60f5d4d9f3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -770,8 +770,12 @@ nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 		goto done;
 	}
 
-	if (nouveau_atomic)
-		drm_driver->driver_features |= DRIVER_ATOMIC;
+	if (nouveau_atomic) {
+		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
+			drm_driver->driver_features |= DRIVER_ATOMIC;
+		else
+			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
+	}
 
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
-- 
2.55.0

[PATCH v2 4/5] drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+

[Lyude Paul]

Atomic modesetting support was never added for pre-nv50 chipsets, so make
sure we don't allow it to be forced on. Additionally, print a small warning
when it's not supported.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V2:
* Check against info.family, not info.chipset

 drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ae489dc1b1943..33f60f5d4d9f3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -770,8 +770,12 @@ nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 		goto done;
 	}
 
-	if (nouveau_atomic)
-		drm_driver->driver_features |= DRIVER_ATOMIC;
+	if (nouveau_atomic) {
+		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
+			drm_driver->driver_features |= DRIVER_ATOMIC;
+		else
+			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
+	}
 
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
-- 
2.55.0

[PATCH v2 5/5] drm/nouveau/kms/nv50-: Enable atomic modesetting by default

[Lyude Paul]

Nouveau is one of the very few modern hardware drivers in the kernel that
doesn't have atomic modesetting enabled by default, in part because when it
was originally written by Ben there wasn't much in the way of good atomic
modesetting clients to actually test things out with.

Nowadays however, atomic modesetting is very much the norm - and support in
userspace for non-atomic drivers is starting to bitrot a bit - leading to
its own set of issues. At the same time, many of those issues are fixed by
just turning on atomic in nouveau. Plus, I've been running nouveau with
atomic modesetting on by default for most of the machines I work on, and
I've already fixed quite a number of issues to the point where things seem
quite stable (excluding a single screen flashing bug on my desktop, which I
am not particularly convinced has anything to do with atomic modesetting).

Now that we've protected against breaking things for chipsets where atomic
isn't supported (<nv50) - let's enable it by default on generations of
hardware that support it. We'll leave the module parameter around for the
time being, as turning it off may be helpful in the possibility that we hit
regressions.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 33f60f5d4d9f3..6da38d2e77c82 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -103,8 +103,9 @@ MODULE_PARM_DESC(modeset, "enable driver (default: auto, "
 int nouveau_modeset = -1;
 module_param_named(modeset, nouveau_modeset, int, 0400);
 
-MODULE_PARM_DESC(atomic, "Expose atomic ioctl (default: disabled)");
-static int nouveau_atomic = 0;
+MODULE_PARM_DESC(atomic, "Expose atomic ioctl (default: auto, "
+			 "0 = disabled, 1 = enabled)");
+static int nouveau_atomic = -1;
 module_param_named(atomic, nouveau_atomic, int, 0400);
 
 MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1)");
@@ -770,10 +771,10 @@ nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 		goto done;
 	}
 
-	if (nouveau_atomic) {
+	if (nouveau_atomic != 0) {
 		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
 			drm_driver->driver_features |= DRIVER_ATOMIC;
-		else
+		else if (nouveau_atomic == 1)
 			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
 	}
 
-- 
2.55.0

[PATCH v2 5/5] drm/nouveau/kms/nv50-: Enable atomic modesetting by default

[Lyude Paul]

Nouveau is one of the very few modern hardware drivers in the kernel that
doesn't have atomic modesetting enabled by default, in part because when it
was originally written by Ben there wasn't much in the way of good atomic
modesetting clients to actually test things out with.

Nowadays however, atomic modesetting is very much the norm - and support in
userspace for non-atomic drivers is starting to bitrot a bit - leading to
its own set of issues. At the same time, many of those issues are fixed by
just turning on atomic in nouveau. Plus, I've been running nouveau with
atomic modesetting on by default for most of the machines I work on, and
I've already fixed quite a number of issues to the point where things seem
quite stable (excluding a single screen flashing bug on my desktop, which I
am not particularly convinced has anything to do with atomic modesetting).

Now that we've protected against breaking things for chipsets where atomic
isn't supported (<nv50) - let's enable it by default on generations of
hardware that support it. We'll leave the module parameter around for the
time being, as turning it off may be helpful in the possibility that we hit
regressions.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 33f60f5d4d9f3..6da38d2e77c82 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -103,8 +103,9 @@ MODULE_PARM_DESC(modeset, "enable driver (default: auto, "
 int nouveau_modeset = -1;
 module_param_named(modeset, nouveau_modeset, int, 0400);
 
-MODULE_PARM_DESC(atomic, "Expose atomic ioctl (default: disabled)");
-static int nouveau_atomic = 0;
+MODULE_PARM_DESC(atomic, "Expose atomic ioctl (default: auto, "
+			 "0 = disabled, 1 = enabled)");
+static int nouveau_atomic = -1;
 module_param_named(atomic, nouveau_atomic, int, 0400);
 
 MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1)");
@@ -770,10 +771,10 @@ nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
 		goto done;
 	}
 
-	if (nouveau_atomic) {
+	if (nouveau_atomic != 0) {
 		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
 			drm_driver->driver_features |= DRIVER_ATOMIC;
-		else
+		else if (nouveau_atomic == 1)
 			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
 	}
 
-- 
2.55.0

Re: [PATCH v2 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dynamically allocating `struct drm_driver` with `devm_kzalloc` introduces a Use-After-Free vulnerability when the physical device is unbound.

Pre-existing issues:
- [High] The core driver structure `struct nouveau_drm` is allocated with `devm_kzalloc`, leading to a preexisting Use-After-Free when the device is unbound.
--

--- Patch [3]: [PATCH v2 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling ---
commit 9eb1e724469599d6b2baf3bcec061d6d7b31c8bc
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

This commit addresses issues with how the nouveau.atomic module parameter is
handled. It replaces shared static drm_driver structures with dynamically
allocated ones using devm_kzalloc to allow safely setting per-device driver
flags.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 5565cf2367aa9..ae489dc1b1943 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -737,12 +734,18 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
>  		{}
>  	};
>  	struct nouveau_drm *drm;
> +	struct drm_driver *drm_driver;
>  	int ret;
>  
>  	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does allocating struct nouveau_drm with
devm_kzalloc() cause a use-after-free on device unbind?

If a userspace process holds a DRM file descriptor open while the physical
device is unbound via sysfs or hot-unplug, devres will free this memory.
Since the DRM device lifetime is reference-counted and extended by the open
descriptor, subsequent ioctls or the final drm_release() might dereference
the freed memory via dev->dev_private.

>  	if (!drm)
>  		return ERR_PTR(-ENOMEM);
>  
> +	drm_driver = devm_kzalloc(parent, sizeof(*drm_driver), GFP_KERNEL);

[Severity: High]
Does allocating drm_driver with devm_kzalloc() introduce a similar
use-after-free vulnerability?

If the device is unbound while a DRM file descriptor is open, devres will
free the drm_driver struct. Since the DRM device outlives the physical
device in this scenario, when userspace eventually closes the file
descriptor, the DRM core cleans up by calling driver hooks (like postclose
or release) through dev->driver, which would now point to freed memory.

Could this be avoided by using the static const struct drm_driver and
modifying the drm->dev->driver_features field instead of allocating a new
driver struct for each device?

> +	if (!drm_driver)
> +		return ERR_PTR(-ENOMEM);
> +	*drm_driver = driver_stub;
> +
>  	drm->nvkm = device;
>  
>  	drm->dev = drm_dev_alloc(drm_driver, parent);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730195523.1467423-1-lyude@redhat.com?part=3

Re: [PATCH v2 4/5] drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Dynamically allocating `struct drm_driver` with `devm_kzalloc` creates a Use-After-Free on device unbind.
--

--- Patch [4]: [PATCH v2 4/5] drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+ ---
commit 5c23d4459a380de49a6e089b9ca550a380ff5d97
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms: Only allow enabling atomic modesetting on nv50+

Atomic modesetting support was never added for pre-nv50 chipsets, so make
sure we don't allow it to be forced on. Additionally, print a small warning
when it's not supported.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ae489dc1b1943..33f60f5d4d9f3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -770,8 +770,12 @@ nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
>  		goto done;
>  	}
>  
> -	if (nouveau_atomic)
> -		drm_driver->driver_features |= DRIVER_ATOMIC;
> +	if (nouveau_atomic) {
> +		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
> +			drm_driver->driver_features |= DRIVER_ATOMIC;

[Severity: High]
Earlier in nouveau_drm_device_new(), drm_driver is dynamically allocated
with devm_kzalloc():

	drm_driver = devm_kzalloc(parent, sizeof(*drm_driver), GFP_KERNEL);
	if (!drm_driver)
		return ERR_PTR(-ENOMEM);

Can this create a use-after-free on device unbind?

If userspace opens the DRM device node, it takes a reference on the
drm_device object. If the PCI device is hot-unplugged or manually unbound,
devres cleanup frees the drm_driver (along with nouveau_drm).

Later, when userspace closes the DRM file descriptor, drm_release() calls
drm_dev_needs_global_mutex() which evaluates dev->driver->driver_features,
and eventually executes dev->driver->postclose(), dereferencing the freed
memory.

> +		else
> +			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
> +	}
>  
>  	ret = nvif_device_map(&drm->device);
>  	if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730195523.1467423-1-lyude@redhat.com?part=4

Re: [PATCH v2 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free on DRM driver private data (`struct nouveau_drm`) when userspace retains file descriptors or GEM objects after the physical device is unbound.
--

--- Patch [1]: [PATCH v2 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new() ---
commit 915c6f6927c7e80b9c5ab3b1a2ff665d1e795afc
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

This commit converts the allocation of struct nouveau_drm to use devres
to simplify the cleanup path.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 4d1ad718e09b7..9570850656126 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -740,17 +739,15 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
>  	struct nouveau_drm *drm;
>  	int ret;
>  
> -	drm = kzalloc_obj(*drm);
> +	drm = devm_kzalloc(parent, sizeof(*drm), GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't tying the allocation
of struct nouveau_drm to the physical device's devres lifecycle preserve a
use-after-free condition?

If a user opens the DRM device, allocates a GEM object, and holds onto it
across a device unbind (such as a module unload or hot-unplug), the device
removal will trigger devres cleanup and free this devm_kzalloc'd memory.

Later, when the user drops the last reference to the GEM object, the DRM core
cleans it up and eventually calls nouveau_gem_object_del(). This function
derives the nouveau_drm pointer from the GEM object's embedded TTM buffer
object and dereferences it, accessing the freed memory. TTM core functions
will also access the dangling ttm_device embedded in the freed nouveau_drm
struct.

Would it be safer to use drmm_kzalloc() here so the driver-private data lives
exactly as long as the drm_device refcount?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730195523.1467423-1-lyude@redhat.com?part=1