[PATCH 1/3] drm/i915: consider the source max DP lane count too

[PATCH 1/3] drm/i915: consider the source max DP lane count too

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Even if the panel claims it can support 4 lanes, there's the
possibility that the HW can't, so consider this while selecting the
max lane count.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 104998e..19537a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -120,6 +120,22 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp)
 	return max_link_bw;
 }
 
+static u8 intel_dp_max_lane_count(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;
+	u8 source_max, sink_max;
+
+	source_max = 4;
+	if (HAS_DDI(dev) && intel_dig_port->port == PORT_A &&
+	    (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == 0)
+		source_max = 2;
+
+	sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
+
+	return min(source_max, sink_max);
+}
+
 /*
  * The units on the numbers in the next two are... bizarre.  Examples will
  * make it clearer; this one parallels an example in the eDP spec.
@@ -170,7 +186,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	}
 
 	max_link_clock = drm_dp_bw_code_to_link_rate(intel_dp_max_link_bw(intel_dp));
-	max_lanes = drm_dp_max_lane_count(intel_dp->dpcd);
+	max_lanes = intel_dp_max_lane_count(intel_dp);
 
 	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
 	mode_rate = intel_dp_link_required(target_clock, 18);
@@ -764,7 +780,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct intel_crtc *intel_crtc = encoder->new_crtc;
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int lane_count, clock;
-	int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+	int max_lane_count = intel_dp_max_lane_count(intel_dp);
 	/* Conveniently, the link BW constants become indices with a shift...*/
 	int max_clock = intel_dp_max_link_bw(intel_dp) >> 3;
 	int bpp, mode_rate;
-- 
1.9.0

[PATCH 2/3] drm/i915: extract intel_dp_compute_link_config

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Extract this function to make the input and output parameters more
clear to the code reader.

I also have plans to allow ways to change the current parameters, so
the code will get even more complicated without the refactor. I also
have plans to add alternative implementations to
intel_dp_compute_link_config() and, again, the refactor makes it
easier.

Another change worth noticing is that now we will print all our
"attempts" in dmesg (see the debug code at link_config_is_possible()).
This will allow us to quickly identify if the user is trying fast and
narrow, or wide and slow, and also identify the minimum and maximum BW
and lane count values used by the code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 94 +++++++++++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 19537a6..884166b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -768,6 +768,52 @@ intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
 	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
 }
 
+static bool link_config_is_possible(int mode_rate, int lanes, int bw)
+{
+	int link_clock, link_avail;
+
+	link_clock = drm_dp_bw_code_to_link_rate(bw);
+	link_avail = intel_dp_max_data_rate(link_clock, lanes);
+
+	DRM_DEBUG_KMS("DP link avail with 0x%x bw and %d lanes: %d. Required: %d (%d%%)\n",
+		      bw, lanes, link_avail, mode_rate,
+		      mode_rate * 100 / link_avail);
+
+	return (mode_rate <= link_avail);
+}
+
+/*
+ * This function tries to find the minimal number of lanes and BW required based
+ * on the parameters given. If not possible, it returns false.
+ */
+static bool intel_dp_compute_link_config(int mode_clock, int bpp,
+					 int min_lanes, int max_lanes,
+					 int min_bw, int max_bw,
+					 int *lanes_used, int *bw_used)
+{
+	/* Conveniently, the link BW constants become indices with a shift...*/
+	static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 };
+	int min_bw_index = min_bw >> 3;
+	int max_bw_index = max_bw >> 3;
+	int mode_rate;
+	int lanes, bw_it;
+
+	mode_rate = intel_dp_link_required(mode_clock, bpp);
+
+	for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
+		for (bw_it = min_bw_index; bw_it <= max_bw_index; bw_it++)
+			if (link_config_is_possible(mode_rate, lanes,
+						    bws[bw_it]))
+				goto found;
+
+	return false;
+
+found:
+	*lanes_used = lanes;
+	*bw_used = bws[bw_it];
+	return true;
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -779,13 +825,10 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	enum port port = dp_to_dig_port(intel_dp)->port;
 	struct intel_crtc *intel_crtc = encoder->new_crtc;
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	int lane_count, clock;
+	int lanes_used, bw_used;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
-	/* Conveniently, the link BW constants become indices with a shift...*/
-	int max_clock = intel_dp_max_link_bw(intel_dp) >> 3;
-	int bpp, mode_rate;
-	static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 };
-	int link_avail, link_clock;
+	int max_bw = intel_dp_max_link_bw(intel_dp);
+	int bpp;
 
 	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
 		pipe_config->has_pch_encoder = true;
@@ -808,7 +851,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %02x pixel clock %iKHz\n",
-		      max_lane_count, bws[max_clock],
+		      max_lane_count, max_bw,
 		      adjusted_mode->crtc_clock);
 
 	/* Walk through all bpp values. Luckily they're all nicely spaced with 2
@@ -821,26 +864,17 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		bpp = dev_priv->vbt.edp_bpp;
 	}
 
-	for (; bpp >= 6*3; bpp -= 2*3) {
-		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
-						   bpp);
-
-		for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
-			for (clock = 0; clock <= max_clock; clock++) {
-				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;
-				}
-			}
-		}
-	}
+	for (; bpp >= 6*3; bpp -= 2*3)
+		if (intel_dp_compute_link_config(adjusted_mode->crtc_clock, bpp,
+						 1, max_lane_count,
+						 DP_LINK_BW_1_62,
+						 max_bw,
+						 &lanes_used, &bw_used))
+			break;
 
-	return false;
+	if (bpp < 6*3)
+		return false;
 
-found:
 	if (intel_dp->color_range_auto) {
 		/*
 		 * See:
@@ -856,25 +890,23 @@ found:
 	if (intel_dp->color_range)
 		pipe_config->limited_color_range = true;
 
-	intel_dp->link_bw = bws[clock];
-	intel_dp->lane_count = lane_count;
+	intel_dp->link_bw = bw_used;
+	intel_dp->lane_count = lanes_used;
 	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
 
 	DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
 		      intel_dp->link_bw, intel_dp->lane_count,
 		      pipe_config->port_clock, bpp);
-	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
-		      mode_rate, link_avail);
 
-	intel_link_compute_m_n(bpp, lane_count,
+	intel_link_compute_m_n(bpp, lanes_used,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
 	if (intel_connector->panel.downclock_mode != NULL &&
 		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
-			intel_link_compute_m_n(bpp, lane_count,
+			intel_link_compute_m_n(bpp, lanes_used,
 				intel_connector->panel.downclock_mode->clock,
 				pipe_config->port_clock,
 				&pipe_config->dp_m2_n2);
-- 
1.9.0

[PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We still have way too many bugs with DP link training. We keep
switching between "narrow and fast" and "wide and slow", we recently
added 5GHz support, and whenever there's a bug report, we have to ask
people to apply patches and test them.

Wouldn't it be so much better if we could just ask them to boot with
some specific Kernel boot option instead of applying a patch? This
will move the situation from "i915.ko is completely broken!" to
"i915.ko's default values are broken, but there's an option I can set
to fix it, so I won't need to learn how to compile a Kernel!".

Some useful values:
 - i915.dp_link_train_policy=1 for "wide and slow"
 - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
   which should be able to fit 1920x1080@60Hz and 24bpp
 - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
   not-so-huge modes

The default behavior is still the same.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
 drivers/gpu/drm/i915/intel_dp.c    | 39 ++++++++++++++++++++++++++++++++++----
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d6acb4..d5ae8dc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1941,6 +1941,7 @@ struct i915_params {
 	bool reset;
 	bool disable_display;
 	bool disable_vtd_wa;
+	int dp_link_train_policy;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index d05a2af..8c358e7 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -48,6 +48,7 @@ struct i915_params i915 __read_mostly = {
 	.disable_display = 0,
 	.enable_cmd_parser = 1,
 	.disable_vtd_wa = 0,
+	.dp_link_train_policy = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -156,3 +157,10 @@ MODULE_PARM_DESC(disable_vtd_wa, "Disable all VT-d workarounds (default: false)"
 module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600);
 MODULE_PARM_DESC(enable_cmd_parser,
 		 "Enable command parsing (1=enabled [default], 0=disabled)");
+
+module_param_named(dp_link_train_policy, i915.dp_link_train_policy, int, 0600);
+MODULE_PARM_DESC(dp_link_train_policy,
+	"Choose strategy for DP link training "
+	"(0=narrow and fast [default], 1=wide and slow. For other values, "
+	"bits 11:8 are for the BW and bits 7:4 are for the nubmer of lanes, "
+	"check intel_dp_compute_link_config() for more details.)");
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 884166b..25f7e1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -800,11 +800,42 @@ static bool intel_dp_compute_link_config(int mode_clock, int bpp,
 
 	mode_rate = intel_dp_link_required(mode_clock, bpp);
 
-	for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
+	switch (i915.dp_link_train_policy) {
+	case 0: /* Narrow and fast. */
+		for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
+			for (bw_it = min_bw_index; bw_it <= max_bw_index;
+			     bw_it++)
+				if (link_config_is_possible(mode_rate, lanes,
+							    bws[bw_it]))
+					goto found;
+	case 1: /* Wide and slow. */
 		for (bw_it = min_bw_index; bw_it <= max_bw_index; bw_it++)
-			if (link_config_is_possible(mode_rate, lanes,
-						    bws[bw_it]))
-				goto found;
+			for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
+				if (link_config_is_possible(mode_rate, lanes,
+							    bws[bw_it]))
+					goto found;
+	default: /* Bits 11:8 contain the index of the bws array.
+		  * Bits 7:4 contain the number of lanes.
+		  * Example: i915.dp_link_train_policy=0x140 uses
+		  * DP_LINK_BW_2_7 and 4 lanes. */
+		bw_it = (i915.dp_link_train_policy >> 8) & 0xF;
+		lanes = (i915.dp_link_train_policy >> 4) & 0xF;
+
+		if (bw_it < min_bw_index || bw_it > max_bw_index) {
+			DRM_ERROR("Invalid BW index\n");
+			return false;
+		}
+
+		if (lanes < min_lanes || lanes > max_lanes ||
+		    /* Not a power of two. */
+		    !(lanes != 0 && (lanes & (lanes - 1)) == 0)) {
+			DRM_ERROR("Invalid number of lanes.\n");
+			return false;
+		}
+
+		if (link_config_is_possible(mode_rate, lanes, bws[bw_it]))
+			goto found;
+	}
 
 	return false;
 
-- 
1.9.0

Re: [PATCH 1/3] drm/i915: consider the source max DP lane count too

[Jani Nikula]

On Fri, 25 Apr 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Even if the panel claims it can support 4 lanes, there's the
> possibility that the HW can't, so consider this while selecting the
> max lane count.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 104998e..19537a6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -120,6 +120,22 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp)
>  	return max_link_bw;
>  }
>  
> +static u8 intel_dp_max_lane_count(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;
> +	u8 source_max, sink_max;
> +
> +	source_max = 4;
> +	if (HAS_DDI(dev) && intel_dig_port->port == PORT_A &&
> +	    (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == 0)
> +		source_max = 2;

Wow, good catch. Are you aware of similar restrictions on other
platforms?

Does this potentially fix
https://bugs.freedesktop.org/show_bug.cgi?id=73539 ?

BR,
Jani.

> +
> +	sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
> +
> +	return min(source_max, sink_max);
> +}
> +
>  /*
>   * The units on the numbers in the next two are... bizarre.  Examples will
>   * make it clearer; this one parallels an example in the eDP spec.
> @@ -170,7 +186,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  	}
>  
>  	max_link_clock = drm_dp_bw_code_to_link_rate(intel_dp_max_link_bw(intel_dp));
> -	max_lanes = drm_dp_max_lane_count(intel_dp->dpcd);
> +	max_lanes = intel_dp_max_lane_count(intel_dp);
>  
>  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
>  	mode_rate = intel_dp_link_required(target_clock, 18);
> @@ -764,7 +780,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	struct intel_crtc *intel_crtc = encoder->new_crtc;
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	int lane_count, clock;
> -	int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> +	int max_lane_count = intel_dp_max_lane_count(intel_dp);
>  	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int max_clock = intel_dp_max_link_bw(intel_dp) >> 3;
>  	int bpp, mode_rate;
> -- 
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Daniel Vetter]

On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We still have way too many bugs with DP link training. We keep
> switching between "narrow and fast" and "wide and slow", we recently
> added 5GHz support, and whenever there's a bug report, we have to ask
> people to apply patches and test them.
> 
> Wouldn't it be so much better if we could just ask them to boot with
> some specific Kernel boot option instead of applying a patch? This
> will move the situation from "i915.ko is completely broken!" to
> "i915.ko's default values are broken, but there's an option I can set
> to fix it, so I won't need to learn how to compile a Kernel!".
> 
> Some useful values:
>  - i915.dp_link_train_policy=1 for "wide and slow"
>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
>    which should be able to fit 1920x1080@60Hz and 24bpp
>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
>    not-so-huge modes
> 
> The default behavior is still the same.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah, I like this. I'll sign up Todd to review this all.

> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c    | 39 ++++++++++++++++++++++++++++++++++----
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7d6acb4..d5ae8dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1941,6 +1941,7 @@ struct i915_params {
>  	bool reset;
>  	bool disable_display;
>  	bool disable_vtd_wa;
> +	int dp_link_train_policy;

Small nit: bitfield-y stuff should be unsinged.

>  };
>  extern struct i915_params i915 __read_mostly;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index d05a2af..8c358e7 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -48,6 +48,7 @@ struct i915_params i915 __read_mostly = {
>  	.disable_display = 0,
>  	.enable_cmd_parser = 1,
>  	.disable_vtd_wa = 0,
> +	.dp_link_train_policy = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -156,3 +157,10 @@ MODULE_PARM_DESC(disable_vtd_wa, "Disable all VT-d workarounds (default: false)"
>  module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600);
>  MODULE_PARM_DESC(enable_cmd_parser,
>  		 "Enable command parsing (1=enabled [default], 0=disabled)");
> +
> +module_param_named(dp_link_train_policy, i915.dp_link_train_policy, int, 0600);
> +MODULE_PARM_DESC(dp_link_train_policy,
> +	"Choose strategy for DP link training "
> +	"(0=narrow and fast [default], 1=wide and slow. For other values, "
> +	"bits 11:8 are for the BW and bits 7:4 are for the nubmer of lanes, "
> +	"check intel_dp_compute_link_config() for more details.)");
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 884166b..25f7e1c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -800,11 +800,42 @@ static bool intel_dp_compute_link_config(int mode_clock, int bpp,
>  
>  	mode_rate = intel_dp_link_required(mode_clock, bpp);
>  
> -	for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
> +	switch (i915.dp_link_train_policy) {
> +	case 0: /* Narrow and fast. */
> +		for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
> +			for (bw_it = min_bw_index; bw_it <= max_bw_index;
> +			     bw_it++)
> +				if (link_config_is_possible(mode_rate, lanes,
> +							    bws[bw_it]))
> +					goto found;
> +	case 1: /* Wide and slow. */
>  		for (bw_it = min_bw_index; bw_it <= max_bw_index; bw_it++)
> -			if (link_config_is_possible(mode_rate, lanes,
> -						    bws[bw_it]))
> -				goto found;
> +			for (lanes = min_lanes; lanes <= max_lanes; lanes <<= 1)
> +				if (link_config_is_possible(mode_rate, lanes,
> +							    bws[bw_it]))
> +					goto found;
> +	default: /* Bits 11:8 contain the index of the bws array.
> +		  * Bits 7:4 contain the number of lanes.
> +		  * Example: i915.dp_link_train_policy=0x140 uses
> +		  * DP_LINK_BW_2_7 and 4 lanes. */
> +		bw_it = (i915.dp_link_train_policy >> 8) & 0xF;
> +		lanes = (i915.dp_link_train_policy >> 4) & 0xF;
> +
> +		if (bw_it < min_bw_index || bw_it > max_bw_index) {
> +			DRM_ERROR("Invalid BW index\n");
> +			return false;
> +		}
> +
> +		if (lanes < min_lanes || lanes > max_lanes ||
> +		    /* Not a power of two. */
> +		    !(lanes != 0 && (lanes & (lanes - 1)) == 0)) {
> +			DRM_ERROR("Invalid number of lanes.\n");
> +			return false;
> +		}
> +
> +		if (link_config_is_possible(mode_rate, lanes, bws[bw_it]))
> +			goto found;
> +	}
>  
>  	return false;
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> 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

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Daniel Vetter]

On Fri, Apr 25, 2014 at 10:06 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We still have way too many bugs with DP link training. We keep
>> switching between "narrow and fast" and "wide and slow", we recently
>> added 5GHz support, and whenever there's a bug report, we have to ask
>> people to apply patches and test them.
>>
>> Wouldn't it be so much better if we could just ask them to boot with
>> some specific Kernel boot option instead of applying a patch? This
>> will move the situation from "i915.ko is completely broken!" to
>> "i915.ko's default values are broken, but there's an option I can set
>> to fix it, so I won't need to learn how to compile a Kernel!".
>>
>> Some useful values:
>>  - i915.dp_link_train_policy=1 for "wide and slow"
>>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
>>    which should be able to fit 1920x1080@60Hz and 24bpp
>>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
>>    not-so-huge modes
>>
>> The default behavior is still the same.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Yeah, I like this. I'll sign up Todd to review this all.

Now with Todd actually cc'ed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Jani Nikula]

On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> 
>> We still have way too many bugs with DP link training. We keep
>> switching between "narrow and fast" and "wide and slow", we recently
>> added 5GHz support, and whenever there's a bug report, we have to ask
>> people to apply patches and test them.
>> 
>> Wouldn't it be so much better if we could just ask them to boot with
>> some specific Kernel boot option instead of applying a patch? This
>> will move the situation from "i915.ko is completely broken!" to
>> "i915.ko's default values are broken, but there's an option I can set
>> to fix it, so I won't need to learn how to compile a Kernel!".
>> 
>> Some useful values:
>>  - i915.dp_link_train_policy=1 for "wide and slow"
>>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
>>    which should be able to fit 1920x1080@60Hz and 24bpp
>>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
>>    not-so-huge modes
>> 
>> The default behavior is still the same.
>> 
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Yeah, I like this. I'll sign up Todd to review this all.

I guess we'll go with this then, but I'll step back from this particular
patch for a bit, and share my concerns over module parameters and
quirks.

I am generally opposed to adding module parameters or quirks to
workaround issues in features that should just work. They are an easy
way out for things we should root cause and fix properly.

Do not mistake me for an idealist for thinking this way, as I'm being
pragmatic.

The people who report bugs to us are roughly the same people who are
capable of setting the module parameter. Once they figure that out,
they'll stop responding to our requests for testing and info. We've seen
this happen before. We'd hurt our chances of making things work out of
the box for the average user.

The more we add module parameters, the combinations of them
explode. Debugging *other* problems becomes harder. In the bugs I work
on, the #1 request I have is full dmesg, partially because I want to see
all the wild kernel parameters the user might have set. And all too
often they have. When there are module parameters that fix some bugs,
the blogs and forums get filled with tips about them, and people use
them, whether they strictly have the same bug or not. Search for i915
invert brightness for example.

It's also not easy to drop module parameters after we've added them. You
know the drill. Even after we've fixed everything the module parameter
was supposed to fix, dropping it leads to https://xkcd.com/1172/.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Daniel Vetter]

On Fri, Apr 25, 2014 at 12:00:34PM +0300, Jani Nikula wrote:
> On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> 
> >> We still have way too many bugs with DP link training. We keep
> >> switching between "narrow and fast" and "wide and slow", we recently
> >> added 5GHz support, and whenever there's a bug report, we have to ask
> >> people to apply patches and test them.
> >> 
> >> Wouldn't it be so much better if we could just ask them to boot with
> >> some specific Kernel boot option instead of applying a patch? This
> >> will move the situation from "i915.ko is completely broken!" to
> >> "i915.ko's default values are broken, but there's an option I can set
> >> to fix it, so I won't need to learn how to compile a Kernel!".
> >> 
> >> Some useful values:
> >>  - i915.dp_link_train_policy=1 for "wide and slow"
> >>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
> >>    which should be able to fit 1920x1080@60Hz and 24bpp
> >>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
> >>    not-so-huge modes
> >> 
> >> The default behavior is still the same.
> >> 
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Yeah, I like this. I'll sign up Todd to review this all.
> 
> I guess we'll go with this then, but I'll step back from this particular
> patch for a bit, and share my concerns over module parameters and
> quirks.
> 
> I am generally opposed to adding module parameters or quirks to
> workaround issues in features that should just work. They are an easy
> way out for things we should root cause and fix properly.
> 
> Do not mistake me for an idealist for thinking this way, as I'm being
> pragmatic.
> 
> The people who report bugs to us are roughly the same people who are
> capable of setting the module parameter. Once they figure that out,
> they'll stop responding to our requests for testing and info. We've seen
> this happen before. We'd hurt our chances of making things work out of
> the box for the average user.
> 
> The more we add module parameters, the combinations of them
> explode. Debugging *other* problems becomes harder. In the bugs I work
> on, the #1 request I have is full dmesg, partially because I want to see
> all the wild kernel parameters the user might have set. And all too
> often they have. When there are module parameters that fix some bugs,
> the blogs and forums get filled with tips about them, and people use
> them, whether they strictly have the same bug or not. Search for i915
> invert brightness for example.
> 
> It's also not easy to drop module parameters after we've added them. You
> know the drill. Even after we've fixed everything the module parameter
> was supposed to fix, dropping it leads to https://xkcd.com/1172/.

I fully agree with you. I'm working on a patch (only RFC thus far) which
allows you to designate some module parameters as debug knobs. As soon as
users touch them they'll get
- a stern warning in dmesg
- TAINT_USER'ed kernel

That should be about as good as we can make it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Jani Nikula]

On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 25, 2014 at 12:00:34PM +0300, Jani Nikula wrote:
>> On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> 
>> >> We still have way too many bugs with DP link training. We keep
>> >> switching between "narrow and fast" and "wide and slow", we recently
>> >> added 5GHz support, and whenever there's a bug report, we have to ask
>> >> people to apply patches and test them.
>> >> 
>> >> Wouldn't it be so much better if we could just ask them to boot with
>> >> some specific Kernel boot option instead of applying a patch? This
>> >> will move the situation from "i915.ko is completely broken!" to
>> >> "i915.ko's default values are broken, but there's an option I can set
>> >> to fix it, so I won't need to learn how to compile a Kernel!".
>> >> 
>> >> Some useful values:
>> >>  - i915.dp_link_train_policy=1 for "wide and slow"
>> >>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
>> >>    which should be able to fit 1920x1080@60Hz and 24bpp
>> >>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
>> >>    not-so-huge modes
>> >> 
>> >> The default behavior is still the same.
>> >> 
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Yeah, I like this. I'll sign up Todd to review this all.
>> 
>> I guess we'll go with this then, but I'll step back from this particular
>> patch for a bit, and share my concerns over module parameters and
>> quirks.
>> 
>> I am generally opposed to adding module parameters or quirks to
>> workaround issues in features that should just work. They are an easy
>> way out for things we should root cause and fix properly.
>> 
>> Do not mistake me for an idealist for thinking this way, as I'm being
>> pragmatic.
>> 
>> The people who report bugs to us are roughly the same people who are
>> capable of setting the module parameter. Once they figure that out,
>> they'll stop responding to our requests for testing and info. We've seen
>> this happen before. We'd hurt our chances of making things work out of
>> the box for the average user.
>> 
>> The more we add module parameters, the combinations of them
>> explode. Debugging *other* problems becomes harder. In the bugs I work
>> on, the #1 request I have is full dmesg, partially because I want to see
>> all the wild kernel parameters the user might have set. And all too
>> often they have. When there are module parameters that fix some bugs,
>> the blogs and forums get filled with tips about them, and people use
>> them, whether they strictly have the same bug or not. Search for i915
>> invert brightness for example.
>> 
>> It's also not easy to drop module parameters after we've added them. You
>> know the drill. Even after we've fixed everything the module parameter
>> was supposed to fix, dropping it leads to https://xkcd.com/1172/.
>
> I fully agree with you. I'm working on a patch (only RFC thus far) which
> allows you to designate some module parameters as debug knobs. As soon as
> users touch them they'll get
> - a stern warning in dmesg
> - TAINT_USER'ed kernel

Ah yes, I remember the patch, that's the one thing I was going to ask
about before my colleagues insisted on leaving for lunch... What's the
status and can we get that in first?

BR,
Jani.


>
> That should be about as good as we can make it.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Barbalho, Rafael]

<snip>
 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c    | 39
> ++++++++++++++++++++++++++++++++++----
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 7d6acb4..d5ae8dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1941,6 +1941,7 @@ struct i915_params {
>  	bool reset;
>  	bool disable_display;
>  	bool disable_vtd_wa;
> +	int dp_link_train_policy;
>  };
>  extern struct i915_params i915 __read_mostly;
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index d05a2af..8c358e7 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -48,6 +48,7 @@ struct i915_params i915 __read_mostly = {
>  	.disable_display = 0,
>  	.enable_cmd_parser = 1,
>  	.disable_vtd_wa = 0,
> +	.dp_link_train_policy = 0,
>  };
> 
>  module_param_named(modeset, i915.modeset, int, 0400); @@ -156,3
> +157,10 @@ MODULE_PARM_DESC(disable_vtd_wa, "Disable all VT-d
> workarounds (default: false)"
>  module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int,
> 0600);  MODULE_PARM_DESC(enable_cmd_parser,
>  		 "Enable command parsing (1=enabled [default],
> 0=disabled)");
> +
> +module_param_named(dp_link_train_policy, i915.dp_link_train_policy,
> +int, 0600); MODULE_PARM_DESC(dp_link_train_policy,
> +	"Choose strategy for DP link training "
> +	"(0=narrow and fast [default], 1=wide and slow. For other values, "
> +	"bits 11:8 are for the BW and bits 7:4 are for the nubmer of lanes, "
> +	"check intel_dp_compute_link_config() for more details.)");

There is a typo here, nubmer should be number, otherwise:

Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Friday, April 25, 2014 10:19 AM
> To: Jani Nikula
> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: add i915.dp_link_train_policy
> option
> 
> On Fri, Apr 25, 2014 at 12:00:34PM +0300, Jani Nikula wrote:
> > On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> We still have way too many bugs with DP link training. We keep
> > >> switching between "narrow and fast" and "wide and slow", we
> > >> recently added 5GHz support, and whenever there's a bug report, we
> > >> have to ask people to apply patches and test them.
> > >>
> > >> Wouldn't it be so much better if we could just ask them to boot
> > >> with some specific Kernel boot option instead of applying a patch?
> > >> This will move the situation from "i915.ko is completely broken!"
> > >> to "i915.ko's default values are broken, but there's an option I
> > >> can set to fix it, so I won't need to learn how to compile a Kernel!".
> > >>
> > >> Some useful values:
> > >>  - i915.dp_link_train_policy=1 for "wide and slow"
> > >>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
> > >>    which should be able to fit 1920x1080@60Hz and 24bpp
> > >>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
> > >>    not-so-huge modes
> > >>
> > >> The default behavior is still the same.
> > >>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >
> > > Yeah, I like this. I'll sign up Todd to review this all.
> >
> > I guess we'll go with this then, but I'll step back from this
> > particular patch for a bit, and share my concerns over module
> > parameters and quirks.
> >
> > I am generally opposed to adding module parameters or quirks to
> > workaround issues in features that should just work. They are an easy
> > way out for things we should root cause and fix properly.
> >
> > Do not mistake me for an idealist for thinking this way, as I'm being
> > pragmatic.
> >
> > The people who report bugs to us are roughly the same people who are
> > capable of setting the module parameter. Once they figure that out,
> > they'll stop responding to our requests for testing and info. We've
> > seen this happen before. We'd hurt our chances of making things work
> > out of the box for the average user.
> >
> > The more we add module parameters, the combinations of them explode.
> > Debugging *other* problems becomes harder. In the bugs I work on, the
> > #1 request I have is full dmesg, partially because I want to see all
> > the wild kernel parameters the user might have set. And all too often
> > they have. When there are module parameters that fix some bugs, the
> > blogs and forums get filled with tips about them, and people use them,
> > whether they strictly have the same bug or not. Search for i915 invert
> > brightness for example.
> >
> > It's also not easy to drop module parameters after we've added them.
> > You know the drill. Even after we've fixed everything the module
> > parameter was supposed to fix, dropping it leads to
> https://xkcd.com/1172/.
> 
> I fully agree with you. I'm working on a patch (only RFC thus far) which allows
> you to designate some module parameters as debug knobs. As soon as users
> touch them they'll get
> - a stern warning in dmesg
> - TAINT_USER'ed kernel
> 
> That should be about as good as we can make it.
> -Daniel

How hard would it be to expand the series afterwards this review to automatically try both "wide & slow" and "narrow & fast" for a given mode before giving up and logging the error?

I can foresee us replying a lot to people saying try wide or narrow when we could have the driver do that for us. The link training policy then could just be used to debug things later for those special cases. I think this would be a good thing.

For the whole series:
Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Paulo Zanoni]

2014-04-25 6:00 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> We still have way too many bugs with DP link training. We keep
>>> switching between "narrow and fast" and "wide and slow", we recently
>>> added 5GHz support, and whenever there's a bug report, we have to ask
>>> people to apply patches and test them.
>>>
>>> Wouldn't it be so much better if we could just ask them to boot with
>>> some specific Kernel boot option instead of applying a patch? This
>>> will move the situation from "i915.ko is completely broken!" to
>>> "i915.ko's default values are broken, but there's an option I can set
>>> to fix it, so I won't need to learn how to compile a Kernel!".
>>>
>>> Some useful values:
>>>  - i915.dp_link_train_policy=1 for "wide and slow"
>>>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
>>>    which should be able to fit 1920x1080@60Hz and 24bpp
>>>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
>>>    not-so-huge modes
>>>
>>> The default behavior is still the same.
>>>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Yeah, I like this. I'll sign up Todd to review this all.
>
> I guess we'll go with this then, but I'll step back from this particular
> patch for a bit, and share my concerns over module parameters and
> quirks.
>
> I am generally opposed to adding module parameters or quirks to
> workaround issues in features that should just work. They are an easy
> way out for things we should root cause and fix properly.
>
> Do not mistake me for an idealist for thinking this way, as I'm being
> pragmatic.
>
> The people who report bugs to us are roughly the same people who are
> capable of setting the module parameter. Once they figure that out,
> they'll stop responding to our requests for testing and info. We've seen
> this happen before. We'd hurt our chances of making things work out of
> the box for the average user.
>
> The more we add module parameters, the combinations of them
> explode. Debugging *other* problems becomes harder. In the bugs I work
> on, the #1 request I have is full dmesg, partially because I want to see
> all the wild kernel parameters the user might have set. And all too
> often they have. When there are module parameters that fix some bugs,
> the blogs and forums get filled with tips about them, and people use
> them, whether they strictly have the same bug or not. Search for i915
> invert brightness for example.
>
> It's also not easy to drop module parameters after we've added them. You
> know the drill. Even after we've fixed everything the module parameter
> was supposed to fix, dropping it leads to https://xkcd.com/1172/.

Hi

I totally understand your point and I agree that your concerns are
valid. But OTOH the patch could substantially improve the lives of our
users, and it allow allows much easier debugging for the developers.
So I really think there's a tradeoff between "making it easier for
users" vs "hiding bugs" vs "making it easier to debug". In the ideal
world, we will make the default parameter work for everybody, and that
will be the only option. But while that's not the case, we could at
least try to make something better. I also remember the RC6 case,
where the addition of the Kernel parameter allowed more people to
actually test RC6 and report the problems, and in the end we finally
fixed the remaining issue that was preventing RC6 from being enabled
by default. But yes, we still have people that use RC6 parameters
values they are not supposed to use, but OTOH we also have users who
can enable RC6 on ILK without any problems.

I wrote this patch for my own debugging purposes, and it was very
useful to my development efforts. I have quite a few panels here, and
I was trying to find a panel that doesn't work with either "wide and
slow" or "narrow and fast", so the command-line option allowed me to
quickly replace panels and retry. I also found a panel that doesn't
work at all, and the finer-grained option allowed me to test it.

I also have written other similar debug options in the past, that I
never sent to the mailing list. For example, I have local patches that
allow me to choose a different policy for the panel power sequencing
registers: I wrote these when I noticed that, in a Mac machine, the
power sequencing values set on boot were completely different from the
ones set on the VBT, and the values chosen by our driver were the
slowest of both worlds.

So I guess this discussion is a nice opportunity for us to reach a
general conclusion on what is the policy we should adopt for adding
parameters that allow better debugging. Maybe if we had a way to
specify that some parameters are strictly for debugging, or that some
parameters are expected to break user's machines...

Anyway, I can discard the patch if we want. I can also remove the
option to arbitrarily force a given bw+lane configuration.

I also found a bug on my own patch (missing "break" statement), so I
need to send V2 anyway.

Thanks,
Paulo

>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Daniel Vetter]

On Fri, Apr 25, 2014 at 02:48:07PM -0300, Paulo Zanoni wrote:
> 2014-04-25 6:00 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> > On Fri, 25 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
> >>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>
> >>> We still have way too many bugs with DP link training. We keep
> >>> switching between "narrow and fast" and "wide and slow", we recently
> >>> added 5GHz support, and whenever there's a bug report, we have to ask
> >>> people to apply patches and test them.
> >>>
> >>> Wouldn't it be so much better if we could just ask them to boot with
> >>> some specific Kernel boot option instead of applying a patch? This
> >>> will move the situation from "i915.ko is completely broken!" to
> >>> "i915.ko's default values are broken, but there's an option I can set
> >>> to fix it, so I won't need to learn how to compile a Kernel!".
> >>>
> >>> Some useful values:
> >>>  - i915.dp_link_train_policy=1 for "wide and slow"
> >>>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
> >>>    which should be able to fit 1920x1080@60Hz and 24bpp
> >>>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
> >>>    not-so-huge modes
> >>>
> >>> The default behavior is still the same.
> >>>
> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Yeah, I like this. I'll sign up Todd to review this all.
> >
> > I guess we'll go with this then, but I'll step back from this particular
> > patch for a bit, and share my concerns over module parameters and
> > quirks.
> >
> > I am generally opposed to adding module parameters or quirks to
> > workaround issues in features that should just work. They are an easy
> > way out for things we should root cause and fix properly.
> >
> > Do not mistake me for an idealist for thinking this way, as I'm being
> > pragmatic.
> >
> > The people who report bugs to us are roughly the same people who are
> > capable of setting the module parameter. Once they figure that out,
> > they'll stop responding to our requests for testing and info. We've seen
> > this happen before. We'd hurt our chances of making things work out of
> > the box for the average user.
> >
> > The more we add module parameters, the combinations of them
> > explode. Debugging *other* problems becomes harder. In the bugs I work
> > on, the #1 request I have is full dmesg, partially because I want to see
> > all the wild kernel parameters the user might have set. And all too
> > often they have. When there are module parameters that fix some bugs,
> > the blogs and forums get filled with tips about them, and people use
> > them, whether they strictly have the same bug or not. Search for i915
> > invert brightness for example.
> >
> > It's also not easy to drop module parameters after we've added them. You
> > know the drill. Even after we've fixed everything the module parameter
> > was supposed to fix, dropping it leads to https://xkcd.com/1172/.
> 
> Hi
> 
> I totally understand your point and I agree that your concerns are
> valid. But OTOH the patch could substantially improve the lives of our
> users, and it allow allows much easier debugging for the developers.
> So I really think there's a tradeoff between "making it easier for
> users" vs "hiding bugs" vs "making it easier to debug". In the ideal
> world, we will make the default parameter work for everybody, and that
> will be the only option. But while that's not the case, we could at
> least try to make something better. I also remember the RC6 case,
> where the addition of the Kernel parameter allowed more people to
> actually test RC6 and report the problems, and in the end we finally
> fixed the remaining issue that was preventing RC6 from being enabled
> by default. But yes, we still have people that use RC6 parameters
> values they are not supposed to use, but OTOH we also have users who
> can enable RC6 on ILK without any problems.
> 
> I wrote this patch for my own debugging purposes, and it was very
> useful to my development efforts. I have quite a few panels here, and
> I was trying to find a panel that doesn't work with either "wide and
> slow" or "narrow and fast", so the command-line option allowed me to
> quickly replace panels and retry. I also found a panel that doesn't
> work at all, and the finer-grained option allowed me to test it.
> 
> I also have written other similar debug options in the past, that I
> never sent to the mailing list. For example, I have local patches that
> allow me to choose a different policy for the panel power sequencing
> registers: I wrote these when I noticed that, in a Mac machine, the
> power sequencing values set on boot were completely different from the
> ones set on the VBT, and the values chosen by our driver were the
> slowest of both worlds.
> 
> So I guess this discussion is a nice opportunity for us to reach a
> general conclusion on what is the policy we should adopt for adding
> parameters that allow better debugging. Maybe if we had a way to
> specify that some parameters are strictly for debugging, or that some
> parameters are expected to break user's machines...
> 
> Anyway, I can discard the patch if we want. I can also remove the
> option to arbitrarily force a given bw+lane configuration.
> 
> I also found a bug on my own patch (missing "break" statement), so I
> need to send V2 anyway.

Imo for anything related to modesetting or which needs to be set at driver
load we want module options - for users it's really hard to set something
in debugfs with a black screen.

But we also want to ensure that users to play around with this for fun, so
I'll work on the debug module option thing a bit more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch