* [PATCH v3 1/5] drm/bridge: nwl-dsi: Cleanup endpoint mux control selection
2026-08-10 7:14 [PATCH v3 0/5] drm/bridge: nwl-dsi: Various fixes Esben Haabendal
@ 2026-08-10 7:14 ` Esben Haabendal
2026-08-10 15:02 ` Luca Ceresoli
2026-08-10 7:14 ` [PATCH v3 2/5] drm/bridge: nwl-dsi: Limit LCDIF specific sync override Esben Haabendal
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-08-10 7:14 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg
Cc: Esben Haabendal, dri-devel, linux-kernel, stable
Using the endpoint define values makes it more clear that the argument to
mux_control_try_select() must be valid state integers, and not something
that looks like a boolean.
The endpoint value is kept, as it will be used in a following patch that
changes behavior slightly based on which endpoint is selected.
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/gpu/drm/bridge/nwl-dsi.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index 3c9ae93c4f67..39be58cf5bd2 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -110,6 +110,8 @@ struct nwl_dsi {
int error;
struct nwl_dsi_transfer *xfer;
+
+ unsigned int endpoint;
};
static const struct regmap_config nwl_dsi_regmap_config = {
@@ -1088,13 +1090,12 @@ static int nwl_dsi_parse_dt(struct nwl_dsi *dsi)
static int nwl_dsi_select_input(struct nwl_dsi *dsi)
{
struct device_node *remote;
- u32 use_dcss = 1;
int ret;
remote = of_graph_get_remote_node(dsi->dev->of_node, 0,
NWL_DSI_ENDPOINT_LCDIF);
if (remote) {
- use_dcss = 0;
+ dsi->endpoint = NWL_DSI_ENDPOINT_LCDIF;
} else {
remote = of_graph_get_remote_node(dsi->dev->of_node, 0,
NWL_DSI_ENDPOINT_DCSS);
@@ -1103,11 +1104,12 @@ static int nwl_dsi_select_input(struct nwl_dsi *dsi)
"No valid input endpoint found\n");
return -EINVAL;
}
+ dsi->endpoint = NWL_DSI_ENDPOINT_DCSS;
}
DRM_DEV_INFO(dsi->dev, "Using %s as input source\n",
- (use_dcss) ? "DCSS" : "LCDIF");
- ret = mux_control_try_select(dsi->mux, use_dcss);
+ (dsi->endpoint == NWL_DSI_ENDPOINT_DCSS) ? "DCSS" : "LCDIF");
+ ret = mux_control_try_select(dsi->mux, dsi->endpoint);
if (ret < 0)
DRM_DEV_ERROR(dsi->dev, "Failed to select input: %d\n", ret);
--
2.55.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/5] drm/bridge: nwl-dsi: Cleanup endpoint mux control selection
2026-08-10 7:14 ` [PATCH v3 1/5] drm/bridge: nwl-dsi: Cleanup endpoint mux control selection Esben Haabendal
@ 2026-08-10 15:02 ` Luca Ceresoli
0 siblings, 0 replies; 9+ messages in thread
From: Luca Ceresoli @ 2026-08-10 15:02 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg,
dri-devel, linux-kernel, stable
On Mon, 10 Aug 2026 09:14:37 +0200, Esben Haabendal <esben@geanix.com> wrote:
> Using the endpoint define values makes it more clear that the argument to
> mux_control_try_select() must be valid state integers, and not something
> that looks like a boolean.
>
> The endpoint value is kept, as it will be used in a following patch that
> changes behavior slightly based on which endpoint is selected.
>
> [...]
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/5] drm/bridge: nwl-dsi: Limit LCDIF specific sync override
2026-08-10 7:14 [PATCH v3 0/5] drm/bridge: nwl-dsi: Various fixes Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 1/5] drm/bridge: nwl-dsi: Cleanup endpoint mux control selection Esben Haabendal
@ 2026-08-10 7:14 ` Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 3/5] drm/bridge: nwl-dsi: Correct auto-insert EOTP behavior Esben Haabendal
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Esben Haabendal @ 2026-08-10 7:14 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg
Cc: Esben Haabendal, dri-devel, linux-kernel, stable
When using DCSS with NWL, overriding the mode flags to enforce active high
sync is preventing the use of active low with downstream bridges, such as
ti-sn65dsi83, which will not see such mode flags set by the panel.
Fixes: 44cfc6233447 ("drm/bridge: Add NWL MIPI DSI host controller support")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/gpu/drm/bridge/nwl-dsi.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index 39be58cf5bd2..374884e76f25 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -823,10 +823,13 @@ static int nwl_dsi_bridge_atomic_check(struct drm_bridge *bridge,
struct drm_connector_state *conn_state)
{
struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+ struct nwl_dsi *dsi = bridge_to_dsi(bridge);
/* At least LCDIF + NWL needs active high sync */
- adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
- adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+ if (dsi->endpoint == NWL_DSI_ENDPOINT_LCDIF) {
+ adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+ adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+ }
/*
* Do a full modeset if crtc_state->active is changed to be true.
--
2.55.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 3/5] drm/bridge: nwl-dsi: Correct auto-insert EOTP behavior
2026-08-10 7:14 [PATCH v3 0/5] drm/bridge: nwl-dsi: Various fixes Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 1/5] drm/bridge: nwl-dsi: Cleanup endpoint mux control selection Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 2/5] drm/bridge: nwl-dsi: Limit LCDIF specific sync override Esben Haabendal
@ 2026-08-10 7:14 ` Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set() Esben Haabendal
2026-08-10 7:14 ` [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing Esben Haabendal
4 siblings, 0 replies; 9+ messages in thread
From: Esben Haabendal @ 2026-08-10 7:14 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg
Cc: Esben Haabendal, dri-devel, linux-kernel, stable, Laurentiu Palcu
From: Robert Chiras <robert.chiras@nxp.com>
In order to respect the DSI protocol, make sure that auto-insert EOTP is
enabled according to the NO_EOT_PACKET flag instead of the
CLOCK_NON_CONTINUOUS flag.
Fixes: 44cfc6233447 ("drm/bridge: Add NWL MIPI DSI host controller support")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
Cc: Robert Chiras <robert.chiras@nxp.com>
Cc: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
drivers/gpu/drm/bridge/nwl-dsi.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index 374884e76f25..3fd41212933c 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -222,13 +222,15 @@ static int nwl_dsi_config_host(struct nwl_dsi *dsi)
DRM_DEV_DEBUG_DRIVER(dsi->dev, "DSI Lanes %d\n", dsi->lanes);
nwl_dsi_write(dsi, NWL_DSI_CFG_NUM_LANES, dsi->lanes - 1);
- if (dsi->dsi_mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
+ if (dsi->dsi_mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
nwl_dsi_write(dsi, NWL_DSI_CFG_NONCONTINUOUS_CLK, 0x01);
- nwl_dsi_write(dsi, NWL_DSI_CFG_AUTOINSERT_EOTP, 0x01);
- } else {
+ else
nwl_dsi_write(dsi, NWL_DSI_CFG_NONCONTINUOUS_CLK, 0x00);
+
+ if (dsi->dsi_mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
nwl_dsi_write(dsi, NWL_DSI_CFG_AUTOINSERT_EOTP, 0x00);
- }
+ else
+ nwl_dsi_write(dsi, NWL_DSI_CFG_AUTOINSERT_EOTP, 0x01);
/* values in byte clock cycles */
cycles = ui2bc(cfg->clk_pre);
--
2.55.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set()
2026-08-10 7:14 [PATCH v3 0/5] drm/bridge: nwl-dsi: Various fixes Esben Haabendal
` (2 preceding siblings ...)
2026-08-10 7:14 ` [PATCH v3 3/5] drm/bridge: nwl-dsi: Correct auto-insert EOTP behavior Esben Haabendal
@ 2026-08-10 7:14 ` Esben Haabendal
2026-08-10 7:22 ` sashiko-bot
2026-08-10 7:14 ` [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing Esben Haabendal
4 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-08-10 7:14 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg
Cc: Esben Haabendal, dri-devel, linux-kernel, stable
There does not seem to be any good reasons for ignoring the return value of
the nwl_dsi_mode_set() function.
Fixes: 44cfc6233447 ("drm/bridge: Add NWL MIPI DSI host controller support")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/gpu/drm/bridge/nwl-dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index 3fd41212933c..e90588299c5e 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -884,7 +884,9 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge,
}
/* Step 2 from DSI reset-out instructions */
- nwl_dsi_mode_set(dsi);
+ ret = nwl_dsi_mode_set(dsi);
+ if (ret < 0)
+ goto runtime_put;
/* Step 3 from DSI reset-out instructions */
ret = reset_control_deassert(dsi->rst_esc);
--
2.55.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set()
2026-08-10 7:14 ` [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set() Esben Haabendal
@ 2026-08-10 7:22 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-10 7:22 UTC (permalink / raw)
To: Esben Haabendal; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Unbalanced resource teardown and PM runtime underflow in mode_set error path.
--
--- Patch [4]: [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set() ---
commit 0db3c1cc2f73a9bc66aea2633f754029dd76b743
Author: Esben Haabendal <esben@geanix.com>
drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set()
This commit adds a return value check for nwl_dsi_mode_set() in
nwl_dsi_bridge_mode_set(), jumping to an error path on failure.
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index 3fd41212933c8..e90588299c5ea 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -884,7 +884,9 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge,
> }
>
> /* Step 2 from DSI reset-out instructions */
> - nwl_dsi_mode_set(dsi);
> + ret = nwl_dsi_mode_set(dsi);
> + if (ret < 0)
> + goto runtime_put;
[Severity: High]
This is a pre-existing issue, but does taking the runtime_put error path here
lead to unbalanced resource teardown and PM runtime underflows?
Because the drm_bridge_funcs.mode_set callback returns void, failures cannot be
propagated to the DRM core to abort the commit. If nwl_dsi_mode_set() fails and
jumps to runtime_put, it calls pm_runtime_put_sync() and returns early.
However, the DRM core still considers the bridge pipeline state to be 'enabled',
and will unconditionally call nwl_dsi_bridge_atomic_disable() during the next
pipeline teardown.
When atomic_disable() runs, it unconditionally calls pm_runtime_put(), leading
to a reference count underflow:
drivers/gpu/drm/bridge/nwl-dsi.c:nwl_dsi_bridge_atomic_disable() {
...
pm_runtime_put(dsi->dev);
}
Furthermore, atomic_disable() unconditionally calls nwl_dsi_disable(), which
blindly attempts to tear down the PHY:
drivers/gpu/drm/bridge/nwl-dsi.c:nwl_dsi_bridge_atomic_disable() {
...
nwl_dsi_disable(dsi);
...
}
Since nwl_dsi_mode_set() already cleans up its own PHY resources on internal
failure, this double-disable pushes the PHY framework's power_count and
init_count to -1. The next attempt to initialize the display will increment
these counters to 0, skipping the actual hardware ops->power_on() and
ops->init() calls, permanently leaving the display broken until the module is
reloaded.
While this broken error path already existed for clock initialization failures
in this function, wiring up nwl_dsi_mode_set's return value to it exposes the
PHY breakage on a new failure path.
>
> /* Step 3 from DSI reset-out instructions */
> ret = reset_control_deassert(dsi->rst_esc);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-nwl-dsi-fixes-v3-0-256da9d672e1@geanix.com?part=4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing
2026-08-10 7:14 [PATCH v3 0/5] drm/bridge: nwl-dsi: Various fixes Esben Haabendal
` (3 preceding siblings ...)
2026-08-10 7:14 ` [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set() Esben Haabendal
@ 2026-08-10 7:14 ` Esben Haabendal
2026-08-10 7:26 ` sashiko-bot
4 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-08-10 7:14 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Guido Günther, Fabio Estevam, Robert Chiras, Sam Ravnborg
Cc: Esben Haabendal, dri-devel, linux-kernel, Oliver F. Brown
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 <esben@geanix.com>
Cc: Robert Chiras <robert.chiras@nxp.com>
Cc: Oliver F. Brown <oliver.brown@oss.nxp.com>
---
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 <linux/reset.h>
#include <linux/sys_soc.h>
#include <linux/time64.h>
+#include <linux/math64.h>
#include <drm/drm_atomic_state_helper.h>
#include <drm/drm_bridge.h>
@@ -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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing
2026-08-10 7:14 ` [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing Esben Haabendal
@ 2026-08-10 7:26 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-10 7:26 UTC (permalink / raw)
To: Esben Haabendal; +Cc: dri-devel
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 <esben@geanix.com>
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/20260810-nwl-dsi-fixes-v3-0-256da9d672e1@geanix.com?part=5
^ permalink raw reply [flat|nested] 9+ messages in thread