On Mon, 2015-10-26 at 08:27 +0530, Thulasimani, Sivakumar wrote: > > On 10/23/2015 5:37 PM, R, Durgadoss wrote: > > > -----Original Message----- > > > From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] > > > Sent: Wednesday, October 21, 2015 9:08 PM > > > To: R, Durgadoss; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Subject: Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront > > > link training for typeC DP > > > support on BXT > > > > > > On Wed, 2015-10-14 at 17:30 +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. > > > > * Once link training is done, the port and its PLLs are disabled; > > > > so that the subsequent modeset is not aware of these changes. > > > > * 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 first and then start upfront link training. The crtc is > > > > re-enabled as part of a subsequent modeset. > > > > * For BXT, ddi->enable/disable for DP only enable/disable > > > > audio codec and hence are not included in upfront link > > > > training sequence. > > > > * As of now, this is tested only on BXT A1 platform, on > > > > kernel 4.2-rc2. > > > > > > > > Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx> > > > > --- > > > > drivers/gpu/drm/i915/intel_ddi.c | 152 > > > > +++++++++++++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++- > > > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > > > 3 files changed, 194 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > index 8e4ea36..b3a9bff 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct > > > > intel_digital_port *intel_dig_port) > > > > return connector; > > > > } > > > > > > > > +bool intel_ddi_upfront_link_train(struct drm_device *dev, > > > > + struct intel_dp *intel_dp, struct intel_crtc *crtc) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > > > + struct intel_connector *connector = intel_dp > > > > ->attached_connector; > > > > + struct intel_encoder *tmp_encoder, *encoder = connector > > > > ->encoder; > > > > + struct intel_shared_dpll *pll; > > > > + struct intel_crtc *tmp_crtc; > > > > + struct drm_crtc *tmp_drm_crtc; > > > > + uint8_t tmp_lane_count, tmp_link_bw; > > > > + bool ret, found, valid_crtc = false; > > > > + > > > > + /* For now, we have only SKL and BXT supporting type-C */ > > > > + if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev)) > > > > + return true; > > > > + > > > > + if (!connector || !encoder) { > > > > + DRM_DEBUG_KMS("dp connector/encoder is NULL\n"); > > > > + return false; > > > > + } > > > > + > > > > + /* If we already have a crtc, start link training directly */ > > > > + if (crtc) { > > > > + valid_crtc = true; > > > > + goto start_link_train; > > > > + } > > > > + > > > > + /* Find an unused crtc and use it for link training */ > > > > + for_each_intel_crtc(dev, crtc) { > > > > + if (intel_crtc_active(&crtc->base)) > > > > + continue; > > > > + > > > > + /* Make sure the new crtc will work with the encoder */ > > > > + if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) { > > > > + found = true; > > > > + > > > > + /* Save the existing values */ > > > > + tmp_encoder = connector->new_encoder; > > > > + tmp_crtc = encoder->new_crtc; > > > > + tmp_drm_crtc = encoder->base.crtc; > > > In which case are these different than NULL? I thought at this point there > > > hasn't been a modeset in the hotplug case and you disable the crtc on the > > > connected on boot case. This will also need to be rebased on atomic. > > As far as I tested these, they are NULL in both Hotplug and connected boot > > Cases. There was one issue during suspend/resume where it was not NULL. > > But later we figured out, we always have a valid crtc during resume and > > hence > > Should not take this path. So, yes, with all our testing so far, NULL works > > fine here. > > > > Agreed, this need to be rebased on atomic. Will do this in next version. > > > > > > + > > > > + connector->new_encoder = encoder; > > > > + encoder->new_crtc = crtc; > > > > + encoder->base.crtc = &crtc->base; > > > > + > > > > + break; > > > > + } > > > > + } > > > I think it would be a good idea to split the search for an unused crtc to > > > a > > > separate function. Also, there's similar code in > > > intel_get_load_detect_pipe(), > > > it would be nice if that could use the same function. > > Yes, I also had a similar thought, but did not get to _load_detect_pipe() > > Function. Will look at it and try to use it.. > > > > > > + > > > > + if (!found) { > > > > + DRM_ERROR("Could not find crtc for upfront link > > > > training\n"); > > > > + return false; > > > > + } > > > > + > > > > +start_link_train: > > > > + DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc > > > > ->pipe)); > > > > + found = false; > > > > + > > > > + /* Save the existing lane_count and link_bw values */ > > > > + tmp_lane_count = intel_dp->lane_count; > > > > + tmp_link_bw = intel_dp->link_bw; > > > > + > > > > + /* Initialize with Max Link rate & lane count supported by > > > > panel */ > > > > + intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; > > > > + intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & > > > > + DP_MAX_LANE_COUNT_MASK; > > > > + > > > > + /* Selects 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; > > > > + } > > > > + > > > > + do { > > > > + /* Find port clock from link_bw */ > > > > + crtc->config->port_clock = > > > > + drm_dp_bw_code_to_link_rate(intel_dp > > > > ->link_bw); > > > > + > > > > + ret = intel_ddi_pll_select(crtc, crtc->config, encoder, > > > > false); > > > > + if (!ret) { > > > > + DRM_ERROR("Could not select PLL\n"); > > > > + goto exit; > > > > + } > > > > + > > > > + pll->config.crtc_mask = (1 << crtc->pipe); > > > Is it possible that this pll is being used by another active crtc? In that > > > case > > > you steal the pll and change the configuration behind its back. > > I am not sure either. When we tested on BXT, these were always > > used by one crtc only. So, is this a valid case in BXT or in some other DDI > > based platforms ? > yes, we should handle this. the two platforms this is tested in did not > have PLL > sharing so it was not considered. > > > > + pll->config.hw_state = crtc->config->dpll_hw_state; > > > > + > > > > + DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config > > > > ->shared_dpll); > > > > + > > > > + /* Enable PLL followed by port */ > > > > + intel_enable_shared_dpll(crtc); > > > > + encoder->pre_enable(encoder); > > > > + > > > > + /* Check if link training passed; if so update lane > > > > count */ > > > > + if (intel_dp->train_set_valid) { > > > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] &= > > > > + ~DP_MAX_LANE_COUNT_MASK > > > > ; > > > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] |= > > > > + intel_dp->lane_count & > > > > DP_MAX_LANE_COUNT_MASK; > > > > + > > > > + found = true; > > > > + } > > > > + > > > > + /* Disable port followed by PLL for next retry/clean up > > > > */ > > > > + encoder->post_disable(encoder); > > > > + intel_disable_shared_dpll(crtc); > > > > + > > > > + if (found) > > > > + goto exit; > > > > + > > > > + DRM_DEBUG_KMS("upfront link training failed. lanes:%d > > > > bw:%d\n", > > > > + intel_dp->lane_count, intel_dp > > > > ->link_bw); > > > > + > > > > + /* Go down to the next level and retry link training */ > > > > + if (intel_dp->lane_count == 4) { > > > > + intel_dp->lane_count = 2; > > > > + } else if (intel_dp->lane_count == 2) { > > > > + intel_dp->lane_count = 1; > > > > + } else if (intel_dp->link_bw == DP_LINK_BW_5_4) { > > > > + intel_dp->link_bw = DP_LINK_BW_2_7; > > > > + intel_dp->lane_count = 4; > > > > + } else if (intel_dp->link_bw == DP_LINK_BW_2_7) { > > > > + intel_dp->link_bw = DP_LINK_BW_1_62; > > > > + intel_dp->lane_count = 4; > > > > + } else { > > > > + /* Tried all combinations, so exit */ > > > > + break; > > > > + } > > > I wonder what happens to the lane status dpcd registers when the cable > > > only > > > supports a reduced number of lanes. The DP standard (at least the 1.3 > > > version) > > > says that if clock recovery fails with RBR, the source device should check > > > if > > > the lower number lanes have the CR_DONE bit set, and in that case reduce > > > the > > > number of lanes, go back to the highest rate desired and continue the link > > > training. > > I believe you are talking about DPCD 202h and 203h i.e which one of them is > > actually being used/reported correctly. I will check on this with few > > different > > cables during my next round of testing. > can you please share where is it mentioned in the spec that if CR fails > we should retry with higher rate and lower lanes ? i am not aware of > such a requirement. Section 3.5.1.2.2.1 of the 1.3 version. Ander > The expectation is that if cable supports lesser no of lanes than the > number supported by panel > CR should fail for the additional lanes. resulting in the next link > training with lower lane count > to pass. that is the basic assumption of this patch :) > regards, > Sivakumar > > > > + > > > > + } while (1); > > > Maybe make this into a for (;;) loop. That way one can spot the (lack of) > > > end > > > condition earlier when reading top to bottom. > > Ok, will try this implementation.. > > > > Thank you for having a look at this patch Ander.. > > > > Thanks, > > Durga > > > > > > > > Ander > > > > > > > + > > > > +exit: > > > > + /* Restore local associations made */ > > > > + if (!valid_crtc) { > > > > + connector->new_encoder = tmp_encoder; > > > > + encoder->new_crtc = tmp_crtc; > > > > + encoder->base.crtc = tmp_drm_crtc; > > > > + } > > > > + > > > > + if (found) > > > > + DRM_DEBUG_KMS("upfront link training passed. lanes:%d > > > > bw:%d\n", > > > > + intel_dp->lane_count, intel_dp > > > > ->link_bw); > > > > + /* Restore lane_count and link_bw values */ > > > > + intel_dp->lane_count = tmp_lane_count; > > > > + intel_dp->link_bw = tmp_link_bw; > > > > + > > > > + return found; > > > > +} > > > > + > > > > 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 18bcfbe..8376b47 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp, > > > > intel_display_power_put(to_i915(encoder->base.dev), > > > > power_domain); > > > > } > > > > > > > > +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp) > > > > +{ > > > > + struct intel_digital_port *intel_dig_port = > > > > dp_to_dig_port(intel_dp); > > > > + struct drm_crtc *crtc = intel_dig_port->base.base.crtc; > > > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > > > + struct drm_device *dev = intel_encoder->base.dev; > > > > + struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : > > > > NULL; > > > > + > > > > + /* > > > > + * 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. It gets re-enabled as part of > > > > + * subsequent modeset. > > > > + */ > > > > + if (intel_encoder->connectors_active && crtc && crtc->enabled) > > > > { > > > > + DRM_DEBUG_KMS("Disabling crtc %c for upfront link > > > > training\n", > > > > + pipe_name(intel_crtc->pipe)); > > > > + intel_crtc_control(crtc, false); > > > > + } > > > > + > > > > + if (HAS_DDI(dev)) > > > > + return intel_ddi_upfront_link_train(dev, intel_dp, > > > > intel_crtc); > > > > + > > > > + /* Not a supported platform. Assume we don't need upfront_train > > > > */ > > > > + return true; > > > > +} > > > > + > > > > + > > > > static enum drm_connector_status > > > > intel_dp_detect(struct drm_connector *connector, bool force) > > > > { > > > > @@ -4794,7 +4823,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", > > > > @@ -4852,6 +4881,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(intel_dp); > > > > + if (!ret) > > > > + status = connector_status_disconnected; > > > > + } > > > > out: > > > > intel_dp_power_put(intel_dp, power_domain); > > > > return status; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 5bcdd37..82af4e6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1007,6 +1007,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 drm_device *dev, > > > > + struct intel_dp *intel_dp, struct intel_crtc *crtc); > > > > > > > > /* intel_frontbuffer.c */ > > > > void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
