Re: [PATCH RESEND 11/17] drm/spacemit: add Saturn DPU DRM device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Stalling the atomic commit pipeline by calling wait_for_flip_done() before commit_hw_done()
- [High] Sleeping functions (usleep_range) called from atomic context in CRTC enable/disable
- [High] Sleeping function called from atomic context in plane update due to missing prepare_fb
--

--- Patch [11]: [PATCH RESEND 11/17] drm/spacemit: add Saturn DPU DRM device driver ---
commit db43bd6ae677a859cea76ccf2390ca3d109d0fe0
Author: Cody Kang <codykang.hk@gmail.com>

drm/spacemit: add Saturn DPU DRM device driver

Add the DRM device for the SpacemiT K3 Saturn display processing unit
and hook the driver up to the build. The SoC has two DPU instances;
each drives one CRTC that scans out a single primary plane in linear
RGB at up to 3840x2160@60 and feeds a downstream DP/eDP controller
over an OF-graph endpoint. Buffers use drm_gem_shmem_helper. The
AFBC/compressed decode path is not exposed and writeback is not
implemented.

[ ... ]

> diff --git a/drivers/gpu/drm/spacemit/spacemit_drm.c b/drivers/gpu/drm/spacemit/spacemit_drm.c
> new file mode 100644
> index 0000000000000..9fb6a7661b484
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_drm.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025-2026 SpacemiT Co., Ltd.
> + *
> + */

[ ... ]

> +static void spacemit_drm_atomic_commit_tail(struct drm_atomic_commit *old_state)
> +{
> +	struct drm_device *dev = old_state->dev;
> +
> +	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +
> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +
> +	drm_atomic_helper_commit_planes(dev, old_state,
> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +
> +	drm_atomic_helper_wait_for_flip_done(dev, old_state);
> +
> +	drm_atomic_helper_commit_hw_done(old_state);

[Severity: High]
Does calling drm_atomic_helper_wait_for_flip_done() before
drm_atomic_helper_commit_hw_done() stall the commit pipeline?

drm_atomic_helper_commit_hw_done() signals that the driver has finished
programming the hardware, which unblocks subsequent non-blocking atomic commits.

Waiting for the VBLANK page flip to complete before signaling hw_done blocks
all subsequent commits, artificially serializing the commit pipeline and
defeating the non-blocking design of atomic KMS.

Could this lead to UI stutter and performance regressions?

> +
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
> +}

I also noticed a few additional regressions in files that appear to be part of
this driver series:

[Severity: High]
Are these usleep_range() calls safe in atomic context?

According to DRM subsystem guidelines, CRTC atomic enable/disable callbacks
execute in atomic context during non-blocking commits where sleeping is
forbidden.

Both spacemit_crtc_atomic_enable() and dpu_pm_suspend() (called from
spacemit_crtc_atomic_disable()) invoke usleep_range():

drivers/gpu/drm/spacemit/spacemit_crtc.c:spacemit_crtc_atomic_enable() {
    ...
		dpu_reset_assert(a_crtc->dev, "mclk", a_crtc->mclk_reset);
		dpu_reset_assert(a_crtc->dev, "aclk", a_crtc->aclk_reset);
		dpu_reset_assert(a_crtc->dev, "dsc", a_crtc->dsc_reset);
		usleep_range(10000, 20000);
    ...
}

drivers/gpu/drm/spacemit/spacemit_crtc.c:dpu_pm_suspend() {
    ...
	dpu_reset_assert(dev, "aclk", a_crtc->aclk_reset);
	dpu_reset_assert(dev, "dsc", a_crtc->dsc_reset);
	usleep_range(10000, 20000);
    ...
}

Calling sleeping functions in this context causes kernel warnings and system
instability.

Should non-sleeping delays like udelay() be used instead?


[Severity: High]
Does spacemit_plane_atomic_update() sleep in atomic context?

The DRM subsystem guidelines explicitly state that Plane atomic update callbacks
execute in atomic context where sleeping is forbidden.

spacemit_plane_atomic_update() calls spacemit_dmmu_map():

drivers/gpu/drm/spacemit/spacemit_planes.c:spacemit_plane_atomic_update() {
    ...
		ret = spacemit_dmmu_map(plane->state->fb,
					&spacemit_pstate->mmu_tbl, tbu_id,
					a_crtc->cl_rdma, plane);
    ...
}

which then calls drm_gem_shmem_get_pages_sgt():

drivers/gpu/drm/spacemit/spacemit_dmmu.c:spacemit_dmmu_map() {
    ...
	sgt = drm_gem_shmem_get_pages_sgt(to_drm_gem_shmem_obj(fb->obj[0]));
	if (IS_ERR(sgt))
    ...
}

This helper acquires a sleeping mutex via dma_resv_lock_interruptible(). This
causes a sleep in atomic context, risking kernel panics.

Should the driver implement a prepare_fb hook (e.g.
drm_gem_shmem_plane_helper_prepare_fb) to pin the pages and obtain the SGT
outside of the atomic update phase?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-k3-display-v1-0-6de34d80e86c@gmail.com?part=11

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy