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 <[email protected]> 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/[email protected]?part=4
