[PATCH 0/9] drm/bridge: ti-sn65dsi86: Improve support for AUO B116XAK01 + other low res DP

[PATCH 0/9] drm/bridge: ti-sn65dsi86: Improve support for AUO B116XAK01 + other low res DP

[Douglas Anderson]

This series contains a pile of patches that was created to support
hooking up the AUO B116XAK01 panel to the eDP side of the bridge.  In
general it should be useful for hooking up a wider variety of DP
panels to the bridge, especially those with lower resolution and lower
bits per pixel.

The overall result of this series:
* Allows panels with fewer than 4 DP lanes hooked up to work.
* Optimizes the link rate for panels with 6 bpp.
* Supports trying more than one link rate when training if the main
  link rate didn't work.

It's not expected that this series will break any existing users, but
it is possible that the patch to skip non-standard DP rates could mean
that a panel that used to use one of these non-standard link rates
will now run at a higher rate than it used to.  If this happens, the
patch could be reverted or someone could figure out how to decide when
it's OK to use the non-standard rates.

To support the AUO B116XAK01, we could actually stop at the ("Use
18-bit DP if we can") patch since that causes the panel to run at a
link rate of 1.62 which works.  The patches to try more than one link
rate were all developed prior to realizing that I could just use
18-bit mode and were validated with that patch reverted.

The patch to try more than one rate was validated by forcing the code
to try 2.16 GHz (but still skip 2.43 GHz, which trains but shows
garbage on AUO B116XAK01) and seeing that we'd try 2.16 GHz (and fail)
and then eventually pass at 2.7 GHz and show a pretty screen.

These patches were tested on sdm845-cheza atop mainline as of
2019-12-13 and also on another board (the one with AUO B116XAK01) atop
a downstream kernel tree.

This patch series doesn't do anything to optimize the MIPI link and
only focuses on the DP link.  For instance, it's left as an exercise
to the reader to see if we can use the 666-packed mode on the MIPI
link and save some power (because we could lower the clock rate).

I am nowhere near a display expert and my knowledge of DP and MIPI is
pretty much zero.  If something about this patch series smells wrong,
it probably is.  Please let know and I'll try to fix it.


Douglas Anderson (9):
  drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates
  drm/bridge: ti-sn65dsi86: zero is never greater than an unsigned int
  drm/bridge: ti-sn65dsi86: Don't use MIPI variables for DP link
  drm/bridge: ti-sn65dsi86: Config number of DP lanes Mo' Betta
  drm/bridge: ti-sn65dsi86: Read num lanes from the DP sink
  drm/bridge: ti-sn65dsi86: Use 18-bit DP if we can
  drm/bridge: ti-sn65dsi86: Group DP link training bits in a function
  drm/bridge: ti-sn65dsi86: Train at faster rates if slower ones fail
  drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 230 +++++++++++++++++++++-----
 1 file changed, 187 insertions(+), 43 deletions(-)

-- 
2.24.1.735.g03f4e72817-goog

[PATCH 1/9] drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates

[Douglas Anderson]

These two things were in one function.  Split into two.  This looks
like it's duplicating some code, but don't worry.  This is is just in
preparation for future changes.

This is intended to have zero functional change and will just make
future patches easier to understand.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 33 +++++++++++++++++++--------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 43abf01ebd4c..2fb9370a76e6 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -417,6 +417,24 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
 			   REFCLK_FREQ(i));
 }
 
+static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
+{
+	unsigned int bit_rate_mhz, clk_freq_mhz;
+	unsigned int val;
+	struct drm_display_mode *mode =
+		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+
+	/* set DSIA clk frequency */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
+	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
+}
+
 /**
  * LUT index corresponds to register value and
  * LUT values corresponds to dp data rate supported
@@ -426,22 +444,16 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
+static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
 {
-	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
-	unsigned int val, i;
+	unsigned int bit_rate_mhz, dp_rate_mhz;
+	unsigned int i;
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
 
 	/* set DSIA clk frequency */
 	bit_rate_mhz = (mode->clock / 1000) *
 			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
-	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
-
-	/* for each increment in val, frequency increases by 5MHz */
-	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
-		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
-	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 
 	/* set DP data rate */
 	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
@@ -510,7 +522,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   val);
 
 	/* set dsi/dp clk frequency value */
-	ti_sn_bridge_set_dsi_dp_rate(pdata);
+	ti_sn_bridge_set_dsi_rate(pdata);
+	ti_sn_bridge_set_dp_rate(pdata);
 
 	/* enable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 2/9] drm/bridge: ti-sn65dsi86: zero is never greater than an unsigned int

[Douglas Anderson]

When we iterate over ti_sn_bridge_dp_rate_lut, there's no reason to
start at index 0 which always contains the value 0.  0 is not a valid
link rate.

This change should have no real effect but is a small cleanup.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 2fb9370a76e6..7b596af265e4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -458,7 +458,7 @@ static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
 	/* set DP data rate */
 	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
 							DP_CLK_FUDGE_DEN;
-	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
+	for (i = 1; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
 		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
 			break;
 
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 3/9] drm/bridge: ti-sn65dsi86: Don't use MIPI variables for DP link

[Douglas Anderson]

The ti-sn65dsi86 is a bridge from MIPI to DP and thus has two links:
the MIPI link and the DP link.  The two links do not need to have the
same format or number of lanes.  Stop using MIPI variables when
talking about the DP link.

This has zero functional change because:
* currently we are hardcoding the MIPI link as unpacked RGB888 which
  requires 24 bits and currently we are not changing the DP link rate
  from the bridge's default of 8 bits per pixel.
* currently we are hardcoding both the MIPI and DP as being 4 lanes.

This is all in prep for fixing some of the above.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 7b596af265e4..ab644baaf90c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -100,6 +100,7 @@ struct ti_sn_bridge {
 	struct drm_panel		*panel;
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
+	int				dp_lanes;
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -313,6 +314,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 	}
 
 	/* TODO: setting to 4 lanes always for now */
+	pdata->dp_lanes = 4;
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
@@ -451,13 +453,17 @@ static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
 
-	/* set DSIA clk frequency */
-	bit_rate_mhz = (mode->clock / 1000) *
-			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	/*
+	 * Calculate minimum bit rate based on our pixel clock.  At
+	 * the moment this driver never sets the DP_18BPP_EN bit in
+	 * register 0x5b so we hardcode 24bpp.
+	 */
+	bit_rate_mhz = (mode->clock / 1000) * 24;
 
-	/* set DP data rate */
-	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
+	/* Calculate minimum DP data rate, taking 80% as per DP spec */
+	dp_rate_mhz = ((bit_rate_mhz / pdata->dp_lanes) * DP_CLK_FUDGE_NUM) /
 							DP_CLK_FUDGE_DEN;
+
 	for (i = 1; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
 		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
 			break;
@@ -517,7 +523,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   CHA_DSI_LANES_MASK, val);
 
 	/* DP lane config */
-	val = DP_NUM_LANES(pdata->dsi->lanes - 1);
+	val = DP_NUM_LANES(pdata->dp_lanes - 1);
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
 			   val);
 
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 4/9] drm/bridge: ti-sn65dsi86: Config number of DP lanes Mo' Betta

[Douglas Anderson]

The driver used to say that the value to program into bridge register
0x93 was dp_lanes - 1.  Looking at the datasheet for the bridge, this
is wrong.  The data sheet says:
* 1 = 1 lane
* 2 = 2 lanes
* 3 = 4 lanes

A more proper way to express this encoding is min(dp_lanes, 3).

At the moment this change has zero effect because we've hardcoded the
number of DP lanes to 4.  ...and (4 - 1) == min(4, 3).  How fortunate!
...but soon we'll stop hardcoding the number of lanes.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ab644baaf90c..d55d19759796 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -523,7 +523,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   CHA_DSI_LANES_MASK, val);
 
 	/* DP lane config */
-	val = DP_NUM_LANES(pdata->dp_lanes - 1);
+	val = DP_NUM_LANES(min(pdata->dp_lanes, 3));
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
 			   val);
 
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 5/9] drm/bridge: ti-sn65dsi86: Read num lanes from the DP sink

[Douglas Anderson]

At least one panel hooked up to the bridge (AUO B116XAK01) only
supports 1 lane of DP.  Let's read this information and stop
hardcoding 4 DP lanes.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 +++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d55d19759796..0fc9e97b2d98 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -313,8 +313,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 		goto err_dsi_host;
 	}
 
-	/* TODO: setting to 4 lanes always for now */
-	pdata->dp_lanes = 4;
+	/* TODO: setting to 4 MIPI lanes always for now */
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
@@ -511,12 +510,41 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
 	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
 }
 
+static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
+{
+	u8 data;
+	int ret;
+
+	ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LANE_COUNT, &data);
+	if (ret != 1) {
+		DRM_DEV_ERROR(pdata->dev,
+			      "Can't read lane count (%d); assuming 4\n", ret);
+		return 4;
+	}
+
+	return data & DP_LANE_COUNT_MASK;
+}
+
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
 	unsigned int val;
 	int ret;
 
+	/*
+	 * Run with the maximum number of lanes that the DP sink supports.
+	 *
+	 * Depending use cases, we might want to revisit this later because:
+	 * - It's plausible that someone may have run fewer lines to the
+	 *   sink than the sink actually supports, assuming that the lines
+	 *   will just be driven at a higher rate.
+	 * - The DP spec seems to indicate that it's more important to minimize
+	 *   the number of lanes than the link rate.
+	 *
+	 * If we do revisit, it would be important to measure the power impact.
+	 */
+	pdata->dp_lanes = ti_sn_get_max_lanes(pdata);
+
 	/* DSI_A lane config */
 	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
 	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 6/9] drm/bridge: ti-sn65dsi86: Use 18-bit DP if we can

[Douglas Anderson]

The current bridge driver always forced us to use 24 bits per pixel
over the DP link.  This is a waste if you are hooked up to a panel
that only supports 6 bits per color or fewer, since in that case you
ran run at 18 bits per pixel and thus end up at a lower DP clock rate.

Let's support this.

While at it, let's clean up the math in the function to avoid rounding
errors (and round in the correct direction when we have to round).
Numbers are sufficiently small (because mode->clock is in kHz) that we
don't need to worry about integer overflow.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 0fc9e97b2d98..d5990a0947b9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -51,6 +51,7 @@
 #define SN_ENH_FRAME_REG			0x5A
 #define  VSTREAM_ENABLE				BIT(3)
 #define SN_DATA_FORMAT_REG			0x5B
+#define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
 #define SN_AUX_WDATA_REG(x)			(0x64 + (x))
@@ -436,6 +437,14 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 }
 
+static unsigned int ti_sn_bridge_get_bpp(struct ti_sn_bridge *pdata)
+{
+	if (pdata->connector.display_info.bpc <= 6)
+		return 18;
+	else
+		return 24;
+}
+
 /**
  * LUT index corresponds to register value and
  * LUT values corresponds to dp data rate supported
@@ -447,21 +456,17 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 
 static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
 {
-	unsigned int bit_rate_mhz, dp_rate_mhz;
+	unsigned int bit_rate_khz, dp_rate_mhz;
 	unsigned int i;
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
 
-	/*
-	 * Calculate minimum bit rate based on our pixel clock.  At
-	 * the moment this driver never sets the DP_18BPP_EN bit in
-	 * register 0x5b so we hardcode 24bpp.
-	 */
-	bit_rate_mhz = (mode->clock / 1000) * 24;
+	/* Calculate minimum bit rate based on our pixel clock. */
+	bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
 
 	/* Calculate minimum DP data rate, taking 80% as per DP spec */
-	dp_rate_mhz = ((bit_rate_mhz / pdata->dp_lanes) * DP_CLK_FUDGE_NUM) /
-							DP_CLK_FUDGE_DEN;
+	dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
+				   1000 * pdata->dp_lanes * DP_CLK_FUDGE_DEN);
 
 	for (i = 1; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
 		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
@@ -550,6 +555,10 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
 			   CHA_DSI_LANES_MASK, val);
 
+	/* Set the DP output format (18 bpp or 24 bpp) */
+	val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
+	regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
+
 	/* DP lane config */
 	val = DP_NUM_LANES(min(pdata->dp_lanes, 3));
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 7/9] drm/bridge: ti-sn65dsi86: Group DP link training bits in a function

[Douglas Anderson]

We'll re-organize the ti_sn_bridge_enable() function a bit to group
together all the parts relating to link training and split them into a
sub-function.  This is not intended to have any functional change and
is in preparation for trying link training several times at different
rates.  One small side effect here is that if link training fails
we'll now leave the DP PLL disabled, but that seems like a sane thing
to do.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 86 ++++++++++++++++-----------
 1 file changed, 52 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d5990a0947b9..48fb4dc72e1c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -530,6 +530,46 @@ static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
 	return data & DP_LANE_COUNT_MASK;
 }
 
+static int ti_sn_link_training(struct ti_sn_bridge *pdata)
+{
+	unsigned int val;
+	int ret;
+
+	/* set dp clk frequency value */
+	ti_sn_bridge_set_dp_rate(pdata);
+
+	/* enable DP PLL */
+	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
+
+	ret = regmap_read_poll_timeout(pdata->regmap, SN_DPPLL_SRC_REG, val,
+				       val & DPPLL_SRC_DP_PLL_LOCK, 1000,
+				       50 * 1000);
+	if (ret) {
+		DRM_ERROR("DP_PLL_LOCK polling failed (%d)\n", ret);
+		goto exit;
+	}
+
+	/* Semi auto link training mode */
+	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
+	ret = regmap_read_poll_timeout(pdata->regmap, SN_ML_TX_MODE_REG, val,
+				       val == ML_TX_MAIN_LINK_OFF ||
+				       val == ML_TX_NORMAL_MODE, 1000,
+				       500 * 1000);
+	if (ret) {
+		DRM_ERROR("Training complete polling failed (%d)\n", ret);
+	} else if (val == ML_TX_MAIN_LINK_OFF) {
+		DRM_ERROR("Link training failed, link is off\n");
+		ret = -EIO;
+	}
+
+exit:
+	/* Disable the PLL if we failed */
+	if (ret)
+		regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
+
+	return ret;
+}
+
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
@@ -555,29 +595,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
 			   CHA_DSI_LANES_MASK, val);
 
-	/* Set the DP output format (18 bpp or 24 bpp) */
-	val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
-	regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
-
-	/* DP lane config */
-	val = DP_NUM_LANES(min(pdata->dp_lanes, 3));
-	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
-			   val);
-
-	/* set dsi/dp clk frequency value */
+	/* set dsi clk frequency value */
 	ti_sn_bridge_set_dsi_rate(pdata);
-	ti_sn_bridge_set_dp_rate(pdata);
-
-	/* enable DP PLL */
-	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
-
-	ret = regmap_read_poll_timeout(pdata->regmap, SN_DPPLL_SRC_REG, val,
-				       val & DPPLL_SRC_DP_PLL_LOCK, 1000,
-				       50 * 1000);
-	if (ret) {
-		DRM_ERROR("DP_PLL_LOCK polling failed (%d)\n", ret);
-		return;
-	}
 
 	/**
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
@@ -588,19 +607,18 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
 			   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
 
-	/* Semi auto link training mode */
-	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
-	ret = regmap_read_poll_timeout(pdata->regmap, SN_ML_TX_MODE_REG, val,
-				       val == ML_TX_MAIN_LINK_OFF ||
-				       val == ML_TX_NORMAL_MODE, 1000,
-				       500 * 1000);
-	if (ret) {
-		DRM_ERROR("Training complete polling failed (%d)\n", ret);
-		return;
-	} else if (val == ML_TX_MAIN_LINK_OFF) {
-		DRM_ERROR("Link training failed, link is off\n");
+	/* Set the DP output format (18 bpp or 24 bpp) */
+	val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
+	regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
+
+	/* DP lane config */
+	val = DP_NUM_LANES(min(pdata->dp_lanes, 3));
+	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
+			   val);
+
+	ret = ti_sn_link_training(pdata);
+	if (ret)
 		return;
-	}
 
 	/* config video parameters */
 	ti_sn_bridge_set_video_timings(pdata);
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 8/9] drm/bridge: ti-sn65dsi86: Train at faster rates if slower ones fail

[Douglas Anderson]

If we fail training at a lower DP link rate let's now keep trying
until we run out of rates to try.  Basically the algorithm here is to
start at the link rate that is the theoretical minimum and then slowly
bump up until we run out of rates or hit the max rate of the sink.  We
query the sink using a DPCD read.

This is, in fact, important in practice.  Specifically at least one
panel hooked up to the bridge (AUO B116XAK01) had a theoretical min
rate more than 1.62 GHz (if run at 24 bpp) and fails to train at the
next rate (2.16 GHz).  It would train at 2.7 GHz, though.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 69 +++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 48fb4dc72e1c..cc8bef172f69 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -454,7 +454,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
 	unsigned int i;
@@ -472,8 +472,42 @@ static void ti_sn_bridge_set_dp_rate(struct ti_sn_bridge *pdata)
 		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
 			break;
 
-	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
-			   DP_DATARATE_MASK, DP_DATARATE(i));
+	return i;
+}
+
+static int ti_sn_bridge_get_max_dp_rate_idx(struct ti_sn_bridge *pdata)
+{
+	u8 data;
+	int ret;
+
+	ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &data);
+	if (ret != 1) {
+		DRM_DEV_ERROR(pdata->dev,
+			      "Can't read max rate (%d); assuming 5.4 GHz\n",
+			      ret);
+		return ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1;
+	}
+
+	/*
+	 * Return an index into ti_sn_bridge_dp_rate_lut.  Just hardcode
+	 * these indicies since it's not like the register spec is ever going
+	 * to change and a loop would just be more complicated.  Apparently
+	 * the DP sink can only return these few rates as supported even
+	 * though the bridge allows some rates in between.
+	 */
+	switch (data) {
+	case DP_LINK_BW_1_62:
+		return 1;
+	case DP_LINK_BW_2_7:
+		return 4;
+	case DP_LINK_BW_5_4:
+		return 7;
+	}
+
+	DRM_DEV_ERROR(pdata->dev,
+		      "Unexpected max data rate (%#x); assuming 5.4 GHz\n",
+		      (int)data);
+	return ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1;
 }
 
 static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
@@ -530,13 +564,15 @@ static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
 	return data & DP_LANE_COUNT_MASK;
 }
 
-static int ti_sn_link_training(struct ti_sn_bridge *pdata)
+static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
+			       const char **last_err_str)
 {
 	unsigned int val;
 	int ret;
 
 	/* set dp clk frequency value */
-	ti_sn_bridge_set_dp_rate(pdata);
+	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
+			   DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
 
 	/* enable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
@@ -545,7 +581,7 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata)
 				       val & DPPLL_SRC_DP_PLL_LOCK, 1000,
 				       50 * 1000);
 	if (ret) {
-		DRM_ERROR("DP_PLL_LOCK polling failed (%d)\n", ret);
+		*last_err_str = "DP_PLL_LOCK polling failed";
 		goto exit;
 	}
 
@@ -556,9 +592,9 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata)
 				       val == ML_TX_NORMAL_MODE, 1000,
 				       500 * 1000);
 	if (ret) {
-		DRM_ERROR("Training complete polling failed (%d)\n", ret);
+		*last_err_str = "Training complete polling failed";
 	} else if (val == ML_TX_MAIN_LINK_OFF) {
-		DRM_ERROR("Link training failed, link is off\n");
+		*last_err_str = "Link training failed, link is off";
 		ret = -EIO;
 	}
 
@@ -573,6 +609,9 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata)
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	const char *last_err_str;
+	int dp_rate_idx;
+	int max_dp_rate_idx;
 	unsigned int val;
 	int ret;
 
@@ -616,9 +655,19 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
 			   val);
 
-	ret = ti_sn_link_training(pdata);
-	if (ret)
+	/* Train until we run out of rates */
+	max_dp_rate_idx = ti_sn_bridge_get_max_dp_rate_idx(pdata);
+	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
+	     dp_rate_idx <= max_dp_rate_idx;
+	     dp_rate_idx++) {
+		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
+		if (!ret)
+			break;
+	}
+	if (ret) {
+		DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
 		return;
+	}
 
 	/* config video parameters */
 	ti_sn_bridge_set_video_timings(pdata);
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 9/9] drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Douglas Anderson]

The bridge chip supports these DP rates according to TI's spec:
* 1.62 Gbps (RBR)
* 2.16 Gbps
* 2.43 Gbps
* 2.7 Gbps (HBR)
* 3.24 Gbps
* 4.32 Gbps
* 5.4 Gbps (HBR2)

As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec.
If other rates work then I believe it's because the sink has allowed
bending the spec a little bit.

I hoped that we could tell which rates would work and which rates
didn't work based on whether link training passed or not.
Unfortunately this wasn't so good on at least one panel hooked up to
the bridge (AUO B116XAK01).  On that panel with 24 bpp configured:
* 1.62: too small for 69500 kHz at 24 bpp
* 2.16: link training failed
* 2.43: link training passed, but garbage on screen
* 2.7:  joy and happiness

Let's bypass all non-standard rates, which makes this panel happy
working.  I'll still keep the code organized in such a way where it
_could_ try the other rates, though, on the assumption that eventually
someone will find a way to make use of them.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index cc8bef172f69..cb774ee536cd 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -454,6 +454,15 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
+/**
+ * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut
+ * is as per the DP spec (AKA a standard) as opposed to an intermediate
+ * rate.
+ */
+static const bool ti_sn_bridge_dp_rate_standard[] = {
+	false, true, false, false, true, false, false, true
+};
+
 static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
@@ -660,6 +669,18 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
 	     dp_rate_idx <= max_dp_rate_idx;
 	     dp_rate_idx++) {
+		/*
+		 * To be on the safe side, we'll skip all non-standard
+		 * rates and move up to the next standard one.  This is
+		 * because some panels will pass link training with a non-
+		 * standard rate but just show garbage.  If the non-standard
+		 * rates are useful we should figure out how to enable them
+		 * through querying the panel, having a per-panel whitelist,
+		 * or adding a DT property.
+		 */
+		if (!ti_sn_bridge_dp_rate_standard[dp_rate_idx])
+			continue;
+
 		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
 		if (!ret)
 			break;
-- 
2.24.1.735.g03f4e72817-goog

Re: [PATCH 9/9] drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Daniel Vetter]

On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote:
> The bridge chip supports these DP rates according to TI's spec:
> * 1.62 Gbps (RBR)
> * 2.16 Gbps
> * 2.43 Gbps
> * 2.7 Gbps (HBR)
> * 3.24 Gbps
> * 4.32 Gbps
> * 5.4 Gbps (HBR2)
> 
> As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec.
> If other rates work then I believe it's because the sink has allowed
> bending the spec a little bit.

I think you need to look at the eDP spec. And filter this stuff correctly
(there's more fields there for these somewhat irky edp timings). Simply
not using them works, but it's defeating the point of having these
intermediate clocks for edp panels.
-Daniel

> 
> I hoped that we could tell which rates would work and which rates
> didn't work based on whether link training passed or not.
> Unfortunately this wasn't so good on at least one panel hooked up to
> the bridge (AUO B116XAK01).  On that panel with 24 bpp configured:
> * 1.62: too small for 69500 kHz at 24 bpp
> * 2.16: link training failed
> * 2.43: link training passed, but garbage on screen
> * 2.7:  joy and happiness
> 
> Let's bypass all non-standard rates, which makes this panel happy
> working.  I'll still keep the code organized in such a way where it
> _could_ try the other rates, though, on the assumption that eventually
> someone will find a way to make use of them.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index cc8bef172f69..cb774ee536cd 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -454,6 +454,15 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
>  	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
>  };
>  
> +/**
> + * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut
> + * is as per the DP spec (AKA a standard) as opposed to an intermediate
> + * rate.
> + */
> +static const bool ti_sn_bridge_dp_rate_standard[] = {
> +	false, true, false, false, true, false, false, true
> +};
> +
>  static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
>  {
>  	unsigned int bit_rate_khz, dp_rate_mhz;
> @@ -660,6 +669,18 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
>  	     dp_rate_idx <= max_dp_rate_idx;
>  	     dp_rate_idx++) {
> +		/*
> +		 * To be on the safe side, we'll skip all non-standard
> +		 * rates and move up to the next standard one.  This is
> +		 * because some panels will pass link training with a non-
> +		 * standard rate but just show garbage.  If the non-standard
> +		 * rates are useful we should figure out how to enable them
> +		 * through querying the panel, having a per-panel whitelist,
> +		 * or adding a DT property.
> +		 */
> +		if (!ti_sn_bridge_dp_rate_standard[dp_rate_idx])
> +			continue;
> +
>  		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
>  		if (!ret)
>  			break;
> -- 
> 2.24.1.735.g03f4e72817-goog
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 9/9] drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Doug Anderson]

Hi,

On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote:
> > The bridge chip supports these DP rates according to TI's spec:
> > * 1.62 Gbps (RBR)
> > * 2.16 Gbps
> > * 2.43 Gbps
> > * 2.7 Gbps (HBR)
> > * 3.24 Gbps
> > * 4.32 Gbps
> > * 5.4 Gbps (HBR2)
> >
> > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec.
> > If other rates work then I believe it's because the sink has allowed
> > bending the spec a little bit.
>
> I think you need to look at the eDP spec. And filter this stuff correctly
> (there's more fields there for these somewhat irky edp timings). Simply
> not using them works, but it's defeating the point of having these
> intermediate clocks for edp panels.

Ah, I see my problem.  I had earlier only found the eDP 1.3 spec which
doesn't mention these rates.  The eDP 1.4 spec does, however.  ...and
the change log for 1.4 specifically mentions that it added 4 new link
rates and also adds the "SUPPORTED_LINK_RATES" register.

I can try to spin a v2 but for now I'll hold off for additional feedback.

I'll also note that I'd be totally OK if just the first 8 patches in
this series landed for now and someone could eventually figure out how
to make this work.  With just the first 8 patches I think we will
still be in an improved state compared to where we were before (and it
fixes the panel I care about) and someone could later write the code
to skip unsupported rates...


-Doug

Re: [PATCH 0/9] drm/bridge: ti-sn65dsi86: Improve support for AUO B116XAK01 + other low res DP

[Rob Clark]

On Fri, Dec 13, 2019 at 3:46 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> This series contains a pile of patches that was created to support
> hooking up the AUO B116XAK01 panel to the eDP side of the bridge.  In
> general it should be useful for hooking up a wider variety of DP
> panels to the bridge, especially those with lower resolution and lower
> bits per pixel.
>
> The overall result of this series:
> * Allows panels with fewer than 4 DP lanes hooked up to work.
> * Optimizes the link rate for panels with 6 bpp.
> * Supports trying more than one link rate when training if the main
>   link rate didn't work.
>
> It's not expected that this series will break any existing users, but
> it is possible that the patch to skip non-standard DP rates could mean
> that a panel that used to use one of these non-standard link rates
> will now run at a higher rate than it used to.  If this happens, the
> patch could be reverted or someone could figure out how to decide when
> it's OK to use the non-standard rates.
>
> To support the AUO B116XAK01, we could actually stop at the ("Use
> 18-bit DP if we can") patch since that causes the panel to run at a
> link rate of 1.62 which works.  The patches to try more than one link
> rate were all developed prior to realizing that I could just use
> 18-bit mode and were validated with that patch reverted.
>
> The patch to try more than one rate was validated by forcing the code
> to try 2.16 GHz (but still skip 2.43 GHz, which trains but shows
> garbage on AUO B116XAK01) and seeing that we'd try 2.16 GHz (and fail)
> and then eventually pass at 2.7 GHz and show a pretty screen.
>
> These patches were tested on sdm845-cheza atop mainline as of
> 2019-12-13 and also on another board (the one with AUO B116XAK01) atop
> a downstream kernel tree.
>
> This patch series doesn't do anything to optimize the MIPI link and
> only focuses on the DP link.  For instance, it's left as an exercise
> to the reader to see if we can use the 666-packed mode on the MIPI
> link and save some power (because we could lower the clock rate).
>
> I am nowhere near a display expert and my knowledge of DP and MIPI is
> pretty much zero.  If something about this patch series smells wrong,
> it probably is.  Please let know and I'll try to fix it.
>
>
> Douglas Anderson (9):
>   drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates
>   drm/bridge: ti-sn65dsi86: zero is never greater than an unsigned int
>   drm/bridge: ti-sn65dsi86: Don't use MIPI variables for DP link
>   drm/bridge: ti-sn65dsi86: Config number of DP lanes Mo' Betta
>   drm/bridge: ti-sn65dsi86: Read num lanes from the DP sink
>   drm/bridge: ti-sn65dsi86: Use 18-bit DP if we can
>   drm/bridge: ti-sn65dsi86: Group DP link training bits in a function
>   drm/bridge: ti-sn65dsi86: Train at faster rates if slower ones fail
>   drm/bridge: ti-sn65dsi86: Skip non-standard DP rates
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 230 +++++++++++++++++++++-----
>  1 file changed, 187 insertions(+), 43 deletions(-)

I've given these a spin on my yoga c630, which uses the same bridge, so:

Tested-by: Rob Clark <robdclark@gmail.com>

I've got one small fixup for a compiler warning for the 2nd to last,
and with that, the first 8 are:

Reviewed-by: Rob Clark <robdclark@gmail.com>

I've also got a fixup for the last one which reads
SUPPORTED_LINK_RATES.  However the panel I have is pre eDP 1.4, so the
interesting codepath there is untested.  Not sure offhand if the
panels you have are eDP 1.4+ or not?

BR,
-R

>
> --
> 2.24.1.735.g03f4e72817-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] fixup! drm/bridge: ti-sn65dsi86: Train at faster rates if slower ones fail

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Fixes:

In file included from ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:25:
../drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function ‘ti_sn_bridge_enable’:
../include/drm/drm_print.h:339:2: warning: ‘last_err_str’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  339 |  drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
      |  ^~~~~~~~~~~~~~
../drivers/gpu/drm/bridge/ti-sn65dsi86.c:650:14: note: ‘last_err_str’ was declared here
  650 |  const char *last_err_str;
      |              ^~~~~~~~~~~~
In file included from ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:25:
../include/drm/drm_print.h:339:2: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  339 |  drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
      |  ^~~~~~~~~~~~~~
../drivers/gpu/drm/bridge/ti-sn65dsi86.c:654:6: note: ‘ret’ was declared here
  654 |  int ret;
      |      ^~~
  Building modules, stage 2.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index cb774ee536cd..976f98991b3d 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -618,11 +618,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
-	const char *last_err_str;
+	const char *last_err_str = "No supported DP rate";
 	int dp_rate_idx;
 	int max_dp_rate_idx;
 	unsigned int val;
-	int ret;
+	int ret = -EINVAL;
 
 	/*
 	 * Run with the maximum number of lanes that the DP sink supports.
-- 
2.23.0

[PATCH 2/2] fixup! drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

---
Note however, the panel I have on the yoga c630 is not eDP 1.4+, so I
cannot test that path.  But I think it looks correct.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 110 +++++++++++++++++++++-----
 1 file changed, 89 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 976f98991b3d..1cb53be7c9e9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -102,6 +102,9 @@ struct ti_sn_bridge {
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 	int				dp_lanes;
+	u8				edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
+	int				num_sink_rates;
+	int				sink_rate_idxs[DP_MAX_SUPPORTED_RATES];
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -454,15 +457,6 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-/**
- * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut
- * is as per the DP spec (AKA a standard) as opposed to an intermediate
- * rate.
- */
-static const bool ti_sn_bridge_dp_rate_standard[] = {
-	false, true, false, false, true, false, false, true
-};
-
 static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
@@ -573,11 +567,95 @@ static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
 	return data & DP_LANE_COUNT_MASK;
 }
 
+/* TODO possibly fold ti_sn_get_max_lanes() into this? */
+static void ti_sn_read_sink_config(struct ti_sn_bridge *pdata)
+{
+	memset(pdata->edp_dpcd, 0, sizeof(pdata->edp_dpcd));
+
+	/*
+	 * Read the eDP display control registers.
+	 *
+	 * Do this independent of DP_DPCD_DISPLAY_CONTROL_CAPABLE bit in
+	 * DP_EDP_CONFIGURATION_CAP, because some buggy displays do not have it
+	 * set, but require eDP 1.4+ detection (e.g. for supported link rates
+	 * method). The display control registers should read zero if they're
+	 * not supported anyway.
+	 */
+	if (drm_dp_dpcd_read(&pdata->aux, DP_EDP_DPCD_REV,
+			     pdata->edp_dpcd, sizeof(pdata->edp_dpcd)) ==
+			     sizeof(pdata->edp_dpcd))
+		DRM_DEBUG_KMS("eDP DPCD: %*ph\n", (int) sizeof(pdata->edp_dpcd),
+			      pdata->edp_dpcd);
+
+	/* Read the eDP 1.4+ supported link rates. */
+	if (pdata->edp_dpcd[0] >= DP_EDP_14) {
+		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
+		int i, j;
+
+		drm_dp_dpcd_read(&pdata->aux, DP_SUPPORTED_LINK_RATES,
+				sink_rates, sizeof(sink_rates));
+
+		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
+			int val = le16_to_cpu(sink_rates[i]);
+			int rate_mhz;
+
+			if (val == 0)
+				break;
+
+			/* Value read multiplied by 200kHz gives the per-lane
+			 * link rate in kHz. The source rates are, however,
+			 * stored in MHz
+			 */
+			rate_mhz = DIV_ROUND_UP(val * 200, 1000);
+
+			/* If the rate is also supported by the bridge, add it
+			 * to the table of supported rates:
+			 */
+			for (j = 1; j < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); j++) {
+				if (rate_mhz == ti_sn_bridge_dp_rate_lut[j]) {
+					pdata->sink_rate_idxs[pdata->num_sink_rates++] = j;
+					break;
+				}
+			}
+		}
+		pdata->num_sink_rates = i;
+	} else {
+		int i;
+
+		/**
+		 * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut
+		 * is as per the DP spec (AKA a standard) as opposed to an intermediate
+		 * rate.
+		 */
+		static const bool ti_sn_bridge_dp_rate_standard[] = {
+			false, true, false, false, true, false, false, true
+		};
+
+		/* if prior to eDP 1.4, then just use the supported standard rates: */
+		for (i = 1; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
+			if (!ti_sn_bridge_dp_rate_standard[i])
+				continue;
+			pdata->sink_rate_idxs[pdata->num_sink_rates++] = i;
+		}
+	}
+}
+
 static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 			       const char **last_err_str)
 {
 	unsigned int val;
 	int ret;
+	int i;
+
+	/* check for supported rate: */
+	for (i = 0; i < pdata->num_sink_rates; i++)
+		if (pdata->sink_rate_idxs[i] == dp_rate_idx)
+			break;
+
+	if (i == pdata->num_sink_rates) {
+		*last_err_str = "Unsupported rate";
+		return -EINVAL;
+	}
 
 	/* set dp clk frequency value */
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
@@ -624,6 +702,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	unsigned int val;
 	int ret = -EINVAL;
 
+	ti_sn_read_sink_config(pdata);
+
 	/*
 	 * Run with the maximum number of lanes that the DP sink supports.
 	 *
@@ -669,18 +749,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
 	     dp_rate_idx <= max_dp_rate_idx;
 	     dp_rate_idx++) {
-		/*
-		 * To be on the safe side, we'll skip all non-standard
-		 * rates and move up to the next standard one.  This is
-		 * because some panels will pass link training with a non-
-		 * standard rate but just show garbage.  If the non-standard
-		 * rates are useful we should figure out how to enable them
-		 * through querying the panel, having a per-panel whitelist,
-		 * or adding a DT property.
-		 */
-		if (!ti_sn_bridge_dp_rate_standard[dp_rate_idx])
-			continue;
-
 		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
 		if (!ret)
 			break;
-- 
2.23.0

Re: [PATCH 9/9] drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Jeffrey Hugo]

On Fri, Dec 13, 2019 at 5:49 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote:
> > > The bridge chip supports these DP rates according to TI's spec:
> > > * 1.62 Gbps (RBR)
> > > * 2.16 Gbps
> > > * 2.43 Gbps
> > > * 2.7 Gbps (HBR)
> > > * 3.24 Gbps
> > > * 4.32 Gbps
> > > * 5.4 Gbps (HBR2)
> > >
> > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec.
> > > If other rates work then I believe it's because the sink has allowed
> > > bending the spec a little bit.
> >
> > I think you need to look at the eDP spec. And filter this stuff correctly
> > (there's more fields there for these somewhat irky edp timings). Simply
> > not using them works, but it's defeating the point of having these
> > intermediate clocks for edp panels.
>
> Ah, I see my problem.  I had earlier only found the eDP 1.3 spec which
> doesn't mention these rates.  The eDP 1.4 spec does, however.  ...and
> the change log for 1.4 specifically mentions that it added 4 new link
> rates and also adds the "SUPPORTED_LINK_RATES" register.

Yeah, you need the eDP spec.  I previously posted
https://patchwork.kernel.org/patch/11205201/ and was hoping Bjorn
would find time to test it.  Maybe it would fit well with your series?
 I'm coming back from tracel, and hope to review everything you have,
but this caught my eye.

>
> I can try to spin a v2 but for now I'll hold off for additional feedback.
>
> I'll also note that I'd be totally OK if just the first 8 patches in
> this series landed for now and someone could eventually figure out how
> to make this work.  With just the first 8 patches I think we will
> still be in an improved state compared to where we were before (and it
> fixes the panel I care about) and someone could later write the code
> to skip unsupported rates...
>
>
> -Doug

Re: [PATCH 9/9] drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Doug Anderson]

Hi,

On Sun, Dec 15, 2019 at 5:19 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>
> On Fri, Dec 13, 2019 at 5:49 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote:
> > > > The bridge chip supports these DP rates according to TI's spec:
> > > > * 1.62 Gbps (RBR)
> > > > * 2.16 Gbps
> > > > * 2.43 Gbps
> > > > * 2.7 Gbps (HBR)
> > > > * 3.24 Gbps
> > > > * 4.32 Gbps
> > > > * 5.4 Gbps (HBR2)
> > > >
> > > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec.
> > > > If other rates work then I believe it's because the sink has allowed
> > > > bending the spec a little bit.
> > >
> > > I think you need to look at the eDP spec. And filter this stuff correctly
> > > (there's more fields there for these somewhat irky edp timings). Simply
> > > not using them works, but it's defeating the point of having these
> > > intermediate clocks for edp panels.
> >
> > Ah, I see my problem.  I had earlier only found the eDP 1.3 spec which
> > doesn't mention these rates.  The eDP 1.4 spec does, however.  ...and
> > the change log for 1.4 specifically mentions that it added 4 new link
> > rates and also adds the "SUPPORTED_LINK_RATES" register.
>
> Yeah, you need the eDP spec.  I previously posted
> https://patchwork.kernel.org/patch/11205201/ and was hoping Bjorn
> would find time to test it.  Maybe it would fit well with your series?
>  I'm coming back from tracel, and hope to review everything you have,
> but this caught my eye.

Ah, interesting.  It looks like Rob has already posted a Fixup on my patch:

https://lore.kernel.org/r/20191215200632.1019065-1-robdclark@gmail.com

...that should also read the supported rates.  I need to go and review
/ test his new patch (I lost access to the hardware but should get it
back tomorrow or the next day), but would you be OK with going that
route?  I think my series is a superset of yours.  Specifically it has
these extra features atop yours:

* If link training fails and the panel supports faster rates, it will
try a faster rate in case it works.

* It adds support for using 6bpp when that's all that's needed,
reducing bandwidth to the panel (and link rate)

* It breaks things into smaller functions (assuming you agree this is
a good thing).

-Doug

Re: [PATCH 1/2] fixup! drm/bridge: ti-sn65dsi86: Train at faster rates if slower ones fail

[Doug Anderson]

Hi,

On Sun, Dec 15, 2019 at 12:05 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Fixes:
>
> In file included from ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:25:
> ../drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function ‘ti_sn_bridge_enable’:
> ../include/drm/drm_print.h:339:2: warning: ‘last_err_str’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   339 |  drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
>       |  ^~~~~~~~~~~~~~
> ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:650:14: note: ‘last_err_str’ was declared here
>   650 |  const char *last_err_str;
>       |              ^~~~~~~~~~~~
> In file included from ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:25:
> ../include/drm/drm_print.h:339:2: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   339 |  drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
>       |  ^~~~~~~~~~~~~~
> ../drivers/gpu/drm/bridge/ti-sn65dsi86.c:654:6: note: ‘ret’ was declared here
>   654 |  int ret;
>       |      ^~~
>   Building modules, stage 2.
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks!  I have rolled this into my v2.

-Doug

Re: [PATCH 2/2] fixup! drm/bridge: ti-sn65dsi86: Skip non-standard DP rates

[Doug Anderson]

Hi,

On Sun, Dec 15, 2019 at 12:06 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> ---
> Note however, the panel I have on the yoga c630 is not eDP 1.4+, so I
> cannot test that path.  But I think it looks correct.
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 110 +++++++++++++++++++++-----
>  1 file changed, 89 insertions(+), 21 deletions(-)

I ended up basing patch #9 in my v2 series slightly more on Jeffrey's
patch.  Hopefully it looks OK.

I don't have any eDP 1.4 panels either, so hopefully it's all good there...

-Doug