[RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge

[RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge

[Marek Vasut]

This patch series attempts to address a problem of missing support for DSI
bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem
has two variants.

First, a DSI->to->x bridge derives its own internal clock from DSI HS clock,
but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such
bridge in case it operates without Xtal. In that case, the TC358767 driver
must be able to negotiate the specific suitable DSI HS clock frequency for
the chip.

Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather
guess and hope they both guess the same number as their neighbor, the DSI HS
clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is
dangerous, since the PLL capabilities on both ends of the DSI bus might differ
and the DSI host could easily end up generating wildly different clock than
what the DSI bridge/panel expects to receive.

This series attempts to address these negotiation problems by extending the
existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs
callback in struct drm_bridge_funcs {}. The extended version returns not only
a list of a list of bus formats supported by a bridge, but the entire list of
struct drm_bus_cfg, which currently contains format and bus flags, but can be
extended with other members, like desired clock frequency, as required.

This series demonstrates such extension by adding the support for negotiating
the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI
bridge.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>

TC358767 part depends on:
https://patchwork.freedesktop.org/series/100372

Marek Vasut (7):
  drm/bridge: Pass struct drm_bus_cfg to select_bus_fmt_recursive()
  drm/bridge: Add new atomic_get_input_bus_cfgs callback
  drm/bridge: Extend struct drm_bus_cfg with clock field
  drm/bridge: dw-mipi-dsi: Move PLL setup into atomic_enable
  drm/bridge: dw-mipi-dsi: Pass bridge state into
    dw_mipi_dsi_get_lane_mbps()
  drm/bridge: dw-mipi-dsi: Prefer DSI bus clock settings from
    bridge_state
  drm/bridge: tc358767: Add support for PLL clock derivation from DSI HS
    clock

 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 22 +++--
 drivers/gpu/drm/bridge/tc358767.c             | 97 +++++++++++++++----
 drivers/gpu/drm/drm_bridge.c                  | 78 +++++++++++----
 .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   |  1 +
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c         | 17 ++--
 include/drm/bridge/dw_mipi_dsi.h              |  2 +
 include/drm/drm_atomic.h                      |  5 +
 include/drm/drm_bridge.h                      | 42 ++++++++
 8 files changed, 214 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH 1/7] drm/bridge: Pass struct drm_bus_cfg to select_bus_fmt_recursive()

[Marek Vasut]

Pass the entire struct drm_bus_cfg {} to select_bus_fmt_recursive()
instead of only u32 out_bus_fmt which contains only the pixel format.
This would permit passing more bus format data around during the
bridge-to-bridge format negotiation.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_bridge.c | 39 +++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebcb..f052d50106758 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -821,11 +821,13 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 				    struct drm_bridge *cur_bridge,
 				    struct drm_crtc_state *crtc_state,
 				    struct drm_connector_state *conn_state,
-				    u32 out_bus_fmt)
+				    struct drm_bus_cfg *out_bus_cfg)
 {
 	struct drm_bridge_state *cur_state;
 	unsigned int num_in_bus_fmts, i;
 	struct drm_bridge *prev_bridge;
+	struct drm_bus_cfg fixed_bus_cfg;
+	struct drm_bus_cfg *in_bus_cfgs;
 	u32 *in_bus_fmts;
 	int ret;
 
@@ -841,10 +843,11 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 	 */
 	if (!cur_bridge->funcs->atomic_get_input_bus_fmts) {
 		if (cur_bridge != first_bridge) {
+			fixed_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
 			ret = select_bus_fmt_recursive(first_bridge,
 						       prev_bridge, crtc_state,
 						       conn_state,
-						       MEDIA_BUS_FMT_FIXED);
+						       &fixed_bus_cfg);
 			if (ret)
 				return ret;
 		}
@@ -855,7 +858,7 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 		 */
 		if (cur_state) {
 			cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
-			cur_state->output_bus_cfg.format = out_bus_fmt;
+			cur_state->output_bus_cfg.format = out_bus_cfg->format;
 		}
 
 		return 0;
@@ -872,34 +875,44 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 							cur_state,
 							crtc_state,
 							conn_state,
-							out_bus_fmt,
+							out_bus_cfg->format,
 							&num_in_bus_fmts);
 	if (!num_in_bus_fmts)
 		return -ENOTSUPP;
 	else if (!in_bus_fmts)
 		return -ENOMEM;
 
+	/* Transcribe in_bus_fmts to in_bus_cfgs */
+	in_bus_cfgs = kcalloc(num_in_bus_fmts, sizeof(*in_bus_cfgs), GFP_KERNEL);
+	if (!in_bus_cfgs)
+		return -ENOMEM;
+
+	for (i = 0; i < num_in_bus_fmts; i++)
+		in_bus_cfgs[i].format = in_bus_fmts[i];
+
+	kfree(in_bus_fmts);
+
 	if (first_bridge == cur_bridge) {
-		cur_state->input_bus_cfg.format = in_bus_fmts[0];
-		cur_state->output_bus_cfg.format = out_bus_fmt;
-		kfree(in_bus_fmts);
+		cur_state->input_bus_cfg.format = in_bus_cfgs[0].format;
+		cur_state->output_bus_cfg.format = out_bus_cfg->format;
+		kfree(in_bus_cfgs);
 		return 0;
 	}
 
 	for (i = 0; i < num_in_bus_fmts; i++) {
 		ret = select_bus_fmt_recursive(first_bridge, prev_bridge,
 					       crtc_state, conn_state,
-					       in_bus_fmts[i]);
+					       &(in_bus_cfgs[i]));
 		if (ret != -ENOTSUPP)
 			break;
 	}
 
 	if (!ret) {
-		cur_state->input_bus_cfg.format = in_bus_fmts[i];
-		cur_state->output_bus_cfg.format = out_bus_fmt;
+		cur_state->input_bus_cfg.format = in_bus_cfgs[i].format;
+		cur_state->output_bus_cfg.format = out_bus_cfg->format;
 	}
 
-	kfree(in_bus_fmts);
+	kfree(in_bus_cfgs);
 	return ret;
 }
 
@@ -947,6 +960,7 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
 	struct drm_bridge_state *last_bridge_state;
 	unsigned int i, num_out_bus_fmts;
 	struct drm_bridge *last_bridge;
+	struct drm_bus_cfg out_bus_cfg;
 	u32 *out_bus_fmts;
 	int ret = 0;
 
@@ -988,8 +1002,9 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
 	}
 
 	for (i = 0; i < num_out_bus_fmts; i++) {
+		out_bus_cfg.format = out_bus_fmts[i];
 		ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
-					       conn_state, out_bus_fmts[i]);
+					       conn_state, &out_bus_cfg);
 		if (ret != -ENOTSUPP)
 			break;
 	}
-- 
2.34.1

[PATCH 2/7] drm/bridge: Add new atomic_get_input_bus_cfgs callback

[Marek Vasut]

Add new .atomic_get_input_bus_cfgs callback into struct drm_bridge_funcs {}.
This is an extended version of .atomic_get_input_bus_fmts callback which
only returns list of bus formats supported by a bridge and provides no way
to e.g. limit clock frequency required between neighboring bridges.

The new .atomic_get_input_bus_cfgs callback returns list of struct drm_bus_cfg,
which currently contains format and bus flags, but can be extended with other
members, like desired clock frequency, as required.

The .atomic_get_input_bus_cfgs should replace the .atomic_get_input_bus_fmts
once drivers get converted over. The conversion could be done using a script.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_bridge.c | 47 ++++++++++++++++++++++++------------
 include/drm/drm_bridge.h     | 42 ++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index f052d50106758..a069c50cc7d6b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -841,7 +841,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 	 * hope that it can handle this situation gracefully (by providing
 	 * appropriate default values).
 	 */
-	if (!cur_bridge->funcs->atomic_get_input_bus_fmts) {
+	if (!cur_bridge->funcs->atomic_get_input_bus_cfgs &&
+	    !cur_bridge->funcs->atomic_get_input_bus_fmts) {
 		if (cur_bridge != first_bridge) {
 			fixed_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
 			ret = select_bus_fmt_recursive(first_bridge,
@@ -865,32 +866,48 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 	}
 
 	/*
-	 * If the driver implements ->atomic_get_input_bus_fmts() it
-	 * should also implement the atomic state hooks.
+	 * If the driver implements ->atomic_get_input_bus_cfgs()
+	 * or legacy ->atomic_get_input_bus_fmts() it should also
+	 * implement the atomic state hooks.
 	 */
 	if (WARN_ON(!cur_state))
 		return -EINVAL;
 
-	in_bus_fmts = cur_bridge->funcs->atomic_get_input_bus_fmts(cur_bridge,
+	if (cur_bridge->funcs->atomic_get_input_bus_cfgs) {
+		in_bus_cfgs = cur_bridge->funcs->atomic_get_input_bus_cfgs(
+							cur_bridge,
+							cur_state,
+							crtc_state,
+							conn_state,
+							out_bus_cfg,
+							&num_in_bus_fmts);
+		if (!num_in_bus_fmts)
+			return -ENOTSUPP;
+		else if (!in_bus_cfgs)
+			return -ENOMEM;
+	} else {
+		in_bus_fmts = cur_bridge->funcs->atomic_get_input_bus_fmts(
+							cur_bridge,
 							cur_state,
 							crtc_state,
 							conn_state,
 							out_bus_cfg->format,
 							&num_in_bus_fmts);
-	if (!num_in_bus_fmts)
-		return -ENOTSUPP;
-	else if (!in_bus_fmts)
-		return -ENOMEM;
+		if (!num_in_bus_fmts)
+			return -ENOTSUPP;
+		else if (!in_bus_fmts)
+			return -ENOMEM;
 
-	/* Transcribe in_bus_fmts to in_bus_cfgs */
-	in_bus_cfgs = kcalloc(num_in_bus_fmts, sizeof(*in_bus_cfgs), GFP_KERNEL);
-	if (!in_bus_cfgs)
-		return -ENOMEM;
+		/* Transcribe in_bus_fmts to in_bus_cfgs */
+		in_bus_cfgs = kcalloc(num_in_bus_fmts, sizeof(*in_bus_cfgs), GFP_KERNEL);
+		if (!in_bus_cfgs)
+			return -ENOMEM;
 
-	for (i = 0; i < num_in_bus_fmts; i++)
-		in_bus_cfgs[i].format = in_bus_fmts[i];
+		for (i = 0; i < num_in_bus_fmts; i++)
+			in_bus_cfgs[i].format = in_bus_fmts[i];
 
-	kfree(in_bus_fmts);
+		kfree(in_bus_fmts);
+	}
 
 	if (first_bridge == cur_bridge) {
 		cur_state->input_bus_cfg.format = in_bus_cfgs[0].format;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 061d87313fac9..b2fe0ee7294b5 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -439,6 +439,48 @@ struct drm_bridge_funcs {
 					   struct drm_connector_state *conn_state,
 					   unsigned int *num_output_fmts);
 
+	/**
+	 * @atomic_get_input_bus_cfgs:
+	 *
+	 * Return the supported bus configurations on the input end of a bridge
+	 * for a specific output bus format.
+	 *
+	 * The returned array must be allocated with kmalloc() and will be
+	 * freed by the caller. If the allocation fails, NULL should be
+	 * returned. num_output_fmts must be set to the returned array size.
+	 * Formats listed in the returned array should be listed in decreasing
+	 * preference order (the core will try all formats until it finds one
+	 * that works). When the format is not supported NULL should be
+	 * returned and num_output_fmts should be set to 0.
+	 *
+	 * This method is called on all elements of the bridge chain as part of
+	 * the bus format negotiation process that happens in
+	 * drm_atomic_bridge_chain_select_bus_fmts().
+	 * This method is optional. When not implemented, the core will bypass
+	 * bus format negotiation on this element of the bridge without
+	 * failing, and the previous element in the chain will be passed
+	 * MEDIA_BUS_FMT_FIXED as its output bus format.
+	 *
+	 * Bridge drivers that need to support being linked to bridges that are
+	 * not supporting bus format negotiation should handle the
+	 * output_fmt == MEDIA_BUS_FMT_FIXED case appropriately, by selecting a
+	 * sensible default value or extracting this information from somewhere
+	 * else (FW property, &drm_display_mode, &drm_display_info, ...)
+	 *
+	 * Note: Even if input format selection on the first bridge has no
+	 * impact on the negotiation process (bus format negotiation stops once
+	 * we reach the first element of the chain), drivers are expected to
+	 * return accurate input formats as the input format may be used to
+	 * configure the CRTC output appropriately.
+	 */
+	struct drm_bus_cfg *(*atomic_get_input_bus_cfgs)
+					(struct drm_bridge *bridge,
+					 struct drm_bridge_state *bridge_state,
+					 struct drm_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state,
+					 struct drm_bus_cfg *output_cfg,
+					 unsigned int *num_input_cfgs);
+
 	/**
 	 * @atomic_get_input_bus_fmts:
 	 *
-- 
2.34.1

[PATCH 3/7] drm/bridge: Extend struct drm_bus_cfg with clock field

[Marek Vasut]

Extend struct drm_bus_cfg with a clock field. This makes it possible for an
upstream bridge (further from scanout engine) to indicate to a downstream
bridge which frequency it expects on a link. This is particularly useful in
case of DSI bridges which derive their own internal clock from the DSI HS
clock.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++++++
 include/drm/drm_atomic.h     | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index a069c50cc7d6b..6a5981b82499a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -859,7 +859,9 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 		 */
 		if (cur_state) {
 			cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
+			cur_state->input_bus_cfg.clock = 0;
 			cur_state->output_bus_cfg.format = out_bus_cfg->format;
+			cur_state->output_bus_cfg.clock = out_bus_cfg->clock;
 		}
 
 		return 0;
@@ -911,7 +913,9 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 
 	if (first_bridge == cur_bridge) {
 		cur_state->input_bus_cfg.format = in_bus_cfgs[0].format;
+		cur_state->input_bus_cfg.clock = in_bus_cfgs[0].clock;
 		cur_state->output_bus_cfg.format = out_bus_cfg->format;
+		cur_state->output_bus_cfg.clock = out_bus_cfg->clock;
 		kfree(in_bus_cfgs);
 		return 0;
 	}
@@ -926,7 +930,9 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 
 	if (!ret) {
 		cur_state->input_bus_cfg.format = in_bus_cfgs[i].format;
+		cur_state->input_bus_cfg.clock = in_bus_cfgs[i].clock;
 		cur_state->output_bus_cfg.format = out_bus_cfg->format;
+		cur_state->output_bus_cfg.clock = out_bus_cfg->clock;
 	}
 
 	kfree(in_bus_cfgs);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 1701c2128a5cb..32455cf28f0bc 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1077,6 +1077,11 @@ struct drm_bus_cfg {
 	 * @flags: DRM_BUS_* flags used on this bus
 	 */
 	u32 flags;
+
+	/**
+	 * @clock: Clock frequency in kHz used on this bus
+	 */
+	u32 clock;
 };
 
 /**
-- 
2.34.1

[PATCH 4/7] drm/bridge: dw-mipi-dsi: Move PLL setup into atomic_enable

[Marek Vasut]

The bridge clock configuration should happen in atomic_enable
instead of mode_set callback, since that is where the current
state of the bridge is available. Move the clock configuration
into atomic_enable callback.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index f4a6e4da903ca..7a2ea21dc0554 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -267,6 +267,8 @@ struct dw_mipi_dsi {
 	u32 format;
 	unsigned long mode_flags;
 
+	struct drm_display_mode mode;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 	struct debugfs_entries *debugfs_vpg;
@@ -1045,15 +1047,18 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
 {
 	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
 
-	dw_mipi_dsi_mode_set(dsi, adjusted_mode);
-	if (dsi->slave)
-		dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
+	drm_mode_copy(&dsi->mode, adjusted_mode);
 }
 
 static void dw_mipi_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 					     struct drm_bridge_state *old_bridge_state)
 {
 	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+	const struct drm_display_mode *adjusted_mode = &dsi->mode;
+
+	dw_mipi_dsi_mode_set(dsi, adjusted_mode);
+	if (dsi->slave)
+		dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
 
 	/* Switch to video mode for panel-bridge enable & panel enable */
 	dw_mipi_dsi_set_mode(dsi, MIPI_DSI_MODE_VIDEO);
-- 
2.34.1

[PATCH 5/7] drm/bridge: dw-mipi-dsi: Pass bridge state into dw_mipi_dsi_get_lane_mbps()

[Marek Vasut]

Pass negotiated bridge state, which now optionally contains required DSI bus
clock, into dw_mipi_dsi_get_lane_mbps(). The dw_mipi_dsi_get_lane_mbps() is
a misnomer, it is also responsible for configuration of the bridge PLL and
thus also configuration of the clock the bridge outputs on the DSI bus.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c   | 15 ++++++++++-----
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c |  1 +
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c           |  1 +
 include/drm/bridge/dw_mipi_dsi.h                |  2 ++
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 7a2ea21dc0554..d5f3c98cefdb5 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -996,7 +996,8 @@ static unsigned int dw_mipi_dsi_get_lanes(struct dw_mipi_dsi *dsi)
 }
 
 static void dw_mipi_dsi_mode_set(struct dw_mipi_dsi *dsi,
-				 const struct drm_display_mode *adjusted_mode)
+				 const struct drm_display_mode *adjusted_mode,
+				 const struct drm_bridge_state *bridge_state)
 {
 	const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops;
 	void *priv_data = dsi->plat_data->priv_data;
@@ -1005,8 +1006,9 @@ static void dw_mipi_dsi_mode_set(struct dw_mipi_dsi *dsi,
 
 	clk_prepare_enable(dsi->pclk);
 
-	ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags,
-				     lanes, dsi->format, &dsi->lane_mbps);
+	ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, bridge_state,
+				     dsi->mode_flags, lanes, dsi->format,
+				     &dsi->lane_mbps);
 	if (ret)
 		DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n");
 
@@ -1054,11 +1056,14 @@ static void dw_mipi_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 					     struct drm_bridge_state *old_bridge_state)
 {
 	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	const struct drm_bridge_state *bridge_state;
 	const struct drm_display_mode *adjusted_mode = &dsi->mode;
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
 
-	dw_mipi_dsi_mode_set(dsi, adjusted_mode);
+	dw_mipi_dsi_mode_set(dsi, adjusted_mode, bridge_state);
 	if (dsi->slave)
-		dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
+		dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode, bridge_state);
 
 	/* Switch to video mode for panel-bridge enable & panel enable */
 	dw_mipi_dsi_set_mode(dsi, MIPI_DSI_MODE_VIDEO);
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 4ed7a68681978..39965e0d16c95 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -526,6 +526,7 @@ static void dw_mipi_dsi_phy_power_off(void *priv_data)
 
 static int
 dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
+			  const struct drm_bridge_state *bridge_state,
 			  unsigned long mode_flags, u32 lanes, u32 format,
 			  unsigned int *lane_mbps)
 {
diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index 32cb41b2202fe..0132e576339dd 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -239,6 +239,7 @@ static void dw_mipi_dsi_phy_power_off(void *priv_data)
 
 static int
 dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
+			  const struct drm_bridge_state *bridge_state,
 			  unsigned long mode_flags, u32 lanes, u32 format,
 			  unsigned int *lane_mbps)
 {
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 5286a53a1875d..f449ff36c4653 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -11,6 +11,7 @@
 
 #include <linux/types.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_modes.h>
 
 struct drm_display_mode;
@@ -32,6 +33,7 @@ struct dw_mipi_dsi_phy_ops {
 	void (*power_off)(void *priv_data);
 	int (*get_lane_mbps)(void *priv_data,
 			     const struct drm_display_mode *mode,
+			     const struct drm_bridge_state *bridge_state,
 			     unsigned long mode_flags, u32 lanes, u32 format,
 			     unsigned int *lane_mbps);
 	int (*get_timing)(void *priv_data, unsigned int lane_mbps,
-- 
2.34.1

[PATCH 6/7] drm/bridge: dw-mipi-dsi: Prefer DSI bus clock settings from bridge_state

[Marek Vasut]

The bridge_state now contains clock frequency required by the next bridge.
This information is optional, since very few drivers pass this information
now. In case the required clock frequency is part of bridge state, use it
for the DSI bus clock frequency, otherwise fall back to the old guesswork.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index 0132e576339dd..e3fea94db9bfa 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -258,13 +258,17 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
 
 	pll_in_khz = (unsigned int)(clk_get_rate(dsi->pllref_clk) / 1000);
 
-	/* Compute requested pll out */
-	bpp = mipi_dsi_pixel_format_to_bpp(format);
-	pll_out_khz = mode->clock * bpp / lanes;
+	pll_out_khz = bridge_state->output_bus_cfg.clock;
 
-	/* Add 20% to pll out to be higher than pixel bw (burst mode only) */
-	if (mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
-		pll_out_khz = (pll_out_khz * 12) / 10;
+	if (pll_out_khz == 0) {
+		/* Guess requested pll out */
+		bpp = mipi_dsi_pixel_format_to_bpp(format);
+		pll_out_khz = mode->clock * bpp / lanes;
+
+		/* Add 20% to pll out to be higher than pixel bw (burst mode only) */
+		if (mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
+			pll_out_khz = (pll_out_khz * 12) / 10;
+	}
 
 	if (pll_out_khz > dsi->lane_max_kbps) {
 		pll_out_khz = dsi->lane_max_kbps;
-- 
2.34.1

[PATCH 7/7] drm/bridge: tc358767: Add support for PLL clock derivation from DSI HS clock

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating without RefClk
Xtal in DSI-to-DPI mode. In this mode of operation, the chip PLLs are fed
with clock derived from the DSI HS clock.

The SYSPLL is fed from DSI HS clock divided by 2*7 and the resulting clock
must match the allowed RefClk supported by the chip, which is either of
13 MHz, 26 MHz, 19.2 MHz, 38.4 MHz and no other RefClk clock are allowed.
That limits the allowed DSI HS clock as well. The PixelPLL is fed directly
with DSI HS clock SDR and derives DPI pixel clock from those clock.

In order to impose these very specific limitations on DSI HS clock, this
patch implements .atomic_get_input_bus_cfgs callback and calculates the
most suitable clock to be passed on to the DSI host bridge.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/bridge/tc358767.c | 97 +++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 2b9fceb1333fa..98ea07912f721 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -250,11 +250,13 @@
 #define PLLEN				BIT(0)
 #define PXL_PLLPARAM		0x0914
 #define IN_SEL_REFCLK			(0 << 14)
+#define IN_SEL_DSI_HSCK			(1 << 14)
 #define SYS_PLLPARAM		0x0918
 #define REF_FREQ_38M4			(0 << 8) /* 38.4 MHz */
 #define REF_FREQ_19M2			(1 << 8) /* 19.2 MHz */
 #define REF_FREQ_26M			(2 << 8) /* 26 MHz */
 #define REF_FREQ_13M			(3 << 8) /* 13 MHz */
+#define SYSCLK_SEL_DSI_HSCLK		(1 << 4)
 #define SYSCLK_SEL_LSCLK		(0 << 4)
 #define LSCLK_DIV_1			(0 << 0)
 #define LSCLK_DIV_2			(1 << 0)
@@ -492,6 +494,27 @@ static u32 tc_srcctrl(struct tc_data *tc)
 	return reg;
 }
 
+static int tc_mode_to_refclk(struct tc_data *tc, struct drm_display_mode *mode)
+{
+	u32 clock;
+
+	/* Minimum DSI clock lane clock. */
+	clock = (mode->htotal * mode->vtotal * 24 * 60) / tc->dsi_lanes / 2;
+	clock /= 7;	/* DSIClk/7 */
+
+	/* Round the DSI clock to nearest supported faster clock. */
+	if (clock < 13000000)		/* DSIClk / 2 / 7 = 13 MHz RefClk */
+		return 13000000;
+	else if (clock < 19200000)	/* DSIClk / 2 / 7 = 19.2 MHz RefClk */
+		return 19200000;
+	else if (clock < 26000000)	/* DSIClk / 2 / 7 = 26 MHz RefClk */
+		return 26000000;
+	else if (clock < 38400000)	/* DSIClk / 2 / 7 = 38.4 MHz RefClk */
+		return 38400000;
+	else
+		return 0;
+}
+
 static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
 {
 	int ret;
@@ -508,6 +531,7 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
 
 static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 {
+	unsigned long rate;
 	int ret;
 	int i_pre, best_pre = 1;
 	int i_post, best_post = 1;
@@ -596,10 +620,17 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 	pxl_pllparam  = vco_hi << 24; /* For PLL VCO >= 300 MHz = 1 */
 	pxl_pllparam |= ext_div[best_pre] << 20; /* External Pre-divider */
 	pxl_pllparam |= ext_div[best_post] << 16; /* External Post-divider */
-	pxl_pllparam |= IN_SEL_REFCLK; /* Use RefClk as PLL input */
 	pxl_pllparam |= best_div << 8; /* Divider for PLL RefClk */
 	pxl_pllparam |= best_mul; /* Multiplier for PLL */
 
+	rate = clk_get_rate(tc->refclk);
+
+	/* DSI-to-DPI mode with clock inferred from DSI HS */
+	if (rate == 0 && tc->bridge.type == DRM_MODE_CONNECTOR_DPI)
+		pxl_pllparam |= IN_SEL_DSI_HSCK; /* Use DSI HSCK as PLL input */
+	else
+		pxl_pllparam |= IN_SEL_REFCLK; /* Use RefClk as PLL input */
+
 	ret = regmap_write(tc->regmap, PXL_PLLPARAM, pxl_pllparam);
 	if (ret)
 		return ret;
@@ -637,9 +668,19 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 static int tc_set_syspllparam(struct tc_data *tc)
 {
 	unsigned long rate;
-	u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
+	u32 pllparam = LSCLK_DIV_2;
 
 	rate = clk_get_rate(tc->refclk);
+
+	/* DSI-to-DPI mode with clock inferred from DSI HS */
+	if (rate == 0 && tc->bridge.type == DRM_MODE_CONNECTOR_DPI) {
+		pllparam |= SYSCLK_SEL_DSI_HSCLK;
+		/* Get RefClk rate from DSI HS clock. */
+		rate = tc_mode_to_refclk(tc, &tc->mode);
+	} else {
+		pllparam |= SYSCLK_SEL_LSCLK;
+	}
+
 	switch (rate) {
 	case 38400000:
 		pllparam |= REF_FREQ_38M4;
@@ -1266,9 +1307,18 @@ static int tc_dpi_stream_enable(struct tc_data *tc)
 	if (ret)
 		return ret;
 
+	value = clk_get_rate(tc->refclk);
+	if (!value) {
+		/*
+		 * If RefClk Xtal is not connected, it is the DSI HS clock
+		 * SDR that is directly fed into the Pixel PLL and used as
+		 * input clock. Hence no multiplication of this value by 2.
+		 */
+		value = 7 * tc_mode_to_refclk(tc, &tc->mode);
+	}
+
 	/* Pixel PLL must always be enabled for DPI mode */
-	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-			    1000 * tc->mode.clock);
+	ret = tc_pxl_pll_en(tc, value, 1000 * tc->mode.clock);
 	if (ret)
 		return ret;
 
@@ -1781,30 +1831,38 @@ static void tc_edp_bridge_detach(struct drm_bridge *bridge)
 	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
 }
 
-#define MAX_INPUT_SEL_FORMATS	1
+#define MAX_INPUT_SEL_CONFIGS	1
 
-static u32 *
-tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+static struct drm_bus_cfg *
+tc_dpi_atomic_get_input_bus_cfgs(struct drm_bridge *bridge,
 				    struct drm_bridge_state *bridge_state,
 				    struct drm_crtc_state *crtc_state,
 				    struct drm_connector_state *conn_state,
-				    u32 output_fmt,
-				    unsigned int *num_input_fmts)
+				    struct drm_bus_cfg *out_cfg,
+				    unsigned int *num_input_cfgs)
 {
-	u32 *input_fmts;
+	struct tc_data *tc = bridge_to_tc(bridge);
+	struct drm_bus_cfg *input_cfgs;
+	u32 clock;
 
-	*num_input_fmts = 0;
+	/* Minimum DSI clock lane clock in kHz (DSIClk / 7 = RefClk). */
+	clock = 2 * 7 * tc_mode_to_refclk(tc, &crtc_state->adjusted_mode) / 1000;
+	if (clock == 0)
+		return NULL;
 
-	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+	*num_input_cfgs = 0;
+
+	input_cfgs = kcalloc(MAX_INPUT_SEL_CONFIGS, sizeof(*input_cfgs),
 			     GFP_KERNEL);
-	if (!input_fmts)
+	if (!input_cfgs)
 		return NULL;
 
 	/* This is the DSI-end bus format */
-	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
-	*num_input_fmts = 1;
+	input_cfgs[0].format = MEDIA_BUS_FMT_RGB888_1X24;
+	input_cfgs[0].clock = clock;	/* kHz */
+	*num_input_cfgs = MAX_INPUT_SEL_CONFIGS;
 
-	return input_fmts;
+	return input_cfgs;
 }
 
 static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
@@ -1817,7 +1875,7 @@ static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
-	.atomic_get_input_bus_fmts = tc_dpi_atomic_get_input_bus_fmts,
+	.atomic_get_input_bus_cfgs = tc_dpi_atomic_get_input_bus_cfgs,
 };
 
 static const struct drm_bridge_funcs tc_bridge_funcs = {
@@ -2107,7 +2165,10 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		usleep_range(5000, 10000);
 	}
 
-	tc->refclk = devm_clk_get(dev, "ref");
+	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI)
+		tc->refclk = devm_clk_get_optional(dev, "ref");
+	else
+		tc->refclk = devm_clk_get(dev, "ref");
 	if (IS_ERR(tc->refclk)) {
 		ret = PTR_ERR(tc->refclk);
 		dev_err(dev, "Failed to get refclk: %d\n", ret);
-- 
2.34.1

Re: [PATCH 3/7] drm/bridge: Extend struct drm_bus_cfg with clock field

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

Hi,

On Sat, Feb 19, 2022 at 01:28:40AM +0100, Marek Vasut wrote:
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 1701c2128a5cb..32455cf28f0bc 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1077,6 +1077,11 @@ struct drm_bus_cfg {
>  	 * @flags: DRM_BUS_* flags used on this bus
>  	 */
>  	u32 flags;
> +
> +	/**
> +	 * @clock: Clock frequency in kHz used on this bus
> +	 */
> +	u32 clock;
>  };

This is fairly vague. You were mentioning DSI: is it the pixel clock?
The HS clock rate? With or without counting the lanes? What about the
burst mode: would it be the lane or pixel rate?

It would be just as confusing for HDMI: is it the the TMDS character
rate? The TMDS bit rate ? TMDS Clock rate?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]

Hi,

On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote:
> This patch series attempts to address a problem of missing support for DSI
> bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem
> has two variants.
> 
> First, a DSI->to->x bridge derives its own internal clock from DSI HS clock,
> but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such
> bridge in case it operates without Xtal. In that case, the TC358767 driver
> must be able to negotiate the specific suitable DSI HS clock frequency for
> the chip.
> 
> Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather
> guess and hope they both guess the same number as their neighbor, the DSI HS
> clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is
> dangerous, since the PLL capabilities on both ends of the DSI bus might differ
> and the DSI host could easily end up generating wildly different clock than
> what the DSI bridge/panel expects to receive.
> 
> This series attempts to address these negotiation problems by extending the
> existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs
> callback in struct drm_bridge_funcs {}. The extended version returns not only
> a list of a list of bus formats supported by a bridge, but the entire list of
> struct drm_bus_cfg, which currently contains format and bus flags, but can be
> extended with other members, like desired clock frequency, as required.
> 
> This series demonstrates such extension by adding the support for negotiating
> the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI
> bridge.

We discussed it a bit on IRC as well but there's another issue with
this, let's imagine this setup:

encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor

HDMI is fairly favorable, and it would probably use pixel clocks of
either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your
DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz.

594Mhz is going to be used by those new fancy monitors, and thus the
preferred mode is likely to be using 594MHz.

With your solution, it effectively means that when the system will boot
up, the preferred mode will be reported to the userspace (and the fbdev
emulation), whatever is coming next is going to use it, and you're just
going to... refuse it because it never worked in the first place. You'll
leave a blank display, and that's it. That's not a great behavior,
really.

And since you don't get a state until you start a commit, this would
need to be able to work without one. Of course, some state parameters
will affect the clock (like the bpc count) so it won't be perfect, but
we can at least try.

Another thing is that the clock that needs to be negociated is likely to
be device specific. It's probably going to be fairly similar across
similar devices (like all the DSI bridges you mentioned are using the HS
clock), but I'm not sure we can make that assumption.

I think we could make something that work by asking the previous bridge
in the chain for a given clock rate with a given mode, and then filter
out / adjust anything we don't like. It would then be able to first
check if it can provide that clock in the first place, and then the rate
it has, and would be free to forward the query up to the encoder. And
since it's tied to the mode, it would work with mode_valid too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/7] drm/bridge: Extend struct drm_bus_cfg with clock field

[Marek Vasut]

On 2/24/22 16:19, Maxime Ripard wrote:
> Hi,

Hi,

> On Sat, Feb 19, 2022 at 01:28:40AM +0100, Marek Vasut wrote:
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 1701c2128a5cb..32455cf28f0bc 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -1077,6 +1077,11 @@ struct drm_bus_cfg {
>>   	 * @flags: DRM_BUS_* flags used on this bus
>>   	 */
>>   	u32 flags;
>> +
>> +	/**
>> +	 * @clock: Clock frequency in kHz used on this bus
>> +	 */
>> +	u32 clock;
>>   };
> 
> This is fairly vague. You were mentioning DSI: is it the pixel clock?

DSI HS clock is the one I need.

I hope we can flesh out what exactly should be in here.

> The HS clock rate?

Yes

> With or without counting the lanes? What about the

Without

> burst mode: would it be the lane or pixel rate?

Still the HS clock rate.

> It would be just as confusing for HDMI: is it the the TMDS character
> rate? The TMDS bit rate ? TMDS Clock rate?

For HDMI I would expect 148.5 MHz here , and if HDMI needs additional 
extras, they might have to be added to struct drm_bus_cfg as extra fields ?

Re: [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge

[Marek Vasut]

On 2/24/22 16:40, Maxime Ripard wrote:
> Hi,

Hi,

> On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote:
>> This patch series attempts to address a problem of missing support for DSI
>> bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem
>> has two variants.
>>
>> First, a DSI->to->x bridge derives its own internal clock from DSI HS clock,
>> but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such
>> bridge in case it operates without Xtal. In that case, the TC358767 driver
>> must be able to negotiate the specific suitable DSI HS clock frequency for
>> the chip.
>>
>> Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather
>> guess and hope they both guess the same number as their neighbor, the DSI HS
>> clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is
>> dangerous, since the PLL capabilities on both ends of the DSI bus might differ
>> and the DSI host could easily end up generating wildly different clock than
>> what the DSI bridge/panel expects to receive.
>>
>> This series attempts to address these negotiation problems by extending the
>> existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs
>> callback in struct drm_bridge_funcs {}. The extended version returns not only
>> a list of a list of bus formats supported by a bridge, but the entire list of
>> struct drm_bus_cfg, which currently contains format and bus flags, but can be
>> extended with other members, like desired clock frequency, as required.
>>
>> This series demonstrates such extension by adding the support for negotiating
>> the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI
>> bridge.
> 
> We discussed it a bit on IRC as well but there's another issue with
> this, let's imagine this setup:
> 
> encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor
> 
> HDMI is fairly favorable, and it would probably use pixel clocks of
> either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your
> DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz.
> 
> 594Mhz is going to be used by those new fancy monitors, and thus the
> preferred mode is likely to be using 594MHz.
> 
> With your solution, it effectively means that when the system will boot
> up, the preferred mode will be reported to the userspace (and the fbdev
> emulation), whatever is coming next is going to use it, and you're just
> going to... refuse it because it never worked in the first place. You'll
> leave a blank display, and that's it. That's not a great behavior,
> really.

If you cannot support such a panel with this kind of scanout engine, 
what else would you do than blank screen ?

> And since you don't get a state until you start a commit, this would
> need to be able to work without one. Of course, some state parameters
> will affect the clock (like the bpc count) so it won't be perfect, but
> we can at least try.
> 
> Another thing is that the clock that needs to be negociated is likely to
> be device specific. It's probably going to be fairly similar across
> similar devices (like all the DSI bridges you mentioned are using the HS
> clock), but I'm not sure we can make that assumption.

The bridge (data sink) should be able to figure out what kind of clock 
it needs from the source and then request those, yes. With DSI you can 
make an assumption about what kind of clock frequencies each link mode 
would require, but in general, you cannot assume much.

> I think we could make something that work by asking the previous bridge
> in the chain for a given clock rate with a given mode, and then filter
> out / adjust anything we don't like. It would then be able to first
> check if it can provide that clock in the first place, and then the rate
> it has, and would be free to forward the query up to the encoder. And
> since it's tied to the mode, it would work with mode_valid too.

It seems to me this is similar to this solution, except it must happen 
when the mode is available ? But then the question also comes to mind, 
should select_bus_fmt_recursive() be called only after mode is available 
too ?

Re: [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5444 bytes --]

On Thu, Feb 24, 2022 at 09:24:57PM +0100, Marek Vasut wrote:
> On 2/24/22 16:40, Maxime Ripard wrote:
> > Hi,
> 
> Hi,
> 
> > On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote:
> > > This patch series attempts to address a problem of missing support for DSI
> > > bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem
> > > has two variants.
> > > 
> > > First, a DSI->to->x bridge derives its own internal clock from DSI HS clock,
> > > but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such
> > > bridge in case it operates without Xtal. In that case, the TC358767 driver
> > > must be able to negotiate the specific suitable DSI HS clock frequency for
> > > the chip.
> > > 
> > > Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather
> > > guess and hope they both guess the same number as their neighbor, the DSI HS
> > > clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is
> > > dangerous, since the PLL capabilities on both ends of the DSI bus might differ
> > > and the DSI host could easily end up generating wildly different clock than
> > > what the DSI bridge/panel expects to receive.
> > > 
> > > This series attempts to address these negotiation problems by extending the
> > > existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs
> > > callback in struct drm_bridge_funcs {}. The extended version returns not only
> > > a list of a list of bus formats supported by a bridge, but the entire list of
> > > struct drm_bus_cfg, which currently contains format and bus flags, but can be
> > > extended with other members, like desired clock frequency, as required.
> > > 
> > > This series demonstrates such extension by adding the support for negotiating
> > > the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI
> > > bridge.
> > 
> > We discussed it a bit on IRC as well but there's another issue with
> > this, let's imagine this setup:
> > 
> > encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor
> > 
> > HDMI is fairly favorable, and it would probably use pixel clocks of
> > either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your
> > DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz.
> > 
> > 594Mhz is going to be used by those new fancy monitors, and thus the
> > preferred mode is likely to be using 594MHz.
> > 
> > With your solution, it effectively means that when the system will boot
> > up, the preferred mode will be reported to the userspace (and the fbdev
> > emulation), whatever is coming next is going to use it, and you're just
> > going to... refuse it because it never worked in the first place. You'll
> > leave a blank display, and that's it. That's not a great behavior,
> > really.
> 
> If you cannot support such a panel with this kind of scanout engine, what
> else would you do than blank screen ?

An HDMI monitor typically supports multiple pixel clocks. If you can't
reach one there's usually plenty of other options that could work. And
if there's none, then so be it, we shouldn't report any either.

> > And since you don't get a state until you start a commit, this would
> > need to be able to work without one. Of course, some state parameters
> > will affect the clock (like the bpc count) so it won't be perfect, but
> > we can at least try.
> > 
> > Another thing is that the clock that needs to be negociated is likely to
> > be device specific. It's probably going to be fairly similar across
> > similar devices (like all the DSI bridges you mentioned are using the HS
> > clock), but I'm not sure we can make that assumption.
> 
> The bridge (data sink) should be able to figure out what kind of clock it
> needs from the source and then request those, yes. With DSI you can make an
> assumption about what kind of clock frequencies each link mode would
> require, but in general, you cannot assume much.

You're modifying the core, you shouldn't be reasoning about DSI, but any
display interface really.

> > I think we could make something that work by asking the previous bridge
> > in the chain for a given clock rate with a given mode, and then filter
> > out / adjust anything we don't like. It would then be able to first
> > check if it can provide that clock in the first place, and then the rate
> > it has, and would be free to forward the query up to the encoder. And
> > since it's tied to the mode, it would work with mode_valid too.
> 
> It seems to me this is similar to this solution, except it must happen when
> the mode is available ?

The mode will pretty much always be available. You can't really reason
about a clock rate without a mode.

> But then the question also comes to mind, should
> select_bus_fmt_recursive() be called only after mode is available too
> ?

Most of the drivers that use either atomic_get_input_bus_fmts or
atomic_get_output_bus_fmts don't use the state at all, so yes.

The only exception is the HDMI output format selection in dw-hdmi, but
that's a bit of a mess across drivers, each one going its own separate
way to achieve this.

So, yeah. Usually formats are less important when it comes to whether a
device can actually support a given mode than the clock it feeds from
though, so it has a lesser impact.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/7] drm/bridge: Extend struct drm_bus_cfg with clock field

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On Thu, Feb 24, 2022 at 09:07:19PM +0100, Marek Vasut wrote:
> On 2/24/22 16:19, Maxime Ripard wrote:
> > On Sat, Feb 19, 2022 at 01:28:40AM +0100, Marek Vasut wrote:
> > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > index 1701c2128a5cb..32455cf28f0bc 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -1077,6 +1077,11 @@ struct drm_bus_cfg {
> > >   	 * @flags: DRM_BUS_* flags used on this bus
> > >   	 */
> > >   	u32 flags;
> > > +
> > > +	/**
> > > +	 * @clock: Clock frequency in kHz used on this bus
> > > +	 */
> > > +	u32 clock;
> > >   };
> > 
> > This is fairly vague. You were mentioning DSI: is it the pixel clock?
> 
> DSI HS clock is the one I need.
> 
> I hope we can flesh out what exactly should be in here.
> 
> > The HS clock rate?
> 
> Yes
> 
> > With or without counting the lanes? What about the
> 
> Without
> 
> > burst mode: would it be the lane or pixel rate?
> 
> Still the HS clock rate.
> 
> > It would be just as confusing for HDMI: is it the the TMDS character
> > rate? The TMDS bit rate ? TMDS Clock rate?
> 
> For HDMI I would expect 148.5 MHz here , and if HDMI needs additional
> extras, they might have to be added to struct drm_bus_cfg as extra fields ?

The thing is: you're patching some core code here. Whatever you come up
with needs to be properly defined, documented, and should apply to all
the display interfaces we support. It cannot be an after-thought.

Even for DSI, I don't think that the HS clock is something that is
desirable: how does it interacts with virtual channels? burst mode or
not?

The pixel clock is a better choice I think for this, since this is
abstract enough to apply to all the interfaces, and the devices can
easily compute whatever they want to based on the pixel clock as well.

If you *really* need the HS clock itself, then the struct
mipi_dsi_device feels like a better abstraction. Which raises the
question: why can't you use hs_rate?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]