Hi,
plugging a USB Type-C to HDMI adapter on a Chromebook Pixel 2015 works
only if the HDMI port is in its normal (not reverted) state.
Theorically, the USB chip should provide the information whether or not
the lane are resversed, but I could not find anything in the Intel PRM
regarding this.
So use the technical solution implemented in ChromeOS by Stéphane and
Todd: try to train one lane in the normal setting, if it doesn't work,
then chances are that the lane are reverted.
The ChromeOS commits are:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e8c654a0a6d466b3a7b0e84fd27e3a7236a2243e
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4fed50fad79ba3fde782d913bac2968b2d0262bc
If I could have a Signed-off-by by Stéphane and Todd, that would be even
better :)
I should also note that while testing this, I discovered 2 regressions
in drm-intel-nightly:
- b432e5cfd (drm/i915: BDW clock change support):
this one prevents the Broadwell-U GPU to correctly set the clock when
we connect a 4K monitor over HDMI (30.0 Hz)
Using a lower resolution works (the internal display is 2560x1700)
I ended up disabling in intel_display.c all of the codes that are
broadwell specific, and the default are just working.
- aaf5ec2e5 (drm/i915: Handle HPD when it has actually occurred):
this one raises a storm of errors:
[drm:gen8_irq_handler [i915]] *ERROR* The master control interrupt lied (SDE)!
This doesn't seems to affect the DP output, but having that many
errors is not always a good sign IMO :)
Reverting this commit makes them go away.
Cheers,
Benjamin
Benjamin Tissoires (3):
drm/i915: add parameters to dp_start_link_train and
dp_complete_link_train
drm/i915: hide errors when probing for a reverse display port
drm/i915: Support DDI lane reversal for DP
drivers/gpu/drm/i915/intel_ddi.c | 13 +++
drivers/gpu/drm/i915/intel_dp.c | 173 ++++++++++++++++++++++++++++++---------
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 149 insertions(+), 38 deletions(-)
--
2.4.3
In order to detect if the Display Port is reversed or not (when connected
to a USC type-C connector), we need to probe the training with one lane
to check if the polarity is correct.
Factor out the code that we need later on.
This commit has no functional change
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/gpu/drm/i915/intel_dp.c | 75 ++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f93..f2352d8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3566,15 +3566,15 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
}
/* Enable corresponding port and start training pattern 1 */
-void
-intel_dp_start_link_train(struct intel_dp *intel_dp)
+static bool
+_intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
+ uint32_t *DP)
{
struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
struct drm_device *dev = encoder->dev;
int i;
uint8_t voltage;
int voltage_tries, loop_tries;
- uint32_t DP = intel_dp->DP;
uint8_t link_config[2];
if (HAS_DDI(dev))
@@ -3582,7 +3582,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
/* Write the link configuration data */
link_config[0] = intel_dp->link_bw;
- link_config[1] = intel_dp->lane_count;
+ link_config[1] = lane_count;
if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
@@ -3594,14 +3594,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
link_config[1] = DP_SET_ANSI_8B10B;
drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
- DP |= DP_PORT_EN;
+ *DP |= DP_PORT_EN;
/* clock recovery */
- if (!intel_dp_reset_link_train(intel_dp, &DP,
+ if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
- return;
+ return false;
}
voltage = 0xff;
@@ -3616,7 +3616,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
break;
}
- if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
+ if (drm_dp_clock_recovery_ok(link_status, lane_count)) {
DRM_DEBUG_KMS("clock recovery OK\n");
break;
}
@@ -3629,26 +3629,26 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
DRM_DEBUG_KMS("clock recovery not ok, reset");
/* clear the flag as we are not reusing train set */
intel_dp->train_set_valid = false;
- if (!intel_dp_reset_link_train(intel_dp, &DP,
+ if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
- return;
+ return false;
}
continue;
}
/* Check to see if we've tried the max voltage */
- for (i = 0; i < intel_dp->lane_count; i++)
+ for (i = 0; i < lane_count; i++)
if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
break;
- if (i == intel_dp->lane_count) {
+ if (i == lane_count) {
++loop_tries;
if (loop_tries == 5) {
DRM_ERROR("too many full retries, give up\n");
break;
}
- intel_dp_reset_link_train(intel_dp, &DP,
+ intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE);
voltage_tries = 0;
@@ -3667,21 +3667,31 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
/* Update training set as requested by target */
- if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+ if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
DRM_ERROR("failed to update link training\n");
break;
}
}
- intel_dp->DP = DP;
+ return true;
}
+/* Enable corresponding port and start training pattern 1 */
void
-intel_dp_complete_link_train(struct intel_dp *intel_dp)
+intel_dp_start_link_train(struct intel_dp *intel_dp)
+{
+ uint32_t DP = intel_dp->DP;
+
+ if (_intel_dp_start_link_train(intel_dp, intel_dp->lane_count, &DP))
+ intel_dp->DP = DP;
+}
+
+static bool
+_intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
+ uint32_t *DP)
{
bool channel_eq = false;
int tries, cr_tries;
- uint32_t DP = intel_dp->DP;
uint32_t training_pattern = DP_TRAINING_PATTERN_2;
/* Training Pattern 3 for HBR2 ot 1.2 devices that support it*/
@@ -3689,11 +3699,11 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
training_pattern = DP_TRAINING_PATTERN_3;
/* channel equalization */
- if (!intel_dp_set_link_train(intel_dp, &DP,
+ if (!intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to start channel equalization\n");
- return;
+ return false;
}
tries = 0;
@@ -3714,17 +3724,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
}
/* Make sure clock is still ok */
- if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
+ if (!drm_dp_clock_recovery_ok(link_status, lane_count)) {
intel_dp->train_set_valid = false;
- intel_dp_start_link_train(intel_dp);
- intel_dp_set_link_train(intel_dp, &DP,
+ _intel_dp_start_link_train(intel_dp, lane_count, DP);
+ intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE);
cr_tries++;
continue;
}
- if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
+ if (drm_dp_channel_eq_ok(link_status, lane_count)) {
channel_eq = true;
break;
}
@@ -3732,8 +3742,8 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
/* Try 5 times, then try clock recovery if that fails */
if (tries > 5) {
intel_dp->train_set_valid = false;
- intel_dp_start_link_train(intel_dp);
- intel_dp_set_link_train(intel_dp, &DP,
+ _intel_dp_start_link_train(intel_dp);
+ intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE);
tries = 0;
@@ -3742,7 +3752,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
}
/* Update training set as requested by target */
- if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+ if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
DRM_ERROR("failed to update link training\n");
break;
}
@@ -3751,12 +3761,21 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
intel_dp_set_idle_link_train(intel_dp);
- intel_dp->DP = DP;
-
if (channel_eq) {
intel_dp->train_set_valid = true;
DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
}
+
+ return channel_eq;
+}
+
+void
+intel_dp_complete_link_train(struct intel_dp *intel_dp)
+{
+ uint32_t DP = intel_dp->DP;
+
+ if (_intel_dp_complete_link_train(intel_dp, intel_dp->lane_count, &DP))
+ intel_dp->DP = DP;
}
void intel_dp_stop_link_train(struct intel_dp *intel_dp)
--
2.4.3
We check the polarity of the attached dp, so it is normal to fail.
Do not send errors to the users.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/gpu/drm/i915/intel_dp.c | 74 ++++++++++++++++++++++++++++++++---------
1 file changed, 58 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2352d8..b740987 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3568,7 +3568,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
/* Enable corresponding port and start training pattern 1 */
static bool
_intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
- uint32_t *DP)
+ uint32_t *DP, bool probing)
{
struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
struct drm_device *dev = encoder->dev;
@@ -3576,6 +3576,7 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
uint8_t voltage;
int voltage_tries, loop_tries;
uint8_t link_config[2];
+ char *error_str;
if (HAS_DDI(dev))
intel_ddi_prepare_link_retrain(encoder);
@@ -3600,7 +3601,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
- DRM_ERROR("failed to enable link training\n");
+ error_str = "failed to enable link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
return false;
}
@@ -3612,7 +3617,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
if (!intel_dp_get_link_status(intel_dp, link_status)) {
- DRM_ERROR("failed to get link status\n");
+ error_str = "failed to get link status\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
@@ -3632,7 +3641,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
- DRM_ERROR("failed to enable link training\n");
+ error_str = "failed to enable link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
return false;
}
continue;
@@ -3645,7 +3658,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
if (i == lane_count) {
++loop_tries;
if (loop_tries == 5) {
- DRM_ERROR("too many full retries, give up\n");
+ error_str = "too many full retries, give up\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
intel_dp_reset_link_train(intel_dp, DP,
@@ -3659,7 +3676,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
++voltage_tries;
if (voltage_tries == 5) {
- DRM_ERROR("too many voltage retries, give up\n");
+ error_str = "too many voltage retries, give up\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
} else
@@ -3668,7 +3689,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
/* Update training set as requested by target */
if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
- DRM_ERROR("failed to update link training\n");
+ error_str = "failed to update link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
}
@@ -3682,17 +3707,18 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
{
uint32_t DP = intel_dp->DP;
- if (_intel_dp_start_link_train(intel_dp, intel_dp->lane_count, &DP))
+ if (_intel_dp_start_link_train(intel_dp, intel_dp->lane_count, &DP, false))
intel_dp->DP = DP;
}
static bool
_intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
- uint32_t *DP)
+ uint32_t *DP, bool probing)
{
bool channel_eq = false;
int tries, cr_tries;
uint32_t training_pattern = DP_TRAINING_PATTERN_2;
+ char *error_str;
/* Training Pattern 3 for HBR2 ot 1.2 devices that support it*/
if (intel_dp->link_bw == DP_LINK_BW_5_4 || intel_dp->use_tps3)
@@ -3702,7 +3728,11 @@ _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
if (!intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE)) {
- DRM_ERROR("failed to start channel equalization\n");
+ error_str = "failed to start channel equalization\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
return false;
}
@@ -3713,20 +3743,28 @@ _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
uint8_t link_status[DP_LINK_STATUS_SIZE];
if (cr_tries > 5) {
- DRM_ERROR("failed to train DP, aborting\n");
+ error_str = "failed to train DP, aborting\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
if (!intel_dp_get_link_status(intel_dp, link_status)) {
- DRM_ERROR("failed to get link status\n");
+ error_str = "failed to get link status\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
/* Make sure clock is still ok */
if (!drm_dp_clock_recovery_ok(link_status, lane_count)) {
intel_dp->train_set_valid = false;
- _intel_dp_start_link_train(intel_dp, lane_count, DP);
+ _intel_dp_start_link_train(intel_dp, lane_count, DP, probing);
intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE);
@@ -3742,7 +3780,7 @@ _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
/* Try 5 times, then try clock recovery if that fails */
if (tries > 5) {
intel_dp->train_set_valid = false;
- _intel_dp_start_link_train(intel_dp);
+ _intel_dp_start_link_train(intel_dp, lane_count, DP, probing);
intel_dp_set_link_train(intel_dp, DP,
training_pattern |
DP_LINK_SCRAMBLING_DISABLE);
@@ -3753,7 +3791,11 @@ _intel_dp_complete_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
/* Update training set as requested by target */
if (!intel_dp_update_link_train(intel_dp, DP, link_status)) {
- DRM_ERROR("failed to update link training\n");
+ error_str = "failed to update link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
++tries;
@@ -3774,7 +3816,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
{
uint32_t DP = intel_dp->DP;
- if (_intel_dp_complete_link_train(intel_dp, intel_dp->lane_count, &DP))
+ if (_intel_dp_complete_link_train(intel_dp, intel_dp->lane_count, &DP, false))
intel_dp->DP = DP;
}
--
2.4.3
The DP outputs connected through a USB Type-C port can have inverted
lanes. To detect that case, we implement autodetection by training only
the first lane if it doesn't work, we assume that we need to invert
the lanes.
Tested on a Chromebook Pixel 2015 (samus) with a USB Type-C to HDMI
adapter and a Dell 4K and some various regular monitors.
Based on 2 patches from the ChromeOS tree by:
Stéphane Marchesin <[email protected]>
Todd Broch <[email protected]>
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9a40bfb..0b0c1ec 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2249,6 +2249,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
enum port port = intel_ddi_get_encoder_port(intel_encoder);
int type = intel_encoder->type;
int hdmi_level;
+ bool reversed = false;
if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
@@ -2295,8 +2296,20 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ if (IS_BROADWELL(dev) && type == INTEL_OUTPUT_DISPLAYPORT) {
+ intel_ddi_init_dp_buf_reg(intel_encoder);
+ reversed = intel_dp_is_reversed(intel_dp);
+ }
+
intel_ddi_init_dp_buf_reg(intel_encoder);
+ if (IS_BROADWELL(dev)) {
+ if (reversed)
+ intel_dp->DP |= DDI_BUF_PORT_REVERSAL;
+ else
+ intel_dp->DP &= ~DDI_BUF_PORT_REVERSAL;
+ }
+
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b740987..18280cc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3820,6 +3820,42 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
intel_dp->DP = DP;
}
+bool intel_dp_is_reversed(struct intel_dp *intel_dp)
+{
+ struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
+ struct drm_device *dev = encoder->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint32_t DP = intel_dp->DP;
+
+ /*
+ * Train with 1 lane. There is no guarantee that the monitor supports
+ * 2 or 4 lanes, and we wouldn't see any asymetricity with 4 lanes.
+ */
+ const uint8_t lane_count = 1;
+ bool reversed;
+
+ if (!HAS_DDI(dev))
+ return false;
+
+ DP &= ~(DDI_BUF_PORT_REVERSAL | DDI_PORT_WIDTH(4));
+ DP |= DDI_PORT_WIDTH(lane_count);
+
+ I915_WRITE(intel_dp->output_reg, DP);
+ POSTING_READ(intel_dp->output_reg);
+ udelay(600);
+
+ if (!_intel_dp_start_link_train(intel_dp, lane_count, &DP, true))
+ return true;
+
+ reversed = !_intel_dp_complete_link_train(intel_dp, lane_count, &DP, true);
+
+ /* clear training, we had only one lane */
+ intel_dp->train_set_valid = false;
+
+ return reversed;
+
+}
+
void intel_dp_stop_link_train(struct intel_dp *intel_dp)
{
intel_dp_set_link_train(intel_dp, &intel_dp->DP,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6..cba00c6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1169,6 +1169,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
bool intel_dp_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *pipe_config);
bool intel_dp_is_edp(struct drm_device *dev, enum port port);
+bool intel_dp_is_reversed(struct intel_dp *intel_dp);
enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
bool long_hpd);
void intel_edp_backlight_on(struct intel_dp *intel_dp);
--
2.4.3
On Tue, Jul 28, 2015 at 12:03:28PM -0400, Benjamin Tissoires wrote:
> We check the polarity of the attached dp, so it is normal to fail.
> Do not send errors to the users.
if (probe) DRM_DEBUG else DRM_ERROR is fairly offensive.
It strikes me that you could make each of these functions report the
failure to the caller (have the caller even do so error handling!) and
as part of that have the caller report an error and so demote all of
these to DRM_DEBUG_KMS().
Who knows, actually doing some error handling may make monitor training
more reliable! Or we may even get carried away and report the failure
all the way back to userspace.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you
can identify both lane count and reversal state without touching
anything in the link training code. i am yet to upstream my changes for
CHT that i can share if required that does the same in intel_dp_detect
without touching any line in link training path.
On 7/28/2015 9:33 PM, Benjamin Tissoires wrote:
> The DP outputs connected through a USB Type-C port can have inverted
> lanes. To detect that case, we implement autodetection by training only
> the first lane if it doesn't work, we assume that we need to invert
> the lanes.
>
> Tested on a Chromebook Pixel 2015 (samus) with a USB Type-C to HDMI
> adapter and a Dell 4K and some various regular monitors.
>
> Based on 2 patches from the ChromeOS tree by:
> Stéphane Marchesin <[email protected]>
> Todd Broch <[email protected]>
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
> drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9a40bfb..0b0c1ec 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2249,6 +2249,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> int type = intel_encoder->type;
> int hdmi_level;
> + bool reversed = false;
>
> if (type == INTEL_OUTPUT_EDP) {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> @@ -2295,8 +2296,20 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>
> + if (IS_BROADWELL(dev) && type == INTEL_OUTPUT_DISPLAYPORT) {
> + intel_ddi_init_dp_buf_reg(intel_encoder);
> + reversed = intel_dp_is_reversed(intel_dp);
> + }
> +
> intel_ddi_init_dp_buf_reg(intel_encoder);
>
> + if (IS_BROADWELL(dev)) {
> + if (reversed)
> + intel_dp->DP |= DDI_BUF_PORT_REVERSAL;
> + else
> + intel_dp->DP &= ~DDI_BUF_PORT_REVERSAL;
> + }
> +
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> intel_dp_start_link_train(intel_dp);
> intel_dp_complete_link_train(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b740987..18280cc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3820,6 +3820,42 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> intel_dp->DP = DP;
> }
>
> +bool intel_dp_is_reversed(struct intel_dp *intel_dp)
> +{
> + struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> + struct drm_device *dev = encoder->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + uint32_t DP = intel_dp->DP;
> +
> + /*
> + * Train with 1 lane. There is no guarantee that the monitor supports
> + * 2 or 4 lanes, and we wouldn't see any asymetricity with 4 lanes.
> + */
> + const uint8_t lane_count = 1;
> + bool reversed;
> +
> + if (!HAS_DDI(dev))
> + return false;
> +
> + DP &= ~(DDI_BUF_PORT_REVERSAL | DDI_PORT_WIDTH(4));
> + DP |= DDI_PORT_WIDTH(lane_count);
> +
> + I915_WRITE(intel_dp->output_reg, DP);
> + POSTING_READ(intel_dp->output_reg);
> + udelay(600);
> +
> + if (!_intel_dp_start_link_train(intel_dp, lane_count, &DP, true))
> + return true;
> +
> + reversed = !_intel_dp_complete_link_train(intel_dp, lane_count, &DP, true);
> +
> + /* clear training, we had only one lane */
> + intel_dp->train_set_valid = false;
> +
> + return reversed;
> +
> +}
> +
> void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> {
> intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6..cba00c6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1169,6 +1169,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> bool intel_dp_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config);
> bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> +bool intel_dp_is_reversed(struct intel_dp *intel_dp);
> enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
> bool long_hpd);
> void intel_edp_backlight_on(struct intel_dp *intel_dp);
--
regards,
Sivakumar
On Jul 28 2015 or thereabouts, Chris Wilson wrote:
> On Tue, Jul 28, 2015 at 12:03:28PM -0400, Benjamin Tissoires wrote:
> > We check the polarity of the attached dp, so it is normal to fail.
> > Do not send errors to the users.
>
> if (probe) DRM_DEBUG else DRM_ERROR is fairly offensive.
>
> It strikes me that you could make each of these functions report the
> failure to the caller (have the caller even do so error handling!) and
> as part of that have the caller report an error and so demote all of
> these to DRM_DEBUG_KMS().
Yes, sorry for that. I will change it to return an actual error code and
the error string if I still need to use these functions given Sivakumar
ideas.
>
> Who knows, actually doing some error handling may make monitor training
> more reliable! Or we may even get carried away and report the failure
> all the way back to userspace.
That would be a very good improvement indeed. But I can already tell you
that I will not do it by myself, I already have too much on my plate.
I'll do my share for this feature, but don't count on me for the whole
error handling rewrite :)
Cheers,
Benjamin
On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> identify both lane count and reversal state without touching anything in the
> link training code. i am yet to upstream my changes for CHT that i can share
> if required that does the same in intel_dp_detect without touching any line
> in link training path.
With my current limited knowledge of the dp hotplug (and i915 driver) I
am not sure we could detect the reversed state without trying to train 1
lane only. I'd be glad to look at your changes and test them on my
system if you think that could help having a cleaner solution.
Cheers,
Benjamin
>
> On 7/28/2015 9:33 PM, Benjamin Tissoires wrote:
> >The DP outputs connected through a USB Type-C port can have inverted
> >lanes. To detect that case, we implement autodetection by training only
> >the first lane if it doesn't work, we assume that we need to invert
> >the lanes.
> >
> >Tested on a Chromebook Pixel 2015 (samus) with a USB Type-C to HDMI
> >adapter and a Dell 4K and some various regular monitors.
> >
> >Based on 2 patches from the ChromeOS tree by:
> >Stéphane Marchesin <[email protected]>
> >Todd Broch <[email protected]>
> >
> >Signed-off-by: Benjamin Tissoires <[email protected]>
> >---
> > drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 50 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >index 9a40bfb..0b0c1ec 100644
> >--- a/drivers/gpu/drm/i915/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >@@ -2249,6 +2249,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > int type = intel_encoder->type;
> > int hdmi_level;
> >+ bool reversed = false;
> > if (type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >@@ -2295,8 +2296,20 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >+ if (IS_BROADWELL(dev) && type == INTEL_OUTPUT_DISPLAYPORT) {
> >+ intel_ddi_init_dp_buf_reg(intel_encoder);
> >+ reversed = intel_dp_is_reversed(intel_dp);
> >+ }
> >+
> > intel_ddi_init_dp_buf_reg(intel_encoder);
> >+ if (IS_BROADWELL(dev)) {
> >+ if (reversed)
> >+ intel_dp->DP |= DDI_BUF_PORT_REVERSAL;
> >+ else
> >+ intel_dp->DP &= ~DDI_BUF_PORT_REVERSAL;
> >+ }
> >+
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > intel_dp_start_link_train(intel_dp);
> > intel_dp_complete_link_train(intel_dp);
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >index b740987..18280cc 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -3820,6 +3820,42 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> > intel_dp->DP = DP;
> > }
> >+bool intel_dp_is_reversed(struct intel_dp *intel_dp)
> >+{
> >+ struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> >+ struct drm_device *dev = encoder->dev;
> >+ struct drm_i915_private *dev_priv = dev->dev_private;
> >+ uint32_t DP = intel_dp->DP;
> >+
> >+ /*
> >+ * Train with 1 lane. There is no guarantee that the monitor supports
> >+ * 2 or 4 lanes, and we wouldn't see any asymetricity with 4 lanes.
> >+ */
> >+ const uint8_t lane_count = 1;
> >+ bool reversed;
> >+
> >+ if (!HAS_DDI(dev))
> >+ return false;
> >+
> >+ DP &= ~(DDI_BUF_PORT_REVERSAL | DDI_PORT_WIDTH(4));
> >+ DP |= DDI_PORT_WIDTH(lane_count);
> >+
> >+ I915_WRITE(intel_dp->output_reg, DP);
> >+ POSTING_READ(intel_dp->output_reg);
> >+ udelay(600);
> >+
> >+ if (!_intel_dp_start_link_train(intel_dp, lane_count, &DP, true))
> >+ return true;
> >+
> >+ reversed = !_intel_dp_complete_link_train(intel_dp, lane_count, &DP, true);
> >+
> >+ /* clear training, we had only one lane */
> >+ intel_dp->train_set_valid = false;
> >+
> >+ return reversed;
> >+
> >+}
> >+
> > void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> > {
> > intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >index 320c9e6..cba00c6 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -1169,6 +1169,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> > bool intel_dp_compute_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *pipe_config);
> > bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> >+bool intel_dp_is_reversed(struct intel_dp *intel_dp);
> > enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
> > bool long_hpd);
> > void intel_edp_backlight_on(struct intel_dp *intel_dp);
>
> --
> regards,
> Sivakumar
>
On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
>> identify both lane count and reversal state without touching anything in the
>> link training code. i am yet to upstream my changes for CHT that i can share
>> if required that does the same in intel_dp_detect without touching any line
>> in link training path.
> With my current limited knowledge of the dp hotplug (and i915 driver) I
> am not sure we could detect the reversed state without trying to train 1
> lane only. I'd be glad to look at your changes and test them on my
> system if you think that could help having a cleaner solution.
>
> Cheers,
> Benjamin
No, what i recommended was to do link training but in intel_dp_detect.
Since USB Type C cable
also has its own lane count restriction (it can have different lane
count than the one supported
by panel) you might have to figure that out as well. so both reversal
and lane count detection
can be done outside the modeset path and keep the code free of type C
changes outside
detection path.
Please find below the code to do the same. Do not waste time trying to
apply this directly on
nightly since this is based on a local tree and because this is pre-
atomic changes code, so you
might have to modify chv_upfront_link_train to work on top of the latest
nightly code. we
are supposed to upstream this and is in my todo list.
---------------------------------------------------------------------------------------------------------------------------
Author: Durgadoss R <[email protected]>
Date: Fri May 22 14:30:07 2015 +0530
drm/i915: Enable Upfront link training for type-C DP support
To support USB type C alternate DP mode, the display driver needs
to know the
number of lanes required by 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
the link
training fails, the driver then falls back to x2 lanes.
* Since link training is done before modeset, planes are not
enabled. Only
encoder and the its associated PLLs are enabled.
* Once link training is done, the encoder and its PLLs are
disabled; so that
the subsequent modeset is not aware of these changes.
* As of now, this is tested only on CHV.
Signed-off-by: Durgadoss R <[email protected]>
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 0c8ae2a..c72dcaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14793,3 +14793,121 @@ intel_display_print_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, " VSYNC: %08x\n", error->transcoder[i].vsync);
}
}
+
+bool chv_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_connector *connector = intel_dp->attached_connector;
+ struct intel_encoder *encoder = connector->encoder;
+ bool found = false;
+ bool valid_crtc = false;
+
+ 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;
+
+ connector->new_encoder = encoder;
+ encoder->new_crtc = crtc;
+ encoder->base.crtc = &crtc->base;
+
+ /* Make sure the new crtc will work with the encoder */
+ if (drm_encoder_crtc_ok(&encoder->base,
+ &crtc->base)) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ DRM_ERROR("Could not find crtc for upfront link training\n");
+ return false;
+ }
+
+start_link_train:
+
+ DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
+ pipe_name(crtc->pipe));
+ found = false;
+
+ /* 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;
+
+ do {
+ /* Find port clock from link_bw */
+ crtc->config.port_clock =
+ drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+ /* Enable PLL followed by port */
+ intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw);
+ chv_update_pll(crtc);
+ encoder->pre_pll_enable(encoder);
+ chv_enable_pll(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;
+ }
+
+ /* Reset encoder for next retry or for clean up */
+ encoder->disable(encoder);
+ encoder->post_disable(encoder);
+ chv_disable_pll(dev_priv, crtc->pipe);
+
+ 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:
+ /* Clear local associations made */
+ if (!valid_crtc) {
+ connector->new_encoder = NULL;
+ encoder->new_crtc = NULL;
+ encoder->base.crtc = NULL;
+ }
+
+ if (found)
+ DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
+ intel_dp->lane_count, intel_dp->link_bw);
+ return found;
+}
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 97f03bf..394a7a5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -878,7 +878,7 @@ intel_dp_connector_unregister(struct intel_connector
*intel_connector)
intel_connector_unregister(intel_connector);
}
-static void
+void
intel_dp_set_clock(struct intel_encoder *encoder,
struct intel_crtc_config *pipe_config, int link_bw)
{
@@ -1010,7 +1010,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
link_clock = drm_dp_bw_code_to_link_rate(bws[clock]);
link_avail = intel_dp_max_data_rate(link_clock,
lane_count);
-
if (mode_rate <= link_avail) {
goto found;
}
@@ -4522,6 +4521,11 @@ g4x_dp_detect(struct intel_dp *intel_dp)
if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0)
return connector_status_disconnected;
+ /* Avoid DPCD opertations if status is same */
+ if (intel_dp->attached_connector->base.status ==
+ connector_status_connected)
+ return connector_status_connected;
+
return intel_dp_detect_dpcd(intel_dp);
}
@@ -4566,11 +4570,13 @@ intel_dp_detect(struct drm_connector *connector,
bool force)
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct intel_encoder *intel_encoder = &intel_dig_port->base;
+ struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
struct drm_device *dev = connector->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
enum drm_connector_status status;
enum intel_display_power_domain power_domain;
struct edid *edid = NULL;
+ struct intel_crtc *intel_crtc = NULL;
intel_runtime_pm_get(dev_priv);
@@ -4590,6 +4596,11 @@ intel_dp_detect(struct drm_connector *connector,
bool force)
if (status != connector_status_connected)
goto out;
+ if (connector->status == connector_status_connected) {
+ DRM_DEBUG_KMS("Connector status is already connected\n");
+ goto out;
+ }
+
intel_dp_probe_oui(intel_dp);
if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
@@ -4604,6 +4615,22 @@ intel_dp_detect(struct drm_connector *connector,
bool force)
if (intel_encoder->type != INTEL_OUTPUT_EDP)
intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+
+ if (IS_CHERRYVIEW(dev) &&
+ intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
+ /* Handle connected boot scenario where panel is enabled
+ * by GOP/VBIOS.
+ */
+ if (intel_encoder->connectors_active &&
+ crtc && crtc->enabled) {
+ intel_crtc = to_intel_crtc(crtc);
+ DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
+ pipe_name(intel_crtc->pipe));
+ intel_crtc_control(crtc, false);
+ }
+ chv_upfront_link_train(dev, intel_dp, intel_crtc);
+ }
+
status = connector_status_connected;
out:
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index fc266da..15eef94d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -991,6 +991,10 @@ void intel_dp_start_link_train(struct intel_dp
*intel_dp);
void intel_dp_complete_link_train(struct intel_dp *intel_dp);
void intel_dp_stop_link_train(struct intel_dp *intel_dp);
bool intel_dp_fast_link_train(struct intel_dp *intel_dp);
+bool chv_upfront_link_train(struct drm_device *dev,
+ struct intel_dp *intel_dp, struct intel_crtc *crtc);
+void intel_dp_set_clock(struct intel_encoder *encoder,
+ struct intel_crtc_config *pipe_config, int link_bw);
void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
void intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n
*m_n);
void intel_dp_encoder_destroy(struct drm_encoder *encoder);
--
regards,
Sivakumar
On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>
>
> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >>identify both lane count and reversal state without touching anything in the
> >>link training code. i am yet to upstream my changes for CHT that i can share
> >>if required that does the same in intel_dp_detect without touching any line
> >>in link training path.
> >With my current limited knowledge of the dp hotplug (and i915 driver) I
> >am not sure we could detect the reversed state without trying to train 1
> >lane only. I'd be glad to look at your changes and test them on my
> >system if you think that could help having a cleaner solution.
> >
> >Cheers,
> >Benjamin
> No, what i recommended was to do link training but in intel_dp_detect. Since
> USB Type C cable
> also has its own lane count restriction (it can have different lane count
> than the one supported
> by panel) you might have to figure that out as well. so both reversal and
> lane count detection
> can be done outside the modeset path and keep the code free of type C
> changes outside
> detection path.
Thanks for the detailed explanations. Now I see what you mean and
definitively understand why this is better.
>
> Please find below the code to do the same. Do not waste time trying to apply
> this directly on
> nightly since this is based on a local tree and because this is pre- atomic
> changes code, so you
> might have to modify chv_upfront_link_train to work on top of the latest
> nightly code. we
> are supposed to upstream this and is in my todo list.
Thanks for this patch. I'll try to adapt it to test on the Broadwell
laptop I have and get the reversed state detected here.
Cheers,
Benjamin
>
> ---------------------------------------------------------------------------------------------------------------------------
>
> Author: Durgadoss R <[email protected]>
> Date: Fri May 22 14:30:07 2015 +0530
>
> drm/i915: Enable Upfront link training for type-C DP support
>
> To support USB type C alternate DP mode, the display driver needs to
> know the
> number of lanes required by 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 the
> link
> training fails, the driver then falls back to x2 lanes.
>
> * Since link training is done before modeset, planes are not enabled.
> Only
> encoder and the its associated PLLs are enabled.
> * Once link training is done, the encoder and its PLLs are disabled; so
> that
> the subsequent modeset is not aware of these changes.
> * As of now, this is tested only on CHV.
>
> Signed-off-by: Durgadoss R <[email protected]>
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0c8ae2a..c72dcaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14793,3 +14793,121 @@ intel_display_print_error_state(struct
> drm_i915_error_state_buf *m,
> err_printf(m, " VSYNC: %08x\n", error->transcoder[i].vsync);
> }
> }
> +
> +bool chv_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_connector *connector = intel_dp->attached_connector;
> + struct intel_encoder *encoder = connector->encoder;
> + bool found = false;
> + bool valid_crtc = false;
> +
> + 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;
> +
> + connector->new_encoder = encoder;
> + encoder->new_crtc = crtc;
> + encoder->base.crtc = &crtc->base;
> +
> + /* Make sure the new crtc will work with the encoder */
> + if (drm_encoder_crtc_ok(&encoder->base,
> + &crtc->base)) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + DRM_ERROR("Could not find crtc for upfront link training\n");
> + return false;
> + }
> +
> +start_link_train:
> +
> + DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
> + pipe_name(crtc->pipe));
> + found = false;
> +
> + /* 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;
> +
> + do {
> + /* Find port clock from link_bw */
> + crtc->config.port_clock =
> + drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> +
> + /* Enable PLL followed by port */
> + intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw);
> + chv_update_pll(crtc);
> + encoder->pre_pll_enable(encoder);
> + chv_enable_pll(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;
> + }
> +
> + /* Reset encoder for next retry or for clean up */
> + encoder->disable(encoder);
> + encoder->post_disable(encoder);
> + chv_disable_pll(dev_priv, crtc->pipe);
> +
> + 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:
> + /* Clear local associations made */
> + if (!valid_crtc) {
> + connector->new_encoder = NULL;
> + encoder->new_crtc = NULL;
> + encoder->base.crtc = NULL;
> + }
> +
> + if (found)
> + DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
> + intel_dp->lane_count, intel_dp->link_bw);
> + return found;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 97f03bf..394a7a5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -878,7 +878,7 @@ intel_dp_connector_unregister(struct intel_connector
> *intel_connector)
> intel_connector_unregister(intel_connector);
> }
>
> -static void
> +void
> intel_dp_set_clock(struct intel_encoder *encoder,
> struct intel_crtc_config *pipe_config, int link_bw)
> {
> @@ -1010,7 +1010,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> link_clock = drm_dp_bw_code_to_link_rate(bws[clock]);
> link_avail = intel_dp_max_data_rate(link_clock,
> lane_count);
> -
> if (mode_rate <= link_avail) {
> goto found;
> }
> @@ -4522,6 +4521,11 @@ g4x_dp_detect(struct intel_dp *intel_dp)
> if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0)
> return connector_status_disconnected;
>
> + /* Avoid DPCD opertations if status is same */
> + if (intel_dp->attached_connector->base.status ==
> + connector_status_connected)
> + return connector_status_connected;
> +
> return intel_dp_detect_dpcd(intel_dp);
> }
>
> @@ -4566,11 +4570,13 @@ intel_dp_detect(struct drm_connector *connector,
> bool force)
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct intel_encoder *intel_encoder = &intel_dig_port->base;
> + struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> struct drm_device *dev = connector->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> enum drm_connector_status status;
> enum intel_display_power_domain power_domain;
> struct edid *edid = NULL;
> + struct intel_crtc *intel_crtc = NULL;
>
> intel_runtime_pm_get(dev_priv);
>
> @@ -4590,6 +4596,11 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
> if (status != connector_status_connected)
> goto out;
>
> + if (connector->status == connector_status_connected) {
> + DRM_DEBUG_KMS("Connector status is already connected\n");
> + goto out;
> + }
> +
> intel_dp_probe_oui(intel_dp);
>
> if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
> @@ -4604,6 +4615,22 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +
> + if (IS_CHERRYVIEW(dev) &&
> + intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> + /* Handle connected boot scenario where panel is enabled
> + * by GOP/VBIOS.
> + */
> + if (intel_encoder->connectors_active &&
> + crtc && crtc->enabled) {
> + intel_crtc = to_intel_crtc(crtc);
> + DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
> + pipe_name(intel_crtc->pipe));
> + intel_crtc_control(crtc, false);
> + }
> + chv_upfront_link_train(dev, intel_dp, intel_crtc);
> + }
> +
> status = connector_status_connected;
>
> out:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index fc266da..15eef94d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -991,6 +991,10 @@ void intel_dp_start_link_train(struct intel_dp
> *intel_dp);
> void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> bool intel_dp_fast_link_train(struct intel_dp *intel_dp);
> +bool chv_upfront_link_train(struct drm_device *dev,
> + struct intel_dp *intel_dp, struct intel_crtc *crtc);
> +void intel_dp_set_clock(struct intel_encoder *encoder,
> + struct intel_crtc_config *pipe_config, int link_bw);
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> void intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n
> *m_n);
> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>
> --
> regards,
> Sivakumar
>
On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>
>
> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >>identify both lane count and reversal state without touching anything in the
> >>link training code. i am yet to upstream my changes for CHT that i can share
> >>if required that does the same in intel_dp_detect without touching any line
> >>in link training path.
> >With my current limited knowledge of the dp hotplug (and i915 driver) I
> >am not sure we could detect the reversed state without trying to train 1
> >lane only. I'd be glad to look at your changes and test them on my
> >system if you think that could help having a cleaner solution.
> >
> >Cheers,
> >Benjamin
> No, what i recommended was to do link training but in intel_dp_detect. Since
> USB Type C cable
> also has its own lane count restriction (it can have different lane count
> than the one supported
> by panel) you might have to figure that out as well. so both reversal and
> lane count detection
> can be done outside the modeset path and keep the code free of type C
> changes outside
> detection path.
>
> Please find below the code to do the same. Do not waste time trying to apply
> this directly on
> nightly since this is based on a local tree and because this is pre- atomic
> changes code, so you
> might have to modify chv_upfront_link_train to work on top of the latest
> nightly code. we
> are supposed to upstream this and is in my todo list.
>
[original patch snipped...]
Hi Sivakumar,
So I managed to manually re-apply your patch on top of
drm-intel-nightly, and tried to port it to make Broadwell working too.
It works OK if the system is already boot without any external DP used.
In this case, the detection works and I can see my external monitor
working properly.
However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
understand why. I think I enabled all that is mentioned in the PRM to be
able to train the DP link, but I am obviously missing something else.
Can you have a look?
My patch is now:
>From 11e9c42f55ae27bd6e7b0168bf24d15082c7d517 Mon Sep 17 00:00:00 2001
From: Durgadoss R <[email protected]>
Date: Mon, 3 Aug 2015 11:51:16 -0400
Subject: [PATCH] drm/i915: Enable Upfront link training for type-C DP support
To support USB type C alternate DP mode, the display driver needs to know the
number of lanes required by 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 the link
training fails, the driver then falls back to x2 lanes.
* Since link training is done before modeset, planes are not enabled. Only
encoder and the its associated PLLs are enabled.
* Once link training is done, the encoder and its PLLs are disabled; so that
the subsequent modeset is not aware of these changes.
* As of now, this is tested only on CHV.
Signed-off-by: Durgadoss R <[email protected]>
[BT: add broadwell support and USB-C reverted state detection]
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 163 +++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_dp.c | 22 ++++-
drivers/gpu/drm/i915/intel_drv.h | 7 ++
3 files changed, 190 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 818f3a3..e8ddba0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15799,3 +15799,166 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
spin_unlock_irq(&dev->event_lock);
}
}
+
+static bool
+intel_upfront_link_train_config(struct drm_device *dev,
+ struct intel_dp *intel_dp,
+ struct intel_crtc *crtc,
+ uint8_t link_bw,
+ uint8_t lane_count)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_connector *connector = intel_dp->attached_connector;
+ struct intel_encoder *encoder = connector->encoder;
+ bool success;
+
+ intel_dp->link_bw = link_bw;
+ intel_dp->lane_count = lane_count;
+
+ /* Find port clock from link_bw */
+ crtc->config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+ /* Enable PLL followed by port */
+ if (IS_BROADWELL(dev)) {
+ hsw_dp_set_ddi_pll_sel(crtc->config, intel_dp->link_bw);
+ if (intel_crtc_to_shared_dpll(crtc))
+ intel_enable_shared_dpll(crtc);
+
+ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
+
+ } else if (IS_CHERRYVIEW(dev)) {
+ intel_dp_set_clock(encoder, crtc->config, intel_dp->link_bw);
+ if (encoder->pre_pll_enable)
+ encoder->pre_pll_enable(encoder);
+ if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+ chv_enable_pll(crtc, crtc->config);
+ }
+ if (encoder->pre_enable)
+ encoder->pre_enable(encoder);
+
+ success = intel_dp->train_set_valid;
+
+ /* Reset encoder for next retry or for clean up */
+ if (encoder->post_disable)
+ encoder->post_disable(encoder);
+ if (IS_CHERRYVIEW(dev)) {
+ if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+ chv_disable_pll(dev_priv, crtc->pipe);
+ } else if (IS_BROADWELL(dev)) {
+ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
+ if (intel_crtc_to_shared_dpll(crtc))
+ intel_disable_shared_dpll(crtc);
+ }
+
+ intel_dp->train_set_valid = false;
+
+ return success;
+}
+
+bool
+intel_upfront_link_train(struct drm_device *dev,
+ struct intel_dp *intel_dp,
+ struct intel_crtc *crtc)
+{
+ struct intel_connector *connector = intel_dp->attached_connector;
+ struct intel_encoder *encoder = connector->encoder;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(&encoder->base);
+ uint8_t link_bw, lane_count;
+ bool found = false;
+ bool valid_crtc = !!crtc;
+
+ if (!connector || !encoder) {
+ DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
+ return false;
+ }
+
+ /* Find an unused crtc and use it for link training if no crtc is given */
+ if (!crtc) {
+ 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))
+ continue;
+
+ encoder->base.crtc = &crtc->base;
+
+ found = true;
+ break;
+ }
+
+ if (!found) {
+ DRM_ERROR("Could not find crtc for upfront link training\n");
+ goto exit;
+ }
+ }
+
+ DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
+ pipe_name(crtc->pipe));
+
+ /* check the reversed state of the adapter first */
+
+ /* unset the reversal bit */
+ intel_dig_port->saved_port_bits &= ~DDI_BUF_PORT_REVERSAL;
+
+ /* Initialize with Max Link rate supported by panel & one lane */
+ intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
+ intel_dp->lane_count = 1;
+
+ /* Check if link training passed; if not, update the reversed state */
+ if (!intel_upfront_link_train_config(dev, intel_dp, crtc,
+ intel_dp->dpcd[DP_MAX_LINK_RATE], 1))
+ intel_dig_port->saved_port_bits |= DDI_BUF_PORT_REVERSAL;
+
+ found = false;
+
+ /* Initialize with Max Link rate & lane count supported by panel */
+ link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
+ lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK;
+
+ do {
+ /* Check if link training passed; if so update lane count */
+ if (intel_upfront_link_train_config(dev, intel_dp, crtc,
+ link_bw, lane_count)) {
+ 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;
+ 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 (lane_count == 4) {
+ lane_count = 2;
+ } else if (lane_count == 2) {
+ lane_count = 1;
+ } else if (link_bw == DP_LINK_BW_5_4) {
+ link_bw = DP_LINK_BW_2_7;
+ lane_count = 4;
+ } else if (link_bw == DP_LINK_BW_2_7) {
+ link_bw = DP_LINK_BW_1_62;
+ lane_count = 4;
+ } else {
+ /* Tried all combinations, so exit */
+ break;
+ }
+
+ } while (1);
+
+exit:
+ /* Clear local associations made */
+ if (!valid_crtc)
+ encoder->base.crtc = NULL;
+
+ if (found)
+ DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
+ intel_dp->lane_count, intel_dp->link_bw);
+
+ return found;
+}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44f8a32..187800b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1144,7 +1144,7 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
pipe_config->dpll_hw_state.ctrl1 = ctrl1;
}
-static void
+void
hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
{
memset(&pipe_config->dpll_hw_state, 0,
@@ -1202,7 +1202,7 @@ intel_dp_source_rates(struct drm_device *dev, const int **source_rates)
return (DP_LINK_BW_2_7 >> 3) + 1;
}
-static void
+void
intel_dp_set_clock(struct intel_encoder *encoder,
struct intel_crtc_state *pipe_config, int link_bw)
{
@@ -4423,6 +4423,10 @@ g4x_dp_detect(struct intel_dp *intel_dp)
else if (ret == 0)
return connector_status_disconnected;
+ /* Avoid DPCD opertations if status is same */
+ if (intel_dp->attached_connector->base.status == connector_status_connected)
+ return connector_status_connected;
+
return intel_dp_detect_dpcd(intel_dp);
}
@@ -4495,7 +4499,9 @@ intel_dp_detect(struct drm_connector *connector, bool force)
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct intel_encoder *intel_encoder = &intel_dig_port->base;
+ struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
struct drm_device *dev = connector->dev;
+ struct intel_crtc *intel_crtc;
enum drm_connector_status status;
enum intel_display_power_domain power_domain;
bool ret;
@@ -4540,6 +4546,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
if (intel_encoder->type != INTEL_OUTPUT_EDP)
intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+
+ if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
+ (IS_BROADWELL(dev) || IS_CHERRYVIEW(dev))) {
+ /* Do not handle connected boot scenario where panel is enabled
+ * by GOP/VBIOS.
+ */
+ if ((connector->status != connector_status_connected) &&
+ !(intel_encoder->connectors_active &&
+ crtc && crtc->enabled))
+ intel_upfront_link_train(dev, intel_dp, NULL);
+ }
+
status = connector_status_connected;
/* Try to read the source of the interrupt */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6..fcc95d5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1163,6 +1163,13 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
void intel_dp_start_link_train(struct intel_dp *intel_dp);
void intel_dp_complete_link_train(struct intel_dp *intel_dp);
void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+bool intel_upfront_link_train(struct drm_device *dev,
+ struct intel_dp *intel_dp,
+ struct intel_crtc *crtc);
+void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw);
+void intel_dp_set_clock(struct intel_encoder *encoder,
+ struct intel_crtc_state *pipe_config, int link_bw);
+
void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
void intel_dp_encoder_destroy(struct drm_encoder *encoder);
int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
--
2.4.3
Cheers,
Benjamin
On 8/6/2015 1:04 AM, Benjamin Tissoires wrote:
> On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>
>> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
>>> On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>>> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
>>>> identify both lane count and reversal state without touching anything in the
>>>> link training code. i am yet to upstream my changes for CHT that i can share
>>>> if required that does the same in intel_dp_detect without touching any line
>>>> in link training path.
>>> With my current limited knowledge of the dp hotplug (and i915 driver) I
>>> am not sure we could detect the reversed state without trying to train 1
>>> lane only. I'd be glad to look at your changes and test them on my
>>> system if you think that could help having a cleaner solution.
>>>
>>> Cheers,
>>> Benjamin
>> No, what i recommended was to do link training but in intel_dp_detect. Since
>> USB Type C cable
>> also has its own lane count restriction (it can have different lane count
>> than the one supported
>> by panel) you might have to figure that out as well. so both reversal and
>> lane count detection
>> can be done outside the modeset path and keep the code free of type C
>> changes outside
>> detection path.
>>
>> Please find below the code to do the same. Do not waste time trying to apply
>> this directly on
>> nightly since this is based on a local tree and because this is pre- atomic
>> changes code, so you
>> might have to modify chv_upfront_link_train to work on top of the latest
>> nightly code. we
>> are supposed to upstream this and is in my todo list.
>>
> [original patch snipped...]
>
> Hi Sivakumar,
>
> So I managed to manually re-apply your patch on top of
> drm-intel-nightly, and tried to port it to make Broadwell working too.
> It works OK if the system is already boot without any external DP used.
> In this case, the detection works and I can see my external monitor
> working properly.
>
> However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
> understand why. I think I enabled all that is mentioned in the PRM to be
> able to train the DP link, but I am obviously missing something else.
> Can you have a look?
>
> My patch is now:
</snipping some more>
>
> @@ -4495,7 +4499,9 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct intel_encoder *intel_encoder = &intel_dig_port->base;
> + struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> struct drm_device *dev = connector->dev;
> + struct intel_crtc *intel_crtc;
> enum drm_connector_status status;
> enum intel_display_power_domain power_domain;
> bool ret;
> @@ -4540,6 +4546,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +
> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> + (IS_BROADWELL(dev) || IS_CHERRYVIEW(dev))) {
> + /* Do not handle connected boot scenario where panel is enabled
> + * by GOP/VBIOS.
> + */
> + if ((connector->status != connector_status_connected) &&
> + !(intel_encoder->connectors_active &&
> + crtc && crtc->enabled))
> + intel_upfront_link_train(dev, intel_dp, NULL);
> + }
> +
here you are calling intel_upfront_link_train only if display is plugged
in and there is no crtc associated with it.
hence your code is working on hotplug. modify the above to consider
scenario when crtc is set and enabled
which happens when you plug in dp and boot the system. a good pointer is
the original code :)
+ if (intel_encoder->connectors_active &&
+ crtc && crtc->enabled) {
+ intel_crtc = to_intel_crtc(crtc);
+ DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
+ pipe_name(intel_crtc->pipe));
+ intel_crtc_control(crtc, false);
+ }
But again, all these are experimental :), any point you touch crtc it is
not in line with the new "atomic"
thinking and will not be allowed upstream till it is fixed as per the
new way :).
> status = connector_status_connected;
>
> /* Try to read the source of the interrupt */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6..fcc95d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,6 +1163,13 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> void intel_dp_start_link_train(struct intel_dp *intel_dp);
> void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +bool intel_upfront_link_train(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct intel_crtc *crtc);
> +void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw);
> +void intel_dp_set_clock(struct intel_encoder *encoder,
> + struct intel_crtc_state *pipe_config, int link_bw);
> +
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
--
regards,
Sivakumar
On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
<[email protected]> wrote:
> On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>
>>
>> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
>> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
>> >>identify both lane count and reversal state without touching anything in the
>> >>link training code. i am yet to upstream my changes for CHT that i can share
>> >>if required that does the same in intel_dp_detect without touching any line
>> >>in link training path.
>> >With my current limited knowledge of the dp hotplug (and i915 driver) I
>> >am not sure we could detect the reversed state without trying to train 1
>> >lane only. I'd be glad to look at your changes and test them on my
>> >system if you think that could help having a cleaner solution.
>> >
>> >Cheers,
>> >Benjamin
>> No, what i recommended was to do link training but in intel_dp_detect. Since
>> USB Type C cable
>> also has its own lane count restriction (it can have different lane count
>> than the one supported
>> by panel) you might have to figure that out as well. so both reversal and
>> lane count detection
>> can be done outside the modeset path and keep the code free of type C
>> changes outside
>> detection path.
>>
>> Please find below the code to do the same. Do not waste time trying to apply
>> this directly on
>> nightly since this is based on a local tree and because this is pre- atomic
>> changes code, so you
>> might have to modify chv_upfront_link_train to work on top of the latest
>> nightly code. we
>> are supposed to upstream this and is in my todo list.
>>
>
> [original patch snipped...]
>
> Hi Sivakumar,
>
> So I managed to manually re-apply your patch on top of
> drm-intel-nightly, and tried to port it to make Broadwell working too.
> It works OK if the system is already boot without any external DP used.
> In this case, the detection works and I can see my external monitor
> working properly.
>
> However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
> understand why. I think I enabled all that is mentioned in the PRM to be
> able to train the DP link, but I am obviously missing something else.
> Can you have a look?
>
Hi Benjamin,
I would recommend against this approach. Some adapters will claim that
they recovered a clock even when it isn't on the lanes you enabled,
which means that the reversal detection doesn't always work. The only
reliable way to do this is to go talk to the Chrome OS EC (you can
find these patches later in the Chrome OS tree). It's not as generic,
but we might be able to abstract that logic, maybe.
Stéphane
> My patch is now:
>
> From 11e9c42f55ae27bd6e7b0168bf24d15082c7d517 Mon Sep 17 00:00:00 2001
> From: Durgadoss R <[email protected]>
> Date: Mon, 3 Aug 2015 11:51:16 -0400
> Subject: [PATCH] drm/i915: Enable Upfront link training for type-C DP support
>
> To support USB type C alternate DP mode, the display driver needs to know the
> number of lanes required by 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 the link
> training fails, the driver then falls back to x2 lanes.
>
> * Since link training is done before modeset, planes are not enabled. Only
> encoder and the its associated PLLs are enabled.
> * Once link training is done, the encoder and its PLLs are disabled; so that
> the subsequent modeset is not aware of these changes.
> * As of now, this is tested only on CHV.
>
> Signed-off-by: Durgadoss R <[email protected]>
> [BT: add broadwell support and USB-C reverted state detection]
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_display.c | 163 +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_dp.c | 22 ++++-
> drivers/gpu/drm/i915/intel_drv.h | 7 ++
> 3 files changed, 190 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 818f3a3..e8ddba0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15799,3 +15799,166 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> spin_unlock_irq(&dev->event_lock);
> }
> }
> +
> +static bool
> +intel_upfront_link_train_config(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct intel_crtc *crtc,
> + uint8_t link_bw,
> + uint8_t lane_count)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_connector *connector = intel_dp->attached_connector;
> + struct intel_encoder *encoder = connector->encoder;
> + bool success;
> +
> + intel_dp->link_bw = link_bw;
> + intel_dp->lane_count = lane_count;
> +
> + /* Find port clock from link_bw */
> + crtc->config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> +
> + /* Enable PLL followed by port */
> + if (IS_BROADWELL(dev)) {
> + hsw_dp_set_ddi_pll_sel(crtc->config, intel_dp->link_bw);
> + if (intel_crtc_to_shared_dpll(crtc))
> + intel_enable_shared_dpll(crtc);
> +
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> +
> + } else if (IS_CHERRYVIEW(dev)) {
> + intel_dp_set_clock(encoder, crtc->config, intel_dp->link_bw);
> + if (encoder->pre_pll_enable)
> + encoder->pre_pll_enable(encoder);
> + if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> + chv_enable_pll(crtc, crtc->config);
> + }
> + if (encoder->pre_enable)
> + encoder->pre_enable(encoder);
> +
> + success = intel_dp->train_set_valid;
> +
> + /* Reset encoder for next retry or for clean up */
> + if (encoder->post_disable)
> + encoder->post_disable(encoder);
> + if (IS_CHERRYVIEW(dev)) {
> + if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> + chv_disable_pll(dev_priv, crtc->pipe);
> + } else if (IS_BROADWELL(dev)) {
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> + if (intel_crtc_to_shared_dpll(crtc))
> + intel_disable_shared_dpll(crtc);
> + }
> +
> + intel_dp->train_set_valid = false;
> +
> + return success;
> +}
> +
> +bool
> +intel_upfront_link_train(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct intel_crtc *crtc)
> +{
> + struct intel_connector *connector = intel_dp->attached_connector;
> + struct intel_encoder *encoder = connector->encoder;
> + struct intel_digital_port *intel_dig_port = enc_to_dig_port(&encoder->base);
> + uint8_t link_bw, lane_count;
> + bool found = false;
> + bool valid_crtc = !!crtc;
> +
> + if (!connector || !encoder) {
> + DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
> + return false;
> + }
> +
> + /* Find an unused crtc and use it for link training if no crtc is given */
> + if (!crtc) {
> + 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))
> + continue;
> +
> + encoder->base.crtc = &crtc->base;
> +
> + found = true;
> + break;
> + }
> +
> + if (!found) {
> + DRM_ERROR("Could not find crtc for upfront link training\n");
> + goto exit;
> + }
> + }
> +
> + DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
> + pipe_name(crtc->pipe));
> +
> + /* check the reversed state of the adapter first */
> +
> + /* unset the reversal bit */
> + intel_dig_port->saved_port_bits &= ~DDI_BUF_PORT_REVERSAL;
> +
> + /* Initialize with Max Link rate supported by panel & one lane */
> + intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
> + intel_dp->lane_count = 1;
> +
> + /* Check if link training passed; if not, update the reversed state */
> + if (!intel_upfront_link_train_config(dev, intel_dp, crtc,
> + intel_dp->dpcd[DP_MAX_LINK_RATE], 1))
> + intel_dig_port->saved_port_bits |= DDI_BUF_PORT_REVERSAL;
> +
> + found = false;
> +
> + /* Initialize with Max Link rate & lane count supported by panel */
> + link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
> + lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK;
> +
> + do {
> + /* Check if link training passed; if so update lane count */
> + if (intel_upfront_link_train_config(dev, intel_dp, crtc,
> + link_bw, lane_count)) {
> + 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;
> + 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 (lane_count == 4) {
> + lane_count = 2;
> + } else if (lane_count == 2) {
> + lane_count = 1;
> + } else if (link_bw == DP_LINK_BW_5_4) {
> + link_bw = DP_LINK_BW_2_7;
> + lane_count = 4;
> + } else if (link_bw == DP_LINK_BW_2_7) {
> + link_bw = DP_LINK_BW_1_62;
> + lane_count = 4;
> + } else {
> + /* Tried all combinations, so exit */
> + break;
> + }
> +
> + } while (1);
> +
> +exit:
> + /* Clear local associations made */
> + if (!valid_crtc)
> + encoder->base.crtc = NULL;
> +
> + if (found)
> + DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
> + intel_dp->lane_count, intel_dp->link_bw);
> +
> + return found;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 44f8a32..187800b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1144,7 +1144,7 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
> pipe_config->dpll_hw_state.ctrl1 = ctrl1;
> }
>
> -static void
> +void
> hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
> {
> memset(&pipe_config->dpll_hw_state, 0,
> @@ -1202,7 +1202,7 @@ intel_dp_source_rates(struct drm_device *dev, const int **source_rates)
> return (DP_LINK_BW_2_7 >> 3) + 1;
> }
>
> -static void
> +void
> intel_dp_set_clock(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config, int link_bw)
> {
> @@ -4423,6 +4423,10 @@ g4x_dp_detect(struct intel_dp *intel_dp)
> else if (ret == 0)
> return connector_status_disconnected;
>
> + /* Avoid DPCD opertations if status is same */
> + if (intel_dp->attached_connector->base.status == connector_status_connected)
> + return connector_status_connected;
> +
> return intel_dp_detect_dpcd(intel_dp);
> }
>
> @@ -4495,7 +4499,9 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct intel_encoder *intel_encoder = &intel_dig_port->base;
> + struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> struct drm_device *dev = connector->dev;
> + struct intel_crtc *intel_crtc;
> enum drm_connector_status status;
> enum intel_display_power_domain power_domain;
> bool ret;
> @@ -4540,6 +4546,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +
> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> + (IS_BROADWELL(dev) || IS_CHERRYVIEW(dev))) {
> + /* Do not handle connected boot scenario where panel is enabled
> + * by GOP/VBIOS.
> + */
> + if ((connector->status != connector_status_connected) &&
> + !(intel_encoder->connectors_active &&
> + crtc && crtc->enabled))
> + intel_upfront_link_train(dev, intel_dp, NULL);
> + }
> +
> status = connector_status_connected;
>
> /* Try to read the source of the interrupt */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6..fcc95d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,6 +1163,13 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> void intel_dp_start_link_train(struct intel_dp *intel_dp);
> void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +bool intel_upfront_link_train(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct intel_crtc *crtc);
> +void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw);
> +void intel_dp_set_clock(struct intel_encoder *encoder,
> + struct intel_crtc_state *pipe_config, int link_bw);
> +
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> --
> 2.4.3
>
>
> Cheers,
> Benjamin
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote:
> On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
> <[email protected]> wrote:
> > On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>
> >>
> >> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >> >>identify both lane count and reversal state without touching anything in the
> >> >>link training code. i am yet to upstream my changes for CHT that i can share
> >> >>if required that does the same in intel_dp_detect without touching any line
> >> >>in link training path.
> >> >With my current limited knowledge of the dp hotplug (and i915 driver) I
> >> >am not sure we could detect the reversed state without trying to train 1
> >> >lane only. I'd be glad to look at your changes and test them on my
> >> >system if you think that could help having a cleaner solution.
> >> >
> >> >Cheers,
> >> >Benjamin
> >> No, what i recommended was to do link training but in intel_dp_detect. Since
> >> USB Type C cable
> >> also has its own lane count restriction (it can have different lane count
> >> than the one supported
> >> by panel) you might have to figure that out as well. so both reversal and
> >> lane count detection
> >> can be done outside the modeset path and keep the code free of type C
> >> changes outside
> >> detection path.
> >>
> >> Please find below the code to do the same. Do not waste time trying to apply
> >> this directly on
> >> nightly since this is based on a local tree and because this is pre- atomic
> >> changes code, so you
> >> might have to modify chv_upfront_link_train to work on top of the latest
> >> nightly code. we
> >> are supposed to upstream this and is in my todo list.
> >>
> >
> > [original patch snipped...]
> >
> > Hi Sivakumar,
> >
> > So I managed to manually re-apply your patch on top of
> > drm-intel-nightly, and tried to port it to make Broadwell working too.
> > It works OK if the system is already boot without any external DP used.
> > In this case, the detection works and I can see my external monitor
> > working properly.
> >
> > However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
> > understand why. I think I enabled all that is mentioned in the PRM to be
> > able to train the DP link, but I am obviously missing something else.
> > Can you have a look?
> >
>
> Hi Benjamin,
>
> I would recommend against this approach. Some adapters will claim that
> they recovered a clock even when it isn't on the lanes you enabled,
> which means that the reversal detection doesn't always work. The only
> reliable way to do this is to go talk to the Chrome OS EC (you can
> find these patches later in the Chrome OS tree). It's not as generic,
> but we might be able to abstract that logic, maybe.
>
Hi Stéphane,
This is a very good news. I was afraid we would not have access to the
hardware controller because the Intel controller hub spec was not
public.
I will try to have a look at it, but the latest chromeos branch (3.18)
seems to differ quite a lot from the upstream one. Anyway, fingers
crossed.
Cheers,
Benjamin
On 8/18/2015 1:36 AM, Benjamin Tissoires wrote:
> On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote:
>> On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
>> <[email protected]> wrote:
>>> On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>>>
>>>> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
>>>>> On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>>>>> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
>>>>>> identify both lane count and reversal state without touching anything in the
>>>>>> link training code. i am yet to upstream my changes for CHT that i can share
>>>>>> if required that does the same in intel_dp_detect without touching any line
>>>>>> in link training path.
>>>>> With my current limited knowledge of the dp hotplug (and i915 driver) I
>>>>> am not sure we could detect the reversed state without trying to train 1
>>>>> lane only. I'd be glad to look at your changes and test them on my
>>>>> system if you think that could help having a cleaner solution.
>>>>>
>>>>> Cheers,
>>>>> Benjamin
>>>> No, what i recommended was to do link training but in intel_dp_detect. Since
>>>> USB Type C cable
>>>> also has its own lane count restriction (it can have different lane count
>>>> than the one supported
>>>> by panel) you might have to figure that out as well. so both reversal and
>>>> lane count detection
>>>> can be done outside the modeset path and keep the code free of type C
>>>> changes outside
>>>> detection path.
>>>>
>>>> Please find below the code to do the same. Do not waste time trying to apply
>>>> this directly on
>>>> nightly since this is based on a local tree and because this is pre- atomic
>>>> changes code, so you
>>>> might have to modify chv_upfront_link_train to work on top of the latest
>>>> nightly code. we
>>>> are supposed to upstream this and is in my todo list.
>>>>
>>> [original patch snipped...]
>>>
>>> Hi Sivakumar,
>>>
>>> So I managed to manually re-apply your patch on top of
>>> drm-intel-nightly, and tried to port it to make Broadwell working too.
>>> It works OK if the system is already boot without any external DP used.
>>> In this case, the detection works and I can see my external monitor
>>> working properly.
>>>
>>> However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
>>> understand why. I think I enabled all that is mentioned in the PRM to be
>>> able to train the DP link, but I am obviously missing something else.
>>> Can you have a look?
>>>
>> Hi Benjamin,
>>
>> I would recommend against this approach. Some adapters will claim that
>> they recovered a clock even when it isn't on the lanes you enabled,
>> which means that the reversal detection doesn't always work. The only
>> reliable way to do this is to go talk to the Chrome OS EC (you can
>> find these patches later in the Chrome OS tree). It's not as generic,
>> but we might be able to abstract that logic, maybe.
>>
> Hi Stéphane,
>
> This is a very good news. I was afraid we would not have access to the
> hardware controller because the Intel controller hub spec was not
> public.
>
> I will try to have a look at it, but the latest chromeos branch (3.18)
> seems to differ quite a lot from the upstream one. Anyway, fingers
> crossed.
>
> Cheers,
> Benjamin
Hi Benjamin/Stéphan,
All Intel platforms (at-least those i inquired about) handle lane
reversal in HW.
your statement that link training will pass even on reversed lane seems
to point
to the same fact. in such a scenario why should the encoder/connector care
if the lane is reversed or not ?
--
regards,
Sivakumar
On Aug 26 2015 or thereabouts, Sivakumar Thulasimani wrote:
>
>
> On 8/18/2015 1:36 AM, Benjamin Tissoires wrote:
> >On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote:
> >>On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
> >><[email protected]> wrote:
> >>>On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>>>
> >>>>On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >>>>>On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>>>>>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >>>>>>identify both lane count and reversal state without touching anything in the
> >>>>>>link training code. i am yet to upstream my changes for CHT that i can share
> >>>>>>if required that does the same in intel_dp_detect without touching any line
> >>>>>>in link training path.
> >>>>>With my current limited knowledge of the dp hotplug (and i915 driver) I
> >>>>>am not sure we could detect the reversed state without trying to train 1
> >>>>>lane only. I'd be glad to look at your changes and test them on my
> >>>>>system if you think that could help having a cleaner solution.
> >>>>>
> >>>>>Cheers,
> >>>>>Benjamin
> >>>>No, what i recommended was to do link training but in intel_dp_detect. Since
> >>>>USB Type C cable
> >>>>also has its own lane count restriction (it can have different lane count
> >>>>than the one supported
> >>>>by panel) you might have to figure that out as well. so both reversal and
> >>>>lane count detection
> >>>>can be done outside the modeset path and keep the code free of type C
> >>>>changes outside
> >>>>detection path.
> >>>>
> >>>>Please find below the code to do the same. Do not waste time trying to apply
> >>>>this directly on
> >>>>nightly since this is based on a local tree and because this is pre- atomic
> >>>>changes code, so you
> >>>>might have to modify chv_upfront_link_train to work on top of the latest
> >>>>nightly code. we
> >>>>are supposed to upstream this and is in my todo list.
> >>>>
> >>>[original patch snipped...]
> >>>
> >>>Hi Sivakumar,
> >>>
> >>>So I managed to manually re-apply your patch on top of
> >>>drm-intel-nightly, and tried to port it to make Broadwell working too.
> >>>It works OK if the system is already boot without any external DP used.
> >>>In this case, the detection works and I can see my external monitor
> >>>working properly.
> >>>
> >>>However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
> >>>understand why. I think I enabled all that is mentioned in the PRM to be
> >>>able to train the DP link, but I am obviously missing something else.
> >>>Can you have a look?
> >>>
> >>Hi Benjamin,
> >>
> >>I would recommend against this approach. Some adapters will claim that
> >>they recovered a clock even when it isn't on the lanes you enabled,
> >>which means that the reversal detection doesn't always work. The only
> >>reliable way to do this is to go talk to the Chrome OS EC (you can
> >>find these patches later in the Chrome OS tree). It's not as generic,
> >>but we might be able to abstract that logic, maybe.
> >>
> >Hi Stéphane,
> >
> >This is a very good news. I was afraid we would not have access to the
> >hardware controller because the Intel controller hub spec was not
> >public.
> >
> >I will try to have a look at it, but the latest chromeos branch (3.18)
> >seems to differ quite a lot from the upstream one. Anyway, fingers
> >crossed.
> >
> >Cheers,
> >Benjamin
> Hi Benjamin/Stéphan,
Hi Sivakumar,
> All Intel platforms (at-least those i inquired about) handle lane
> reversal in HW.
That is the theory and what is written in the USB Type C spec IIRC.
Problem is, the Chromebook Pixel 2 external display does not work when a
USB Type-C adapter is in the reversed position (or believe me, I would
definitively not have submitted a patch for the beauty of it).
Everything else works (link training when 4 lanes are activated, or
other communication channels). Only the order of the 4 data lanes
matters in this situation and the USB controller does not reverse them
for us on this laptop.
> your statement that link training will pass even on reversed lane seems to
> point
> to the same fact. in such a scenario why should the encoder/connector care
> if the lane is reversed or not ?
Problem is that Stephane said some adapters are lying regarding the
clock recovery. They claim everything is fine while in the end, the
display doesn't show anything because the lanes are reversed. If this is
just a chromebook Pixel 2 issue, that's better then. I won't have to try
to put some generic interface to notify that the display port lanes have
to be reversed.
Cheers,
Benjamin
>
> --
> regards,
> Sivakumar
>
On 8/26/2015 7:59 PM, Benjamin Tissoires wrote:
> On Aug 26 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>
>> On 8/18/2015 1:36 AM, Benjamin Tissoires wrote:
>>> On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote:
>>>> On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
>>>> <[email protected]> wrote:
>>>>> On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>>>>> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
>>>>>>> On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>>>>>>> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
>>>>>>>> identify both lane count and reversal state without touching anything in the
>>>>>>>> link training code. i am yet to upstream my changes for CHT that i can share
>>>>>>>> if required that does the same in intel_dp_detect without touching any line
>>>>>>>> in link training path.
>>>>>>> With my current limited knowledge of the dp hotplug (and i915 driver) I
>>>>>>> am not sure we could detect the reversed state without trying to train 1
>>>>>>> lane only. I'd be glad to look at your changes and test them on my
>>>>>>> system if you think that could help having a cleaner solution.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Benjamin
>>>>>> No, what i recommended was to do link training but in intel_dp_detect. Since
>>>>>> USB Type C cable
>>>>>> also has its own lane count restriction (it can have different lane count
>>>>>> than the one supported
>>>>>> by panel) you might have to figure that out as well. so both reversal and
>>>>>> lane count detection
>>>>>> can be done outside the modeset path and keep the code free of type C
>>>>>> changes outside
>>>>>> detection path.
>>>>>>
>>>>>> Please find below the code to do the same. Do not waste time trying to apply
>>>>>> this directly on
>>>>>> nightly since this is based on a local tree and because this is pre- atomic
>>>>>> changes code, so you
>>>>>> might have to modify chv_upfront_link_train to work on top of the latest
>>>>>> nightly code. we
>>>>>> are supposed to upstream this and is in my todo list.
>>>>>>
>>>>> [original patch snipped...]
>>>>>
>>>>> Hi Sivakumar,
>>>>>
>>>>> So I managed to manually re-apply your patch on top of
>>>>> drm-intel-nightly, and tried to port it to make Broadwell working too.
>>>>> It works OK if the system is already boot without any external DP used.
>>>>> In this case, the detection works and I can see my external monitor
>>>>> working properly.
>>>>>
>>>>> However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
>>>>> understand why. I think I enabled all that is mentioned in the PRM to be
>>>>> able to train the DP link, but I am obviously missing something else.
>>>>> Can you have a look?
>>>>>
>>>> Hi Benjamin,
>>>>
>>>> I would recommend against this approach. Some adapters will claim that
>>>> they recovered a clock even when it isn't on the lanes you enabled,
>>>> which means that the reversal detection doesn't always work. The only
>>>> reliable way to do this is to go talk to the Chrome OS EC (you can
>>>> find these patches later in the Chrome OS tree). It's not as generic,
>>>> but we might be able to abstract that logic, maybe.
>>>>
>>> Hi Stéphane,
>>>
>>> This is a very good news. I was afraid we would not have access to the
>>> hardware controller because the Intel controller hub spec was not
>>> public.
>>>
>>> I will try to have a look at it, but the latest chromeos branch (3.18)
>>> seems to differ quite a lot from the upstream one. Anyway, fingers
>>> crossed.
>>>
>>> Cheers,
>>> Benjamin
>> Hi Benjamin/Stéphan,
> Hi Sivakumar,
>
>> All Intel platforms (at-least those i inquired about) handle lane
>> reversal in HW.
> That is the theory and what is written in the USB Type C spec IIRC.
> Problem is, the Chromebook Pixel 2 external display does not work when a
> USB Type-C adapter is in the reversed position (or believe me, I would
> definitively not have submitted a patch for the beauty of it).
> Everything else works (link training when 4 lanes are activated, or
> other communication channels). Only the order of the 4 data lanes
> matters in this situation and the USB controller does not reverse them
> for us on this laptop.
>> your statement that link training will pass even on reversed lane seems to
>> point
>> to the same fact. in such a scenario why should the encoder/connector care
>> if the lane is reversed or not ?
> Problem is that Stephane said some adapters are lying regarding the
> clock recovery. They claim everything is fine while in the end, the
> display doesn't show anything because the lanes are reversed. If this is
> just a chromebook Pixel 2 issue, that's better then. I won't have to try
> to put some generic interface to notify that the display port lanes have
> to be reversed.
>
> Cheers,
> Benjamin
Thanks for the info :). There is no way in code to confirm if after link
training display
is working properly (other than short pulse for link related issues)
so this has to be on a case by case basis. i would recommend
if you plan to upstream it to restrict the changes for chromebook Pixel
2 alone.
>> --
>> regards,
>> Sivakumar
>>
--
regards,
Sivakumar