Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Integer division truncation in bandwidth calculation causes DSI link underflow when combined with the newly reduced margin. Pre-existing issues: - [High] 32-bit signed integer overflow in PHY pixel clock calculation for displays with high pixel clocks (> 214.7 MHz). - [High] Unsigned long overflow on 32-bit platforms within the core PHY configuration calculation. -- --- Patch [3]: [PATCH v3 3/3] drm/rockchip: dsi: Relax the lane rate margin requirements --- commit 93b5a610ac5007bdda923b4d779bc2ab6fd40792 Author: Chaoyi Chen <[email protected]> drm/rockchip: dsi: Relax the lane rate margin requirements The commit reduces the DSI lane rate margin from 25% to about 11% to accommodate platforms limited to 1Gbps lane rates. It updates the lane rate calculation in dw_mipi_dsi_get_lane_mbps() and adjusts the PHY configuration parameter accordingly. > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > index 557708bc4faa4..bb2b35571af73 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > @@ -589,8 +589,8 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct > drm_display_mode *mode, > > mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC); > if (mpclk) { > - /* take 1 / 0.8, since mbps must big than bandwidth of RGB */ > - tmp = mpclk * (bpp / lanes) * 10 / 8; > + /* take 1 / 0.9, since mbps must big than bandwidth of RGB */ > + tmp = mpclk * (bpp / lanes) * 10 / 9; [Severity: High] Does the integer division (bpp / lanes) cause a truncation that leads to DSI link underflow with this new reduced margin? For example, with a panel using RGB666 (18 bpp) over 4 lanes, (18 / 4) evaluates to 4 instead of 4.5. With the old margin (10 / 8), the multiplier was 4 * 1.25 = 5.0, which safely exceeded the 4.5 raw multiplier requirement. With the new margin (10 / 9), the allocated bandwidth calculation becomes 4 * 10 / 9 = 4.44. Since 4.44 is strictly less than 4.5, the configured lane rate provides less bandwidth than the raw pixel payload requires, which could lead to FIFO underflows. [ ... ] > @@ -600,7 +600,7 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct > drm_display_mode *mode, > > /* for external phy only a the mipi_dphy_config is necessary */ > if (dsi->phy) { > - phy_mipi_dphy_get_default_config(mode->clock * 1000 * 10 / 8, > + phy_mipi_dphy_get_default_config(mode->clock * 1000 * 10 / 9, [Severity: High] This isn't a bug introduced by this patch, but does mode->clock * 1000 * 10 silently overflow 32-bit signed integers for high pixel clocks? Because mode->clock is a signed 32-bit integer (representing kHz) and the literals are also 32-bit signed ints, the intermediate product overflows INT_MAX (2,147,483,647) for any pixel clock exceeding ~214.7 MHz. The result would wrap to a negative value. [Severity: High] This is a pre-existing issue, but does passing this margined pixel clock to phy_mipi_dphy_get_default_config() overflow on 32-bit platforms? When the inflated pixel clock reaches phy_mipi_dphy_calc_config() in drivers/phy/phy-core-mipi-dphy.c, it computes: hs_clk_rate = pixel_clock * bpp; On 32-bit architectures, pixel_clock is a 32-bit unsigned long. If mode->clock is roughly 165 MHz, pixel_clock becomes 165,000 * 1000 * 10 / 9 = 183,333,333. Multiplying by bpp (e.g., 24) gives 4,400,000,000, which silently overflows the 32-bit ULONG_MAX before the result is assigned to the 64-bit hs_clk_rate. > bpp, lanes, > &dsi->phy_opts.mipi_dphy); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
