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

[PATCH 0/4] 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.

Lyude Paul (4):
  drm/nouveau: Print the nouveau.atomic parameter in
    nouveau_display_options()
  drm/nouveau/kms: Check nouveau.atomic parameter after
    nvif_device_ctor()
  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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)


base-commit: 4d49f1adfbf226c26ebba2958cc184f787abab41
-- 
2.55.0

[PATCH 0/4] 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.

Lyude Paul (4):
  drm/nouveau: Print the nouveau.atomic parameter in
    nouveau_display_options()
  drm/nouveau/kms: Check nouveau.atomic parameter after
    nvif_device_ctor()
  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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)


base-commit: 4d49f1adfbf226c26ebba2958cc184f787abab41
-- 
2.55.0

[PATCH 1/4] 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 4d1ad718e09b7..ad693d2849bec 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1416,6 +1416,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 1/4] 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 4d1ad718e09b7..ad693d2849bec 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1416,6 +1416,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 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

[Lyude Paul]

Waiting until the NVIF device is ready is preferable, as this will allow us
to factor in the chipset generation into whether or not we enable atomic by
default.

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

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ad693d2849bec..8899f009f825e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -727,7 +727,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,
+nouveau_drm_device_new(struct drm_driver *drm_driver, struct device *parent,
 		       struct nvkm_device *device)
 {
 	static const struct nvif_mclass
@@ -770,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		goto done;
 	}
 
+	if (nouveau_atomic)
+		driver_pci.driver_features |= DRIVER_ATOMIC;
+
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
 		NV_ERROR(drm, "Failed to map PRI: %d\n", ret);
@@ -879,9 +882,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
-
 	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
 	if (IS_ERR(drm)) {
 		ret = PTR_ERR(drm);
-- 
2.55.0

[PATCH 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

[Lyude Paul]

Waiting until the NVIF device is ready is preferable, as this will allow us
to factor in the chipset generation into whether or not we enable atomic by
default.

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

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ad693d2849bec..8899f009f825e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -727,7 +727,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,
+nouveau_drm_device_new(struct drm_driver *drm_driver, struct device *parent,
 		       struct nvkm_device *device)
 {
 	static const struct nvif_mclass
@@ -770,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
 		goto done;
 	}
 
+	if (nouveau_atomic)
+		driver_pci.driver_features |= DRIVER_ATOMIC;
+
 	ret = nvif_device_map(&drm->device);
 	if (ret) {
 		NV_ERROR(drm, "Failed to map PRI: %d\n", ret);
@@ -879,9 +882,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
-
 	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
 	if (IS_ERR(drm)) {
 		ret = PTR_ERR(drm);
-- 
2.55.0

[PATCH 3/4] 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>
---
 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 8899f009f825e..b959d79f7e1c7 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 drm_driver *drm_driver, struct device *parent,
 		goto done;
 	}
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
+	if (nouveau_atomic) {
+		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)
+			driver_pci.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 3/4] 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>
---
 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 8899f009f825e..b959d79f7e1c7 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 drm_driver *drm_driver, struct device *parent,
 		goto done;
 	}
 
-	if (nouveau_atomic)
-		driver_pci.driver_features |= DRIVER_ATOMIC;
+	if (nouveau_atomic) {
+		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)
+			driver_pci.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 4/4] 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 b959d79f7e1c7..5580ac9c782ba 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 drm_driver *drm_driver, struct device *parent,
 		goto done;
 	}
 
-	if (nouveau_atomic) {
+	if (nouveau_atomic != 0) {
 		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)
 			driver_pci.driver_features |= DRIVER_ATOMIC;
-		else
+		else if (nouveau_atomic == 1)
 			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
 	}
 
-- 
2.55.0

[PATCH 4/4] 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 b959d79f7e1c7..5580ac9c782ba 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 drm_driver *drm_driver, struct device *parent,
 		goto done;
 	}
 
-	if (nouveau_atomic) {
+	if (nouveau_atomic != 0) {
 		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)
 			driver_pci.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 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

[sashiko-bot]

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

New issues:
- [Medium] The shared initialization function nouveau_drm_device_new() incorrectly hardcodes modifications to the global driver_pci struct instead of the provided drm_driver parameter.

Pre-existing issues:
- [High] nouveau_drm_device_del() attempts to release an error pointer if drm_dev_alloc() fails, causing a kernel panic.
--

--- Patch [2]: [PATCH 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor() ---
commit c020f75bf537ed725d051dbe5e66bd9bd3dfb6b7
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

This commit moves the check for the nouveau.atomic parameter into
nouveau_drm_device_new() so it occurs after the NVIF device is ready.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ad693d2849bec..8899f009f825e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -727,7 +727,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,
> +nouveau_drm_device_new(struct drm_driver *drm_driver, struct device *parent,
>  		       struct nvkm_device *device)
>  {

[Severity: High]
This is a pre-existing issue, but does the error path in
nouveau_drm_device_new() cause a crash if drm_dev_alloc() fails?

If drm_dev_alloc() fails, it returns an error pointer which is stored
directly in drm->dev:

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_new() {
    ...
    drm->dev = drm_dev_alloc(drm_driver, parent);
    if (IS_ERR(drm->dev)) {
        ret = PTR_ERR(drm->dev);
        goto done;
    }
    ...
done:
    if (ret) {
        nouveau_drm_device_del(drm);
...
}

Then in nouveau_drm_device_del(), the truthiness check will succeed for
the error pointer, leading to a panic when it is passed to drm_dev_put():

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_del() {
    if (drm->dev)
        drm_dev_put(drm->dev);
    ...
}

Could we clear drm->dev on allocation failure, or check for IS_ERR()
in the cleanup function?

[ ... ]

> @@ -770,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
>  		goto done;
>  	}
>  
> +	if (nouveau_atomic)
> +		driver_pci.driver_features |= DRIVER_ATOMIC;

[Severity: Medium]
Does this unintentionally modify the global driver_pci struct instead of the
drm_driver parameter passed to nouveau_drm_device_new()?

If nouveau_drm_device_new() is called for a platform device (such as Tegra),
the platform driver template would miss the DRIVER_ATOMIC flag, and the
PCI driver template would be unexpectedly mutated instead.

Should this update drm_driver->driver_features rather than driver_pci?

> +
>  	ret = nvif_device_map(&drm->device);
>  	if (ret) {

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

Re: [PATCH 4/4] drm/nouveau/kms/nv50-: Enable atomic modesetting by default

[sashiko-bot]

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

Pre-existing issues:
- [High] Dynamic mutation of the shared global `driver_pci` struct during device probe causes a data race, ignores the passed `drm_driver` parameter, and fails to enable atomic modesetting on non-PCI (e.g., Tegra platform) devices.
- [High] Comparing the `chipset` field to the `NV_DEVICE_INFO_V0_TESLA` family enum incorrectly enables atomic modesetting on unsupported pre-nv50 hardware.
--

--- Patch [4]: [PATCH 4/4] drm/nouveau/kms/nv50-: Enable atomic modesetting by default ---
commit 2db79d2947cdc47fc30e7b1b87de35e2d70fb39d
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms/nv50-: Enable atomic modesetting by default

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.

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index b959d79f7e1c7..5580ac9c782ba 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -770,10 +771,10 @@ nouveau_drm_device_new(struct drm_driver *drm_driver, struct device *parent,
>  		goto done;
>  	}
>  
> -	if (nouveau_atomic) {
> +	if (nouveau_atomic != 0) {
>  		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)

[Severity: High]
This isn't a bug introduced by this patch, but changing the default of
nouveau_atomic to auto makes it active by default. In nouveau_drm_device_new()
in drivers/gpu/drm/nouveau/nouveau_drm.c, does this compare a chipset hex
identifier (e.g., 0x40 for NV40) against a family enum (0x06 for TESLA)?

Because 0x40 is greater than 0x06, this evaluates to true for unsupported older
hardware, forcing atomic modesetting on pre-nv50 hardware and breaking display
support. Should this check use device.info.family instead of chipset?

>  			driver_pci.driver_features |= DRIVER_ATOMIC;

[Severity: High]
This is a pre-existing issue, but with nouveau_atomic active by default,
does dynamically mutating the shared global driver_pci struct during device
probe in nouveau_drm_device_new() cause a data race?

Modifying driver_pci pollutes the template for all subsequent PCI devices,
even if they do not meet the chipset requirements. Additionally, does this
ignore the passed drm_driver parameter, preventing atomic modesetting from
being enabled on Tegra platform devices (which use driver_platform)?

> -		else
> +		else if (nouveau_atomic == 1)
>  			NV_WARN(drm, "Atomic modesetting not supported (needs nv50+)\n");
>  	}
>

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect comparison of `chipset` against a `family` enum constant (`NV_DEVICE_INFO_V0_TESLA`) fails to restrict atomic modesetting to NV50+ hardware.
- [Medium] Conditionally mutating the shared global `driver_pci` structure based on a per-device property causes state bleed across multiple heterogeneous GPUs.
--

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

This patch aims to prevent atomic modesetting from being forced on for
pre-nv50 chipsets. It adds a hardware generation check before enabling
the atomic driver feature.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> --- 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 drm_driver *drm_driver, struct device *parent,
>  		goto done;
>  	}
>  
> -	if (nouveau_atomic)
> -		driver_pci.driver_features |= DRIVER_ATOMIC;
> +	if (nouveau_atomic) {
> +		if (drm->device.info.chipset >= NV_DEVICE_INFO_V0_TESLA)

[Severity: High]
Does this comparison correctly identify NV50+ hardware?

The chipset field stores the hardware hex ID (e.g., 0x10 for NV10, 0x40 for
NV40), while NV_DEVICE_INFO_V0_TESLA is an architectural family enum defined
as 0x06.

Because legacy values like 0x10 and 0x40 are numerically greater than 0x06,
will this condition incorrectly evaluate to true for pre-nv50 hardware when
nouveau.atomic=1 is set?

> +			driver_pci.driver_features |= DRIVER_ATOMIC;

[Severity: Medium]
Is it safe to mutate the static global driver_pci structure here?

If a user has a heterogeneous multi-GPU setup (e.g., an NV50+ and an NV40)
and boots with nouveau.atomic=1, the NV50+ might probe first and append
DRIVER_ATOMIC to the global driver_pci.driver_features mask.

Will a subsequently probed legacy GPU inherit this mutated capability flag
since it shares the driver_pci template?

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

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