This series contains various fixes and cleanups for TC358768. The target
of this work is to get TC358768 working on Toradex's AM62 based board,
which has the following display pipeline:
AM62 DPI -> TC358768 -> LT8912B -> HDMI connector
The two main things the series does:
- Improves DSI HSW, HFP and VSDly calculations
- Adds DRM_BRIDGE_ATTACH_NO_CONNECTOR support
Tomi
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Tomi Valkeinen (11):
drm/bridge: tc358768: Fix use of uninitialized variable
drm/bridge: tc358768: Fix bit updates
drm/bridge: tc358768: Cleanup PLL calculations
drm/bridge: tc358768: Use struct videomode
drm/bridge: tc358768: Print logical values, not raw register values
drm/bridge: tc358768: Use dev for dbg prints, not priv->dev
drm/bridge: tc358768: Rename dsibclk to hsbyteclk
drm/bridge: tc358768: Clean up clock period code
drm/bridge: tc358768: Fix tc358768_ns_to_cnt()
drm/bridge: tc358768: Attempt to fix DSI horizontal timings
drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support
drivers/gpu/drm/bridge/tc358768.c | 427 +++++++++++++++++++++++++++-----------
1 file changed, 309 insertions(+), 118 deletions(-)
---
base-commit: b0e9267d4ccce9be9217337f4bc364ca24cf7f73
change-id: 20230804-tc358768-1b6949ef2e3d
Best regards,
--
Tomi Valkeinen <[email protected]>
smatch reports:
drivers/gpu/drm/bridge/tc358768.c:223 tc358768_update_bits() error: uninitialized symbol 'orig'.
Fix this by bailing out from tc358768_update_bits() if the
tc358768_read() produces an error.
Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/bridge/tc358768.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 819a4b6ec2a0..bc97a837955b 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -216,6 +216,10 @@ static void tc358768_update_bits(struct tc358768_priv *priv, u32 reg, u32 mask,
u32 tmp, orig;
tc358768_read(priv, reg, &orig);
+
+ if (priv->error)
+ return;
+
tmp = orig & ~mask;
tmp |= val & mask;
if (tmp != orig)
--
2.34.1
The DSI horizontal timing calculations done by the driver seem to often
lead to underflows or overflows, depending on the videomode.
There are two main things the current driver doesn't seem to get right:
DSI HSW and HFP, and VSDly. However, even following Toshiba's
documentation it seems we don't always get a working display.
This patch attempts to fix the horizontal timings for DSI event mode, and
on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
seem to work. The work relies on Toshiba's documentation, but also quite
a bit on empirical testing.
This also adds timing related debug prints to make it easier to improve
on this later.
The DSI pulse mode has only been tested with a fixed-resolution panel,
which limits the testing of different modes on DSI pulse mode. However,
as the VSDly calculation also affects pulse mode, so this might cause a
regression.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/bridge/tc358768.c | 211 +++++++++++++++++++++++++++++++++-----
1 file changed, 183 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index dc2241c18dde..ea19de5509ed 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -9,6 +9,7 @@
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
+#include <linux/math64.h>
#include <linux/media-bus-format.h>
#include <linux/minmax.h>
#include <linux/module.h>
@@ -157,6 +158,7 @@ struct tc358768_priv {
u32 frs; /* PLL Freqency range for HSCK (post divider) */
u32 dsiclk; /* pll_clk / 2 */
+ u32 pclk; /* incoming pclk rate */
};
static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
@@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
priv->prd = best_prd;
priv->frs = frs;
priv->dsiclk = best_pll / 2;
+ priv->pclk = mode->clock * 1000;
return 0;
}
@@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
return ps / 1000;
}
+static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
+{
+ return (u32)div_u64((u64)val * NANO, pclk);
+}
+
+/* Convert value in DPI pixel clock units to DSI byte count */
+static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
+{
+ u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
+ u64 n = priv->pclk;
+
+ return (u32)div_u64(m + n - 1, n);
+}
+
+static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
+{
+ u64 m = (u64)val * NANO;
+ u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
+
+ return (u32)div_u64(m, n);
+}
+
static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
{
struct tc358768_priv *priv = bridge_to_tc358768(bridge);
@@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
s32 raw_val;
const struct drm_display_mode *mode;
u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
- u32 dsiclk, hsbyteclk, video_start;
- const u32 internal_delay = 40;
+ u32 dsiclk, hsbyteclk;
int ret, i;
struct videomode vm;
struct device *dev = priv->dev;
+ /* In pixelclock units */
+ u32 dpi_htot, dpi_data_start;
+ /* In byte units */
+ u32 dsi_dpi_htot, dsi_dpi_data_start;
+ u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
+ const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
+ /* In hsbyteclk units */
+ u32 dsi_vsdly;
+ const u32 internal_dly = 40;
if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
@@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
case MIPI_DSI_FMT_RGB888:
val |= (0x3 << 4);
hact = vm.hactive * 3;
- video_start = (vm.hsync_len + vm.hback_porch) * 3;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
break;
case MIPI_DSI_FMT_RGB666:
val |= (0x4 << 4);
hact = vm.hactive * 3;
- video_start = (vm.hsync_len + vm.hback_porch) * 3;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
val |= (0x4 << 4) | BIT(3);
hact = vm.hactive * 18 / 8;
- video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
break;
case MIPI_DSI_FMT_RGB565:
val |= (0x5 << 4);
hact = vm.hactive * 2;
- video_start = (vm.hsync_len + vm.hback_porch) * 2;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
break;
default:
@@ -716,9 +745,150 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
return;
}
+ /*
+ * There are three important things to make TC358768 work correctly,
+ * which are not trivial to manage:
+ *
+ * 1. Keep the DPI line-time and the DSI line-time as close to each
+ * other as possible.
+ * 2. TC358768 goes to LP mode after each line's active area. The DSI
+ * HFP period has to be long enough for entering and exiting LP mode.
+ * But it is not clear how to calculate this.
+ * 3. VSDly (video start delay) has to be long enough to ensure that the
+ * DSI TX does not start transmitting util we have started receiving
+ * pixel data from the DPI input. It is not clear how to calculate
+ * this either.
+ */
+
+ dpi_htot = vm.hactive + vm.hfront_porch + vm.hsync_len + vm.hback_porch;
+ dpi_data_start = vm.hsync_len + vm.hback_porch;
+
+ dev_dbg(dev, "dpi horiz timing (pclk): %u + %u + %u + %u = %u\n",
+ vm.hsync_len, vm.hback_porch, vm.hactive, vm.hfront_porch,
+ dpi_htot);
+
+ dev_dbg(dev, "dpi horiz timing (ns): %u + %u + %u + %u = %u\n",
+ tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
+ tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
+ tc358768_dpi_to_ns(vm.hactive, vm.pixelclock),
+ tc358768_dpi_to_ns(vm.hfront_porch, vm.pixelclock),
+ tc358768_dpi_to_ns(dpi_htot, vm.pixelclock));
+
+ dev_dbg(dev, "dpi data start (ns): %u + %u = %u\n",
+ tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
+ tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
+ tc358768_dpi_to_ns(dpi_data_start, vm.pixelclock));
+
+ dsi_dpi_htot = tc358768_dpi_to_dsi_bytes(priv, dpi_htot);
+ dsi_dpi_data_start = tc358768_dpi_to_dsi_bytes(priv, dpi_data_start);
+
+ if (dsi_dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
+ dsi_hsw = tc358768_dpi_to_dsi_bytes(priv, vm.hsync_len);
+ dsi_hbp = tc358768_dpi_to_dsi_bytes(priv, vm.hback_porch);
+ } else {
+ /* HBP is included in HSW in event mode */
+ dsi_hbp = 0;
+ dsi_hsw = tc358768_dpi_to_dsi_bytes(priv,
+ vm.hsync_len + vm.hback_porch);
+
+ /*
+ * The pixel packet includes the actual pixel data, and:
+ * DSI packet header = 4 bytes
+ * DCS code = 1 byte
+ * DSI packet footer = 2 bytes
+ */
+ dsi_hact = hact + 4 + 1 + 2;
+
+ dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
+
+ /*
+ * Here we should check if HFP is long enough for entering LP
+ * and exiting LP, but it's not clear how to calculate that.
+ * Instead, this is a naive algorithm that just adjusts the HFP
+ * and HSW so that HFP is (at least) roughly 2/3 of the total
+ * blanking time.
+ */
+ if (dsi_hfp < (dsi_hfp + dsi_hsw + dsi_hss) * 2 / 3) {
+ u32 old_hfp = dsi_hfp;
+ u32 old_hsw = dsi_hsw;
+ u32 tot = dsi_hfp + dsi_hsw + dsi_hss;
+
+ dsi_hsw = tot / 3;
+
+ /*
+ * Seems like sometimes HSW has to be divisible by num-lanes, but
+ * not always...
+ */
+ dsi_hsw = roundup(dsi_hsw, priv->dsi_lanes);
+
+ dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
+
+ dev_dbg(dev,
+ "hfp too short, adjusting dsi hfp and dsi hsw from %u, %u to %u, %u\n",
+ old_hfp, old_hsw, dsi_hfp, dsi_hsw);
+ }
+
+ dev_dbg(dev,
+ "dsi horiz timing (bytes): %u, %u + %u + %u + %u = %u\n",
+ dsi_hss, dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp,
+ dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp);
+
+ dev_dbg(dev, "dsi horiz timing (ns): %u + %u + %u + %u + %u = %u\n",
+ tc358768_dsi_bytes_to_ns(priv, dsi_hss),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hact),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hfp),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp));
+ }
+
+ /* VSDly calculation */
+
+ /* Start with the HW internal delay */
+ dsi_vsdly = internal_dly;
+
+ /* Convert to byte units as the other variables are in byte units */
+ dsi_vsdly *= priv->dsi_lanes;
+
+ /* Do we need more delay, in addition to the internal? */
+ if (dsi_dpi_data_start > dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp) {
+ dsi_vsdly = dsi_dpi_data_start - dsi_hss - dsi_hsw - dsi_hbp;
+ dsi_vsdly = roundup(dsi_vsdly, priv->dsi_lanes);
+ }
+
+ dev_dbg(dev, "dsi data start (bytes) %u + %u + %u + %u = %u\n",
+ dsi_vsdly, dsi_hss, dsi_hsw, dsi_hbp,
+ dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp);
+
+ dev_dbg(dev, "dsi data start (ns) %u + %u + %u + %u = %u\n",
+ tc358768_dsi_bytes_to_ns(priv, dsi_vsdly),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hss),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
+ tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
+ tc358768_dsi_bytes_to_ns(priv, dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp));
+
+ /* Convert back to hsbyteclk */
+ dsi_vsdly /= priv->dsi_lanes;
+
+ /*
+ * The docs say that there is an internal delay of 40 cycles.
+ * However, we get underflows if we follow that rule. If we
+ * instead ignore the internal delay, things work. So either
+ * the docs are wrong or the calculations are wrong.
+ *
+ * As a temporary fix, add the internal delay here, to counter
+ * the subtraction when writing the register.
+ */
+ dsi_vsdly += internal_dly;
+
+ /* Clamp to the register max */
+ if (dsi_vsdly - internal_dly > 0x3ff) {
+ dev_warn(dev, "VSDly too high, underflows likely\n");
+ dsi_vsdly = 0x3ff + internal_dly;
+ }
+
/* VSDly[9:0] */
- video_start = max(video_start, internal_delay + 1) - internal_delay;
- tc358768_write(priv, TC358768_VSDLY, video_start);
+ tc358768_write(priv, TC358768_VSDLY, dsi_vsdly - internal_dly);
tc358768_write(priv, TC358768_DATAFMT, val);
tc358768_write(priv, TC358768_DSITX_DT, data_type);
@@ -826,18 +996,6 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
/* vbp */
tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
-
- /* hsw * byteclk * ndl / pclk */
- val = (u32)div_u64(vm.hsync_len *
- (u64)hsbyteclk * priv->dsi_lanes,
- vm.pixelclock);
- tc358768_write(priv, TC358768_DSI_HSW, val);
-
- /* hbp * byteclk * ndl / pclk */
- val = (u32)div_u64(vm.hback_porch *
- (u64)hsbyteclk * priv->dsi_lanes,
- vm.pixelclock);
- tc358768_write(priv, TC358768_DSI_HBPR, val);
} else {
/* Set event mode */
tc358768_write(priv, TC358768_DSI_EVENT, 1);
@@ -851,16 +1009,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
/* vbp (not used in event mode) */
tc358768_write(priv, TC358768_DSI_VBPR, 0);
+ }
- /* (hsw + hbp) * byteclk * ndl / pclk */
- val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
- (u64)hsbyteclk * priv->dsi_lanes,
- vm.pixelclock);
- tc358768_write(priv, TC358768_DSI_HSW, val);
+ /* hsw (bytes) */
+ tc358768_write(priv, TC358768_DSI_HSW, dsi_hsw);
- /* hbp (not used in event mode) */
- tc358768_write(priv, TC358768_DSI_HBPR, 0);
- }
+ /* hbp (bytes) */
+ tc358768_write(priv, TC358768_DSI_HBPR, dsi_hbp);
/* hact (bytes) */
tc358768_write(priv, TC358768_DSI_HACT, hact);
--
2.34.1
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index ea19de5509ed..a567f136ddc7 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
struct tc358768_dsi_output {
struct mipi_dsi_device *dev;
+
+ /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
struct drm_panel *panel;
- struct drm_bridge *bridge;
+
+ /*
+ * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
+ * to tc358768, 'next_bridge' contains the bridge the driver created
+ * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
+ * the next bridge the driver found.
+ */
+ struct drm_bridge *next_bridge;
};
struct tc358768_priv {
@@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
struct mipi_dsi_device *dev)
{
struct tc358768_priv *priv = dsi_host_to_tc358768(host);
- struct drm_bridge *bridge;
- struct drm_panel *panel;
struct device_node *ep;
int ret;
@@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
return -ENOTSUPP;
}
- ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
- &bridge);
- if (ret)
- return ret;
-
- if (panel) {
- bridge = drm_panel_bridge_add_typed(panel,
- DRM_MODE_CONNECTOR_DSI);
- if (IS_ERR(bridge))
- return PTR_ERR(bridge);
- }
-
priv->output.dev = dev;
- priv->output.bridge = bridge;
- priv->output.panel = panel;
priv->dsi_lanes = dev->lanes;
priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
@@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
drm_bridge_remove(&priv->bridge);
if (priv->output.panel)
- drm_panel_bridge_remove(priv->output.bridge);
+ drm_panel_bridge_remove(priv->output.next_bridge);
return 0;
}
@@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
return -ENOTSUPP;
}
- return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
+ if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
+ struct device_node *node;
+
+ /* Get the next bridge, connected to port@1. */
+ node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
+ if (!node)
+ return -ENODEV;
+
+ priv->output.next_bridge = of_drm_find_bridge(node);
+ of_node_put(node);
+ if (!priv->output.next_bridge)
+ return -EPROBE_DEFER;
+ } else {
+ struct drm_bridge *bridge;
+ struct drm_panel *panel;
+ int ret;
+
+ ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
+ &panel, &bridge);
+ if (ret)
+ return ret;
+
+ if (panel) {
+ bridge = drm_panel_bridge_add_typed(panel,
+ DRM_MODE_CONNECTOR_DSI);
+ if (IS_ERR(bridge))
+ return PTR_ERR(bridge);
+ }
+
+ priv->output.next_bridge = bridge;
+ priv->output.panel = panel;
+ }
+
+ return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
flags);
}
--
2.34.1
The driver has a few places where it does:
if (thing_is_enabled_in_config)
update_thing_bit_in_hw()
This means that if the thing is _not_ enabled, the bit never gets
cleared. This affects the h/vsyncs and continuous DSI clock bits.
Fix the driver to always update the bit.
Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index bc97a837955b..b668f77673c3 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
val |= BIT(i + 1);
tc358768_write(priv, TC358768_HSTXVREGEN, val);
- if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
- tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
+ tc358768_write(priv, TC358768_TXOPTIONCNTRL,
+ (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
@@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
tc358768_write(priv, TC358768_DSI_HACT, hact);
/* VSYNC polarity */
- if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
- tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
+ tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
+ (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
+
/* HSYNC polarity */
- if (mode->flags & DRM_MODE_FLAG_PHSYNC)
- tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0), BIT(0));
+ tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0),
+ (mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIT(0) : 0);
/* Start DSI Tx */
tc358768_write(priv, TC358768_DSI_START, 0x1);
--
2.34.1
The tc358768_ns_to_cnt() is, most likely, supposed to do a div-round-up
operation, but it misses subtracting one from the dividend.
Fix this by just using DIV_ROUND_UP().
Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/bridge/tc358768.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 9411b0fb471e..dc2241c18dde 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -630,7 +630,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
{
- return (ns * 1000 + period_ps) / period_ps;
+ return DIV_ROUND_UP(ns * 1000, period_ps);
}
static u32 tc358768_ps_to_ns(u32 ps)
--
2.34.1
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> smatch reports:
>
> drivers/gpu/drm/bridge/tc358768.c:223 tc358768_update_bits() error: uninitialized symbol 'orig'.
>
> Fix this by bailing out from tc358768_update_bits() if the
> tc358768_read() produces an error.
>
> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 819a4b6ec2a0..bc97a837955b 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -216,6 +216,10 @@ static void tc358768_update_bits(struct tc358768_priv *priv, u32 reg, u32 mask,
> u32 tmp, orig;
>
> tc358768_read(priv, reg, &orig);
> +
no need for blank line
> + if (priv->error)
> + return;
> +
> tmp = orig & ~mask;
> tmp |= val & mask;
> if (tmp != orig)
>
--
Péter
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The tc358768_ns_to_cnt() is, most likely, supposed to do a div-round-up
> operation, but it misses subtracting one from the dividend.
>
> Fix this by just using DIV_ROUND_UP().
Reviewed-by: Peter Ujfalusi <[email protected]>
> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 9411b0fb471e..dc2241c18dde 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -630,7 +630,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
>
> static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
> {
> - return (ns * 1000 + period_ps) / period_ps;
> + return DIV_ROUND_UP(ns * 1000, period_ps);
> }
>
> static u32 tc358768_ps_to_ns(u32 ps)
>
--
Péter
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The driver has a few places where it does:
>
> if (thing_is_enabled_in_config)
> update_thing_bit_in_hw()
>
> This means that if the thing is _not_ enabled, the bit never gets
> cleared. This affects the h/vsyncs and continuous DSI clock bits.
I guess the idea was to keep the reset value unless it needs to be flipped.
>
> Fix the driver to always update the bit.
>
> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index bc97a837955b..b668f77673c3 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> val |= BIT(i + 1);
> tc358768_write(priv, TC358768_HSTXVREGEN, val);
>
> - if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> - tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
> + tc358768_write(priv, TC358768_TXOPTIONCNTRL,
> + (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>
> /* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
> val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> tc358768_write(priv, TC358768_DSI_HACT, hact);
>
> /* VSYNC polarity */
> - if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
> - tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
> + tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
> + (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
Was this the reverse before and should be:
(mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
> +
> /* HSYNC polarity */
> - if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> - tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0), BIT(0));
> + tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0),
> + (mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIT(0) : 0);
>
> /* Start DSI Tx */
> tc358768_write(priv, TC358768_DSI_START, 0x1);
>
--
Péter
On 11/08/2023 19:44, Péter Ujfalusi wrote:
>
>
> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>
> I would rather have a commit message than a blank one.
Oops...
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>> 1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>
>> struct tc358768_dsi_output {
>> struct mipi_dsi_device *dev;
>> +
>> + /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>> struct drm_panel *panel;
>> - struct drm_bridge *bridge;
>> +
>> + /*
>> + * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> + * to tc358768, 'next_bridge' contains the bridge the driver created
>> + * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> + * the next bridge the driver found.
>> + */
>> + struct drm_bridge *next_bridge;
>
> why it is better to call it next_bridge than just bridge? Is there a
> prev_bridge also?
There is, prev bridge would be the bridge behind tc358768 in the chain.
Bridge is tc358768. Next bridge is the following one.
Here, it's in the tc358768_dsi_output struct, so bridge is perhaps ok. I
just wanted to be extra clear here, as I think it's often called
next_bridge in other drivers.
Tomi
On 11/08/2023 19:23, Péter Ujfalusi wrote:
>
>
> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>> The driver has a few places where it does:
>>
>> if (thing_is_enabled_in_config)
>> update_thing_bit_in_hw()
>>
>> This means that if the thing is _not_ enabled, the bit never gets
>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>
> I guess the idea was to keep the reset value unless it needs to be flipped.
>
>>
>> Fix the driver to always update the bit.
>>
>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index bc97a837955b..b668f77673c3 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>> val |= BIT(i + 1);
>> tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>
>> - if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>> - tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>> + tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>> + (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>
>> /* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>> val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>> tc358768_write(priv, TC358768_DSI_HACT, hact);
>>
>> /* VSYNC polarity */
>> - if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>> - tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>> + tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>> + (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>
> Was this the reverse before and should be:
> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
Bit 5 is 1 for active high vsync polarity. The test was previously
!nvsync, i.e. the same as pvsync.
Tomi
On 04/08/2023 13:44, Tomi Valkeinen wrote:
I would rather have a commit message than a blank one.
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index ea19de5509ed..a567f136ddc7 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>
> struct tc358768_dsi_output {
> struct mipi_dsi_device *dev;
> +
> + /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
> struct drm_panel *panel;
> - struct drm_bridge *bridge;
> +
> + /*
> + * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
> + * to tc358768, 'next_bridge' contains the bridge the driver created
> + * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
> + * the next bridge the driver found.
> + */
> + struct drm_bridge *next_bridge;
why it is better to call it next_bridge than just bridge? Is there a
prev_bridge also?
> };
>
> struct tc358768_priv {
> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
> struct mipi_dsi_device *dev)
> {
> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> - struct drm_bridge *bridge;
> - struct drm_panel *panel;
> struct device_node *ep;
> int ret;
>
> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
> return -ENOTSUPP;
> }
>
> - ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> - &bridge);
> - if (ret)
> - return ret;
> -
> - if (panel) {
> - bridge = drm_panel_bridge_add_typed(panel,
> - DRM_MODE_CONNECTOR_DSI);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> - }
> -
> priv->output.dev = dev;
> - priv->output.bridge = bridge;
> - priv->output.panel = panel;
>
> priv->dsi_lanes = dev->lanes;
> priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>
> drm_bridge_remove(&priv->bridge);
> if (priv->output.panel)
> - drm_panel_bridge_remove(priv->output.bridge);
> + drm_panel_bridge_remove(priv->output.next_bridge);
>
> return 0;
> }
> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
> return -ENOTSUPP;
> }
>
> - return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> + struct device_node *node;
> +
> + /* Get the next bridge, connected to port@1. */
> + node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> + if (!node)
> + return -ENODEV;
> +
> + priv->output.next_bridge = of_drm_find_bridge(node);
> + of_node_put(node);
> + if (!priv->output.next_bridge)
> + return -EPROBE_DEFER;
> + } else {
> + struct drm_bridge *bridge;
> + struct drm_panel *panel;
> + int ret;
> +
> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> + &panel, &bridge);
> + if (ret)
> + return ret;
> +
> + if (panel) {
> + bridge = drm_panel_bridge_add_typed(panel,
> + DRM_MODE_CONNECTOR_DSI);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> + }
> +
> + priv->output.next_bridge = bridge;
> + priv->output.panel = panel;
> + }
> +
> + return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
> flags);
> }
>
>
--
Péter
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The DSI horizontal timing calculations done by the driver seem to often
> lead to underflows or overflows, depending on the videomode.
>
> There are two main things the current driver doesn't seem to get right:
> DSI HSW and HFP, and VSDly. However, even following Toshiba's
> documentation it seems we don't always get a working display.
>
> This patch attempts to fix the horizontal timings for DSI event mode, and
> on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
> seem to work. The work relies on Toshiba's documentation, but also quite
> a bit on empirical testing.
>
> This also adds timing related debug prints to make it easier to improve
> on this later.
>
> The DSI pulse mode has only been tested with a fixed-resolution panel,
> which limits the testing of different modes on DSI pulse mode. However,
> as the VSDly calculation also affects pulse mode, so this might cause a
> regression.
If my memory serves me right we only had this used in a sinlge, static
configuration and again, it might be mentioned in a comment somewhere?
Nice work!
Reviewed-by: Peter Ujfalusi <[email protected]>
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 211 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 183 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index dc2241c18dde..ea19de5509ed 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -9,6 +9,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/kernel.h>
> +#include <linux/math64.h>
> #include <linux/media-bus-format.h>
> #include <linux/minmax.h>
> #include <linux/module.h>
> @@ -157,6 +158,7 @@ struct tc358768_priv {
> u32 frs; /* PLL Freqency range for HSCK (post divider) */
>
> u32 dsiclk; /* pll_clk / 2 */
> + u32 pclk; /* incoming pclk rate */
> };
>
> static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
> @@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
> priv->prd = best_prd;
> priv->frs = frs;
> priv->dsiclk = best_pll / 2;
> + priv->pclk = mode->clock * 1000;
>
> return 0;
> }
> @@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
> return ps / 1000;
> }
>
> +static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
> +{
> + return (u32)div_u64((u64)val * NANO, pclk);
> +}
> +
> +/* Convert value in DPI pixel clock units to DSI byte count */
> +static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
> +{
> + u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
> + u64 n = priv->pclk;
> +
> + return (u32)div_u64(m + n - 1, n);
> +}
> +
> +static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
> +{
> + u64 m = (u64)val * NANO;
> + u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
> +
> + return (u32)div_u64(m, n);
> +}
> +
> static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> {
> struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> @@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> s32 raw_val;
> const struct drm_display_mode *mode;
> u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
> - u32 dsiclk, hsbyteclk, video_start;
> - const u32 internal_delay = 40;
> + u32 dsiclk, hsbyteclk;
> int ret, i;
> struct videomode vm;
> struct device *dev = priv->dev;
> + /* In pixelclock units */
> + u32 dpi_htot, dpi_data_start;
> + /* In byte units */
> + u32 dsi_dpi_htot, dsi_dpi_data_start;
> + u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
> + const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
> + /* In hsbyteclk units */
> + u32 dsi_vsdly;
> + const u32 internal_dly = 40;
>
> if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
> @@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> case MIPI_DSI_FMT_RGB888:
> val |= (0x3 << 4);
> hact = vm.hactive * 3;
> - video_start = (vm.hsync_len + vm.hback_porch) * 3;
> data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
> break;
> case MIPI_DSI_FMT_RGB666:
> val |= (0x4 << 4);
> hact = vm.hactive * 3;
> - video_start = (vm.hsync_len + vm.hback_porch) * 3;
> data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
> break;
>
> case MIPI_DSI_FMT_RGB666_PACKED:
> val |= (0x4 << 4) | BIT(3);
> hact = vm.hactive * 18 / 8;
> - video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
> data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
> break;
>
> case MIPI_DSI_FMT_RGB565:
> val |= (0x5 << 4);
> hact = vm.hactive * 2;
> - video_start = (vm.hsync_len + vm.hback_porch) * 2;
> data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
> break;
> default:
> @@ -716,9 +745,150 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> return;
> }
>
> + /*
> + * There are three important things to make TC358768 work correctly,
> + * which are not trivial to manage:
> + *
> + * 1. Keep the DPI line-time and the DSI line-time as close to each
> + * other as possible.
> + * 2. TC358768 goes to LP mode after each line's active area. The DSI
> + * HFP period has to be long enough for entering and exiting LP mode.
> + * But it is not clear how to calculate this.
> + * 3. VSDly (video start delay) has to be long enough to ensure that the
> + * DSI TX does not start transmitting util we have started receiving
> + * pixel data from the DPI input. It is not clear how to calculate
> + * this either.
> + */
> +
> + dpi_htot = vm.hactive + vm.hfront_porch + vm.hsync_len + vm.hback_porch;
> + dpi_data_start = vm.hsync_len + vm.hback_porch;
> +
> + dev_dbg(dev, "dpi horiz timing (pclk): %u + %u + %u + %u = %u\n",
> + vm.hsync_len, vm.hback_porch, vm.hactive, vm.hfront_porch,
> + dpi_htot);
> +
> + dev_dbg(dev, "dpi horiz timing (ns): %u + %u + %u + %u = %u\n",
> + tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
> + tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
> + tc358768_dpi_to_ns(vm.hactive, vm.pixelclock),
> + tc358768_dpi_to_ns(vm.hfront_porch, vm.pixelclock),
> + tc358768_dpi_to_ns(dpi_htot, vm.pixelclock));
> +
> + dev_dbg(dev, "dpi data start (ns): %u + %u = %u\n",
> + tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
> + tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
> + tc358768_dpi_to_ns(dpi_data_start, vm.pixelclock));
> +
> + dsi_dpi_htot = tc358768_dpi_to_dsi_bytes(priv, dpi_htot);
> + dsi_dpi_data_start = tc358768_dpi_to_dsi_bytes(priv, dpi_data_start);
> +
> + if (dsi_dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
> + dsi_hsw = tc358768_dpi_to_dsi_bytes(priv, vm.hsync_len);
> + dsi_hbp = tc358768_dpi_to_dsi_bytes(priv, vm.hback_porch);
> + } else {
> + /* HBP is included in HSW in event mode */
> + dsi_hbp = 0;
> + dsi_hsw = tc358768_dpi_to_dsi_bytes(priv,
> + vm.hsync_len + vm.hback_porch);
> +
> + /*
> + * The pixel packet includes the actual pixel data, and:
> + * DSI packet header = 4 bytes
> + * DCS code = 1 byte
> + * DSI packet footer = 2 bytes
> + */
> + dsi_hact = hact + 4 + 1 + 2;
> +
> + dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
> +
> + /*
> + * Here we should check if HFP is long enough for entering LP
> + * and exiting LP, but it's not clear how to calculate that.
> + * Instead, this is a naive algorithm that just adjusts the HFP
> + * and HSW so that HFP is (at least) roughly 2/3 of the total
> + * blanking time.
> + */
> + if (dsi_hfp < (dsi_hfp + dsi_hsw + dsi_hss) * 2 / 3) {
> + u32 old_hfp = dsi_hfp;
> + u32 old_hsw = dsi_hsw;
> + u32 tot = dsi_hfp + dsi_hsw + dsi_hss;
> +
> + dsi_hsw = tot / 3;
> +
> + /*
> + * Seems like sometimes HSW has to be divisible by num-lanes, but
> + * not always...
> + */
> + dsi_hsw = roundup(dsi_hsw, priv->dsi_lanes);
> +
> + dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
> +
> + dev_dbg(dev,
> + "hfp too short, adjusting dsi hfp and dsi hsw from %u, %u to %u, %u\n",
> + old_hfp, old_hsw, dsi_hfp, dsi_hsw);
> + }
> +
> + dev_dbg(dev,
> + "dsi horiz timing (bytes): %u, %u + %u + %u + %u = %u\n",
> + dsi_hss, dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp,
> + dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp);
> +
> + dev_dbg(dev, "dsi horiz timing (ns): %u + %u + %u + %u + %u = %u\n",
> + tc358768_dsi_bytes_to_ns(priv, dsi_hss),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hact),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hfp),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp));
> + }
> +
> + /* VSDly calculation */
> +
> + /* Start with the HW internal delay */
> + dsi_vsdly = internal_dly;
> +
> + /* Convert to byte units as the other variables are in byte units */
> + dsi_vsdly *= priv->dsi_lanes;
> +
> + /* Do we need more delay, in addition to the internal? */
> + if (dsi_dpi_data_start > dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp) {
> + dsi_vsdly = dsi_dpi_data_start - dsi_hss - dsi_hsw - dsi_hbp;
> + dsi_vsdly = roundup(dsi_vsdly, priv->dsi_lanes);
> + }
> +
> + dev_dbg(dev, "dsi data start (bytes) %u + %u + %u + %u = %u\n",
> + dsi_vsdly, dsi_hss, dsi_hsw, dsi_hbp,
> + dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp);
> +
> + dev_dbg(dev, "dsi data start (ns) %u + %u + %u + %u = %u\n",
> + tc358768_dsi_bytes_to_ns(priv, dsi_vsdly),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hss),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
> + tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
> + tc358768_dsi_bytes_to_ns(priv, dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp));
> +
> + /* Convert back to hsbyteclk */
> + dsi_vsdly /= priv->dsi_lanes;
> +
> + /*
> + * The docs say that there is an internal delay of 40 cycles.
> + * However, we get underflows if we follow that rule. If we
> + * instead ignore the internal delay, things work. So either
> + * the docs are wrong or the calculations are wrong.
> + *
> + * As a temporary fix, add the internal delay here, to counter
> + * the subtraction when writing the register.
> + */
> + dsi_vsdly += internal_dly;
> +
> + /* Clamp to the register max */
> + if (dsi_vsdly - internal_dly > 0x3ff) {
> + dev_warn(dev, "VSDly too high, underflows likely\n");
> + dsi_vsdly = 0x3ff + internal_dly;
> + }
> +
> /* VSDly[9:0] */
> - video_start = max(video_start, internal_delay + 1) - internal_delay;
> - tc358768_write(priv, TC358768_VSDLY, video_start);
> + tc358768_write(priv, TC358768_VSDLY, dsi_vsdly - internal_dly);
>
> tc358768_write(priv, TC358768_DATAFMT, val);
> tc358768_write(priv, TC358768_DSITX_DT, data_type);
> @@ -826,18 +996,6 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>
> /* vbp */
> tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
> -
> - /* hsw * byteclk * ndl / pclk */
> - val = (u32)div_u64(vm.hsync_len *
> - (u64)hsbyteclk * priv->dsi_lanes,
> - vm.pixelclock);
> - tc358768_write(priv, TC358768_DSI_HSW, val);
> -
> - /* hbp * byteclk * ndl / pclk */
> - val = (u32)div_u64(vm.hback_porch *
> - (u64)hsbyteclk * priv->dsi_lanes,
> - vm.pixelclock);
> - tc358768_write(priv, TC358768_DSI_HBPR, val);
> } else {
> /* Set event mode */
> tc358768_write(priv, TC358768_DSI_EVENT, 1);
> @@ -851,16 +1009,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>
> /* vbp (not used in event mode) */
> tc358768_write(priv, TC358768_DSI_VBPR, 0);
> + }
>
> - /* (hsw + hbp) * byteclk * ndl / pclk */
> - val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
> - (u64)hsbyteclk * priv->dsi_lanes,
> - vm.pixelclock);
> - tc358768_write(priv, TC358768_DSI_HSW, val);
> + /* hsw (bytes) */
> + tc358768_write(priv, TC358768_DSI_HSW, dsi_hsw);
>
> - /* hbp (not used in event mode) */
> - tc358768_write(priv, TC358768_DSI_HBPR, 0);
> - }
> + /* hbp (bytes) */
> + tc358768_write(priv, TC358768_DSI_HBPR, dsi_hbp);
>
> /* hact (bytes) */
> tc358768_write(priv, TC358768_DSI_HACT, hact);
>
--
Péter
Hi,
On 11.08.23 19:02, Tomi Valkeinen wrote:
> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>
>>
>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>> The driver has a few places where it does:
>>>
>>> if (thing_is_enabled_in_config)
>>> update_thing_bit_in_hw()
>>>
>>> This means that if the thing is _not_ enabled, the bit never gets
>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>
>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>
>>>
>>> Fix the driver to always update the bit.
>>>
>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>> Signed-off-by: Tomi Valkeinen <[email protected]>
>>> ---
>>> drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>> index bc97a837955b..b668f77673c3 100644
>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>> val |= BIT(i + 1);
>>> tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>
>>> - if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>> - tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>> + tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>> + (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>
>>> /* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>> val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>> tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>
>>> /* VSYNC polarity */
>>> - if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>> - tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>> + tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>> + (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>
>> Was this the reverse before and should be:
>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>
> Bit 5 is 1 for active high vsync polarity. The test was previously
> !nvsync, i.e. the same as pvsync.
this statement doesn't seem to be true, since this change causes a
regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
false in the present case.
Best regards,
Maxim
On 04.08.23 12:44, Tomi Valkeinen wrote:
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index ea19de5509ed..a567f136ddc7 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>
> struct tc358768_dsi_output {
> struct mipi_dsi_device *dev;
> +
> + /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
> struct drm_panel *panel;
> - struct drm_bridge *bridge;
> +
> + /*
> + * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
> + * to tc358768, 'next_bridge' contains the bridge the driver created
> + * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
> + * the next bridge the driver found.
> + */
> + struct drm_bridge *next_bridge;
> };
>
> struct tc358768_priv {
> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
> struct mipi_dsi_device *dev)
> {
> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> - struct drm_bridge *bridge;
> - struct drm_panel *panel;
> struct device_node *ep;
> int ret;
>
> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
> return -ENOTSUPP;
> }
>
> - ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> - &bridge);
> - if (ret)
> - return ret;
> -
> - if (panel) {
> - bridge = drm_panel_bridge_add_typed(panel,
> - DRM_MODE_CONNECTOR_DSI);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> - }
> -
> priv->output.dev = dev;
> - priv->output.bridge = bridge;
> - priv->output.panel = panel;
>
> priv->dsi_lanes = dev->lanes;
> priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>
> drm_bridge_remove(&priv->bridge);
> if (priv->output.panel)
> - drm_panel_bridge_remove(priv->output.bridge);
> + drm_panel_bridge_remove(priv->output.next_bridge);
>
> return 0;
> }
> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
> return -ENOTSUPP;
> }
>
> - return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> + struct device_node *node;
> +
> + /* Get the next bridge, connected to port@1. */
> + node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> + if (!node)
> + return -ENODEV;
> +
> + priv->output.next_bridge = of_drm_find_bridge(node);
> + of_node_put(node);
> + if (!priv->output.next_bridge)
> + return -EPROBE_DEFER;
> + } else {
> + struct drm_bridge *bridge;
> + struct drm_panel *panel;
> + int ret;
> +
> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> + &panel, &bridge);
> + if (ret)
> + return ret;
> +
> + if (panel) {
> + bridge = drm_panel_bridge_add_typed(panel,
> + DRM_MODE_CONNECTOR_DSI);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> + }
> +
> + priv->output.next_bridge = bridge;
> + priv->output.panel = panel;
> + }
> +
> + return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
> flags);
> }
>
>
This patch unfortunately breaks the display output on the Asus TF700T:
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
tegra-dc 54200000.dc: failed to initialize RGB output: -517
drm drm: failed to initialize 54200000.dc: -517
------------[ cut here ]------------
WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
refcount_t: underflow; use-after-free.
Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x40/0x4c
dump_stack_lvl from __warn+0x94/0xc0
__warn from warn_slowpath_fmt+0x118/0x16c
warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
tegra_dc_init from host1x_device_init+0x84/0x15c
host1x_device_init from host1x_drm_probe+0xd8/0x3c4
host1x_drm_probe from really_probe+0xc8/0x2dc
really_probe from __driver_probe_device+0x88/0x19c
__driver_probe_device from driver_probe_device+0x30/0x104
driver_probe_device from __device_attach_driver+0x94/0x108
__device_attach_driver from bus_for_each_drv+0x80/0xb8
bus_for_each_drv from __device_attach+0xa0/0x190
__device_attach from bus_probe_device+0x88/0x8c
bus_probe_device from deferred_probe_work_func+0x78/0xa4
deferred_probe_work_func from process_one_work+0x208/0x420
process_one_work from worker_thread+0x54/0x550
worker_thread from kthread+0xdc/0xf8
kthread from ret_from_fork+0x14/0x2c
Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
5fa0: 00000000 00000000 00000000 00000000
5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 0000000000000000 ]---
Best regards,
Maxim
On 13/08/2023 20:11, Maxim Schwalm wrote:
> On 04.08.23 12:44, Tomi Valkeinen wrote:
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>> 1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>
>> struct tc358768_dsi_output {
>> struct mipi_dsi_device *dev;
>> +
>> + /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>> struct drm_panel *panel;
>> - struct drm_bridge *bridge;
>> +
>> + /*
>> + * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> + * to tc358768, 'next_bridge' contains the bridge the driver created
>> + * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> + * the next bridge the driver found.
>> + */
>> + struct drm_bridge *next_bridge;
>> };
>>
>> struct tc358768_priv {
>> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>> struct mipi_dsi_device *dev)
>> {
>> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>> - struct drm_bridge *bridge;
>> - struct drm_panel *panel;
>> struct device_node *ep;
>> int ret;
>>
>> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>> return -ENOTSUPP;
>> }
>>
>> - ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
>> - &bridge);
>> - if (ret)
>> - return ret;
>> -
>> - if (panel) {
>> - bridge = drm_panel_bridge_add_typed(panel,
>> - DRM_MODE_CONNECTOR_DSI);
>> - if (IS_ERR(bridge))
>> - return PTR_ERR(bridge);
>> - }
>> -
>> priv->output.dev = dev;
>> - priv->output.bridge = bridge;
>> - priv->output.panel = panel;
>>
>> priv->dsi_lanes = dev->lanes;
>> priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
>> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>
>> drm_bridge_remove(&priv->bridge);
>> if (priv->output.panel)
>> - drm_panel_bridge_remove(priv->output.bridge);
>> + drm_panel_bridge_remove(priv->output.next_bridge);
>>
>> return 0;
>> }
>> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>> return -ENOTSUPP;
>> }
>>
>> - return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> + struct device_node *node;
>> +
>> + /* Get the next bridge, connected to port@1. */
>> + node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> + if (!node)
>> + return -ENODEV;
>> +
>> + priv->output.next_bridge = of_drm_find_bridge(node);
>> + of_node_put(node);
>> + if (!priv->output.next_bridge)
>> + return -EPROBE_DEFER;
>> + } else {
>> + struct drm_bridge *bridge;
>> + struct drm_panel *panel;
>> + int ret;
>> +
>> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> + &panel, &bridge);
>> + if (ret)
>> + return ret;
>> +
>> + if (panel) {
>> + bridge = drm_panel_bridge_add_typed(panel,
>> + DRM_MODE_CONNECTOR_DSI);
>> + if (IS_ERR(bridge))
>> + return PTR_ERR(bridge);
>> + }
>> +
>> + priv->output.next_bridge = bridge;
>> + priv->output.panel = panel;
>> + }
>> +
>> + return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>> flags);
>> }
>>
>>
> This patch unfortunately breaks the display output on the Asus TF700T:
>
> [drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
> tegra-dc 54200000.dc: failed to initialize RGB output: -517
> drm drm: failed to initialize 54200000.dc: -517
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
> refcount_t: underflow; use-after-free.
> Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
> CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x40/0x4c
> dump_stack_lvl from __warn+0x94/0xc0
> __warn from warn_slowpath_fmt+0x118/0x16c
> warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
> tegra_dc_init from host1x_device_init+0x84/0x15c
> host1x_device_init from host1x_drm_probe+0xd8/0x3c4
> host1x_drm_probe from really_probe+0xc8/0x2dc
> really_probe from __driver_probe_device+0x88/0x19c
> __driver_probe_device from driver_probe_device+0x30/0x104
> driver_probe_device from __device_attach_driver+0x94/0x108
> __device_attach_driver from bus_for_each_drv+0x80/0xb8
> bus_for_each_drv from __device_attach+0xa0/0x190
> __device_attach from bus_probe_device+0x88/0x8c
> bus_probe_device from deferred_probe_work_func+0x78/0xa4
> deferred_probe_work_func from process_one_work+0x208/0x420
> process_one_work from worker_thread+0x54/0x550
> worker_thread from kthread+0xdc/0xf8
> kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
> 5fa0: 00000000 00000000 00000000 00000000
> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 0000000000000000 ]---
Sounds like the Tegra driver has issues cleaning up after getting a
EPROBE_DEFER.
The tc358768 driver might have an issue with a setup where a panel is
directly attached to it. I don't have such a setup, but maybe I can fake
it just to see if the probing goes ok.
Tomi
On 13/08/2023 03:23, Maxim Schwalm wrote:
> Hi,
>
> On 11.08.23 19:02, Tomi Valkeinen wrote:
>> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>>
>>>
>>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>>> The driver has a few places where it does:
>>>>
>>>> if (thing_is_enabled_in_config)
>>>> update_thing_bit_in_hw()
>>>>
>>>> This means that if the thing is _not_ enabled, the bit never gets
>>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>>
>>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>>
>>>>
>>>> Fix the driver to always update the bit.
>>>>
>>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>>> Signed-off-by: Tomi Valkeinen <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>>> index bc97a837955b..b668f77673c3 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>> val |= BIT(i + 1);
>>>> tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>>
>>>> - if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>>> - tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>>> + tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>>> + (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>>
>>>> /* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>> val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>> tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>>
>>>> /* VSYNC polarity */
>>>> - if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>>> - tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>>> + tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>>> + (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>>
>>> Was this the reverse before and should be:
>>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>>
>> Bit 5 is 1 for active high vsync polarity. The test was previously
>> !nvsync, i.e. the same as pvsync.
>
> this statement doesn't seem to be true, since this change causes a
> regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
> false in the present case.
panasonic_vvx10f004b00_mode in panel_simple.c doesn't seem to have mode
flags set. I would say that means the panel doesn't care about the sync
polarities (which obviously is not the case), but maybe there's an
assumption that if sync polarities are not set, the default is...
positive? But I can't find any mention about this.
Does it work for you if you set the polarities in
panasonic_vvx10f004b00_mode?
Tomi
On Mon, Aug 14, 2023 at 12:10:41PM +0200, Sam Ravnborg wrote:
> > From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
> > From: Tomi Valkeinen <[email protected]>
> > Date: Mon, 14 Aug 2023 13:02:23 +0300
> > Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
> > support'
> >
> > Signed-off-by: Tomi Valkeinen <[email protected]>
> > ---
> > drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
> > 1 file changed, 24 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> > index 82ea4d9a814a..9705ce1bd028 100644
> > --- a/drivers/gpu/drm/bridge/tc358768.c
> > +++ b/drivers/gpu/drm/bridge/tc358768.c
> > @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
> > struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> >
> > drm_bridge_remove(&priv->bridge);
> > - if (priv->output.panel)
> > - drm_panel_bridge_remove(priv->output.next_bridge);
> >
> > return 0;
> > }
> > @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
> > enum drm_bridge_attach_flags flags)
> > {
> > struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> > + struct drm_bridge *next_bridge;
> > + struct drm_panel *panel;
> > + int ret;
> >
> > if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> > dev_err(priv->dev, "needs atomic updates support\n");
> > return -ENOTSUPP;
> > }
> >
> > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > - struct device_node *node;
> > -
> > - /* Get the next bridge, connected to port@1. */
> > - node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> > - if (!node)
> > - return -ENODEV;
> > -
> > - priv->output.next_bridge = of_drm_find_bridge(node);
> > - of_node_put(node);
> > - if (!priv->output.next_bridge)
> > - return -EPROBE_DEFER;
> > - } else {
> > - struct drm_bridge *bridge;
> > - struct drm_panel *panel;
> > - int ret;
> > -
> > - ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> > - &panel, &bridge);
> > - if (ret)
> > - return ret;
> > -
> > - if (panel) {
> > - bridge = drm_panel_bridge_add_typed(panel,
> > - DRM_MODE_CONNECTOR_DSI);
> > - if (IS_ERR(bridge))
> > - return PTR_ERR(bridge);
> > - }
> > + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
> > + &next_bridge);
>
> I think the right way is to wrap the panel in a bridge,
> so something like:
>
> next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)
>
> if (IS_ERR(next_bridge))
> return ...
> priv->output.next_bridge = next_bridge;
Should we at some point bite the bullet and wrap panels in bridges
directly in drm_panel.c ? That would simplify all the consumers.
> > + if (ret)
> > + return ret;
> >
> > - priv->output.next_bridge = bridge;
> > - priv->output.panel = panel;
> > + if (panel) {
> > + next_bridge = drm_panel_bridge_add_typed(panel,
> > + DRM_MODE_CONNECTOR_DSI);
> > + if (IS_ERR(next_bridge))
> > + return PTR_ERR(next_bridge);
> > }
> >
> > + priv->output.next_bridge = next_bridge;
> > + priv->output.panel = panel;
> > +
> > return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
> > flags);
> > }
> >
> > +void tc358768_bridge_detach(struct drm_bridge *bridge)
> > +{
> > + struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> > +
> > + if (priv->output.panel)
> > + drm_panel_bridge_remove(priv->output.next_bridge);
> > +}
> > +
> > static enum drm_mode_status
> > tc358768_bridge_mode_valid(struct drm_bridge *bridge,
> > const struct drm_display_info *info,
> > @@ -1156,6 +1147,7 @@ tc358768_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >
> > static const struct drm_bridge_funcs tc358768_bridge_funcs = {
> > .attach = tc358768_bridge_attach,
> > + .detach = tc358768_bridge_detach,
> > .mode_valid = tc358768_bridge_mode_valid,
> > .pre_enable = tc358768_bridge_pre_enable,
> > .enable = tc358768_bridge_enable,
--
Regards,
Laurent Pinchart
On 13/08/2023 20:11, Maxim Schwalm wrote:
> On 04.08.23 12:44, Tomi Valkeinen wrote:
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>> 1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>
>> struct tc358768_dsi_output {
>> struct mipi_dsi_device *dev;
>> +
>> + /* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>> struct drm_panel *panel;
>> - struct drm_bridge *bridge;
>> +
>> + /*
>> + * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> + * to tc358768, 'next_bridge' contains the bridge the driver created
>> + * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> + * the next bridge the driver found.
>> + */
>> + struct drm_bridge *next_bridge;
>> };
>>
>> struct tc358768_priv {
>> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>> struct mipi_dsi_device *dev)
>> {
>> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>> - struct drm_bridge *bridge;
>> - struct drm_panel *panel;
>> struct device_node *ep;
>> int ret;
>>
>> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>> return -ENOTSUPP;
>> }
>>
>> - ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
>> - &bridge);
>> - if (ret)
>> - return ret;
>> -
>> - if (panel) {
>> - bridge = drm_panel_bridge_add_typed(panel,
>> - DRM_MODE_CONNECTOR_DSI);
>> - if (IS_ERR(bridge))
>> - return PTR_ERR(bridge);
>> - }
>> -
>> priv->output.dev = dev;
>> - priv->output.bridge = bridge;
>> - priv->output.panel = panel;
>>
>> priv->dsi_lanes = dev->lanes;
>> priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
>> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>
>> drm_bridge_remove(&priv->bridge);
>> if (priv->output.panel)
>> - drm_panel_bridge_remove(priv->output.bridge);
>> + drm_panel_bridge_remove(priv->output.next_bridge);
>>
>> return 0;
>> }
>> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>> return -ENOTSUPP;
>> }
>>
>> - return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> + struct device_node *node;
>> +
>> + /* Get the next bridge, connected to port@1. */
>> + node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> + if (!node)
>> + return -ENODEV;
>> +
>> + priv->output.next_bridge = of_drm_find_bridge(node);
>> + of_node_put(node);
>> + if (!priv->output.next_bridge)
>> + return -EPROBE_DEFER;
>> + } else {
>> + struct drm_bridge *bridge;
>> + struct drm_panel *panel;
>> + int ret;
>> +
>> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> + &panel, &bridge);
>> + if (ret)
>> + return ret;
>> +
>> + if (panel) {
>> + bridge = drm_panel_bridge_add_typed(panel,
>> + DRM_MODE_CONNECTOR_DSI);
>> + if (IS_ERR(bridge))
>> + return PTR_ERR(bridge);
>> + }
>> +
>> + priv->output.next_bridge = bridge;
>> + priv->output.panel = panel;
>> + }
>> +
>> + return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>> flags);
>> }
>>
>>
> This patch unfortunately breaks the display output on the Asus TF700T:
>
> [drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
> tegra-dc 54200000.dc: failed to initialize RGB output: -517
> drm drm: failed to initialize 54200000.dc: -517
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
> refcount_t: underflow; use-after-free.
> Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
> CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x40/0x4c
> dump_stack_lvl from __warn+0x94/0xc0
> __warn from warn_slowpath_fmt+0x118/0x16c
> warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
> tegra_dc_init from host1x_device_init+0x84/0x15c
> host1x_device_init from host1x_drm_probe+0xd8/0x3c4
> host1x_drm_probe from really_probe+0xc8/0x2dc
> really_probe from __driver_probe_device+0x88/0x19c
> __driver_probe_device from driver_probe_device+0x30/0x104
> driver_probe_device from __device_attach_driver+0x94/0x108
> __device_attach_driver from bus_for_each_drv+0x80/0xb8
> bus_for_each_drv from __device_attach+0xa0/0x190
> __device_attach from bus_probe_device+0x88/0x8c
> bus_probe_device from deferred_probe_work_func+0x78/0xa4
> deferred_probe_work_func from process_one_work+0x208/0x420
> process_one_work from worker_thread+0x54/0x550
> worker_thread from kthread+0xdc/0xf8
> kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
> 5fa0: 00000000 00000000 00000000 00000000
> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 0000000000000000 ]---
Hi Maxim,
Can you try the attached patch (on top of the series)? If it helps, I'll
refresh the series with the fix.
Tomi
Hi Tomi,
> From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <[email protected]>
> Date: Mon, 14 Aug 2023 13:02:23 +0300
> Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
> support'
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
> 1 file changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 82ea4d9a814a..9705ce1bd028 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>
> drm_bridge_remove(&priv->bridge);
> - if (priv->output.panel)
> - drm_panel_bridge_remove(priv->output.next_bridge);
>
> return 0;
> }
> @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> + struct drm_bridge *next_bridge;
> + struct drm_panel *panel;
> + int ret;
>
> if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> dev_err(priv->dev, "needs atomic updates support\n");
> return -ENOTSUPP;
> }
>
> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> - struct device_node *node;
> -
> - /* Get the next bridge, connected to port@1. */
> - node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> - if (!node)
> - return -ENODEV;
> -
> - priv->output.next_bridge = of_drm_find_bridge(node);
> - of_node_put(node);
> - if (!priv->output.next_bridge)
> - return -EPROBE_DEFER;
> - } else {
> - struct drm_bridge *bridge;
> - struct drm_panel *panel;
> - int ret;
> -
> - ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> - &panel, &bridge);
> - if (ret)
> - return ret;
> -
> - if (panel) {
> - bridge = drm_panel_bridge_add_typed(panel,
> - DRM_MODE_CONNECTOR_DSI);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> - }
> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
> + &next_bridge);
I think the right way is to wrap the panel in a bridge,
so something like:
next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)
if (IS_ERR(next_bridge))
return ...
priv->output.next_bridge = next_bridge;
Sam
> + if (ret)
> + return ret;
>
> - priv->output.next_bridge = bridge;
> - priv->output.panel = panel;
> + if (panel) {
> + next_bridge = drm_panel_bridge_add_typed(panel,
> + DRM_MODE_CONNECTOR_DSI);
> + if (IS_ERR(next_bridge))
> + return PTR_ERR(next_bridge);
> }
>
> + priv->output.next_bridge = next_bridge;
> + priv->output.panel = panel;
> +
> return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
> flags);
> }
>
> +void tc358768_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> +
> + if (priv->output.panel)
> + drm_panel_bridge_remove(priv->output.next_bridge);
> +}
> +
> static enum drm_mode_status
> tc358768_bridge_mode_valid(struct drm_bridge *bridge,
> const struct drm_display_info *info,
> @@ -1156,6 +1147,7 @@ tc358768_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>
> static const struct drm_bridge_funcs tc358768_bridge_funcs = {
> .attach = tc358768_bridge_attach,
> + .detach = tc358768_bridge_detach,
> .mode_valid = tc358768_bridge_mode_valid,
> .pre_enable = tc358768_bridge_pre_enable,
> .enable = tc358768_bridge_enable,
> --
> 2.34.1
>
Hi Sam,
On 14/08/2023 13:10, Sam Ravnborg wrote:
> Hi Tomi,
>
>> From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
>> From: Tomi Valkeinen <[email protected]>
>> Date: Mon, 14 Aug 2023 13:02:23 +0300
>> Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
>> support'
>>
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
>> 1 file changed, 24 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index 82ea4d9a814a..9705ce1bd028 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>> struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>>
>> drm_bridge_remove(&priv->bridge);
>> - if (priv->output.panel)
>> - drm_panel_bridge_remove(priv->output.next_bridge);
>>
>> return 0;
>> }
>> @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>> enum drm_bridge_attach_flags flags)
>> {
>> struct tc358768_priv *priv = bridge_to_tc358768(bridge);
>> + struct drm_bridge *next_bridge;
>> + struct drm_panel *panel;
>> + int ret;
>>
>> if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
>> dev_err(priv->dev, "needs atomic updates support\n");
>> return -ENOTSUPP;
>> }
>>
>> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> - struct device_node *node;
>> -
>> - /* Get the next bridge, connected to port@1. */
>> - node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> - if (!node)
>> - return -ENODEV;
>> -
>> - priv->output.next_bridge = of_drm_find_bridge(node);
>> - of_node_put(node);
>> - if (!priv->output.next_bridge)
>> - return -EPROBE_DEFER;
>> - } else {
>> - struct drm_bridge *bridge;
>> - struct drm_panel *panel;
>> - int ret;
>> -
>> - ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> - &panel, &bridge);
>> - if (ret)
>> - return ret;
>> -
>> - if (panel) {
>> - bridge = drm_panel_bridge_add_typed(panel,
>> - DRM_MODE_CONNECTOR_DSI);
>> - if (IS_ERR(bridge))
>> - return PTR_ERR(bridge);
>> - }
>> + ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
>> + &next_bridge);
>
> I think the right way is to wrap the panel in a bridge,
> so something like:
>
> next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)
>
> if (IS_ERR(next_bridge))
> return ...
> priv->output.next_bridge = next_bridge;
I tried that, but I had trouble with the cleanup side.
In the fixup patch I attached in my reply to Maxim I used
drm_of_find_panel_or_bridge() + drm_panel_bridge_add_typed(), and on
bridge_detach callback I used drm_panel_bridge_remove() (if there is a
panel). This worked for me, but it does feel like a bit too much work
for a driver to do.
Tomi