Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The driver calls sleeping delay functions (`usleep_range()`) from within CRTC atomic enable and disable callbacks, violating DRM atomic context rules. - [Medium] Setting drm_mode_config.max_width and max_height to a single CRTC's maximum resolution breaks multi-monitor support. - [Medium] Missing error check after `drm_of_component_match_add()` inside the OF component loop causes device node reference leaks on subsequent iterations. --
--- Patch [11]: [PATCH v2 11/17] drm/spacemit: add Saturn DPU DRM device driver --- commit cbbaf4042bf32b29313b65d5c670b16c44e0fb03 Author: Cody Kang <[email protected]> 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 > --- /dev/null > +++ b/drivers/gpu/drm/spacemit/spacemit_drm.c [ ... ] > +int spacemit_drm_mode_config_init(struct drm_device *drm) > +{ > + int ret; > + > + ret = drmm_mode_config_init(drm); > + if (ret) > + return ret; > + > + drm->mode_config.min_width = 1; > + drm->mode_config.min_height = 1; > + /* K3 user manual: each DPU scans out at most 3840x2160@60 */ > + drm->mode_config.max_width = 3840; > + drm->mode_config.max_height = 2160; [Severity: Medium] Will setting max_width and max_height to a single CRTC's maximum resolution break multi-monitor support? These values dictate the maximum dimensional bounds for the entire DRM framebuffer, not just a single CRTC's scanout. Windowing systems allocating a single large framebuffer to span all connected monitors might fail if they attempt to configure side-by-side dual monitors exceeding 3840x2160. > + > + drm->mode_config.funcs = &spacemit_drm_mode_config_funcs; > + drm->mode_config.helper_private = &spacemit_drm_mode_config_helper; > + > + return 0; > +} [ ... ] > +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); [Severity: Medium] Could this loop leak an OF device node reference on subsequent iterations if drm_of_component_match_add() fails and returns an error pointer? If drm_of_component_match_add() encounters a memory allocation failure, it sets the match pointer to ERR_PTR(-ENOMEM). Because this loop does not check IS_ERR(match) to break out early, subsequent iterations will pass the error pointer back into the function. Internally, drm_of_component_match_add() unconditionally calls of_node_get() to increment the reference count, but component_match_add_release() immediately returns without registering a release callback when the match pointer is already an error. > + of_node_put(remote); > + } > + } [ ... ] > diff --git a/drivers/gpu/drm/spacemit/spacemit_crtc.c > b/drivers/gpu/drm/spacemit/spacemit_crtc.c > --- a/drivers/gpu/drm/spacemit/spacemit_crtc.c > +++ b/drivers/gpu/drm/spacemit/spacemit_crtc.c [ ... ] > static void spacemit_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_atomic_commit *old_state) > { > struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc); > struct spacemit_drm_private *priv = crtc->dev->dev_private; > int idx; > int ret; > > if (!drm_dev_enter(crtc->dev, &idx)) > return; > > trace_spacemit_crtc_atomic_enable(a_crtc->dev_id); > > /* > * U-Boot leaves its boot-logo pipeline running. Hard-stop it with the > * resets alone; the clocks are not ours to gate until we have enabled > * them. > */ > if (unlikely(a_crtc->first_modeset)) { > ret = reset_control_bulk_assert(ARRAY_SIZE(priv->pipe_resets), > priv->pipe_resets); > if (ret) > dev_warn(a_crtc->dev, > "failed to assert resets: %d\n", ret); > usleep_range(10000, 20000); [Severity: High] Does this code call a sleeping function from within atomic context? CRTC atomic enable callbacks execute in atomic context, where sleeping is strictly forbidden. Calling usleep_range() here can cause kernel warnings, system instability, and potential deadlocks. > a_crtc->first_modeset = false; > } [ ... ] > static int dpu_pm_suspend(struct device *dev) > { > struct spacemit_drm_private *priv = dev_get_drvdata(dev); > struct spacemit_crtc *a_crtc = priv->a_crtc; > int ret; > > /* > * Assert before gating: a reset asserted into an already-gated block > * has no clock edges to propagate on and leaves the register file > * untouched. > */ > ret = reset_control_bulk_assert(ARRAY_SIZE(priv->pipe_resets), > priv->pipe_resets); > if (ret) > dev_warn(dev, "failed to assert resets: %d\n", ret); > usleep_range(10000, 20000); [Severity: High] Is this sleeping function also reached from atomic context? Because dpu_pm_suspend() is called during CRTC atomic disable (via spacemit_crtc_atomic_disable()), this usleep_range() call also occurs in an atomic context where sleeping is forbidden. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
