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