>-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] >Sent: Monday, January 11, 2016 7:40 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 Tue, 2015-12-29 at 18:50 +0000, R, Durgadoss wrote: >> 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. > >One idea would be split the part of intel_get_shared_dpll() that needs atomic >interfaces to a separate function. Then it would be possible to add a second >entry point that just takes crtc, encoder and the pll parameters and calls the >logic that doesn't depend on the atomic state. > >Then you could move the calls to intel_get_shared_dpll() from *_ddi_pll_select() >to haswell_crtc_compute_clock() so they don't get on the way during upfront link >training, where the new non-atomic entry point is called instead. Ok, Will try this implementation. Thanks, Durga > >Ander > >> >> 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
