| Message ID | 1474051514-5811-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > According to the DisplayPort Spec, in case of Clock Recovery failure > the link training sequence should fall back to the lower link rate > followed by lower lane count until CR succeeds. > On CR success, the sequence proceeds with Channel EQ. > In case of Channel EQ failures, it should fallback to > lower link rate and lane count and start the CR phase again. This change makes the link training start at the max lane count and max link rate. This is not ideal, as it wastes the link. And it is not a spec requirement. "The Link Policy Maker of the upstream device may choose any link count and link rate as long as they do not exceed the capabilities of the DP receiver." Our current code starts at the minimum required bandwidth for the mode, therefore we can't fall back to lower link rate and lane count without reducing the mode. AFAICT this patch here makes it possible for the link bandwidth to drop below what is required for the mode. This is unacceptable. BR, Jani. > > v7: > * Address readability concerns (Mika Kahola) > v6: > * Do not split quoted string across line (Mika Kahola) > v5: > * Reset the link rate index to the max link rate index > before lowering the lane count (Jani Nikula) > * Use the paradigm for loop in intel_dp_link_rate_index > v4: > * Fixed the link rate fallback loop (Manasi Navare) > v3: > * Fixed some rebase issues (Mika Kahola) > v2: > * Add a helper function to return index of requested link rate > into common_rates array > * Changed the link rate fallback loop to make use > of common_rates array (Mika Kahola) > * Changed INTEL_INFO to INTEL_GEN (David Weinehall) > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_dp.c | 15 ++++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- > drivers/gpu/drm/i915/intel_drv.h | 6 +- > 4 files changed, 133 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 8065a5f..093038c 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, > } > } > > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, > int link_rate, uint32_t lane_count, > - struct intel_shared_dpll *pll, > - bool link_mst) > + struct intel_shared_dpll *pll) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > enum port port = intel_ddi_get_encoder_port(encoder); > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > - link_mst); > - if (encoder->type == INTEL_OUTPUT_EDP) > - intel_edp_panel_on(intel_dp); > + false); > + > + intel_edp_panel_on(intel_dp); > > intel_ddi_clk_select(encoder, pll); > intel_prepare_dp_ddi_buffers(encoder); > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > intel_dp_stop_link_train(intel_dp); > } > > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > + int link_rate, uint32_t lane_count, > + struct intel_shared_dpll *pll, > + bool link_mst) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_shared_dpll_config tmp_pll_config; > + > + /* Disable the PLL and obtain the PLL for Link Training > + * that starts with highest link rate and lane count. > + */ > + tmp_pll_config = pll->config; > + pll->funcs.disable(dev_priv, pll); > + pll->config.crtc_mask = 0; > + > + /* If Link Training fails, send a uevent to generate a hotplug */ > + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) > + drm_kms_helper_hotplug_event(encoder->base.dev); > + pll->config = tmp_pll_config; > +} > + > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > bool has_hdmi_sink, > struct drm_display_mode *adjusted_mode, > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, > struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); > int type = intel_encoder->type; > > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { > + if (type == INTEL_OUTPUT_EDP) > + intel_ddi_pre_enable_edp(intel_encoder, > + crtc->config->port_clock, > + crtc->config->lane_count, > + crtc->config->shared_dpll); > + > + if (type == INTEL_OUTPUT_DP) > intel_ddi_pre_enable_dp(intel_encoder, > crtc->config->port_clock, > crtc->config->lane_count, > crtc->config->shared_dpll, > intel_crtc_has_type(crtc->config, > INTEL_OUTPUT_DP_MST)); > - } > - if (type == INTEL_OUTPUT_HDMI) { > + > + if (type == INTEL_OUTPUT_HDMI) > intel_ddi_pre_enable_hdmi(intel_encoder, > crtc->config->has_hdmi_sink, > &crtc->config->base.adjusted_mode, > crtc->config->shared_dpll); > - } > + > } > > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, > @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) > return pll; > } > > +bool > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > + uint8_t max_lane_count, bool link_mst) > +{ > + struct intel_connector *connector = intel_dp->attached_connector; > + struct intel_encoder *encoder = connector->encoder; > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_shared_dpll *pll; > + struct intel_shared_dpll_config tmp_pll_config; > + int link_rate, max_link_rate_index, link_rate_index; > + uint8_t lane_count; > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > + bool ret = false; > + > + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, > + max_link_rate); > + if (max_link_rate_index < 0) { > + DRM_ERROR("Invalid Link Rate\n"); > + return false; > + } > + > + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { > + for (link_rate_index = max_link_rate_index; > + link_rate_index >= 0; link_rate_index--) { > + link_rate = common_rates[link_rate_index]; > + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); > + if (pll == NULL) { > + DRM_ERROR("Could not find DPLL for link training.\n"); > + return false; > + } > + tmp_pll_config = pll->config; > + pll->funcs.enable(dev_priv, pll); > + > + intel_dp_set_link_params(intel_dp, link_rate, > + lane_count, link_mst); > + > + intel_ddi_clk_select(encoder, pll); > + intel_prepare_dp_ddi_buffers(encoder); > + intel_ddi_init_dp_buf_reg(encoder); > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > + ret = intel_dp_start_link_train(intel_dp); > + if (ret) > + break; > + > + /* Disable port followed by PLL for next > + *retry/clean up > + */ > + intel_ddi_post_disable(encoder, NULL, NULL); > + pll->funcs.disable(dev_priv, pll); > + pll->config = tmp_pll_config; > + } > + if (ret) { > + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", > + link_rate, lane_count); > + break; > + } > + } > + > + intel_dp_stop_link_train(intel_dp); > + > + if (!lane_count) > + DRM_ERROR("Link Training Failed\n"); > + > + return ret; > +} > + > void intel_ddi_init(struct drm_device *dev, enum port port) > { > struct drm_i915_private *dev_priv = to_i915(dev); > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 69cee9b..d81c67cb 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) > return rates[len - 1]; > } > > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > + int link_rate) > +{ > + int common_len; > + int index; > + > + common_len = intel_dp_common_rates(intel_dp, common_rates); > + for (index = 0; index < common_len; index++) { > + if (link_rate == common_rates[common_len - index - 1]) > + return common_len - index - 1; > + } > + > + return -1; > +} > + > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) > { > return rate_to_index(rate, intel_dp->sink_rates); > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index c438b02..6eb5eb6 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > DP_TRAINING_PATTERN_DISABLE); > } > > -void > +bool > intel_dp_start_link_train(struct intel_dp *intel_dp) > { > - intel_dp_link_training_clock_recovery(intel_dp); > - intel_dp_link_training_channel_equalization(intel_dp); > + bool ret; > + > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > + ret = intel_dp_link_training_channel_equalization(intel_dp); > + if (ret) > + return true; > + } > + > + return false; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8fd16ad..08cb571 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config); > void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); > uint32_t ddi_signal_levels(struct intel_dp *intel_dp); > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > + uint8_t max_lane_count, bool link_mst); > struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, > int clock); > unsigned int intel_fb_align_height(struct drm_device *dev, > @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > void intel_dp_set_link_params(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count, > bool link_mst); > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co > void intel_dp_mst_suspend(struct drm_device *dev); > void intel_dp_mst_resume(struct drm_device *dev); > int intel_dp_max_link_rate(struct intel_dp *intel_dp); > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > + int link_rate); > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv);
On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > According to the DisplayPort Spec, in case of Clock Recovery failure > > the link training sequence should fall back to the lower link rate > > followed by lower lane count until CR succeeds. > > On CR success, the sequence proceeds with Channel EQ. > > In case of Channel EQ failures, it should fallback to > > lower link rate and lane count and start the CR phase again. > > This change makes the link training start at the max lane count and max > link rate. This is not ideal, as it wastes the link. And it is not a > spec requirement. "The Link Policy Maker of the upstream device may > choose any link count and link rate as long as they do not exceed the > capabilities of the DP receiver." > > Our current code starts at the minimum required bandwidth for the mode, > therefore we can't fall back to lower link rate and lane count without > reducing the mode. > > AFAICT this patch here makes it possible for the link bandwidth to drop > below what is required for the mode. This is unacceptable. > > BR, > Jani. > > Thanks Jani for your review comments. Yes in this change we start at the max link rate and lane count. This change was made according to the design document discussions we had before strating this DP Redesign project. The main reason for starting at the maxlink rate and max lane count was for ensuring proper behavior of DP MST. In case of DP MST, we want to train the link at the maximum supported link rate/lane count based on an early/ upfront link training result so that we dont fail when we try to connect a higher resolution monitor as a second monitor. This a trade off between wsting the link or higher power vs. needing to retrain for every monitor that requests a higher BW in case of DP MST. Actually this is also the reason for enabling upfront link training in the following patch where we train the link much ahead in the modeset sequence to understand the link rate and lane count values at which the link can be successfully trained and then the link training through modeset will always start at the upfront values (maximum supported values of lane count and link rate based on upfront link training). As per the CTS, all the test 4.3.1.4 requires that you fall back to the lower link rate after trying to train at the maximum link rate advertised through the DPCD registers. This will not drop the link BW to a number below what is required for the mode because the requested modes are pruned or validated in intel_dp_mode_valid based on the upfront link training results in the following patch. And these values are used here as the starting values of link rate and lane count. I almost feel that the upfront link training patch and this patch should be combined so that insead of starting from the max link rate and lane count it is clear that we are starting from the upfront values. Regards, Manasi > > > > v7: > > * Address readability concerns (Mika Kahola) > > v6: > > * Do not split quoted string across line (Mika Kahola) > > v5: > > * Reset the link rate index to the max link rate index > > before lowering the lane count (Jani Nikula) > > * Use the paradigm for loop in intel_dp_link_rate_index > > v4: > > * Fixed the link rate fallback loop (Manasi Navare) > > v3: > > * Fixed some rebase issues (Mika Kahola) > > v2: > > * Add a helper function to return index of requested link rate > > into common_rates array > > * Changed the link rate fallback loop to make use > > of common_rates array (Mika Kahola) > > * Changed INTEL_INFO to INTEL_GEN (David Weinehall) > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_dp.c | 15 ++++ > > drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- > > drivers/gpu/drm/i915/intel_drv.h | 6 +- > > 4 files changed, 133 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 8065a5f..093038c 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, > > } > > } > > > > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, > > int link_rate, uint32_t lane_count, > > - struct intel_shared_dpll *pll, > > - bool link_mst) > > + struct intel_shared_dpll *pll) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > enum port port = intel_ddi_get_encoder_port(encoder); > > > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > > - link_mst); > > - if (encoder->type == INTEL_OUTPUT_EDP) > > - intel_edp_panel_on(intel_dp); > > + false); > > + > > + intel_edp_panel_on(intel_dp); > > > > intel_ddi_clk_select(encoder, pll); > > intel_prepare_dp_ddi_buffers(encoder); > > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > intel_dp_stop_link_train(intel_dp); > > } > > > > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > + int link_rate, uint32_t lane_count, > > + struct intel_shared_dpll *pll, > > + bool link_mst) > > +{ > > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_shared_dpll_config tmp_pll_config; > > + > > + /* Disable the PLL and obtain the PLL for Link Training > > + * that starts with highest link rate and lane count. > > + */ > > + tmp_pll_config = pll->config; > > + pll->funcs.disable(dev_priv, pll); > > + pll->config.crtc_mask = 0; > > + > > + /* If Link Training fails, send a uevent to generate a hotplug */ > > + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) > > + drm_kms_helper_hotplug_event(encoder->base.dev); > > + pll->config = tmp_pll_config; > > +} > > + > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > > bool has_hdmi_sink, > > struct drm_display_mode *adjusted_mode, > > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, > > struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); > > int type = intel_encoder->type; > > > > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { > > + if (type == INTEL_OUTPUT_EDP) > > + intel_ddi_pre_enable_edp(intel_encoder, > > + crtc->config->port_clock, > > + crtc->config->lane_count, > > + crtc->config->shared_dpll); > > + > > + if (type == INTEL_OUTPUT_DP) > > intel_ddi_pre_enable_dp(intel_encoder, > > crtc->config->port_clock, > > crtc->config->lane_count, > > crtc->config->shared_dpll, > > intel_crtc_has_type(crtc->config, > > INTEL_OUTPUT_DP_MST)); > > - } > > - if (type == INTEL_OUTPUT_HDMI) { > > + > > + if (type == INTEL_OUTPUT_HDMI) > > intel_ddi_pre_enable_hdmi(intel_encoder, > > crtc->config->has_hdmi_sink, > > &crtc->config->base.adjusted_mode, > > crtc->config->shared_dpll); > > - } > > + > > } > > > > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, > > @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) > > return pll; > > } > > > > +bool > > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > > + uint8_t max_lane_count, bool link_mst) > > +{ > > + struct intel_connector *connector = intel_dp->attached_connector; > > + struct intel_encoder *encoder = connector->encoder; > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_shared_dpll *pll; > > + struct intel_shared_dpll_config tmp_pll_config; > > + int link_rate, max_link_rate_index, link_rate_index; > > + uint8_t lane_count; > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > + bool ret = false; > > + > > + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, > > + max_link_rate); > > + if (max_link_rate_index < 0) { > > + DRM_ERROR("Invalid Link Rate\n"); > > + return false; > > + } > > + > > + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { > > + for (link_rate_index = max_link_rate_index; > > + link_rate_index >= 0; link_rate_index--) { > > + link_rate = common_rates[link_rate_index]; > > + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); > > + if (pll == NULL) { > > + DRM_ERROR("Could not find DPLL for link training.\n"); > > + return false; > > + } > > + tmp_pll_config = pll->config; > > + pll->funcs.enable(dev_priv, pll); > > + > > + intel_dp_set_link_params(intel_dp, link_rate, > > + lane_count, link_mst); > > + > > + intel_ddi_clk_select(encoder, pll); > > + intel_prepare_dp_ddi_buffers(encoder); > > + intel_ddi_init_dp_buf_reg(encoder); > > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > + ret = intel_dp_start_link_train(intel_dp); > > + if (ret) > > + break; > > + > > + /* Disable port followed by PLL for next > > + *retry/clean up > > + */ > > + intel_ddi_post_disable(encoder, NULL, NULL); > > + pll->funcs.disable(dev_priv, pll); > > + pll->config = tmp_pll_config; > > + } > > + if (ret) { > > + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", > > + link_rate, lane_count); > > + break; > > + } > > + } > > + > > + intel_dp_stop_link_train(intel_dp); > > + > > + if (!lane_count) > > + DRM_ERROR("Link Training Failed\n"); > > + > > + return ret; > > +} > > + > > void intel_ddi_init(struct drm_device *dev, enum port port) > > { > > struct drm_i915_private *dev_priv = to_i915(dev); > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 69cee9b..d81c67cb 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) > > return rates[len - 1]; > > } > > > > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > > + int link_rate) > > +{ > > + int common_len; > > + int index; > > + > > + common_len = intel_dp_common_rates(intel_dp, common_rates); > > + for (index = 0; index < common_len; index++) { > > + if (link_rate == common_rates[common_len - index - 1]) > > + return common_len - index - 1; > > + } > > + > > + return -1; > > +} > > + > > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) > > { > > return rate_to_index(rate, intel_dp->sink_rates); > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index c438b02..6eb5eb6 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > > DP_TRAINING_PATTERN_DISABLE); > > } > > > > -void > > +bool > > intel_dp_start_link_train(struct intel_dp *intel_dp) > > { > > - intel_dp_link_training_clock_recovery(intel_dp); > > - intel_dp_link_training_channel_equalization(intel_dp); > > + bool ret; > > + > > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > > + ret = intel_dp_link_training_channel_equalization(intel_dp); > > + if (ret) > > + return true; > > + } > > + > > + return false; > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 8fd16ad..08cb571 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, > > struct intel_crtc_state *pipe_config); > > void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); > > uint32_t ddi_signal_levels(struct intel_dp *intel_dp); > > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > > + uint8_t max_lane_count, bool link_mst); > > struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, > > int clock); > > unsigned int intel_fb_align_height(struct drm_device *dev, > > @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count, > > bool link_mst); > > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > void intel_dp_encoder_reset(struct drm_encoder *encoder); > > @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co > > void intel_dp_mst_suspend(struct drm_device *dev); > > void intel_dp_mst_resume(struct drm_device *dev); > > int intel_dp_max_link_rate(struct intel_dp *intel_dp); > > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > > + int link_rate); > > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); > > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); > > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv); > > -- > Jani Nikula, Intel Open Source Technology Center
On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > According to the DisplayPort Spec, in case of Clock Recovery failure >> > the link training sequence should fall back to the lower link rate >> > followed by lower lane count until CR succeeds. >> > On CR success, the sequence proceeds with Channel EQ. >> > In case of Channel EQ failures, it should fallback to >> > lower link rate and lane count and start the CR phase again. >> >> This change makes the link training start at the max lane count and max >> link rate. This is not ideal, as it wastes the link. And it is not a >> spec requirement. "The Link Policy Maker of the upstream device may >> choose any link count and link rate as long as they do not exceed the >> capabilities of the DP receiver." >> >> Our current code starts at the minimum required bandwidth for the mode, >> therefore we can't fall back to lower link rate and lane count without >> reducing the mode. >> >> AFAICT this patch here makes it possible for the link bandwidth to drop >> below what is required for the mode. This is unacceptable. >> >> BR, >> Jani. >> >> > > Thanks Jani for your review comments. > Yes in this change we start at the max link rate and lane count. This > change was made according to the design document discussions we had > before strating this DP Redesign project. The main reason for starting > at the maxlink rate and max lane count was for ensuring proper > behavior of DP MST. In case of DP MST, we want to train the link at > the maximum supported link rate/lane count based on an early/ upfront > link training result so that we dont fail when we try to connect a > higher resolution monitor as a second monitor. This a trade off > between wsting the link or higher power vs. needing to retrain for > every monitor that requests a higher BW in case of DP MST. We already train at max bandwidth for DP MST, which seems to be the sensible thing to do. > Actually this is also the reason for enabling upfront link training in > the following patch where we train the link much ahead in the modeset > sequence to understand the link rate and lane count values at which > the link can be successfully trained and then the link training > through modeset will always start at the upfront values (maximum > supported values of lane count and link rate based on upfront link > training). I don't see a need to do this for DP SST. > As per the CTS, all the test 4.3.1.4 requires that you fall back to > the lower link rate after trying to train at the maximum link rate > advertised through the DPCD registers. That test does not require the source DUT to default to maximum lane count or link rate of the sink. The source may freely choose the lane count and link rate as long as they don't exceed sink capabilities. For the purposes of the test, the test setup can request specific parameters to be used, but that does not mean using maximum by *default*. We currently lack the feature to reduce lane count and link rate. The key to understand here is that starting at max and reducing down to the sufficient parameters for the mode (which is where we start now) offers no real benefit for any use case. What we're lacking is a feature to reduce the link parameters *below* what's required by the mode the userspace wants. This can only be achieved through cooperation with userspace. > This will not drop the link BW to a number below what is required for > the mode because the requested modes are pruned or validated in > intel_dp_mode_valid based on the upfront link training results in the > following patch. And these values are used here as the starting values > of link rate and lane count. Each patch must be a worthwhile change on its own. By my reading of this patch, we can go under the required bandwidth. You can't justify that by saying the follow-up patch fixes it. > I almost feel that the upfront link training patch and this patch should be > combined so that insead of starting from the max link rate and lane count it > is clear that we are starting from the upfront values. I am still reading and gathering more feedback on the upfront link training patch. I will get back to you. But the impression I'm currently getting is that we can't do this. The upfront link training patch was originally written for USB type C. But if DP compliance has priority, the order of business should be getting compliance without upfront link training. I am also still not convinced upfront link training is required for compliance. To be continued... BR, Jani. > > Regards, > Manasi >> > >> > v7: >> > * Address readability concerns (Mika Kahola) >> > v6: >> > * Do not split quoted string across line (Mika Kahola) >> > v5: >> > * Reset the link rate index to the max link rate index >> > before lowering the lane count (Jani Nikula) >> > * Use the paradigm for loop in intel_dp_link_rate_index >> > v4: >> > * Fixed the link rate fallback loop (Manasi Navare) >> > v3: >> > * Fixed some rebase issues (Mika Kahola) >> > v2: >> > * Add a helper function to return index of requested link rate >> > into common_rates array >> > * Changed the link rate fallback loop to make use >> > of common_rates array (Mika Kahola) >> > * Changed INTEL_INFO to INTEL_GEN (David Weinehall) >> > >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- >> > drivers/gpu/drm/i915/intel_dp.c | 15 ++++ >> > drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- >> > drivers/gpu/drm/i915/intel_drv.h | 6 +- >> > 4 files changed, 133 insertions(+), 14 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> > index 8065a5f..093038c 100644 >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, >> > } >> > } >> > >> > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, >> > int link_rate, uint32_t lane_count, >> > - struct intel_shared_dpll *pll, >> > - bool link_mst) >> > + struct intel_shared_dpll *pll) >> > { >> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> > enum port port = intel_ddi_get_encoder_port(encoder); >> > >> > intel_dp_set_link_params(intel_dp, link_rate, lane_count, >> > - link_mst); >> > - if (encoder->type == INTEL_OUTPUT_EDP) >> > - intel_edp_panel_on(intel_dp); >> > + false); >> > + >> > + intel_edp_panel_on(intel_dp); >> > >> > intel_ddi_clk_select(encoder, pll); >> > intel_prepare_dp_ddi_buffers(encoder); >> > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> > intel_dp_stop_link_train(intel_dp); >> > } >> > >> > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> > + int link_rate, uint32_t lane_count, >> > + struct intel_shared_dpll *pll, >> > + bool link_mst) >> > +{ >> > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> > + struct intel_shared_dpll_config tmp_pll_config; >> > + >> > + /* Disable the PLL and obtain the PLL for Link Training >> > + * that starts with highest link rate and lane count. >> > + */ >> > + tmp_pll_config = pll->config; >> > + pll->funcs.disable(dev_priv, pll); >> > + pll->config.crtc_mask = 0; >> > + >> > + /* If Link Training fails, send a uevent to generate a hotplug */ >> > + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) >> > + drm_kms_helper_hotplug_event(encoder->base.dev); >> > + pll->config = tmp_pll_config; >> > +} >> > + >> > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, >> > bool has_hdmi_sink, >> > struct drm_display_mode *adjusted_mode, >> > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, >> > struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); >> > int type = intel_encoder->type; >> > >> > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { >> > + if (type == INTEL_OUTPUT_EDP) >> > + intel_ddi_pre_enable_edp(intel_encoder, >> > + crtc->config->port_clock, >> > + crtc->config->lane_count, >> > + crtc->config->shared_dpll); >> > + >> > + if (type == INTEL_OUTPUT_DP) >> > intel_ddi_pre_enable_dp(intel_encoder, >> > crtc->config->port_clock, >> > crtc->config->lane_count, >> > crtc->config->shared_dpll, >> > intel_crtc_has_type(crtc->config, >> > INTEL_OUTPUT_DP_MST)); >> > - } >> > - if (type == INTEL_OUTPUT_HDMI) { >> > + >> > + if (type == INTEL_OUTPUT_HDMI) >> > intel_ddi_pre_enable_hdmi(intel_encoder, >> > crtc->config->has_hdmi_sink, >> > &crtc->config->base.adjusted_mode, >> > crtc->config->shared_dpll); >> > - } >> > + >> > } >> > >> > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, >> > @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) >> > return pll; >> > } >> > >> > +bool >> > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, >> > + uint8_t max_lane_count, bool link_mst) >> > +{ >> > + struct intel_connector *connector = intel_dp->attached_connector; >> > + struct intel_encoder *encoder = connector->encoder; >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> > + struct intel_shared_dpll *pll; >> > + struct intel_shared_dpll_config tmp_pll_config; >> > + int link_rate, max_link_rate_index, link_rate_index; >> > + uint8_t lane_count; >> > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; >> > + bool ret = false; >> > + >> > + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, >> > + max_link_rate); >> > + if (max_link_rate_index < 0) { >> > + DRM_ERROR("Invalid Link Rate\n"); >> > + return false; >> > + } >> > + >> > + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { >> > + for (link_rate_index = max_link_rate_index; >> > + link_rate_index >= 0; link_rate_index--) { >> > + link_rate = common_rates[link_rate_index]; >> > + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); >> > + if (pll == NULL) { >> > + DRM_ERROR("Could not find DPLL for link training.\n"); >> > + return false; >> > + } >> > + tmp_pll_config = pll->config; >> > + pll->funcs.enable(dev_priv, pll); >> > + >> > + intel_dp_set_link_params(intel_dp, link_rate, >> > + lane_count, link_mst); >> > + >> > + intel_ddi_clk_select(encoder, pll); >> > + intel_prepare_dp_ddi_buffers(encoder); >> > + intel_ddi_init_dp_buf_reg(encoder); >> > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> > + ret = intel_dp_start_link_train(intel_dp); >> > + if (ret) >> > + break; >> > + >> > + /* Disable port followed by PLL for next >> > + *retry/clean up >> > + */ >> > + intel_ddi_post_disable(encoder, NULL, NULL); >> > + pll->funcs.disable(dev_priv, pll); >> > + pll->config = tmp_pll_config; >> > + } >> > + if (ret) { >> > + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", >> > + link_rate, lane_count); >> > + break; >> > + } >> > + } >> > + >> > + intel_dp_stop_link_train(intel_dp); >> > + >> > + if (!lane_count) >> > + DRM_ERROR("Link Training Failed\n"); >> > + >> > + return ret; >> > +} >> > + >> > void intel_ddi_init(struct drm_device *dev, enum port port) >> > { >> > struct drm_i915_private *dev_priv = to_i915(dev); >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index 69cee9b..d81c67cb 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) >> > return rates[len - 1]; >> > } >> > >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, >> > + int link_rate) >> > +{ >> > + int common_len; >> > + int index; >> > + >> > + common_len = intel_dp_common_rates(intel_dp, common_rates); >> > + for (index = 0; index < common_len; index++) { >> > + if (link_rate == common_rates[common_len - index - 1]) >> > + return common_len - index - 1; >> > + } >> > + >> > + return -1; >> > +} >> > + >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) >> > { >> > return rate_to_index(rate, intel_dp->sink_rates); >> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c >> > index c438b02..6eb5eb6 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c >> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c >> > @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) >> > DP_TRAINING_PATTERN_DISABLE); >> > } >> > >> > -void >> > +bool >> > intel_dp_start_link_train(struct intel_dp *intel_dp) >> > { >> > - intel_dp_link_training_clock_recovery(intel_dp); >> > - intel_dp_link_training_channel_equalization(intel_dp); >> > + bool ret; >> > + >> > + if (intel_dp_link_training_clock_recovery(intel_dp)) { >> > + ret = intel_dp_link_training_channel_equalization(intel_dp); >> > + if (ret) >> > + return true; >> > + } >> > + >> > + return false; >> > } >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> > index 8fd16ad..08cb571 100644 >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, >> > struct intel_crtc_state *pipe_config); >> > void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); >> > uint32_t ddi_signal_levels(struct intel_dp *intel_dp); >> > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, >> > + uint8_t max_lane_count, bool link_mst); >> > struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, >> > int clock); >> > unsigned int intel_fb_align_height(struct drm_device *dev, >> > @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> > void intel_dp_set_link_params(struct intel_dp *intel_dp, >> > int link_rate, uint8_t lane_count, >> > bool link_mst); >> > -void intel_dp_start_link_train(struct intel_dp *intel_dp); >> > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); >> > void intel_dp_stop_link_train(struct intel_dp *intel_dp); >> > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); >> > void intel_dp_encoder_reset(struct drm_encoder *encoder); >> > @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co >> > void intel_dp_mst_suspend(struct drm_device *dev); >> > void intel_dp_mst_resume(struct drm_device *dev); >> > int intel_dp_max_link_rate(struct intel_dp *intel_dp); >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, >> > + int link_rate); >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); >> > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); >> > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv); >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> > According to the DisplayPort Spec, in case of Clock Recovery failure > >> > the link training sequence should fall back to the lower link rate > >> > followed by lower lane count until CR succeeds. > >> > On CR success, the sequence proceeds with Channel EQ. > >> > In case of Channel EQ failures, it should fallback to > >> > lower link rate and lane count and start the CR phase again. > >> > >> This change makes the link training start at the max lane count and max > >> link rate. This is not ideal, as it wastes the link. And it is not a > >> spec requirement. "The Link Policy Maker of the upstream device may > >> choose any link count and link rate as long as they do not exceed the > >> capabilities of the DP receiver." > >> > >> Our current code starts at the minimum required bandwidth for the mode, > >> therefore we can't fall back to lower link rate and lane count without > >> reducing the mode. > >> > >> AFAICT this patch here makes it possible for the link bandwidth to drop > >> below what is required for the mode. This is unacceptable. > >> > >> BR, > >> Jani. > >> > >> > > > > Thanks Jani for your review comments. > > Yes in this change we start at the max link rate and lane count. This > > change was made according to the design document discussions we had > > before strating this DP Redesign project. The main reason for starting > > at the maxlink rate and max lane count was for ensuring proper > > behavior of DP MST. In case of DP MST, we want to train the link at > > the maximum supported link rate/lane count based on an early/ upfront > > link training result so that we dont fail when we try to connect a > > higher resolution monitor as a second monitor. This a trade off > > between wsting the link or higher power vs. needing to retrain for > > every monitor that requests a higher BW in case of DP MST. > > We already train at max bandwidth for DP MST, which seems to be the > sensible thing to do. > > > Actually this is also the reason for enabling upfront link training in > > the following patch where we train the link much ahead in the modeset > > sequence to understand the link rate and lane count values at which > > the link can be successfully trained and then the link training > > through modeset will always start at the upfront values (maximum > > supported values of lane count and link rate based on upfront link > > training). > > I don't see a need to do this for DP SST. > > > As per the CTS, all the test 4.3.1.4 requires that you fall back to > > the lower link rate after trying to train at the maximum link rate > > advertised through the DPCD registers. > > That test does not require the source DUT to default to maximum lane > count or link rate of the sink. The source may freely choose the lane > count and link rate as long as they don't exceed sink capabilities. > > For the purposes of the test, the test setup can request specific > parameters to be used, but that does not mean using maximum by > *default*. > > We currently lack the feature to reduce lane count and link rate. The > key to understand here is that starting at max and reducing down to the > sufficient parameters for the mode (which is where we start now) offers > no real benefit for any use case. What we're lacking is a feature to > reduce the link parameters *below* what's required by the mode the > userspace wants. This can only be achieved through cooperation with > userspace. > We can train at the optimal link rate required for the requested mode as done in the existing implementation and retrain whenever the link training test request is sent. For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the driver to fall back to even lower link rate. We do not implement this in the current driver and so this test fails. Could you elaborate on how this can be achieved with the the cooperation with userspace? Should we send a uevent to the userspace asking to retry at a lower resolution after retraining at the lower link rate? This is pertty much the place where majority of the compliance tests are failing. How can we pass compliance with regards to this feature? Regards Manasi > > This will not drop the link BW to a number below what is required for > > the mode because the requested modes are pruned or validated in > > intel_dp_mode_valid based on the upfront link training results in the > > following patch. And these values are used here as the starting values > > of link rate and lane count. > > Each patch must be a worthwhile change on its own. By my reading of this > patch, we can go under the required bandwidth. You can't justify that by > saying the follow-up patch fixes it. > > > I almost feel that the upfront link training patch and this patch should be > > combined so that insead of starting from the max link rate and lane count it > > is clear that we are starting from the upfront values. > > I am still reading and gathering more feedback on the upfront link > training patch. I will get back to you. But the impression I'm currently > getting is that we can't do this. The upfront link training patch was > originally written for USB type C. But if DP compliance has priority, > the order of business should be getting compliance without upfront link > training. I am also still not convinced upfront link training is > required for compliance. > > To be continued... > > BR, > Jani. > > > > > > > Regards, > > Manasi > >> > > >> > v7: > >> > * Address readability concerns (Mika Kahola) > >> > v6: > >> > * Do not split quoted string across line (Mika Kahola) > >> > v5: > >> > * Reset the link rate index to the max link rate index > >> > before lowering the lane count (Jani Nikula) > >> > * Use the paradigm for loop in intel_dp_link_rate_index > >> > v4: > >> > * Fixed the link rate fallback loop (Manasi Navare) > >> > v3: > >> > * Fixed some rebase issues (Mika Kahola) > >> > v2: > >> > * Add a helper function to return index of requested link rate > >> > into common_rates array > >> > * Changed the link rate fallback loop to make use > >> > of common_rates array (Mika Kahola) > >> > * Changed INTEL_INFO to INTEL_GEN (David Weinehall) > >> > > >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- > >> > drivers/gpu/drm/i915/intel_dp.c | 15 ++++ > >> > drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- > >> > drivers/gpu/drm/i915/intel_drv.h | 6 +- > >> > 4 files changed, 133 insertions(+), 14 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> > index 8065a5f..093038c 100644 > >> > --- a/drivers/gpu/drm/i915/intel_ddi.c > >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, > >> > } > >> > } > >> > > >> > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > >> > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, > >> > int link_rate, uint32_t lane_count, > >> > - struct intel_shared_dpll *pll, > >> > - bool link_mst) > >> > + struct intel_shared_dpll *pll) > >> > { > >> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> > enum port port = intel_ddi_get_encoder_port(encoder); > >> > > >> > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > >> > - link_mst); > >> > - if (encoder->type == INTEL_OUTPUT_EDP) > >> > - intel_edp_panel_on(intel_dp); > >> > + false); > >> > + > >> > + intel_edp_panel_on(intel_dp); > >> > > >> > intel_ddi_clk_select(encoder, pll); > >> > intel_prepare_dp_ddi_buffers(encoder); > >> > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > >> > intel_dp_stop_link_train(intel_dp); > >> > } > >> > > >> > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > >> > + int link_rate, uint32_t lane_count, > >> > + struct intel_shared_dpll *pll, > >> > + bool link_mst) > >> > +{ > >> > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> > + struct intel_shared_dpll_config tmp_pll_config; > >> > + > >> > + /* Disable the PLL and obtain the PLL for Link Training > >> > + * that starts with highest link rate and lane count. > >> > + */ > >> > + tmp_pll_config = pll->config; > >> > + pll->funcs.disable(dev_priv, pll); > >> > + pll->config.crtc_mask = 0; > >> > + > >> > + /* If Link Training fails, send a uevent to generate a hotplug */ > >> > + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) > >> > + drm_kms_helper_hotplug_event(encoder->base.dev); > >> > + pll->config = tmp_pll_config; > >> > +} > >> > + > >> > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > >> > bool has_hdmi_sink, > >> > struct drm_display_mode *adjusted_mode, > >> > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, > >> > struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); > >> > int type = intel_encoder->type; > >> > > >> > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { > >> > + if (type == INTEL_OUTPUT_EDP) > >> > + intel_ddi_pre_enable_edp(intel_encoder, > >> > + crtc->config->port_clock, > >> > + crtc->config->lane_count, > >> > + crtc->config->shared_dpll); > >> > + > >> > + if (type == INTEL_OUTPUT_DP) > >> > intel_ddi_pre_enable_dp(intel_encoder, > >> > crtc->config->port_clock, > >> > crtc->config->lane_count, > >> > crtc->config->shared_dpll, > >> > intel_crtc_has_type(crtc->config, > >> > INTEL_OUTPUT_DP_MST)); > >> > - } > >> > - if (type == INTEL_OUTPUT_HDMI) { > >> > + > >> > + if (type == INTEL_OUTPUT_HDMI) > >> > intel_ddi_pre_enable_hdmi(intel_encoder, > >> > crtc->config->has_hdmi_sink, > >> > &crtc->config->base.adjusted_mode, > >> > crtc->config->shared_dpll); > >> > - } > >> > + > >> > } > >> > > >> > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, > >> > @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) > >> > return pll; > >> > } > >> > > >> > +bool > >> > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > >> > + uint8_t max_lane_count, bool link_mst) > >> > +{ > >> > + struct intel_connector *connector = intel_dp->attached_connector; > >> > + struct intel_encoder *encoder = connector->encoder; > >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> > + struct intel_shared_dpll *pll; > >> > + struct intel_shared_dpll_config tmp_pll_config; > >> > + int link_rate, max_link_rate_index, link_rate_index; > >> > + uint8_t lane_count; > >> > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > >> > + bool ret = false; > >> > + > >> > + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, > >> > + max_link_rate); > >> > + if (max_link_rate_index < 0) { > >> > + DRM_ERROR("Invalid Link Rate\n"); > >> > + return false; > >> > + } > >> > + > >> > + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { > >> > + for (link_rate_index = max_link_rate_index; > >> > + link_rate_index >= 0; link_rate_index--) { > >> > + link_rate = common_rates[link_rate_index]; > >> > + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); > >> > + if (pll == NULL) { > >> > + DRM_ERROR("Could not find DPLL for link training.\n"); > >> > + return false; > >> > + } > >> > + tmp_pll_config = pll->config; > >> > + pll->funcs.enable(dev_priv, pll); > >> > + > >> > + intel_dp_set_link_params(intel_dp, link_rate, > >> > + lane_count, link_mst); > >> > + > >> > + intel_ddi_clk_select(encoder, pll); > >> > + intel_prepare_dp_ddi_buffers(encoder); > >> > + intel_ddi_init_dp_buf_reg(encoder); > >> > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > >> > + ret = intel_dp_start_link_train(intel_dp); > >> > + if (ret) > >> > + break; > >> > + > >> > + /* Disable port followed by PLL for next > >> > + *retry/clean up > >> > + */ > >> > + intel_ddi_post_disable(encoder, NULL, NULL); > >> > + pll->funcs.disable(dev_priv, pll); > >> > + pll->config = tmp_pll_config; > >> > + } > >> > + if (ret) { > >> > + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", > >> > + link_rate, lane_count); > >> > + break; > >> > + } > >> > + } > >> > + > >> > + intel_dp_stop_link_train(intel_dp); > >> > + > >> > + if (!lane_count) > >> > + DRM_ERROR("Link Training Failed\n"); > >> > + > >> > + return ret; > >> > +} > >> > + > >> > void intel_ddi_init(struct drm_device *dev, enum port port) > >> > { > >> > struct drm_i915_private *dev_priv = to_i915(dev); > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> > index 69cee9b..d81c67cb 100644 > >> > --- a/drivers/gpu/drm/i915/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/intel_dp.c > >> > @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) > >> > return rates[len - 1]; > >> > } > >> > > >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > >> > + int link_rate) > >> > +{ > >> > + int common_len; > >> > + int index; > >> > + > >> > + common_len = intel_dp_common_rates(intel_dp, common_rates); > >> > + for (index = 0; index < common_len; index++) { > >> > + if (link_rate == common_rates[common_len - index - 1]) > >> > + return common_len - index - 1; > >> > + } > >> > + > >> > + return -1; > >> > +} > >> > + > >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) > >> > { > >> > return rate_to_index(rate, intel_dp->sink_rates); > >> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > >> > index c438b02..6eb5eb6 100644 > >> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > >> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > >> > @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > >> > DP_TRAINING_PATTERN_DISABLE); > >> > } > >> > > >> > -void > >> > +bool > >> > intel_dp_start_link_train(struct intel_dp *intel_dp) > >> > { > >> > - intel_dp_link_training_clock_recovery(intel_dp); > >> > - intel_dp_link_training_channel_equalization(intel_dp); > >> > + bool ret; > >> > + > >> > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > >> > + ret = intel_dp_link_training_channel_equalization(intel_dp); > >> > + if (ret) > >> > + return true; > >> > + } > >> > + > >> > + return false; > >> > } > >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> > index 8fd16ad..08cb571 100644 > >> > --- a/drivers/gpu/drm/i915/intel_drv.h > >> > +++ b/drivers/gpu/drm/i915/intel_drv.h > >> > @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, > >> > struct intel_crtc_state *pipe_config); > >> > void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); > >> > uint32_t ddi_signal_levels(struct intel_dp *intel_dp); > >> > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, > >> > + uint8_t max_lane_count, bool link_mst); > >> > struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, > >> > int clock); > >> > unsigned int intel_fb_align_height(struct drm_device *dev, > >> > @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > >> > void intel_dp_set_link_params(struct intel_dp *intel_dp, > >> > int link_rate, uint8_t lane_count, > >> > bool link_mst); > >> > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > >> > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > >> > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > >> > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > >> > void intel_dp_encoder_reset(struct drm_encoder *encoder); > >> > @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co > >> > void intel_dp_mst_suspend(struct drm_device *dev); > >> > void intel_dp_mst_resume(struct drm_device *dev); > >> > int intel_dp_max_link_rate(struct intel_dp *intel_dp); > >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, > >> > + int link_rate); > >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); > >> > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); > >> > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv); > >> > >> -- > >> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center
On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure >> >> > the link training sequence should fall back to the lower link rate >> >> > followed by lower lane count until CR succeeds. >> >> > On CR success, the sequence proceeds with Channel EQ. >> >> > In case of Channel EQ failures, it should fallback to >> >> > lower link rate and lane count and start the CR phase again. >> >> >> >> This change makes the link training start at the max lane count and max >> >> link rate. This is not ideal, as it wastes the link. And it is not a >> >> spec requirement. "The Link Policy Maker of the upstream device may >> >> choose any link count and link rate as long as they do not exceed the >> >> capabilities of the DP receiver." >> >> >> >> Our current code starts at the minimum required bandwidth for the mode, >> >> therefore we can't fall back to lower link rate and lane count without >> >> reducing the mode. >> >> >> >> AFAICT this patch here makes it possible for the link bandwidth to drop >> >> below what is required for the mode. This is unacceptable. >> >> >> >> BR, >> >> Jani. >> >> >> >> >> > >> > Thanks Jani for your review comments. >> > Yes in this change we start at the max link rate and lane count. This >> > change was made according to the design document discussions we had >> > before strating this DP Redesign project. The main reason for starting >> > at the maxlink rate and max lane count was for ensuring proper >> > behavior of DP MST. In case of DP MST, we want to train the link at >> > the maximum supported link rate/lane count based on an early/ upfront >> > link training result so that we dont fail when we try to connect a >> > higher resolution monitor as a second monitor. This a trade off >> > between wsting the link or higher power vs. needing to retrain for >> > every monitor that requests a higher BW in case of DP MST. >> >> We already train at max bandwidth for DP MST, which seems to be the >> sensible thing to do. >> >> > Actually this is also the reason for enabling upfront link training in >> > the following patch where we train the link much ahead in the modeset >> > sequence to understand the link rate and lane count values at which >> > the link can be successfully trained and then the link training >> > through modeset will always start at the upfront values (maximum >> > supported values of lane count and link rate based on upfront link >> > training). >> >> I don't see a need to do this for DP SST. >> >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to >> > the lower link rate after trying to train at the maximum link rate >> > advertised through the DPCD registers. >> >> That test does not require the source DUT to default to maximum lane >> count or link rate of the sink. The source may freely choose the lane >> count and link rate as long as they don't exceed sink capabilities. >> >> For the purposes of the test, the test setup can request specific >> parameters to be used, but that does not mean using maximum by >> *default*. >> >> We currently lack the feature to reduce lane count and link rate. The >> key to understand here is that starting at max and reducing down to the >> sufficient parameters for the mode (which is where we start now) offers >> no real benefit for any use case. What we're lacking is a feature to >> reduce the link parameters *below* what's required by the mode the >> userspace wants. This can only be achieved through cooperation with >> userspace. >> > > We can train at the optimal link rate required for the requested mode as > done in the existing implementation and retrain whenever the link training > test request is sent. > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > driver to fall back to even lower link rate. We do not implement this in the > current driver and so this test fails. Could you elaborate on how this can > be achieved with the the cooperation with userspace? > Should we send a uevent to the userspace asking to retry at a lower resolution > after retraining at the lower link rate? > This is pertty much the place where majority of the compliance tests are failing. > How can we pass compliance with regards to this feature? So here's an idea Ville and I came up with. It's not completely thought out yet, probably has some wrinkles still, but then there are wrinkles with the upfront link training too (I'll get back to those separately). If link training fails during modeset (either for real or because it's a test sink that wants to test failures), we 1) store the link parameters as failing, 2) send a uevent to userspace, hopefully getting the userspace to do another get modes and try again, 3) propage errors from modeset. When the userspace asks for the modes again, we can prune the modes that require using the parameters that failed. If the link training fails again, we repeat the steps. When we detect long hpd, we drop the list of failing modes, so we can start from scratch (it could be another display or another cable, etc.). This same approach could be used with sink issued link status checks when the link has degraded during operation. Ville, anything to add to that? BR, Jani. > > Regards > Manasi >> > This will not drop the link BW to a number below what is required for >> > the mode because the requested modes are pruned or validated in >> > intel_dp_mode_valid based on the upfront link training results in the >> > following patch. And these values are used here as the starting values >> > of link rate and lane count. >> >> Each patch must be a worthwhile change on its own. By my reading of this >> patch, we can go under the required bandwidth. You can't justify that by >> saying the follow-up patch fixes it. >> >> > I almost feel that the upfront link training patch and this patch should be >> > combined so that insead of starting from the max link rate and lane count it >> > is clear that we are starting from the upfront values. >> >> I am still reading and gathering more feedback on the upfront link >> training patch. I will get back to you. But the impression I'm currently >> getting is that we can't do this. The upfront link training patch was >> originally written for USB type C. But if DP compliance has priority, >> the order of business should be getting compliance without upfront link >> training. I am also still not convinced upfront link training is >> required for compliance. >> >> To be continued... >> >> BR, >> Jani. >> >> >> >> > >> > Regards, >> > Manasi >> >> > >> >> > v7: >> >> > * Address readability concerns (Mika Kahola) >> >> > v6: >> >> > * Do not split quoted string across line (Mika Kahola) >> >> > v5: >> >> > * Reset the link rate index to the max link rate index >> >> > before lowering the lane count (Jani Nikula) >> >> > * Use the paradigm for loop in intel_dp_link_rate_index >> >> > v4: >> >> > * Fixed the link rate fallback loop (Manasi Navare) >> >> > v3: >> >> > * Fixed some rebase issues (Mika Kahola) >> >> > v2: >> >> > * Add a helper function to return index of requested link rate >> >> > into common_rates array >> >> > * Changed the link rate fallback loop to make use >> >> > of common_rates array (Mika Kahola) >> >> > * Changed INTEL_INFO to INTEL_GEN (David Weinehall) >> >> > >> >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- >> >> > drivers/gpu/drm/i915/intel_dp.c | 15 ++++ >> >> > drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- >> >> > drivers/gpu/drm/i915/intel_drv.h | 6 +- >> >> > 4 files changed, 133 insertions(+), 14 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> >> > index 8065a5f..093038c 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> >> > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, >> >> > } >> >> > } >> >> > >> >> > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> >> > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, >> >> > int link_rate, uint32_t lane_count, >> >> > - struct intel_shared_dpll *pll, >> >> > - bool link_mst) >> >> > + struct intel_shared_dpll *pll) >> >> > { >> >> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> >> > enum port port = intel_ddi_get_encoder_port(encoder); >> >> > >> >> > intel_dp_set_link_params(intel_dp, link_rate, lane_count, >> >> > - link_mst); >> >> > - if (encoder->type == INTEL_OUTPUT_EDP) >> >> > - intel_edp_panel_on(intel_dp); >> >> > + false); >> >> > + >> >> > + intel_edp_panel_on(intel_dp); >> >> > >> >> > intel_ddi_clk_select(encoder, pll); >> >> > intel_prepare_dp_ddi_buffers(encoder); >> >> > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> >> > intel_dp_stop_link_train(intel_dp); >> >> > } >> >> > >> >> > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >> >> > + int link_rate, uint32_t lane_count, >> >> > + struct intel_shared_dpll *pll, >> >> > + bool link_mst) >> >> > +{ >> >> > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> >> > + struct intel_shared_dpll_config tmp_pll_config; >> >> > + >> >> > + /* Disable the PLL and obtain the PLL for Link Training >> >> > + * that starts with highest link rate and lane count. >> >> > + */ >> >> > + tmp_pll_config = pll->config; >> >> > + pll->funcs.disable(dev_priv, pll); >> >> > + pll->config.crtc_mask = 0; >> >> > + >> >> > + /* If Link Training fails, send a uevent to generate a hotplug */ >> >> > + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) >> >> > + drm_kms_helper_hotplug_event(encoder->base.dev); >> >> > + pll->config = tmp_pll_config; >> >> > +} >> >> > + >> >> > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, >> >> > bool has_hdmi_sink, >> >> > struct drm_display_mode *adjusted_mode, >> >> > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, >> >> > struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); >> >> > int type = intel_encoder->type; >> >> > >> >> > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { >> >> > + if (type == INTEL_OUTPUT_EDP) >> >> > + intel_ddi_pre_enable_edp(intel_encoder, >> >> > + crtc->config->port_clock, >> >> > + crtc->config->lane_count, >> >> > + crtc->config->shared_dpll); >> >> > + >> >> > + if (type == INTEL_OUTPUT_DP) >> >> > intel_ddi_pre_enable_dp(intel_encoder, >> >> > crtc->config->port_clock, >> >> > crtc->config->lane_count, >> >> > crtc->config->shared_dpll, >> >> > intel_crtc_has_type(crtc->config, >> >> > INTEL_OUTPUT_DP_MST)); >> >> > - } >> >> > - if (type == INTEL_OUTPUT_HDMI) { >> >> > + >> >> > + if (type == INTEL_OUTPUT_HDMI) >> >> > intel_ddi_pre_enable_hdmi(intel_encoder, >> >> > crtc->config->has_hdmi_sink, >> >> > &crtc->config->base.adjusted_mode, >> >> > crtc->config->shared_dpll); >> >> > - } >> >> > + >> >> > } >> >> > >> >> > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, >> >> > @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) >> >> > return pll; >> >> > } >> >> > >> >> > +bool >> >> > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, >> >> > + uint8_t max_lane_count, bool link_mst) >> >> > +{ >> >> > + struct intel_connector *connector = intel_dp->attached_connector; >> >> > + struct intel_encoder *encoder = connector->encoder; >> >> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> >> > + struct intel_shared_dpll *pll; >> >> > + struct intel_shared_dpll_config tmp_pll_config; >> >> > + int link_rate, max_link_rate_index, link_rate_index; >> >> > + uint8_t lane_count; >> >> > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; >> >> > + bool ret = false; >> >> > + >> >> > + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, >> >> > + max_link_rate); >> >> > + if (max_link_rate_index < 0) { >> >> > + DRM_ERROR("Invalid Link Rate\n"); >> >> > + return false; >> >> > + } >> >> > + >> >> > + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { >> >> > + for (link_rate_index = max_link_rate_index; >> >> > + link_rate_index >= 0; link_rate_index--) { >> >> > + link_rate = common_rates[link_rate_index]; >> >> > + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); >> >> > + if (pll == NULL) { >> >> > + DRM_ERROR("Could not find DPLL for link training.\n"); >> >> > + return false; >> >> > + } >> >> > + tmp_pll_config = pll->config; >> >> > + pll->funcs.enable(dev_priv, pll); >> >> > + >> >> > + intel_dp_set_link_params(intel_dp, link_rate, >> >> > + lane_count, link_mst); >> >> > + >> >> > + intel_ddi_clk_select(encoder, pll); >> >> > + intel_prepare_dp_ddi_buffers(encoder); >> >> > + intel_ddi_init_dp_buf_reg(encoder); >> >> > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> >> > + ret = intel_dp_start_link_train(intel_dp); >> >> > + if (ret) >> >> > + break; >> >> > + >> >> > + /* Disable port followed by PLL for next >> >> > + *retry/clean up >> >> > + */ >> >> > + intel_ddi_post_disable(encoder, NULL, NULL); >> >> > + pll->funcs.disable(dev_priv, pll); >> >> > + pll->config = tmp_pll_config; >> >> > + } >> >> > + if (ret) { >> >> > + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", >> >> > + link_rate, lane_count); >> >> > + break; >> >> > + } >> >> > + } >> >> > + >> >> > + intel_dp_stop_link_train(intel_dp); >> >> > + >> >> > + if (!lane_count) >> >> > + DRM_ERROR("Link Training Failed\n"); >> >> > + >> >> > + return ret; >> >> > +} >> >> > + >> >> > void intel_ddi_init(struct drm_device *dev, enum port port) >> >> > { >> >> > struct drm_i915_private *dev_priv = to_i915(dev); >> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> >> > index 69cee9b..d81c67cb 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> >> > @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) >> >> > return rates[len - 1]; >> >> > } >> >> > >> >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, >> >> > + int link_rate) >> >> > +{ >> >> > + int common_len; >> >> > + int index; >> >> > + >> >> > + common_len = intel_dp_common_rates(intel_dp, common_rates); >> >> > + for (index = 0; index < common_len; index++) { >> >> > + if (link_rate == common_rates[common_len - index - 1]) >> >> > + return common_len - index - 1; >> >> > + } >> >> > + >> >> > + return -1; >> >> > +} >> >> > + >> >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) >> >> > { >> >> > return rate_to_index(rate, intel_dp->sink_rates); >> >> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c >> >> > index c438b02..6eb5eb6 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c >> >> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c >> >> > @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) >> >> > DP_TRAINING_PATTERN_DISABLE); >> >> > } >> >> > >> >> > -void >> >> > +bool >> >> > intel_dp_start_link_train(struct intel_dp *intel_dp) >> >> > { >> >> > - intel_dp_link_training_clock_recovery(intel_dp); >> >> > - intel_dp_link_training_channel_equalization(intel_dp); >> >> > + bool ret; >> >> > + >> >> > + if (intel_dp_link_training_clock_recovery(intel_dp)) { >> >> > + ret = intel_dp_link_training_channel_equalization(intel_dp); >> >> > + if (ret) >> >> > + return true; >> >> > + } >> >> > + >> >> > + return false; >> >> > } >> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> >> > index 8fd16ad..08cb571 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> >> > @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, >> >> > struct intel_crtc_state *pipe_config); >> >> > void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); >> >> > uint32_t ddi_signal_levels(struct intel_dp *intel_dp); >> >> > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, >> >> > + uint8_t max_lane_count, bool link_mst); >> >> > struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, >> >> > int clock); >> >> > unsigned int intel_fb_align_height(struct drm_device *dev, >> >> > @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> >> > void intel_dp_set_link_params(struct intel_dp *intel_dp, >> >> > int link_rate, uint8_t lane_count, >> >> > bool link_mst); >> >> > -void intel_dp_start_link_train(struct intel_dp *intel_dp); >> >> > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); >> >> > void intel_dp_stop_link_train(struct intel_dp *intel_dp); >> >> > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); >> >> > void intel_dp_encoder_reset(struct drm_encoder *encoder); >> >> > @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co >> >> > void intel_dp_mst_suspend(struct drm_device *dev); >> >> > void intel_dp_mst_resume(struct drm_device *dev); >> >> > int intel_dp_max_link_rate(struct intel_dp *intel_dp); >> >> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, >> >> > + int link_rate); >> >> > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); >> >> > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); >> >> > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv); >> >> >> >> -- >> >> Jani Nikula, Intel Open Source Technology Center >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure > >> >> > the link training sequence should fall back to the lower link rate > >> >> > followed by lower lane count until CR succeeds. > >> >> > On CR success, the sequence proceeds with Channel EQ. > >> >> > In case of Channel EQ failures, it should fallback to > >> >> > lower link rate and lane count and start the CR phase again. > >> >> > >> >> This change makes the link training start at the max lane count and max > >> >> link rate. This is not ideal, as it wastes the link. And it is not a > >> >> spec requirement. "The Link Policy Maker of the upstream device may > >> >> choose any link count and link rate as long as they do not exceed the > >> >> capabilities of the DP receiver." > >> >> > >> >> Our current code starts at the minimum required bandwidth for the mode, > >> >> therefore we can't fall back to lower link rate and lane count without > >> >> reducing the mode. > >> >> > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop > >> >> below what is required for the mode. This is unacceptable. > >> >> > >> >> BR, > >> >> Jani. > >> >> > >> >> > >> > > >> > Thanks Jani for your review comments. > >> > Yes in this change we start at the max link rate and lane count. This > >> > change was made according to the design document discussions we had > >> > before strating this DP Redesign project. The main reason for starting > >> > at the maxlink rate and max lane count was for ensuring proper > >> > behavior of DP MST. In case of DP MST, we want to train the link at > >> > the maximum supported link rate/lane count based on an early/ upfront > >> > link training result so that we dont fail when we try to connect a > >> > higher resolution monitor as a second monitor. This a trade off > >> > between wsting the link or higher power vs. needing to retrain for > >> > every monitor that requests a higher BW in case of DP MST. > >> > >> We already train at max bandwidth for DP MST, which seems to be the > >> sensible thing to do. > >> > >> > Actually this is also the reason for enabling upfront link training in > >> > the following patch where we train the link much ahead in the modeset > >> > sequence to understand the link rate and lane count values at which > >> > the link can be successfully trained and then the link training > >> > through modeset will always start at the upfront values (maximum > >> > supported values of lane count and link rate based on upfront link > >> > training). > >> > >> I don't see a need to do this for DP SST. > >> > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to > >> > the lower link rate after trying to train at the maximum link rate > >> > advertised through the DPCD registers. > >> > >> That test does not require the source DUT to default to maximum lane > >> count or link rate of the sink. The source may freely choose the lane > >> count and link rate as long as they don't exceed sink capabilities. > >> > >> For the purposes of the test, the test setup can request specific > >> parameters to be used, but that does not mean using maximum by > >> *default*. > >> > >> We currently lack the feature to reduce lane count and link rate. The > >> key to understand here is that starting at max and reducing down to the > >> sufficient parameters for the mode (which is where we start now) offers > >> no real benefit for any use case. What we're lacking is a feature to > >> reduce the link parameters *below* what's required by the mode the > >> userspace wants. This can only be achieved through cooperation with > >> userspace. > >> > > > > We can train at the optimal link rate required for the requested mode as > > done in the existing implementation and retrain whenever the link training > > test request is sent. > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > > driver to fall back to even lower link rate. We do not implement this in the > > current driver and so this test fails. Could you elaborate on how this can > > be achieved with the the cooperation with userspace? > > Should we send a uevent to the userspace asking to retry at a lower resolution > > after retraining at the lower link rate? > > This is pertty much the place where majority of the compliance tests are failing. > > How can we pass compliance with regards to this feature? > > So here's an idea Ville and I came up with. It's not completely thought > out yet, probably has some wrinkles still, but then there are wrinkles > with the upfront link training too (I'll get back to those separately). > > If link training fails during modeset (either for real or because it's a > test sink that wants to test failures), we 1) store the link parameters > as failing, 2) send a uevent to userspace, hopefully getting the > userspace to do another get modes and try again, 3) propage errors from > modeset. userspace already tries to do a reprobe after a setcrtc fails, to try and gracefully handle the race between hotplug being in its event queue and performing setcrtc, i.e. I think the error is enough. -Chris
On Thu, Sep 29, 2016 at 12:44:19PM +0100, Chris Wilson wrote: > On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: > > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure > > >> >> > the link training sequence should fall back to the lower link rate > > >> >> > followed by lower lane count until CR succeeds. > > >> >> > On CR success, the sequence proceeds with Channel EQ. > > >> >> > In case of Channel EQ failures, it should fallback to > > >> >> > lower link rate and lane count and start the CR phase again. > > >> >> > > >> >> This change makes the link training start at the max lane count and max > > >> >> link rate. This is not ideal, as it wastes the link. And it is not a > > >> >> spec requirement. "The Link Policy Maker of the upstream device may > > >> >> choose any link count and link rate as long as they do not exceed the > > >> >> capabilities of the DP receiver." > > >> >> > > >> >> Our current code starts at the minimum required bandwidth for the mode, > > >> >> therefore we can't fall back to lower link rate and lane count without > > >> >> reducing the mode. > > >> >> > > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop > > >> >> below what is required for the mode. This is unacceptable. > > >> >> > > >> >> BR, > > >> >> Jani. > > >> >> > > >> >> > > >> > > > >> > Thanks Jani for your review comments. > > >> > Yes in this change we start at the max link rate and lane count. This > > >> > change was made according to the design document discussions we had > > >> > before strating this DP Redesign project. The main reason for starting > > >> > at the maxlink rate and max lane count was for ensuring proper > > >> > behavior of DP MST. In case of DP MST, we want to train the link at > > >> > the maximum supported link rate/lane count based on an early/ upfront > > >> > link training result so that we dont fail when we try to connect a > > >> > higher resolution monitor as a second monitor. This a trade off > > >> > between wsting the link or higher power vs. needing to retrain for > > >> > every monitor that requests a higher BW in case of DP MST. > > >> > > >> We already train at max bandwidth for DP MST, which seems to be the > > >> sensible thing to do. > > >> > > >> > Actually this is also the reason for enabling upfront link training in > > >> > the following patch where we train the link much ahead in the modeset > > >> > sequence to understand the link rate and lane count values at which > > >> > the link can be successfully trained and then the link training > > >> > through modeset will always start at the upfront values (maximum > > >> > supported values of lane count and link rate based on upfront link > > >> > training). > > >> > > >> I don't see a need to do this for DP SST. > > >> > > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to > > >> > the lower link rate after trying to train at the maximum link rate > > >> > advertised through the DPCD registers. > > >> > > >> That test does not require the source DUT to default to maximum lane > > >> count or link rate of the sink. The source may freely choose the lane > > >> count and link rate as long as they don't exceed sink capabilities. > > >> > > >> For the purposes of the test, the test setup can request specific > > >> parameters to be used, but that does not mean using maximum by > > >> *default*. > > >> > > >> We currently lack the feature to reduce lane count and link rate. The > > >> key to understand here is that starting at max and reducing down to the > > >> sufficient parameters for the mode (which is where we start now) offers > > >> no real benefit for any use case. What we're lacking is a feature to > > >> reduce the link parameters *below* what's required by the mode the > > >> userspace wants. This can only be achieved through cooperation with > > >> userspace. > > >> > > > > > > We can train at the optimal link rate required for the requested mode as > > > done in the existing implementation and retrain whenever the link training > > > test request is sent. > > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > > > driver to fall back to even lower link rate. We do not implement this in the > > > current driver and so this test fails. Could you elaborate on how this can > > > be achieved with the the cooperation with userspace? > > > Should we send a uevent to the userspace asking to retry at a lower resolution > > > after retraining at the lower link rate? > > > This is pertty much the place where majority of the compliance tests are failing. > > > How can we pass compliance with regards to this feature? > > > > So here's an idea Ville and I came up with. It's not completely thought > > out yet, probably has some wrinkles still, but then there are wrinkles > > with the upfront link training too (I'll get back to those separately). > > > > If link training fails during modeset (either for real or because it's a > > test sink that wants to test failures), we 1) store the link parameters > > as failing, 2) send a uevent to userspace, hopefully getting the > > userspace to do another get modes and try again, 3) propage errors from > > modeset. > > userspace already tries to do a reprobe after a setcrtc fails, to try > and gracefully handle the race between hotplug being in its event queue > and performing setcrtc, i.e. I think the error is enough. I presume we want the modeset to be async, so by the time we notice the problem we're no longer in the ioctl.
On Thu, 29 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Sep 29, 2016 at 12:44:19PM +0100, Chris Wilson wrote: >> On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: >> > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: >> > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: >> > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure >> > >> >> > the link training sequence should fall back to the lower link rate >> > >> >> > followed by lower lane count until CR succeeds. >> > >> >> > On CR success, the sequence proceeds with Channel EQ. >> > >> >> > In case of Channel EQ failures, it should fallback to >> > >> >> > lower link rate and lane count and start the CR phase again. >> > >> >> >> > >> >> This change makes the link training start at the max lane count and max >> > >> >> link rate. This is not ideal, as it wastes the link. And it is not a >> > >> >> spec requirement. "The Link Policy Maker of the upstream device may >> > >> >> choose any link count and link rate as long as they do not exceed the >> > >> >> capabilities of the DP receiver." >> > >> >> >> > >> >> Our current code starts at the minimum required bandwidth for the mode, >> > >> >> therefore we can't fall back to lower link rate and lane count without >> > >> >> reducing the mode. >> > >> >> >> > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop >> > >> >> below what is required for the mode. This is unacceptable. >> > >> >> >> > >> >> BR, >> > >> >> Jani. >> > >> >> >> > >> >> >> > >> > >> > >> > Thanks Jani for your review comments. >> > >> > Yes in this change we start at the max link rate and lane count. This >> > >> > change was made according to the design document discussions we had >> > >> > before strating this DP Redesign project. The main reason for starting >> > >> > at the maxlink rate and max lane count was for ensuring proper >> > >> > behavior of DP MST. In case of DP MST, we want to train the link at >> > >> > the maximum supported link rate/lane count based on an early/ upfront >> > >> > link training result so that we dont fail when we try to connect a >> > >> > higher resolution monitor as a second monitor. This a trade off >> > >> > between wsting the link or higher power vs. needing to retrain for >> > >> > every monitor that requests a higher BW in case of DP MST. >> > >> >> > >> We already train at max bandwidth for DP MST, which seems to be the >> > >> sensible thing to do. >> > >> >> > >> > Actually this is also the reason for enabling upfront link training in >> > >> > the following patch where we train the link much ahead in the modeset >> > >> > sequence to understand the link rate and lane count values at which >> > >> > the link can be successfully trained and then the link training >> > >> > through modeset will always start at the upfront values (maximum >> > >> > supported values of lane count and link rate based on upfront link >> > >> > training). >> > >> >> > >> I don't see a need to do this for DP SST. >> > >> >> > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to >> > >> > the lower link rate after trying to train at the maximum link rate >> > >> > advertised through the DPCD registers. >> > >> >> > >> That test does not require the source DUT to default to maximum lane >> > >> count or link rate of the sink. The source may freely choose the lane >> > >> count and link rate as long as they don't exceed sink capabilities. >> > >> >> > >> For the purposes of the test, the test setup can request specific >> > >> parameters to be used, but that does not mean using maximum by >> > >> *default*. >> > >> >> > >> We currently lack the feature to reduce lane count and link rate. The >> > >> key to understand here is that starting at max and reducing down to the >> > >> sufficient parameters for the mode (which is where we start now) offers >> > >> no real benefit for any use case. What we're lacking is a feature to >> > >> reduce the link parameters *below* what's required by the mode the >> > >> userspace wants. This can only be achieved through cooperation with >> > >> userspace. >> > >> >> > > >> > > We can train at the optimal link rate required for the requested mode as >> > > done in the existing implementation and retrain whenever the link training >> > > test request is sent. >> > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the >> > > driver to fall back to even lower link rate. We do not implement this in the >> > > current driver and so this test fails. Could you elaborate on how this can >> > > be achieved with the the cooperation with userspace? >> > > Should we send a uevent to the userspace asking to retry at a lower resolution >> > > after retraining at the lower link rate? >> > > This is pertty much the place where majority of the compliance tests are failing. >> > > How can we pass compliance with regards to this feature? >> > >> > So here's an idea Ville and I came up with. It's not completely thought >> > out yet, probably has some wrinkles still, but then there are wrinkles >> > with the upfront link training too (I'll get back to those separately). >> > >> > If link training fails during modeset (either for real or because it's a >> > test sink that wants to test failures), we 1) store the link parameters >> > as failing, 2) send a uevent to userspace, hopefully getting the >> > userspace to do another get modes and try again, 3) propage errors from >> > modeset. >> >> userspace already tries to do a reprobe after a setcrtc fails, to try >> and gracefully handle the race between hotplug being in its event queue >> and performing setcrtc, i.e. I think the error is enough. > > I presume we want the modeset to be async, so by the time we notice the > problem we're no longer in the ioctl. IOW, we'll just need to send the hotplug uevent anyway. BR, Jani.
On Thu, Sep 29, 2016 at 06:48:43PM +0300, Jani Nikula wrote: > On Thu, 29 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Sep 29, 2016 at 12:44:19PM +0100, Chris Wilson wrote: > >> On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: > >> > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > >> > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > >> > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > >> > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure > >> > >> >> > the link training sequence should fall back to the lower link rate > >> > >> >> > followed by lower lane count until CR succeeds. > >> > >> >> > On CR success, the sequence proceeds with Channel EQ. > >> > >> >> > In case of Channel EQ failures, it should fallback to > >> > >> >> > lower link rate and lane count and start the CR phase again. > >> > >> >> > >> > >> >> This change makes the link training start at the max lane count and max > >> > >> >> link rate. This is not ideal, as it wastes the link. And it is not a > >> > >> >> spec requirement. "The Link Policy Maker of the upstream device may > >> > >> >> choose any link count and link rate as long as they do not exceed the > >> > >> >> capabilities of the DP receiver." > >> > >> >> > >> > >> >> Our current code starts at the minimum required bandwidth for the mode, > >> > >> >> therefore we can't fall back to lower link rate and lane count without > >> > >> >> reducing the mode. > >> > >> >> > >> > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop > >> > >> >> below what is required for the mode. This is unacceptable. > >> > >> >> > >> > >> >> BR, > >> > >> >> Jani. > >> > >> >> > >> > >> >> > >> > >> > > >> > >> > Thanks Jani for your review comments. > >> > >> > Yes in this change we start at the max link rate and lane count. This > >> > >> > change was made according to the design document discussions we had > >> > >> > before strating this DP Redesign project. The main reason for starting > >> > >> > at the maxlink rate and max lane count was for ensuring proper > >> > >> > behavior of DP MST. In case of DP MST, we want to train the link at > >> > >> > the maximum supported link rate/lane count based on an early/ upfront > >> > >> > link training result so that we dont fail when we try to connect a > >> > >> > higher resolution monitor as a second monitor. This a trade off > >> > >> > between wsting the link or higher power vs. needing to retrain for > >> > >> > every monitor that requests a higher BW in case of DP MST. > >> > >> > >> > >> We already train at max bandwidth for DP MST, which seems to be the > >> > >> sensible thing to do. > >> > >> > >> > >> > Actually this is also the reason for enabling upfront link training in > >> > >> > the following patch where we train the link much ahead in the modeset > >> > >> > sequence to understand the link rate and lane count values at which > >> > >> > the link can be successfully trained and then the link training > >> > >> > through modeset will always start at the upfront values (maximum > >> > >> > supported values of lane count and link rate based on upfront link > >> > >> > training). > >> > >> > >> > >> I don't see a need to do this for DP SST. > >> > >> > >> > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to > >> > >> > the lower link rate after trying to train at the maximum link rate > >> > >> > advertised through the DPCD registers. > >> > >> > >> > >> That test does not require the source DUT to default to maximum lane > >> > >> count or link rate of the sink. The source may freely choose the lane > >> > >> count and link rate as long as they don't exceed sink capabilities. > >> > >> > >> > >> For the purposes of the test, the test setup can request specific > >> > >> parameters to be used, but that does not mean using maximum by > >> > >> *default*. > >> > >> > >> > >> We currently lack the feature to reduce lane count and link rate. The > >> > >> key to understand here is that starting at max and reducing down to the > >> > >> sufficient parameters for the mode (which is where we start now) offers > >> > >> no real benefit for any use case. What we're lacking is a feature to > >> > >> reduce the link parameters *below* what's required by the mode the > >> > >> userspace wants. This can only be achieved through cooperation with > >> > >> userspace. > >> > >> > >> > > > >> > > We can train at the optimal link rate required for the requested mode as > >> > > done in the existing implementation and retrain whenever the link training > >> > > test request is sent. > >> > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > >> > > driver to fall back to even lower link rate. We do not implement this in the > >> > > current driver and so this test fails. Could you elaborate on how this can > >> > > be achieved with the the cooperation with userspace? > >> > > Should we send a uevent to the userspace asking to retry at a lower resolution > >> > > after retraining at the lower link rate? > >> > > This is pertty much the place where majority of the compliance tests are failing. > >> > > How can we pass compliance with regards to this feature? > >> > > >> > So here's an idea Ville and I came up with. It's not completely thought > >> > out yet, probably has some wrinkles still, but then there are wrinkles > >> > with the upfront link training too (I'll get back to those separately). > >> > > >> > If link training fails during modeset (either for real or because it's a > >> > test sink that wants to test failures), we 1) store the link parameters > >> > as failing, 2) send a uevent to userspace, hopefully getting the > >> > userspace to do another get modes and try again, 3) propage errors from > >> > modeset. > >> > >> userspace already tries to do a reprobe after a setcrtc fails, to try > >> and gracefully handle the race between hotplug being in its event queue > >> and performing setcrtc, i.e. I think the error is enough. > > > > I presume we want the modeset to be async, so by the time we notice the > > problem we're no longer in the ioctl. > > IOW, we'll just need to send the hotplug uevent anyway. > > BR, > Jani. > I am going to try to implement a the code where if wefail link training at a particular link rate then i send the uevent to the userspace saving off the values at which thelink training failed so that these values can be used in the next attempt of the modeset to prune the modes accordingly and link training should be tried in that attempt with the lower link rate. The hope is that this will make the compliance test 4.3.1.4 happy. Regards Manasi > -- > Jani Nikula, Intel Open Source Technology Center
On Thu, Sep 29, 2016 at 09:05:01AM -0700, Manasi Navare wrote: > On Thu, Sep 29, 2016 at 06:48:43PM +0300, Jani Nikula wrote: > > On Thu, 29 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Thu, Sep 29, 2016 at 12:44:19PM +0100, Chris Wilson wrote: > > >> On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: > > >> > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > >> > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > > >> > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > >> > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > > >> > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > >> > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure > > >> > >> >> > the link training sequence should fall back to the lower link rate > > >> > >> >> > followed by lower lane count until CR succeeds. > > >> > >> >> > On CR success, the sequence proceeds with Channel EQ. > > >> > >> >> > In case of Channel EQ failures, it should fallback to > > >> > >> >> > lower link rate and lane count and start the CR phase again. > > >> > >> >> > > >> > >> >> This change makes the link training start at the max lane count and max > > >> > >> >> link rate. This is not ideal, as it wastes the link. And it is not a > > >> > >> >> spec requirement. "The Link Policy Maker of the upstream device may > > >> > >> >> choose any link count and link rate as long as they do not exceed the > > >> > >> >> capabilities of the DP receiver." > > >> > >> >> > > >> > >> >> Our current code starts at the minimum required bandwidth for the mode, > > >> > >> >> therefore we can't fall back to lower link rate and lane count without > > >> > >> >> reducing the mode. > > >> > >> >> > > >> > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop > > >> > >> >> below what is required for the mode. This is unacceptable. > > >> > >> >> > > >> > >> >> BR, > > >> > >> >> Jani. > > >> > >> >> > > >> > >> >> > > >> > >> > > > >> > >> > Thanks Jani for your review comments. > > >> > >> > Yes in this change we start at the max link rate and lane count. This > > >> > >> > change was made according to the design document discussions we had > > >> > >> > before strating this DP Redesign project. The main reason for starting > > >> > >> > at the maxlink rate and max lane count was for ensuring proper > > >> > >> > behavior of DP MST. In case of DP MST, we want to train the link at > > >> > >> > the maximum supported link rate/lane count based on an early/ upfront > > >> > >> > link training result so that we dont fail when we try to connect a > > >> > >> > higher resolution monitor as a second monitor. This a trade off > > >> > >> > between wsting the link or higher power vs. needing to retrain for > > >> > >> > every monitor that requests a higher BW in case of DP MST. > > >> > >> > > >> > >> We already train at max bandwidth for DP MST, which seems to be the > > >> > >> sensible thing to do. > > >> > >> > > >> > >> > Actually this is also the reason for enabling upfront link training in > > >> > >> > the following patch where we train the link much ahead in the modeset > > >> > >> > sequence to understand the link rate and lane count values at which > > >> > >> > the link can be successfully trained and then the link training > > >> > >> > through modeset will always start at the upfront values (maximum > > >> > >> > supported values of lane count and link rate based on upfront link > > >> > >> > training). > > >> > >> > > >> > >> I don't see a need to do this for DP SST. > > >> > >> > > >> > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to > > >> > >> > the lower link rate after trying to train at the maximum link rate > > >> > >> > advertised through the DPCD registers. > > >> > >> > > >> > >> That test does not require the source DUT to default to maximum lane > > >> > >> count or link rate of the sink. The source may freely choose the lane > > >> > >> count and link rate as long as they don't exceed sink capabilities. > > >> > >> > > >> > >> For the purposes of the test, the test setup can request specific > > >> > >> parameters to be used, but that does not mean using maximum by > > >> > >> *default*. > > >> > >> > > >> > >> We currently lack the feature to reduce lane count and link rate. The > > >> > >> key to understand here is that starting at max and reducing down to the > > >> > >> sufficient parameters for the mode (which is where we start now) offers > > >> > >> no real benefit for any use case. What we're lacking is a feature to > > >> > >> reduce the link parameters *below* what's required by the mode the > > >> > >> userspace wants. This can only be achieved through cooperation with > > >> > >> userspace. > > >> > >> > > >> > > > > >> > > We can train at the optimal link rate required for the requested mode as > > >> > > done in the existing implementation and retrain whenever the link training > > >> > > test request is sent. > > >> > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > > >> > > driver to fall back to even lower link rate. We do not implement this in the > > >> > > current driver and so this test fails. Could you elaborate on how this can > > >> > > be achieved with the the cooperation with userspace? > > >> > > Should we send a uevent to the userspace asking to retry at a lower resolution > > >> > > after retraining at the lower link rate? > > >> > > This is pertty much the place where majority of the compliance tests are failing. > > >> > > How can we pass compliance with regards to this feature? > > >> > > > >> > So here's an idea Ville and I came up with. It's not completely thought > > >> > out yet, probably has some wrinkles still, but then there are wrinkles > > >> > with the upfront link training too (I'll get back to those separately). > > >> > > > >> > If link training fails during modeset (either for real or because it's a > > >> > test sink that wants to test failures), we 1) store the link parameters > > >> > as failing, 2) send a uevent to userspace, hopefully getting the > > >> > userspace to do another get modes and try again, 3) propage errors from > > >> > modeset. > > >> > > >> userspace already tries to do a reprobe after a setcrtc fails, to try > > >> and gracefully handle the race between hotplug being in its event queue > > >> and performing setcrtc, i.e. I think the error is enough. > > > > > > I presume we want the modeset to be async, so by the time we notice the > > > problem we're no longer in the ioctl. > > > > IOW, we'll just need to send the hotplug uevent anyway. > > > > BR, > > Jani. > > > > I am going to try to implement a the code where if wefail link training at a > particular link rate then i send the uevent to the userspace saving off the > values at which thelink training failed so that these values can be used in the next > attempt of the modeset to prune the modes accordingly and link training should be > tried in that attempt with the lower link rate. The hope is that this will make the > compliance test 4.3.1.4 happy. > > Regards > Manasi This is what I am doing when we get a test request to train at a particular rate: if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING)) { intel_dp_set_link_params(intel_dp, drm_dp_bw_code_to_link_rate(intel_dp-> compliance_test_link_rate), intel_dp->compliance_test_lane_count, false); drm_kms_helper_hotplug_event(intel_encoder->base.dev); } I see in the dmesg that it sends a hotplug uevent to the userspace that triggers a drm_setup_crtcs() But it finds that the connector is already enabled and has a CRTC so it does not go ahead with compute_config. Do we need to disable the crtc and update the atomic state before generating this uevent? How can this be done? Manasi > > -- > > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 29, 2016 at 04:17:06PM -0700, Manasi Navare wrote: > On Thu, Sep 29, 2016 at 09:05:01AM -0700, Manasi Navare wrote: > > On Thu, Sep 29, 2016 at 06:48:43PM +0300, Jani Nikula wrote: > > > On Thu, 29 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > On Thu, Sep 29, 2016 at 12:44:19PM +0100, Chris Wilson wrote: > > > >> On Thu, Sep 29, 2016 at 02:26:16PM +0300, Jani Nikula wrote: > > > >> > On Thu, 29 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > > >> > > On Tue, Sep 27, 2016 at 08:07:01PM +0300, Jani Nikula wrote: > > > >> > >> On Tue, 27 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > > >> > >> > On Mon, Sep 26, 2016 at 04:39:34PM +0300, Jani Nikula wrote: > > > >> > >> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > > >> > >> >> > According to the DisplayPort Spec, in case of Clock Recovery failure > > > >> > >> >> > the link training sequence should fall back to the lower link rate > > > >> > >> >> > followed by lower lane count until CR succeeds. > > > >> > >> >> > On CR success, the sequence proceeds with Channel EQ. > > > >> > >> >> > In case of Channel EQ failures, it should fallback to > > > >> > >> >> > lower link rate and lane count and start the CR phase again. > > > >> > >> >> > > > >> > >> >> This change makes the link training start at the max lane count and max > > > >> > >> >> link rate. This is not ideal, as it wastes the link. And it is not a > > > >> > >> >> spec requirement. "The Link Policy Maker of the upstream device may > > > >> > >> >> choose any link count and link rate as long as they do not exceed the > > > >> > >> >> capabilities of the DP receiver." > > > >> > >> >> > > > >> > >> >> Our current code starts at the minimum required bandwidth for the mode, > > > >> > >> >> therefore we can't fall back to lower link rate and lane count without > > > >> > >> >> reducing the mode. > > > >> > >> >> > > > >> > >> >> AFAICT this patch here makes it possible for the link bandwidth to drop > > > >> > >> >> below what is required for the mode. This is unacceptable. > > > >> > >> >> > > > >> > >> >> BR, > > > >> > >> >> Jani. > > > >> > >> >> > > > >> > >> >> > > > >> > >> > > > > >> > >> > Thanks Jani for your review comments. > > > >> > >> > Yes in this change we start at the max link rate and lane count. This > > > >> > >> > change was made according to the design document discussions we had > > > >> > >> > before strating this DP Redesign project. The main reason for starting > > > >> > >> > at the maxlink rate and max lane count was for ensuring proper > > > >> > >> > behavior of DP MST. In case of DP MST, we want to train the link at > > > >> > >> > the maximum supported link rate/lane count based on an early/ upfront > > > >> > >> > link training result so that we dont fail when we try to connect a > > > >> > >> > higher resolution monitor as a second monitor. This a trade off > > > >> > >> > between wsting the link or higher power vs. needing to retrain for > > > >> > >> > every monitor that requests a higher BW in case of DP MST. > > > >> > >> > > > >> > >> We already train at max bandwidth for DP MST, which seems to be the > > > >> > >> sensible thing to do. > > > >> > >> > > > >> > >> > Actually this is also the reason for enabling upfront link training in > > > >> > >> > the following patch where we train the link much ahead in the modeset > > > >> > >> > sequence to understand the link rate and lane count values at which > > > >> > >> > the link can be successfully trained and then the link training > > > >> > >> > through modeset will always start at the upfront values (maximum > > > >> > >> > supported values of lane count and link rate based on upfront link > > > >> > >> > training). > > > >> > >> > > > >> > >> I don't see a need to do this for DP SST. > > > >> > >> > > > >> > >> > As per the CTS, all the test 4.3.1.4 requires that you fall back to > > > >> > >> > the lower link rate after trying to train at the maximum link rate > > > >> > >> > advertised through the DPCD registers. > > > >> > >> > > > >> > >> That test does not require the source DUT to default to maximum lane > > > >> > >> count or link rate of the sink. The source may freely choose the lane > > > >> > >> count and link rate as long as they don't exceed sink capabilities. > > > >> > >> > > > >> > >> For the purposes of the test, the test setup can request specific > > > >> > >> parameters to be used, but that does not mean using maximum by > > > >> > >> *default*. > > > >> > >> > > > >> > >> We currently lack the feature to reduce lane count and link rate. The > > > >> > >> key to understand here is that starting at max and reducing down to the > > > >> > >> sufficient parameters for the mode (which is where we start now) offers > > > >> > >> no real benefit for any use case. What we're lacking is a feature to > > > >> > >> reduce the link parameters *below* what's required by the mode the > > > >> > >> userspace wants. This can only be achieved through cooperation with > > > >> > >> userspace. > > > >> > >> > > > >> > > > > > >> > > We can train at the optimal link rate required for the requested mode as > > > >> > > done in the existing implementation and retrain whenever the link training > > > >> > > test request is sent. > > > >> > > For the test 4.3.1.4 in CTS, it does force a failure in CR and expects the > > > >> > > driver to fall back to even lower link rate. We do not implement this in the > > > >> > > current driver and so this test fails. Could you elaborate on how this can > > > >> > > be achieved with the the cooperation with userspace? > > > >> > > Should we send a uevent to the userspace asking to retry at a lower resolution > > > >> > > after retraining at the lower link rate? > > > >> > > This is pertty much the place where majority of the compliance tests are failing. > > > >> > > How can we pass compliance with regards to this feature? > > > >> > > > > >> > So here's an idea Ville and I came up with. It's not completely thought > > > >> > out yet, probably has some wrinkles still, but then there are wrinkles > > > >> > with the upfront link training too (I'll get back to those separately). > > > >> > > > > >> > If link training fails during modeset (either for real or because it's a > > > >> > test sink that wants to test failures), we 1) store the link parameters > > > >> > as failing, 2) send a uevent to userspace, hopefully getting the > > > >> > userspace to do another get modes and try again, 3) propage errors from > > > >> > modeset. > > > >> > > > >> userspace already tries to do a reprobe after a setcrtc fails, to try > > > >> and gracefully handle the race between hotplug being in its event queue > > > >> and performing setcrtc, i.e. I think the error is enough. > > > > > > > > I presume we want the modeset to be async, so by the time we notice the > > > > problem we're no longer in the ioctl. > > > > > > IOW, we'll just need to send the hotplug uevent anyway. > > > > > > BR, > > > Jani. > > > When the test sink is testing the failure, link training fails in ddi_pre_enable(). This is still while we are holding the modeset locks hence we cannot send a hotplug uevent here. If i try to send the hotplug uevent here it just freezes due to deadlock. I am reading up on how to set up a work queue and call the uevent in a separate thread. Is this a right approach or you have any other suggestion for sending a hotplug uevent on link train failure during atomic commit phase. Manasi > > > > I am going to try to implement a the code where if wefail link training at a > > particular link rate then i send the uevent to the userspace saving off the > > values at which thelink training failed so that these values can be used in the next > > attempt of the modeset to prune the modes accordingly and link training should be > > tried in that attempt with the lower link rate. The hope is that this will make the > > compliance test 4.3.1.4 happy. > > > > Regards > > Manasi > > This is what I am doing when we get a test request to train at a particular rate: > if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING)) { > intel_dp_set_link_params(intel_dp, > drm_dp_bw_code_to_link_rate(intel_dp-> > compliance_test_link_rate), > intel_dp->compliance_test_lane_count, > false); > drm_kms_helper_hotplug_event(intel_encoder->base.dev); > } > > I see in the dmesg that it sends a hotplug uevent to the userspace that triggers a drm_setup_crtcs() > But it finds that the connector is already enabled and has a CRTC so it does not go ahead with > compute_config. Do we need to disable the crtc and update the atomic state before generating > this uevent? How can this be done? > > Manasi > > > > -- > > > Jani Nikula, Intel Open Source Technology Center > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 8065a5f..093038c 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct intel_encoder *encoder, } } -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder, int link_rate, uint32_t lane_count, - struct intel_shared_dpll *pll, - bool link_mst) + struct intel_shared_dpll *pll) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); enum port port = intel_ddi_get_encoder_port(encoder); intel_dp_set_link_params(intel_dp, link_rate, lane_count, - link_mst); - if (encoder->type == INTEL_OUTPUT_EDP) - intel_edp_panel_on(intel_dp); + false); + + intel_edp_panel_on(intel_dp); intel_ddi_clk_select(encoder, pll); intel_prepare_dp_ddi_buffers(encoder); @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, intel_dp_stop_link_train(intel_dp); } +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, + int link_rate, uint32_t lane_count, + struct intel_shared_dpll *pll, + bool link_mst) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_shared_dpll_config tmp_pll_config; + + /* Disable the PLL and obtain the PLL for Link Training + * that starts with highest link rate and lane count. + */ + tmp_pll_config = pll->config; + pll->funcs.disable(dev_priv, pll); + pll->config.crtc_mask = 0; + + /* If Link Training fails, send a uevent to generate a hotplug */ + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst)) + drm_kms_helper_hotplug_event(encoder->base.dev); + pll->config = tmp_pll_config; +} + static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, bool has_hdmi_sink, struct drm_display_mode *adjusted_mode, @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder, struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); int type = intel_encoder->type; - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { + if (type == INTEL_OUTPUT_EDP) + intel_ddi_pre_enable_edp(intel_encoder, + crtc->config->port_clock, + crtc->config->lane_count, + crtc->config->shared_dpll); + + if (type == INTEL_OUTPUT_DP) intel_ddi_pre_enable_dp(intel_encoder, crtc->config->port_clock, crtc->config->lane_count, crtc->config->shared_dpll, intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP_MST)); - } - if (type == INTEL_OUTPUT_HDMI) { + + if (type == INTEL_OUTPUT_HDMI) intel_ddi_pre_enable_hdmi(intel_encoder, crtc->config->has_hdmi_sink, &crtc->config->base.adjusted_mode, crtc->config->shared_dpll); - } + } static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, @@ -2435,6 +2462,72 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock) return pll; } +bool +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, + uint8_t max_lane_count, bool link_mst) +{ + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_encoder *encoder = connector->encoder; + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_shared_dpll *pll; + struct intel_shared_dpll_config tmp_pll_config; + int link_rate, max_link_rate_index, link_rate_index; + uint8_t lane_count; + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; + bool ret = false; + + max_link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates, + max_link_rate); + if (max_link_rate_index < 0) { + DRM_ERROR("Invalid Link Rate\n"); + return false; + } + + for (lane_count = max_lane_count; lane_count > 0; lane_count >>= 1) { + for (link_rate_index = max_link_rate_index; + link_rate_index >= 0; link_rate_index--) { + link_rate = common_rates[link_rate_index]; + pll = intel_ddi_get_link_dpll(intel_dp, link_rate); + if (pll == NULL) { + DRM_ERROR("Could not find DPLL for link training.\n"); + return false; + } + tmp_pll_config = pll->config; + pll->funcs.enable(dev_priv, pll); + + intel_dp_set_link_params(intel_dp, link_rate, + lane_count, link_mst); + + intel_ddi_clk_select(encoder, pll); + intel_prepare_dp_ddi_buffers(encoder); + intel_ddi_init_dp_buf_reg(encoder); + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); + ret = intel_dp_start_link_train(intel_dp); + if (ret) + break; + + /* Disable port followed by PLL for next + *retry/clean up + */ + intel_ddi_post_disable(encoder, NULL, NULL); + pll->funcs.disable(dev_priv, pll); + pll->config = tmp_pll_config; + } + if (ret) { + DRM_DEBUG_KMS("Link Training successful at link rate: %d lane: %d\n", + link_rate, lane_count); + break; + } + } + + intel_dp_stop_link_train(intel_dp); + + if (!lane_count) + DRM_ERROR("Link Training Failed\n"); + + return ret; +} + void intel_ddi_init(struct drm_device *dev, enum port port) { struct drm_i915_private *dev_priv = to_i915(dev); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 69cee9b..d81c67cb 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1506,6 +1506,21 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp) return rates[len - 1]; } +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, + int link_rate) +{ + int common_len; + int index; + + common_len = intel_dp_common_rates(intel_dp, common_rates); + for (index = 0; index < common_len; index++) { + if (link_rate == common_rates[common_len - index - 1]) + return common_len - index - 1; + } + + return -1; +} + int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) { return rate_to_index(rate, intel_dp->sink_rates); diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index c438b02..6eb5eb6 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -313,9 +313,16 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) DP_TRAINING_PATTERN_DISABLE); } -void +bool intel_dp_start_link_train(struct intel_dp *intel_dp) { - intel_dp_link_training_clock_recovery(intel_dp); - intel_dp_link_training_channel_equalization(intel_dp); + bool ret; + + if (intel_dp_link_training_clock_recovery(intel_dp)) { + ret = intel_dp_link_training_channel_equalization(intel_dp); + if (ret) + return true; + } + + return false; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8fd16ad..08cb571 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1164,6 +1164,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config); void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state); uint32_t ddi_signal_levels(struct intel_dp *intel_dp); +bool intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate, + uint8_t max_lane_count, bool link_mst); struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock); unsigned int intel_fb_align_height(struct drm_device *dev, @@ -1385,7 +1387,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, void intel_dp_set_link_params(struct intel_dp *intel_dp, int link_rate, uint8_t lane_count, bool link_mst); -void intel_dp_start_link_train(struct intel_dp *intel_dp); +bool intel_dp_start_link_train(struct intel_dp *intel_dp); void intel_dp_stop_link_train(struct intel_dp *intel_dp); void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); void intel_dp_encoder_reset(struct drm_encoder *encoder); @@ -1407,6 +1409,8 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co void intel_dp_mst_suspend(struct drm_device *dev); void intel_dp_mst_resume(struct drm_device *dev); int intel_dp_max_link_rate(struct intel_dp *intel_dp); +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int *common_rates, + int link_rate); int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); void intel_dp_hot_plug(struct intel_encoder *intel_encoder); void intel_power_sequencer_reset(struct drm_i915_private *dev_priv);
According to the DisplayPort Spec, in case of Clock Recovery failure the link training sequence should fall back to the lower link rate followed by lower lane count until CR succeeds. On CR success, the sequence proceeds with Channel EQ. In case of Channel EQ failures, it should fallback to lower link rate and lane count and start the CR phase again. v7: * Address readability concerns (Mika Kahola) v6: * Do not split quoted string across line (Mika Kahola) v5: * Reset the link rate index to the max link rate index before lowering the lane count (Jani Nikula) * Use the paradigm for loop in intel_dp_link_rate_index v4: * Fixed the link rate fallback loop (Manasi Navare) v3: * Fixed some rebase issues (Mika Kahola) v2: * Add a helper function to return index of requested link rate into common_rates array * Changed the link rate fallback loop to make use of common_rates array (Mika Kahola) * Changed INTEL_INFO to INTEL_GEN (David Weinehall) Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 113 +++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_dp.c | 15 ++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++- drivers/gpu/drm/i915/intel_drv.h | 6 +- 4 files changed, 133 insertions(+), 14 deletions(-)