Re: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing
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 > > 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
Re: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing
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 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? > + 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. [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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
[PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing
The NWL MIPI Host controller registers specifies the horizontal front
porch, sync pulse, and back porch in DSI packet payload size in bytes (ie.
not in pixel clocks).
The calculation for this is (mostly) described in section 13.6.3.5.1.2 of
the i.MX 8M Dual/8M QuadLite/8M Quad Applications Processors Reference
Manual (rev. 3.1). The formula shown there does not take packet header size
into account though.
The formula implemented here converts the hfp, hsa, and hbp to DSI packet
bytes and then subtracts the number of packet header bytes.
It is worth noting that these values only needs to match approximately,
according to the reference manual.
Based on commit in NXP vendor tree
commit f2a61699749d ("LF-7981-4: drm/bridge: nwl-dsi: Correct MIPI DSI
horizontal sync timing")
Signed-off-by: Esben Haabendal
Cc: Robert Chiras
Cc: Oliver F. Brown
---
drivers/gpu/drm/bridge/nwl-dsi.c | 64 ++--
1 file changed, 61 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index e90588299c5e..87d22b3bd79c 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
@@ -264,6 +265,10 @@ static int nwl_dsi_config_dpi(struct nwl_dsi *dsi)
bool burst_mode;
int hfront_porch, hback_porch, vfront_porch, vback_porch;
int hsync_len, vsync_len;
+ int hfp, hbp, hsa;
+ unsigned long long pclk_period;
+ unsigned long long hs_period;
+ int h_blank, pkt_hdr_len, pkt_len;
hfront_porch = dsi->mode.hsync_start - dsi->mode.hdisplay;
hsync_len = dsi->mode.hsync_end - dsi->mode.hsync_start;
@@ -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);
+ 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);
+
+ /* Make sure horizontal blankins are even numbers */
+ hfp = roundup(hfp, 2);
+ hbp = roundup(hbp, 2);
+ hsa = roundup(hsa, 2);
+
+ /*
+* We need to subtract the packet header length: 32
+* In order to make sure we don't get negative values,
+* subtract a proportional value to the total length of the
+* horizontal blanking duration.
+*/
+ h_blank = hfp + hbp + hsa;
+
+ if (h_blank) {
+ pkt_len = min(hfp, roundup(((hfp * 100 / h_blank) * 32) / 100,
2));
+ pkt_hdr_len = pkt_len;
+ hfp -= pkt_len;
+
+ pkt_len = min(hbp, roundup(((hbp * 100 / h_blank) * 32) / 100,
2));
+ pkt_hdr_len += pkt_len;
+ hbp -= pkt_len;
+
+ pkt_len = min(hsa, 32 - pkt_hdr_len);
+ pkt_hdr_len += pkt_len;
+ hsa -= pkt_len;
+ if (pkt_hdr_len != 32)
+ dev_warn(dsi->dev,
+"Unable to subtract packet header length
fully: %d left\n",
+32 - pkt_hdr_len);
+ }
+
+ DRM_DEV_DEBUG_DRIVER(dsi->dev, "Register values: hfp=%d hbp=%d
hsa=%d\n", hfp, hbp, hsa);
+
+ nwl_dsi_write(dsi, NWL_DSI_HFP, hfp);
+ nwl_dsi_write(dsi, NWL_DSI_HBP, hbp);
+ nwl_dsi_write(dsi, NWL_DSI_HSA, hsa);
nwl_dsi_write(dsi, NWL_DSI_ENABLE_MULT_PKTS, 0x0);
nwl_dsi_write(dsi, NWL_DSI_BLLP_MODE, 0x1);
--
2.55.0
