Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/bridge: dw-mipi-dsi: wrong lane count in dw_mipi_dsi_get_hcomponent_lbcc() - [Medium] drm/bridge: dw-mipi-dsi: 1/8 margin check in dw_mipi_dsi_get_hcomponent_lbcc() --
commit 6499adefdc008cafd18dfe3208439c2abd0174aa Author: Romain TISSERAND <[email protected]> drm/bridge: dw-mipi-dsi: use the lane rate for lbcc when lanes run faster This commit updates dw_mipi_dsi_get_hcomponent_lbcc() to calculate horizontal timing values from the DSI lane rate rather than the pixel clock whenever the lane rate exceeds the pixel bandwidth by more than 1/8, fixing DPI payload FIFO overflows on glues such as Rockchip that run the lanes faster in non-burst video modes. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 22d1b8cbe1d57..b026b602abc0f 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -781,19 +781,28 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct > dw_mipi_dsi *dsi, > u32 hcomponent) > { > u32 frac, lbcc, minimum_lbcc; > + u64 pixel_kbps; > int bpp; > > - if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > + bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > + if (bpp < 0) { > + dev_err(dsi->dev, "failed to get bpp\n"); > + return 0; > + } > + > + /* > + * Use the lane rate when it is above the pixel bandwidth (burst, or > + * glues adding margin in all modes); the 1/8 slack ignores lane_mbps > + * being rounded up to the next MHz. Otherwise the pixel clock is more > + * precise. > + */ > + pixel_kbps = div_u64((u64)mode->clock * bpp, dsi->lanes); > + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST || > + (u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8) { [Severity: Medium] Can the 1/8 margin check miss panels where the lane rate runs faster than the pixel bandwidth by less than 12.5%? In drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c, dw_mipi_dsi_get_lane_mbps() computes the default lane rate using integer division for (bpp / lanes): 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; For a 4-lane non-burst panel using MIPI_DSI_FMT_RGB666_PACKED (bpp = 18), (bpp / lanes) truncates from 4.5 to 4, giving a lane rate of 5 * mpclk Mbps. That is only 11.1% (1/9) above the 4.5 * mode->clock pixel bandwidth, rather than 25%. For any pixel clock >= 16 MHz on such a panel (or when a panel's requested hs_rate or max_mbps clamp puts dsi->lane_mbps between +1 MHz and +12.5% above pixel_kbps), (u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8 evaluates to false. Wouldn't that fall back to the pixel-clock formula and program DSI_VID_HLINE_TIME, DSI_VID_HSA_TIME, and DSI_VID_HBP_TIME shorter than the DPI line period, triggering the same DPI payload FIFO overflow? If the slack is only meant to ignore lane_mbps being rounded up to the next MHz, should the check compare dsi->lane_mbps against DIV_ROUND_UP_ULL(pixel_kbps, MSEC_PER_SEC) instead of adding pixel_kbps / 8? > /* lbcc based on lane_mbps */ > lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8; > } else { > /* lbcc based on pixel clock rate */ > - bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > - if (bpp < 0) { > - dev_err(dsi->dev, "failed to get bpp\n"); > - return 0; > - } > - > lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes > * 8); [Severity: Medium] Should pixel_kbps and the pixel-clock lbcc calculation divide by dw_mipi_dsi_get_lanes(dsi) instead of dsi->lanes to handle dual-DSI panels? In dw_mipi_dsi_bridge_atomic_pre_enable() -> dw_mipi_dsi_mode_set(), dsi->lane_mbps is obtained by passing the combined master and slave lane count from dw_mipi_dsi_get_lanes(dsi) along with the full-panel adjusted_mode: u32 lanes = dw_mipi_dsi_get_lanes(dsi); clk_prepare_enable(dsi->pclk); ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags, lanes, dsi->format, &dsi->lane_mbps); When a dual-DSI panel is attached in non-burst video mode via dw_mipi_dsi_set_slave() (such as innolux,p097pfg on rk3399-gru-scarlet with 4 + 4 lanes), dsi->lane_mbps is the per-lane rate across all 8 lanes, whereas dividing mode->clock * bpp by dsi->lanes (4) makes pixel_kbps twice the actual per-lane pixel bandwidth. Because pixel_kbps is doubled, the condition (u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8 evaluates to false, and the else branch then also divides by dsi->lanes (4) instead of dw_mipi_dsi_get_lanes(dsi) (8), programming DSI_VID_HLINE_TIME, DSI_VID_HSA_TIME, and DSI_VID_HBP_TIME for twice the per-lane pixel bandwidth rather than the actual lane rate. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
