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

[sashiko-bot@kernel.org]

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