| Message ID | 1444824013-23147-6-git-send-email-durgadoss.r@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Durgadoss,
[auto build test WARNING on drm-intel/for-linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/Durgadoss-R/Add-support-for-USB-typeC-based-DP/20151014-193613
config: x86_64-randconfig-s5-10142016 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/mod_devicetable.h:11,
from include/linux/i2c.h:29,
from drivers/gpu/drm/i915/intel_dp.c:28:
drivers/gpu/drm/i915/intel_dp.c: In function 'intel_dp_upfront_link_train':
drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
if (intel_encoder->connectors_active && crtc && crtc->enabled) {
^
drivers/gpu/drm/i915/intel_dp.c:4806:3: error: implicit declaration of function 'intel_crtc_control' [-Werror=implicit-function-declaration]
intel_crtc_control(crtc, false);
^
cc1: some warnings being treated as errors
vim +/if +4803 drivers/gpu/drm/i915/intel_dp.c
4787
4788 static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
4789 {
4790 struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
4791 struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
4792 struct intel_encoder *intel_encoder = &intel_dig_port->base;
4793 struct drm_device *dev = intel_encoder->base.dev;
4794 struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
4795
4796 /*
4797 * On hotplug cases, we call _upfront_link_train directly.
4798 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
4799 * BIOS typically brings up DP. Hence, we disable the crtc
4800 * to do _upfront_link_training. It gets re-enabled as part of
4801 * subsequent modeset.
4802 */
> 4803 if (intel_encoder->connectors_active && crtc && crtc->enabled) {
4804 DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
4805 pipe_name(intel_crtc->pipe));
4806 intel_crtc_control(crtc, false);
4807 }
4808
4809 if (HAS_DDI(dev))
4810 return intel_ddi_upfront_link_train(dev, intel_dp, intel_crtc);
4811
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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@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. > + > + 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. > + > + 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. > + 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. > + > + } 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. 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,
>-----Original Message----- >From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] >Sent: Wednesday, October 21, 2015 9:08 PM >To: R, Durgadoss; intel-gfx@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@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 ? > >> + 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. > >> + >> + } 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,
On 10/23/2015 5:37 PM, R, Durgadoss wrote: >> -----Original Message----- >> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] >> Sent: Wednesday, October 21, 2015 9:08 PM >> To: R, Durgadoss; intel-gfx@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@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. 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@gmail.com] > > > Sent: Wednesday, October 21, 2015 9:08 PM > > > To: R, Durgadoss; intel-gfx@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@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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; + + connector->new_encoder = encoder; + encoder->new_crtc = crtc; + encoder->base.crtc = &crtc->base; + + break; + } + } + + 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); + 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; + } + + } while (1); + +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,
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@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(-)