[PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this series of patches improves the DP helpers a bit and cleans up some
inconsistencies along the way.

v2 incorporates all review comments add collects Reviewed-bys from v1.

Thierry

Thierry Reding (21):
  drm/dp: Sort includes alphabetically
  drm/dp: Add missing kerneldoc for struct drm_dp_link
  drm/dp: Add drm_dp_link_reset() implementation
  drm/dp: Track link capabilities alongside settings
  drm/dp: Turn link capabilities into booleans
  drm/dp: Probe link using existing parsing helpers
  drm/dp: Read fast training capability from link
  drm/dp: Read TPS3 capability from sink
  drm/dp: Read channel coding capability from sink
  drm/dp: Read alternate scrambler reset capability from sink
  drm/dp: Read eDP version from DPCD
  drm/dp: Read AUX read interval from DPCD
  drm/dp: Do not busy-loop during link training
  drm/dp: Use drm_dp_aux_rd_interval()
  drm/dp: Add helper to get post-cursor adjustments
  drm/dp: Set channel coding on link configuration
  drm/dp: Enable alternate scrambler reset when supported
  drm/dp: Add drm_dp_link_choose() helper
  drm/dp: Add support for eDP link rates
  drm/dp: Remove a gratuituous blank line
  drm/bridge: tc358767: Use DP nomenclature

 drivers/gpu/drm/bridge/tc358767.c      |  22 +-
 drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
 drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
 drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
 drivers/gpu/drm/tegra/dpaux.c          |   8 +-
 drivers/gpu/drm/tegra/sor.c            |  32 +--
 include/drm/drm_dp_helper.h            | 124 +++++++++-
 8 files changed, 459 insertions(+), 87 deletions(-)

-- 
2.22.0

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

[PATCH v2 01/21] drm/dp: Sort includes alphabetically

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Keeping the list sorted alphabetically makes it much easier to determine
where to add new includes.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_dp_helper.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8364502f92cf..114261158b73 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -23,9 +23,9 @@
 #ifndef _DRM_DP_HELPER_H_
 #define _DRM_DP_HELPER_H_
 
-#include <linux/types.h>
-#include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/types.h>
 
 /*
  * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
-- 
2.22.0

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

[PATCH v2 02/21] drm/dp: Add missing kerneldoc for struct drm_dp_link

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The drm_dp_link structure tracks capabilities on the DP link. Add some
kerneldoc to explain what each of its fields means.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_dp_helper.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 114261158b73..935f331e6e72 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1358,6 +1358,13 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
  */
 #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
 
+/**
+ * struct drm_dp_link - DP link capabilities
+ * @revision: DP specification revision supported on the link
+ * @rate: maximum clock rate supported on the link
+ * @num_lanes: maximum number of lanes supported on the link
+ * @capabilities: bitmask of capabilities supported on the link
+ */
 struct drm_dp_link {
 	unsigned char revision;
 	unsigned int rate;
-- 
2.22.0

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

[PATCH v2 03/21] drm/dp: Add drm_dp_link_reset() implementation

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Subsequent patches will add non-volatile fields to struct drm_dp_link,
so introduce a function to zero out only the volatile fields.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ffc68d305afe..f5af71ec1b7d 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -336,6 +336,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
 
+static void drm_dp_link_reset(struct drm_dp_link *link)
+{
+	if (!link)
+		return;
+
+	link->revision = 0;
+	link->rate = 0;
+	link->num_lanes = 0;
+	link->capabilities = 0;
+}
+
 /**
  * drm_dp_link_probe() - probe a DisplayPort link for capabilities
  * @aux: DisplayPort AUX channel
@@ -352,7 +363,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	u8 values[3];
 	int err;
 
-	memset(link, 0, sizeof(*link));
+	drm_dp_link_reset(link);
 
 	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
 	if (err < 0)
-- 
2.22.0

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

[PATCH v2 04/21] drm/dp: Track link capabilities alongside settings

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Store capabilities in max_* fields and add separate fields for the
currently selected settings.

Cc: Rob Clark <robdclark@gmail.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/bridge/tc358767.c      | 14 ++++++-------
 drivers/gpu/drm/drm_dp_helper.c        | 16 ++++++++++-----
 drivers/gpu/drm/msm/edp/edp_ctrl.c     |  8 ++++----
 drivers/gpu/drm/rockchip/cdn-dp-core.c |  8 ++++----
 drivers/gpu/drm/rockchip/cdn-dp-reg.c  | 13 ++++++------
 drivers/gpu/drm/tegra/dpaux.c          |  8 ++++----
 drivers/gpu/drm/tegra/sor.c            | 28 +++++++++++++-------------
 include/drm/drm_dp_helper.h            | 15 +++++++++-----
 8 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index cebc8e620820..733fca7d3829 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -437,7 +437,7 @@ static u32 tc_srcctrl(struct tc_data *tc)
 		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
 	if (tc->link.spread)
 		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
-	if (tc->link.base.num_lanes == 2)
+	if (tc->link.base.lanes == 2)
 		reg |= DP0_SRCCTRL_LANES_2;	/* Two Main Channel Lanes */
 	if (tc->link.base.rate != 162000)
 		reg |= DP0_SRCCTRL_BW27;	/* 2.7 Gbps link */
@@ -674,9 +674,9 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.rate = 270000;
 	}
 
-	if (tc->link.base.num_lanes > 2) {
+	if (tc->link.base.lanes > 2) {
 		dev_dbg(tc->dev, "Falling to 2 lanes\n");
-		tc->link.base.num_lanes = 2;
+		tc->link.base.lanes = 2;
 	}
 
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, &reg);
@@ -698,7 +698,7 @@ static int tc_get_display_props(struct tc_data *tc)
 	dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
 		tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
 		(tc->link.base.rate == 162000) ? "1.62Gbps" : "2.7Gbps",
-		tc->link.base.num_lanes,
+		tc->link.base.lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
 	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
@@ -906,7 +906,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	/* Setup Main Link */
 	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN | PHY_M0_EN;
-	if (tc->link.base.num_lanes == 2)
+	if (tc->link.base.lanes == 2)
 		dp_phy_ctrl |= PHY_2LANE;
 
 	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
@@ -1094,7 +1094,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 		ret = -ENODEV;
 	}
 
-	if (tc->link.base.num_lanes == 2) {
+	if (tc->link.base.lanes == 2) {
 		value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS;
 
 		if (value != DP_CHANNEL_EQ_BITS) {
@@ -1291,7 +1291,7 @@ static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
 		return MODE_CLOCK_HIGH;
 
 	req = mode->clock * bits_per_pixel / 8;
-	avail = tc->link.base.num_lanes * tc->link.base.rate;
+	avail = tc->link.base.lanes * tc->link.base.rate;
 
 	if (req > avail)
 		return MODE_BAD;
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f5af71ec1b7d..365de63a02fb 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -342,9 +342,12 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
 		return;
 
 	link->revision = 0;
-	link->rate = 0;
-	link->num_lanes = 0;
+	link->max_rate = 0;
+	link->max_lanes = 0;
 	link->capabilities = 0;
+
+	link->rate = 0;
+	link->lanes = 0;
 }
 
 /**
@@ -370,12 +373,15 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 		return err;
 
 	link->revision = values[0];
-	link->rate = drm_dp_bw_code_to_link_rate(values[1]);
-	link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
+	link->max_rate = drm_dp_bw_code_to_link_rate(values[1]);
+	link->max_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
 
 	if (values[2] & DP_ENHANCED_FRAME_CAP)
 		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
 
+	link->rate = link->max_rate;
+	link->lanes = link->max_lanes;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_link_probe);
@@ -462,7 +468,7 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	int err;
 
 	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
-	values[1] = link->num_lanes;
+	values[1] = link->lanes;
 
 	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 		values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c b/drivers/gpu/drm/msm/edp/edp_ctrl.c
index 7f3dd3ffe2c9..4b31bc30be2c 100644
--- a/drivers/gpu/drm/msm/edp/edp_ctrl.c
+++ b/drivers/gpu/drm/msm/edp/edp_ctrl.c
@@ -403,7 +403,7 @@ static void edp_fill_link_cfg(struct edp_ctrl *ctrl)
 	u32 prate;
 	u32 lrate;
 	u32 bpp;
-	u8 max_lane = ctrl->dp_link.num_lanes;
+	u8 max_lane = ctrl->dp_link.max_lanes;
 	u8 lane;
 
 	prate = ctrl->pixel_rate;
@@ -413,7 +413,7 @@ static void edp_fill_link_cfg(struct edp_ctrl *ctrl)
 	 * By default, use the maximum link rate and minimum lane count,
 	 * so that we can do rate down shift during link training.
 	 */
-	ctrl->link_rate = drm_dp_link_rate_to_bw_code(ctrl->dp_link.rate);
+	ctrl->link_rate = drm_dp_link_rate_to_bw_code(ctrl->dp_link.max_rate);
 
 	prate *= bpp;
 	prate /= 8; /* in kByte */
@@ -701,7 +701,7 @@ static int edp_link_rate_down_shift(struct edp_ctrl *ctrl)
 
 	rate = ctrl->link_rate;
 	lane = ctrl->lane_cnt;
-	max_lane = ctrl->dp_link.num_lanes;
+	max_lane = ctrl->dp_link.max_lanes;
 
 	bpp = ctrl->color_depth * 3;
 	prate = ctrl->pixel_rate;
@@ -759,7 +759,7 @@ static int edp_do_link_train(struct edp_ctrl *ctrl)
 	 * Set the current link rate and lane cnt to panel. They may have been
 	 * adjusted and the values are different from them in DPCD CAP
 	 */
-	dp_link.num_lanes = ctrl->lane_cnt;
+	dp_link.lanes = ctrl->lane_cnt;
 	dp_link.rate = drm_dp_bw_code_to_link_rate(ctrl->link_rate);
 	dp_link.capabilities = ctrl->dp_link.capabilities;
 	if (drm_dp_link_configure(ctrl->drm_aux, &dp_link) < 0)
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index d505ea7d5384..c500be895e64 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -478,7 +478,7 @@ static int cdn_dp_disable(struct cdn_dp_device *dp)
 	cdn_dp_clk_disable(dp);
 	dp->active = false;
 	dp->link.rate = 0;
-	dp->link.num_lanes = 0;
+	dp->link.lanes = 0;
 	if (!dp->connected) {
 		kfree(dp->edid);
 		dp->edid = NULL;
@@ -570,7 +570,7 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
 	struct cdn_dp_port *port = cdn_dp_connected_port(dp);
 	u8 sink_lanes = drm_dp_max_lane_count(dp->dpcd);
 
-	if (!port || !dp->link.rate || !dp->link.num_lanes)
+	if (!port || !dp->link.rate || !dp->link.lanes)
 		return false;
 
 	if (cdn_dp_dpcd_read(dp, DP_LANE0_1_STATUS, link_status,
@@ -953,7 +953,7 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
 	/* Enabled and connected with a sink, re-train if requested */
 	} else if (!cdn_dp_check_link_status(dp)) {
 		unsigned int rate = dp->link.rate;
-		unsigned int lanes = dp->link.num_lanes;
+		unsigned int lanes = dp->link.lanes;
 		struct drm_display_mode *mode = &dp->mode;
 
 		DRM_DEV_INFO(dp->dev, "Connected with sink. Re-train link\n");
@@ -966,7 +966,7 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
 
 		/* If training result is changed, update the video config */
 		if (mode->clock &&
-		    (rate != dp->link.rate || lanes != dp->link.num_lanes)) {
+		    (rate != dp->link.rate || lanes != dp->link.lanes)) {
 			ret = cdn_dp_config_video(dp);
 			if (ret) {
 				dp->connected = false;
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
index 077c87021908..7b254d0d0ba2 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
@@ -536,7 +536,7 @@ static int cdn_dp_get_training_status(struct cdn_dp_device *dp)
 		goto err_get_training_status;
 
 	dp->link.rate = drm_dp_bw_code_to_link_rate(status[0]);
-	dp->link.num_lanes = status[1];
+	dp->link.lanes = status[1];
 
 err_get_training_status:
 	if (ret)
@@ -561,7 +561,7 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
 	}
 
 	DRM_DEV_DEBUG_KMS(dp->dev, "rate:0x%x, lanes:%d\n", dp->link.rate,
-			  dp->link.num_lanes);
+			  dp->link.lanes);
 	return ret;
 }
 
@@ -659,14 +659,13 @@ int cdn_dp_config_video(struct cdn_dp_device *dp)
 	do {
 		tu_size_reg += 2;
 		symbol = tu_size_reg * mode->clock * bit_per_pix;
-		do_div(symbol, dp->link.num_lanes * link_rate * 8);
+		do_div(symbol, dp->link.lanes * link_rate * 8);
 		rem = do_div(symbol, 1000);
 		if (tu_size_reg > 64) {
 			ret = -EINVAL;
 			DRM_DEV_ERROR(dp->dev,
 				      "tu error, clk:%d, lanes:%d, rate:%d\n",
-				      mode->clock, dp->link.num_lanes,
-				      link_rate);
+				      mode->clock, dp->link.lanes, link_rate);
 			goto err_config_video;
 		}
 	} while ((symbol <= 1) || (tu_size_reg - symbol < 4) ||
@@ -680,7 +679,7 @@ int cdn_dp_config_video(struct cdn_dp_device *dp)
 
 	/* set the FIFO Buffer size */
 	val = div_u64(mode->clock * (symbol + 1), 1000) + link_rate;
-	val /= (dp->link.num_lanes * link_rate);
+	val /= (dp->link.lanes * link_rate);
 	val = div_u64(8 * (symbol + 1), bit_per_pix) - val;
 	val += 2;
 	ret = cdn_dp_reg_write(dp, DP_VC_TABLE(15), val);
@@ -833,7 +832,7 @@ static void cdn_dp_audio_config_i2s(struct cdn_dp_device *dp,
 	u32 val;
 
 	if (audio->channels == 2) {
-		if (dp->link.num_lanes == 1)
+		if (dp->link.lanes == 1)
 			sub_pckt_num = 2;
 		else
 			sub_pckt_num = 4;
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index a0f6f9b0d258..ec1688bdff8d 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -792,14 +792,14 @@ int drm_dp_aux_train(struct drm_dp_aux *aux, struct drm_dp_link *link,
 	if (tp == DP_TRAINING_PATTERN_DISABLE)
 		return 0;
 
-	for (i = 0; i < link->num_lanes; i++)
+	for (i = 0; i < link->lanes; i++)
 		values[i] = DP_TRAIN_MAX_PRE_EMPHASIS_REACHED |
 			    DP_TRAIN_PRE_EMPH_LEVEL_0 |
 			    DP_TRAIN_MAX_SWING_REACHED |
 			    DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
 
 	err = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, values,
-				link->num_lanes);
+				link->lanes);
 	if (err < 0)
 		return err;
 
@@ -811,13 +811,13 @@ int drm_dp_aux_train(struct drm_dp_aux *aux, struct drm_dp_link *link,
 
 	switch (tp) {
 	case DP_TRAINING_PATTERN_1:
-		if (!drm_dp_clock_recovery_ok(status, link->num_lanes))
+		if (!drm_dp_clock_recovery_ok(status, link->lanes))
 			return -EAGAIN;
 
 		break;
 
 	case DP_TRAINING_PATTERN_2:
-		if (!drm_dp_channel_eq_ok(status, link->num_lanes))
+		if (!drm_dp_channel_eq_ok(status, link->lanes))
 			return -EAGAIN;
 
 		break;
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index e1669ada0a40..cb94091d98a7 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -649,7 +649,7 @@ static int tegra_sor_dp_train_fast(struct tegra_sor *sor,
 	if (err < 0)
 		return err;
 
-	for (i = 0, value = 0; i < link->num_lanes; i++) {
+	for (i = 0, value = 0; i < link->lanes; i++) {
 		unsigned long lane = SOR_DP_TPG_CHANNEL_CODING |
 				     SOR_DP_TPG_SCRAMBLER_NONE |
 				     SOR_DP_TPG_PATTERN_TRAIN1;
@@ -670,7 +670,7 @@ static int tegra_sor_dp_train_fast(struct tegra_sor *sor,
 	value |= SOR_DP_SPARE_MACRO_SOR_CLK;
 	tegra_sor_writel(sor, value, SOR_DP_SPARE0);
 
-	for (i = 0, value = 0; i < link->num_lanes; i++) {
+	for (i = 0, value = 0; i < link->lanes; i++) {
 		unsigned long lane = SOR_DP_TPG_CHANNEL_CODING |
 				     SOR_DP_TPG_SCRAMBLER_NONE |
 				     SOR_DP_TPG_PATTERN_TRAIN2;
@@ -685,7 +685,7 @@ static int tegra_sor_dp_train_fast(struct tegra_sor *sor,
 	if (err < 0)
 		return err;
 
-	for (i = 0, value = 0; i < link->num_lanes; i++) {
+	for (i = 0, value = 0; i < link->lanes; i++) {
 		unsigned long lane = SOR_DP_TPG_CHANNEL_CODING |
 				     SOR_DP_TPG_SCRAMBLER_GALIOS |
 				     SOR_DP_TPG_PATTERN_NONE;
@@ -912,11 +912,11 @@ static int tegra_sor_compute_config(struct tegra_sor *sor,
 	u32 num_syms_per_line;
 	unsigned int i;
 
-	if (!link_rate || !link->num_lanes || !pclk || !config->bits_per_pixel)
+	if (!link_rate || !link->lanes || !pclk || !config->bits_per_pixel)
 		return -EINVAL;
 
-	output = link_rate * 8 * link->num_lanes;
 	input = pclk * config->bits_per_pixel;
+	output = link_rate * 8 * link->lanes;
 
 	if (input >= output)
 		return -ERANGE;
@@ -959,7 +959,7 @@ static int tegra_sor_compute_config(struct tegra_sor *sor,
 	watermark = div_u64(watermark + params.error, f);
 	config->watermark = watermark + (config->bits_per_pixel / 8) + 2;
 	num_syms_per_line = (mode->hdisplay * config->bits_per_pixel) *
-			    (link->num_lanes * 8);
+			    (link->lanes * 8);
 
 	if (config->watermark > 30) {
 		config->watermark = 30;
@@ -979,12 +979,12 @@ static int tegra_sor_compute_config(struct tegra_sor *sor,
 	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 		config->hblank_symbols -= 3;
 
-	config->hblank_symbols -= 12 / link->num_lanes;
+	config->hblank_symbols -= 12 / link->lanes;
 
 	/* compute the number of symbols per vertical blanking interval */
 	num = (mode->hdisplay - 25) * link_rate;
 	config->vblank_symbols = div_u64(num, pclk);
-	config->vblank_symbols -= 36 / link->num_lanes + 4;
+	config->vblank_symbols -= 36 / link->lanes + 4;
 
 	dev_dbg(sor->dev, "blank symbols: H:%u V:%u\n", config->hblank_symbols,
 		config->vblank_symbols);
@@ -1830,17 +1830,17 @@ static void tegra_sor_edp_enable(struct drm_encoder *encoder)
 	/* power DP lanes */
 	value = tegra_sor_readl(sor, sor->soc->regs->dp_padctl0);
 
-	if (link.num_lanes <= 2)
+	if (link.lanes <= 2)
 		value &= ~(SOR_DP_PADCTL_PD_TXD_3 | SOR_DP_PADCTL_PD_TXD_2);
 	else
 		value |= SOR_DP_PADCTL_PD_TXD_3 | SOR_DP_PADCTL_PD_TXD_2;
 
-	if (link.num_lanes <= 1)
+	if (link.lanes <= 1)
 		value &= ~SOR_DP_PADCTL_PD_TXD_1;
 	else
 		value |= SOR_DP_PADCTL_PD_TXD_1;
 
-	if (link.num_lanes == 0)
+	if (link.lanes == 0)
 		value &= ~SOR_DP_PADCTL_PD_TXD_0;
 	else
 		value |= SOR_DP_PADCTL_PD_TXD_0;
@@ -1849,7 +1849,7 @@ static void tegra_sor_edp_enable(struct drm_encoder *encoder)
 
 	value = tegra_sor_readl(sor, SOR_DP_LINKCTL0);
 	value &= ~SOR_DP_LINKCTL_LANE_COUNT_MASK;
-	value |= SOR_DP_LINKCTL_LANE_COUNT(link.num_lanes);
+	value |= SOR_DP_LINKCTL_LANE_COUNT(link.lanes);
 	tegra_sor_writel(sor, value, SOR_DP_LINKCTL0);
 
 	/* start lane sequencer */
@@ -1906,7 +1906,7 @@ static void tegra_sor_edp_enable(struct drm_encoder *encoder)
 		dev_err(sor->dev, "failed to configure eDP link: %d\n", err);
 
 	rate = drm_dp_link_rate_to_bw_code(link.rate);
-	lanes = link.num_lanes;
+	lanes = link.lanes;
 
 	value = tegra_sor_readl(sor, SOR_CLK_CNTRL);
 	value &= ~SOR_CLK_CNTRL_DP_LINK_SPEED_MASK;
@@ -1924,7 +1924,7 @@ static void tegra_sor_edp_enable(struct drm_encoder *encoder)
 
 	/* disable training pattern generator */
 
-	for (i = 0; i < link.num_lanes; i++) {
+	for (i = 0; i < link.lanes; i++) {
 		unsigned long lane = SOR_DP_TPG_CHANNEL_CODING |
 				     SOR_DP_TPG_SCRAMBLER_GALIOS |
 				     SOR_DP_TPG_PATTERN_NONE;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 935f331e6e72..9c675bde11e8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1359,17 +1359,22 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
 
 /**
- * struct drm_dp_link - DP link capabilities
+ * struct drm_dp_link - DP link capabilities and configuration
  * @revision: DP specification revision supported on the link
- * @rate: maximum clock rate supported on the link
- * @num_lanes: maximum number of lanes supported on the link
+ * @max_rate: maximum clock rate supported on the link
+ * @max_lanes: maximum number of lanes supported on the link
  * @capabilities: bitmask of capabilities supported on the link
+ * @rate: currently configured link rate
+ * @lanes: currently configured number of lanes
  */
 struct drm_dp_link {
 	unsigned char revision;
-	unsigned int rate;
-	unsigned int num_lanes;
+	unsigned int max_rate;
+	unsigned int max_lanes;
 	unsigned long capabilities;
+
+	unsigned int rate;
+	unsigned int lanes;
 };
 
 int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
-- 
2.22.0

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

[PATCH v2 05/21] drm/dp: Turn link capabilities into booleans

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Rather than storing capabilities as flags in an integer, use a separate
boolean per capability. This simplifies the code that checks for these
capabilities.

Cc: Rob Clark <robdclark@gmail.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/bridge/tc358767.c  |  9 ++++-----
 drivers/gpu/drm/drm_dp_helper.c    | 19 ++++++++++++++++---
 drivers/gpu/drm/msm/edp/edp_ctrl.c |  4 ++--
 drivers/gpu/drm/tegra/sor.c        |  4 ++--
 include/drm/drm_dp_helper.h        | 17 ++++++++++++++---
 5 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 733fca7d3829..240a9b69244d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -699,8 +699,8 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
 		(tc->link.base.rate == 162000) ? "1.62Gbps" : "2.7Gbps",
 		tc->link.base.lanes,
-		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
-		"enhanced" : "non-enhanced");
+		tc->link.base.caps.enhanced_framing ? "enhanced" :
+			"non-enhanced");
 	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
 		tc->link.spread ? "0.5%" : "0.0%",
 		tc->link.scrambler_dis ? "disabled" : "enabled");
@@ -1013,8 +1013,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	/* Enable DP0 to start Link Training */
 	ret = regmap_write(tc->regmap, DP0CTL,
-			   ((tc->link.base.capabilities &
-			     DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
+			   (tc->link.base.caps.enhanced_framing ? EF_EN : 0) |
 			   DP_EN);
 	if (ret)
 		return ret;
@@ -1165,7 +1164,7 @@ static int tc_stream_enable(struct tc_data *tc)
 		return ret;
 
 	value = VID_MN_GEN | DP_EN;
-	if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+	if (tc->link.base.caps.enhanced_framing)
 		value |= EF_EN;
 	ret = regmap_write(tc->regmap, DP0CTL, value);
 	if (ret)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 365de63a02fb..bdf999bb6cfa 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -336,6 +336,18 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
 
+static void drm_dp_link_caps_reset(struct drm_dp_link_caps *caps)
+{
+	caps->enhanced_framing = false;
+}
+
+void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
+			   const struct drm_dp_link_caps *src)
+{
+	dest->enhanced_framing = src->enhanced_framing;
+}
+EXPORT_SYMBOL(drm_dp_link_caps_copy);
+
 static void drm_dp_link_reset(struct drm_dp_link *link)
 {
 	if (!link)
@@ -344,7 +356,8 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
 	link->revision = 0;
 	link->max_rate = 0;
 	link->max_lanes = 0;
-	link->capabilities = 0;
+
+	drm_dp_link_caps_reset(&link->caps);
 
 	link->rate = 0;
 	link->lanes = 0;
@@ -377,7 +390,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->max_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
 
 	if (values[2] & DP_ENHANCED_FRAME_CAP)
-		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
+		link->caps.enhanced_framing = true;
 
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
@@ -470,7 +483,7 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
 	values[1] = link->lanes;
 
-	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+	if (link->caps.enhanced_framing)
 		values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 
 	err = drm_dp_dpcd_write(aux, DP_LINK_BW_SET, values, sizeof(values));
diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c b/drivers/gpu/drm/msm/edp/edp_ctrl.c
index 4b31bc30be2c..0a6c77e791aa 100644
--- a/drivers/gpu/drm/msm/edp/edp_ctrl.c
+++ b/drivers/gpu/drm/msm/edp/edp_ctrl.c
@@ -439,7 +439,7 @@ static void edp_config_ctrl(struct edp_ctrl *ctrl)
 
 	data = EDP_CONFIGURATION_CTRL_LANES(ctrl->lane_cnt - 1);
 
-	if (ctrl->dp_link.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+	if (ctrl->dp_link.caps.enhanced_framing)
 		data |= EDP_CONFIGURATION_CTRL_ENHANCED_FRAMING;
 
 	depth = EDP_6BIT;
@@ -761,7 +761,7 @@ static int edp_do_link_train(struct edp_ctrl *ctrl)
 	 */
 	dp_link.lanes = ctrl->lane_cnt;
 	dp_link.rate = drm_dp_bw_code_to_link_rate(ctrl->link_rate);
-	dp_link.capabilities = ctrl->dp_link.capabilities;
+	drm_dp_link_caps_copy(&dp_link.caps, &ctrl->dp_link.caps);
 	if (drm_dp_link_configure(ctrl->drm_aux, &dp_link) < 0)
 		return EDP_TRAIN_FAIL;
 
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index cb94091d98a7..0b033b964ac0 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -976,7 +976,7 @@ static int tegra_sor_compute_config(struct tegra_sor *sor,
 	num = ((mode->htotal - mode->hdisplay) - 7) * link_rate;
 	config->hblank_symbols = div_u64(num, pclk);
 
-	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+	if (link->caps.enhanced_framing)
 		config->hblank_symbols -= 3;
 
 	config->hblank_symbols -= 12 / link->lanes;
@@ -1917,7 +1917,7 @@ static void tegra_sor_edp_enable(struct drm_encoder *encoder)
 	value &= ~SOR_DP_LINKCTL_LANE_COUNT_MASK;
 	value |= SOR_DP_LINKCTL_LANE_COUNT(lanes);
 
-	if (link.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+	if (link.caps.enhanced_framing)
 		value |= SOR_DP_LINKCTL_ENHANCED_FRAME;
 
 	tegra_sor_writel(sor, value, SOR_DP_LINKCTL0);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 9c675bde11e8..2759f8d7e90d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1356,14 +1356,24 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 /*
  * DisplayPort link
  */
-#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
+
+/**
+ * struct drm_dp_link_caps - DP link capabilities
+ * @enhanced_framing: enhanced framing capability (mandatory as of DP 1.2)
+ */
+struct drm_dp_link_caps {
+	bool enhanced_framing;
+};
+
+void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
+			   const struct drm_dp_link_caps *src);
 
 /**
  * struct drm_dp_link - DP link capabilities and configuration
  * @revision: DP specification revision supported on the link
  * @max_rate: maximum clock rate supported on the link
  * @max_lanes: maximum number of lanes supported on the link
- * @capabilities: bitmask of capabilities supported on the link
+ * @caps: capabilities supported on the link (see &drm_dp_link_caps)
  * @rate: currently configured link rate
  * @lanes: currently configured number of lanes
  */
@@ -1371,7 +1381,8 @@ struct drm_dp_link {
 	unsigned char revision;
 	unsigned int max_rate;
 	unsigned int max_lanes;
-	unsigned long capabilities;
+
+	struct drm_dp_link_caps caps;
 
 	unsigned int rate;
 	unsigned int lanes;
-- 
2.22.0

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

[PATCH v2 06/21] drm/dp: Probe link using existing parsing helpers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use existing parsing helpers to probe a DisplayPort link.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 29 ++++++++++++++++++++++-------
 include/drm/drm_dp_helper.h     |  2 ++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index bdf999bb6cfa..cdf49e8d5e3a 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -320,6 +320,22 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write);
 
+/**
+ * drm_dp_dpcd_read_link_caps() - read DPCD link capabilities
+ * @aux: DisplayPort AUX channel
+ * @caps: buffer to store the link capabilities in
+ *
+ * Returns:
+ * The number of bytes transferred on success or a negative error code on
+ * failure.
+ */
+int drm_dp_dpcd_read_link_caps(struct drm_dp_aux *aux,
+			       u8 caps[DP_RECEIVER_CAP_SIZE])
+{
+	return drm_dp_dpcd_read(aux, DP_DPCD_REV, caps, DP_RECEIVER_CAP_SIZE);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_read_link_caps);
+
 /**
  * drm_dp_dpcd_read_link_status() - read DPCD link status (bytes 0x202-0x207)
  * @aux: DisplayPort AUX channel
@@ -376,21 +392,20 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
  */
 int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 {
-	u8 values[3];
+	u8 values[DP_RECEIVER_CAP_SIZE];
 	int err;
 
 	drm_dp_link_reset(link);
 
-	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
+	err = drm_dp_dpcd_read_link_caps(aux, values);
 	if (err < 0)
 		return err;
 
-	link->revision = values[0];
-	link->max_rate = drm_dp_bw_code_to_link_rate(values[1]);
-	link->max_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
+	link->revision = values[DP_DPCD_REV];
+	link->max_rate = drm_dp_max_link_rate(values);
+	link->max_lanes = drm_dp_max_lane_count(values);
 
-	if (values[2] & DP_ENHANCED_FRAME_CAP)
-		link->caps.enhanced_framing = true;
+	link->caps.enhanced_framing = drm_dp_enhanced_frame_cap(values);
 
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 2759f8d7e90d..60bb030c858d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1350,6 +1350,8 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
 	return drm_dp_dpcd_write(aux, offset, &value, 1);
 }
 
+int drm_dp_dpcd_read_link_caps(struct drm_dp_aux *aux,
+			       u8 caps[DP_RECEIVER_CAP_SIZE]);
 int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 				 u8 status[DP_LINK_STATUS_SIZE]);
 
-- 
2.22.0

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

[PATCH v2 07/21] drm/dp: Read fast training capability from link

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

While probing the DisplayPort link, query the fast training capability.
If supported, drivers can use the fast link training sequence instead of
the more involved full link training sequence.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 3 +++
 include/drm/drm_dp_helper.h     | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index cdf49e8d5e3a..74e4fceace7e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -355,12 +355,14 @@ EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
 static void drm_dp_link_caps_reset(struct drm_dp_link_caps *caps)
 {
 	caps->enhanced_framing = false;
+	caps->fast_training = false;
 }
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
 			   const struct drm_dp_link_caps *src)
 {
 	dest->enhanced_framing = src->enhanced_framing;
+	dest->fast_training = src->fast_training;
 }
 EXPORT_SYMBOL(drm_dp_link_caps_copy);
 
@@ -406,6 +408,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->max_lanes = drm_dp_max_lane_count(values);
 
 	link->caps.enhanced_framing = drm_dp_enhanced_frame_cap(values);
+	link->caps.fast_training = drm_dp_fast_training_cap(values);
 
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 60bb030c858d..c148f5685195 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1143,6 +1143,13 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 		(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
 }
 
+static inline bool
+drm_dp_fast_training_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	return dpcd[DP_DPCD_REV] >= 0x11 &&
+		(dpcd[DP_MAX_DOWNSPREAD] & DP_NO_AUX_HANDSHAKE_LINK_TRAINING);
+}
+
 static inline bool
 drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
@@ -1362,9 +1369,11 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 /**
  * struct drm_dp_link_caps - DP link capabilities
  * @enhanced_framing: enhanced framing capability (mandatory as of DP 1.2)
+ * @fast_training: AUX CH handshake not required for link training
  */
 struct drm_dp_link_caps {
 	bool enhanced_framing;
+	bool fast_training;
 };
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
-- 
2.22.0

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

[PATCH v2 08/21] drm/dp: Read TPS3 capability from sink

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The TPS3 capability can be exposed by DP 1.2 and later sinks if they
support the alternative training pattern for channel equalization.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 3 +++
 include/drm/drm_dp_helper.h     | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 74e4fceace7e..c47d78973c1e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -355,6 +355,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
 static void drm_dp_link_caps_reset(struct drm_dp_link_caps *caps)
 {
 	caps->enhanced_framing = false;
+	caps->tps3_supported = false;
 	caps->fast_training = false;
 }
 
@@ -362,6 +363,7 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
 			   const struct drm_dp_link_caps *src)
 {
 	dest->enhanced_framing = src->enhanced_framing;
+	dest->tps3_supported = src->tps3_supported;
 	dest->fast_training = src->fast_training;
 }
 EXPORT_SYMBOL(drm_dp_link_caps_copy);
@@ -408,6 +410,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->max_lanes = drm_dp_max_lane_count(values);
 
 	link->caps.enhanced_framing = drm_dp_enhanced_frame_cap(values);
+	link->caps.tps3_supported = drm_dp_tps3_supported(values);
 	link->caps.fast_training = drm_dp_fast_training_cap(values);
 
 	link->rate = link->max_rate;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c148f5685195..ab98ebb302a9 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1369,10 +1369,12 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 /**
  * struct drm_dp_link_caps - DP link capabilities
  * @enhanced_framing: enhanced framing capability (mandatory as of DP 1.2)
+ * @tps3_supported: training pattern sequence 3 supported for equalization
  * @fast_training: AUX CH handshake not required for link training
  */
 struct drm_dp_link_caps {
 	bool enhanced_framing;
+	bool tps3_supported;
 	bool fast_training;
 };
 
-- 
2.22.0

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

[PATCH v2 09/21] drm/dp: Read channel coding capability from sink

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Parse from the sink capabilities whether or not it supports ANSI 8B/10B
channel coding as specified in ANSI X3.230-1994, clause 11.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 3 +++
 include/drm/drm_dp_helper.h     | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index c47d78973c1e..1c238196c8b4 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -357,6 +357,7 @@ static void drm_dp_link_caps_reset(struct drm_dp_link_caps *caps)
 	caps->enhanced_framing = false;
 	caps->tps3_supported = false;
 	caps->fast_training = false;
+	caps->channel_coding = false;
 }
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
@@ -365,6 +366,7 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
 	dest->enhanced_framing = src->enhanced_framing;
 	dest->tps3_supported = src->tps3_supported;
 	dest->fast_training = src->fast_training;
+	dest->channel_coding = src->channel_coding;
 }
 EXPORT_SYMBOL(drm_dp_link_caps_copy);
 
@@ -412,6 +414,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->caps.enhanced_framing = drm_dp_enhanced_frame_cap(values);
 	link->caps.tps3_supported = drm_dp_tps3_supported(values);
 	link->caps.fast_training = drm_dp_fast_training_cap(values);
+	link->caps.channel_coding = drm_dp_channel_coding_supported(values);
 
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index ab98ebb302a9..d144d3a54dbc 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -95,6 +95,7 @@
 # define DP_DETAILED_CAP_INFO_AVAILABLE	    (1 << 4) /* DPI */
 
 #define DP_MAIN_LINK_CHANNEL_CODING         0x006
+# define DP_CAP_ANSI_8B10B		    (1 << 0)
 
 #define DP_DOWN_STREAM_PORT_COUNT	    0x007
 # define DP_PORT_COUNT_MASK		    0x0f
@@ -1215,6 +1216,12 @@ drm_dp_sink_supports_fec(const u8 fec_capable)
 	return fec_capable & DP_FEC_CAPABLE;
 }
 
+static inline bool
+drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
+}
+
 /*
  * DisplayPort AUX channel
  */
@@ -1371,11 +1378,13 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
  * @enhanced_framing: enhanced framing capability (mandatory as of DP 1.2)
  * @tps3_supported: training pattern sequence 3 supported for equalization
  * @fast_training: AUX CH handshake not required for link training
+ * @channel_coding: ANSI 8B/10B channel coding capability
  */
 struct drm_dp_link_caps {
 	bool enhanced_framing;
 	bool tps3_supported;
 	bool fast_training;
+	bool channel_coding;
 };
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
-- 
2.22.0

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

[PATCH v2 10/21] drm/dp: Read alternate scrambler reset capability from sink

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Parse from the sink capabilities whether or not the eDP alternate
scrambler reset value of 0xfffe is supported.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 include/drm/drm_dp_helper.h     | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 1c238196c8b4..acab8dc48e2c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -358,6 +358,7 @@ static void drm_dp_link_caps_reset(struct drm_dp_link_caps *caps)
 	caps->tps3_supported = false;
 	caps->fast_training = false;
 	caps->channel_coding = false;
+	caps->alternate_scrambler_reset = false;
 }
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
@@ -367,6 +368,7 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
 	dest->tps3_supported = src->tps3_supported;
 	dest->fast_training = src->fast_training;
 	dest->channel_coding = src->channel_coding;
+	dest->alternate_scrambler_reset = src->alternate_scrambler_reset;
 }
 EXPORT_SYMBOL(drm_dp_link_caps_copy);
 
@@ -416,6 +418,9 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->caps.fast_training = drm_dp_fast_training_cap(values);
 	link->caps.channel_coding = drm_dp_channel_coding_supported(values);
 
+	if (drm_dp_alternate_scrambler_reset_cap(values))
+		link->caps.alternate_scrambler_reset = true;
+
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index d144d3a54dbc..f9f65bc13df5 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1222,6 +1222,13 @@ drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
 }
 
+static inline bool
+drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	return dpcd[DP_EDP_CONFIGURATION_CAP] &
+			DP_ALTERNATE_SCRAMBLER_RESET_CAP;
+}
+
 /*
  * DisplayPort AUX channel
  */
@@ -1379,12 +1386,14 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
  * @tps3_supported: training pattern sequence 3 supported for equalization
  * @fast_training: AUX CH handshake not required for link training
  * @channel_coding: ANSI 8B/10B channel coding capability
+ * @alternate_scrambler_reset: eDP alternate scrambler reset capability
  */
 struct drm_dp_link_caps {
 	bool enhanced_framing;
 	bool tps3_supported;
 	bool fast_training;
 	bool channel_coding;
+	bool alternate_scrambler_reset;
 };
 
 void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
-- 
2.22.0

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

[PATCH v2 11/21] drm/dp: Read eDP version from DPCD

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If the sink supports eDP, read the eDP revision from it's DPCD.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 16 +++++++++++++++-
 include/drm/drm_dp_helper.h     |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index acab8dc48e2c..5b36e8e39ca7 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -382,6 +382,7 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
 	link->max_lanes = 0;
 
 	drm_dp_link_caps_reset(&link->caps);
+	link->edp = 0;
 
 	link->rate = 0;
 	link->lanes = 0;
@@ -418,9 +419,22 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->caps.fast_training = drm_dp_fast_training_cap(values);
 	link->caps.channel_coding = drm_dp_channel_coding_supported(values);
 
-	if (drm_dp_alternate_scrambler_reset_cap(values))
+	if (drm_dp_alternate_scrambler_reset_cap(values)) {
+		static const u8 edp_revs[] = { 0x11, 0x12, 0x13, 0x14 };
+		u8 value;
+
 		link->caps.alternate_scrambler_reset = true;
 
+		err = drm_dp_dpcd_readb(aux, DP_EDP_DPCD_REV, &value);
+		if (err < 0)
+			return err;
+
+		if (value >= ARRAY_SIZE(edp_revs))
+			DRM_ERROR("unsupported eDP version: %02x\n", value);
+		else
+			link->edp = edp_revs[value];
+	}
+
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index f9f65bc13df5..13c50e905205 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1405,6 +1405,7 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
  * @max_rate: maximum clock rate supported on the link
  * @max_lanes: maximum number of lanes supported on the link
  * @caps: capabilities supported on the link (see &drm_dp_link_caps)
+ * @edp: eDP revision (0x11: eDP 1.1, 0x12: eDP 1.2, ...)
  * @rate: currently configured link rate
  * @lanes: currently configured number of lanes
  */
@@ -1414,6 +1415,7 @@ struct drm_dp_link {
 	unsigned int max_lanes;
 
 	struct drm_dp_link_caps caps;
+	unsigned char edp;
 
 	unsigned int rate;
 	unsigned int lanes;
-- 
2.22.0

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

[PATCH v2 12/21] drm/dp: Read AUX read interval from DPCD

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Store the AUX read interval from DPCD, so that it can be used to wait
for the durations given in the specification during link training.

v2: use USEC_PER_MSEC instead of MSEC_PER_SEC for clarity

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c |  3 +++
 include/drm/drm_dp_helper.h     | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 5b36e8e39ca7..4112570dbe67 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -382,6 +382,7 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
 	link->max_lanes = 0;
 
 	drm_dp_link_caps_reset(&link->caps);
+	link->aux_rd_interval = 0;
 	link->edp = 0;
 
 	link->rate = 0;
@@ -435,6 +436,8 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 			link->edp = edp_revs[value];
 	}
 
+	link->aux_rd_interval = drm_dp_aux_rd_interval(values);
+
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 13c50e905205..e28b0941a8be 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -25,8 +25,11 @@
 
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/time64.h>
 #include <linux/types.h>
 
+#include <drm/drm_print.h>
+
 /*
  * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
  * DP and DPCD versions are independent.  Differences from 1.0 are not noted,
@@ -1229,6 +1232,36 @@ drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 			DP_ALTERNATE_SCRAMBLER_RESET_CAP;
 }
 
+/**
+ * drm_dp_read_aux_interval() - read the AUX read interval from the DPCD
+ * @dpcd: receiver capacity buffer
+ *
+ * Reads the AUX read interval (in microseconds) from the DPCD. Note that the
+ * TRAINING_AUX_RD_INTERVAL stores the value in units of 4 milliseconds. If no
+ * read interval is specified and for DPCD v1.4 and later, the read interval
+ * is always 100 microseconds.
+ *
+ * Returns:
+ * The read AUX interval in microseconds.
+ */
+static inline unsigned int
+drm_dp_aux_rd_interval(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	unsigned int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
+					DP_TRAINING_AUX_RD_MASK;
+
+	if (rd_interval > 4)
+		DRM_DEBUG_KMS("AUX interval %u, out of range (max: 4)\n",
+			      rd_interval);
+
+	if (rd_interval > 0 && dpcd[DP_DPCD_REV] < DP_DPCD_REV_14)
+		rd_interval *= 4 * USEC_PER_MSEC;
+	else
+		rd_interval = 100;
+
+	return rd_interval;
+}
+
 /*
  * DisplayPort AUX channel
  */
@@ -1405,6 +1438,7 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
  * @max_rate: maximum clock rate supported on the link
  * @max_lanes: maximum number of lanes supported on the link
  * @caps: capabilities supported on the link (see &drm_dp_link_caps)
+ * @aux_rd_interval: AUX read interval to use for training (in microseconds)
  * @edp: eDP revision (0x11: eDP 1.1, 0x12: eDP 1.2, ...)
  * @rate: currently configured link rate
  * @lanes: currently configured number of lanes
@@ -1415,6 +1449,7 @@ struct drm_dp_link {
 	unsigned int max_lanes;
 
 	struct drm_dp_link_caps caps;
+	unsigned int aux_rd_interval;
 	unsigned char edp;
 
 	unsigned int rate;
-- 
2.22.0

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

[PATCH v2 13/21] drm/dp: Do not busy-loop during link training

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use microsecond sleeps for the clock recovery and channel equalization
delays during link training. The duration of these delays can be from
100 us up to 16 ms. It is rude to busy-loop for that amount of time.

While at it, also convert to standard coding style by putting the
opening braces in a function definition on a new line.

v2: use correct multiplier for training delays (Philipp Zabel)

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4112570dbe67..681d28988776 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -120,33 +120,39 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
 }
 EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis);
 
-void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
-	int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
-			  DP_TRAINING_AUX_RD_MASK;
+void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	unsigned int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
+					DP_TRAINING_AUX_RD_MASK;
 
 	if (rd_interval > 4)
-		DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)\n",
+		DRM_DEBUG_KMS("AUX interval %u, out of range (max 4)\n",
 			      rd_interval);
 
 	if (rd_interval == 0 || dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
-		udelay(100);
+		rd_interval = 100;
 	else
-		mdelay(rd_interval * 4);
+		rd_interval *= 4 * USEC_PER_MSEC;
+
+	usleep_range(rd_interval, rd_interval * 2);
 }
 EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay);
 
-void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
-	int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
-			  DP_TRAINING_AUX_RD_MASK;
+void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	unsigned int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
+					DP_TRAINING_AUX_RD_MASK;
 
 	if (rd_interval > 4)
-		DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)\n",
+		DRM_DEBUG_KMS("AUX interval %u, out of range (max 4)\n",
 			      rd_interval);
 
 	if (rd_interval == 0)
-		udelay(400);
+		rd_interval = 400;
 	else
-		mdelay(rd_interval * 4);
+		rd_interval *= 4 * USEC_PER_MSEC;
+
+	usleep_range(rd_interval, rd_interval * 2);
 }
 EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
 
-- 
2.22.0

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

[PATCH v2 14/21] drm/dp: Use drm_dp_aux_rd_interval()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Make use of the newly added drm_dp_aux_rd_interval() helper in existing
DP link training helpers.

v2: drop stale sentence from commit message (Philipp Zabel)

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 681d28988776..1fb3c27cd012 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -122,17 +122,7 @@ EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis);
 
 void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
-	unsigned int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
-					DP_TRAINING_AUX_RD_MASK;
-
-	if (rd_interval > 4)
-		DRM_DEBUG_KMS("AUX interval %u, out of range (max 4)\n",
-			      rd_interval);
-
-	if (rd_interval == 0 || dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
-		rd_interval = 100;
-	else
-		rd_interval *= 4 * USEC_PER_MSEC;
+	unsigned int rd_interval = drm_dp_aux_rd_interval(dpcd);
 
 	usleep_range(rd_interval, rd_interval * 2);
 }
@@ -140,19 +130,9 @@ EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay);
 
 void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
-	unsigned int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
-					DP_TRAINING_AUX_RD_MASK;
-
-	if (rd_interval > 4)
-		DRM_DEBUG_KMS("AUX interval %u, out of range (max 4)\n",
-			      rd_interval);
+	unsigned int min = drm_dp_aux_rd_interval(dpcd);
 
-	if (rd_interval == 0)
-		rd_interval = 400;
-	else
-		rd_interval *= 4 * USEC_PER_MSEC;
-
-	usleep_range(rd_interval, rd_interval * 2);
+	usleep_range(min, min * 2);
 }
 EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
 
-- 
2.22.0

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

[PATCH v2 15/21] drm/dp: Add helper to get post-cursor adjustments

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If the transmitter supports pre-emphasis post cursor2 the sink will
request adjustments in a similar way to how it requests adjustments to
the voltage swing and pre-emphasis settings.

Add a helper to extract these adjustments on a per-lane basis from the
DPCD link status.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 10 ++++++++++
 include/drm/drm_dp_helper.h     | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 1fb3c27cd012..f1f3705fade9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -120,6 +120,16 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
 }
 EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis);
 
+u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
+					 unsigned int lane)
+{
+	unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
+	u8 value = dp_link_status(link_status, offset);
+
+	return (value >> (lane << 1)) & 0x3;
+}
+EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
+
 void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
 	unsigned int rd_interval = drm_dp_aux_rd_interval(dpcd);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index e28b0941a8be..5d262a453878 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -566,6 +566,14 @@
 # define DP_ADJUST_PRE_EMPHASIS_LANE1_SHIFT  6
 
 #define DP_ADJUST_REQUEST_POST_CURSOR2      0x20c
+# define DP_ADJUST_POST_CURSOR2_LANE0_MASK  0x03
+# define DP_ADJUST_POST_CURSOR2_LANE0_SHIFT 0
+# define DP_ADJUST_POST_CURSOR2_LANE1_MASK  0x0c
+# define DP_ADJUST_POST_CURSOR2_LANE1_SHIFT 2
+# define DP_ADJUST_POST_CURSOR2_LANE2_MASK  0x30
+# define DP_ADJUST_POST_CURSOR2_LANE2_SHIFT 4
+# define DP_ADJUST_POST_CURSOR2_LANE3_MASK  0xc0
+# define DP_ADJUST_POST_CURSOR2_LANE3_SHIFT 6
 
 #define DP_TEST_REQUEST			    0x218
 # define DP_TEST_LINK_TRAINING		    (1 << 0)
@@ -1053,6 +1061,8 @@ u8 drm_dp_get_adjust_request_voltage(const u8 link_status[DP_LINK_STATUS_SIZE],
 				     int lane);
 u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SIZE],
 					  int lane);
+u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
+					 unsigned int lane);
 
 #define DP_BRANCH_OUI_HEADER_SIZE	0xc
 #define DP_RECEIVER_CAP_SIZE		0xf
-- 
2.22.0

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

[PATCH v2 16/21] drm/dp: Set channel coding on link configuration

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Make use of ANSI 8B/10B channel coding if the DisplayPort sink supports
it.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f1f3705fade9..f51a5595ebc0 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -519,7 +519,7 @@ EXPORT_SYMBOL(drm_dp_link_power_down);
  */
 int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 {
-	u8 values[2];
+	u8 values[2], value = 0;
 	int err;
 
 	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
@@ -532,6 +532,13 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	if (err < 0)
 		return err;
 
+	if (link->caps.channel_coding)
+		value = DP_SET_ANSI_8B10B;
+
+	err = drm_dp_dpcd_writeb(aux, DP_MAIN_LINK_CHANNEL_CODING_SET, value);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_link_configure);
-- 
2.22.0

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

[PATCH v2 17/21] drm/dp: Enable alternate scrambler reset when supported

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If the sink is eDP and supports the alternate scrambler reset, enable
it.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f51a5595ebc0..85956f3a24e3 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -539,6 +539,13 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	if (err < 0)
 		return err;
 
+	if (link->caps.alternate_scrambler_reset) {
+		err = drm_dp_dpcd_writeb(aux, DP_EDP_CONFIGURATION_SET,
+					 DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+		if (err < 0)
+			return err;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_link_configure);
-- 
2.22.0

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

[PATCH v2 18/21] drm/dp: Add drm_dp_link_choose() helper

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This helper chooses an appropriate configuration, according to the
bitrate requirements of the video mode and the capabilities of the
DisplayPort sink.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 55 +++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  4 +++
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 85956f3a24e3..01d34f33c658 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -550,6 +550,61 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 }
 EXPORT_SYMBOL(drm_dp_link_configure);
 
+/**
+ * drm_dp_link_choose() - choose the lowest possible configuration for a mode
+ * @link: DRM DP link object
+ * @mode: DRM display mode
+ * @info: DRM display information
+ *
+ * According to the eDP specification, a source should select a configuration
+ * with the lowest number of lanes and the lowest possible link rate that can
+ * match the bitrate requirements of a video mode. However it must ensure not
+ * to exceed the capabilities of the sink.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_choose(struct drm_dp_link *link,
+		       const struct drm_display_mode *mode,
+		       const struct drm_display_info *info)
+{
+	/* available link symbol clock rates */
+	static const unsigned int rates[3] = { 162000, 270000, 540000 };
+	/* available number of lanes */
+	static const unsigned int lanes[3] = { 1, 2, 4 };
+	unsigned long requirement, capacity;
+	unsigned int rate = link->max_rate;
+	unsigned int i, j;
+
+	/* bandwidth requirement */
+	requirement = mode->clock * info->bpc * 3;
+
+	for (i = 0; i < ARRAY_SIZE(lanes) && lanes[i] <= link->max_lanes; i++) {
+		for (j = 0; j < ARRAY_SIZE(rates) && rates[j] <= rate; j++) {
+			/*
+			 * Capacity for this combination of lanes and rate,
+			 * factoring in the ANSI 8B/10B encoding.
+			 *
+			 * Link rates in the DRM DP helpers are really link
+			 * symbol frequencies, so a tenth of the actual rate
+			 * of the link.
+			 */
+			capacity = lanes[i] * (rates[j] * 10) * 8 / 10;
+
+			if (capacity >= requirement) {
+				DRM_DEBUG_KMS("using %u lanes at %u kHz (%lu/%lu kbps)\n",
+					      lanes[i], rates[j], requirement,
+					      capacity);
+				link->lanes = lanes[i];
+				link->rate = rates[j];
+				return 0;
+			}
+		}
+	}
+
+	return -ERANGE;
+}
+EXPORT_SYMBOL(drm_dp_link_choose);
+
 /**
  * drm_dp_downstream_max_clock() - extract branch device max
  *                                 pixel rate for legacy VGA
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 5d262a453878..817231e38053 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -28,6 +28,7 @@
 #include <linux/time64.h>
 #include <linux/types.h>
 
+#include <drm/drm_crtc.h>
 #include <drm/drm_print.h>
 
 /*
@@ -1470,6 +1471,9 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
+int drm_dp_link_choose(struct drm_dp_link *link,
+		       const struct drm_display_mode *mode,
+		       const struct drm_display_info *info);
 int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 				const u8 port_cap[4]);
 int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
-- 
2.22.0

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

[PATCH v2 19/21] drm/dp: Add support for eDP link rates

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 121 ++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |   9 +++
 2 files changed, 130 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 01d34f33c658..136ee609f2ee 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -385,6 +385,107 @@ static void drm_dp_link_reset(struct drm_dp_link *link)
 	link->lanes = 0;
 }
 
+/**
+ * drm_dp_link_add_rate() - add a rate to the list of supported rates
+ * @link: the link to add the rate to
+ * @rate: the rate to add
+ *
+ * Add a link rate to the list of supported link rates.
+ *
+ * Returns:
+ * 0 on success or one of the following negative error codes on failure:
+ * - ENOSPC if the maximum number of supported rates has been reached
+ * - EEXISTS if the link already supports this rate
+ *
+ * See also:
+ * drm_dp_link_remove_rate()
+ */
+int drm_dp_link_add_rate(struct drm_dp_link *link, unsigned long rate)
+{
+	unsigned int i, pivot;
+
+	if (link->num_rates == DP_MAX_SUPPORTED_RATES)
+		return -ENOSPC;
+
+	for (pivot = 0; pivot < link->num_rates; pivot++)
+		if (rate <= link->rates[pivot])
+			break;
+
+	if (pivot != link->num_rates && rate == link->rates[pivot])
+		return -EEXIST;
+
+	for (i = link->num_rates; i > pivot; i--)
+		link->rates[i] = link->rates[i - 1];
+
+	link->rates[pivot] = rate;
+	link->num_rates++;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_link_add_rate);
+
+/**
+ * drm_dp_link_remove_rate() - remove a rate from the list of supported rates
+ * @link: the link from which to remove the rate
+ * @rate: the rate to remove
+ *
+ * Removes a link rate from the list of supported link rates.
+ *
+ * Returns:
+ * 0 on success or one of the following negative error codes on failure:
+ * - EINVAL if the specified rate is not among the supported rates
+ *
+ * See also:
+ * drm_dp_link_add_rate()
+ */
+int drm_dp_link_remove_rate(struct drm_dp_link *link, unsigned long rate)
+{
+	unsigned int i;
+
+	for (i = 0; i < link->num_rates; i++)
+		if (rate == link->rates[i])
+			break;
+
+	if (i == link->num_rates)
+		return -EINVAL;
+
+	link->num_rates--;
+
+	for (i = i; i < link->num_rates; i++)
+		link->rates[i] = link->rates[i + 1];
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_link_remove_rate);
+
+/**
+ * drm_dp_link_update_rates() - normalize the supported link rates array
+ * @link: the link for which to normalize the supported link rates
+ *
+ * Users should call this function after they've manually modified the array
+ * of supported link rates. This function removes any stale entries, compacts
+ * the array and updates the supported link rate count. Note that calling the
+ * drm_dp_link_remove_rate() function already does this janitorial work.
+ *
+ * See also:
+ * drm_dp_link_add_rate(), drm_dp_link_remove_rate()
+ */
+void drm_dp_link_update_rates(struct drm_dp_link *link)
+{
+	unsigned int i, count = 0;
+
+	for (i = 0; i < link->num_rates; i++) {
+		if (link->rates[i] != 0)
+			link->rates[count++] = link->rates[i];
+	}
+
+	for (i = count; i < link->num_rates; i++)
+		link->rates[i] = 0;
+
+	link->num_rates = count;
+}
+EXPORT_SYMBOL(drm_dp_link_update_rates);
+
 /**
  * drm_dp_link_probe() - probe a DisplayPort link for capabilities
  * @aux: DisplayPort AUX channel
@@ -437,6 +538,26 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
 	link->rate = link->max_rate;
 	link->lanes = link->max_lanes;
 
+	/* Parse SUPPORTED_LINK_RATES from eDP 1.4 */
+	if (link->edp >= 0x14) {
+		u8 supported_rates[DP_MAX_SUPPORTED_RATES * 2];
+		unsigned int i;
+		u16 rate;
+
+		err = drm_dp_dpcd_read(aux, DP_SUPPORTED_LINK_RATES,
+				       supported_rates,
+				       sizeof(supported_rates));
+		if (err < 0)
+			return err;
+
+		for (i = 0; i < DP_MAX_SUPPORTED_RATES; i++) {
+			rate = supported_rates[i * 2 + 1] << 8 |
+			       supported_rates[i * 2 + 0];
+
+			drm_dp_link_add_rate(link, rate * 200);
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_link_probe);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 817231e38053..dfe16069e029 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1453,6 +1453,8 @@ void drm_dp_link_caps_copy(struct drm_dp_link_caps *dest,
  * @edp: eDP revision (0x11: eDP 1.1, 0x12: eDP 1.2, ...)
  * @rate: currently configured link rate
  * @lanes: currently configured number of lanes
+ * @rates: additional supported link rates in kHz (eDP 1.4)
+ * @num_rates: number of additional supported link rates (eDP 1.4)
  */
 struct drm_dp_link {
 	unsigned char revision;
@@ -1465,8 +1467,15 @@ struct drm_dp_link {
 
 	unsigned int rate;
 	unsigned int lanes;
+
+	unsigned long rates[DP_MAX_SUPPORTED_RATES];
+	unsigned int num_rates;
 };
 
+int drm_dp_link_add_rate(struct drm_dp_link *link, unsigned long rate);
+int drm_dp_link_remove_rate(struct drm_dp_link *link, unsigned long rate);
+void drm_dp_link_update_rates(struct drm_dp_link *link);
+
 int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
-- 
2.22.0

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

[PATCH v2 20/21] drm/dp: Remove a gratuituous blank line

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

It's idiomatic to check the return value of a function call immediately
after the function call, without any blank lines in between, to make it
more obvious that the two lines belong together.

v2: fix outdated commit message (Philipp Zabel)

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 136ee609f2ee..6b4431bade3e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -216,7 +216,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 		}
 
 		ret = aux->transfer(aux, &msg);
-
 		if (ret >= 0) {
 			native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
 			if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
-- 
2.22.0

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

[PATCH v2 21/21] drm/bridge: tc358767: Use DP nomenclature

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The DP specification uses the term "default framing" instead of "non-
enhanced framing".

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 240a9b69244d..468925f80329 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -699,8 +699,7 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
 		(tc->link.base.rate == 162000) ? "1.62Gbps" : "2.7Gbps",
 		tc->link.base.lanes,
-		tc->link.base.caps.enhanced_framing ? "enhanced" :
-			"non-enhanced");
+		tc->link.base.caps.enhanced_framing ? "enhanced" : "default");
 	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
 		tc->link.spread ? "0.5%" : "0.0%",
 		tc->link.scrambler_dis ? "disabled" : "enabled");
-- 
2.22.0

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

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2001 bytes --]

On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> this series of patches improves the DP helpers a bit and cleans up some
> inconsistencies along the way.
> 
> v2 incorporates all review comments add collects Reviewed-bys from v1.
> 
> Thierry
> 
> Thierry Reding (21):
>   drm/dp: Sort includes alphabetically
>   drm/dp: Add missing kerneldoc for struct drm_dp_link
>   drm/dp: Add drm_dp_link_reset() implementation
>   drm/dp: Track link capabilities alongside settings
>   drm/dp: Turn link capabilities into booleans
>   drm/dp: Probe link using existing parsing helpers
>   drm/dp: Read fast training capability from link
>   drm/dp: Read TPS3 capability from sink
>   drm/dp: Read channel coding capability from sink
>   drm/dp: Read alternate scrambler reset capability from sink
>   drm/dp: Read eDP version from DPCD
>   drm/dp: Read AUX read interval from DPCD
>   drm/dp: Do not busy-loop during link training
>   drm/dp: Use drm_dp_aux_rd_interval()
>   drm/dp: Add helper to get post-cursor adjustments
>   drm/dp: Set channel coding on link configuration
>   drm/dp: Enable alternate scrambler reset when supported
>   drm/dp: Add drm_dp_link_choose() helper
>   drm/dp: Add support for eDP link rates
>   drm/dp: Remove a gratuituous blank line
>   drm/bridge: tc358767: Use DP nomenclature

Anyone interested in reviewing these?

Thierry

> 
>  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
>  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
>  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
>  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
>  drivers/gpu/drm/tegra/sor.c            |  32 +--
>  include/drm/drm_dp_helper.h            | 124 +++++++++-
>  8 files changed, 459 insertions(+), 87 deletions(-)
> 
> -- 
> 2.22.0
> 

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Jani Nikula]

On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>> 
>> Hi,
>> 
>> this series of patches improves the DP helpers a bit and cleans up some
>> inconsistencies along the way.
>> 
>> v2 incorporates all review comments add collects Reviewed-bys from v1.
>> 
>> Thierry
>> 
>> Thierry Reding (21):
>>   drm/dp: Sort includes alphabetically
>>   drm/dp: Add missing kerneldoc for struct drm_dp_link
>>   drm/dp: Add drm_dp_link_reset() implementation
>>   drm/dp: Track link capabilities alongside settings
>>   drm/dp: Turn link capabilities into booleans
>>   drm/dp: Probe link using existing parsing helpers
>>   drm/dp: Read fast training capability from link
>>   drm/dp: Read TPS3 capability from sink
>>   drm/dp: Read channel coding capability from sink
>>   drm/dp: Read alternate scrambler reset capability from sink
>>   drm/dp: Read eDP version from DPCD
>>   drm/dp: Read AUX read interval from DPCD
>>   drm/dp: Do not busy-loop during link training
>>   drm/dp: Use drm_dp_aux_rd_interval()
>>   drm/dp: Add helper to get post-cursor adjustments
>>   drm/dp: Set channel coding on link configuration
>>   drm/dp: Enable alternate scrambler reset when supported
>>   drm/dp: Add drm_dp_link_choose() helper
>>   drm/dp: Add support for eDP link rates
>>   drm/dp: Remove a gratuituous blank line
>>   drm/bridge: tc358767: Use DP nomenclature
>
> Anyone interested in reviewing these?

Thierry, I don't quite know how to put this nicely, but I also don't
think it's nice to not reply at all. So I'll try to be fair but it'll be
blunt. Fair enough?

I've glanced over the series, already before you pinged for reviews. It
looks like you've put effort into it, and it all looks nice. However, it
does not look like we could use this in i915, without significant effort
both on top of this work and in i915. It does not feel like there's any
incentive for us to review this in detail.

It also feels like there's an increasing disconnect between "small" and
"big" drivers (*) when it comes to handling DP link and training. It
scares me a bit that this work is being done on the terms of the "small"
drivers, and that later in time this might be considered the One True
Way of handling DP.

One of the technical observations is that you fill the struct
drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear
that the link caps really are an intersection of the source and sink
caps. The eDP 1.4 link rates are the prime example. I think you should
have sets of source and sink rates, and you should intersect those to
find out the available link rates. The max rate is the highest number in
that set. Similarly for many things, like training pattern support. I
think it's only going to get more complicated with DP 2.0.

Another pain point is the caching of the caps as bits in
drm_dp_link_caps. How far are you going to take it? There's an insane
and growing amount of things in the DPCD that describe the link in one
way or another. Should they all be added to caps? Where do you draw the
line? Do we add both the bit and the helper for getting that bit from
the DPCD? And are you then going to add support for intersecting all
those cap bits between the source and the sink?

---

Overall I think there is value in unifying how we handle DP in drm. Even
if just by providing helpers to simplify things in drivers. It's just
that I feel this series isn't taking us closer to that goal, except for
a subset of drivers. In the big picture, it may be increasing the
divide.

If we get a confirmation from our drm overlords that drivers doing
things the way they see fit in this regard is fine, then I'm okay with
this. But I'm definitely not committing to switching to using the
drm_dp_link structures and helpers in i915, quite the opposite actually.


BR,
Jani.


(*) Please don't read too much into "small" and "big", just two groups
of drivers handling things differently.




>
> Thierry
>
>> 
>>  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
>>  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
>>  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
>>  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
>>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
>>  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
>>  drivers/gpu/drm/tegra/sor.c            |  32 +--
>>  include/drm/drm_dp_helper.h            | 124 +++++++++-
>>  8 files changed, 459 insertions(+), 87 deletions(-)
>> 
>> -- 
>> 2.22.0
>> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 10367 bytes --]

On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote:
> On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
> >> From: Thierry Reding <treding@nvidia.com>
> >> 
> >> Hi,
> >> 
> >> this series of patches improves the DP helpers a bit and cleans up some
> >> inconsistencies along the way.
> >> 
> >> v2 incorporates all review comments add collects Reviewed-bys from v1.
> >> 
> >> Thierry
> >> 
> >> Thierry Reding (21):
> >>   drm/dp: Sort includes alphabetically
> >>   drm/dp: Add missing kerneldoc for struct drm_dp_link
> >>   drm/dp: Add drm_dp_link_reset() implementation
> >>   drm/dp: Track link capabilities alongside settings
> >>   drm/dp: Turn link capabilities into booleans
> >>   drm/dp: Probe link using existing parsing helpers
> >>   drm/dp: Read fast training capability from link
> >>   drm/dp: Read TPS3 capability from sink
> >>   drm/dp: Read channel coding capability from sink
> >>   drm/dp: Read alternate scrambler reset capability from sink
> >>   drm/dp: Read eDP version from DPCD
> >>   drm/dp: Read AUX read interval from DPCD
> >>   drm/dp: Do not busy-loop during link training
> >>   drm/dp: Use drm_dp_aux_rd_interval()
> >>   drm/dp: Add helper to get post-cursor adjustments
> >>   drm/dp: Set channel coding on link configuration
> >>   drm/dp: Enable alternate scrambler reset when supported
> >>   drm/dp: Add drm_dp_link_choose() helper
> >>   drm/dp: Add support for eDP link rates
> >>   drm/dp: Remove a gratuituous blank line
> >>   drm/bridge: tc358767: Use DP nomenclature
> >
> > Anyone interested in reviewing these?
> 
> Thierry, I don't quite know how to put this nicely, but I also don't
> think it's nice to not reply at all. So I'll try to be fair but it'll be
> blunt. Fair enough?

Fair enough.

> I've glanced over the series, already before you pinged for reviews. It
> looks like you've put effort into it, and it all looks nice. However, it
> does not look like we could use this in i915, without significant effort
> both on top of this work and in i915. It does not feel like there's any
> incentive for us to review this in detail.
> 
> It also feels like there's an increasing disconnect between "small" and
> "big" drivers (*) when it comes to handling DP link and training. It
> scares me a bit that this work is being done on the terms of the "small"
> drivers, and that later in time this might be considered the One True
> Way of handling DP.

I'm not sure I understand your concern here. The goal of the series is
primarily to extend the existing support for DP. It follows the pattern
established by existing helpers and then goes one step further and
provides some common way of actually storing the values that are being
read from the sink so that they can be used.

These are meant to be helpers and in no way should anyone feel obliged
to use them. If you've got this all figured out already, great! If you
do this already much better in i915, by all means stay away from this.

At the same time it seems counter-productive to write all of this code
as part of the Tegra DRM driver. In my opinion subsystems should provide
generic helpers that can help multiple drivers share code. This is
especially true for things that are defined in a specification because
there's not a lot of room for interpretation. The helpers in these
patches are meant to be that kind of helpers.

> One of the technical observations is that you fill the struct
> drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear
> that the link caps really are an intersection of the source and sink
> caps. The eDP 1.4 link rates are the prime example. I think you should
> have sets of source and sink rates, and you should intersect those to
> find out the available link rates. The max rate is the highest number in
> that set. Similarly for many things, like training pattern support. I
> think it's only going to get more complicated with DP 2.0.

The idea here was to provide only helpers to collect the DPCD data
defined by the specification. Anything specific to the source is meant
to be handled by display driver. In case of eDP 1.4 link rates the code
will only add the rates read from DPCD. It's up to the driver to filter
out rates that it doesn't support from that list.

I think the fact that things will keep getting more complicated is an
argument in favour of sharing code rather than keep doing the same
(complicated) thing over and over again in every driver.

> Another pain point is the caching of the caps as bits in
> drm_dp_link_caps. How far are you going to take it? There's an insane
> and growing amount of things in the DPCD that describe the link in one
> way or another. Should they all be added to caps? Where do you draw the
> line? Do we add both the bit and the helper for getting that bit from
> the DPCD? And are you then going to add support for intersecting all
> those cap bits between the source and the sink?

Like I said the primary goal here is to have common code to read the
common values from DPCD. Once the link has been "probed" it is up to the
driver to do whatever it wants with that data.

Originally I had intended this shared code to do much more, but this was
shot down during review (I think by Daniel and yourself) for many of the
same reasons that you're pointing out. Initially there was code in this
series to standardize the link training sequence, for example. This was
all strictly according to the specification, so I thought that would
give us enough common ground for shared code. But you guys didn't agree,
so I've moved that out into Tegra specific patches since then.

As for how far to take this, I think the most sensible is to do what we
do everywhere else. We add to this whatever is needed on an on-demand
basis. The current series here adds what I found to be necessary to
support DP on Tegra. There's not a lot of fancy stuff here, I know, but
that doesn't mean this code is useless for everyone else.

So, can i915 use this? Probably yes. Would that be a good idea? Probably
not. And that's perfectly fine. But I could imagine that others may very
well want to use some shared code to avoid having to copy/paste code and
then later fix up cargo-culted bugs in every driver.

I'm also fully aware that this is not a lot and it may not be perfect.
But most helpers aren't initially. The point here is to start collecting
the common bits in one location and evolve them, just like we do for so
many other helpers.

Note also that I haven't made any attempt here to convert any drivers to
these helpers. That's because these are meant to be opt-in to simplify
drivers. If you want to do everything yourself, feel free to do that. It
is perfectly legit to do everything yourself if these helpers aren't
flexible enough to do what you want. The better option would be to help
improve the helpers to make them work for a wider range of drivers, but
if you don't want to, then don't.

> Overall I think there is value in unifying how we handle DP in drm. Even
> if just by providing helpers to simplify things in drivers. It's just
> that I feel this series isn't taking us closer to that goal, except for
> a subset of drivers. In the big picture, it may be increasing the
> divide.

So the bulk of this series is stuff that's purely parsing values from
DPCD, which is very much in line with existing helpers. I don't think
those are in any way contentious. There's also a bit of cleanup here
where new helpers are used to simplify existing ones. Maybe a handful
of these patches are what you claim might be "increasing the divide".

But I really don't understand where this is coming from. i915 doesn't
use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet
these are not increasing any divide, are they? Why do you think these
helpers here are different?

Again, if you've got all of this implemented in i915 (or any of the
other "big" drivers), you probably want to stay away from these. But
does that mean everyone else has to go and figure all of this out from
scratch? Shouldn't we at least attempt to write common code? Or should
we all go and write our own DP stacks like the big drivers? I don't see
any advantage in that.

> If we get a confirmation from our drm overlords that drivers doing
> things the way they see fit in this regard is fine, then I'm okay with
> this. But I'm definitely not committing to switching to using the
> drm_dp_link structures and helpers in i915, quite the opposite actually.

I don't think anyone's going to force you to convert to the drm_dp_link
helpers if you don't want to. It's definitely not my intention to make
this "The One And Only Way To Do DP in DRM". The goal here is to create
helpers that can simplify adding DP support.

Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I
will get over it and move this into Tegra code. But since we're being
blunt, I'd like to get a third (and ideally fourth) opinion on whether
we really want this stuff to be reinvented in every driver or whether
we want to try and come up with something that works for more than one
driver.

Thierry

> BR,
> Jani.
> 
> 
> (*) Please don't read too much into "small" and "big", just two groups
> of drivers handling things differently.
> 
> 
> 
> 
> >
> > Thierry
> >
> >> 
> >>  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
> >>  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
> >>  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
> >>  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
> >>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
> >>  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
> >>  drivers/gpu/drm/tegra/sor.c            |  32 +--
> >>  include/drm/drm_dp_helper.h            | 124 +++++++++-
> >>  8 files changed, 459 insertions(+), 87 deletions(-)
> >> 
> >> -- 
> >> 2.22.0
> >> 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 11617 bytes --]

On Mon, Sep 23, 2019 at 04:52:02PM +0200, Thierry Reding wrote:
> On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote:
> > On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
> > >> From: Thierry Reding <treding@nvidia.com>
> > >> 
> > >> Hi,
> > >> 
> > >> this series of patches improves the DP helpers a bit and cleans up some
> > >> inconsistencies along the way.
> > >> 
> > >> v2 incorporates all review comments add collects Reviewed-bys from v1.
> > >> 
> > >> Thierry
> > >> 
> > >> Thierry Reding (21):
> > >>   drm/dp: Sort includes alphabetically
> > >>   drm/dp: Add missing kerneldoc for struct drm_dp_link
> > >>   drm/dp: Add drm_dp_link_reset() implementation
> > >>   drm/dp: Track link capabilities alongside settings
> > >>   drm/dp: Turn link capabilities into booleans
> > >>   drm/dp: Probe link using existing parsing helpers
> > >>   drm/dp: Read fast training capability from link
> > >>   drm/dp: Read TPS3 capability from sink
> > >>   drm/dp: Read channel coding capability from sink
> > >>   drm/dp: Read alternate scrambler reset capability from sink
> > >>   drm/dp: Read eDP version from DPCD
> > >>   drm/dp: Read AUX read interval from DPCD
> > >>   drm/dp: Do not busy-loop during link training
> > >>   drm/dp: Use drm_dp_aux_rd_interval()
> > >>   drm/dp: Add helper to get post-cursor adjustments
> > >>   drm/dp: Set channel coding on link configuration
> > >>   drm/dp: Enable alternate scrambler reset when supported
> > >>   drm/dp: Add drm_dp_link_choose() helper
> > >>   drm/dp: Add support for eDP link rates
> > >>   drm/dp: Remove a gratuituous blank line
> > >>   drm/bridge: tc358767: Use DP nomenclature
> > >
> > > Anyone interested in reviewing these?
> > 
> > Thierry, I don't quite know how to put this nicely, but I also don't
> > think it's nice to not reply at all. So I'll try to be fair but it'll be
> > blunt. Fair enough?
> 
> Fair enough.
> 
> > I've glanced over the series, already before you pinged for reviews. It
> > looks like you've put effort into it, and it all looks nice. However, it
> > does not look like we could use this in i915, without significant effort
> > both on top of this work and in i915. It does not feel like there's any
> > incentive for us to review this in detail.
> > 
> > It also feels like there's an increasing disconnect between "small" and
> > "big" drivers (*) when it comes to handling DP link and training. It
> > scares me a bit that this work is being done on the terms of the "small"
> > drivers, and that later in time this might be considered the One True
> > Way of handling DP.
> 
> I'm not sure I understand your concern here. The goal of the series is
> primarily to extend the existing support for DP. It follows the pattern
> established by existing helpers and then goes one step further and
> provides some common way of actually storing the values that are being
> read from the sink so that they can be used.
> 
> These are meant to be helpers and in no way should anyone feel obliged
> to use them. If you've got this all figured out already, great! If you
> do this already much better in i915, by all means stay away from this.
> 
> At the same time it seems counter-productive to write all of this code
> as part of the Tegra DRM driver. In my opinion subsystems should provide
> generic helpers that can help multiple drivers share code. This is
> especially true for things that are defined in a specification because
> there's not a lot of room for interpretation. The helpers in these
> patches are meant to be that kind of helpers.
> 
> > One of the technical observations is that you fill the struct
> > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear
> > that the link caps really are an intersection of the source and sink
> > caps. The eDP 1.4 link rates are the prime example. I think you should
> > have sets of source and sink rates, and you should intersect those to
> > find out the available link rates. The max rate is the highest number in
> > that set. Similarly for many things, like training pattern support. I
> > think it's only going to get more complicated with DP 2.0.
> 
> The idea here was to provide only helpers to collect the DPCD data
> defined by the specification. Anything specific to the source is meant
> to be handled by display driver. In case of eDP 1.4 link rates the code
> will only add the rates read from DPCD. It's up to the driver to filter
> out rates that it doesn't support from that list.
> 
> I think the fact that things will keep getting more complicated is an
> argument in favour of sharing code rather than keep doing the same
> (complicated) thing over and over again in every driver.
> 
> > Another pain point is the caching of the caps as bits in
> > drm_dp_link_caps. How far are you going to take it? There's an insane
> > and growing amount of things in the DPCD that describe the link in one
> > way or another. Should they all be added to caps? Where do you draw the
> > line? Do we add both the bit and the helper for getting that bit from
> > the DPCD? And are you then going to add support for intersecting all
> > those cap bits between the source and the sink?
> 
> Like I said the primary goal here is to have common code to read the
> common values from DPCD. Once the link has been "probed" it is up to the
> driver to do whatever it wants with that data.
> 
> Originally I had intended this shared code to do much more, but this was
> shot down during review (I think by Daniel and yourself) for many of the
> same reasons that you're pointing out. Initially there was code in this
> series to standardize the link training sequence, for example. This was
> all strictly according to the specification, so I thought that would
> give us enough common ground for shared code. But you guys didn't agree,
> so I've moved that out into Tegra specific patches since then.
> 
> As for how far to take this, I think the most sensible is to do what we
> do everywhere else. We add to this whatever is needed on an on-demand
> basis. The current series here adds what I found to be necessary to
> support DP on Tegra. There's not a lot of fancy stuff here, I know, but
> that doesn't mean this code is useless for everyone else.
> 
> So, can i915 use this? Probably yes. Would that be a good idea? Probably
> not. And that's perfectly fine. But I could imagine that others may very
> well want to use some shared code to avoid having to copy/paste code and
> then later fix up cargo-culted bugs in every driver.
> 
> I'm also fully aware that this is not a lot and it may not be perfect.
> But most helpers aren't initially. The point here is to start collecting
> the common bits in one location and evolve them, just like we do for so
> many other helpers.
> 
> Note also that I haven't made any attempt here to convert any drivers to
> these helpers. That's because these are meant to be opt-in to simplify
> drivers. If you want to do everything yourself, feel free to do that. It
> is perfectly legit to do everything yourself if these helpers aren't
> flexible enough to do what you want. The better option would be to help
> improve the helpers to make them work for a wider range of drivers, but
> if you don't want to, then don't.
> 
> > Overall I think there is value in unifying how we handle DP in drm. Even
> > if just by providing helpers to simplify things in drivers. It's just
> > that I feel this series isn't taking us closer to that goal, except for
> > a subset of drivers. In the big picture, it may be increasing the
> > divide.
> 
> So the bulk of this series is stuff that's purely parsing values from
> DPCD, which is very much in line with existing helpers. I don't think
> those are in any way contentious. There's also a bit of cleanup here
> where new helpers are used to simplify existing ones. Maybe a handful
> of these patches are what you claim might be "increasing the divide".
> 
> But I really don't understand where this is coming from. i915 doesn't
> use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet
> these are not increasing any divide, are they? Why do you think these
> helpers here are different?
> 
> Again, if you've got all of this implemented in i915 (or any of the
> other "big" drivers), you probably want to stay away from these. But
> does that mean everyone else has to go and figure all of this out from
> scratch? Shouldn't we at least attempt to write common code? Or should
> we all go and write our own DP stacks like the big drivers? I don't see
> any advantage in that.
> 
> > If we get a confirmation from our drm overlords that drivers doing
> > things the way they see fit in this regard is fine, then I'm okay with
> > this. But I'm definitely not committing to switching to using the
> > drm_dp_link structures and helpers in i915, quite the opposite actually.
> 
> I don't think anyone's going to force you to convert to the drm_dp_link
> helpers if you don't want to. It's definitely not my intention to make
> this "The One And Only Way To Do DP in DRM". The goal here is to create
> helpers that can simplify adding DP support.
> 
> Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I
> will get over it and move this into Tegra code. But since we're being
> blunt, I'd like to get a third (and ideally fourth) opinion on whether
> we really want this stuff to be reinvented in every driver or whether
> we want to try and come up with something that works for more than one
> driver.
> 
> Thierry
> 
> > BR,
> > Jani.
> > 
> > 
> > (*) Please don't read too much into "small" and "big", just two groups
> > of drivers handling things differently.

Dave, Daniel,

how can we make progress on this? Is it okay to go forward with this
series if we agree that it's not going to be required for drivers to
adopt it if they already have a working DP implementation?

If we can't agree on the struct drm_dp_link helpers, perhaps I should go
and at least merge the additional DPCD parsing helpers. Those are very
much in line with existing helpers. I could then move the drm_dp_link
helpers into the Tegra driver and add replacement code to the other
drivers that already use struct drm_dp_link. It'd be a shame to
duplicate the code, but I'm willing to invest that additional work so
that I can finally make progress on this series and the Tegra DP support
that depends on this.

Thierry

> > 
> > 
> > 
> > 
> > >
> > > Thierry
> > >
> > >> 
> > >>  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
> > >>  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
> > >>  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
> > >>  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
> > >>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
> > >>  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
> > >>  drivers/gpu/drm/tegra/sor.c            |  32 +--
> > >>  include/drm/drm_dp_helper.h            | 124 +++++++++-
> > >>  8 files changed, 459 insertions(+), 87 deletions(-)
> > >> 
> > >> -- 
> > >> 2.22.0
> > >> 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center



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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Daniel Vetter]

On Wed, Oct 02, 2019 at 06:14:19PM +0200, Thierry Reding wrote:
> On Mon, Sep 23, 2019 at 04:52:02PM +0200, Thierry Reding wrote:
> > On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote:
> > > On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
> > > >> From: Thierry Reding <treding@nvidia.com>
> > > >> 
> > > >> Hi,
> > > >> 
> > > >> this series of patches improves the DP helpers a bit and cleans up some
> > > >> inconsistencies along the way.
> > > >> 
> > > >> v2 incorporates all review comments add collects Reviewed-bys from v1.
> > > >> 
> > > >> Thierry
> > > >> 
> > > >> Thierry Reding (21):
> > > >>   drm/dp: Sort includes alphabetically
> > > >>   drm/dp: Add missing kerneldoc for struct drm_dp_link
> > > >>   drm/dp: Add drm_dp_link_reset() implementation
> > > >>   drm/dp: Track link capabilities alongside settings
> > > >>   drm/dp: Turn link capabilities into booleans
> > > >>   drm/dp: Probe link using existing parsing helpers
> > > >>   drm/dp: Read fast training capability from link
> > > >>   drm/dp: Read TPS3 capability from sink
> > > >>   drm/dp: Read channel coding capability from sink
> > > >>   drm/dp: Read alternate scrambler reset capability from sink
> > > >>   drm/dp: Read eDP version from DPCD
> > > >>   drm/dp: Read AUX read interval from DPCD
> > > >>   drm/dp: Do not busy-loop during link training
> > > >>   drm/dp: Use drm_dp_aux_rd_interval()
> > > >>   drm/dp: Add helper to get post-cursor adjustments
> > > >>   drm/dp: Set channel coding on link configuration
> > > >>   drm/dp: Enable alternate scrambler reset when supported
> > > >>   drm/dp: Add drm_dp_link_choose() helper
> > > >>   drm/dp: Add support for eDP link rates
> > > >>   drm/dp: Remove a gratuituous blank line
> > > >>   drm/bridge: tc358767: Use DP nomenclature
> > > >
> > > > Anyone interested in reviewing these?
> > > 
> > > Thierry, I don't quite know how to put this nicely, but I also don't
> > > think it's nice to not reply at all. So I'll try to be fair but it'll be
> > > blunt. Fair enough?
> > 
> > Fair enough.
> > 
> > > I've glanced over the series, already before you pinged for reviews. It
> > > looks like you've put effort into it, and it all looks nice. However, it
> > > does not look like we could use this in i915, without significant effort
> > > both on top of this work and in i915. It does not feel like there's any
> > > incentive for us to review this in detail.
> > > 
> > > It also feels like there's an increasing disconnect between "small" and
> > > "big" drivers (*) when it comes to handling DP link and training. It
> > > scares me a bit that this work is being done on the terms of the "small"
> > > drivers, and that later in time this might be considered the One True
> > > Way of handling DP.
> > 
> > I'm not sure I understand your concern here. The goal of the series is
> > primarily to extend the existing support for DP. It follows the pattern
> > established by existing helpers and then goes one step further and
> > provides some common way of actually storing the values that are being
> > read from the sink so that they can be used.
> > 
> > These are meant to be helpers and in no way should anyone feel obliged
> > to use them. If you've got this all figured out already, great! If you
> > do this already much better in i915, by all means stay away from this.
> > 
> > At the same time it seems counter-productive to write all of this code
> > as part of the Tegra DRM driver. In my opinion subsystems should provide
> > generic helpers that can help multiple drivers share code. This is
> > especially true for things that are defined in a specification because
> > there's not a lot of room for interpretation. The helpers in these
> > patches are meant to be that kind of helpers.
> > 
> > > One of the technical observations is that you fill the struct
> > > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear
> > > that the link caps really are an intersection of the source and sink
> > > caps. The eDP 1.4 link rates are the prime example. I think you should
> > > have sets of source and sink rates, and you should intersect those to
> > > find out the available link rates. The max rate is the highest number in
> > > that set. Similarly for many things, like training pattern support. I
> > > think it's only going to get more complicated with DP 2.0.
> > 
> > The idea here was to provide only helpers to collect the DPCD data
> > defined by the specification. Anything specific to the source is meant
> > to be handled by display driver. In case of eDP 1.4 link rates the code
> > will only add the rates read from DPCD. It's up to the driver to filter
> > out rates that it doesn't support from that list.
> > 
> > I think the fact that things will keep getting more complicated is an
> > argument in favour of sharing code rather than keep doing the same
> > (complicated) thing over and over again in every driver.
> > 
> > > Another pain point is the caching of the caps as bits in
> > > drm_dp_link_caps. How far are you going to take it? There's an insane
> > > and growing amount of things in the DPCD that describe the link in one
> > > way or another. Should they all be added to caps? Where do you draw the
> > > line? Do we add both the bit and the helper for getting that bit from
> > > the DPCD? And are you then going to add support for intersecting all
> > > those cap bits between the source and the sink?
> > 
> > Like I said the primary goal here is to have common code to read the
> > common values from DPCD. Once the link has been "probed" it is up to the
> > driver to do whatever it wants with that data.
> > 
> > Originally I had intended this shared code to do much more, but this was
> > shot down during review (I think by Daniel and yourself) for many of the
> > same reasons that you're pointing out. Initially there was code in this
> > series to standardize the link training sequence, for example. This was
> > all strictly according to the specification, so I thought that would
> > give us enough common ground for shared code. But you guys didn't agree,
> > so I've moved that out into Tegra specific patches since then.
> > 
> > As for how far to take this, I think the most sensible is to do what we
> > do everywhere else. We add to this whatever is needed on an on-demand
> > basis. The current series here adds what I found to be necessary to
> > support DP on Tegra. There's not a lot of fancy stuff here, I know, but
> > that doesn't mean this code is useless for everyone else.
> > 
> > So, can i915 use this? Probably yes. Would that be a good idea? Probably
> > not. And that's perfectly fine. But I could imagine that others may very
> > well want to use some shared code to avoid having to copy/paste code and
> > then later fix up cargo-culted bugs in every driver.
> > 
> > I'm also fully aware that this is not a lot and it may not be perfect.
> > But most helpers aren't initially. The point here is to start collecting
> > the common bits in one location and evolve them, just like we do for so
> > many other helpers.
> > 
> > Note also that I haven't made any attempt here to convert any drivers to
> > these helpers. That's because these are meant to be opt-in to simplify
> > drivers. If you want to do everything yourself, feel free to do that. It
> > is perfectly legit to do everything yourself if these helpers aren't
> > flexible enough to do what you want. The better option would be to help
> > improve the helpers to make them work for a wider range of drivers, but
> > if you don't want to, then don't.
> > 
> > > Overall I think there is value in unifying how we handle DP in drm. Even
> > > if just by providing helpers to simplify things in drivers. It's just
> > > that I feel this series isn't taking us closer to that goal, except for
> > > a subset of drivers. In the big picture, it may be increasing the
> > > divide.
> > 
> > So the bulk of this series is stuff that's purely parsing values from
> > DPCD, which is very much in line with existing helpers. I don't think
> > those are in any way contentious. There's also a bit of cleanup here
> > where new helpers are used to simplify existing ones. Maybe a handful
> > of these patches are what you claim might be "increasing the divide".
> > 
> > But I really don't understand where this is coming from. i915 doesn't
> > use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet
> > these are not increasing any divide, are they? Why do you think these
> > helpers here are different?
> > 
> > Again, if you've got all of this implemented in i915 (or any of the
> > other "big" drivers), you probably want to stay away from these. But
> > does that mean everyone else has to go and figure all of this out from
> > scratch? Shouldn't we at least attempt to write common code? Or should
> > we all go and write our own DP stacks like the big drivers? I don't see
> > any advantage in that.
> > 
> > > If we get a confirmation from our drm overlords that drivers doing
> > > things the way they see fit in this regard is fine, then I'm okay with
> > > this. But I'm definitely not committing to switching to using the
> > > drm_dp_link structures and helpers in i915, quite the opposite actually.
> > 
> > I don't think anyone's going to force you to convert to the drm_dp_link
> > helpers if you don't want to. It's definitely not my intention to make
> > this "The One And Only Way To Do DP in DRM". The goal here is to create
> > helpers that can simplify adding DP support.
> > 
> > Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I
> > will get over it and move this into Tegra code. But since we're being
> > blunt, I'd like to get a third (and ideally fourth) opinion on whether
> > we really want this stuff to be reinvented in every driver or whether
> > we want to try and come up with something that works for more than one
> > driver.
> > 
> > Thierry
> > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > (*) Please don't read too much into "small" and "big", just two groups
> > > of drivers handling things differently.
> 
> Dave, Daniel,
> 
> how can we make progress on this? Is it okay to go forward with this
> series if we agree that it's not going to be required for drivers to
> adopt it if they already have a working DP implementation?
> 
> If we can't agree on the struct drm_dp_link helpers, perhaps I should go
> and at least merge the additional DPCD parsing helpers. Those are very
> much in line with existing helpers. I could then move the drm_dp_link
> helpers into the Tegra driver and add replacement code to the other
> drivers that already use struct drm_dp_link. It'd be a shame to
> duplicate the code, but I'm willing to invest that additional work so
> that I can finally make progress on this series and the Tegra DP support
> that depends on this.

I ... don't know.

In general I think helpers are totally ok with being optional (that's the
point after all), but not for color choice reasons. And from very much
afar this feels a bit like that.

I also think that if we want smaller helpers for simpler drivers, it's
better to build that on top of the full-featured helpers and dumb them
down. This worked very well for simple display pipe (built on top of
atomic helpers) and simple vram support (built on top of ttm). Having two
distinct set of helpers for small and big drivers seems wrong.

I also think that the functions/control-flow/callbacks are the important
part of helpers, not really the data structures. Just having common
data structures gives you as much a mess as having a pile of
distinct implementations.

Aside from all these generic thoughts on helpers, for dp specifically I
think the proof of the pudding is in how it integrates with mst. If we
have dp helpers that work for mst (the mst vs sst case decision and
transitions are especially nasty in all drivers), then we can dumb it down
for simple drivers and modularize it for special cases like we do with
other helpers. Without that I fear we're just ending up in a dead-end (but
won't realize it until it's too late).

Finally I'm ok with no helpers in areas where we haven't figured out a
good solution yet. Bunch of copypasta is imo better than going the wrong
way collectively (since it prevents the experiments that might shine a
light on better solutions).

tldr; Still don't know.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Lyude Paul]

Hi! a couple people poked me to take a look at this, hopefully I can provide
some helpful insight here.

So: I've tried a _lot_ of various redesigns with MST and some portions of the
DP helpers. I actually was going to try to write up some common helpers for
handling link training across drivers, but when I started trying to implement
them (ironically, I think it was in i915!) I realized I wasn't getting rid of
nearly as much code as I thought I was going to.

Now-I'd love to tell you if this series is good or bad, but I'm not entirely
sure myself either. Sometimes I wonder myself if I'm overcomplicating things
with MST. The only way I've really found of figuring out whether or not
helpers like this are overkill is to just implement them everywhere. With my
atomic VCPI helpers for MST, I tried implementing them everywhere I could
(except for amdgpu, but I _did_ try there originally!) to see how awkward they
were to use. I think if there's questions to how useful this series is, it'd
probably be good to try implementing these helpers in drivers like i915 to see
how things play out.

It should also be noted that I did actually try to come up with common link
rate helpers like this one, but I ended up realizing I was adding far more
code then I was getting rid of after I tried implementing them in i915
(ironic, huh?). Things got even more complicated when I looked at how
nouveau/nvidia hardware does link training.

On Fri, 2019-09-20 at 18:00 +0200, Thierry Reding wrote:
> On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Hi,
> > 
> > this series of patches improves the DP helpers a bit and cleans up some
> > inconsistencies along the way.
> > 
> > v2 incorporates all review comments add collects Reviewed-bys from v1.
> > 
> > Thierry
> > 
> > Thierry Reding (21):
> >   drm/dp: Sort includes alphabetically
> >   drm/dp: Add missing kerneldoc for struct drm_dp_link
> >   drm/dp: Add drm_dp_link_reset() implementation
> >   drm/dp: Track link capabilities alongside settings
> >   drm/dp: Turn link capabilities into booleans
> >   drm/dp: Probe link using existing parsing helpers
> >   drm/dp: Read fast training capability from link
> >   drm/dp: Read TPS3 capability from sink
> >   drm/dp: Read channel coding capability from sink
> >   drm/dp: Read alternate scrambler reset capability from sink
> >   drm/dp: Read eDP version from DPCD
> >   drm/dp: Read AUX read interval from DPCD
> >   drm/dp: Do not busy-loop during link training
> >   drm/dp: Use drm_dp_aux_rd_interval()
> >   drm/dp: Add helper to get post-cursor adjustments
> >   drm/dp: Set channel coding on link configuration
> >   drm/dp: Enable alternate scrambler reset when supported
> >   drm/dp: Add drm_dp_link_choose() helper
> >   drm/dp: Add support for eDP link rates
> >   drm/dp: Remove a gratuituous blank line
> >   drm/bridge: tc358767: Use DP nomenclature
> 
> Anyone interested in reviewing these?
> 
> Thierry
> 
> >  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
> >  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
> >  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
> >  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
> >  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
> >  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
> >  drivers/gpu/drm/tegra/sor.c            |  32 +--
> >  include/drm/drm_dp_helper.h            | 124 +++++++++-
> >  8 files changed, 459 insertions(+), 87 deletions(-)
> > 
> > -- 
> > 2.22.0
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Cheers,
	Lyude Paul

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