[PATCH 0/5] RFC: Displayport Link Policy Maker Update

[PATCH 0/5] RFC: Displayport Link Policy Maker Update

[Todd Previte]

This patch set lays the initial groundwork for updating the Displayport link policy maker
in the i915 driver. These first 5 patches add modular functions that have been split out
from the larger, monolithic ones present in the driver. The purpose of this work is the
following:
	1) Improve reliability and operation of the Displayport link policy maker
	2) Modularize and compartmentalize functionality to enhance readability and reliability
	3) Allow more fine-grain control over the operation of the link policy maker
	4) Enable features of Displayport such as Multistream Transport and compliance testing

Note that these new functions are not yet integrated into the operational code path(s). The 
original functions have been left in place, as is, to maintain operational status. Follow on
patch sets will integrate these functions into the policy maker.

Todd Previte (5):
  dmr/i915: Displayport - Add a function to set the training pattern
  drm/i915: Displayport - Add function to check link status
  drm/i915: Displayport - Add function to enable/disable scrambling on
    the main link
  drm/i915: Displayport - Add function for executing a single iteration
    of clock recovery
  drm/i915: Displayport - Add function to execute a single iteration of
    channel equalization

 drivers/gpu/drm/i915/i915_reg.h |   8 ++
 drivers/gpu/drm/i915/intel_dp.c | 217 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 225 insertions(+)

-- 
1.8.3.2

[PATCH 1/5] dmr/i915: Displayport - Add a function to set the training pattern

[Todd Previte]

Adds a function to set the training pattern for Displayport. This is
functionality required to establish more fine-grained control over
the Displayport interface, both for operational reliability and
compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  2 ++
 drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index adcb9c7..0f0d549 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5303,6 +5303,8 @@ enum punit_power_well {
 #define  DP_TP_CTL_LINK_TRAIN_NORMAL		(3<<8)
 #define  DP_TP_CTL_SCRAMBLE_DISABLE		(1<<7)
 
+#define DP_TRAINING_PATTERN_MASK_1P2	0x7
+
 /* DisplayPort Transport Status */
 #define DP_TP_STATUS_A			0x64044
 #define DP_TP_STATUS_B			0x64144
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6f767e5..64c9803 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2356,6 +2356,76 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	*DP = (*DP & ~mask) | signal_levels;
 }
 
+uint32_t 
+intel_dp_set_training_pattern(uint8_t training_pattern, 
+							  struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum port port = intel_dig_port->port;
+	uint8_t buf[sizeof(intel_dp->train_set) + 1];
+	int ret, len;
+
+	uint32_t reg_value, ctrl_reg, tp_select = 0;
+	uint32_t tp_mask = DP_TRAINING_PATTERN_MASK;
+
+	if (HAS_DDI(dev))
+		ctrl_reg = DP_TP_CTL(port);
+	else
+		ctrl_reg = intel_dp->output_reg;
+
+	reg_value = I915_READ(ctrl_reg);
+
+	// Check DPCD revision to enable TP3
+	if (intel_dp->dpcd[0] >= 12)
+		tp_mask = DP_TRAINING_PATTERN_MASK_1P2;
+
+	// Mask selection above ensures TP3 does not get enabled for < DP 1.2
+	switch (training_pattern & tp_mask) {
+		case DP_TRAINING_PATTERN_DISABLE:
+			tp_select = DP_TP_CTL_LINK_TRAIN_NORMAL;
+			break;
+		case DP_TRAINING_PATTERN_1:
+			tp_select = DP_TP_CTL_LINK_TRAIN_PAT1;
+			break;
+		case DP_TRAINING_PATTERN_2:
+			tp_select = DP_TP_CTL_LINK_TRAIN_PAT2;
+			break;
+		case DP_TRAINING_PATTERN_3:
+			tp_select = DP_TP_CTL_LINK_TRAIN_PAT3;
+			break;
+	}
+
+	if (HAS_DDI(dev) || (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A))) {
+		reg_value &= ~(tp_mask << 8);
+		reg_value |= tp_select;
+	}
+	else {
+		reg_value &= ~(tp_mask << 28);
+		reg_value |= tp_select << 20;
+	}
+
+	I915_WRITE(ctrl_reg, reg_value);
+	POSTING_READ(ctrl_reg);
+
+	buf[0] = training_pattern;
+	if ((training_pattern & tp_mask) ==
+		 DP_TRAINING_PATTERN_DISABLE) {
+		/* don't write DP_TRAINING_LANEx_SET on disable */
+		len = 1;
+	} else {
+		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
+		memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
+		len = intel_dp->lane_count + 1;
+	}
+
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
+				buf, len);
+
+	return ret == len;
+}
+
 static bool
 intel_dp_set_link_train(struct intel_dp *intel_dp,
 			uint32_t *DP,
-- 
1.8.3.2

[PATCH 2/5] drm/i915: Displayport - Add function to check link status

[Todd Previte]

Adds a function to check the link status across all lanes for Displayport.
This is functionality required to establish more fine-grained control over
the Displayport interface, both for operational reliability and
compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 61 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c9803..c865c32 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -38,6 +38,22 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+typedef enum {
+	LINK_TRAINING_STATUS_OK = 0,
+	LINK_TRAINING_CR_FAILED,
+	LINK_TRAINING_CR_COMPLETE,
+	LINK_TRAINING_CE_FAILED,
+	LINK_TRAINING_CE_COMPLETE,
+	LINK_TRAINING_STATUS_READ_FAILED
+} IntelDPLinkTrainingStatus;
+
+typedef enum {
+	DP_LINK_TRAINING_STATE_IDLE = 0,
+	DP_LINK_TRAINING_STATE_CLOCK_REC,
+	DP_LINK_TRAINING_STATE_CHANNEL_EQ,
+	DP_LINK_TRAINING_STATE_ADJUST
+} DPLinkTrainingState;
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;
@@ -2356,6 +2372,51 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	*DP = (*DP & ~mask) | signal_levels;
 }
 
+bool 
+intel_dp_verify_link_status(DPLinkTrainingState state, 
+							uint8_t lane_count, 
+							uint8_t *link_status)
+{
+	uint32_t status = 0;
+	bool link_ok = false;
+	uint8_t ls[5];
+
+	switch (state) {
+		case DP_LINK_TRAINING_STATE_CLOCK_REC:
+			status = (link_status[0] & DP_LANE_CR_DONE) +
+					((link_status[0] >> 4) & DP_LANE_CR_DONE) +
+					(link_status[1] & DP_LANE_CR_DONE) +
+					((link_status[1] >> 4) & DP_LANE_CR_DONE);
+			// Valid CR status = sum of bits == lane count
+			link_ok = status == lane_count ? true : false;
+			break;
+		case DP_LINK_TRAINING_STATE_CHANNEL_EQ:
+			ls[0] = ((link_status[0] & DP_LANE_CHANNEL_EQ_DONE) >> 1) +
+					((link_status[0] & DP_LANE_SYMBOL_LOCKED) >> 2);
+			ls[1] = (((link_status[0] >> 4) & DP_LANE_CHANNEL_EQ_DONE) >> 1) +
+					(((link_status[0] >> 4) & DP_LANE_SYMBOL_LOCKED) >> 2);
+			ls[2] = ((link_status[1] & DP_LANE_CHANNEL_EQ_DONE) >> 1) +
+					((link_status[1] & DP_LANE_SYMBOL_LOCKED) >> 2);
+			ls[3] = (((link_status[1] >> 4) & DP_LANE_CHANNEL_EQ_DONE) >> 1) +
+					(((link_status[1] >> 4) & DP_LANE_SYMBOL_LOCKED) >> 2);
+			ls[4] = link_status[2] & DP_INTERLANE_ALIGN_DONE;
+			// Combine lane status - must equal active lane count
+			status = ls[0] + ls[1] + ls[2] + ls[3];
+			// CE status is all lanes + lane alignment (ls[4])
+			link_ok = (status == lane_count) && ls[4] ? true : false;
+			break;
+		default:
+			// Invalid state for checking link status
+			break;
+	}
+
+	if (!link_ok) {
+		// FIXME: Log failure here, detailed error status output
+	}
+
+	return link_ok;
+}
+
 uint32_t 
 intel_dp_set_training_pattern(uint8_t training_pattern, 
 							  struct intel_dp *intel_dp)
-- 
1.8.3.2

[PATCH 3/5] drm/i915: Displayport - Add function to enable/disable scrambling on the main link

[Todd Previte]

Adds a function to enable and disable scrambling directly for the main link.
This is functionality required to establish more fine-grained control over
the Displayport interface, both for operational reliability and
compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c865c32..1209de8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2372,6 +2372,33 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	*DP = (*DP & ~mask) | signal_levels;
 }
 
+void intel_dp_scrambler_disable(bool disable, struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	// SNB is 0x40000. ILK is 0x4400
+	// IVB is 0x64000, HSW+ is 0x64040/0x64140
+	uint32_t ctrl_reg, reg_value;
+
+	if (HAS_DDI(dev))
+		ctrl_reg = DP_TP_CTL(intel_dig_port->port);
+	else
+		ctrl_reg = intel_dp->output_reg;
+
+	reg_value = I915_READ(ctrl_reg);
+
+	// Scrambling is bit 7 (scrambling on == 0)
+	if (disable)
+		reg_value |= DP_TP_CTL_SCRAMBLE_DISABLE;
+	else
+		reg_value &= ~DP_TP_CTL_SCRAMBLE_DISABLE;
+
+	I915_WRITE(ctrl_reg, reg_value);
+	POSTING_READ(ctrl_reg);
+}
+
 bool 
 intel_dp_verify_link_status(DPLinkTrainingState state, 
 							uint8_t lane_count, 
-- 
1.8.3.2

[PATCH 4/5] drm/i915: Displayport - Add function for executing a single iteration of clock recovery

[Todd Previte]

Adds a function to execute a single iteration of the clock recovery
sequence for Displayport. This is functionality required to establish
more fine-grained control over the Displayport interface, both for
operational reliability and compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_dp.c | 30 +++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f0d549..eebc4f2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5304,6 +5304,12 @@ enum punit_power_well {
 #define  DP_TP_CTL_SCRAMBLE_DISABLE		(1<<7)
 
 #define DP_TRAINING_PATTERN_MASK_1P2	0x7
+#define DP_CLOCK_RECOVERY_COMPLETE  		0x0
+#define DP_CLOCK_RECOVERY_FAILED			0x1
+#define DP_LINK_STATUS_READ_FAILED  		0x2
+#define DP_CHANNEL_EQUALIZATION_COMPLETE	0x0
+#define DP_CHANNEL_EQUALIZATION_FAILED  	0x1
+#define DP_SYMBOL_LOCK_FAILED				0x1
 
 /* DisplayPort Transport Status */
 #define DP_TP_STATUS_A			0x64044
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1209de8..6baa26c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2466,7 +2466,7 @@ intel_dp_set_training_pattern(uint8_t training_pattern,
 	reg_value = I915_READ(ctrl_reg);
 
 	// Check DPCD revision to enable TP3
-	if (intel_dp->dpcd[0] >= 12)
+	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x12)
 		tp_mask = DP_TRAINING_PATTERN_MASK_1P2;
 
 	// Mask selection above ensures TP3 does not get enabled for < DP 1.2
@@ -2850,6 +2850,34 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
+// State-based link training functions
+// FIXME: Remove these comments before commit!
+uint32_t intel_dp_exec_clock_recovery(struct intel_dp *intel_dp)
+{
+	uint32_t clock_recovery_status = DP_CLOCK_RECOVERY_FAILED;
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
+
+	// Set the training pattern for clock recovery
+	intel_dp_set_training_pattern(DP_TRAINING_PATTERN_1, intel_dp);
+
+	// Wait for clock recovery time period to expire
+	drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
+
+	// Check link status
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		clock_recovery_status = DP_LINK_STATUS_READ_FAILED;
+		goto exit;
+	}
+
+	// Verify clock recovery is successful
+	if (intel_dp_verify_link_status(DP_LINK_TRAINING_STATE_CLOCK_REC,
+									intel_dp->lane_count, link_status))
+		clock_recovery_status = DP_CLOCK_RECOVERY_COMPLETE;
+
+exit:
+	return clock_recovery_status;
+}
+
 static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
-- 
1.8.3.2

[PATCH 5/5] drm/i915: Displayport - Add function to execute a single iteration of channel equalization

[Todd Previte]

Adds a function to execute a single iteration of the channel equalization
sequence for Displayport. This is functionality required to establish more
fine-grained control over the Displayport interface, both for operational
reliability and compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6baa26c..66dbda6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2878,6 +2878,37 @@ exit:
 	return clock_recovery_status;
 }
 
+uint32_t intel_dp_exec_channel_equalization(struct intel_dp *intel_dp)
+{
+	uint32_t channel_equalization_status = DP_CHANNEL_EQUALIZATION_FAILED;
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
+
+	// Set the correct training pattern
+	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x12)
+		intel_dp_set_training_pattern(DP_TRAINING_PATTERN_2, intel_dp); 
+	else
+		intel_dp_set_training_pattern(DP_TRAINING_PATTERN_2, intel_dp); 
+
+	// Wait for channel equalization time period to expire
+	drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
+
+	// Check link status
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		channel_equalization_status = DP_LINK_STATUS_READ_FAILED;
+		goto exit;
+	}
+
+	// Verify that both CR is still valid and CE is successful
+	if (intel_dp_verify_link_status(DP_LINK_TRAINING_STATE_CLOCK_REC,
+									intel_dp->lane_count, link_status) &&
+		intel_dp_verify_link_status(DP_LINK_TRAINING_STATE_CHANNEL_EQ,
+									intel_dp->lane_count, link_status))
+			channel_equalization_status = DP_CHANNEL_EQUALIZATION_COMPLETE;
+
+exit:
+	return channel_equalization_status;
+}
+
 static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
-- 
1.8.3.2

Re: [PATCH 1/5] dmr/i915: Displayport - Add a function to set the training pattern

[Paulo Zanoni]

Hi

2014-04-08 14:47 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Adds a function to set the training pattern for Displayport. This is
> functionality required to establish more fine-grained control over
> the Displayport interface, both for operational reliability and
> compliance testing.

Just a suggestion: it's quite hard to review a patch that adds a new
function that is not used anywhere. Is the new function a complete
equivalent of some code we already have? Is it possible to just erase
some code on this file and replace it for a call to the new function?
If yes, then I suggest you do it on the same patch, since IMHO it
makes things much easier to review since we won't need to guess how
the function will be called later.

When extracting+changing functions from current code, we usually do
the following approach:

1 - Write a patch that creates a new function that is completely
equivalent to the current code, and replace the old code with a call
to the new function. This patch can't change how the code behaves.
2 - Write another patch doing whatever modification is necessary to
the new function. This is the patch that should introduce new
cases/features, and adjust any possible callers to do the correct
thing.

A little more about coding style below:

>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index adcb9c7..0f0d549 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5303,6 +5303,8 @@ enum punit_power_well {
>  #define  DP_TP_CTL_LINK_TRAIN_NORMAL           (3<<8)
>  #define  DP_TP_CTL_SCRAMBLE_DISABLE            (1<<7)
>
> +#define DP_TRAINING_PATTERN_MASK_1P2   0x7

Shouldn't this be defined at include/drm/drm_dp_helper.h? If yes, then
I guess it needs to be in a separate patch since it will touch a
generic drm header.

Also, is "1P2" supposed to mean "version one point two"? If yes, I
think I'd suggest adding a "v" somewhere to indicate it's a version :)

> +
>  /* DisplayPort Transport Status */
>  #define DP_TP_STATUS_A                 0x64044
>  #define DP_TP_STATUS_B                 0x64144
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6f767e5..64c9803 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2356,6 +2356,76 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>         *DP = (*DP & ~mask) | signal_levels;
>  }
>
> +uint32_t
> +intel_dp_set_training_pattern(uint8_t training_pattern,

If this function is going to be called only from intel_dp.c, then it
needs to be static. But since it has no callers, we will start getting
a WARN saying we don't have any callers. That's one of the reasons for
my suggestion above.

Also, you have trailing white spaces on the two lines above. I
configure my editor to show any
white-space-or-tab-at-the-end-of-the-line as a big red square.

> +                                                         struct intel_dp *intel_dp)

Lots of tabs here. Please set your editor to display tabs as 8 spaces.
I also configure my editor to paint the background of column 81 as
red, so I can easily see if I go past it.

> +{
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = intel_dig_port->base.base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       enum port port = intel_dig_port->port;
> +       uint8_t buf[sizeof(intel_dp->train_set) + 1];
> +       int ret, len;
> +
> +       uint32_t reg_value, ctrl_reg, tp_select = 0;
> +       uint32_t tp_mask = DP_TRAINING_PATTERN_MASK;
> +
> +       if (HAS_DDI(dev))
> +               ctrl_reg = DP_TP_CTL(port);
> +       else
> +               ctrl_reg = intel_dp->output_reg;
> +
> +       reg_value = I915_READ(ctrl_reg);
> +
> +       // Check DPCD revision to enable TP3

Please use C-style /* comments */ here and below. See
Documentation/CodingStyle Chapter 8.


> +       if (intel_dp->dpcd[0] >= 12)

On patch 4 you replace this zero with "DP_DPCD_REV". I think you
should use the macro right now.


> +               tp_mask = DP_TRAINING_PATTERN_MASK_1P2;
> +
> +       // Mask selection above ensures TP3 does not get enabled for < DP 1.2
> +       switch (training_pattern & tp_mask) {
> +               case DP_TRAINING_PATTERN_DISABLE:
> +                       tp_select = DP_TP_CTL_LINK_TRAIN_NORMAL;
> +                       break;
> +               case DP_TRAINING_PATTERN_1:
> +                       tp_select = DP_TP_CTL_LINK_TRAIN_PAT1;
> +                       break;
> +               case DP_TRAINING_PATTERN_2:
> +                       tp_select = DP_TP_CTL_LINK_TRAIN_PAT2;
> +                       break;
> +               case DP_TRAINING_PATTERN_3:
> +                       tp_select = DP_TP_CTL_LINK_TRAIN_PAT3;
> +                       break;
> +       }

Per Documentation/CodingStyle Chapter 1, please put the "case" in the
same column as the "switch".


> +
> +       if (HAS_DDI(dev) || (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A))) {
> +               reg_value &= ~(tp_mask << 8);
> +               reg_value |= tp_select;
> +       }
> +       else {
> +               reg_value &= ~(tp_mask << 28);
> +               reg_value |= tp_select << 20;
> +       }
> +
> +       I915_WRITE(ctrl_reg, reg_value);
> +       POSTING_READ(ctrl_reg);
> +
> +       buf[0] = training_pattern;
> +       if ((training_pattern & tp_mask) ==
> +                DP_TRAINING_PATTERN_DISABLE) {
> +               /* don't write DP_TRAINING_LANEx_SET on disable */
> +               len = 1;
> +       } else {
> +               /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> +               memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
> +               len = intel_dp->lane_count + 1;
> +       }
> +
> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> +                               buf, len);
> +
> +       return ret == len;

I can't see the callers, but do we ever expect to have "ret != len"?
If this is something that should never happen and we have to real way
to recover from, I would suggest just using WARN().

> +}
> +
>  static bool
>  intel_dp_set_link_train(struct intel_dp *intel_dp,
>                         uint32_t *DP,
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 0/5] RFC: Displayport Link Policy Maker Update

[Daniel Vetter]

On Tue, Apr 08, 2014 at 10:47:38AM -0700, Todd Previte wrote:
> This patch set lays the initial groundwork for updating the Displayport link policy maker
> in the i915 driver. These first 5 patches add modular functions that have been split out
> from the larger, monolithic ones present in the driver. The purpose of this work is the
> following:
> 	1) Improve reliability and operation of the Displayport link policy maker
> 	2) Modularize and compartmentalize functionality to enhance readability and reliability
> 	3) Allow more fine-grain control over the operation of the link policy maker
> 	4) Enable features of Displayport such as Multistream Transport and compliance testing
> 
> Note that these new functions are not yet integrated into the operational code path(s). The 
> original functions have been left in place, as is, to maintain operational status. Follow on
> patch sets will integrate these functions into the policy maker.

So ignoring the details and just commenting on the overall approach:

When we're currently at A and want to go to B then we do that with a
series of small patche refactoring the existing code step-by-step in fully
bisectable patches. Writing completely new code and then throwing the
switch to move from A to B is the exceptional case and needs exceptional
justification. Examples:

- Switching to the common dp aux helper. But even in that case Jani first
  adjusted the existing code to make it match better with the semantics of
  the new code so that the transition was as smooth as possible. Still
  blew up.

- Completely new hardware/platforms, e.g. the DDI code on hsw+. But even
  there the very first enabling code started out from intel_dp.c and then
  only after an awful lot of refactoring.

As-is I don't see a justification why the DP code qualifies. Also note
that we now have the shared dp helper library for handling all kinds of dp
sink behaviour. Dave Airlie is already working like crazy to get dp mst
(at least the discovery part) off the ground. See:

http://cgit.freedesktop.org/~airlied/linux/log/?h=i915-mst-hacks

Imo any sink related big changes should be done in the dp helper code and
not on our side. Of course all the source handling needs to still be in
our driver (i.e. in intel_dp.c and intel_ddi.c).

So overall I'm not sold on why exactly we need these patches here at all.
-Daniel

> 
> Todd Previte (5):
>   dmr/i915: Displayport - Add a function to set the training pattern
>   drm/i915: Displayport - Add function to check link status
>   drm/i915: Displayport - Add function to enable/disable scrambling on
>     the main link
>   drm/i915: Displayport - Add function for executing a single iteration
>     of clock recovery
>   drm/i915: Displayport - Add function to execute a single iteration of
>     channel equalization
> 
>  drivers/gpu/drm/i915/i915_reg.h |   8 ++
>  drivers/gpu/drm/i915/intel_dp.c | 217 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 225 insertions(+)
> 
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch