Re: [PATCH RESEND 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 2 potential issue(s) to consider:
- [High] Precision Loss in DP Timing Configuration due to integer division
- [High] Protocol Violation in I2C-over-AUX EDID Read (Missing MOT flag)
--

--- Patch [15]: [PATCH RESEND 15/17] riscv: dts: spacemit: k3-pico-itx: enable the DisplayPort output ---
commit 8521347c30ea3dc057bb9d8b08c1de584bd6ba67
Author: Cody Kang <codykang.hk@gmail.com>

riscv: dts: spacemit: k3-pico-itx: enable the DisplayPort output

This commit enables the second Saturn DPU and the corresponding DisplayPort
controller on the Pico-ITX board. While this patch configures the devicetree
to route the DPU output, it exposes regressions in the newly added underlying
Innosilicon DP/eDP driver.

> diff --git a/drivers/gpu/drm/spacemit/spacemit_inno_dp.c b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c
[ ... ]
> @@ -982,19 +982,19 @@ static void spacemit_dp_hw_set_msa_and_enable_video(struct spacemit_dp_dev *dp,
>  						    const struct drm_display_mode *mode,
>  					       enum spacemit_dp_link_rate rate,
>  					       enum spacemit_dp_lane_count lanes,
>  		u32 color_format)
>  {
>  	u64 hb_num;
>  	u32 link_rate;
>  	u32 fp; /* Pixel clock in MHz */
>  	u32 bpp, misc0;
>  	u32 tu, tu_frac, tu_int, rd_thres;
>  	u32 hsync_len;
>  
>  	/* mode->clock unit is kHz, fp unit is MHz */
>  	if (dp->use_ext_pixel_clock)
>  		fp = dp->pixel_clock / 1000;
>  	else
>  		fp = mode->clock / 1000;
>  
>  	if (fp == 0)
>  		fp = 1; /* Prevent division by zero */

[Severity: High]
Will dividing by 1000 here truncate necessary precision?

Since mode->clock / 1000 discards the fractional part (for example, a 74.25
MHz clock becomes 74 MHz), this truncated value is used in calculating the
Transfer Unit size and horizontal blanking interval. This discrepancy can
cause the display controller and the DP transmitter to disagree on the
required bandwidth, resulting in the DP TX FIFO overflowing and corrupting
the display for common standard resolutions like 720p and 1080p.

[ ... ]
> @@ -1278,9 +1278,9 @@ static int spacemit_dp_aux_i2c_write(struct spacemit_dp_dev *dp, u32 address,
>  				     const u8 *buf, size_t len)
>  {
>  	struct drm_dp_aux_msg msg = {
>  		.address = address,
>  		.request = DP_AUX_I2C_WRITE,

[Severity: High]
Is it intentional to hardcode DP_AUX_I2C_WRITE here without allowing the
DP_AUX_I2C_MOT (Middle Of Transaction) flag to be appended?

[ ... ]
> @@ -1320,11 +1320,11 @@ static int spacemit_dp_conn_get_edid_block(void *data, u8 *buf,
>  	int ret, retry;
>  	size_t offset;
>  
>  	if (segment) {
>  		for (retry = 0; retry < 3; retry++) {
>  			ret = spacemit_dp_aux_i2c_write(dp,
>  				SPACEMIT_DP_DDC_SEGMENT_ADDR, &segment, 1);
>  			if (!ret)
>  				break;
>  		}

[Severity: High]
Can this segment write cause the hardware to generate an I2C STOP condition?

Because spacemit_dp_aux_i2c_write() lacks the MOT flag, writing the E-DDC
segment pointer without it directs the hardware to generate a STOP condition.
According to the E-DDC specification, a STOP condition immediately after
writing the segment pointer forces the display EEPROM to reset its segment
pointer to 0.

As a result, subsequent reads might always fetch EDID blocks 0 and 1, making
it impossible to correctly read EDID blocks >= 2 (such as CTA-861 extension
blocks) and potentially breaking support for advanced resolutions and audio
and HDR features.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-k3-display-v1-0-6de34d80e86c@gmail.com?part=15

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