[Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Mon Oct 26 00:13:26 PDT 2015
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 at gmail.com]
> > > Sent: Wednesday, October 21, 2015 9:08 PM
> > > To: R, Durgadoss; intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [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 at intel.com>
> > > > ---
> > > > 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list