Re: [PATCH v2 17/23] drm/msm/dcp: Register DCP display pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/msm/dcp: dcp_dpu_kms_cleanup_bridges() called under component_mutex
- [High] drm/msm/dcp: missing remote port check and sequential bridge lookup
- [High] drm/msm/dcp: race and runtime PM leak in dcp_dpu_kms_enable_commit()
- [High] drm/msm/dcp: unbalanced msm_kms and runtime PM teardown in dcp_dpu_kms_destroy()
- [High] drm/msm/dcp: dangling CRTC and drm_device pointers in dcp_dpu_kms_destroy()
- [Medium] drm/msm/dcp: invalid possible_crtcs mask when max_crtc_count is clamped
--

commit 5fc21d18f27dcbb7ac8d64f95bc52703e58d9984
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Register DCP display pipeline

This commit registers the DCP display pipeline using firmware capabilities
and the device-tree display graph, integrating runtime PM and IOMMU setup and
creating the DRM planes, CRTCs, encoders, bridges, and connectors.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c
> index 61a4d85653594..8a73a99b2c763 100644
> --- a/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c
> +++ b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c

[ ... ]

> @@ -59,3 +78,609 @@ bool dcp_dpu_kms_needs_dirtyfb(struct dcp_dpu_kms *dcp_kms,
>  
>  	return false;
>  }
> +
> +void dcp_dpu_kms_cleanup_bridges(struct dcp_dpu_kms *dcp_kms)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dcp_kms->dsi_bridge); i++) {
> +		if (!dcp_kms->dsi_bridge[i])
> +			continue;
> +
> +		dcp_dsi_bridge_cleanup(dcp_kms->dsi_bridge[i]);
> +		dcp_kms->dsi_bridge[i] = NULL;
> +	}
> +}

[Severity: High]
Will calling dcp_dpu_kms_cleanup_bridges() from dcp_dpu_kms_setup_bridges(),
dcp_dpu_setup_pipeline(), or dcp_dpu_kms_destroy() deadlock on
component_mutex and break deferred probe retries?

Before registering the aggregate component master, dcp_component_register()
calls dcp_dpu_kms_init_dsi_bridges() -> dcp_dsi_bridge_init(), which
registers dsi_host_dev and mipi_dsi_host and adds each dsi_np to the
component match list. When the DSI panel attaches, dcp_dsi_host_attach()
calls component_add(&dsi_bridge->dsi_host_dev, &dcp_dsi_component_ops).

Once all components are added, drivers/base/component.c holds the non-
recursive component_mutex across both aggregate bind and unbind:

  try_to_bring_up_aggregate_device() [holds component_mutex]
    -> msm_drm_bind()
      -> msm_drm_init()
        -> msm_drm_kms_init()
          -> dcp_dpu_kms_hw_init()
            -> dcp_dpu_drm_obj_init()
              -> dcp_dpu_setup_pipeline()
                -> dcp_dpu_kms_setup_bridges()

  take_down_aggregate_device() [holds component_mutex]
    -> msm_drm_unbind()
      -> msm_drm_uninit()
        -> msm_drm_kms_uninit()
          -> dcp_dpu_kms_destroy()

When dcp_dpu_kms_cleanup_bridges() runs from any of those paths, it calls:

  dcp_dsi_bridge_cleanup()
    -> mipi_dsi_host_unregister()
      -> mipi_dsi_remove_device_fn()
        -> mipi_dsi_detach()
          -> dcp_dsi_host_detach()
            -> component_del(&dsi_bridge->dsi_host_dev, ...)

which tries to acquire component_mutex while it is already held by the
calling thread.

In addition, in msm_drm_uninit(), msm_drm_kms_uninit() runs before
component_unbind_all() and drm_dev_put(), so dcp_dpu_kms_destroy() tears down
the bridges and deletes dsi_host_dev while it is still bound as a component
and attached to encoder->bridge_chain. Destroying and clearing
dcp_kms->dsi_bridge[] on error or -EPROBE_DEFER in dcp_dpu_kms_setup_bridges()
or dcp_dpu_setup_pipeline() also prevents any subsequent deferred probe retry
from succeeding because dcp_dpu_kms_init_dsi_bridges() only runs once before
component_master_add_with_match().

[ ... ]

> +static void dcp_dpu_kms_enable_commit(struct msm_kms *kms)
> +{
> +	struct dcp_dpu_kms *dcp_kms = to_dcp_dpu_kms(kms);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dcp_kms->pm_dev);
> +	if (ret < 0) {
> +		dcp_kms->commit_pm_active = false;
> +		drm_err_ratelimited(dcp_kms->dev,
> +				    "pm_runtime_resume_and_get failed: %d\n",
> +				    ret);
> +		return;
> +	}
> +
> +	dcp_kms->commit_pm_active = true;
> +}
> +
> +static void dcp_dpu_kms_disable_commit(struct msm_kms *kms)
> +{
> +	struct dcp_dpu_kms *dcp_kms = to_dcp_dpu_kms(kms);
> +
> +	if (dcp_kms->commit_pm_active) {
> +		pm_runtime_put_autosuspend(dcp_kms->pm_dev);
> +		dcp_kms->commit_pm_active = false;
> +	}
> +}

[Severity: High]
Can overlapping commits race on dcp_kms->commit_pm_active and leak a runtime
PM reference on dcp_kms->pm_dev?

msm_atomic_commit_tail() calls kms->funcs->enable_commit(kms) before
lock_crtcs(kms, crtc_mask) and calls kms->funcs->disable_commit(kms) after
unlock_crtcs(kms, crtc_mask), while msm_atomic_async_commit() also calls
enable_commit() and disable_commit() from per-CRTC worker threads, and
commits on separate CRTCs execute concurrently.

If two commits overlap between dcp_dpu_kms_enable_commit() and
dcp_dpu_kms_disable_commit():

1. Both commits call pm_runtime_resume_and_get(dcp_kms->pm_dev) and set the
   single shared boolean dcp_kms->commit_pm_active = true.
2. When the first commit finishes and calls dcp_dpu_kms_disable_commit(), it
   calls pm_runtime_put_autosuspend(dcp_kms->pm_dev) once and sets
   dcp_kms->commit_pm_active = false.
3. The second in-flight commit then sees !dcp_kms->commit_pm_active in
   dcp_crtc_atomic_begin() (failing the commit with cstate->hfi_error = -EIO),
   and when it reaches dcp_dpu_kms_disable_commit() it sees
   dcp_kms->commit_pm_active == false and skips
   pm_runtime_put_autosuspend(dcp_kms->pm_dev), leaking its runtime PM usage
   count.

[ ... ]

> +static void dcp_dpu_kms_destroy(struct msm_kms *kms)
> +{
> +	struct dcp_dpu_kms *dcp_kms = to_dcp_dpu_kms(kms);
> +
> +	dcp_dpu_kms_cleanup_bridges(dcp_kms);
> +
> +	if (kms->vm) {
> +		struct msm_gem_vm *msm_vm = to_msm_vm(kms->vm);
> +
> +		if (msm_vm->mmu && msm_vm->mmu->funcs->detach) {
> +			drm_dbg_driver(dcp_kms->dev,
> +				       "detaching IOMMU domain from dcp-dpus\n");
> +			msm_vm->mmu->funcs->detach(msm_vm->mmu);
> +		}
> +
> +		drm_gpuvm_put(kms->vm);
> +		kms->vm = NULL;
> +	}
> +
> +	if (dcp_kms->rpm_enabled) {
> +		pm_runtime_disable(dcp_kms->pm_dev);
> +		dcp_kms->rpm_enabled = false;
> +	}
> +
> +	msm_kms_destroy(&dcp_kms->base);
> +}

[Severity: High]
Is the teardown in dcp_dpu_kms_destroy() balanced with dcp_kms_init() across
deferred probe or component rebind?

In msm_drm_init(), msm_drm_kms_init() calls priv->kms_init(ddev)
(dcp_kms_init()) and kms->funcs->hw_init(kms) (dcp_dpu_kms_hw_init()). If
msm_drm_kms_init() fails (for instance with -EPROBE_DEFER when a panel is not
ready in dcp_dpu_kms_setup_bridges()) or when the aggregate device unbinds,
msm_drm_uninit() calls msm_drm_kms_uninit(), which flushes kms->wq and calls
dcp_dpu_kms_destroy().

However, msm_kms_init(&dcp_kms->base, &dcp_dpu_kms_funcs) and
pm_runtime_enable(&drv->dcp_subsystem_dev->dev) are only called once during
virtio probe (in dcp_virtio_probe() and dcp_subsystem_dev_init()), not in
dcp_kms_init(). When dcp_dpu_kms_destroy() runs:

- pm_runtime_disable(dcp_kms->pm_dev) disables runtime PM on pm_dev (and on
  driver removal, dcp_subsystem_dev_deinit() calls pm_runtime_disable() a
  second time on the same device).
- msm_kms_destroy(&dcp_kms->base) destroys kms->wq and kms->pending_timers[],
  leaving kms->wq as a dangling pointer that will be flushed and destroyed
  again on a subsequent bind attempt or uninit.

[Severity: High]
Does dcp_dpu_kms_destroy() (and the error path in dcp_dpu_setup_pipeline())
leave dangling CRTC and drm_device pointers reachable from HFI event
callbacks?

dcp_kms_init() sets dcp_kms->dev = ddev and drv->kms = dcp_kms, and
dcp_dpu_setup_pipeline() populates dcp_kms->crtcs[] with CRTCs allocated via
drmm_kzalloc(ddev, ...) in dcp_crtc_init() and increments dcp_kms->num_crtcs.

When msm_drm_uninit() runs on init failure, component unbind, or during
dcp_virtio_remove() -> dcp_component_unregister() before
dcp_virtio_hfi_cleanup() stops the VirtIO queues, drm_dev_put(ddev) frees
ddev and all drmm-managed dcp_crtc instances while dcp_kms and drv remain
allocated. Because neither dcp_dpu_kms_destroy() nor
dcp_dpu_setup_pipeline()'s error path clears dcp_kms->num_crtcs,
dcp_kms->crtcs[], dcp_kms->dev, or drv->kms, an incoming
HFI_COMMAND_DISPLAY_EVENT_VSYNC or
HFI_COMMAND_DISPLAY_EVENT_FRAME_SCAN_COMPLETE message in
dcp_hfi_resp_callback() will invoke dcp_crtc_hfi_vblank_cb() or
dcp_crtc_hfi_frame_done_cb() and dereference the freed dcp_crtc or drm_device.

[ ... ]

> +int dcp_dpu_kms_init_dsi_bridges(struct dcp_dpu_kms *dcp_kms,
> +				 struct component_match **matchptr)
> +{
> +	struct device_node *dpu_np = dcp_kms->dpus_np;
> +	struct device_node *ep_np, *dsi_np, *child_np;
> +	struct of_endpoint ep;
> +	int ret;
> +
> +	for_each_of_graph_port(dpu_np, port_np) {
> +		u32 dsi_idx, fw_display_id;
> +		struct drm_bridge *bridge;
> +
> +		ep_np = of_get_next_child(port_np, NULL);
> +		if (!ep_np)
> +			continue;
> +
> +		ret = of_graph_parse_endpoint(ep_np, &ep);
> +		of_node_put(ep_np);
> +		if (ret)
> +			continue;
> +
> +		dsi_idx = ep.port;
> +		if (dsi_idx >= ARRAY_SIZE(dcp_kms->dsi_bridge))
> +			return -E2BIG;
> +
> +		if (dcp_kms->dsi_bridge[dsi_idx])
> +			continue;
> +
> +		dsi_np = NULL;
> +		for_each_available_child_of_node(dpu_np, child_np) {
> +			u32 reg;
> +
> +			if (!of_node_name_prefix(child_np, "dsi") ||
> +			    of_property_read_u32(child_np, "reg", &reg) ||
> +			    reg != dsi_idx)
> +				continue;
> +
> +			dsi_np = of_node_get(child_np);
> +			break;
> +		}
> +		of_node_put(child_np);
> +		if (!dsi_np)
> +			return -ENODEV;
> +
> +		fw_display_id =
> +			dcp_dpu_kms_resolve_fw_display_id(dcp_kms, dsi_idx);
> +		if (fw_display_id == U32_MAX) {
> +			of_node_put(dsi_np);
> +			return -EINVAL;
> +		}
> +
> +		bridge = dcp_dsi_bridge_init(dcp_kms, dsi_np, dpu_np,
> +					     ep.port, dsi_idx, fw_display_id);
> +		if (IS_ERR(bridge)) {
> +			ret = PTR_ERR(bridge);
> +			of_node_put(dsi_np);
> +			return ret;
> +		}
> +
> +		dcp_kms->dsi_bridge[dsi_idx] = to_dcp_dsi_bridge(bridge);
> +		drm_of_component_match_add(dcp_kms->pm_dev, matchptr,
> +					   component_compare_of, dsi_np);
> +		of_node_put(dsi_np);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int dcp_dpu_setup_pipeline(struct dcp_dpu_kms *dcp_kms,
> +				  struct device_node *dpu_np)
> +{

[ ... ]

> +	dcp_kms->num_crtcs = 0;
> +	for (i = 0; i < max_crtc_count; i++) {
> +		u32 display_id;
> +
> +		if (!dcp_kms->dsi_bridge[i]) {
> +			drm_err(dev, "missing DSI bridge for CRTC%u\n", i);
> +			ret = -ENODEV;
> +			goto err_cleanup_bridges;
> +		}
> +
> +		display_id = dcp_kms->dsi_bridge[i]->fw_display_id;

[Severity: High]
What happens if a DT graph port has no available remote panel or if port@0 is
omitted or disabled while port@1 is active?

First, dcp_dpu_kms_setup_bridges() checks of_graph_get_remote_port_parent()
and of_device_is_available(remote_np) and skips ports without an available
panel, but dcp_dpu_kms_init_dsi_bridges() does not perform this check before
creating dcp_kms->dsi_bridge[dsi_idx] and adding dsi_np to matchptr via
drm_of_component_match_add(). Because component_add() is only called from
dcp_dsi_host_attach() when a panel driver attaches to the DSI host, any
unpopulated or disabled panel endpoint in DT will prevent the component
master from ever binding.

Second, both dcp_dpu_kms_init_dsi_bridges() and dcp_dpu_kms_setup_bridges()
index dcp_kms->dsi_bridge[] by ep.port, whereas dcp_dpu_setup_pipeline()
loops from i = 0 to max_crtc_count - 1 and looks up dcp_kms->dsi_bridge[i]
using the sequential CRTC index i. If port@0 is absent or disabled and only
port@1 is active, dcp_kms->dsi_bridge[1] holds the active bridge while
max_crtc_count is 1, so dcp_dpu_setup_pipeline() checks
dcp_kms->dsi_bridge[0] and either fails with -ENODEV or binds CRTC 0 to the
wrong bridge's fw_display_id.

> +		if (dcp_dpu_kms_get_crtc(dcp_kms, display_id)) {
> +			drm_err(dev, "duplicate firmware display ID %u\n",
> +				display_id);
> +			ret = -EINVAL;
> +			goto err_cleanup_bridges;
> +		}
> +
> +		crtc = dcp_crtc_init(dcp_kms, primary_planes[i],
> +				     display_id);
> +		if (IS_ERR(crtc)) {
> +			ret = PTR_ERR(crtc);
> +			drm_err(dev, "CRTC init failed: %d\n", ret);
> +			goto err_cleanup_bridges;
> +		}
> +		dcp_kms->crtcs[dcp_kms->num_crtcs].crtc = crtc;
> +		dcp_kms->crtcs[dcp_kms->num_crtcs++].display_id = display_id;
> +	}
> +
> +	return 0;
> +
> +err_cleanup_bridges:
> +	dcp_dpu_kms_cleanup_bridges(dcp_kms);
> +	return ret;
> +}
> +
> +int dcp_dpu_drm_obj_init(struct dcp_dpu_kms *dcp_kms)
> +{
> +	struct drm_device *dev = dcp_kms->dev;
> +	struct device_node *dpus_np = dcp_kms->dpus_np;
> +	struct drm_encoder *encoder;
> +	int enc_idx = 0;
> +	int ret;
> +
> +	if (!dpus_np) {
> +		drm_err(dev, "no DCP subsystem node — cannot create DRM objects\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = dcp_dpu_setup_pipeline(dcp_kms, dpus_np);
> +	if (ret)
> +		return dev_err_probe(dev->dev, ret, "pipeline setup failed\n");
> +
> +	/* v1 - Encoders map one-to-one to CRTCs; cloning is unsupported. */
> +	drm_for_each_encoder(encoder, dev) {
> +		encoder->possible_crtcs = BIT(enc_idx);
> +		encoder->possible_clones = 0;
> +		enc_idx++;
> +	}

[Severity: Medium]
Can plane and encoder possible_crtcs masks reference non-existent CRTCs when
max_crtc_count is clamped in dcp_dpu_setup_pipeline()?

In dcp_dpu_setup_pipeline(), dcp_dpu_kms_setup_bridges() creates an encoder
for each active DSI port before CRTC and plane limits are applied:

  max_crtc_count = min(max_crtc_count, DCP_DPU_MAX_CRTCS);
  max_crtc_count = min_t(int, max_crtc_count,
                         dcp_kms->caps->num_displays);
  max_crtc_count = min_t(int, max_crtc_count,
                         ARRAY_SIZE(dcp_kms->dsi_bridge));

Then, during plane creation, possible_crtcs is computed from max_crtc_count
before max_crtc_count is clamped again to min(max_crtc_count, primary_idx).
Finally, in dcp_dpu_drm_obj_init(), every created encoder is assigned
encoder->possible_crtcs = BIT(enc_idx).

If caps->num_displays or total_planes clamps the number of created CRTCs
below the number of encoders, planes and encoders will have possible_crtcs
bits set for CRTC indices that do not exist, triggering a WARN() in
validate_encoder_possible_crtcs() during drm_mode_config_validate().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=17