Re: [PATCH RESEND 09/17] drm/spacemit: add Saturn DPU hardware backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Sleeping memory allocations (`GFP_KERNEL`) in atomic context plane and CRTC update paths, with a silent fallback that breaks atomic update semantics.
- [High] Sleeping delay function (`usleep_range`) used in CRTC atomic enable context.
- [High] Unconditional dereference of `state->fb` for disabled or solid-fill planes.
- [High] Race condition between the configuration timer handler and the VSYNC ISR leads to a double `drm_crtc_vblank_put` and refcount underflow.
- [High] The `cfg_rdy_timer` is not synchronously canceled when the DPU hardware is uninitialized or disabled.
- [High] `row_eof_tag` and un-strobed `module_regs` are left uninitialized in `saturn_hee_cmdlist_fill_data_row()`.
- [High] Accessing `crtc->state->event` from the VBLANK interrupt handler races with concurrent non-blocking atomic commits, causing leaked and premature events.
- [High] Command list length counter desynchronization leads to missing EOF tags and display hangs.
- [High] RDMA hardware configuration is written to a shared staging buffer but is never packed into the DMA command list, causing updates to be dropped or overwritten.
- [Medium] `dpu_run()` ignores the return value of `drm_crtc_vblank_get()`, potentially leading to refcount underflow.
- [Medium] Using C bitfields for DMA-shared hardware descriptors (`struct cmdlist_header` and `struct cmdlist_row`) leads to compiler-dependent memory layout and breaks on different endiannesses.
--

--- Patch [9]: [PATCH RESEND 09/17] drm/spacemit: add Saturn DPU hardware backend ---
commit 51bff8176b75d13f3aa75c39d52083ed25e121c0
Author: Cody Kang <codykang.hk@gmail.com>
Date:   Sat Jul 25 04:51:18 2026 +0000

    drm/spacemit: add Saturn DPU hardware backend
    
    Add the register-level programming of one Saturn instance: clock and
    reset bring-up, scene control and timing generator setup, RDMA channel
    and composer layer programming, FBC line-buffer budgeting, interrupt
    dispatch and the cfg-ready/vsync handshake that paces commits.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
> index 8d5f13bc5373c.....
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
[ ... ]
> +void spacemit_cfg_rdy_timer_handler(struct timer_list *t)
> +{
[ ... ]
> +	spin_lock_irqsave(&drm->event_lock, flags);
> +	event = crtc->state->event;

[Severity: High]
Does this race with concurrent non-blocking atomic commits? 

The DRM core's atomic helpers synchronously swap the crtc->state pointer
during the commit phase before the hardware's VBLANK interrupt fires. If two
non-blocking commits are issued back-to-back, the second commit swaps
crtc->state to its new state.

When the first commit's VBLANK fires (or this timer handler executes), reading
crtc->state->event prematurely consumes the second commit's event, leaking
the first commit's event. Should the event be stored in a driver-private CRTC
structure instead?

> +	if (event) {
> +		crtc->state->event = NULL;
> +		drm_crtc_send_vblank_event(crtc, event);
> +	}
> +	spin_unlock_irqrestore(&drm->event_lock, flags);
> +
> +	drm_crtc_vblank_put(crtc);
> +}
[ ... ]
> +static int dpu_init(struct spacemit_crtc *a_crtc)
> +{
> +	unsigned int timeout = 1000;
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +
> +	if (!a_crtc->clocks_on)
> +		return 0;
> +
> +	while (timeout) {
> +		if (hwdev->get_cfg_rdy(a_crtc, hwdev) == 0)
> +			break;
> +		usleep_range(100, 200);

[Severity: High]
Is it safe to use a sleeping delay function here?

dpu_init() is hooked to the .init callback of dpu_core_ops, which
executes during the CRTC atomic enable phase. The DRM subsystem guidelines
explicitly state that CRTC atomic enable callbacks run in atomic context
where sleeping is strictly forbidden. 

Will this trigger a "scheduling while atomic" panic?

> +		timeout--;
> +	}
> +	if (timeout == 0)
> +		drm_err(a_crtc->crtc.dev, "wait for cfg ready timed out\n");
> +
> +	hwdev->dpu_init(a_crtc);
> +
> +	return 0;
> +}
> +
> +static void dpu_uninit(struct spacemit_crtc *a_crtc)
> +{
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +
> +	if (!a_crtc->clocks_on)
> +		return;
> +
> +	trace_dpu_uninit(a_crtc->dev_id);
> +	hwdev->irq_enable(a_crtc, false);
> +}

[Severity: High]
Does this uninitialization path need to cancel the configuration timer?

saturn_ctrl_cfg_ready() arms a_crtc->cfg_rdy_timer when the DPU is started.
If the display pipeline is disabled before the hardware raises the CFG_RDY
interrupt, this timer will remain active and could fire asynchronously after
shutdown.

Can this lead to spurious timeout errors, incorrect vblank reference decrements,
or a use-after-free if the CRTC object is destroyed before the timer is
canceled?

> +static inline void dpu_isr_vblank(struct spacemit_crtc *a_crtc, bool *flip)
> +{
[ ... ]
> +	spin_lock(&drm->event_lock);
> +	event = crtc->state->event;

[Severity: High]
Can this read the wrong event pointer?

Similar to the timer handler above, accessing crtc->state->event races with
concurrent non-blocking atomic commits that swap the crtc->state pointer.
If a new commit is queued, this reads the new state's event and leaks the
current flip's event.

> +	if (event) {
[ ... ]
> +static u32 dpu_online_isr(struct spacemit_crtc *a_crtc)
> +{
[ ... ]
> +	irq_bit = hwdev->get_irq_bit(INT_CFG_RDY, dev_id);
> +	if (irq_raw & irq_bit) {
> +		hwdev->clr_int_sts(a_crtc, irq_bit, dev_id);
> +		trace_dpu_isr_status("cfg_rdy_clr", irq_raw & irq_bit);
> +		/*
> +		 * A fired watchdog already sent the event and put this flip's
> +		 * vblank reference; re-arming the vsync path would put twice.
> +		 */
> +		if (timer_delete(&a_crtc->cfg_rdy_timer))
> +			a_crtc->flip_done = false;

[Severity: High]
Does using the non-blocking timer_delete() return value create a race
condition?

If timer_delete() returns 0 (e.g., because the callback is actively executing
on another CPU), it leaves a_crtc->flip_done unmodified. 

The ISR then processes INT_VSYNC and calls dpu_isr_vblank(), which reads
flip_done == false, sets it to true, and calls drm_crtc_vblank_put().
Concurrently, the timer handler runs, sets flip_done = true, and also
unconditionally calls drm_crtc_vblank_put(). 

Could this lead to a double vblank put and refcount underflow?

> +		a_crtc->ur_reported = false;
[ ... ]
> +static void dpu_run(struct drm_crtc *crtc,
> +		    struct drm_crtc_state *old_state)
> +{
> +	struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc);
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +
> +	trace_dpu_run(a_crtc->dev_id);
> +
> +	if (hwdev->conf_dpuctrl)
> +		hwdev->conf_dpuctrl(crtc, old_state);
> +
> +	/* flush all config register writes before CFG_RDY is raised */
> +	mb();
> +
> +	drm_crtc_vblank_get(crtc);

[Severity: Medium]
Should the return value of drm_crtc_vblank_get() be checked?

drm_crtc_vblank_get() can fail (e.g., if vblank interrupts are uninitialized
or disabled), returning a negative error code without incrementing the vblank
reference count. 

By ignoring this return value, this function proceeds as if the reference was
successfully acquired. When the hardware interrupt or timeout later triggers,
they unconditionally call drm_crtc_vblank_put(), which could cause a refcount
underflow.

> +	if (hwdev->enable_cfg_irq)
> +		hwdev->enable_cfg_irq(a_crtc, hwdev, true);
> +
> +	saturn_ctrl_cfg_ready(a_crtc, true);

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
> index c23e4ca36d26b.....
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
[ ... ]
> +void saturn_hee_plane_update_hw_channel(struct drm_plane *plane)
> +{
[ ... ]
> +	/* For solid color both src_w and src_h are 0 */
> +	if (!solid_en) {
> +		base = RDMA_BASE_ADDR[rdma_id];
> +		/* linear scanout only: the FBC decode path is not exposed */
> +		dpu_write(hwdev, RDMA_PATH_X_REG, base, layer_mode, 0, cl_rdma);

[Severity: High]
Are these RDMA hardware configuration writes dropped because they are never
packed into the DMA command list?

This uses a_crtc->cl_rdma to stage hardware writes for the RDMA block.
However, there is no corresponding call to cmdlist_regs_packing() for
cl_rdma to commit these writes to the DMA-backed command list.

Since a_crtc->cl_rdma is a single shared buffer, do subsequent plane updates
overwrite the staged settings of previous planes, silently corrupting the
configuration and failing to program the hardware?

> +		dpu_write(hwdev, RDMA_PATH_X_REG, base, layer_cmpsr_id,
[ ... ]
> +	switch (pixel_alpha) {
> +	case DRM_MODE_BLEND_COVERAGE:
> +		blend_mode = 0x0;
> +		break;
> +	case DRM_MODE_BLEND_PREMULTI:
> +		blend_mode = 0x1;
> +		break;
> +	case DRM_MODE_BLEND_PIXEL_NONE:
> +		blend_mode = 0x0;
> +		break;
> +	default:
> +		drm_err(plane->dev, "unsupported blend mode for pixel alpha\n");
> +	}
> +
> +	if (state->fb->format && state->fb->format->has_alpha &&

[Severity: High]
Will this unconditionally dereference a NULL pointer for solid-fill planes?

During DRM atomic commits, a plane's state->fb is frequently NULL (e.g., when
a plane is being disabled, or for solid color fill planes). 

While the earlier code handles solid fills by skipping the RDMA block
(if (!solid_en)), this block evaluates state->fb->format unconditionally,
which will cause a kernel panic if state->fb is NULL.

> +	    pixel_alpha != DRM_MODE_BLEND_PIXEL_NONE) {
> +		if (alpha != 0xff)
> +			alpha_sel = 0x2;
> +		else
> +			alpha_sel = 0x1;
> +	} else {
> +		blend_mode = 0x0;
> +		alpha_sel = 0x0;
> +	}
> +
> +	cl_cmp = alloc_cmdlist_regs(CMPS_X_REG);

[Severity: High]
Can this trigger a "scheduling while atomic" panic or silent fallback
failures?

alloc_cmdlist_regs() is a macro that uses GFP_KERNEL allocations
(kzalloc(sizeof(*cl), GFP_KERNEL)). This plane update callback runs in atomic
context where sleeping is forbidden.

Additionally, if the allocation fails and returns NULL, the dpu_write() macro
silently falls back to immediate MMIO writes (dpu_write_reg_word). This
bypasses the command list's VBLANK synchronization, applying partial hardware
state immediately and causing visual tearing or lockups.

> +
> +	base = CMP_BASE_ADDR[a_crtc->dev_id];
[ ... ]
> +struct cmdlist_header {
> +	u64 next_list_addr : 39;
> +	/* reserved */
> +	u32: 1;
> +	u32 nod_len : 16;
> +	/* 1: the last cmdlist node, 2: pending node, otherwise 0 */
> +	u32 nod_type : 2;
> +	u32 next_nod_secu : 1;
> +	u64 wait_event_low    : 5;
> +	u64 wait_event    : 59;
> +	/* reserved */
> +	u32: 5;
> +};
> +
> +struct cmdlist_row {
> +	u32 module_cfg_addr   : 19;
> +	u32 module_cfg_strobe : 12;
> +	/* the last row tag = 1, else tag = 0. */
> +	u32 row_eof_tag       : 1;
> +	u32 module_regs[3];
> +};

[Severity: Medium]
Does using C bitfields for DMA-shared hardware descriptors break on different
compilers or endiannesses?

These structures format command list nodes directly into a DMA buffer consumed
by the hardware. The C standard leaves bitfield packing order and alignment
entirely implementation-defined. 

Mixing u64 and u32 bitfields makes the exact memory layout highly sensitive to
compiler ABIs and versions, and it will not work correctly across different
endiannesses. Could you use explicit bitwise operations with fixed-width types
instead?

> +
> +/* DMA stream layout consumed by the cmdlist engine; sizes are load-bearing. */
[ ... ]
> +void saturn_hee_cmdlist_fill_data_row(struct cmdlist *cl, u32 strobe,
> +				      u32 offset, u32 value[])
> +{
> +	struct cmdlist_row *row;
> +	u8 i;
> +
> +	/*
> +	 * packing counts rows blindly; drop the row rather than the buffer end
> +	 */
> +	if (WARN_ONCE(CL_HEADER_SZ + (cl->nod_len + 1) * CL_ROW_SZ > cl->size,
> +		      "cmdlist buffer full (rows %u)\n", cl->nod_len))
> +		return;
> +
> +	row = (struct cmdlist_row *)((char *)cl->va + CL_HEADER_SZ) + cl->nod_len;
> +	row->module_cfg_addr = offset >> 2;
> +	row->module_cfg_strobe = strobe;
> +
> +	for (i = 0; i < CMDLIST_ROW_REGS; i++)

[Severity: High]
Are row_eof_tag and un-strobed module_regs left uninitialized here?

This overlays a struct cmdlist_row onto the cl->va DMA buffer without
explicitly clearing or setting row->row_eof_tag. If the buffer is reused
across frames and the current frame is shorter than the previous one,
row_eof_tag could contain a stale 1 from the end of a previous commit's
list. 

If the hardware sees this uninitialized bitfield as 1, it will terminate
command processing prematurely, ignoring all subsequent commands in the current
frame and causing display hangs.

> +		if (strobe & CMDLIST_REG_STROBE(i))
[ ... ]
> +void saturn_hee_cmdlist_fill_conf_row(struct cmdlist *cl, struct spacemit_hw_device *hwdev,
> +				      u8 dev_id)
> +{
[ ... ]
> +	/* At most three rows are appended below; bail before overrunning. */
> +	if (WARN_ONCE(CL_HEADER_SZ + (cl->nod_len + 3) * CL_ROW_SZ > cl->size,
> +		      "cmdlist buffer full (type %u, rows %u)\n",
> +		      cl->type, cl->nod_len))
> +		return;

[Severity: High]
Does a desynchronized cl->nod_len cause the EOF tag to be dropped?

The calling code in cmdlist_regs_packing() unconditionally increments
cl->nod_len after calling saturn_hee_cmdlist_fill_data_row(), even if that
function silently dropped the row due to the buffer being full.

This causes cl->nod_len to artificially inflate beyond the actual rows
written. If it hits this WARN_ONCE, it aborts before appending the critical
configuration row containing row_eof_tag = 1. 

Could the hardware process an incomplete command list lacking an EOF tag,
thereby failing to signal the CFG_RDY interrupt?

> +
> +	header = (struct cmdlist_header *)(cl->va);

> diff --git a/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c
> index 144716ec9b80c.....
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c
[ ... ]
> +int saturn_cal_layer_fbcmem_size(struct drm_plane *plane,
> +				 struct drm_plane_state *state)
> +{
> +	struct spacemit_plane_state *pstate = to_spacemit_plane_state(state);
> +	u32 drm_4cc_fmt = pstate->state.fb->format->format;

[Severity: High]
Will this dereference a NULL pointer when pstate->state.fb is NULL?

Similar to the issue in saturn_hee_plane_update_hw_channel(), state->fb
can be NULL during plane disablement or solid-fill scenarios. This
unconditional dereference will cause a kernel panic in those cases.

> +	u32 crop_w = pstate->state.src_w >> 16;
> +
> +	/* Only linear sizing: the compressed path is not exposed. */

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