<[email protected]> writes: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) > to consider: > - [High] Implicit 64-bit division using the `/` operator causes link errors > on 32-bit architectures. > - [Low] Integer overflow in `dsi->mode.clock * 1000` for high pixel clocks. > - [Low] Redundant picosecond period conversion introduces precision loss and > forces 64-bit math. > -- > > --- Patch [5]: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI > horizontal sync timing --- > commit 54cf1e5377e7b6795c12c78939b18d1607d54031 > Author: Esben Haabendal <[email protected]> > > drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing > > The commit updates the NWL MIPI Host controller's horizontal sync timing > calculations. It converts hfp, hsa, and hbp to DSI packet bytes and > subtracts the packet header size. This aligns the timing configuration > with the i.MX 8M reference manual. > >> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c >> b/drivers/gpu/drm/bridge/nwl-dsi.c >> index e90588299c5ea..87d22b3bd79c7 100644 >> --- a/drivers/gpu/drm/bridge/nwl-dsi.c >> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c > [ ... ] >> @@ -317,9 +322,62 @@ static int nwl_dsi_config_dpi(struct nwl_dsi *dsi) >> dsi->mode.hdisplay); >> } >> >> - nwl_dsi_write(dsi, NWL_DSI_HFP, hfront_porch); >> - nwl_dsi_write(dsi, NWL_DSI_HBP, hback_porch); >> - nwl_dsi_write(dsi, NWL_DSI_HSA, hsync_len); >> + pclk_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, dsi->mode.clock * 1000); > > [Severity: Low] > Could this multiplication overflow on high pixel clocks? dsi->mode.clock is a > signed 32-bit integer, so multiplying by 1000 could overflow if the clock > exceeds roughly 2.147 GHz. The resulting negative number would then be > sign-extended into a large unsigned long long for DIV_ROUND_UP_ULL(). Perhaps > casting to 1000ULL would avoid this?
There is no 32-bit platform using this driver. Should/could we mark this driver in some way to make it clear for sashiko-bot and others that it is not relevant to consider 32-bit integer calculations for this driver? >> + DRM_DEV_DEBUG_DRIVER(dsi->dev, "pclk_period: %llu\n", pclk_period); >> + >> + hs_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, >> dsi->phy_cfg.mipi_dphy.hs_clk_rate); >> + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hs_period: %llu\n", hs_period); >> + >> + /* >> + * Calculate the bytes needed, according to the RM formula: >> + * Time of DPI event = time to transmit x number of bytes on the DSI >> + * interface >> + * dpi_event_size * dpi_pclk_period = dsi_bytes * 8 * hs_bit_period / >> + * num_lanes >> + * ===> >> + * dsi_bytes = dpi_event_size * dpi_pclk_period * num_lanes / >> + * (8 * hs_bit_period) >> + */ >> + hfp = hfront_porch * pclk_period * dsi->lanes / (8 * hs_period); >> + hbp = hback_porch * pclk_period * dsi->lanes / (8 * hs_period); >> + hsa = hsync_len * pclk_period * dsi->lanes / (8 * hs_period); > > [Severity: Low] > Does calculating the period in picoseconds introduce unnecessary precision > loss? > By calculating pclk_period and hs_period first and then dividing them, the > math effectively does (10^12 / pclk) / (10^12 / hs_clk). For example, 148.5MHz > becomes 6735ps instead of 6734.0067ps. Using the raw clock rates directly > might eliminate intermediate rounding errors. I change the calculation to use the raw clock rates directly for next version. > [Severity: High] > Will this standard division operator cause build failures on 32-bit platforms? > pclk_period and hs_period are 64-bit variables, so using the / operator here > will result in undefined reference to __udivdi3 during linking on 32-bit > architectures. Using div64_u64() or refactoring to use a 32-bit denominator > would prevent this link error. As mentioned above, there is no use of this driver on 32-bit platforms. /Esben
