On Fri, Sep 02, 2016 at 04:00:20PM +0300, Mika Kahola wrote:
> On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare 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.
> >
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 109
> > +++++++++++++++++++++++---
> > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++-
> > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > 3 files changed, 110 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 67a6a0b..78d6687 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1634,29 +1634,50 @@ 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);
> > intel_ddi_init_dp_buf_reg(encoder);
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > intel_dp_start_link_train(intel_dp);
> > - if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > + if (port != PORT_A || INTEL_INFO(dev_priv)->gen >= 9)
> > 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);
> At first glance, this seems that hotplug events are generated every
> time when link training fails. Is there a way to limit the number of
> link training tries so that we don't end up generating hotplug events
> over and over again?
This uevent will be sent only after retrying all the volatge swings/pre emphasis
and trying at all all possible lane counts and link rates. Also since we are
doing upfront link training, link training failing at this point and generating a
hotplug event is very rare.
I could possibly add a ERROR message to indicate that link training has failed
after all retries before it generates a hotplug event.
Manasi
>
> > + 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,
> > @@ -1690,20 +1711,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_OU
> > TPUT_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,
> > @@ -2431,6 +2458,66 @@ 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;
> > + uint8_t lane_count;
> > + bool link_rate_not_found = true;
> > + bool ret = false;
> > +
> > + for (lane_count = max_lane_count; lane_count > 0; lane_count
> > >>= 1) {
> > +
> > + link_rate = max_link_rate;
> > + while (link_rate_not_found) {
> At first glance, I read this as we haven't found the link rate yet and
> start from 0. This is not the case here as we have found the link rate
> which is the maximum link rate available. As we iterate link rates from
> max to min should we switch the logic here too? I mean something like
> this
>
> bool link_rate_not_found = false;
> ...
> while (!link_rate_not_found) {
> ...
> }
>
> The other way of doing this is not to use the boolean
> link_rate_not_found parameter at all but utilize the link_rate
> parameter only and iterate in a loop as long as the link_rate exceeds
> zero value. Anyway, this was just a thought and is more related on
> personal preference than the functionality.
Thanks for the review comment. I will rework this logic to use the
link rates/common rates array to iterate through the link rates
from max to min.
Manasi
>
> > + 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);
> It should be safe to move this ddi buffer initialization outside the
> loop.
>
> > + 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);
> > + if (link_rate == 540000)
> > + link_rate = 270000;
> > + else if (link_rate == 270000)
> > + link_rate = 162000;
> > + else
> > + link_rate_not_found = false;
> > + }
> > + if (ret) {
> > + DRM_DEBUG_KMS("Link Training successful\n");
> > + intel_dp_stop_link_train(intel_dp);
> > + break;
> > + }
> > + }
> > + if (!lane_count)
> > + DRM_ERROR("Link Training Failed\n");
> In case of failure, I think we should still stop the link training and
> call 'intel_dp_stop_link_train()'
>
> > +
> > + 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_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 07f0159..0df49e8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -309,9 +309,15 @@ 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;
> yep, this is what I had in mind when I reviewed the previous patch.
>
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e5bc976..342a2d5 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1160,6 +1160,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,
> > @@ -1381,7 +1383,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);
> --
> Mika Kahola - Intel OTC
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
