Re: [PATCH v4 11/14] drm/i915: Fallback to lower link rate and lane count during link training

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 09 Sep 2016, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Wed, Sep 7, 2016 at 5:30 PM, 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.
>>
>> 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              | 109 +++++++++++++++++++++++---
>>  drivers/gpu/drm/i915/intel_dp.c               |  15 ++++
>>  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
>>  drivers/gpu/drm/i915/intel_drv.h              |   6 +-
>>  4 files changed, 128 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 25e7973..1278daa 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1634,19 +1634,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);
>> @@ -1657,6 +1656,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,
>> @@ -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_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,
>> @@ -2432,6 +2459,68 @@ 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, link_rate_index;
>> +       uint8_t lane_count;
>> +       int common_rates[DP_MAX_SUPPORTED_RATES] = {};
>> +       bool ret = false;
>> +
>> +       link_rate_index = intel_dp_link_rate_index(intel_dp, common_rates,
>> +                                                  max_link_rate);
>> +       if (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 >= 0; link_rate_index --) {
>
> ERROR: space prohibited before that '--' (ctx:WxB)
> #148: FILE: drivers/gpu/drm/i915/intel_ddi.c:2483:
> + for (; 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");
>
> The most common way in our code is to not split the comment and go over 80 cols.
>
> checkpatch complains either ways anyway:
>
> WARNING: quoted string split across lines
> #153: FILE: drivers/gpu/drm/i915/intel_ddi.c:2488:
> + DRM_ERROR("Could not find DPLL for link "
> +  "training.\n");
>
> Not sure what our maintainer prefer. Jani?

In this case, I'd prefer rigorous review of the functional aspects of
the changes.

>
>
>> +                               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 */
>
> And here it goes over 80 cols the most common is to
>
> /*
>  * Disable...
>  * ...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);
>
> also here and in 2 other places in
> drm-i915-Make-DP-link-training-channel-equalization-.patch
>
> Thanks,
> Rodrigo.
>
>> +                       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 75ac62f..1378116 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1443,6 +1443,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 = common_len - 1; index >= 0; index--) {
>> +               if (link_rate == common_rates[index])
>> +                       return index;
>> +       }
>> +
>> +       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..f1e08f0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
>> @@ -313,9 +313,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;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index ca51e1a..5b97a7d4 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);
>> @@ -1403,6 +1405,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);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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