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(-)
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
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;
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)
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, ®); @@ -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);
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;
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]);
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,
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; };
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,
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,
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;
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;
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);
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);
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
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);
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);
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],
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);
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) {
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");
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
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Lyude Paul -
Thierry Reding