Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] drm/msm/dcp: off-by-one DSC chroma and color space enums in dcp_dsi_host_attach() - [High] drm/msm/dcp: deadlock and unsafe teardown in dcp_dsi_bridge_cleanup() - [Medium] drm/msm/dcp: OF node leak of dcp_np on error in dcp_dsi_bridge_init() - [Medium] drm/msm/dcp: dcp_dsi_bridge_setup_dsc() never sets dsc_rc_override - [Medium] drm/msm/dcp: duplicate dsi_host_dev name in dcp_dsi_bridge_init() - [Medium] drm/msm/dcp: invalid session close and goto mixing in dcp_dsi_bridge_prepare_session() - [Medium] drm/msm/dcp: missing session_lock in dcp_dsi_bridge_atomic_enable() - [Medium] drm/msm/dcp: unsynchronized session_id and packet_id in power control - [Medium] drm/msm/dcp: firmware session leak on dcp_hfi_display_close() early return - [Medium] drm/msm/dcp: wrong byte count returned for DSI reads in dcp_dsi_host_transfer() --
commit 7535bc53c50473e4189f52c83b74358ab514c8d0 Author: Jayant Shekhar <[email protected]> drm/msm/dcp: Add DCP DSI host and panel support This commit implements the MIPI DSI host, panel-device support, bridge state, DSC configuration, DCS transfers, and DSI buffer management for the DCP display path. > diff --git a/drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c > b/drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c > new file mode 100644 > index 0000000000000..b3e782b8f0470 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c > @@ -0,0 +1,1110 @@ [ ... ] > +static void dcp_dsi_power_control_work(struct work_struct *work) > +{ > + struct dcp_dsi_bridge *dsi_bridge = > + container_of(work, struct dcp_dsi_bridge, power_control_work); > + struct dcp_dpu_kms *kms = dsi_bridge->kms; > + > + dcp_hfi_display_power_control_ack(kms->hfi, > + dsi_bridge->session_id, > + dsi_bridge->power_packet_id); > + > + complete(&dsi_bridge->power_done); > +} > + > +static void dcp_dsi_power_control_cb(void *priv, u32 display_id, > + u32 power_mask, bool enable, > + u32 packet_id) > +{ > + struct dcp_dpu_kms *kms = priv; > + struct dcp_dsi_bridge *dsi_bridge = NULL; > + int idx; > + > + for (idx = 0; idx < ARRAY_SIZE(kms->dsi_bridge); idx++) { > + struct dcp_dsi_bridge *b = kms->dsi_bridge[idx]; > + > + if (b && b->session_id == display_id) { > + dsi_bridge = b; > + break; > + } > + } > + > + if (!dsi_bridge) { > + dev_warn(kms->pm_dev, > + "DSI: power_control for unknown display_id=%u\n", > display_id); > + return; > + } > + > + dsi_bridge->power_packet_id = packet_id; > + schedule_work(&dsi_bridge->power_control_work); > +} [Severity: Medium] Are dsi_bridge->session_id and dsi_bridge->power_packet_id synchronized and matched safely between dcp_dsi_power_control_cb() and dcp_dsi_power_control_work()? When dcp_hfi_dispatch_event() drops resp_drain_lock and invokes dcp_dsi_power_control_cb(), the loop checks b->session_id == display_id without checking b->session_open or holding session_lock. Since closed bridges have session_id == 0, an unopened bridge matches when display_id == 0, whereas a temporary session opened in dcp_dsi_host_transfer() stores its session ID in a local variable and leaves dsi_bridge->session_id == 0, failing to match a non-zero display_id. Furthermore, dcp_dsi_power_control_work() reads dsi_bridge->session_id and dsi_bridge->power_packet_id without locking while dcp_dsi_bridge_atomic_post_disable(), dcp_dsi_bridge_atomic_enable(), and dcp_dsi_bridge_prepare_session() reset dsi_bridge->session_id = 0 without flushing power_control_work. And in dcp_dsi_bridge_cleanup(), dcp_hfi_clear_power_control_cb() does not wait for an in-flight callback that already dropped resp_drain_lock, allowing schedule_work(&dsi_bridge->power_control_work) to run after cancel_work_sync(). [ ... ] > +static int dcp_dsi_host_attach(struct mipi_dsi_host *host, > + struct mipi_dsi_device *dsi) > +{ > + struct dcp_dsi_bridge *dsi_bridge = to_dcp_dsi_host(host); > + struct dcp_dsi_hfi_panel_config *cfg = &dsi_bridge->hfi_panel_config; > + int ret; [ ... ] > + if (dsi->dsc) { > + struct drm_dsc_config *dsc = dsi->dsc; > + u32 chroma_format = 0; > + u32 color_space = 0; > + > + if (dsc->native_422) > + chroma_format = 1; > + else if (dsc->native_420) > + chroma_format = 2; > + if (dsc->native_422 || dsc->native_420) > + color_space = 1; > + > + cfg->dsc_enabled = true; > + cfg->dsc_version_major = dsc->dsc_version_major; > + cfg->dsc_version_minor = dsc->dsc_version_minor; > + cfg->dsc_scr_version = 0; > + cfg->dsc_slice_height = dsc->slice_height; > + cfg->dsc_slice_width = dsc->slice_width; > + cfg->dsc_slices_per_pkt = 1; > + cfg->dsc_bits_per_component = dsc->bits_per_component; > + cfg->dsc_pps_delay_ms = 0; > + cfg->dsc_bits_per_pixel = dsc->bits_per_pixel; > + cfg->dsc_chroma_format = chroma_format; > + cfg->dsc_color_space = color_space; [Severity: High] Are chroma_format and color_space off by one here? In drivers/gpu/drm/msm/dcp/hfi/hfi_defs_panel.h, the HFI protocol enums packed into HFI_PROPERTY_PANEL_COMPRESSION_DATA by dcp_hfi_panel_init() are 1-based: enum hfi_panel_chroma_format_type { HFI_PANEL_CHROMA_FORMAT_444 = 1, HFI_PANEL_CHROMA_FORMAT_422 = 2, HFI_PANEL_CHROMA_FORMAT_420 = 3, }; enum hfi_panel_color_space_type { HFI_PANEL_COLOR_SPACE_RGB = 1, HFI_PANEL_COLOR_SPACE_YUV = 2, }; Should dcp_dsi_host_attach() use the HFI_PANEL_CHROMA_FORMAT_* and HFI_PANEL_COLOR_SPACE_* constants instead of 0-based values? [ ... ] > +static ssize_t dcp_dsi_host_transfer(struct mipi_dsi_host *host, > + const struct mipi_dsi_msg *msg) > +{ > + struct dcp_dsi_bridge *dsi_bridge = to_dcp_dsi_host(host); > + struct dcp_dpu_kms *kms = dsi_bridge->kms; > + u32 session_id; > + bool temporary_session = false; > + ssize_t ret; > + > + guard(mutex)(&dsi_bridge->session_lock); > + if (!dsi_bridge->dcs_tx_buf) > + return -ENODEV; > + > + /* Serialize DCS transfers */ > + guard(mutex)(&dsi_bridge->dcs_lock); > + > + if (dsi_bridge->session_open) { > + session_id = dsi_bridge->session_id; > + } else { > + /* > + * mipi_dsi_host_ops::transfer() may be called while the host is > + * disabled. Open a transaction-only DCP session in that case; > + * atomic_pre_enable() will create the full display session > later. > + */ > + ret = dcp_hfi_display_enable(kms->hfi, dsi_bridge->intf_idx, > + dsi_bridge->fw_display_id, > + &session_id); > + if (ret) > + return ret; > + > + temporary_session = true; > + } > + > + ret = dcp_hfi_dcs_transfer(kms->hfi, session_id, msg, > + dsi_bridge->dcs_tx_buf, > dsi_bridge->dcs_tx_iova, > + dsi_bridge->dcs_rx_buf, > dsi_bridge->dcs_rx_iova, 0, > + > dsi_bridge->hfi_panel_config.ctrl_indices[0]); [Severity: Medium] Does dcp_dsi_host_transfer() return the wrong byte count for DSI read transfers and reject 0-parameter generic transfers? In drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c, dcp_hfi_dcs_transfer() ends with: if (msg->rx_len > 0) memcpy(msg->rx_buf, rx_buf, msg->rx_len); return msg->tx_len; and rejects msg->tx_len == 0 at entry with -EINVAL. For mipi_dsi_host_ops::transfer(), callers such as mipi_dsi_dcs_read() and mipi_dsi_generic_read() expect the number of received bytes (msg->rx_len) on read transfers, and 0-parameter generic transfers (MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM and MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM) have msg->tx_len == 0. > + if (temporary_session) { > + int close_ret = dcp_hfi_display_close(kms->hfi, session_id); > + > + if (ret >= 0 && close_ret) > + ret = close_ret; > + } [Severity: Medium] Can calling dcp_hfi_display_close() here (and on the err_close path in dcp_dsi_bridge_atomic_enable()) leak the firmware display session if DISPLAY_DISABLE or DISPLAY_POST_DISABLE fails? In drivers/gpu/drm/msm/dcp/dcp_hfi_dsi.c, dcp_hfi_display_close() returns early before sending HFI_COMMAND_DISPLAY_CLOSE if either dcp_hfi_display_disable() or dcp_hfi_display_post_disable() returns an error: int dcp_hfi_display_close(struct dcp_hfi *hfi, u32 session_id) { int ret; ret = dcp_hfi_display_disable(hfi, session_id); if (ret) { dev_warn(hfi->dev, "HFI DISPLAY_DISABLE failed: %d\n", ret); return ret; } ret = dcp_hfi_display_post_disable(hfi, session_id); if (ret) { dev_warn(hfi->dev, "HFI DISPLAY_POST_DISABLE failed: %d\n", ret); return ret; } When closing a temporary session in dcp_dsi_host_transfer() (which only called dcp_hfi_display_enable() and never called dcp_hfi_display_post_enable()) or rolling back a partially enabled session in dcp_dsi_bridge_atomic_enable(), an error from DISPLAY_DISABLE or DISPLAY_POST_DISABLE skips HFI_COMMAND_DISPLAY_CLOSE while the driver drops session_id and marks session_open = false. In contrast, dcp_dsi_bridge_atomic_disable() and dcp_dsi_bridge_atomic_post_disable() still send HFI_COMMAND_DISPLAY_CLOSE even if the preceding disable commands warn. [ ... ] > +static int dcp_dsi_bridge_setup_dsc(struct dcp_dsi_bridge *dsi_bridge, > + struct dcp_dsi_hfi_panel_config *cfg, > + const struct drm_display_mode *mode) > +{ [ ... ] > + for (i = 0; i < ARRAY_SIZE(cfg->dsc_rc_min_qp); i++) { > + cfg->dsc_rc_min_qp[i] = dsc->rc_range_params[i].range_min_qp; > + cfg->dsc_rc_max_qp[i] = dsc->rc_range_params[i].range_max_qp; > + cfg->dsc_rc_bpg_offsets[i] = > dsc->rc_range_params[i].range_bpg_offset; > + } > + > + return 0; > +} [Severity: Medium] Should dcp_dsi_bridge_setup_dsc() also set cfg->dsc_rc_override = true? dcp_dsi_host_attach() initializes cfg->dsc_rc_override = false, and dcp_dsi_bridge_prepare_session() passes a stack-local copy of cfg to dcp_dsi_bridge_setup_dsc() and then to dcp_hfi_panel_init(). In drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c, dcp_hfi_panel_init() only packs HFI_PROPERTY_PANEL_COMPRESSION_RC_OVERRIDE when (cfg->dsc_enabled && cfg->dsc_rc_override) is true, so the rate-control parameters populated in cfg->dsc_rc_min_qp, cfg->dsc_rc_max_qp, and cfg->dsc_rc_bpg_offsets are never sent to the firmware. > + > +int dcp_dsi_bridge_prepare_session(struct dcp_dsi_bridge *dsi_bridge, > + const struct drm_display_mode *mode) > +{ > + struct dcp_dpu_kms *kms = dsi_bridge->kms; > + struct dcp_dsi_bridge_state *state = > dcp_dsi_bridge_get_state(dsi_bridge); > + struct dcp_dsi_hfi_panel_config cfg; > + int ret; > + u32 opened_session_id; > + > + if (!mode) > + return -ENODEV; > + > + guard(mutex)(&dsi_bridge->session_lock); > + if (dsi_bridge->session_open) > + return 0; > + > + dsi_bridge->session_id = dsi_bridge->fw_display_id; > + > + if (!dsi_bridge->dsi_device) > + return -ENODEV; [ ... ] > + ret = dcp_hfi_display_enable(kms->hfi, > + dsi_bridge->intf_idx, > + dsi_bridge->fw_display_id, > + &opened_session_id); > + if (ret) { > + drm_err(kms->dev, "DSI%u: HFI DISPLAY_ENABLE failed: %d\n", > + dsi_bridge->intf_idx, ret); > + goto err_close; > + } > + dsi_bridge->session_id = opened_session_id; > + dsi_bridge->session_open = true; > + dcp_dsi_bridge_publish_session(dsi_bridge); > + > + drm_dbg_driver(kms->dev, "DSI%u: session prepared (session_id=%u)\n", > + dsi_bridge->intf_idx, dsi_bridge->session_id); > + return 0; > + > +err_close: > + dcp_hfi_clear_power_control_cb(kms->hfi, kms); > + dcp_hfi_display_close(kms->hfi, dsi_bridge->session_id); > + dsi_bridge->session_id = 0; > + dsi_bridge->session_open = false; > + dcp_dsi_bridge_clear_session(dsi_bridge); > + > + return ret; > +} [Severity: Medium] There appear to be several issues with error handling and cleanup in dcp_dsi_bridge_prepare_session(): 1. It mixes guard(mutex)(&dsi_bridge->session_lock) with goto err_close, contrary to the cleanup.h guideline not to mix goto error paths and scope-based cleanup helpers in the same function. 2. If !dsi_bridge->dsi_device is true, returning -ENODEV early leaves dsi_bridge->session_id set to dsi_bridge->fw_display_id while session_open is false. 3. Every goto err_close path triggers before dcp_hfi_display_enable() has succeeded (even if dcp_dsi_bridge_setup_dsc() fails in software before any HFI command is sent), yet err_close calls dcp_hfi_display_close() to send DISPLAY_DISABLE, DISPLAY_POST_DISABLE, and DISPLAY_CLOSE for a session that was never enabled. 4. Calling dcp_hfi_clear_power_control_cb(kms->hfi, kms) in err_close clears the single shared power control callback on kms->hfi for all DSI bridges in kms->dsi_bridge[]. > + > +static void dcp_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + struct dcp_dsi_bridge *dsi_bridge = to_dcp_dsi_bridge(bridge); > + int ret; > + > + ret = dsi_bridge->session_open ? 0 : -ENODEV; > + dcp_dsi_bridge_get_state(dsi_bridge)->prepare_error = ret; > + if (ret) > + drm_err(bridge->dev, "DSI%u: session prepare failed: %d\n", > + dsi_bridge->intf_idx, ret); > +} > + > +static void dcp_dsi_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + struct dcp_dsi_bridge *dsi_bridge = to_dcp_dsi_bridge(bridge); > + struct dcp_dpu_kms *kms = dsi_bridge->kms; > + bool scan_start_registered = false; > + bool scan_complete_registered = false; > + bool autorefresh_enabled = false; > + int ret; > + > + drm_dbg_driver(kms->dev, > + "DSI%u: bridge atomic_enable (session_id=%u)\n", > + dsi_bridge->intf_idx, dsi_bridge->session_id); > + > + if (dcp_dsi_bridge_get_state(dsi_bridge)->prepare_error) > + return; > + if (!dsi_bridge->session_open) > + return; > + > + ret = dcp_hfi_display_post_enable(kms->hfi, dsi_bridge->session_id); [ ... ] > +err_close: > + if (autorefresh_enabled) > + dcp_hfi_crtc_set_autorefresh(kms->hfi, dsi_bridge->session_id, > + false, 0); > + if (scan_complete_registered) > + dcp_hfi_display_event_deregister(kms->hfi, > dsi_bridge->session_id, > + HFI_EVENT_FRAME_SCAN_COMPLETE); > + if (scan_start_registered) > + dcp_hfi_display_event_deregister(kms->hfi, > dsi_bridge->session_id, > + HFI_EVENT_FRAME_SCAN_START); > + drm_err(bridge->dev, > + "DSI%u: bridge enable failed; session closed\n", > + dsi_bridge->intf_idx); > + dcp_hfi_display_close(kms->hfi, dsi_bridge->session_id); > + { > + guard(mutex)(&dsi_bridge->session_lock); > + dsi_bridge->session_id = 0; > + dsi_bridge->session_open = false; > + } > + dcp_dsi_bridge_clear_session(dsi_bridge); > +} [Severity: Medium] Should dcp_dsi_bridge_atomic_enable() (and dcp_dsi_bridge_atomic_pre_enable()) hold dsi_bridge->session_lock across the session operations rather than only around clearing session_id and session_open at the end of err_close? In addition to mixing goto err_close with guard(mutex), reading dsi_bridge->session_open and dsi_bridge->session_id and calling dcp_hfi_display_close() without holding session_lock allows a concurrent dcp_dsi_host_transfer() call to observe session_open == true and send DCS commands on a session ID that is actively being closed or has already been closed in firmware. [ ... ] > +struct drm_bridge *dcp_dsi_bridge_init(struct dcp_dpu_kms *kms, > + struct device_node *np, > + struct device_node *dcp_np, > + u32 dcp_port, u32 intf_idx, > + u32 fw_display_id) > +{ > + struct dcp_dsi_bridge *dsi_bridge; > + struct dcp_dsi_hfi_panel_config *cfg; > + struct device *dev = kms->pm_dev; > + struct device *dma_dev = kms->hfi->dma_dev; > + int ret; > + > + dsi_bridge = devm_drm_bridge_alloc(dev, struct dcp_dsi_bridge, > + bridge, &dcp_dsi_bridge_funcs); > + if (IS_ERR(dsi_bridge)) > + return ERR_CAST(dsi_bridge); > + > + dsi_bridge->dcp_np = of_node_get(dcp_np); [ ... ] > + device_initialize(&dsi_bridge->dsi_host_dev); > + dsi_bridge->dsi_host_dev.release = dcp_dsi_host_dev_release; > + dsi_bridge->dsi_host_dev.parent = dev; > + dsi_bridge->dsi_host_dev.of_node = of_node_get(np); > + dev_set_name(&dsi_bridge->dsi_host_dev, "%pOFn", np); > + dsi_bridge->dsi_host.ops = &dcp_dsi_host_ops; > + dsi_bridge->dsi_host.dev = &dsi_bridge->dsi_host_dev; > + > + ret = device_add(&dsi_bridge->dsi_host_dev); [Severity: Medium] Won't dev_set_name(&dsi_bridge->dsi_host_dev, "%pOFn", np) cause a device name collision in device_add() when two DSI bridges are present? The %pOFn specifier strips the @unit-address suffix from the node name, so both dsi@0 and dsi@1 receive the name "dsi" under the same parent device (kms->pm_dev), causing the second device_add() to fail with -EEXIST. [ ... ] > +err_unwind_dma: > + if (dsi_bridge->dcs_rx_buf) { > + dma_free_coherent(dma_dev, DCP_DSI_DCS_RX_BUF_SIZE, > + dsi_bridge->dcs_rx_buf, > + dsi_bridge->dcs_rx_iova); > + } > + if (dsi_bridge->dcs_tx_buf) { > + dma_free_coherent(dma_dev, DCP_DSI_DCS_TX_BUF_SIZE, > + dsi_bridge->dcs_tx_buf, > + dsi_bridge->dcs_tx_iova); > + } > + cancel_work_sync(&dsi_bridge->power_control_work); > + mutex_destroy(&dsi_bridge->session_lock); > + mutex_destroy(&dsi_bridge->dcs_lock); > + return ERR_PTR(ret); > +} [Severity: Medium] Does the err_unwind_dma path in dcp_dsi_bridge_init() leak the dsi_bridge->dcp_np device_node reference acquired via of_node_get(dcp_np) if dma_alloc_coherent(), device_add(), or mipi_dsi_host_register() fails? [ ... ] > +void dcp_dsi_bridge_cleanup(struct dcp_dsi_bridge *dsi_bridge) > +{ > + struct device *dma_dev = dsi_bridge->kms->hfi->dma_dev; > + > + dcp_hfi_clear_power_control_cb(dsi_bridge->kms->hfi, dsi_bridge->kms); > + cancel_work_sync(&dsi_bridge->power_control_work); > + > + drm_bridge_remove(&dsi_bridge->bridge); > + of_node_put(dsi_bridge->bridge.of_node); > + of_node_put(dsi_bridge->dcp_np); > + > + mipi_dsi_host_unregister(&dsi_bridge->dsi_host); > + device_del(&dsi_bridge->dsi_host_dev); > + put_device(&dsi_bridge->dsi_host_dev); > + > + dcp_dsi_bridge_unmap_buffers(dsi_bridge); > + dma_free_coherent(dma_dev, DCP_DSI_DCS_TX_BUF_SIZE, > + dsi_bridge->dcs_tx_buf, dsi_bridge->dcs_tx_iova); > + > + dma_free_coherent(dma_dev, DCP_DSI_DCS_RX_BUF_SIZE, > + dsi_bridge->dcs_rx_buf, dsi_bridge->dcs_rx_iova); > + > + mutex_destroy(&dsi_bridge->session_lock); > + mutex_destroy(&dsi_bridge->dcs_lock); > +} [Severity: High] Can calling mipi_dsi_host_unregister() from dcp_dsi_bridge_cleanup() deadlock on component_mutex and trigger use-after-free during teardown? 1. When a DSI device is attached, mipi_dsi_host_unregister() removes the child device and invokes: mipi_dsi_remove_device_fn() -> mipi_dsi_detach() -> dcp_dsi_host_detach() -> component_del(&dsi_bridge->dsi_host_dev, &dcp_dsi_component_ops) component_del() acquires the global component_mutex. When dcp_dsi_bridge_cleanup() is called from aggregate device bringup failure or aggregate device takedown (for example via component_master_del() / take_down_aggregate_device() -> msm_drm_unbind() -> msm_drm_uninit() -> dcp_dpu_kms_destroy()), component_mutex is already held by the caller, causing a recursive deadlock. 2. struct dcp_dsi_bridge is allocated with devm_drm_bridge_alloc() on kms->pm_dev and embeds struct device dsi_host_dev, which is refcounted independently. Calling device_del() and put_device() on dsi_host_dev during kms->destroy() happens before component_unbind_all() finishes using component->dev (&dsi_bridge->dsi_host_dev). In addition, dcs_tx_buf and dcs_rx_buf are freed without holding session_lock/dcs_lock or clearing dsi_bridge->dcs_tx_buf to NULL, and session_lock and dcs_lock are destroyed while the bridge object is still alive. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
