Re: [PATCH 15/17] riscv: dts: spacemit: k3-pico-itx: enable the DisplayPort output

[sashiko-bot@kernel.org]

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 <codykang.hk@gmail.com>

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/20260724-k3-display-v1-0-f22a10a52867@gmail.com?part=15

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