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

[Cody Kang <codykang.hk@gmail.com>]

On Sat, 25 Jul 2026 05:17:13 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing IS_ERR() check for 'match' causes an ERR_PTR dereference inside component_master_add_with_match().
> - [Medium] The atomic commit tail stalls the commit pipeline by waiting for flip completion before signaling hardware done.
> --
>
> --- Patch [11]: [PATCH 11/17] drm/spacemit: add Saturn DPU DRM device driver ---
> commit e12174b9614adeccb5a06f03b5944837004e96f4
> Author: Cody Kang <codykang.hk@gmail.com>
>
> drm/spacemit: add Saturn DPU DRM device driver
>
> This commit adds the DRM device for the SpacemiT K3 Saturn display processing
> unit. The SoC has two DPU instances, each driving one CRTC that scans out a
> single primary plane in linear RGB, feeding a downstream DP/eDP controller
> over an OF-graph endpoint.
>
> > diff --git a/drivers/gpu/drm/spacemit/spacemit_drm.c b/drivers/gpu/drm/spacemit/spacemit_drm.c
> > new file mode 100644
> > --- /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: Medium]
> Does calling drm_atomic_helper_wait_for_flip_done() before
> drm_atomic_helper_commit_hw_done() stall the commit pipeline?

Deliberate: the cmdlist engine reads the old state's DMA buffers
until the frame boundary, so the next commit must not start
programming (nor the old buffers be freed) before the flip
completes. Will add a comment in v2 saying so.

> By waiting for the flip completion before signaling hardware done, the CPU work
> for the next frame (such as state preparation and buffer pinning) is blocked
> until the hardware flip completes.
>
> Should commit_hw_done() be called before wait_for_flip_done() to avoid
> degrading atomic commit performance?
>
> > +
> > +	drm_atomic_helper_cleanup_planes(dev, old_state);
> > +}
>
> [ ... ]
>
> > +int spacemit_drm_of_component_probe(struct device *dev)
> > +{
> > +	struct device_node *ports, *port, *ep;
> > +	struct device_node *remote;
> > +	struct component_match *match = NULL;
>
> [ ... ]
>
> > +	for_each_child_of_node(ports, port) {
> > +		if (!of_node_name_eq(port, "port"))
> > +			continue;
> > +		for_each_child_of_node(port, ep) {
> > +			if (!of_node_name_eq(ep, "endpoint"))
> > +				continue;
> > +			remote = of_graph_get_remote_port_parent(ep);
> > +			if (!remote)
> > +				continue;
> > +			if (of_device_is_available(remote))
> > +				drm_of_component_match_add(dev, &match,
> > +							   compare_of, remote);
> > +			of_node_put(remote);
> > +		}
> > +	}
> > +	of_node_put(ports);
> > +
> > +	if (!match) {
> > +		dev_err(dev, "no downstream component available\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return component_master_add_with_match(dev, &spacemit_drm_master_ops,
> > +					       match);
>
> [Severity: High]
> Can match be an ERR_PTR here instead of just NULL?

Real; will add the IS_ERR() check in v2.

Cody

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