[Intel-gfx] [PATCH v18 5/6] drm/i915/dp: Enable Upfront link training on DDI platforms
Jani Nikula
jani.nikula at linux.intel.com
Thu Sep 29 16:05:01 UTC 2016
On Thu, 29 Sep 2016, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Wed, 21 Sep 2016, Manasi Navare <manasi.d.navare at intel.com> wrote:
>> To support USB type C alternate DP mode, the display driver needs to
>> know the number of lanes required by the DP panel as well as number
>> of lanes that can be supported by the type-C cable. Sometimes, the
>> type-C cable may limit the bandwidth even if Panel can support
>> more lanes. To address these scenarios we need to train the link before
>> modeset. This upfront link training caches the values of max link rate
>> and max lane count that get used later during modeset. Upfront link
>> training does not change any HW state, the link is disabled and PLL
>> values are reset to previous values after upfront link tarining so
>> that subsequent modeset is not aware of these changes.
>
> Some of the concerns and questions I've gathered about the upfront link
> training:
>
> * What if the userspace hasn't disabled the crtc by the time we get the
> hotplug? Upfront link training just goes ahead and messes with that
> state.
>
> * One of the potential benefits of upfront link training is that it
> could make the hotplug faster by doing the link training before the
> userspace even asks for a modeset. However, IIUC, the patch now does
> the upfront link training, disables the link, and then does link
> training again at modeset. Is that right? So it can actually make the
> link training slower? (On the plus side, this avoids the problem of
> leaving the link up and running if there isn't a userspace responding
> to hotplug or userspace never asks for a modeset.)
>
> * Another benefit of upfront link training is that we can prune the
> modes according to what the link can actually do, before
> modeset. However, this still doesn't help the case of link degrading
> during operation. (I am not sure how much we really care about this,
> but it would seem that the approach described in my other mail might
> solve it.)
>
> * Upfront link training is only enabled for DDI platforms, duplicating
> parts of link training and apparently parts of modeset for DDI, and
> diverging the link training code for DDI and non-DDI platforms. With
> the current approach, this makes it impossible to run DP compliance
> tests for non-DDI platforms.
>
> * How does upfront link training interact with atomic and fastboot? /me
> clueless.
>
> * There just is a subjectively scary feeling to the change. The DP link
> training code has been riddled with regressions in the past, and even
> the smallest and innocent seeming changes have caused them. This is a
> hard thing to justify, call it a gut feeling if you will, but history
> has taught me not to dismiss those instincts with a shrug.
>
> All in all, I'd like to decouple current DP compliance efforts from
> upfront link training.
Another that I failed to mention: Eventually we'll want the DP
compliance stuff to be something that other drivers can have too. We'll
want to push more DP code to drm core DP helpers, and we'll want to
share the burden of keeping DP compliant. As the upfront link training
doesn't seem to be an (easy) option even for our non-DDI hardware, it
may be a difficult thing for other hardware as well. Maybe. Better stick
with something that's probably an easier sell for drm?
BR,
Jani.
>
> Ville, feel free to clarify or add to the list.
>
>
> BR,
> Jani.
>
>
>>
>> This patch is based on prior work done by
>> R,Durgadoss <durgadoss.r at intel.com>
>>
>> Changes since v17:
>> * Rebased on the latest nightly
>> Changes since v16:
>> * Use HAS_DDI macro for enabling this feature (Rodrigo Vivi)
>> * Fix some unnecessary removals/changes due to rebase (Rodrigo Vivi)
>>
>> Changes since v15:
>> * Split this patch into two patches - one with functional
>> changes to enable upfront and other with moving the existing
>> functions around so that they can be used for upfront (Jani Nikula)
>> * Cleaned up the commit message
>>
>> Signed-off-by: Durgadoss R <durgadoss.r at intel.com>
>> Signed-off-by: Jim Bride <jim.bride at linux.intel.com>
>> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 21 ++-
>> drivers/gpu/drm/i915/intel_dp.c | 190 +++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/intel_dp_link_training.c | 1 -
>> drivers/gpu/drm/i915/intel_drv.h | 14 +-
>> 4 files changed, 218 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 093038c..8e52507 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1676,7 +1676,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>> 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))
>> + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst,
>> + false))
>> drm_kms_helper_hotplug_event(encoder->base.dev);
>> pll->config = tmp_pll_config;
>> }
>> @@ -2464,7 +2465,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
>>
>> bool
>> intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
>> - uint8_t max_lane_count, bool link_mst)
>> + uint8_t max_lane_count, bool link_mst, bool is_upfront)
>> {
>> struct intel_connector *connector = intel_dp->attached_connector;
>> struct intel_encoder *encoder = connector->encoder;
>> @@ -2513,6 +2514,7 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
>> 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);
>> @@ -2522,6 +2524,21 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
>>
>> intel_dp_stop_link_train(intel_dp);
>>
>> + if (is_upfront) {
>> + DRM_DEBUG_KMS("Upfront link train %s: link_clock:%d lanes:%d\n",
>> + ret ? "Passed" : "Failed",
>> + link_rate, lane_count);
>> + /* 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) {
>> + /* Save the upfront values */
>> + intel_dp->max_lanes_upfront = lane_count;
>> + intel_dp->max_link_rate_upfront = link_rate;
>> + }
>> + }
>> +
>> if (!lane_count)
>> DRM_ERROR("Link Training Failed\n");
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8d9a8ab..a058d5d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -153,12 +153,21 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp)
>> static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
>> {
>> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> - u8 source_max, sink_max;
>> + u8 temp, source_max, sink_max;
>>
>> source_max = intel_dig_port->max_lanes;
>> sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
>>
>> - return min(source_max, sink_max);
>> + temp = min(source_max, sink_max);
>> +
>> + /*
>> + * Limit max lanes w.r.t to the max value found
>> + * using Upfront link training also.
>> + */
>> + if (intel_dp->max_lanes_upfront)
>> + return min(temp, intel_dp->max_lanes_upfront);
>> + else
>> + return temp;
>> }
>>
>> /*
>> @@ -190,6 +199,42 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>> return (max_link_clock * max_lanes * 8) / 10;
>> }
>>
>> +static int intel_dp_upfront_crtc_disable(struct intel_crtc *crtc,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool enable)
>> +{
>> + int ret;
>> + struct drm_atomic_state *state;
>> + struct intel_crtc_state *crtc_state;
>> + struct drm_device *dev = crtc->base.dev;
>> + enum pipe pipe = crtc->pipe;
>> +
>> + state = drm_atomic_state_alloc(dev);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + state->acquire_ctx = ctx;
>> +
>> + crtc_state = intel_atomic_get_crtc_state(state, crtc);
>> + if (IS_ERR(crtc_state)) {
>> + ret = PTR_ERR(crtc_state);
>> + drm_atomic_state_free(state);
>> + return ret;
>> + }
>> +
>> + DRM_DEBUG_KMS("%sabling crtc %c %s upfront link train\n",
>> + enable ? "En" : "Dis",
>> + pipe_name(pipe),
>> + enable ? "after" : "before");
>> +
>> + crtc_state->base.active = enable;
>> + ret = drm_atomic_commit(state);
>> + if (ret)
>> + drm_atomic_state_free(state);
>> +
>> + return ret;
>> +}
>> +
>> static int
>> intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp)
>> {
>> @@ -281,6 +326,17 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
>> int source_len, sink_len;
>>
>> sink_len = intel_dp_sink_rates(intel_dp, &sink_rates);
>> +
>> + /* Cap sink rates w.r.t upfront values */
>> + if (intel_dp->max_link_rate_upfront) {
>> + int len = sink_len - 1;
>> +
>> + while (len > 0 && sink_rates[len] >
>> + intel_dp->max_link_rate_upfront)
>> + len--;
>> + sink_len = len + 1;
>> + }
>> +
>> source_len = intel_dp_source_rates(intel_dp, &source_rates);
>>
>> return intersect_rates(source_rates, source_len,
>> @@ -288,6 +344,92 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
>> common_rates);
>> }
>>
>> +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
>> +{
>> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> + struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> + struct drm_device *dev = intel_encoder->base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_mode_config *config = &dev->mode_config;
>> + struct drm_modeset_acquire_ctx ctx;
>> + struct intel_crtc *intel_crtc;
>> + struct drm_crtc *crtc = NULL;
>> + struct intel_shared_dpll *pll;
>> + struct intel_shared_dpll_config tmp_pll_config;
>> + bool disable_dpll = false;
>> + int ret;
>> + bool done = false, has_mst = false;
>> + uint8_t max_lanes;
>> + int common_rates[DP_MAX_SUPPORTED_RATES] = {};
>> + int common_len;
>> + enum intel_display_power_domain power_domain;
>> +
>> + power_domain = intel_display_port_power_domain(intel_encoder);
>> + intel_display_power_get(dev_priv, power_domain);
>> +
>> + common_len = intel_dp_common_rates(intel_dp, common_rates);
>> + max_lanes = intel_dp_max_lane_count(intel_dp);
>> + if (WARN_ON(common_len <= 0))
>> + return true;
>> +
>> + drm_modeset_acquire_init(&ctx, 0);
>> +retry:
>> + ret = drm_modeset_lock(&config->connection_mutex, &ctx);
>> + if (ret)
>> + goto exit_fail;
>> +
>> + if (intel_encoder->base.crtc) {
>> + crtc = intel_encoder->base.crtc;
>> +
>> + ret = drm_modeset_lock(&crtc->mutex, &ctx);
>> + if (ret)
>> + goto exit_fail;
>> +
>> + ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
>> + if (ret)
>> + goto exit_fail;
>> +
>> + intel_crtc = to_intel_crtc(crtc);
>> + pll = intel_crtc->config->shared_dpll;
>> + disable_dpll = true;
>> + has_mst = intel_crtc_has_type(intel_crtc->config,
>> + INTEL_OUTPUT_DP_MST);
>> + ret = intel_dp_upfront_crtc_disable(intel_crtc, &ctx, false);
>> + if (ret)
>> + goto exit_fail;
>> + }
>> +
>> + mutex_lock(&dev_priv->dpll_lock);
>> + if (disable_dpll) {
>> + /* Clear the PLL config state */
>> + tmp_pll_config = pll->config;
>> + pll->config.crtc_mask = 0;
>> + }
>> +
>> + done = intel_dp->upfront_link_train(intel_dp,
>> + common_rates[common_len-1],
>> + max_lanes,
>> + has_mst,
>> + true);
>> + if (disable_dpll)
>> + pll->config = tmp_pll_config;
>> +
>> + mutex_unlock(&dev_priv->dpll_lock);
>> +
>> + if (crtc)
>> + ret = intel_dp_upfront_crtc_disable(intel_crtc, &ctx, true);
>> +
>> +exit_fail:
>> + if (ret == -EDEADLK) {
>> + drm_modeset_backoff(&ctx);
>> + goto retry;
>> + }
>> + drm_modeset_drop_locks(&ctx);
>> + drm_modeset_acquire_fini(&ctx);
>> + intel_display_power_put(dev_priv, power_domain);
>> + return done;
>> +}
>> +
>> static enum drm_mode_status
>> intel_dp_mode_valid(struct drm_connector *connector,
>> struct drm_display_mode *mode)
>> @@ -311,6 +453,19 @@ intel_dp_mode_valid(struct drm_connector *connector,
>> target_clock = fixed_mode->clock;
>> }
>>
>> + if (intel_dp->upfront_link_train && !intel_dp->upfront_done) {
>> + bool do_upfront_link_train;
>> + /* Do not do upfront link train, if it is a compliance
>> + * request
>> + */
>> + do_upfront_link_train = !intel_dp->upfront_done &&
>> + (intel_dp->compliance_test_type !=
>> + DP_TEST_LINK_TRAINING);
>> +
>> + if (do_upfront_link_train)
>> + intel_dp->upfront_done = intel_dp_upfront_link_train(intel_dp);
>> + }
>> +
>> max_link_clock = intel_dp_max_link_rate(intel_dp);
>> max_lanes = intel_dp_max_lane_count(intel_dp);
>>
>> @@ -1499,6 +1654,9 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
>> int rates[DP_MAX_SUPPORTED_RATES] = {};
>> int len;
>>
>> + if (intel_dp->max_link_rate_upfront)
>> + return intel_dp->max_link_rate_upfront;
>> +
>> len = intel_dp_common_rates(intel_dp, rates);
>> if (WARN_ON(len <= 0))
>> return 162000;
>> @@ -1644,6 +1802,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> for (; bpp >= 6*3; bpp -= 2*3) {
>> mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> bpp);
>> +
>> + if (!is_edp(intel_dp) && intel_dp->upfront_done) {
>> + clock = max_clock;
>> + lane_count = intel_dp->max_lanes_upfront;
>> + link_clock = intel_dp->max_link_rate_upfront;
>> + link_avail = intel_dp_max_data_rate(link_clock,
>> + lane_count);
>> + mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> + bpp);
>> + if (mode_rate <= link_avail)
>> + goto found;
>> + else
>> + continue;
>> + }
>> +
>> clock = max_clock;
>> lane_count = max_lane_count;
>> link_clock = common_rates[clock];
>> @@ -1672,7 +1845,6 @@ found:
>> }
>>
>> pipe_config->lane_count = lane_count;
>> -
>> pipe_config->pipe_bpp = bpp;
>> pipe_config->port_clock = common_rates[clock];
>>
>> @@ -4453,8 +4625,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>>
>> out:
>> if ((status != connector_status_connected) &&
>> - (intel_dp->is_mst == false))
>> + (intel_dp->is_mst == false)) {
>> intel_dp_unset_edid(intel_dp);
>> + intel_dp->upfront_done = false;
>> + intel_dp->max_lanes_upfront = 0;
>> + intel_dp->max_link_rate_upfront = 0;
>> + }
>>
>> intel_display_power_put(to_i915(dev), power_domain);
>> return;
>> @@ -5698,6 +5874,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>> if (type == DRM_MODE_CONNECTOR_eDP)
>> intel_encoder->type = INTEL_OUTPUT_EDP;
>>
>> + /* Initialize upfront link training vfunc for DP */
>> + if (intel_encoder->type != INTEL_OUTPUT_EDP) {
>> + if (HAS_DDI(dev_priv))
>> + intel_dp->upfront_link_train = intel_ddi_link_train;
>> + }
>> +
>> /* eDP only on port B and/or C on vlv/chv */
>> if (WARN_ON((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) &&
>> is_edp(intel_dp) && port != PORT_B && port != PORT_C))
>> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
>> index 6eb5eb6..782a919 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
>> @@ -304,7 +304,6 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>> intel_dp_set_idle_link_train(intel_dp);
>>
>> return intel_dp->channel_eq_status;
>> -
>> }
>>
>> void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0aeb317..fdfc0b6 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -887,6 +887,12 @@ struct intel_dp {
>> enum hdmi_force_audio force_audio;
>> bool limited_color_range;
>> bool color_range_auto;
>> +
>> + /* Upfront link train parameters */
>> + int max_link_rate_upfront;
>> + uint8_t max_lanes_upfront;
>> + bool upfront_done;
>> +
>> uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
>> uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
>> uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>> @@ -944,6 +950,11 @@ struct intel_dp {
>> /* This is called before a link training is starterd */
>> void (*prepare_link_retrain)(struct intel_dp *intel_dp);
>>
>> + /* For Upfront link training */
>> + bool (*upfront_link_train)(struct intel_dp *intel_dp, int clock,
>> + uint8_t lane_count, bool link_mst,
>> + bool is_upfront);
>> +
>> /* Displayport compliance testing */
>> unsigned long compliance_test_type;
>> unsigned long compliance_test_data;
>> @@ -1166,7 +1177,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>> 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);
>> + uint8_t max_lane_count, bool link_mst,
>> + bool is_upfront);
>> 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,
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list