Hi Ander, Thanks for looking into this.. >-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] >Sent: Tuesday, December 29, 2015 10:52 PM >To: R, Durgadoss; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [PATCH 7/7] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT > >On Fri, 2015-12-11 at 15:09 +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. >> >> Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 81 >> ++++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 77 +++++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_drv.h | 2 + >> 3 files changed, 159 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 632044a..43efc26 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -3274,6 +3274,87 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port >> *intel_dig_port) >> return connector; >> } >> >> +bool intel_ddi_upfront_link_train(struct intel_dp *intel_dp, >> + struct intel_crtc *crtc) >> +{ >> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); >> + struct intel_connector *connector = intel_dp->attached_connector; >> + struct intel_encoder *encoder = connector->encoder; >> + struct drm_device *dev = encoder->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_shared_dpll *pll; >> + struct drm_crtc *drm_crtc = NULL; >> + struct intel_crtc_state *tmp_crtc_config; >> + struct intel_dpll_hw_state tmp_dpll_hw_state; >> + 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 false; >> + } >> + 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; > >> + tmp_dpll_hw_state = crtc->config->dpll_hw_state; >> + >> + /* Select the shared DPLL to use for this port */ >> + intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config) > >This reads the current programmed DPLL from the hardware. Is there any reason we >can't trust the value that is in crtc->config already? I don't think this code >would run before reading out and sanitizing the hardware state. I was not sure of what will be the DPLL attached to crtc->config when the crtc is found by 'get_unused_crtc' logic. So, we select the DPLL based on port again.. > >> ; >> + pll = intel_crtc_to_shared_dpll(crtc); >> + if (!pll) { >> + DRM_ERROR("Could not get shared DPLL\n"); >> + goto exit; >> + } >> + DRM_DEBUG_KMS("Using %s for pipe %c\n", pll->name, pipe_name(crtc >> ->pipe)); >> + >> + do { >> + crtc->config->port_clock = >> drm_dp_bw_code_to_link_rate(link_bw); >> + crtc->config->lane_count = lane_count; >> + if (!intel_ddi_pll_select(crtc, crtc->config, encoder, >> false)) > > >> + goto exit; >> + >> + pll->config.crtc_mask |= (1 << crtc->pipe); >> + pll->config.hw_state = crtc->config->dpll_hw_state; >> + >> + /* Enable PLL followed by port */ >> + intel_enable_shared_dpll(crtc); >> + encoder->pre_enable(encoder); > >The pll handling here seems dodgy. Instead of using intel_get_shared_dpll(), I initially tried this, but intel_get_shared_dpll() uses crtc_state which is not set (in outside modeset contexts) thus creating crashes. Specifically, the call to intel_ddi_get_crtc_new_encoder() for broxton code path. That’s why I had to create some initial code to not call get_shared_dpll If we have valid encoder attached. (patches 1/7 and 2/7 of this series) So, If you have suggestions on how to fix this in a neat way, I would be happy to try them out. Thanks, Durga >this fiddles with pll internals itself. I think this will work for broxton, >since it doesn't actually have shared DPLLs (their chosen based on the encoder). >It might just work for haswell too since the plls used by DP are not shared. > >But to do this cleanly we need the DPLL interface to just give us the right pll. > >Ander > > >> + >> + /* 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); >> + >> + } while (!intel_dp->train_set_valid && >> + !intel_dp_get_link_retry_params(&lane_count, &link_bw)); >> + >> + /* Reset pll state as before */ >> + pll->config.crtc_mask &= ~(1 << crtc->pipe); >> + pll->config.hw_state = tmp_dpll_hw_state; >> + >> +exit: >> + /* Reset local associations made */ >> + if (drm_crtc) >> + encoder->base.crtc = NULL; >> + 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; >> +} >> + >> 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_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index d8f7830..47b6266 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4622,6 +4622,71 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) >> intel_dp->has_audio = false; >> } >> >> +static void intel_dp_upfront_dpms_off(struct drm_connector *connector, >> + struct drm_modeset_acquire_ctx *ctx) >> +{ >> + struct intel_dp *intel_dp = intel_attached_dp(connector); >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_crtc *crtc = intel_dig_port->base.base.crtc; >> + struct intel_load_detect_pipe tmp; >> + >> + if (intel_get_load_detect_pipe(connector, NULL, &tmp, ctx)) { >> + crtc->acquire_ctx = ctx; >> + tmp.dpms_mode = DRM_MODE_DPMS_OFF; >> + intel_release_load_detect_pipe(connector, &tmp, ctx); >> + } >> +} >> + >> +static void intel_dp_upfront_dpms_on(struct drm_connector *connector, >> + struct drm_modeset_acquire_ctx *ctx) >> +{ >> + struct intel_load_detect_pipe tmp; >> + >> + intel_get_load_detect_pipe(connector, NULL, &tmp, ctx); >> + >> + drm_modeset_drop_locks(ctx); >> + drm_modeset_acquire_fini(ctx); >> +} >> + >> +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 intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL; >> + struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL; >> + bool ret = true, need_dpms_on = false; >> + >> + 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) { >> + DRM_DEBUG_KMS("Disabling pipe %c\n", pipe_name(intel_crtc >> ->pipe)); >> + old_ctx = crtc->acquire_ctx; >> + drm_modeset_acquire_init(&ctx, 0); >> + intel_dp_upfront_dpms_off(connector, &ctx); >> + 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) { >> + intel_dp_upfront_dpms_on(connector, &ctx); >> + crtc->acquire_ctx = old_ctx; >> + } >> + return ret; >> +} >> + >> + >> static enum drm_connector_status >> intel_dp_detect(struct drm_connector *connector, bool force) >> { >> @@ -4631,7 +4696,7 @@ 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 ret, do_upfront_link_train; >> u8 sink_irq_vector; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> @@ -4705,6 +4770,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; >> + } >> 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 5912955..3cfab20 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1025,6 +1025,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_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, _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
