Re: [PATCH 08/17] drm/spacemit: add Saturn DPU core types, cmdlist and display MMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Passing exhausted NULL pointer to hardware programming function.
- [High] NULL pointer dereference when accessing state for disabled planes due to stale `in_use` flag.
- [High] DMMU page tables map multi-planar framebuffers out-of-bounds by exclusively using the first GEM object.
- [High] Hardware DMA state buffers are reused across atomic commits without double-buffering.
- [High] Missing bounds check in `cmdlist_regs_packing` causes DMA buffer overflow.
- [Medium] Unconditional dereference of `fb` in `dpu_plane_info` tracepoint.
--

--- Patch [8]: [PATCH 08/17] drm/spacemit: add Saturn DPU core types, cmdlist and display MMU ---
commit 82f0596ca65ff96e2081646dccab37f7bb9d1c0c
Author: Cody Kang <codykang.hk@gmail.com>
Subject: drm/spacemit: add Saturn DPU core types, cmdlist and display MMU

This commit introduces the Saturn DPU driver core types, register accessors,
and MMU table code. It implements a batched register programming model using
in-memory command lists mapped through the DPU's private address translation
unit.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_trace.h b/drivers/gpu/drm/spacemit/dpu/dpu_trace.h
> new file mode 100644
> index 0000000000000..abfe34a67a4bd
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_trace.h
[ ... ]
> +TRACE_EVENT(dpu_plane_info,
> +	    TP_PROTO(struct drm_plane_state *state, struct drm_framebuffer *fb,
> +		     u32 rdma_id, u32 alpha, u32 rotation),
> +	TP_ARGS(state, fb, rdma_id, alpha, rotation),
> +	TP_STRUCT__entry(
> +			__field(u32, rdma_id)
> +			__field(u32, src_w)
> +			__field(u32, src_h)
> +			__field(u32, src_x)
> +			__field(u32, src_y)
> +			__field(u32, crtc_w)
> +			__field(u32, crtc_h)
> +			__field(u32, crtc_x)
> +			__field(u32, crtc_y)
> +			__field(u32, width)
> +			__field(u32, height)
> +			__field(u32, format)
> +			__field(u32, blend_mode)
> +			__field(u32, alpha)
> +			__field(u32, zpos)
> +			__field(u32, rotation)
> +	),
> +	TP_fast_assign(
> +			__entry->rdma_id = rdma_id;
> +			__entry->src_w = state->src_w >> 16;
> +			__entry->src_h = state->src_h >> 16;
> +			__entry->src_x = state->src_x >> 16;
> +			__entry->src_y = state->src_y >> 16;
> +			__entry->crtc_w = state->crtc_w;
> +			__entry->crtc_h = state->crtc_h;
> +			__entry->crtc_x = state->crtc_x;
> +			__entry->crtc_y = state->crtc_y;
> +			__entry->width = fb->width;
> +			__entry->height = fb->height;
> +			__entry->format = fb->format->format;

[Severity: Medium]
If this tracepoint fires during a plane disable operation, will the fb
parameter be NULL? Since state->fb is typically NULL during a disable, it
looks like evaluating fb->format->format here could cause a kernel panic
when tracing is enabled.

> +			__entry->blend_mode = state->pixel_blend_mode;
> +			__entry->alpha = alpha;
> +			__entry->zpos = state->zpos;
> +			__entry->rotation = rotation;
> +	),
> +	TP_printk("rdma_id=%d src: w=%d h=%d x=%d y=%d crtc: w=%d h=%d x=%d y=%d width=%d height=%d fmt=0x%x blend=%d alpha=%d zpos=%d rot=%d",
[ ... ]
> diff --git a/drivers/gpu/drm/spacemit/spacemit_cmdlist.c b/drivers/gpu/drm/spacemit/spacemit_cmdlist.c
> new file mode 100644
> index 0000000000000..787d38e9e135f
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_cmdlist.c
[ ... ]
> +void cmdlist_regs_packing(struct cmdlist *cl, enum cmdlist_mode_type mod,
> +			  struct cmdlist_regs *cl_regs)
> +{
[ ... ]
> +		if (cl_regs->flags[i]) {
> +			offset = cl_regs->base + i * sizeof(u32);
> +			for (u8 j = 0; j < CMDLIST_ROW_REGS; j++) {
> +				index = i + j;
> +				if (likely(index < cl_regs->size)) {
> +					if (cl_regs->flags[index]) {
> +						value[j] = reg_base[index];
> +						regs_in_row++;
> +						strobe |= CMDLIST_REG_STROBE(j);
> +					}
> +				} else {
> +					break;
> +				}
> +			}
> +			hwdev->cmdlist_fill_data_row(cl, strobe, offset, value);
> +			/*
> +			 * the row covers the whole window; a shorter stride
> +			 * would revisit covered words as duplicate rows
> +			 */
> +			i += CMDLIST_ROW_REGS;
> +			cl->nod_len++;

[Severity: High]
Is there a bounds check missing here to prevent cl->nod_len from exceeding
cl->size? If cl is shallow-copied and reused across commits, nod_len might
accumulate and eventually overflow the 4096-byte DMA buffer, overwriting
adjacent physical memory.

> +		} else {
> +			i++;
> +		}
> +	}
> +
> +	drm_dbg(a_crtc->crtc.dev, "row_num = %d\n", cl->nod_len);
> +}
[ ... ]
> +void cmdlist_atomic_commit(struct drm_crtc *crtc,
> +			   struct drm_crtc_state *old_state)
> +{
> +	int i;
> +	struct cmdlist *cur_cl, *first_cl;
> +	struct spacemit_crtc *a_crtc = NULL;
> +	u32 chy, addrl, addrh;
> +	struct spacemit_drm_private *priv = crtc->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	struct spacemit_crtc_rdma *cur_rdmas =
> +			to_spacemit_crtc_state(crtc->state)->rdmas;
> +	struct spacemit_crtc_rdma *old_rdmas =
> +			to_spacemit_crtc_state(old_state)->rdmas;
> +
> +	a_crtc = to_spacemit_crtc(crtc);
> +
> +	for (i = 0; i < hwdev->rdma_nums; i++) {
> +		if (old_rdmas[i].in_use)
> +			hwdev->enable_cmdlist(a_crtc, hwdev, i, false);
> +
> +		if (cur_rdmas[i].in_use) {
> +			drm_dbg(crtc->dev, "cmdlist group = %d\n", i);
> +			cur_cl = priv->cmdlist_groups[i];
> +			first_cl = cur_cl;
> +			while (cur_cl) {
> +				hwdev->cmdlist_fill_conf_row(cur_cl, hwdev,
> +					CMDLIST_CMP_INVALID);
> +				cmdlist_dump_node(cur_cl);
> +				cur_cl = cur_cl->next;
> +			}
> +			chy = cl_to_spacemit_pstate(first_cl)->state.crtc_y;

[Severity: High]
Could first_cl be NULL here for a disabled plane? During atomic state
duplication, it seems the in_use flag is inherited. If a plane is disabled,
it is removed from the plane_mask and cmdlist_sort_by_group skips clearing
the flag. This would cause cur_cl and first_cl to be NULL, leading
cl_to_spacemit_pstate to apply container_of to a NULL pointer and trigger a
negative offset dereference.

> +			addrl = (lower_32_bits(priv->cmdlist_groups[i]->pa) &
> +				 CMDLIST_ADDRL_ALIGN_MASK) >> CMDLIST_ADDRL_ALIGN_BITS;
> +
> +			addrh = upper_32_bits(priv->cmdlist_groups[i]->pa);
> +			hwdev->cfg_cmdlist(hwdev, i, chy, addrl, addrh);
> +			hwdev->enable_cmdlist(a_crtc, hwdev, i, true);
> +			priv->cmdlist_groups[i] = NULL;
> +		}
> +	}
> +}
[ ... ]
> +void crtc_cmdlist_atomic_commit(struct spacemit_crtc *a_crtc)
> +{
> +	struct cmdlist *cur_cl;
> +	struct drm_crtc *crtc = &a_crtc->crtc;
> +	struct spacemit_drm_private *priv = crtc->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	u8 crtc_id = drm_crtc_index(crtc);
> +	u8 dev_id = a_crtc->dev_id;
> +
> +	drm_dbg(crtc->dev, "cmdlist group = %d\n",
> +		hwdev->rdma_nums + crtc_id);
> +	cur_cl = priv->cmdlist_groups[hwdev->rdma_nums + crtc_id];
> +	while (cur_cl) {
> +		hwdev->cmdlist_fill_conf_row(cur_cl, hwdev, dev_id);
> +		cmdlist_dump_node(cur_cl);
> +		cur_cl = cur_cl->next;
> +	}
> +	hwdev->crtc_cmdlist(cur_cl, hwdev, priv, crtc_id, dev_id);

[Severity: High]
Does this loop exhaust cur_cl to NULL before passing it to
hwdev->crtc_cmdlist? Once the while loop finishes, cur_cl is guaranteed to be
NULL, which seems like it would pass an invalid pointer to the hardware
programming function instead of the intended command list head.

> +}
> +
> +/* Called from atomic_check(), where -ENOMEM is still an answer. */
> +int spacemit_cmdlist_alloc(struct device *dev, struct cmdlist *cl)
> +{
> +	if (cl->va)
> +		return 0;
> +
> +	cl->size = PER_CMDLIST_SIZE;
> +	cl->va = dma_alloc_coherent(dev, cl->size, &cl->pa,
> +				    GFP_KERNEL | __GFP_ZERO);

[Severity: High]
Does bypassing reallocation when cl->va is present cause the active hardware
state to be overwritten during the prepare phase of a non-blocking commit?
It looks like writing new register sequences directly into this reused buffer
could corrupt the in-flight display state being scanned out by the hardware.

> +	if (!cl->va)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/spacemit/spacemit_dmmu.c b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
> new file mode 100644
> index 0000000000000..2ae30dd1a4f23
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
[ ... ]
> +int spacemit_dmmu_map(struct drm_framebuffer *fb, struct dpu_mmu_tbl *mmu_tbl,
> +		      u8 tbu_id, struct cmdlist_regs *cl_rdma,
> +		 struct drm_plane *plane)
> +{
> +	struct spacemit_drm_private *priv = fb->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	const struct drm_format_info *format = NULL;
> +	struct sg_table *sgt = NULL;
> +	u32 total_size, offset1, offset2;
> +	struct tbu_instance tbu = { };
> +	u8 plane_num;
> +	u32 val;
> +
> +	format = fb->format;
> +	sgt = drm_gem_shmem_get_pages_sgt(to_drm_gem_shmem_obj(fb->obj[0]));
> +	if (IS_ERR(sgt))
> +		return PTR_ERR(sgt);
> +
> +	plane_num = format->num_planes;
> +
> +	total_size = roundup(fb->obj[0]->size, PAGE_SIZE);
> +
> +	offset1 = plane_num > 1 ? fb->offsets[1] : total_size;
> +	offset2 = plane_num > 2 ? fb->offsets[2] : total_size;
> +
> +	switch (plane_num) {
> +	case 3:
> +		tbu.ttb_pa[2] = mmu_tbl->pa + (offset2 >> PAGE_SHIFT) * 4;
> +		tbu.tbu_va[2] = TBU_BASE_VA(tbu_id) + offset2;
> +		tbu.ttb_size[2] = PAGE_ALIGN(total_size - rounddown(offset2, PAGE_SIZE))
> +				  >> PAGE_SHIFT;
> +		fallthrough;
> +	case 2:
> +		tbu.ttb_pa[1] = mmu_tbl->pa + (offset1 >> PAGE_SHIFT) * 4;
> +		tbu.tbu_va[1] = TBU_BASE_VA(tbu_id) + offset1;
> +		tbu.ttb_size[1] = PAGE_ALIGN(offset2 - rounddown(offset1, PAGE_SIZE)) >> PAGE_SHIFT;

[Severity: High]
For multi-planar formats with discrete GEM objects, does this map the
secondary planes out of bounds? The scatter/gather table is fetched strictly
from fb->obj[0]. Applying secondary offsets to the physical pages of the
first GEM object might break hardware isolation.

Could the size calculation offset2 - rounddown(offset1, PAGE_SIZE) also
underflow if these offsets belong to separate, un-ordered buffers?

> +		fallthrough;
> +	case 1:
> +		tbu.ttb_pa[0] = mmu_tbl->pa;
> +		tbu.tbu_va[0] = TBU_BASE_VA(tbu_id);
> +		tbu.ttb_size[0] = PAGE_ALIGN(offset1) >> PAGE_SHIFT;
> +		fallthrough;
> +	default:
> +		break;
> +	}
> +	spacemit_dmmu_fill_pgtable(mmu_tbl, sgt);
[ ... ]

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