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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Usage of sleep-capable functions in atomic CRTC and plane hardware programming callbacks.
- [High] Unchecked return value of `clk_round_rate` leads to hardware misconfiguration.
- [High] Data race on `flip_done` between the timeout handler and ISR leading to vblank refcount underflow.
- [High] Unconditional dereference of `state->fb` triggers kernel panic for solid fill planes.
- [High] When the cmdlist buffer overflows, `cl->nod_len` is incremented despite the row write being aborted, causing the hardware to read out-of-bounds DMA memory.
- [High] The `cl_rdma` register buffer is never packed into the cmdlist and gets corrupted when shared across planes.
- [High] Signed plane coordinates are directly cast to u32, causing massive out-of-bounds register programming when off-screen.
- [Medium] The `INT_VSYNC_UPDATE` interrupt is missing from the `irq_list` mapping array, preventing its status from being cleared.
--

--- Patch [9]: [PATCH 09/17] drm/spacemit: add Saturn DPU hardware backend ---
commit 1cf09ad0cc7d13639bcbfc63e90d8f431590bfbd
Author: Cody Kang <codykang.hk@gmail.com>

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. The
backend is exposed as an ops table and hardware descriptor to be
aggregated by the KMS layer in an upcoming patch.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
> index 0000000000000..8d5f13bc5373c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
[ ... ]
> +static int dpu_enable_clocks(struct spacemit_crtc *a_crtc)
> +{
> +	struct dpu_clk_context *clk_ctx = &a_crtc->clk_ctx;
> +	struct drm_crtc *crtc = &a_crtc->crtc;
> +	struct drm_display_mode *mode = &crtc->mode;
> +	u64 clk_val;
> +	u64 set_clk_val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(clk_ctx->pxclk);
> +	if (ret)
> +		goto err;
> +	set_clk_val = mode->clock * 1000;
> +	if (set_clk_val) {
> +		set_clk_val = clk_round_rate(clk_ctx->pxclk, set_clk_val);

[Severity: High]
Does this code properly handle negative error codes from clk_round_rate()?

If clk_round_rate() fails and returns an error like -EINVAL, the unchecked
value is cast directly to u64 set_clk_val. This will sign-extend the error
code into a massive positive integer before it gets passed to clk_set_rate(),
which can lead to hardware misconfiguration.

> +		clk_val = clk_get_rate(clk_ctx->pxclk);
> +		if (clk_val != set_clk_val) {
> +			clk_set_rate(clk_ctx->pxclk, set_clk_val);
> +			drm_dbg(crtc->dev, "pxclk=%lld\n", clk_val);
> +		}
> +	}
[ ... ]
> +static inline void dpu_isr_vblank(struct spacemit_crtc *a_crtc, bool *flip)
> +{
> +	struct drm_crtc *crtc = &a_crtc->crtc;
> +	struct drm_device *drm = crtc->dev;
> +	struct drm_pending_vblank_event *event;
> +
> +	drm_crtc_handle_vblank(crtc);
> +
> +	if (*flip)
> +		return;
> +	*flip = true;

[Severity: High]
Can this race condition cause a vblank reference underflow?

The timer handler spacemit_cfg_rdy_timer_handler() sets flip_done to true and
calls drm_crtc_vblank_put() asynchronously.

If the ISR runs concurrently, both threads can observe flip_done as false
due to the lockless TOCTOU check here. They will both set it to true and
drop the vblank reference, putting the reference twice.

> +
> +	/*
> +	 * event_lock serialises this against the commit arming
> +	 * crtc->state->event.
> +	 */
> +	spin_lock(&drm->event_lock);
[ ... ]
> +static u32 dpu_online_isr(struct spacemit_crtc *a_crtc)
> +{
> +	u32 irq_raw, irq_bit, irq_ur_bit;
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	int dev_id = a_crtc->dev_id;
> +
> +	trace_dpu_isr(a_crtc->dev_id);
[ ... ]
> +	irq_bit = hwdev->get_irq_bit(INT_REST, dev_id) | irq_ur_bit;
> +	if (irq_raw & irq_bit)
> +		hwdev->clr_int_sts(a_crtc, irq_bit, dev_id);
> +
> +	irq_bit = hwdev->get_irq_bit(INT_VSYNC_UPDATE, dev_id);
> +	if (irq_raw & irq_bit)
> +		hwdev->clr_int_sts(a_crtc, irq_bit, dev_id);

[Severity: Medium]
Is it possible to clear the INT_VSYNC_UPDATE interrupt?

The irq_list array in dpu_saturn_hee.c only defines up to INT_REST.

When get_irq_bit() is called with INT_VSYNC_UPDATE (index 5), it triggers
the ARRAY_SIZE(irq_list) check and returns 0. Consequently, the interrupt
status can never be cleared by this handler.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
> index 0000000000000..c23e4ca36d26b
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
[ ... ]
> +void saturn_hee_plane_update_hw_channel(struct drm_plane *plane)
> +{
> +	struct drm_plane_state *state = plane->state;
> +	struct spacemit_crtc *a_crtc = to_spacemit_crtc(state->crtc);
> +	struct drm_framebuffer *fb = plane->state->fb;
[ ... ]
> +	src_w = state->src_w >> 16;
> +	src_h = state->src_h >> 16;
> +	src_x = state->src_x >> 16;
> +	src_y = state->src_y >> 16;
> +
> +	crtc_w = state->crtc_w;
> +	crtc_h = state->crtc_h;
> +	crtc_x = state->crtc_x;
> +	crtc_y = state->crtc_y;

[Severity: High]
Can this cause massive out-of-bounds register programming if the plane is
partially off-screen?

The plane coordinates state->crtc_x and state->crtc_y are signed 32-bit
integers. Directly assigning them to unsigned 32-bit variables will cause
negative values to overflow into extremely large positive integers.

When these are written to the hardware area control registers, it creates
invalid constraints like area_left being greater than area_right.

> +
> +	drm_dbg(plane->dev, "crtc_x %u crtc_y %u\n", crtc_x, crtc_y);
> +
> +	if (rdma_id == RDMA_INVALID_ID)
> +		solid_en = true;
> +
> +	trace_dpu_plane_info(state, fb, rdma_id, alpha, state->rotation);
> +
> +	/* 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]
Is the cl_rdma register buffer ever flushed to the command list?

RDMA configurations are written to a_crtc->cl_rdma, but cmdlist_regs_packing()
is never called for this buffer, meaning the configurations are discarded and
never reach the hardware.

Furthermore, does sharing a_crtc->cl_rdma across planes corrupt the state?
When dpu_write() is called, it unconditionally overwrites cl_p->base. If
multiple planes are updated concurrently, the last plane will overwrite the
base address for all prior planes' registers in this shared buffer.

> +		dpu_write(hwdev, RDMA_PATH_X_REG, base, layer_cmpsr_id,
> +			  a_crtc->dev_id, cl_rdma);
[ ... ]
> +	} else {
> +		/* Constant-fill layer: zero-size source, transparent black. */
> +		solid_r = 0;
> +		solid_g = 0;
> +		solid_b = 0;
> +		solid_a = 0;
> +	}
> +
> +	switch (pixel_alpha) {
[ ... ]
> +	}
> +
> +	if (state->fb->format && state->fb->format->has_alpha &&

[Severity: High]
Will this crash if the plane is a solid fill plane?

In DRM, solid fill planes do not require an attached framebuffer, meaning
state->fb can be NULL. Unconditionally dereferencing state->fb->format here
without verifying if state->fb is valid will trigger a kernel panic.

> +	    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]
Since plane atomic update callbacks execute in atomic context during
non-blocking commits, is it safe to allocate memory here?

The alloc_cmdlist_regs() macro uses kzalloc() with GFP_KERNEL. Calling
sleeping functions from an atomic context causes kernel warnings,
system instability, and potential deadlocks.

> +
> +	base = CMP_BASE_ADDR[a_crtc->dev_id];
> +	dpu_write(hwdev, CMPS_X_REG, base, dst_w, mode->hdisplay, cl_cmp);
[ ... ]
> +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;

[Severity: High]
Does this correctly handle command list buffer overflows?

When the cmdlist buffer is full, this function issues a warning and returns
early without writing the row.

However, its caller in cmdlist_regs_packing() ignores this failure and
still increments cl->nod_len. This artificially inflates the length in the
header. When the DPU hardware parses the cmdlist, it will attempt to read
out-of-bounds DMA memory.

> +
> +	row = (struct cmdlist_row *)((char *)cl->va + CL_HEADER_SZ) + cl->nod_len;
[ ... ]
> diff --git a/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c
> index 0000000000000..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 crash if the plane is a solid fill plane?

In DRM, solid fill planes do not require an attached framebuffer, meaning
state->fb can be NULL. Unconditionally dereferencing state->fb->format here
without verifying if state->fb is valid will trigger a kernel panic.

> +	u32 crop_w = pstate->state.src_w >> 16;
> +
> +	/* Only linear sizing: the compressed path is not exposed. */
> +	return get_raw_data_plane_rdma_mem_size(drm_4cc_fmt, crop_w,
> +						&pstate->fbcmem_size);
> +}

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