Re: [PATCH v3 2/2] drm/glanda: Add initial DRM driver for GlandaGPU

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