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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: sleeping in atomic context during plane atomic update
- [High] drm: VBLANK IRQ storm due to unhandled interrupt on driver unbind
--

commit 32feffcdec94761d97377d2756a8face5aaf6281
Author: Leander Kieweg <kieweg.leander@gmail.com>

drm/glanda: Add initial DRM driver for GlandaGPU

Introduce the core DRM/KMS driver for GlandaGPU. This driver
supports basic modesetting, atomic updates via shadow plane helpers,
and optional QEMU PCI probing alongside the platform driver.

> diff --git a/drivers/gpu/drm/tiny/glandagpu.c b/drivers/gpu/drm/tiny/glandagpu.c
> new file mode 100644
> index 0000000000000..38fa5809f31e0
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/glandagpu.c

[ ... ]

> +static void glanda_blit_rect(struct glanda_device *gdev,
> +			     const struct drm_rect *dst_clip,
> +			     const struct iosys_map *src,
> +			     struct drm_framebuffer *fb,
> +			     int dst_off_x, int dst_off_y,
> +				 struct drm_format_conv_state *fmtcnv_state)
> +{
> +	unsigned int src_pitch = fb->pitches[0];
> +	unsigned int width = drm_rect_width(dst_clip);
> +	unsigned int height = drm_rect_height(dst_clip);
> +	unsigned int x, y;
> +	size_t len = width * sizeof(u32);
> +	u32 *sbuf;
> +
> +	sbuf = drm_format_conv_state_reserve(fmtcnv_state, len, GFP_KERNEL);

[Severity: High]
Does this code sleep in atomic context? The glanda_blit_rect() function is 
called from the glanda_plane_atomic_update() callback, which runs in atomic 
context where sleeping is forbidden. Using GFP_KERNEL here can cause kernel 
panics or deadlocks.

[ ... ]

> +static void glanda_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_state = to_drm_shadow_plane_state(new_state);
> +	struct drm_rect vram_clip = DRM_RECT_INIT(0, 0, GLANDA_WIDTH, GLANDA_HEIGHT);
> +	struct glanda_device *gdev = to_glanda(plane->dev);
> +	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_rect damage;
> +	int dst_off_x, dst_off_y;
> +	int ret, idx;
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);

[Severity: High]
Can this sleep while waiting for hardware fences? Since 
glanda_plane_atomic_update() is executed in atomic context, calling 
drm_gem_fb_begin_cpu_access() (and the corresponding end_cpu_access) is 
unsafe here.

[ ... ]

> +static void glanda_drm_fini(struct glanda_device *gdev)
> +{
> +	drm_dev_unplug(&gdev->drm);
> +	drm_atomic_helper_shutdown(&gdev->drm);
> +}

[Severity: High]
Does this teardown order cause an unhandled IRQ storm on unbind?

If drm_dev_unplug() is called before drm_atomic_helper_shutdown(), the 
device is marked as unplugged. When the shutdown helper tries to disable 
the CRTC, it calls glanda_crtc_disable_vblank():

static void glanda_crtc_disable_vblank(struct drm_crtc *crtc)
{
	...
	if (!drm_dev_enter(crtc->dev, &idx))
		return;
	...
}

The drm_dev_enter() check will fail because the device is already unplugged, 
causing the hardware write to disable the VBLANK interrupt to be skipped. 
The interrupt would remain active in hardware while the driver frees the 
IRQ handler.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920193944.110983-1-kieweg.leander@gmail.com?part=2