Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] `glanda_crtc_enable_vblank()` enables the hardware VSYNC interrupt without checking if an IRQ handler is registered, leading to an interrupt storm in polling mode. - [High] VRAM is mapped using standard I/O instead of Write-Combining, causing a severe performance regression. - [Medium] Unconditionally enabling the VSYNC interrupt during initialization bypasses DRM core's state management, causing unnecessary CPU wakes. - [Low] Unconditional usage of PCI APIs breaks the build when `CONFIG_PCI` is disabled under `COMPILE_TEST`. --
--- Patch [2]: [PATCH v3 2/2] drm/glanda: Add initial DRM driver for GlandaGPU --- commit a32b58ce69a1f7f207f85c416ed56ab0c146bd56 Author: Leander Kieweg <[email protected]> drm/glanda: Add initial DRM driver for GlandaGPU This commit introduces a new DRM/KMS driver for the GlandaGPU display controller. It supports basic modesetting and atomic updates via shadow planes, and allows for both platform and PCI probing. > diff --git a/drivers/gpu/drm/tiny/glandagpu.c > b/drivers/gpu/drm/tiny/glandagpu.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/glandagpu.c [ ... ] > +static int glanda_crtc_enable_vblank(struct drm_crtc *crtc) > +{ > + struct glanda_device *gdev = to_glanda(crtc->dev); > + u32 ier; > + > + ier = readl(gdev->mmio_base + REG_IER); > + writel(ier | INT_VSYNC, gdev->mmio_base + REG_IER); [Severity: High] Could this cause an unhandled interrupt storm in polling mode? If the driver falls back to polling mode, such as when platform_get_irq_optional() returns -ENXIO, gdev->irq is set to -1 and no handler is registered. If a userspace application calls DRM_IOCTL_WAIT_VBLANK, the DRM core will invoke this callback and enable the hardware vsync interrupt. Since there is no IRQ handler to clear the REG_ISR register, the hardware will keep the interrupt line asserted, which might disable a shared interrupt line and affect other devices. > + > + return 0; > +} [ ... ] > +static int glanda_drm_init(struct glanda_device *gdev, int irq) > +{ [ ... ] > + if (irq > 0) { > + gdev->irq = irq; > + ret = devm_request_irq(gdev->drm.dev, gdev->irq, > glanda_irq_handler, > + IRQF_SHARED, "glandagpu", gdev); > + if (ret) { > + drm_err(&gdev->drm, "Failed to request IRQ %d\n", > + gdev->irq); > + return ret; > + } > + > + writel(INT_VSYNC, gdev->mmio_base + REG_IER); [Severity: Medium] Is it necessary to unconditionally enable the hardware vsync interrupt here? The DRM core initializes software vblank state as disabled. Because it is already considered disabled, the core won't call the disable_vblank() callback. This means the hardware will fire vsync interrupts continuously from the moment the driver loads, even when the display is idle. > + } else { > + drm_warn(&gdev->drm, "No IRQ found, falling back to polling\n"); > + } [ ... ] > +static int glandagpu_probe(struct platform_device *pdev) > +{ [ ... ] > + if (resource_size(res) < GLANDA_MMIO_OFFSET + GLANDA_MMIO_SIZE) { > + dev_err(&pdev->dev, "MMIO region too small: %llu bytes, need at > least %u\n", > + (unsigned long long)resource_size(res), > + GLANDA_MMIO_OFFSET + GLANDA_MMIO_SIZE); > + return -EINVAL; > + } > + > + gdev->vram_phys = res->start; > + gdev->vram_base = devm_ioremap(&pdev->dev, res->start, > GLANDA_VRAM_SIZE); [Severity: High] Will this cause severe CPU stalling during atomic frame updates? The devm_ioremap() function creates a strongly uncacheable memory mapping. Since glanda_plane_atomic_update() writes hundreds of thousands of pixels individually using writel_relaxed(), each write will traverse the bus separately. Should this memory be mapped using write-combining (e.g., devm_ioremap_wc()) so the CPU can burst writes efficiently? > + gdev->mmio_base = devm_ioremap(&pdev->dev, res->start + > GLANDA_MMIO_OFFSET, > + > GLANDA_MMIO_SIZE); [ ... ] > +static struct platform_driver glandagpu_driver = { > + .driver = { > + .name = "glandagpu", > + .of_match_table = glanda_of_match, > + }, > + .probe = glandagpu_probe, > + .remove = glandagpu_remove, > +}; > + > +/* PCI probe path for the QEMU test device, real hardware uses > platform_driver */ > +static int glandagpu_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *id) > +{ [Severity: Low] Does this code break the build when the kernel is configured with CONFIG_DRM_GLANDA=y, CONFIG_COMPILE_TEST=y, and CONFIG_PCI=n? The Kconfig explicitly allows building without PCI for test coverage, but functions like pcim_iomap_regions() and pcim_iomap_table() do not have fallback stubs in the headers when PCI is disabled. Should the PCI-specific driver code be guarded by #ifdef CONFIG_PCI? > + struct glanda_device *gdev; > + int ret; > + > + ret = pcim_enable_device(pdev); > + if (ret) > + return ret; > + pci_set_master(pdev); > + > + if (pci_resource_len(pdev, 0) < GLANDA_MMIO_SIZE || > + pci_resource_len(pdev, 1) < GLANDA_VRAM_SIZE) { > + dev_err(&pdev->dev, "BAR too small: BAR0=%llu (need %u), > BAR1=%llu (need %u)\n", > + (unsigned long long)pci_resource_len(pdev, 0), > GLANDA_MMIO_SIZE, > + (unsigned long long)pci_resource_len(pdev, 1), > GLANDA_VRAM_SIZE); > + return -EINVAL; > + } > + > + ret = pcim_iomap_regions(pdev, BIT(0) | BIT(1), "glandagpu"); [Severity: High] Similar to the platform probe, does this create a strongly uncacheable mapping for VRAM? pcim_iomap_regions() defaults to standard I/O mappings. This will likely cause the same performance degradation during frame updates. > + if (ret) > + return ret; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
