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 <[email protected]> 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/[email protected]?part=15
