[PATCH v3 00/17] drm/bridge: cdns-dsi: Make it work a bit better

[PATCH v3 00/17] drm/bridge: cdns-dsi: Make it work a bit better

[Tomi Valkeinen]

While trying to get the cdns-dsi to work on Toradex's AM69 Aquila
platform, I hit multiple issues in the driver. Basicaly nothing worked
for with the board.

This series fixes those issues. While I itch to make much larger changes
to the cdns-dsi driver, I opted to keep this series relatively simple to
make the fixes more clear and possibly help with backporting.

The series also touches tidss, but those changes are not strictly
needed, and can be merged separately. And the series also touches
cdns-dphy, and those changes are needed.

This has been tested on Toradex AM69 Aquila (upstream) and AM62P Verdin
(Toradex's BSP), with:
- HDMI output using lontium lt8912b
- LVDS panel (sn65dsi84 + panel-lvds)

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v3:
- Add Aradhya's "drm/bridge: cdns-dsi: Fix the _atomic_check()" to the
  dependencies
- The above patch from Aradhya allowed adding "drm/bridge: cdns-dsi:
  Drop crtc_* code", which resulted in quite large changes in the
  commits, even if the end result doesn't really differ.
- Reordered commits to decrease back-and-forth (e.g. fixing something in
  a a code that will be removed in the next commits)
- The reordering caused quite big changes in the commits (even if the
  final end result is more or less the same), so I chose not to add
  tested-by tags.
- Rename 'cdns_get_dphy_pll_cfg' to 'cdns_dphy_get_pll_cfg'
- Use div_u64() instead of div64_u64()
- Drop "Fail if HS rate changed when validating PHY config". This was
  too strict, as clock rounding (especially with DRM's 1kHz
  resolution...) leads to clock rates that do not match exactly.
  However, the rate mismatch should be fine as the commits adjust the
  pixel clock, and the resulting differences should be so small that we
  can't even improve the timings match by adjusting the DSI HFP, as the
  adjustment rounds to 0.
- Link to v2: https://lore.kernel.org/r/20250402-cdns-dsi-impro-v2-0-4a093eaa5e27@ideasonboard.com

Changes in v2:
- Change the tidss clock adjustment from mode_fixup() to atomic_check()
- Link to v1: https://lore.kernel.org/r/20250320-cdns-dsi-impro-v1-0-725277c5f43b@ideasonboard.com

---
Tomi Valkeinen (17):
      drm/tidss: Fix missing includes and struct decls
      drm/tidss: Use the crtc_* timings when programming the HW
      drm/tidss: Adjust the pclk based on the HW capabilities
      phy: cdns-dphy: Store hs_clk_rate and return it
      phy: cdns-dphy: Remove leftover code
      drm/bridge: cdns-dsi: Remove extra line at the end of the file
      drm/bridge: cdns-dsi: Drop crtc_* code
      drm/bridge: cdns-dsi: Remove broken fifo emptying check
      drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()
      drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()
      drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()
      drm/bridge: cdns-dsi: Adjust mode to negative syncs
      drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value
      drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi_mode2cfg()
      drm/bridge: cdns-dsi: Fix event mode
      drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs
      drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST

 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 211 +++++++++++--------------
 drivers/gpu/drm/tidss/tidss_crtc.c             |  25 ++-
 drivers/gpu/drm/tidss/tidss_dispc.c            |  22 ++-
 drivers/gpu/drm/tidss/tidss_dispc.h            |   5 +
 drivers/gpu/drm/tidss/tidss_drv.h              |   2 +
 drivers/gpu/drm/tidss/tidss_plane.h            |   2 +
 drivers/gpu/drm/tidss/tidss_scale_coefs.h      |   2 +
 drivers/phy/cadence/cdns-dphy.c                |  24 ++-
 8 files changed, 142 insertions(+), 151 deletions(-)
---
base-commit: 10646ddac2917b31c985ceff0e4982c42a9c924b
change-id: 20250320-cdns-dsi-impro-3d8fbd7848d1
prerequisite-message-id: 20250226155228.564289-1-aradhya.bhatia@linux.dev
prerequisite-patch-id: 46845a8d15042dd343a29a17fc0b9d0eec2605f5
prerequisite-patch-id: 7ce82c26cb9e18884492d2282a72ff2a760aefda
prerequisite-patch-id: ec2071425cab81da72e0805ad92fc52731d7a24d
prerequisite-patch-id: 32cde02288e0c36ed687f67778937a61f78b2d90
prerequisite-patch-id: 5f302e2bead8994763699a909ad0b5501f09ed9f
prerequisite-patch-id: 30611df6ef38c7872107d6bf6dde4504d46ab224
prerequisite-patch-id: 99831bcaa13e25b957d83a6320f34bcec223b939
prerequisite-patch-id: b0ad38bc6b323ceea7a1d2266b0fab8deaa6b05e
prerequisite-patch-id: 38dbce2b9302a764be9dbdc551578f02d797dfcc
prerequisite-patch-id: 133f8b1dab4f47d429b1924df981564ce3736233
prerequisite-patch-id: 879c546693a53e4b72c1ee25954c894ae57a441f
prerequisite-patch-id: 3e7edc818ac078a138f0e42e3f47fd685fffb84f
prerequisite-patch-id: 673b9f0b1936b5a49973b71cab5d13774583de38
prerequisite-message-id: 20250410134646.96811-1-aradhya.bhatia@linux.dev
prerequisite-patch-id: 04f9a2440cebc87891b51d3f77996c88f7525d1c

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v3 01/17] drm/tidss: Fix missing includes and struct decls

[Tomi Valkeinen]

Fix missing includes and struct declarations. Even if these don't cause
any compile issues at the moment, it's good to have them correct.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/tidss/tidss_dispc.h       | 3 +++
 drivers/gpu/drm/tidss/tidss_drv.h         | 2 ++
 drivers/gpu/drm/tidss/tidss_plane.h       | 2 ++
 drivers/gpu/drm/tidss/tidss_scale_coefs.h | 2 ++
 4 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..c31b477a18b0 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -7,11 +7,14 @@
 #ifndef __TIDSS_DISPC_H__
 #define __TIDSS_DISPC_H__
 
+#include <drm/drm_color_mgmt.h>
+
 #include "tidss_drv.h"
 
 struct dispc_device;
 
 struct drm_crtc_state;
+struct drm_plane_state;
 
 enum tidss_gamma_type { TIDSS_GAMMA_8BIT, TIDSS_GAMMA_10BIT };
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 7f4f4282bc04..56a2020e20d0 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -9,6 +9,8 @@
 
 #include <linux/spinlock.h>
 
+#include <drm/drm_device.h>
+
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
 
diff --git a/drivers/gpu/drm/tidss/tidss_plane.h b/drivers/gpu/drm/tidss/tidss_plane.h
index aecaf2728406..92c560c3a621 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.h
+++ b/drivers/gpu/drm/tidss/tidss_plane.h
@@ -7,6 +7,8 @@
 #ifndef __TIDSS_PLANE_H__
 #define __TIDSS_PLANE_H__
 
+#include <drm/drm_plane.h>
+
 #define to_tidss_plane(p) container_of((p), struct tidss_plane, plane)
 
 struct tidss_device;
diff --git a/drivers/gpu/drm/tidss/tidss_scale_coefs.h b/drivers/gpu/drm/tidss/tidss_scale_coefs.h
index 9c560d0fdac0..9824d02d9d1f 100644
--- a/drivers/gpu/drm/tidss/tidss_scale_coefs.h
+++ b/drivers/gpu/drm/tidss/tidss_scale_coefs.h
@@ -9,6 +9,8 @@
 
 #include <linux/types.h>
 
+struct device;
+
 struct tidss_scale_coefs {
 	s16 c2[16];
 	s16 c1[16];

-- 
2.43.0

[PATCH v3 02/17] drm/tidss: Use the crtc_* timings when programming the HW

[Tomi Valkeinen]

Use the crtc_* fields from drm_display_mode, instead of the "logical"
fields. This shouldn't change anything in practice, but afaiu the crtc_*
fields are the correct ones to use here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
 drivers/gpu/drm/tidss/tidss_dispc.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 94f8e3178df5..1604eca265ef 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -225,7 +225,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 	tidss_runtime_get(tidss);
 
 	r = dispc_vp_set_clk_rate(tidss->dispc, tcrtc->hw_videoport,
-				  mode->clock * 1000);
+				  mode->crtc_clock * 1000);
 	if (r != 0)
 		return;
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index cacb5f3d8085..a5107f2732b1 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1084,13 +1084,13 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 
 	dispc_set_num_datalines(dispc, hw_videoport, fmt->data_width);
 
-	hfp = mode->hsync_start - mode->hdisplay;
-	hsw = mode->hsync_end - mode->hsync_start;
-	hbp = mode->htotal - mode->hsync_end;
+	hfp = mode->crtc_hsync_start - mode->crtc_hdisplay;
+	hsw = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	hbp = mode->crtc_htotal - mode->crtc_hsync_end;
 
-	vfp = mode->vsync_start - mode->vdisplay;
-	vsw = mode->vsync_end - mode->vsync_start;
-	vbp = mode->vtotal - mode->vsync_end;
+	vfp = mode->crtc_vsync_start - mode->crtc_vdisplay;
+	vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
+	vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
 
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_TIMING_H,
 		       FLD_VAL(hsw - 1, 7, 0) |
@@ -1132,8 +1132,8 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 		       FLD_VAL(ivs, 12, 12));
 
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN,
-		       FLD_VAL(mode->hdisplay - 1, 11, 0) |
-		       FLD_VAL(mode->vdisplay - 1, 27, 16));
+		       FLD_VAL(mode->crtc_hdisplay - 1, 11, 0) |
+		       FLD_VAL(mode->crtc_vdisplay - 1, 27, 16));
 
 	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
 }

-- 
2.43.0

[PATCH v3 03/17] drm/tidss: Adjust the pclk based on the HW capabilities

[Tomi Valkeinen]

At the moment the driver just sets the clock rate with clk_set_rate(),
and if the resulting rate is not the same as requested, prints a debug
print, but nothing else.

Add functionality to atomic_check(), in which the clk_round_rate() is
used to get the "rounded" rate, and set that to the adjusted_mode.

In practice, with the current K3 SoCs, the display PLL is capable of
producing very exact clocks, so most likely the rounded rate is the same
as the original one.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
 drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 1604eca265ef..6c3967f70510 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -91,7 +91,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
 	struct dispc_device *dispc = tidss->dispc;
 	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
 	u32 hw_videoport = tcrtc->hw_videoport;
-	const struct drm_display_mode *mode;
+	struct drm_display_mode *adjusted_mode;
 	enum drm_mode_status ok;
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
@@ -99,12 +99,27 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
 	if (!crtc_state->enable)
 		return 0;
 
-	mode = &crtc_state->adjusted_mode;
+	adjusted_mode = &crtc_state->adjusted_mode;
 
-	ok = dispc_vp_mode_valid(dispc, hw_videoport, mode);
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		long rate;
+
+		rate = dispc_vp_round_clk_rate(tidss->dispc,
+					       tcrtc->hw_videoport,
+					       adjusted_mode->clock * 1000);
+		if (rate < 0)
+			return -EINVAL;
+
+		adjusted_mode->clock = rate / 1000;
+
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
+	}
+
+	ok = dispc_vp_mode_valid(dispc, hw_videoport, adjusted_mode);
 	if (ok != MODE_OK) {
 		dev_dbg(ddev->dev, "%s: bad mode: %ux%u pclk %u kHz\n",
-			__func__, mode->hdisplay, mode->vdisplay, mode->clock);
+			__func__, adjusted_mode->hdisplay,
+			adjusted_mode->vdisplay, adjusted_mode->clock);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index a5107f2732b1..3930fb7f03c2 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1318,6 +1318,12 @@ unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
 	return (unsigned int)(abs(((rr - r) * 100) / r));
 }
 
+long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
+			     unsigned long rate)
+{
+	return clk_round_rate(dispc->vp_clk[hw_videoport], rate);
+}
+
 int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
 			  unsigned long rate)
 {
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index c31b477a18b0..d4c335e918fb 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -120,6 +120,8 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
 					 const struct drm_display_mode *mode);
 int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport);
 void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport);
+long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
+			     unsigned long rate);
 int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
 			  unsigned long rate);
 void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,

-- 
2.43.0

[PATCH v3 04/17] phy: cdns-dphy: Store hs_clk_rate and return it

[Tomi Valkeinen]

The DPHY driver does not return the actual hs_clk_rate, so the DSI
driver has no idea what clock was actually achieved. Set the realized
hs_clk_rate to the opts struct, so that the DSI driver gets it back.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/phy/cadence/cdns-dphy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index ed87a3970f83..f79ec4fab409 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
 	u8 pll_ipdiv;
 	u8 pll_opdiv;
 	u16 pll_fbdiv;
+	u32 hs_clk_rate;
 	unsigned int nlanes;
 };
 
@@ -154,6 +155,9 @@ static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
 					  cfg->pll_ipdiv,
 					  pll_ref_hz);
 
+	cfg->hs_clk_rate = div_u64((u64)pll_ref_hz * cfg->pll_fbdiv,
+				   2 * cfg->pll_opdiv * cfg->pll_ipdiv);
+
 	return 0;
 }
 
@@ -297,6 +301,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 	if (ret)
 		return ret;
 
+	opts->hs_clk_rate = cfg->hs_clk_rate;
 	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
 
 	return 0;

-- 
2.43.0

[PATCH v3 05/17] phy: cdns-dphy: Remove leftover code

[Tomi Valkeinen]

The code in cdns-dphy has probably been part of a DSI driver in the
past. Remove DSI defines and variables which are not used or do not
actually do anything. Also rename cdns_dsi_get_dphy_pll_cfg() to
cdns_dphy_get_pll_cfg(), i.e. drop the "dsi", as it's not relevant here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/phy/cadence/cdns-dphy.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index f79ec4fab409..33a42e72362e 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -55,14 +55,6 @@
 #define DPHY_PSM_CFG_FROM_REG		BIT(0)
 #define DPHY_PSM_CLK_DIV(x)		((x) << 1)
 
-#define DSI_HBP_FRAME_OVERHEAD		12
-#define DSI_HSA_FRAME_OVERHEAD		14
-#define DSI_HFP_FRAME_OVERHEAD		6
-#define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
-#define DSI_BLANKING_FRAME_OVERHEAD	6
-#define DSI_NULL_FRAME_OVERHEAD		6
-#define DSI_EOT_PKT_SIZE		4
-
 #define DPHY_TX_J721E_WIZ_PLL_CTRL	0xF04
 #define DPHY_TX_J721E_WIZ_STATUS	0xF08
 #define DPHY_TX_J721E_WIZ_RST_CTRL	0xF0C
@@ -117,10 +109,9 @@ static const unsigned int tx_bands[] = {
 	870, 950, 1000, 1200, 1400, 1600, 1800, 2000, 2200, 2500
 };
 
-static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
-				     struct cdns_dphy_cfg *cfg,
-				     struct phy_configure_opts_mipi_dphy *opts,
-				     unsigned int *dsi_hfp_ext)
+static int cdns_dphy_get_pll_cfg(struct cdns_dphy *dphy,
+				 struct cdns_dphy_cfg *cfg,
+				 struct phy_configure_opts_mipi_dphy *opts)
 {
 	unsigned long pll_ref_hz = clk_get_rate(dphy->pll_ref_clk);
 	u64 dlane_bps;
@@ -289,15 +280,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 				      struct cdns_dphy_cfg *cfg)
 {
 	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-	unsigned int dsi_hfp_ext = 0;
 	int ret;
 
 	ret = phy_mipi_dphy_config_validate(opts);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_get_dphy_pll_cfg(dphy, cfg,
-					opts, &dsi_hfp_ext);
+	ret = cdns_dphy_get_pll_cfg(dphy, cfg, opts);
 	if (ret)
 		return ret;
 

-- 
2.43.0

[PATCH v3 06/17] drm/bridge: cdns-dsi: Remove extra line at the end of the file

[Tomi Valkeinen]

Remove extra line at the end of the file.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 1d300f0da2f5..5315d572f7ab 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1435,4 +1435,3 @@ MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
 MODULE_DESCRIPTION("Cadence DSI driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:cdns-dsi");
-

-- 
2.43.0

[PATCH v3 07/17] drm/bridge: cdns-dsi: Drop crtc_* code

[Tomi Valkeinen]

With recent change the cdns_dsi_check_conf() is always called with
mode_valid_check = true. We can thus remove all the code related to the
"false" paths.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 60 ++++++++------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 5315d572f7ab..d521ddb8bf75 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -452,15 +452,6 @@ bridge_to_cdns_dsi_input(struct drm_bridge *bridge)
 	return container_of(bridge, struct cdns_dsi_input, bridge);
 }
 
-static unsigned int mode_to_dpi_hfp(const struct drm_display_mode *mode,
-				    bool mode_valid_check)
-{
-	if (mode_valid_check)
-		return mode->hsync_start - mode->hdisplay;
-
-	return mode->crtc_hsync_start - mode->crtc_hdisplay;
-}
-
 static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 				      unsigned int dpi_bpp,
 				      unsigned int dsi_pkt_overhead)
@@ -477,8 +468,7 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 
 static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 			     const struct drm_display_mode *mode,
-			     struct cdns_dsi_cfg *dsi_cfg,
-			     bool mode_valid_check)
+			     struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	unsigned int tmp;
@@ -492,30 +482,20 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 
 	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
 
-	if (mode_valid_check)
-		tmp = mode->htotal -
-		      (sync_pulse ? mode->hsync_end : mode->hsync_start);
-	else
-		tmp = mode->crtc_htotal -
-		      (sync_pulse ?
-		       mode->crtc_hsync_end : mode->crtc_hsync_start);
+	tmp = mode->htotal -
+		(sync_pulse ? mode->hsync_end : mode->hsync_start);
 
 	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
 
 	if (sync_pulse) {
-		if (mode_valid_check)
-			tmp = mode->hsync_end - mode->hsync_start;
-		else
-			tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
+		tmp = mode->hsync_end - mode->hsync_start;
 
 		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
 						 DSI_HSA_FRAME_OVERHEAD);
 	}
 
-	dsi_cfg->hact = dpi_to_dsi_timing(mode_valid_check ?
-					  mode->hdisplay : mode->crtc_hdisplay,
-					  bpp, 0);
-	dsi_cfg->hfp = dpi_to_dsi_timing(mode_to_dpi_hfp(mode, mode_valid_check),
+	dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
+	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
 					 bpp, DSI_HFP_FRAME_OVERHEAD);
 
 	return 0;
@@ -524,14 +504,12 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 			      struct cdns_dsi_cfg *dsi_cfg,
 			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct drm_display_mode *mode,
-			      bool mode_valid_check)
+			      const struct drm_display_mode *mode)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	unsigned long long dlane_bps;
 	unsigned long adj_dsi_htotal;
 	unsigned long dsi_htotal;
-	unsigned long dpi_htotal;
 	unsigned long dpi_hz;
 	unsigned int dsi_hfp_ext;
 	unsigned int lanes = output->dev->lanes;
@@ -552,12 +530,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	if (dsi_htotal % lanes)
 		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
 
-	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
+	dpi_hz = mode->clock * 1000;
 	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
 
 	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
-	if (do_div(dlane_bps, lanes * dpi_htotal))
+	if (do_div(dlane_bps, lanes * mode->htotal))
 		return -EINVAL;
 
 	/* data rate was in bytes/sec, convert to bits/sec. */
@@ -572,27 +549,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 			       const struct drm_display_mode *mode,
-			       struct cdns_dsi_cfg *dsi_cfg,
-			       bool mode_valid_check)
+			       struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long dsi_hss_hsa_hse_hbp;
 	unsigned int nlanes = output->dev->lanes;
-	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
 	int ret;
 
-	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
+	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
+	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
 	if (ret)
 		return ret;
 
@@ -610,9 +585,8 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	 * interface.
 	 */
 	if ((u64)phy_cfg->hs_clk_rate *
-	    mode_to_dpi_hfp(mode, mode_valid_check) * nlanes <
-	    (u64)dsi_hss_hsa_hse_hbp *
-	    (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000)
+	    (mode->hsync_start - mode->hdisplay) * nlanes <
+	    (u64)dsi_hss_hsa_hse_hbp * mode->clock  * 1000)
 		return -EINVAL;
 
 	return 0;
@@ -662,7 +636,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	if ((mode->hdisplay * bpp) % 32)
 		return MODE_H_ILLEGAL;
 
-	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
+	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg);
 	if (ret)
 		return MODE_BAD;
 
@@ -993,7 +967,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
 
-	return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg, true);
+	return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg);
 }
 
 static struct drm_bridge_state *

-- 
2.43.0

[PATCH v3 08/17] drm/bridge: cdns-dsi: Remove broken fifo emptying check

[Tomi Valkeinen]

The driver checks if "DPI(HFP) > DSI(HSS+HSA+HSE+HBP)", and rejects the
mode if not.

However, testing shows that this doesn't hold at all. I can set the hfp
to very small values, with no errors. The feedback from the HW team also
was that the check is not right, although it's not clear if there's a
way to validate the FIFO emptying.

The check rejects quite a lot of modes, apparently for no good reason,
so drop the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index d521ddb8bf75..bb8ac7161abf 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -553,7 +553,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
-	unsigned long dsi_hss_hsa_hse_hbp;
 	unsigned int nlanes = output->dev->lanes;
 	int ret;
 
@@ -575,20 +574,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
-		dsi_hss_hsa_hse_hbp += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
-
-	/*
-	 * Make sure DPI(HFP) > DSI(HSS+HSA+HSE+HBP) to guarantee that the FIFO
-	 * is empty before we start a receiving a new line on the DPI
-	 * interface.
-	 */
-	if ((u64)phy_cfg->hs_clk_rate *
-	    (mode->hsync_start - mode->hdisplay) * nlanes <
-	    (u64)dsi_hss_hsa_hse_hbp * mode->clock  * 1000)
-		return -EINVAL;
-
 	return 0;
 }
 

-- 
2.43.0

[PATCH v3 09/17] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Tomi Valkeinen]

The docs say about mode_valid():

"it is not allowed to look at anything else but the passed-in mode, and
validate it against configuration-invariant hardware constraints"

We're doing a lot more than just looking at the mode. The main issue
here is that we're doing checks based on the pixel clock, before we know
what the pixel clock from the crtc actually is.

So, drop the cdns_dsi_check_conf() call from .mode_valid().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index bb8ac7161abf..4759fd6f79b0 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -602,8 +602,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_output *output = &dsi->output;
-	struct cdns_dsi_cfg dsi_cfg;
-	int bpp, ret;
+	int bpp;
 
 	/*
 	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
@@ -621,10 +620,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	if ((mode->hdisplay * bpp) % 32)
 		return MODE_H_ILLEGAL;
 
-	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg);
-	if (ret)
-		return MODE_BAD;
-
 	return MODE_OK;
 }
 

-- 
2.43.0

[PATCH v3 10/17] drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()

[Tomi Valkeinen]

cdns_dsi_mode2cfg() calculates the dsi timings, but for some reason
doesn't set the htotal based on those timings. It is set only later, in
cdns_dsi_adjust_phy_config().

As cdns_dsi_mode2cfg() is the logical place to calculate it, let's move
it there. Especially as the following patch will remove
cdns_dsi_adjust_phy_config().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 4759fd6f79b0..fc034a9624a5 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -498,6 +498,13 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
 					 bpp, DSI_HFP_FRAME_OVERHEAD);
 
+	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+		dsi_cfg->htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+
+	dsi_cfg->htotal += dsi_cfg->hact;
+	dsi_cfg->htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
+
 	return 0;
 }
 
@@ -514,12 +521,7 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	unsigned int dsi_hfp_ext;
 	unsigned int lanes = output->dev->lanes;
 
-	dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
-		dsi_htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
-
-	dsi_htotal += dsi_cfg->hact;
-	dsi_htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
+	dsi_htotal = dsi_cfg->htotal;
 
 	/*
 	 * Make sure DSI htotal is aligned on a lane boundary when calculating

-- 
2.43.0

[PATCH v3 11/17] drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()

[Tomi Valkeinen]

cdns_dsi_adjust_phy_config() is called from cdns_dsi_check_conf(), which
is called from .atomic_check(). It checks the DSI htotal and adjusts it
to align on the DSI lane boundary by changing hfp and then recalculating
htotal and HS clock rate.

This has a few problems.

First is the fact that the whole thing is not needed: we do not need to
align on the lane boundary. The whole frame is sent in HS mode, and it
is fine if the line's last byte clock tick fills, say, only 2 of the 4
lanes. The next line will just continue from there. Assuming the
DSI timing values have been calculated to match the incoming DPI stream,
and the HS clock is compatible with the DPI pixel clock, the "uneven"
DSI lines will even out when multiple lines are being sent.

But we could do the align, aligning is not a problem as such. However,
adding more bytes to the hfp, as the function currently does, makes the
DSI line time longer, so the function then adjusts the HS clock rate.
This is where things fail: we don't know what rates we can get from the
HS clock, and at least in TI K3 SoC case the rates are quite coarsely
grained. Thus small adjustment to hfp will lead to a big change in HS
clock rate, and things break down.

We could do a loop here, adjusting hfp, adjusting clock, checking clock
rate, adjusting hfp again, etc., but considering that the whole
adjustment shouldn't be needed at all, it's easier to just remove the
function.

Something like this function should be added back later, when adding
burst mode support, but that's a bigger change and I don't think this
function would help that work in any way.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 --------------------------
 1 file changed, 45 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index fc034a9624a5..319a6a9a6fe7 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -508,47 +508,6 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 	return 0;
 }
 
-static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
-			      struct cdns_dsi_cfg *dsi_cfg,
-			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct drm_display_mode *mode)
-{
-	struct cdns_dsi_output *output = &dsi->output;
-	unsigned long long dlane_bps;
-	unsigned long adj_dsi_htotal;
-	unsigned long dsi_htotal;
-	unsigned long dpi_hz;
-	unsigned int dsi_hfp_ext;
-	unsigned int lanes = output->dev->lanes;
-
-	dsi_htotal = dsi_cfg->htotal;
-
-	/*
-	 * Make sure DSI htotal is aligned on a lane boundary when calculating
-	 * the expected data rate. This is done by extending HFP in case of
-	 * misalignment.
-	 */
-	adj_dsi_htotal = dsi_htotal;
-	if (dsi_htotal % lanes)
-		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
-
-	dpi_hz = mode->clock * 1000;
-	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
-
-	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	if (do_div(dlane_bps, lanes * mode->htotal))
-		return -EINVAL;
-
-	/* data rate was in bytes/sec, convert to bits/sec. */
-	phy_cfg->hs_clk_rate = dlane_bps * 8;
-
-	dsi_hfp_ext = adj_dsi_htotal - dsi_htotal;
-	dsi_cfg->hfp += dsi_hfp_ext;
-	dsi_cfg->htotal = dsi_htotal + dsi_hfp_ext;
-
-	return 0;
-}
-
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 			       const struct drm_display_mode *mode,
 			       struct cdns_dsi_cfg *dsi_cfg)
@@ -568,10 +527,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
-	if (ret)
-		return ret;
-
 	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &output->phy_opts);
 	if (ret)
 		return ret;

-- 
2.43.0

[PATCH v3 12/17] drm/bridge: cdns-dsi: Adjust mode to negative syncs

[Tomi Valkeinen]

The Cadence DSI requires negative syncs from the incoming video signal,
but at the moment that requirement is not expressed in any way. If the
crtc decides to use positive syncs, things break down.

Use the adjusted_mode in atomic_check to set the sync flags to negative
ones.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 319a6a9a6fe7..182845c54c3d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -901,9 +901,13 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
-	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
 
+	/* cdns-dsi requires negative syncs */
+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
+
 	return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg);
 }
 

-- 
2.43.0

[PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

[Tomi Valkeinen]

The driver tries to calculate the value for REG_WAKEUP_TIME. However,
the calculation itself is not correct, and to add on it, the resulting
value is almost always larger than the field's size, so the actual
result is more or less random.

According to the docs, figuring out the value for REG_WAKEUP_TIME
requires HW characterization and there's no way to have a generic
algorithm to come up with the value. That doesn't help at all...

However, we know that the value must be smaller than the line time, and,
at least in my understanding, the proper value for it is quite small.
Testing shows that setting it to 1/10 of the line time seems to work
well. All video modes from my HDMI monitor work with this algorithm.

Hopefully we'll get more information on how to calculate the value, and
we can then update this.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 182845c54c3d..fb0623d3f854 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -786,7 +786,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 
 	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
 					    phy_cfg->hs_clk_rate);
-	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;
+
+	/*
+	 * Estimated time [in clock cycles] to perform LP->HS on D-PHY.
+	 * It is not clear how to calculate this, so for now,
+	 * set it to 1/10 of the total number of clocks in a line.
+	 */
+	reg_wakeup = dsi_cfg.htotal / nlanes / 10;
 	writel(REG_WAKEUP_TIME(reg_wakeup) | REG_LINE_DURATION(tmp),
 	       dsi->regs + VID_DPHY_TIME);
 

-- 
2.43.0

[PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi_mode2cfg()

[Tomi Valkeinen]

The driver does all the calculations and programming with video timings
(hftp, hbp, etc.) instead of the modeline values (hsync_start, ...).
Thus it makes sense to use struct videomode instead of struct
drm_display_mode internally.

Switch to videomode and do some cleanups in cdns_dsi_mode2cfg() along
the way.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 ++++++++++++++------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index fb0623d3f854..a55f851711f0 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -9,6 +9,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_probe_helper.h>
 #include <video/mipi_display.h>
+#include <video/videomode.h>
 
 #include <linux/clk.h>
 #include <linux/interrupt.h>
@@ -467,36 +468,35 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 }
 
 static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
-			     const struct drm_display_mode *mode,
+			     const struct videomode *vm,
 			     struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
-	unsigned int tmp;
-	bool sync_pulse = false;
+	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
+	bool sync_pulse;
 	int bpp;
 
+	dpi_hsa = vm->hsync_len;
+	dpi_hbp = vm->hback_porch;
+	dpi_hfp = vm->hfront_porch;
+	dpi_hact = vm->hactive;
+
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
-		sync_pulse = true;
+	sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
 
-	tmp = mode->htotal -
-		(sync_pulse ? mode->hsync_end : mode->hsync_start);
+	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
+					 bpp, DSI_HBP_FRAME_OVERHEAD);
 
-	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
+	if (sync_pulse)
+		dsi_cfg->hsa =
+			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
 
-	if (sync_pulse) {
-		tmp = mode->hsync_end - mode->hsync_start;
+	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
 
-		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
-						 DSI_HSA_FRAME_OVERHEAD);
-	}
-
-	dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
-	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
-					 bpp, DSI_HFP_FRAME_OVERHEAD);
+	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
 
 	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
 	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
@@ -509,7 +509,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 }
 
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
-			       const struct drm_display_mode *mode,
+			       const struct videomode *vm,
 			       struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -517,11 +517,11 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	unsigned int nlanes = output->dev->lanes;
 	int ret;
 
-	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
+	ret = cdns_dsi_mode2cfg(dsi, vm, dsi_cfg);
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(vm->pixelclock,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
@@ -909,12 +909,15 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
+	struct videomode vm;
 
 	/* cdns-dsi requires negative syncs */
 	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
-	return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg);
+	drm_display_mode_to_videomode(adjusted_mode, &vm);
+
+	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
 }
 
 static struct drm_bridge_state *

-- 
2.43.0

[PATCH v3 15/17] drm/bridge: cdns-dsi: Fix event mode

[Tomi Valkeinen]

The timings calculation gets it wrong for DSI event mode, resulting in
too large hbp value. Fix the issue by taking into account the
pulse/event mode difference.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 31 +++++++++++++++++---------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index a55f851711f0..63031379459e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -418,7 +418,8 @@
 #define DSI_OUTPUT_PORT			0
 #define DSI_INPUT_PORT(inputid)		(1 + (inputid))
 
-#define DSI_HBP_FRAME_OVERHEAD		12
+#define DSI_HBP_FRAME_PULSE_OVERHEAD	12
+#define DSI_HBP_FRAME_EVENT_OVERHEAD	16
 #define DSI_HSA_FRAME_OVERHEAD		14
 #define DSI_HFP_FRAME_OVERHEAD		6
 #define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
@@ -487,23 +488,31 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 
 	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
 
-	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
-					 bpp, DSI_HBP_FRAME_OVERHEAD);
+	if (sync_pulse) {
+		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp, bpp,
+						 DSI_HBP_FRAME_PULSE_OVERHEAD);
 
-	if (sync_pulse)
-		dsi_cfg->hsa =
-			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
+		dsi_cfg->hsa = dpi_to_dsi_timing(dpi_hsa, bpp,
+						 DSI_HSA_FRAME_OVERHEAD);
+	} else {
+		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + dpi_hsa, bpp,
+						 DSI_HBP_FRAME_EVENT_OVERHEAD);
+
+		dsi_cfg->hsa = 0;
+	}
 
 	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
 
 	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
 
-	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
-		dsi_cfg->htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+	dsi_cfg->htotal = dsi_cfg->hact + dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
 
-	dsi_cfg->htotal += dsi_cfg->hact;
-	dsi_cfg->htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
+	if (sync_pulse) {
+		dsi_cfg->htotal += dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
+		dsi_cfg->htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+	} else {
+		dsi_cfg->htotal += dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
+	}
 
 	return 0;
 }

-- 
2.43.0

[PATCH v3 16/17] drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs

[Tomi Valkeinen]

The driver currently expects the pixel clock and the HS clock to be
compatible, but the DPHY PLL doesn't give very finely grained rates.
This often leads to the situation where the pipeline just fails, as the
resulting HS clock is just too off.

We could change the driver to do a better job on adjusting the DSI
blanking values, hopefully getting a working pipeline even if the pclk
and HS clocks are not exactly compatible. But that is a bigger work.

What we can do easily is to see in .atomic_check() what HS clock rate we
can get, based on the pixel clock rate, and then convert the HS clock
rate back to pixel clock rate and ask that rate from the crtc. If the
crtc has a good PLL (which is the case for TI K3 SoCs), this will fix
any issues wrt. the clock rates.

If the crtc cannot provide the requested clock, well, we're no worse off
with this patch than what we have at the moment.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 37 ++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 63031379459e..165df5d595b8 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -908,6 +908,28 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static long cdns_dsi_round_pclk(struct cdns_dsi *dsi, unsigned long pclk)
+{
+	struct cdns_dsi_output *output = &dsi->output;
+	unsigned int nlanes = output->dev->lanes;
+	union phy_configure_opts phy_opts = { 0 };
+	u32 bitspp;
+	int ret;
+
+	bitspp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
+
+	ret = phy_mipi_dphy_get_default_config(pclk, bitspp, nlanes,
+					       &phy_opts.mipi_dphy);
+	if (ret)
+		return ret;
+
+	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &phy_opts);
+	if (ret)
+		return ret;
+
+	return div_u64((u64)phy_opts.mipi_dphy.hs_clk_rate * nlanes, bitspp);
+}
+
 static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 					struct drm_bridge_state *bridge_state,
 					struct drm_crtc_state *crtc_state,
@@ -919,11 +941,26 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
 	struct videomode vm;
+	long pclk;
 
 	/* cdns-dsi requires negative syncs */
 	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
+	/*
+	 * The DPHY PLL has quite a coarsely grained clock rate options. See
+	 * what hsclk rate we can achieve based on the pixel clock, convert it
+	 * back to pixel clock, set that to the adjusted_mode->clock. This is
+	 * all in hopes that the CRTC will be able to provide us the requested
+	 * clock, as otherwise the DPI and DSI clocks will be out of sync.
+	 */
+
+	pclk = cdns_dsi_round_pclk(dsi, adjusted_mode->clock * 1000);
+	if (pclk < 0)
+		return (int)pclk;
+
+	adjusted_mode->clock = pclk / 1000;
+
 	drm_display_mode_to_videomode(adjusted_mode, &vm);
 
 	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);

-- 
2.43.0

[PATCH v3 17/17] drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST

[Tomi Valkeinen]

While the cdns-dsi does not support DSI burst mode, the burst mode is
essentially DSI event mode with more versatile clocking and timings.
Thus cdns-dsi doesn't need to fail if the DSI peripheral driver requests
MIPI_DSI_MODE_VIDEO_BURST.

In my particular use case, this allows the use of ti-sn65dsi83 driver.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 165df5d595b8..0e9545139af9 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1045,10 +1045,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 	if (output->dev)
 		return -EBUSY;
 
-	/* We do not support burst mode yet. */
-	if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
-		return -ENOTSUPP;
-
 	/*
 	 * The host <-> device link might be described using an OF-graph
 	 * representation, in this case we extract the device of_node from

-- 
2.43.0

Re: [PATCH v3 00/17] drm/bridge: cdns-dsi: Make it work a bit better

[Parth Panchoil]

On Mon, 2025-04-14 at 14:11 +0300, Tomi Valkeinen wrote:
> While trying to get the cdns-dsi to work on Toradex's AM69 Aquila
> platform, I hit multiple issues in the driver. Basicaly nothing
> worked
> for with the board.
> 
> This series fixes those issues. While I itch to make much larger
> changes
> to the cdns-dsi driver, I opted to keep this series relatively simple
> to
> make the fixes more clear and possibly help with backporting.
> 
> The series also touches tidss, but those changes are not strictly
> needed, and can be merged separately. And the series also touches
> cdns-dphy, and those changes are needed.
> 
> This has been tested on Toradex AM69 Aquila (upstream) and AM62P
> Verdin
> (Toradex's BSP), with:
> - HDMI output using lontium lt8912b
> - LVDS panel (sn65dsi84 + panel-lvds)
> 
>  Tomi
> 
> 
(For the entire series)

Tested-by: Parth Pancholi <parth.pancholi@toradex.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Changes in v3:
> - Add Aradhya's "drm/bridge: cdns-dsi: Fix the _atomic_check()" to
> the
>   dependencies
> - The above patch from Aradhya allowed adding "drm/bridge: cdns-dsi:
>   Drop crtc_* code", which resulted in quite large changes in the
>   commits, even if the end result doesn't really differ.
> - Reordered commits to decrease back-and-forth (e.g. fixing something
> in
>   a a code that will be removed in the next commits)
> - The reordering caused quite big changes in the commits (even if the
>   final end result is more or less the same), so I chose not to add
>   tested-by tags.
> - Rename 'cdns_get_dphy_pll_cfg' to 'cdns_dphy_get_pll_cfg'
> - Use div_u64() instead of div64_u64()
> - Drop "Fail if HS rate changed when validating PHY config". This was
>   too strict, as clock rounding (especially with DRM's 1kHz
>   resolution...) leads to clock rates that do not match exactly.
>   However, the rate mismatch should be fine as the commits adjust the
>   pixel clock, and the resulting differences should be so small that
> we
>   can't even improve the timings match by adjusting the DSI HFP, as
> the
>   adjustment rounds to 0.
> - Link to v2:
> https://lore.kernel.org/r/20250402-cdns-dsi-impro-v2-0-4a093eaa5e27@ideasonboard.com
> 
> Changes in v2:
> - Change the tidss clock adjustment from mode_fixup() to
> atomic_check()
> - Link to v1:
> https://lore.kernel.org/r/20250320-cdns-dsi-impro-v1-0-725277c5f43b@ideasonboard.com
> 
> ---
> Tomi Valkeinen (17):
>       drm/tidss: Fix missing includes and struct decls
>       drm/tidss: Use the crtc_* timings when programming the HW
>       drm/tidss: Adjust the pclk based on the HW capabilities
>       phy: cdns-dphy: Store hs_clk_rate and return it
>       phy: cdns-dphy: Remove leftover code
>       drm/bridge: cdns-dsi: Remove extra line at the end of the file
>       drm/bridge: cdns-dsi: Drop crtc_* code
>       drm/bridge: cdns-dsi: Remove broken fifo emptying check
>       drm/bridge: cdns-dsi: Drop checks that shouldn't be in
> .mode_valid()
>       drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()
>       drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()
>       drm/bridge: cdns-dsi: Adjust mode to negative syncs
>       drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value
>       drm/bridge: cdns-dsi: Use video mode and clean up
> cdns_dsi_mode2cfg()
>       drm/bridge: cdns-dsi: Fix event mode
>       drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to
> dsi needs
>       drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST
> 
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 211 +++++++++++----
> ----------
>  drivers/gpu/drm/tidss/tidss_crtc.c             |  25 ++-
>  drivers/gpu/drm/tidss/tidss_dispc.c            |  22 ++-
>  drivers/gpu/drm/tidss/tidss_dispc.h            |   5 +
>  drivers/gpu/drm/tidss/tidss_drv.h              |   2 +
>  drivers/gpu/drm/tidss/tidss_plane.h            |   2 +
>  drivers/gpu/drm/tidss/tidss_scale_coefs.h      |   2 +
>  drivers/phy/cadence/cdns-dphy.c                |  24 ++-
>  8 files changed, 142 insertions(+), 151 deletions(-)
> ---
> base-commit: 10646ddac2917b31c985ceff0e4982c42a9c924b
> change-id: 20250320-cdns-dsi-impro-3d8fbd7848d1
> prerequisite-message-id:
> 20250226155228.564289-1-aradhya.bhatia@linux.dev
> prerequisite-patch-id: 46845a8d15042dd343a29a17fc0b9d0eec2605f5
> prerequisite-patch-id: 7ce82c26cb9e18884492d2282a72ff2a760aefda
> prerequisite-patch-id: ec2071425cab81da72e0805ad92fc52731d7a24d
> prerequisite-patch-id: 32cde02288e0c36ed687f67778937a61f78b2d90
> prerequisite-patch-id: 5f302e2bead8994763699a909ad0b5501f09ed9f
> prerequisite-patch-id: 30611df6ef38c7872107d6bf6dde4504d46ab224
> prerequisite-patch-id: 99831bcaa13e25b957d83a6320f34bcec223b939
> prerequisite-patch-id: b0ad38bc6b323ceea7a1d2266b0fab8deaa6b05e
> prerequisite-patch-id: 38dbce2b9302a764be9dbdc551578f02d797dfcc
> prerequisite-patch-id: 133f8b1dab4f47d429b1924df981564ce3736233
> prerequisite-patch-id: 879c546693a53e4b72c1ee25954c894ae57a441f
> prerequisite-patch-id: 3e7edc818ac078a138f0e42e3f47fd685fffb84f
> prerequisite-patch-id: 673b9f0b1936b5a49973b71cab5d13774583de38
> prerequisite-message-id:
> 20250410134646.96811-1-aradhya.bhatia@linux.dev
> prerequisite-patch-id: 04f9a2440cebc87891b51d3f77996c88f7525d1c
> 
> Best regards,

Re: [PATCH v3 02/17] drm/tidss: Use the crtc_* timings when programming the HW

[Aradhya Bhatia]

Hi Tomi,

Thank you for the patches!

On 14/04/25 16:41, Tomi Valkeinen wrote:
> Use the crtc_* fields from drm_display_mode, instead of the "logical"
> fields. This shouldn't change anything in practice, but afaiu the crtc_*
> fields are the correct ones to use here.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
>  drivers/gpu/drm/tidss/tidss_dispc.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya

Re: [PATCH v3 04/17] phy: cdns-dphy: Store hs_clk_rate and return it

[Aradhya Bhatia]

Hi,

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The DPHY driver does not return the actual hs_clk_rate, so the DSI
> driver has no idea what clock was actually achieved. Set the realized
> hs_clk_rate to the opts struct, so that the DSI driver gets it back.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/phy/cadence/cdns-dphy.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya

Re: [PATCH v3 06/17] drm/bridge: cdns-dsi: Remove extra line at the end of the file

[Aradhya Bhatia]

Hi,

On 14/04/25 16:41, Tomi Valkeinen wrote:
> Remove extra line at the end of the file.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya

Re: [PATCH v3 09/17] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Aradhya Bhatia]

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The docs say about mode_valid():
> 
> "it is not allowed to look at anything else but the passed-in mode, and
> validate it against configuration-invariant hardware constraints"
> 
> We're doing a lot more than just looking at the mode. The main issue
> here is that we're doing checks based on the pixel clock, before we know
> what the pixel clock from the crtc actually is.
> 
> So, drop the cdns_dsi_check_conf() call from .mode_valid().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

--
Regards
Aradhya

Re: [PATCH v3 10/17] drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()

[Aradhya Bhatia]

On 14/04/25 16:41, Tomi Valkeinen wrote:
> cdns_dsi_mode2cfg() calculates the dsi timings, but for some reason
> doesn't set the htotal based on those timings. It is set only later, in
> cdns_dsi_adjust_phy_config().
> 
> As cdns_dsi_mode2cfg() is the logical place to calculate it, let's move
> it there. Especially as the following patch will remove
> cdns_dsi_adjust_phy_config().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya

Re: [PATCH v3 12/17] drm/bridge: cdns-dsi: Adjust mode to negative syncs

[Aradhya Bhatia]

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The Cadence DSI requires negative syncs from the incoming video signal,
> but at the moment that requirement is not expressed in any way. If the
> crtc decides to use positive syncs, things break down.
> 
> Use the adjusted_mode in atomic_check to set the sync flags to negative
> ones.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya

Re: [PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

[Aradhya Bhatia]

Hi Tomi,

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The driver tries to calculate the value for REG_WAKEUP_TIME. However,
> the calculation itself is not correct, and to add on it, the resulting
> value is almost always larger than the field's size, so the actual
> result is more or less random.>
> According to the docs, figuring out the value for REG_WAKEUP_TIME
> requires HW characterization and there's no way to have a generic
> algorithm to come up with the value. That doesn't help at all...
> 
> However, we know that the value must be smaller than the line time, and,
> at least in my understanding, the proper value for it is quite small.
> Testing shows that setting it to 1/10 of the line time seems to work
> well. All video modes from my HDMI monitor work with this algorithm.
> 
> Hopefully we'll get more information on how to calculate the value, and
> we can then update this.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 182845c54c3d..fb0623d3f854 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -786,7 +786,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>  
>  	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
>  					    phy_cfg->hs_clk_rate);
> -	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;

I think the primary point of failure in the original calculation is due
to fact that the hs_prepare and hs_zero are defined in picoseconds (ps),
and the tx_byte_period is in nanoseconds (ns) as evident by the usage of
NSEC_PER_SEC macro.

The resulting tx_by_period is 1000 times smaller, and the reg_wakeup - a
1000 times larger. =)

Further, the TRM does indeed mention that some characterization is
required to fine tune the exact reg_wakeup value, but it ends up giving
a vague-ish formula -

-> reg_wakeup_time = wakeup_time_dsi + wakeup_time_cl + wakeup_time_dl +
		     (hs_host_eot × 4 / lane_nb)

I think the characterization may only be required for the
wakeup_time_dsi component. The existing formula in the driver (after
corrected for time unit) is the wakeup_time_dl component. wakeup_time_cl
seems to be a range of constants, which the phy-core is auto-settling on
defaults. The document never specifically mentions "hs_host_eot" other
than the equation, but on the off-chance it is same as phy_cfg->eot,
then that's 0 and avoidable.

> +
> +	/*
> +	 * Estimated time [in clock cycles] to perform LP->HS on D-PHY.
> +	 * It is not clear how to calculate this, so for now,
> +	 * set it to 1/10 of the total number of clocks in a line.
> +	 */
> +	reg_wakeup = dsi_cfg.htotal / nlanes / 10;
>  	writel(REG_WAKEUP_TIME(reg_wakeup) | REG_LINE_DURATION(tmp),
>  	       dsi->regs + VID_DPHY_TIME);
>  
> 

-- 
Regards
Aradhya

Re: [PATCH v3 16/17] drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs

[Aradhya Bhatia]

Hi Tomi,

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The driver currently expects the pixel clock and the HS clock to be
> compatible, but the DPHY PLL doesn't give very finely grained rates.
> This often leads to the situation where the pipeline just fails, as the
> resulting HS clock is just too off.
> 
> We could change the driver to do a better job on adjusting the DSI
> blanking values, hopefully getting a working pipeline even if the pclk
> and HS clocks are not exactly compatible. But that is a bigger work.
> 
> What we can do easily is to see in .atomic_check() what HS clock rate we
> can get, based on the pixel clock rate, and then convert the HS clock
> rate back to pixel clock rate and ask that rate from the crtc. If the
> crtc has a good PLL (which is the case for TI K3 SoCs), this will fix
> any issues wrt. the clock rates.
> 
> If the crtc cannot provide the requested clock, well, we're no worse off
> with this patch than what we have at the moment.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 37 ++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 63031379459e..165df5d595b8 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -908,6 +908,28 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>  	return input_fmts;
>  }
>  
> +static long cdns_dsi_round_pclk(struct cdns_dsi *dsi, unsigned long pclk)
> +{
> +	struct cdns_dsi_output *output = &dsi->output;
> +	unsigned int nlanes = output->dev->lanes;
> +	union phy_configure_opts phy_opts = { 0 };
> +	u32 bitspp;
> +	int ret;
> +
> +	bitspp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> +
> +	ret = phy_mipi_dphy_get_default_config(pclk, bitspp, nlanes,
> +					       &phy_opts.mipi_dphy);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &phy_opts);
> +	if (ret)
> +		return ret;
> +
> +	return div_u64((u64)phy_opts.mipi_dphy.hs_clk_rate * nlanes, bitspp);
> +}
> +
>  static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  					struct drm_bridge_state *bridge_state,
>  					struct drm_crtc_state *crtc_state,
> @@ -919,11 +941,26 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>  	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
>  	struct videomode vm;
> +	long pclk;
>  
>  	/* cdns-dsi requires negative syncs */
>  	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>  	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>  
> +	/*
> +	 * The DPHY PLL has quite a coarsely grained clock rate options. See
> +	 * what hsclk rate we can achieve based on the pixel clock, convert it
> +	 * back to pixel clock, set that to the adjusted_mode->clock. This is
> +	 * all in hopes that the CRTC will be able to provide us the requested
> +	 * clock, as otherwise the DPI and DSI clocks will be out of sync.
> +	 */
> +
> +	pclk = cdns_dsi_round_pclk(dsi, adjusted_mode->clock * 1000);
> +	if (pclk < 0)
> +		return (int)pclk;
> +
> +	adjusted_mode->clock = pclk / 1000;
> +
>  	drm_display_mode_to_videomode(adjusted_mode, &vm);
>  
>  	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);

I think at this point cdns_dsi_check_conf is really only creating a
dsi_cfg (from the video-mode) so that it can later be used, and then
running phy_mipi_dphy_get_default_config(), and phy_validate(), both of
which have nothing to do with the freshly made dsi_cfg.

If there is no benefit in running both the latter functions again after
cdns_dsi_round_pclk() runs them, then perhaps we can just drop the
cdns_dsi_check_conf() altogether in favor of cdns_dsi_mode2cfg() being
called from .atomic_check()?


Furthermore, I understand updating the adjusted_mode->clock might help
the CRTC to use a pixel clock that's more in-tune with the _actual_
hs_clk_rate that is going to be generated. But, I am worried that it'll
cause a delta from the intended fps from the CRTC's end because the
timings aren't adjusted in accordance with the pixel-clock.

Perhaps, along with pixel-clock, we can update the dsi_htotal and
dpi_htotal both too, to

new_dsi_htotal = (hs_clk_rate * #lanes) / (dpi_votal * fps * 8)
new_dpi_htotal = (hs_clk_rate * #lanes) / (dpi_vtotal * fps * bitspp).

And then, the respective front-porches can too get updated accordingly.


--
Regards
Aradhya

Re: [PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi_mode2cfg()

[Aradhya Bhatia]

Hi,

On 14/04/25 16:41, Tomi Valkeinen wrote:
> The driver does all the calculations and programming with video timings
> (hftp, hbp, etc.) instead of the modeline values (hsync_start, ...).
> Thus it makes sense to use struct videomode instead of struct
> drm_display_mode internally.
> 
> Switch to videomode and do some cleanups in cdns_dsi_mode2cfg() along
> the way.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 ++++++++++++++------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index fb0623d3f854..a55f851711f0 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -9,6 +9,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_probe_helper.h>
>  #include <video/mipi_display.h>
> +#include <video/videomode.h>
>  
>  #include <linux/clk.h>
>  #include <linux/interrupt.h>
> @@ -467,36 +468,35 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>  }
>  
>  static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
> -			     const struct drm_display_mode *mode,
> +			     const struct videomode *vm,
>  			     struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
> -	unsigned int tmp;
> -	bool sync_pulse = false;
> +	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
> +	bool sync_pulse;
>  	int bpp;
>  
> +	dpi_hsa = vm->hsync_len;
> +	dpi_hbp = vm->hback_porch;
> +	dpi_hfp = vm->hfront_porch;
> +	dpi_hact = vm->hactive;
> +
>  	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>  
> -	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> -		sync_pulse = true;
> +	sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>  
> -	tmp = mode->htotal -
> -		(sync_pulse ? mode->hsync_end : mode->hsync_start);
> +	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
> +					 bpp, DSI_HBP_FRAME_OVERHEAD);
>  
> -	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
> +	if (sync_pulse)
> +		dsi_cfg->hsa =
> +			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
>  
> -	if (sync_pulse) {
> -		tmp = mode->hsync_end - mode->hsync_start;
> +	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
>  
> -		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
> -						 DSI_HSA_FRAME_OVERHEAD);
> -	}
> -
> -	dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
> -	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
> -					 bpp, DSI_HFP_FRAME_OVERHEAD);
> +	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
>  
>  	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
>  	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)

I think at this stage, the dsi_cfg->htotal will always come out to be

((dpi_htotal * bitspp) / 8),

no matter whether the sync_pulse or the event_mode is set or not.

Whatever the overheads are there, they get cancelled out. So, it doesn't
need to be individually tracked.


> @@ -509,7 +509,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>  }
>  
>  static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
> -			       const struct drm_display_mode *mode,
> +			       const struct videomode *vm,
>  			       struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
> @@ -517,11 +517,11 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>  	unsigned int nlanes = output->dev->lanes;
>  	int ret;
>  
> -	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
> +	ret = cdns_dsi_mode2cfg(dsi, vm, dsi_cfg);
>  	if (ret)
>  		return ret;
>  
> -	ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
> +	ret = phy_mipi_dphy_get_default_config(vm->pixelclock,
>  					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
>  					       nlanes, phy_cfg);
>  	if (ret)
> @@ -909,12 +909,15 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
>  	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>  	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
> +	struct videomode vm;
>  
>  	/* cdns-dsi requires negative syncs */
>  	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>  	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>  
> -	return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg);
> +	drm_display_mode_to_videomode(adjusted_mode, &vm);
> +
> +	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
>  }
>  
>  static struct drm_bridge_state *
> 

With the above taken care of,

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>


-- 
Regards
Aradhya

Re: [PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

[Tomi Valkeinen]

On 15/04/2025 23:10, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver tries to calculate the value for REG_WAKEUP_TIME. However,
>> the calculation itself is not correct, and to add on it, the resulting
>> value is almost always larger than the field's size, so the actual
>> result is more or less random.>
>> According to the docs, figuring out the value for REG_WAKEUP_TIME
>> requires HW characterization and there's no way to have a generic
>> algorithm to come up with the value. That doesn't help at all...
>>
>> However, we know that the value must be smaller than the line time, and,
>> at least in my understanding, the proper value for it is quite small.
>> Testing shows that setting it to 1/10 of the line time seems to work
>> well. All video modes from my HDMI monitor work with this algorithm.
>>
>> Hopefully we'll get more information on how to calculate the value, and
>> we can then update this.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 182845c54c3d..fb0623d3f854 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -786,7 +786,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>   
>>   	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
>>   					    phy_cfg->hs_clk_rate);
>> -	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;
> 
> I think the primary point of failure in the original calculation is due
> to fact that the hs_prepare and hs_zero are defined in picoseconds (ps),
> and the tx_byte_period is in nanoseconds (ns) as evident by the usage of
> NSEC_PER_SEC macro.
> 
> The resulting tx_by_period is 1000 times smaller, and the reg_wakeup - a
> 1000 times larger. =)

That is true. It didn't work with that fixed either =).

> Further, the TRM does indeed mention that some characterization is
> required to fine tune the exact reg_wakeup value, but it ends up giving
> a vague-ish formula -
> 
> -> reg_wakeup_time = wakeup_time_dsi + wakeup_time_cl + wakeup_time_dl +
> 		     (hs_host_eot × 4 / lane_nb)
> 
> I think the characterization may only be required for the
> wakeup_time_dsi component. The existing formula in the driver (after
> corrected for time unit) is the wakeup_time_dl component. wakeup_time_cl
> seems to be a range of constants, which the phy-core is auto-settling on
> defaults. The document never specifically mentions "hs_host_eot" other
> than the equation, but on the off-chance it is same as phy_cfg->eot,
> then that's 0 and avoidable.

Yep, I tried to decipher how to calculate it using the TRM and the MIPI 
spec, but I just couldn't figure it out. In any case we need to 
characterization (or a known safe value that will be enough for all 
cases) to use a formula. Unfortunately I haven't gotten any details 
about this.

  Tomi

Re: [PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi_mode2cfg()

[Tomi Valkeinen]

Hi,

On 20/04/2025 21:10, Aradhya Bhatia wrote:
> Hi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver does all the calculations and programming with video timings
>> (hftp, hbp, etc.) instead of the modeline values (hsync_start, ...).
>> Thus it makes sense to use struct videomode instead of struct
>> drm_display_mode internally.
>>
>> Switch to videomode and do some cleanups in cdns_dsi_mode2cfg() along
>> the way.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 ++++++++++++++------------
>>   1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index fb0623d3f854..a55f851711f0 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -9,6 +9,7 @@
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <video/mipi_display.h>
>> +#include <video/videomode.h>
>>   
>>   #include <linux/clk.h>
>>   #include <linux/interrupt.h>
>> @@ -467,36 +468,35 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>>   }
>>   
>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>> -			     const struct drm_display_mode *mode,
>> +			     const struct videomode *vm,
>>   			     struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>> -	unsigned int tmp;
>> -	bool sync_pulse = false;
>> +	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>> +	bool sync_pulse;
>>   	int bpp;
>>   
>> +	dpi_hsa = vm->hsync_len;
>> +	dpi_hbp = vm->hback_porch;
>> +	dpi_hfp = vm->hfront_porch;
>> +	dpi_hact = vm->hactive;
>> +
>>   	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>   
>> -	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
>> -		sync_pulse = true;
>> +	sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>>   
>>   	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>>   
>> -	tmp = mode->htotal -
>> -		(sync_pulse ? mode->hsync_end : mode->hsync_start);
>> +	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
>> +					 bpp, DSI_HBP_FRAME_OVERHEAD);
>>   
>> -	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
>> +	if (sync_pulse)
>> +		dsi_cfg->hsa =
>> +			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
>>   
>> -	if (sync_pulse) {
>> -		tmp = mode->hsync_end - mode->hsync_start;
>> +	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
>>   
>> -		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
>> -						 DSI_HSA_FRAME_OVERHEAD);
>> -	}
>> -
>> -	dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
>> -	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
>> -					 bpp, DSI_HFP_FRAME_OVERHEAD);
>> +	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
>>   
>>   	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
>>   	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> 
> I think at this stage, the dsi_cfg->htotal will always come out to be
> 
> ((dpi_htotal * bitspp) / 8),
> 
> no matter whether the sync_pulse or the event_mode is set or not.
> 
> Whatever the overheads are there, they get cancelled out. So, it doesn't
> need to be individually tracked.

dpi_to_dsi_timing() doesn't return the DPI timing converted _exactly_ to 
DSI. It uses DIV_ROUND_UP() and handles the case where the DPI timing is 
too small for DSI with the overhead.

And I'd rather separate DPI and DSI timings as much as possible, even if 
it is a bit more verbose. Here we want to calculate DSI htotal (i.e. the 
total of DSI horizontal ticks), so I'd rather construct it from the DSI 
timings, instead of making shortcuts and trusting that the DPI timings 
match exactly (even if they would). Calculating these is rather error 
prone already.

At some point we want to adjust the DSI timings (at least if/when 
implementing burst mode). While even then we'll aim to match the exact 
DPI time, I think it's better to calculate the DSI htotal from the 
adjusted DSI timings. If the DSI timings are not right, then the htotal 
will also match that "wrongness", instead of just showing the DPI htotal.

  Tomi

Re: [PATCH v3 16/17] drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs

[Tomi Valkeinen]

Hi,

On 20/04/2025 21:01, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver currently expects the pixel clock and the HS clock to be
>> compatible, but the DPHY PLL doesn't give very finely grained rates.
>> This often leads to the situation where the pipeline just fails, as the
>> resulting HS clock is just too off.
>>
>> We could change the driver to do a better job on adjusting the DSI
>> blanking values, hopefully getting a working pipeline even if the pclk
>> and HS clocks are not exactly compatible. But that is a bigger work.
>>
>> What we can do easily is to see in .atomic_check() what HS clock rate we
>> can get, based on the pixel clock rate, and then convert the HS clock
>> rate back to pixel clock rate and ask that rate from the crtc. If the
>> crtc has a good PLL (which is the case for TI K3 SoCs), this will fix
>> any issues wrt. the clock rates.
>>
>> If the crtc cannot provide the requested clock, well, we're no worse off
>> with this patch than what we have at the moment.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 37 ++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 63031379459e..165df5d595b8 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -908,6 +908,28 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>>   	return input_fmts;
>>   }
>>   
>> +static long cdns_dsi_round_pclk(struct cdns_dsi *dsi, unsigned long pclk)
>> +{
>> +	struct cdns_dsi_output *output = &dsi->output;
>> +	unsigned int nlanes = output->dev->lanes;
>> +	union phy_configure_opts phy_opts = { 0 };
>> +	u32 bitspp;
>> +	int ret;
>> +
>> +	bitspp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>> +
>> +	ret = phy_mipi_dphy_get_default_config(pclk, bitspp, nlanes,
>> +					       &phy_opts.mipi_dphy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &phy_opts);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return div_u64((u64)phy_opts.mipi_dphy.hs_clk_rate * nlanes, bitspp);
>> +}
>> +
>>   static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>   					struct drm_bridge_state *bridge_state,
>>   					struct drm_crtc_state *crtc_state,
>> @@ -919,11 +941,26 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>   	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>>   	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
>>   	struct videomode vm;
>> +	long pclk;
>>   
>>   	/* cdns-dsi requires negative syncs */
>>   	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>>   	adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>>   
>> +	/*
>> +	 * The DPHY PLL has quite a coarsely grained clock rate options. See
>> +	 * what hsclk rate we can achieve based on the pixel clock, convert it
>> +	 * back to pixel clock, set that to the adjusted_mode->clock. This is
>> +	 * all in hopes that the CRTC will be able to provide us the requested
>> +	 * clock, as otherwise the DPI and DSI clocks will be out of sync.
>> +	 */
>> +
>> +	pclk = cdns_dsi_round_pclk(dsi, adjusted_mode->clock * 1000);
>> +	if (pclk < 0)
>> +		return (int)pclk;
>> +
>> +	adjusted_mode->clock = pclk / 1000;
>> +
>>   	drm_display_mode_to_videomode(adjusted_mode, &vm);
>>   
>>   	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
> 
> I think at this point cdns_dsi_check_conf is really only creating a
> dsi_cfg (from the video-mode) so that it can later be used, and then
> running phy_mipi_dphy_get_default_config(), and phy_validate(), both of
> which have nothing to do with the freshly made dsi_cfg.

True. And cdns_dsi_mode2cfg() can't fail anymore, so it could be just a 
void function.

> If there is no benefit in running both the latter functions again after
> cdns_dsi_round_pclk() runs them, then perhaps we can just drop the
> cdns_dsi_check_conf() altogether in favor of cdns_dsi_mode2cfg() being
> called from .atomic_check()?

cdns_dsi_round_pclk() doesn't store the phy_cfg anywhere, whereas 
cdns_dsi_check_conf() does.

I think I originally wanted to have this structure, because:

cdns_dsi_round_pclk() is given adjusted_mode->clock * 1000, and it 
returns an "optimal" pixel clock that matches the DSI clock what the DSI 
PLL can provide us.

This rate is then divided by 1000, and set to adjusted_mode->clock.

We then call cdns_dsi_check_conf(), calling 
phy_mipi_dphy_get_default_config() and phy_validate(), but this time 
it's with the "optimal" pixel clock (although truncated to kHz). So the 
calls are not identical to cdns_dsi_round_pclk().

Then, I have a local patch that adds some code to cdns_dsi_check_conf(). 
It will check if there is a difference between the requested hs-clk 
(which we got from phy_mipi_dphy_get_default_config()) and the final one 
(which we got from phy_validate()), and if so, adjusts the DSI HFP so 
that the total DSI line time would be close to the one we would have 
gotten with the requested hs-clk.

While I think that code is valid, the adjustments were always so small 
that they rounded to 0, so I did not post that patch. Nevertheless, it 
kind of points out that calling the functions again is not totally 
pointless =).

To be honest, I'm a bit reluctant to start rehashing the series. It's 
rather time consuming to test, and some errors are not immediately 
visible, if they're only seen as occasional glitches on the screen.

If the code has no issues as such, I would suggest to do further 
cleanups on top so that we'd get the upstream fixed (as it's very badly 
broken at the moment) asap.

> Furthermore, I understand updating the adjusted_mode->clock might help
> the CRTC to use a pixel clock that's more in-tune with the _actual_
> hs_clk_rate that is going to be generated. But, I am worried that it'll
> cause a delta from the intended fps from the CRTC's end because the
> timings aren't adjusted in accordance with the pixel-clock.

Yes, it does. Also, it's not "more in-tune", as in some cases the clocks 
are not at all in-tune =). The DSI PLL here is quite coarse with the rates.

> Perhaps, along with pixel-clock, we can update the dsi_htotal and
> dpi_htotal both too, to
> 
> new_dsi_htotal = (hs_clk_rate * #lanes) / (dpi_votal * fps * 8)
> new_dpi_htotal = (hs_clk_rate * #lanes) / (dpi_vtotal * fps * bitspp).
> 
> And then, the respective front-porches can too get updated accordingly.

Perhaps. But if there's a bridge after the cdns-dsi, its atomic_check 
has already been called, with the original timings. Did it already use 
those timings for creating its config?

And is it better to output with the timings that (at least try to) match 
the requested ones, but with an adjusted pclk, resulting in fps that 
slightly off, or with both adjusted timings and adjusted pclk, getting a 
better fps match?

In my experience devices usually allow variations in the pixel clock 
quite easily, but some are very strict at getting exactly the timings 
that it expects.

  Tomi