>-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] >Sent: Friday, February 12, 2016 10:13 PM >To: R, Durgadoss <durgadoss.r@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on >BXT > >On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R 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, the display driver will >> start link training with max lanes, and if that fails, the driver >> falls back to x2 lanes; and repeats this procedure for all >> bandwidth/lane configurations. >> >> * Since link training is done before modeset only the port >> (and not pipe/planes) and its associated PLLs are enabled. >> * On DP hotplug: Directly start link training on the crtc >> associated with the DP encoder. >> * On Connected boot scenarios: When booted with an LFP and a DP, >> typically, BIOS brings up DP. In these cases, we disable the >> crtc, do upfront link training and then re-enable it back. >> * All local changes made for upfront link training are reset >> to their previous values once it is done; so that the >> subsequent modeset is not aware of these changes. >> > >Please always include a changelog when sending a new version of a patch. Sure, will add it in the next version. > >> Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 102 +++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++- >> drivers/gpu/drm/i915/intel_dp.c | 122 ++++++++++++++++++++++++++++++++++ >> - >> drivers/gpu/drm/i915/intel_drv.h | 10 +++ >> 4 files changed, 270 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 3fb9a03..cc5cad5 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct >> intel_digital_port *intel_dig_port) >> return connector; >> } >> >> +static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp, >> + struct intel_shared_dpll *pll) >> +{ >> + struct intel_shared_dpll_config tmp_config; >> + >> + /* >> + * The shared_dpll_config is computed in intel_get_shared_dpll(). >> + * It is committed to 'pll->config' here to be used in >> + * intel_enable/disable_shared_dpll functions. Before committing. >> + * save the existing config, so that once upfront link training is >> + * done, the original value of 'pll->config' can be restored. >> + */ >> + tmp_config = pll->config; >> + pll->config = intel_dp->upfront_pll_config; >> + intel_dp->upfront_pll_config = tmp_config; >> +} >> + >> +static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config( >> + struct intel_dp *intel_dp, struct intel_crtc *crtc) >> +{ >> + if (!intel_ddi_pll_select(crtc, crtc->config)) >> + return NULL; >> + >> + return intel_crtc_to_shared_dpll(crtc); >> +} >> + >> +static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp, >> + struct intel_shared_dpll *pll) >> +{ >> + pll->config = intel_dp->upfront_pll_config; >> +} >> + > >The shared pll interface is really getting in the way here. And BXT plls aren't >even shared. We are jumping through hoops to get the pll that matches the given >encoder and to call the code that calculates the dpll_hw_state based on the DP >link rate. I'd like to get that interface fixed but I reckon it will be tricky >to find something that works for all the platforms we support. That's the next >thing on my todo list. > >If we have to live with some intermediary solution, I think it would be better >to (almost) completely bypass the shared dpll stuff. First we would need to >split bxt_ddi_pll_sel() so that we would have a function that takes the link >bandwidth and spits out a dpll_hw_state without looking at crtc_state. Then we >would just take the right pll based on dig_port->port, make sure it is not being >used and program it with the hw state we get from that new function. > >I wrote a patch to do that split. With it, the PLL enable logic in the upfront >link train logic would look a bit like below. There is some potential for >problems with the state checker, but it should be fine as long as the old dpll This is similar to what we had in the earlier version except the ddi_pll_select split.. I moved it this way because we wanted all the dpll functionality within the dpll interfaces. But yes, as you said, we could not get the whole thing cleaner. >hw state is saved and restored when everything is over. > > >/* FIXME: this only works for BXT */ >dpll_id = (enum intel_dpll_id) dig_port->port; >pll = dev_priv->shared_dplls[dpll_id]; > >if (WARN_ON(pll->config.crtc_mask) || WARN_ON(pll->active)) > goto exit; > >bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div); >if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div, > &pll->config.hw_state)) { > DRM_ERROR("Could not setup DPLL\n"); > goto exit; >} > > >/* Enable PLL followed by port */ >pll->enable(dev_priv, pll); >encoder->pre_enable(encoder); > > >This solution would remove the need for patches 1 and 3. > >What do you think? This (and your other patch) looks ok to me. I will test and confirm once. > >> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp, >> + struct intel_crtc *crtc) >> +{ >> + struct intel_connector *connector = intel_dp->attached_connector; >> + struct intel_encoder *encoder = connector->encoder; >> + struct intel_shared_dpll *pll = NULL; >> + struct drm_crtc *drm_crtc = NULL; >> + struct intel_crtc_state *tmp_crtc_config; >> + uint8_t link_bw, lane_count; >> + >> + if (!crtc) { >> + drm_crtc = intel_get_unused_crtc(&encoder->base); >> + if (!drm_crtc) { >> + DRM_ERROR("No crtc for upfront link training\n"); >> + return -EINVAL; >> + } >> + encoder->base.crtc = drm_crtc; >> + crtc = to_intel_crtc(drm_crtc); >> + } >> + >> + /* Initialize with Max Link rate & lane count supported by panel */ >> + link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; >> + lane_count = drm_dp_max_lane_count(intel_dp->dpcd); >> + >> + /* Save the crtc->config */ >> + tmp_crtc_config = crtc->config; >> + >> + DRM_DEBUG_KMS("Using pipe %c\n", pipe_name(crtc->pipe)); >> + do { >> + crtc->config->port_clock = >> drm_dp_bw_code_to_link_rate(link_bw); >> + crtc->config->lane_count = lane_count; >> + >> + pll = intel_ddi_select_upfront_pll_config(intel_dp, crtc); >> + if (!pll) { >> + DRM_ERROR("Could not get shared DPLL\n"); >> + goto exit; >> + } >> + >> + intel_ddi_commit_upfront_pll_config(intel_dp, pll); >> + >> + /* Enable PLL followed by port */ >> + intel_enable_shared_dpll(crtc); >> + encoder->pre_enable(encoder); >> + >> + /* Check if link training passed; if so update DPCD */ >> + if (intel_dp->train_set_valid) >> + intel_dp_update_dpcd_params(intel_dp); >> + >> + /* Disable port followed by PLL for next retry/clean up */ >> + encoder->post_disable(encoder); >> + intel_disable_shared_dpll(crtc); >> + >> + intel_ddi_clear_upfront_pll_config(intel_dp, pll); >> + >> + } while (!intel_dp->train_set_valid && >> + intel_dp_get_link_retry_params(intel_dp, &lane_count, >> &link_bw)); >> +exit: >> + /* Reset local associations made */ >> + if (drm_crtc) >> + encoder->base.crtc = NULL; >> + >> + /* Restore crtc config */ >> + crtc->config = tmp_crtc_config; >> + >> + DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n", >> + intel_dp->train_set_valid ? "Passed" : "Failed", lane_count, >> link_bw); >> + >> + return intel_dp->train_set_valid ? 0 : -1; >> +} >> + >> void intel_ddi_init(struct drm_device *dev, enum port port) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index af50e61..6a650aa 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4238,16 +4238,45 @@ static void lpt_pch_enable(struct drm_crtc *crtc) >> lpt_enable_pch_transcoder(dev_priv, cpu_transcoder); >> } >> >> +static >> +void intel_get_new_shared_dpll_config(struct drm_i915_private *dev_priv, >> + struct intel_shared_dpll_config >> *shared_dpll) >> +{ >> + enum intel_dpll_id i; >> + >> + /* Create a new shared dpll config by duplicating pll->config */ >> + for (i = 0; i < dev_priv->num_shared_dpll; i++) { >> + struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i]; >> + shared_dpll[i] = pll->config; >> + shared_dpll[i].crtc_mask = 0; >> + } >> +} >> + >> struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, >> struct intel_crtc_state >> *crtc_state) >> { >> struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; >> struct intel_shared_dpll *pll; >> struct intel_shared_dpll_config *shared_dpll; >> + struct intel_shared_dpll_config tmp_config[I915_NUM_PLLS]; >> enum intel_dpll_id i; >> int max = dev_priv->num_shared_dpll; >> >> - shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state >> ->base.state); >> + /* >> + * intel_get_shared_dpll needs a shared_dpll[] to store the computed >> + * dpll_config values. For atomic path, it is stored in >> + * intel_atomic_state->shared_dpll[], which is later committed to >> + * dev_priv->shared_dpll[] in atomic commit. For non-atomic path, >> + * (where intel_atomic_state does not exist) the dpll_config values >> + * are stored in 'tmp_config[]' and copied to encoder structures >> + * for later use. >> + */ >> + if (crtc_state->base.state) { >> + shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state >> ->base.state); >> + } else { >> + intel_get_new_shared_dpll_config(dev_priv, tmp_config); >> + shared_dpll = tmp_config; >> + } >> >> if (HAS_PCH_IBX(dev_priv->dev)) { >> /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ >> @@ -4277,6 +4306,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct >> intel_crtc *crtc, >> pll = &dev_priv->shared_dplls[i]; >> DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >> crtc->base.base.id, pll->name); >> + >> WARN_ON(shared_dpll[i].crtc_mask); >> >> goto found; >> @@ -4325,6 +4355,12 @@ found: >> >> shared_dpll[i].crtc_mask |= 1 << crtc->pipe; >> >> + /* >> + * For DP, this shared dpll config is saved to intel_dp, >> + * and used by upfront link train logic subsequently. >> + */ >> + intel_dp_update_shared_dpll_config(crtc_state, shared_dpll[i]); >> + >> return pll; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index e2bea710..cc7b6f3 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1679,6 +1679,55 @@ void intel_dp_set_link_params(struct intel_dp >> *intel_dp, >> intel_dp->lane_count = pipe_config->lane_count; >> } >> >> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp) >> +{ >> + intel_dp->dpcd[DP_MAX_LANE_COUNT] &= ~DP_MAX_LANE_COUNT_MASK; >> + intel_dp->dpcd[DP_MAX_LANE_COUNT] |= >> + intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK; >> + >> + intel_dp->dpcd[DP_MAX_LINK_RATE] = >> + drm_dp_link_rate_to_bw_code(intel_dp->link_rate); > >Maybe it would be good to have an explicit field for actual max lane count and >link rate. That way, reading the DPCD from debugfs will give the actual results >read from the sink instead of an edited value. Looks like the 'dpcd_show' interface reads the DPCD everytime and prints It .. And does not seem to look up at intel_dp->dpcd[]. Or, are we talking about two different interfaces ? > >> +} >> + >> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp, >> + uint8_t *lane_count, uint8_t *link_bw) >> +{ >> + /* >> + * As per DP1.3 Spec, retry all link rates for a particular >> + * lane count value, before reducing number of lanes. >> + */ >> + if (*link_bw == DP_LINK_BW_5_4) { >> + *link_bw = DP_LINK_BW_2_7; >> + } else if (*link_bw == DP_LINK_BW_2_7) { >> + *link_bw = DP_LINK_BW_1_62; >> + } else if (*lane_count == 4) { >> + *lane_count = 2; >> + *link_bw = intel_dp_max_link_bw(intel_dp); >> + } else if (*lane_count == 2) { >> + *lane_count = 1; >> + *link_bw = intel_dp_max_link_bw(intel_dp); >> + } else { >> + /* Tried all combinations, so exit */ >> + return false; >> + } >> + >> + return true; >> +} >> + >> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config, >> + struct intel_shared_dpll_config config) >> +{ >> + struct intel_encoder *encoder; >> + struct intel_dp *intel_dp; >> + >> + encoder = intel_ddi_get_crtc_new_encoder(pipe_config); >> + if (!encoder || encoder->type != INTEL_OUTPUT_DISPLAYPORT) >> + return; >> + >> + intel_dp = enc_to_intel_dp(&encoder->base); >> + intel_dp->upfront_pll_config = config; >> +} >> + >> static void intel_dp_prepare(struct intel_encoder *encoder) >> { >> struct drm_device *dev = encoder->base.dev; >> @@ -4601,6 +4650,66 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) >> intel_dp->has_audio = false; >> } >> >> +static bool intel_dp_upfront_link_train(struct drm_connector *connector) >> +{ >> + struct intel_dp *intel_dp = intel_attached_dp(connector); >> + 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_crtc *crtc = intel_dig_port->base.base.crtc; >> + struct drm_mode_config *config = &dev->mode_config; >> + struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL; >> + struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL; >> + bool need_dpms_on = false; >> + int ret; >> + >> + if (!IS_BROXTON(dev)) >> + return true; >> + >> + /* >> + * On hotplug cases, we call _upfront_link_train directly. >> + * In connected boot scenarios (boot with {MIPI/eDP} + DP), >> + * BIOS typically brings up DP. Hence, we disable the crtc >> + * to do _upfront_link_training and then re-enable it back. >> + */ >> + if (crtc && crtc->enabled && intel_crtc->active) { >> + old_ctx = crtc->acquire_ctx; >> + drm_modeset_acquire_init(&ctx, 0); >> +retry: >> + ret = drm_modeset_lock(&config->connection_mutex, &ctx); >> + if (ret == -EDEADLK) { >> + drm_modeset_backoff(&ctx); >> + goto retry; >> + } > >We need to acquire those locks even if the crtc is disabled, otherwise a modeset >could sneak past and wreak havoc while we are doing the link training and >fiddling with the plls. > >> + >> + crtc->acquire_ctx = &ctx; >> + ret = drm_atomic_helper_connector_dpms(connector, >> DRM_MODE_DPMS_OFF); > >The crtc->acquire_ctx override seems wrong. One thing that slipped by me when I >looked at the previous version of this patch is that we need to acquire some >locks even if the crtc is already off. So it is probably better to write a Ok, I think this is what Ville is also mentioning. I did not notice it either ;-( I am still learning the scheme of ww_* and ctx based locking.. So, yes, will re-work this. >separate function that takes an acquire context and uses atomic to set >crtc_state->active to false and calls drm_atomic_commit(). > >> + if (ret) { >> + DRM_ERROR("DPMS off failed:%d\n", ret); >> + goto exit_unlock; >> + } >> + >> + need_dpms_on = true; >> + } >> + >> + if (HAS_DDI(dev)) >> + ret = intel_ddi_upfront_link_train(intel_dp, intel_crtc); >> + /* Other platforms upfront link train call goes here..*/ >> + >> + if (!need_dpms_on) >> + return ret; >> + >> + ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_ON); >> + if (ret) >> + DRM_ERROR("DPMS on failed:%d\n", ret); >> + >> +exit_unlock: >> + drm_modeset_drop_locks(&ctx); >> + drm_modeset_acquire_fini(&ctx); >> + crtc->acquire_ctx = old_ctx; >> + return ret; > >The return type of this function is bool. The conversion here will do the wrong >thing, since ret potentially has the return value of functions that return non >-zero on failure. I wanted to make this as an 'int'. I missed this. Will fix it in next version. > >> +} >> + >> static enum drm_connector_status >> intel_dp_detect(struct drm_connector *connector, bool force) >> { >> @@ -4610,7 +4719,8 @@ intel_dp_detect(struct drm_connector *connector, bool >> force) >> struct drm_device *dev = connector->dev; >> enum drm_connector_status status; >> enum intel_display_power_domain power_domain; >> - bool ret; >> + bool do_upfront_link_train; >> + int ret; >> u8 sink_irq_vector; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> @@ -4684,6 +4794,16 @@ intel_dp_detect(struct drm_connector *connector, bool >> force) >> DRM_DEBUG_DRIVER("CP or sink specific irq >> unhandled\n"); >> } >> >> + /* Do not do upfront link train, if it is a compliance request */ >> + do_upfront_link_train = >> + intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT && >> + intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING; >> + >> + if (do_upfront_link_train) { >> + ret = intel_dp_upfront_link_train(connector); >> + if (ret) >> + status = connector_status_disconnected; >> + } > >So if upfront link training fails because there is no unused crtc we'll say that >the connector is disconnected? I was not sure on what should be the stand we should take here.. Should we distinguish various causes of upfront failure and then report status accordingly ? Not just crtc but pll selection, link training failures, etc.. Please let me know what you think. Thanks, Durga > > >Ander > >> out: >> intel_display_power_put(to_i915(dev), power_domain); >> return status; >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index 9fe7c4b..c5ca4ab 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -812,6 +812,9 @@ struct intel_dp { >> unsigned long compliance_test_type; >> unsigned long compliance_test_data; >> bool compliance_test_active; >> + >> + /* Used to store pll config for upfront link training */ >> + struct intel_shared_dpll_config upfront_pll_config; >> }; >> >> struct intel_digital_port { >> @@ -1031,6 +1034,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); >> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp, >> + struct intel_crtc *crtc); >> >> /* intel_frontbuffer.c */ >> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, >> @@ -1239,6 +1244,9 @@ bool intel_dp_init_connector(struct intel_digital_port >> *intel_dig_port, >> struct intel_connector *intel_connector); >> void intel_dp_set_link_params(struct intel_dp *intel_dp, >> const struct intel_crtc_state *pipe_config); >> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp); >> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp, >> + uint8_t *lane_count, uint8_t *link_bw); >> void 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); >> @@ -1246,6 +1254,8 @@ void intel_dp_encoder_destroy(struct drm_encoder >> *encoder); >> int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc); >> bool intel_dp_compute_config(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config); >> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config, >> + struct intel_shared_dpll_config config); >> bool intel_dp_is_edp(struct drm_device *dev, enum port port); >> enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, >> bool long_hpd); _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx