Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Pixel clock truncation causes DisplayPort Transfer Unit (TU) miscalculation. - [High] EDID extension block reads fail due to omitted DP_AUX_I2C_MOT bit in I2C-over-AUX transactions. - [Medium] Missing get_modes implementation for DRM_BRIDGE_OP_MODES capability. - [Medium] Dropped hotplug interrupt notifications due to omitted DRM_BRIDGE_OP_HPD. --
--- Patch [15]: [PATCH 15/17] riscv: dts: spacemit: k3-pico-itx: enable the DisplayPort output --- commit 81b9c7b43f433a109ef12eb306af123e3e5941fc Author: Cody Kang <[email protected]> riscv: dts: spacemit: k3-pico-itx: enable the DisplayPort output Pico-ITX brings the second DPU's output to an external DisplayPort connector. Enable that DPU and the DisplayPort controller it feeds. > diff --git a/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts > b/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts > index b89c1521e6649..6c31460990a75 100644 > --- a/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts > +++ b/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts [ ... ] > @@ -221,3 +222,13 @@ hub@1 { > &usb2_phy { > status = "okay"; > }; > + > +&dpu1_crtc0 { > + status = "okay"; > +}; > + > +&dp1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&dp1_1_cfg>; > + status = "okay"; > +}; [Severity: Medium] Enabling this DisplayPort output exposes some issues in the underlying driver. Does the DisplayPort driver incorrectly declare support for DRM_BRIDGE_OP_MODES? drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_bind() dp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_MODES; The callback structure omits the logically required .get_modes callback: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_bridge_funcs static const struct drm_bridge_funcs spacemit_dp_bridge_funcs = { .attach = spacemit_dp_bridge_attach, ... [Severity: Medium] Will hotplug events be silently dropped here? The driver omits the DRM_BRIDGE_OP_HPD flag: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_bind() /* * No OP_HPD: the HPD interrupt is gated by the DP pixel clock. * Leaving it out marks the connector POLL_CONNECT | * POLL_DISCONNECT instead. */ dp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_MODES; However, the hardware interrupt is fully enabled: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_dev_init() enable_irq(dp->irq); And the handler invokes the notification function: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_hotplug_event_handler() drm_bridge_hpd_notify(&dp->bridge, status); Because DRM_BRIDGE_OP_HPD is omitted, the DRM core never registers a hotplug callback. This turns the notification into a no-op and forces the system to fall back to a 10-second polling interval. [Severity: High] Could this calculation result in an incorrect DisplayPort Transfer Unit (TU)? drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_hw_set_msa_and_enable_video() fp = mode->clock / 1000; The pixel clock is truncated from kHz to an integer MHz value, losing precision for standard clocks like 148.5 MHz. This truncated value is then used directly to compute the Transfer Unit: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_hw_set_msa_and_enable_video() /* Transfer unit: tu = fp * bpp * 640 / (8 * lanes * link_rate) */ { u64 temp_tu = (u64)fp * bpp * 640; This math error produces a systematically incorrect Transfer Unit which can lead to FIFO underflows and video corruption. [Severity: High] Can EDID extension block reads fail due to protocol violations on the I2C-over-AUX transactions? drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_conn_get_edid_block() ret = spacemit_dp_aux_i2c_write(dp, DDC_ADDR, &edid_offset, 1); if (ret) continue; ret = spacemit_dp_aux_i2c_read(dp, DDC_ADDR, buf + offset, chunk); The custom message request explicitly lacks the DP_AUX_I2C_MOT (Middle Of Transaction) bit: drivers/gpu/drm/spacemit/spacemit_inno_dp.c:spacemit_dp_aux_i2c_write() .request = DP_AUX_I2C_WRITE, Issuing a write and a read separately without maintaining I2C transaction continuity triggers an I2C STOP condition. This can cause I2C EEPROMs to reset their address pointers, preventing the reading of EDID extension blocks. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
