| Message ID | 1449826768-19415-8-git-send-email-durgadoss.r@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > --- > 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. > ; > + 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(), 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,
Hi Ander, Thanks for looking into this.. >-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] >Sent: Tuesday, December 29, 2015 10:52 PM >To: R, Durgadoss; intel-gfx@lists.freedesktop.org >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@intel.com> >> --- >> 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,
On 11 Dec 2015 7:09 PM, "Durgadoss R" <durgadoss.r@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, 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. This is just an aside but do we know if other OSes just always link train at plug time. I do wonder if we should make an OS level decision to require driver do this always. Dave. > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com> > --- > 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); > + 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); > + > + /* 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, > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 30, 2015 at 09:48:43AM +1000, Dave Airlie wrote: > On 11 Dec 2015 7:09 PM, "Durgadoss R" <durgadoss.r@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, 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. > > This is just an aside but do we know if other OSes just always link train > at plug time. > > I do wonder if we should make an OS level decision to require driver do > this always. Based on conversations I've had with folks in VPG, Windows appears to do upfront link training at plug time. Jim > Dave. > > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com> > > --- > > 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); > > + 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); > > + > > + /* 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, > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@gmail.com] > > Sent: Tuesday, December 29, 2015 10:52 PM > > To: R, Durgadoss; intel-gfx@lists.freedesktop.org > > 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@intel.com> > > > --- > > > 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. 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,
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@intel.com> > --- > 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); > + 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); > + > + /* 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); > +} Are these supposed to disable/enable the crtc so that the upfront link training can be performed? The load detect pipe really doesn't belong here, its only purpose is to detect whether there is an output connected in platforms that didn't support hotplug. You should do a normal atomic operation instead. Ander > + > +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,
>-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] >Sent: Monday, January 11, 2016 7:40 PM >To: R, Durgadoss; intel-gfx@lists.freedesktop.org >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@gmail.com] >> > Sent: Tuesday, December 29, 2015 10:52 PM >> > To: R, Durgadoss; intel-gfx@lists.freedesktop.org >> > 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@intel.com> >> > > --- >> > > 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,
>-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] >Sent: Monday, January 11, 2016 8:46 PM >To: R, Durgadoss; intel-gfx@lists.freedesktop.org >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@intel.com> >> --- >> 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); >> + 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); >> + >> + /* 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); >> +} > >Are these supposed to disable/enable the crtc so that the upfront link training >can be performed? The load detect pipe really doesn't belong here, its only >purpose is to detect whether there is an output connected in platforms that >didn't support hotplug. You should do a normal atomic operation instead. I think I should add more comment on this usage here.. During RFC, we were discussing about re-using code from load_detect logic to find unused Crtc's, to do upfront link training. While doing that, I figured out that the same can be used here (replacing intel_crtc_control() from non-atomic world.) Now, if BIOS has enabled the crtc during boot, we disable it during upfront link training and then turn it on once we are done. This set of dpms_on/off operations require the modeset locking infrastructure which are (already) provided by *_load_detect() functions. If using the functions as such does not make logical sense, then we can separate out the required code from *_load_detect() functions, and then re-use them. (like we did for get_unused_crtc() code). Does this sound ok to you ? Thanks, Durga > >Ander > >> + >> +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,
On Mon, 2016-01-11 at 17:53 +0000, R, Durgadoss wrote: > > -----Original Message----- > > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] > > Sent: Monday, January 11, 2016 8:46 PM > > To: R, Durgadoss; intel-gfx@lists.freedesktop.org > > 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@intel.com> > > > --- > > > 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); > > > + 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); > > > + > > > + /* 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); > > > +} > > > > Are these supposed to disable/enable the crtc so that the upfront link > > training > > can be performed? The load detect pipe really doesn't belong here, its only > > purpose is to detect whether there is an output connected in platforms that > > didn't support hotplug. You should do a normal atomic operation instead. > > I think I should add more comment on this usage here.. > > During RFC, we were discussing about re-using code from load_detect logic > to find unused Crtc's, to do upfront link training. While doing that, I > figured out > that the same can be used here (replacing intel_crtc_control() from non-atomic > world.) DPMS is now implemented using atomic, by setting the connector dpms function to drm_atomic_helper_connector_dpms(). It should be ok to call that directly here. > Now, if BIOS has enabled the crtc during boot, we disable it during upfront > link > training and then turn it on once we are done. This set of dpms_on/off > operations > require the modeset locking infrastructure which are (already) provided by > *_load_detect() functions. > > If using the functions as such does not make logical sense, then we can > separate > out the required code from *_load_detect() functions, and then re-use them. > (like we did for get_unused_crtc() code). Does this sound ok to you ? The load detect pipe does the opposite of what you want here. It enables a disabled crtc using an fb allocated by that function. After the pipe has been used for load detection, a call to intel_release_load_detect_pipe() turns the pipe back off and releases the fb. In the code above, there is a call to intel_get_load_detect_pipe() without a matching release. At that point, the crtc is left on with the wrong fb (the one allocated for the load detect pipe instead of the fb it was using before) and that fb is never freed. So I think the best solution is to call drm_atomic_helper_connector_dpms() directly. Ander > Thanks, > Durga > > > > > Ander > > > > > + > > > +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,
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); + 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); + + /* 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,
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@intel.com> --- 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(-)