[PATCH 0/5] Handle Link Training Failure during modeset

[PATCH 0/5] Handle Link Training Failure during modeset

[Manasi Navare]

Link training failure is handled by lowering the link rate first
until it reaches the minimum and keeping the lane count maximum
and then lowering the lane count until it reaches minimim. These
fallback values are saved and hotplug uevent is sent to the userspace
after setting the connector link status property to BAD. Userspace
should triiger another modeset on a uevent and if link status property
is BAD. This will retrain the link at fallback values.
This is repeated until the link is successfully trained.

This has been validated to pass DP compliance.

Manasi Navare (5):
  drm: Add a new connector property for link status
  drm/i915: Set link status property for DP connector
  drm/i915: Update CRTC state if connector link status property changed
  drm/i915: Find fallback link rate/lane count
  drm/i915: Implement Link Rate fallback on Link training failure

 drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
 drivers/gpu/drm/drm_connector.c               |  17 ++++
 drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
 drivers/gpu/drm/i915/intel_dp.c               | 138 +++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
 drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
 include/drm/drm_connector.h                   |   7 +-
 include/drm/drm_crtc.h                        |   5 +
 include/uapi/drm/drm_mode.h                   |   4 +
 9 files changed, 214 insertions(+), 9 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm: Add a new connector property for link status

[Manasi Navare]

A new default connector property is added for keeping
track of whether the link is good (link training passed) or
link is bad (link training  failed). If the link status property
is not good, then userspace should fire off a new modeset at the current
mode even if there have not been any changes in the mode list
or connector status.
Also add link status connector member corersponding to the
decoded value of link status property.

v3:
* Drop "link training" from description since this is
not specific to DP (Jani Nikula)
* Add link status member to store property value locally
(Ville Syrjala)
v2:
* Make this a default connector property (Daniel Vetter)

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_connector.c | 17 +++++++++++++++++
 include/drm/drm_connector.h     |  7 ++++++-
 include/drm/drm_crtc.h          |  5 +++++
 include/uapi/drm/drm_mode.h     |  4 ++++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2db7fb5..d4e852f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -243,6 +243,10 @@ int drm_connector_init(struct drm_device *dev,
 	drm_object_attach_property(&connector->base,
 				      config->dpms_property, 0);
 
+	drm_object_attach_property(&connector->base,
+				   config->link_status_property,
+				   0);
+
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
 	}
@@ -506,6 +510,12 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order)
 };
 DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
 
+static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
+	{ DRM_MODE_LINK_STATUS_GOOD, "Good" },
+	{ DRM_MODE_LINK_STATUS_BAD, "Bad" },
+};
+DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
+
 /**
  * drm_display_info_set_bus_formats - set the supported bus formats
  * @info: display info to store bus formats in
@@ -622,6 +632,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.tile_property = prop;
 
+	prop = drm_property_create_enum(dev, 0, "link-status",
+					drm_link_status_enum_list,
+					ARRAY_SIZE(drm_link_status_enum_list));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.link_status_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3e97272..ad5c8b0 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -695,6 +695,12 @@ struct drm_connector {
 	uint8_t num_h_tile, num_v_tile;
 	uint8_t tile_h_loc, tile_v_loc;
 	uint16_t tile_h_size, tile_v_size;
+
+	/* Connector Link status
+	 * 0: If the link is Good
+	 * 1: If the link is Bad
+	 */
+	int link_status;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
@@ -767,7 +773,6 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
 int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
-
 int drm_mode_connector_set_path_property(struct drm_connector *connector,
 					 const char *path);
 int drm_mode_connector_set_tile_property(struct drm_connector *connector);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8cca2a8..a93148b 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1164,6 +1164,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *tile_property;
 	/**
+	 * @link_status_property: Default connector property for link status
+	 * of a connector
+	 */
+	struct drm_property *link_status_property;
+	/**
 	 * @plane_type_property: Default plane property to differentiate
 	 * CURSOR, PRIMARY and OVERLAY legacy uses of planes.
 	 */
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 01000c9..66b145b 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -129,6 +129,10 @@
 #define DRM_MODE_DIRTY_ON       1
 #define DRM_MODE_DIRTY_ANNOTATE 2
 
+/* Link Status options */
+#define DRM_MODE_LINK_STATUS_GOOD	0
+#define DRM_MODE_LINK_STATUS_BAD	1
+
 struct drm_mode_modeinfo {
 	__u32 clock;
 	__u16 hdisplay;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Set link status property for DP connector

[Manasi Navare]

This defines a helper function to set the property value.
This will be used to set the link status to Bad in case
of link training failures.

v3:
* Set connector->link_status in this function
(Jani Nikula)
v2:
* Simplify the return value (Jani Nikula)

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 117a714..1f1760f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4640,6 +4640,18 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+int
+intel_dp_set_link_status_property(struct drm_connector *connector,
+				  uint64_t val)
+{
+	struct drm_device *dev = connector->dev;
+
+	connector->link_status = val;
+	return drm_object_property_set_value(&connector->base,
+					     dev->mode_config.link_status_property,
+					     val);
+}
+
 static int
 intel_dp_connector_register(struct drm_connector *connector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 003afb8..856dd41 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1381,6 +1381,8 @@ u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
 bool intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port);
 bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			     struct intel_connector *intel_connector);
+int intel_dp_set_link_status_property(struct drm_connector *connector,
+				      uint64_t val);
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      int link_rate, uint8_t lane_count,
 			      bool link_mst);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Manasi Navare]

CRTC state connector_changed needs to be set to true
if connector link status property has changed. This will tell the
driver to do a complete modeset due to change in connector property.

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5007796..aeecf2f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 					       connector_state);
 		if (ret)
 			return ret;
+
+		if (connector->state->crtc) {
+			crtc_state = drm_atomic_get_existing_crtc_state(state,
+									connector->state->crtc);
+			if (connector->link_status == DRM_MODE_LINK_STATUS_BAD)
+				crtc_state->connectors_changed = true;
+		}
 	}
 
 	/*
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Find fallback link rate/lane count

[Manasi Navare]

If link training fails, then we need to fallback to lower
link rate first and if link training fails at RBR, then
fallback to lower lane count.
This function finds the next lower link rate/lane count
value after link training failure.

v2:
Squash the patch that returns the link rate index (Jani Nikula)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 42 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  6 ++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1f1760f..9694857 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -288,6 +288,48 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
 			       common_rates);
 }
 
+static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
+				    int *common_rates, int link_rate)
+{
+	int common_len;
+	int index;
+
+	common_len = intel_dp_common_rates(intel_dp, common_rates);
+	for (index = 0; index < common_len; index++) {
+		if (link_rate == common_rates[common_len - index - 1])
+			return common_len - index - 1;
+	}
+
+	return -1;
+}
+
+int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
+					    int link_rate, uint8_t lane_count)
+{
+	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
+	int common_len;
+	int link_rate_index = -1;
+
+	common_len = intel_dp_common_rates(intel_dp, common_rates);
+	link_rate_index = intel_dp_link_rate_index(intel_dp,
+						   common_rates,
+						   link_rate);
+	if (link_rate_index > 0) {
+		intel_dp->fallback_link_rate_index = link_rate_index - 1;
+		intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index];
+		intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp);
+	} else if (lane_count > 1) {
+		intel_dp->fallback_link_rate_index = common_len - 1;
+		intel_dp->fallback_link_rate = common_rates[intel_dp->fallback_link_rate_index];
+		intel_dp->fallback_lane_count = lane_count >> 1;
+	} else {
+		DRM_ERROR("Link Training Unsuccessful\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static enum drm_mode_status
 intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 856dd41..4a49a2d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -888,6 +888,10 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	int fallback_link_rate;
+	uint8_t fallback_lane_count;
+	int fallback_link_rate_index;
+	bool link_train_failed;
 	uint8_t sink_count;
 	bool link_mst;
 	bool has_audio;
@@ -1386,6 +1390,8 @@ int intel_dp_set_link_status_property(struct drm_connector *connector,
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      int link_rate, uint8_t lane_count,
 			      bool link_mst);
+int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
+					    int link_rate, uint8_t lane_count);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Manasi Navare]

If link training at a link rate optimal for a particular
mode fails during modeset's atomic commit phase, then we
let the modeset complete and then retry. We save the link rate
value at which link training failed, update the link status property
to "BAD" and use a lower link rate to prune the modes. It will redo
the modeset on the current mode at lower link rate or if the current
mode gets pruned due to lower link constraints then, it will send a
hotplug uevent for userspace to handle it.

This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4,
4.3.1.6.

v4:
* Add fallback support for non DDI platforms too
* Set connector->link status inside set_link_status function
(Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variables link_train_failed and link_status
to indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c              |  21 +++++-
 drivers/gpu/drm/i915/intel_dp.c               | 104 +++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
 drivers/gpu/drm/i915/intel_drv.h              |   6 +-
 4 files changed, 133 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0ad4e16..fa5989a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1684,6 +1684,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = intel_ddi_get_encoder_port(encoder);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 
 	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
 				 link_mst);
@@ -1694,7 +1696,24 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	intel_prepare_dp_ddi_buffers(encoder);
 	intel_ddi_init_dp_buf_reg(encoder);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-	intel_dp_start_link_train(intel_dp);
+	if (!intel_dp_start_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+			      link_rate, lane_count);
+		if (!intel_dp_get_link_train_fallback_values(intel_dp,
+							     link_rate,
+							     lane_count))
+			/* Schedule a Hotplug Uevent to userspace to start modeset */
+			schedule_work(&intel_connector->modeset_retry_work);
+	} else {
+		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+			      link_rate, lane_count);
+		intel_dp->fallback_link_rate_index = -1;
+		intel_dp->fallback_link_rate = 0;
+		intel_dp->fallback_lane_count = 0;
+		intel_dp_set_link_status_property(connector,
+						  DRM_MODE_LINK_STATUS_GOOD);
+	}
+
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9694857..b6b9405 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -353,8 +353,14 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 		target_clock = fixed_mode->clock;
 	}
 
-	max_link_clock = intel_dp_max_link_rate(intel_dp);
-	max_lanes = intel_dp_max_lane_count(intel_dp);
+	/* Prune the modes using the fallback link rate/lane count */
+	if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) {
+		max_link_clock = intel_dp->fallback_link_rate;
+		max_lanes = intel_dp->fallback_lane_count;
+	} else {
+		max_link_clock = intel_dp_max_link_rate(intel_dp);
+		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);
@@ -1591,6 +1597,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	enum port port = dp_to_dig_port(intel_dp)->port;
 	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 	int lane_count, clock;
 	int min_lane_count = 1;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
@@ -1638,6 +1645,12 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
+	/* Fall back to lower link rate in case of failure in previous modeset */
+	if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) {
+		min_lane_count = max_lane_count = intel_dp->fallback_lane_count;
+		min_clock = max_clock = intel_dp->fallback_link_rate_index;
+	}
+
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %d pixel clock %iKHz\n",
 		      max_lane_count, common_rates[max_clock],
@@ -2784,6 +2797,8 @@ static void intel_enable_dp(struct intel_encoder *encoder,
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
 	enum pipe pipe = crtc->pipe;
 
@@ -2814,7 +2829,23 @@ static void intel_enable_dp(struct intel_encoder *encoder,
 	}
 
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-	intel_dp_start_link_train(intel_dp);
+	if (!intel_dp_start_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+			      intel_dp->link_rate, intel_dp->lane_count);
+		if (!intel_dp_get_link_train_fallback_values(intel_dp,
+							     intel_dp->link_rate,
+							     intel_dp->lane_count))
+			/* Schedule a Hotplug Uevent to userspace to start modeset */
+			schedule_work(&intel_connector->modeset_retry_work);
+	} else {
+		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+			      intel_dp->link_rate, intel_dp->lane_count);
+		intel_dp->fallback_link_rate_index = -1;
+		intel_dp->fallback_link_rate = 0;
+		intel_dp->fallback_lane_count = 0;
+		intel_dp_set_link_status_property(connector,
+						  DRM_MODE_LINK_STATUS_GOOD);
+	}
 	intel_dp_stop_link_train(intel_dp);
 
 	if (pipe_config->has_audio) {
@@ -4021,6 +4052,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 
 	/* Suppress underruns caused by re-training */
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
@@ -4028,7 +4061,23 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 		intel_set_pch_fifo_underrun_reporting(dev_priv,
 						      intel_crtc_pch_transcoder(crtc), false);
 
-	intel_dp_start_link_train(intel_dp);
+	if (!intel_dp_start_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+			      intel_dp->link_rate, intel_dp->lane_count);
+		if (!intel_dp_get_link_train_fallback_values(intel_dp,
+							     intel_dp->link_rate,
+							     intel_dp->lane_count))
+			/* Schedule a Hotplug Uevent to userspace to start modeset */
+			schedule_work(&intel_connector->modeset_retry_work);
+	} else {
+		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+			      intel_dp->link_rate, intel_dp->lane_count);
+		intel_dp->fallback_link_rate_index = -1;
+		intel_dp->fallback_link_rate = 0;
+		intel_dp->fallback_lane_count = 0;
+		intel_dp_set_link_status_property(connector,
+						  DRM_MODE_LINK_STATUS_GOOD);
+	}
 	intel_dp_stop_link_train(intel_dp);
 
 	/* Keep underrun reporting disabled until things are stable */
@@ -4422,6 +4471,11 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 		intel_dp->compliance_test_active = 0;
 		intel_dp->compliance_test_type = 0;
 		intel_dp->compliance_test_data = 0;
+		intel_dp->fallback_link_rate_index = -1;
+		intel_dp->fallback_link_rate = 0;
+		intel_dp->fallback_lane_count = 0;
+		intel_dp_set_link_status_property(connector,
+						  DRM_MODE_LINK_STATUS_GOOD);
 
 		if (intel_dp->is_mst) {
 			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
@@ -4513,6 +4567,11 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	/* If this is a retry due to link trianing failure */
+	if (status == connector_status_connected &&
+	    connector->link_status == DRM_MODE_LINK_STATUS_BAD)
+		return status;
+
 	/* If full detect is not performed yet, do a full detect */
 	if (!intel_dp->detect_done)
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
@@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	return false;
 }
 
+static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
+{
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+	struct drm_display_mode *mode;
+	bool verbose_prune = true;
+
+	intel_connector = container_of(work, typeof(*intel_connector),
+				       modeset_retry_work);
+	connector = &intel_connector->base;
+
+	/* Grab the locks before changing connector property*/
+	mutex_lock(&connector->dev->mode_config.mutex);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
+		      connector->name);
+	list_for_each_entry(mode, &connector->modes, head) {
+		mode->status = intel_dp_mode_valid(connector,
+						   mode);
+	}
+	drm_mode_prune_invalid(connector->dev, &connector->modes,
+			       verbose_prune);
+
+	/* Set connector link status to BAD and send a Uevent to notify
+	 * userspace to do a modeset.
+	 */
+	intel_dp_set_link_status_property(connector,
+					  DRM_MODE_LINK_STATUS_BAD);
+	mutex_unlock(&connector->dev->mode_config.mutex);
+
+	/* Send Hotplug uevent so userspace can reprobe */
+	drm_kms_helper_hotplug_event(connector->dev);
+}
+
 bool
 intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector)
@@ -5704,6 +5796,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	enum port port = intel_dig_port->port;
 	int type;
 
+	/* Initialize the work for modeset in case of link train failure */
+	INIT_WORK(&intel_connector->modeset_retry_work,
+		  intel_dp_modeset_retry_work_fn);
+
 	if (WARN(intel_dig_port->max_lanes < 1,
 		 "Not enough lanes (%d) for DP on port %c\n",
 		 intel_dig_port->max_lanes, port_name(port)))
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 0048b52..10f81ab 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
-void
+bool
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_link_training_clock_recovery(intel_dp);
-	intel_dp_link_training_channel_equalization(intel_dp);
+	bool ret;
+
+	if (intel_dp_link_training_clock_recovery(intel_dp)) {
+		ret = intel_dp_link_training_channel_equalization(intel_dp);
+		if (ret)
+			return true;
+	}
+	return false;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4a49a2d..2a286c5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -312,6 +312,9 @@ struct intel_connector {
 	void *port; /* store this opaque as its illegal to dereference it */
 
 	struct intel_dp *mst_port;
+
+	/* Work struct to schedule a uevent on link train failure */
+	struct work_struct modeset_retry_work;
 };
 
 struct dpll {
@@ -891,7 +894,6 @@ struct intel_dp {
 	int fallback_link_rate;
 	uint8_t fallback_lane_count;
 	int fallback_link_rate_index;
-	bool link_train_failed;
 	uint8_t sink_count;
 	bool link_mst;
 	bool has_audio;
@@ -1392,7 +1394,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      bool link_mst);
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count);
-void intel_dp_start_link_train(struct intel_dp *intel_dp);
+bool intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Handle Link Training Failure during modeset

[Patchwork]

== Series Details ==

Series: Handle Link Training Failure during modeset
URL   : https://patchwork.freedesktop.org/series/15080/
State : failure

== Summary ==

Series 15080v1 Handle Link Training Failure during modeset
https://patchwork.freedesktop.org/api/1.0/series/15080/revisions/1/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> INCOMPLETE (fi-skl-6700k)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-ivb-3520m)
        Subgroup suspend-read-crc-pipe-b:
                fail       -> PASS       (fi-ivb-3520m)
        Subgroup suspend-read-crc-pipe-c:
                fail       -> PASS       (fi-ivb-3520m)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:6    pass:5    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

1afd351596b6deaead14d5812f0d6850379e30ad drm-intel-nightly: 2016y-11m-09d-21h-03m-31s UTC integration manifest
13ab659 drm/i915: Implement Link Rate fallback on Link training failure
2793b26 drm/i915: Find fallback link rate/lane count
37a0b049 drm/i915: Update CRTC state if connector link status property changed
81cc918 drm/i915: Set link status property for DP connector
5837520 drm: Add a new connector property for link status

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2949/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Jani Nikula]

On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Link training failure is handled by lowering the link rate first
> until it reaches the minimum and keeping the lane count maximum
> and then lowering the lane count until it reaches minimim. These
> fallback values are saved and hotplug uevent is sent to the userspace
> after setting the connector link status property to BAD. Userspace
> should triiger another modeset on a uevent and if link status property
> is BAD. This will retrain the link at fallback values.
> This is repeated until the link is successfully trained.
>
> This has been validated to pass DP compliance.

This cover letter and the commit messages do a good job of explaining
what the patches do. However, you're lacking the crucial information of
*why* we need userspace cooperation to handle link training failures on
DP mode setting, and *why* a new connector property is a good solution
for this.

Here goes, including some alternative approaches we've considered (and
even tried):

At the time userspace does setcrtc, we've already promised the mode
would work. The promise is based on the theoretical capabilities of the
link, but it's possible we can't reach this in practice. The DP spec
describes how the link should be reduced, but we can't reduce the link
below the requirements of the mode. Black screen follows.

One idea would be to have setcrtc return a failure. However, it is my
understanding that it already should not fail as the atomic checks have
passed [citation needed]. It would also conflict with the idea of making
setcrtc asynchronous in the future, returning before the actual mode
setting and link training.

Another idea is to train the link "upfront" at hotplug time, before
pruning the mode list, so that we can do the pruning based on practical
not theoretical capabilities. However, the changes for link training are
pretty drastic, all for the sake of error handling and DP compliance,
when the most common happy day scenario is the current approach of link
training at mode setting time, using the optimal parameters for the
mode. It is also not certain all hardware could do this without the pipe
on; not even all our hardware can do this. Some of this can be solved,
but not trivially.

Both of the above ideas also fail to address link degradation *during*
operation.

So the idea presented in these patches is to do this in a way that a)
changes the current happy day scenario as little as possible, to avoid
regressions, b) can be implemented the same way by all drm drivers, c)
is still opt-in for the drivers and userspace, and opting out doesn't
regress the user experience, d) doesn't prevent drivers from
implementing better or alternate approaches, possibly without userspace
involvement. And, of course, handles all the issues presented.

The solution is to add a "link status" connector property. In the usual
happy day scenario, this is always "good". If something fails during or
after a mode set, the kernel driver can set the link status to "bad",
prune the mode list based on new information as necessary, and send a
hotplug uevent for userspace to have it re-check the valid modes through
getconnector, and try again. If the theoretical capabilities of the link
can't be reached, the mode list is trimmed based on that.

If the userspace is not aware of the property, the user experience is
the same as it currently is. If the userspace is aware of the property,
it has a chance to improve user experience. If a drm driver does not
modify the property (it stays "good"), the user experience is the same
as it currently is. A drm driver can also choose to try to handle more
of the failures in kernel, hardware not limiting, or it can choose to
involve userspace more. Up to the drivers.

The reason for adding the property is to handle link training failures,
but it is not limited to DP or link training. For example, if we
implement asynchronous setcrtc, we can use this to report any failures
in that.

Finally, while DP CTS compliance is advertized (which is great, and
could be made to work similarly for all drm drivers), this can be used
for the more important goal of improving user experience on link
training failures, by avoiding black screens.


BR,
Jani.


>
> Manasi Navare (5):
>   drm: Add a new connector property for link status
>   drm/i915: Set link status property for DP connector
>   drm/i915: Update CRTC state if connector link status property changed
    ^^^^^^^^

This is really drm, not i915.


>   drm/i915: Find fallback link rate/lane count
>   drm/i915: Implement Link Rate fallback on Link training failure
>
>  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
>  drivers/gpu/drm/drm_connector.c               |  17 ++++
>  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
>  drivers/gpu/drm/i915/intel_dp.c               | 138 +++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
>  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
>  include/drm/drm_connector.h                   |   7 +-
>  include/drm/drm_crtc.h                        |   5 +
>  include/uapi/drm/drm_mode.h                   |   4 +
>  9 files changed, 214 insertions(+), 9 deletions(-)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

[Deucher, Alexander]

Adding Harry and Tony from our display team to review.

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Thursday, November 10, 2016 1:20 PM
> To: Manasi Navare; dri-devel@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org
> Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during
> modeset
> 
> On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > Link training failure is handled by lowering the link rate first
> > until it reaches the minimum and keeping the lane count maximum
> > and then lowering the lane count until it reaches minimim. These
> > fallback values are saved and hotplug uevent is sent to the userspace
> > after setting the connector link status property to BAD. Userspace
> > should triiger another modeset on a uevent and if link status property
> > is BAD. This will retrain the link at fallback values.
> > This is repeated until the link is successfully trained.
> >
> > This has been validated to pass DP compliance.
> 
> This cover letter and the commit messages do a good job of explaining
> what the patches do. However, you're lacking the crucial information of
> *why* we need userspace cooperation to handle link training failures on
> DP mode setting, and *why* a new connector property is a good solution
> for this.
> 
> Here goes, including some alternative approaches we've considered (and
> even tried):
> 
> At the time userspace does setcrtc, we've already promised the mode
> would work. The promise is based on the theoretical capabilities of the
> link, but it's possible we can't reach this in practice. The DP spec
> describes how the link should be reduced, but we can't reduce the link
> below the requirements of the mode. Black screen follows.
> 
> One idea would be to have setcrtc return a failure. However, it is my
> understanding that it already should not fail as the atomic checks have
> passed [citation needed]. It would also conflict with the idea of making
> setcrtc asynchronous in the future, returning before the actual mode
> setting and link training.
> 
> Another idea is to train the link "upfront" at hotplug time, before
> pruning the mode list, so that we can do the pruning based on practical
> not theoretical capabilities. However, the changes for link training are
> pretty drastic, all for the sake of error handling and DP compliance,
> when the most common happy day scenario is the current approach of link
> training at mode setting time, using the optimal parameters for the
> mode. It is also not certain all hardware could do this without the pipe
> on; not even all our hardware can do this. Some of this can be solved,
> but not trivially.
> 
> Both of the above ideas also fail to address link degradation *during*
> operation.
> 
> So the idea presented in these patches is to do this in a way that a)
> changes the current happy day scenario as little as possible, to avoid
> regressions, b) can be implemented the same way by all drm drivers, c)
> is still opt-in for the drivers and userspace, and opting out doesn't
> regress the user experience, d) doesn't prevent drivers from
> implementing better or alternate approaches, possibly without userspace
> involvement. And, of course, handles all the issues presented.
> 
> The solution is to add a "link status" connector property. In the usual
> happy day scenario, this is always "good". If something fails during or
> after a mode set, the kernel driver can set the link status to "bad",
> prune the mode list based on new information as necessary, and send a
> hotplug uevent for userspace to have it re-check the valid modes through
> getconnector, and try again. If the theoretical capabilities of the link
> can't be reached, the mode list is trimmed based on that.
> 
> If the userspace is not aware of the property, the user experience is
> the same as it currently is. If the userspace is aware of the property,
> it has a chance to improve user experience. If a drm driver does not
> modify the property (it stays "good"), the user experience is the same
> as it currently is. A drm driver can also choose to try to handle more
> of the failures in kernel, hardware not limiting, or it can choose to
> involve userspace more. Up to the drivers.
> 
> The reason for adding the property is to handle link training failures,
> but it is not limited to DP or link training. For example, if we
> implement asynchronous setcrtc, we can use this to report any failures
> in that.
> 
> Finally, while DP CTS compliance is advertized (which is great, and
> could be made to work similarly for all drm drivers), this can be used
> for the more important goal of improving user experience on link
> training failures, by avoiding black screens.
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Manasi Navare (5):
> >   drm: Add a new connector property for link status
> >   drm/i915: Set link status property for DP connector
> >   drm/i915: Update CRTC state if connector link status property changed
>     ^^^^^^^^
> 
> This is really drm, not i915.
> 
> 
> >   drm/i915: Find fallback link rate/lane count
> >   drm/i915: Implement Link Rate fallback on Link training failure
> >
> >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> >  drivers/gpu/drm/i915/intel_dp.c               | 138
> +++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> >  include/drm/drm_connector.h                   |   7 +-
> >  include/drm/drm_crtc.h                        |   5 +
> >  include/uapi/drm/drm_mode.h                   |   4 +
> >  9 files changed, 214 insertions(+), 9 deletions(-)
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Cheng, Tony]

Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.

AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?

We can also past DP1.2 link layer compliance with this approach.

-----Original Message-----
From: Deucher, Alexander 
Sent: Thursday, November 10, 2016 1:43 PM
To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin <martin.peres@intel.com>
Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

Adding Harry and Tony from our display team to review.

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Thursday, November 10, 2016 1:20 PM
> To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> gfx@lists.freedesktop.org
> Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> during modeset
> 
> On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > Link training failure is handled by lowering the link rate first 
> > until it reaches the minimum and keeping the lane count maximum and 
> > then lowering the lane count until it reaches minimim. These 
> > fallback values are saved and hotplug uevent is sent to the 
> > userspace after setting the connector link status property to BAD. 
> > Userspace should triiger another modeset on a uevent and if link 
> > status property is BAD. This will retrain the link at fallback values.
> > This is repeated until the link is successfully trained.
> >
> > This has been validated to pass DP compliance.
> 
> This cover letter and the commit messages do a good job of explaining 
> what the patches do. However, you're lacking the crucial information 
> of
> *why* we need userspace cooperation to handle link training failures 
> on DP mode setting, and *why* a new connector property is a good 
> solution for this.
> 
> Here goes, including some alternative approaches we've considered (and 
> even tried):
> 
> At the time userspace does setcrtc, we've already promised the mode 
> would work. The promise is based on the theoretical capabilities of 
> the link, but it's possible we can't reach this in practice. The DP 
> spec describes how the link should be reduced, but we can't reduce the 
> link below the requirements of the mode. Black screen follows.
> 
> One idea would be to have setcrtc return a failure. However, it is my 
> understanding that it already should not fail as the atomic checks 
> have passed [citation needed]. It would also conflict with the idea of 
> making setcrtc asynchronous in the future, returning before the actual 
> mode setting and link training.
> 
> Another idea is to train the link "upfront" at hotplug time, before 
> pruning the mode list, so that we can do the pruning based on 
> practical not theoretical capabilities. However, the changes for link 
> training are pretty drastic, all for the sake of error handling and DP 
> compliance, when the most common happy day scenario is the current 
> approach of link training at mode setting time, using the optimal 
> parameters for the mode. It is also not certain all hardware could do 
> this without the pipe on; not even all our hardware can do this. Some 
> of this can be solved, but not trivially.
> 
> Both of the above ideas also fail to address link degradation *during* 
> operation.
> 
> So the idea presented in these patches is to do this in a way that a) 
> changes the current happy day scenario as little as possible, to avoid 
> regressions, b) can be implemented the same way by all drm drivers, c) 
> is still opt-in for the drivers and userspace, and opting out doesn't 
> regress the user experience, d) doesn't prevent drivers from 
> implementing better or alternate approaches, possibly without 
> userspace involvement. And, of course, handles all the issues presented.
> 
> The solution is to add a "link status" connector property. In the 
> usual happy day scenario, this is always "good". If something fails 
> during or after a mode set, the kernel driver can set the link status 
> to "bad", prune the mode list based on new information as necessary, 
> and send a hotplug uevent for userspace to have it re-check the valid 
> modes through getconnector, and try again. If the theoretical 
> capabilities of the link can't be reached, the mode list is trimmed based on that.
> 
> If the userspace is not aware of the property, the user experience is 
> the same as it currently is. If the userspace is aware of the 
> property, it has a chance to improve user experience. If a drm driver 
> does not modify the property (it stays "good"), the user experience is 
> the same as it currently is. A drm driver can also choose to try to 
> handle more of the failures in kernel, hardware not limiting, or it 
> can choose to involve userspace more. Up to the drivers.
> 
> The reason for adding the property is to handle link training 
> failures, but it is not limited to DP or link training. For example, 
> if we implement asynchronous setcrtc, we can use this to report any 
> failures in that.
> 
> Finally, while DP CTS compliance is advertized (which is great, and 
> could be made to work similarly for all drm drivers), this can be used 
> for the more important goal of improving user experience on link 
> training failures, by avoiding black screens.
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Manasi Navare (5):
> >   drm: Add a new connector property for link status
> >   drm/i915: Set link status property for DP connector
> >   drm/i915: Update CRTC state if connector link status property 
> > changed
>     ^^^^^^^^
> 
> This is really drm, not i915.
> 
> 
> >   drm/i915: Find fallback link rate/lane count
> >   drm/i915: Implement Link Rate fallback on Link training failure
> >
> >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> >  drivers/gpu/drm/i915/intel_dp.c               | 138
> +++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> >  include/drm/drm_connector.h                   |   7 +-
> >  include/drm/drm_crtc.h                        |   5 +
> >  include/uapi/drm/drm_mode.h                   |   4 +
> >  9 files changed, 214 insertions(+), 9 deletions(-)
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Daniel Vetter]

On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
> @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> +{
> +	struct intel_connector *intel_connector;
> +	struct drm_connector *connector;
> +	struct drm_display_mode *mode;
> +	bool verbose_prune = true;
> +
> +	intel_connector = container_of(work, typeof(*intel_connector),
> +				       modeset_retry_work);
> +	connector = &intel_connector->base;
> +
> +	/* Grab the locks before changing connector property*/
> +	mutex_lock(&connector->dev->mode_config.mutex);
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> +		      connector->name);
> +	list_for_each_entry(mode, &connector->modes, head) {
> +		mode->status = intel_dp_mode_valid(connector,
> +						   mode);
> +	}
> +	drm_mode_prune_invalid(connector->dev, &connector->modes,
> +			       verbose_prune);
> +
> +	/* Set connector link status to BAD and send a Uevent to notify
> +	 * userspace to do a modeset.
> +	 */
> +	intel_dp_set_link_status_property(connector,
> +					  DRM_MODE_LINK_STATUS_BAD);
> +	mutex_unlock(&connector->dev->mode_config.mutex);
> +
> +	/* Send Hotplug uevent so userspace can reprobe */
> +	drm_kms_helper_hotplug_event(connector->dev);

One downside of keeping all the signalling logic here in i915 is that we
don't have a good spot to put the kerneldoc for this new link status
property within drm_connector.c for others to easily spot. My suggestion
would be to extract function with the following rough pseudo-code:

drm_connector_set_link_status(connector, status)
{
	mutex_lock(mode_config.mutex);

	/* what intel_dp_set_link_status_property() does */
	
	mutex_unlock(mode_config.mutex);
	drm_kms_helper_hotplug_event()
}

Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
lock, and then call this function. The lock drop&reacquire is a bit ugly,
but:
- it doesn't matter from a performance pov, this is a slow, asynchronous
  work.
- it doesn't matter for correctnes, the only thing we need is to drop the
  lock before calling drm_kms_helper_hotplug_event, and that we update the
  link status and the mode list before that too.
- long term I expect that properties will get separate locks to protect
  their values, and restrict mode_config.mutex to just mode probing. Which
  means drm_connnector_set_link_status will use a different lock anyway.
- it nicely encapsulates stuff imo.

Kerneldoc for drm_connector_set_link_status should mention that this needs
to be run from some async work item, and ofc have the full explanation
(maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
of how this should be used.

Since this is late-stage polish definitely wait for more feedback and for
the design to fully settle first.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Jani Nikula]

On Thu, 10 Nov 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
>> @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  	return false;
>>  }
>>  
>> +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
>> +{
>> +	struct intel_connector *intel_connector;
>> +	struct drm_connector *connector;
>> +	struct drm_display_mode *mode;
>> +	bool verbose_prune = true;
>> +
>> +	intel_connector = container_of(work, typeof(*intel_connector),
>> +				       modeset_retry_work);
>> +	connector = &intel_connector->base;
>> +
>> +	/* Grab the locks before changing connector property*/
>> +	mutex_lock(&connector->dev->mode_config.mutex);
>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
>> +		      connector->name);
>> +	list_for_each_entry(mode, &connector->modes, head) {
>> +		mode->status = intel_dp_mode_valid(connector,
>> +						   mode);
>> +	}
>> +	drm_mode_prune_invalid(connector->dev, &connector->modes,
>> +			       verbose_prune);
>> +
>> +	/* Set connector link status to BAD and send a Uevent to notify
>> +	 * userspace to do a modeset.
>> +	 */
>> +	intel_dp_set_link_status_property(connector,
>> +					  DRM_MODE_LINK_STATUS_BAD);
>> +	mutex_unlock(&connector->dev->mode_config.mutex);
>> +
>> +	/* Send Hotplug uevent so userspace can reprobe */
>> +	drm_kms_helper_hotplug_event(connector->dev);
>
> One downside of keeping all the signalling logic here in i915 is that we
> don't have a good spot to put the kerneldoc for this new link status
> property within drm_connector.c for others to easily spot. My suggestion
> would be to extract function with the following rough pseudo-code:

Thanks for this. I wanted Manasi to keep the work struct and function
within i915, but I fell short of coming up with this division.

BR,
Jani.



>
> drm_connector_set_link_status(connector, status)
> {
> 	mutex_lock(mode_config.mutex);
>
> 	/* what intel_dp_set_link_status_property() does */
> 	
> 	mutex_unlock(mode_config.mutex);
> 	drm_kms_helper_hotplug_event()
> }
>
> Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
> lock, and then call this function. The lock drop&reacquire is a bit ugly,
> but:
> - it doesn't matter from a performance pov, this is a slow, asynchronous
>   work.
> - it doesn't matter for correctnes, the only thing we need is to drop the
>   lock before calling drm_kms_helper_hotplug_event, and that we update the
>   link status and the mode list before that too.
> - long term I expect that properties will get separate locks to protect
>   their values, and restrict mode_config.mutex to just mode probing. Which
>   means drm_connnector_set_link_status will use a different lock anyway.
> - it nicely encapsulates stuff imo.
>
> Kerneldoc for drm_connector_set_link_status should mention that this needs
> to be run from some async work item, and ofc have the full explanation
> (maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
> of how this should be used.
>
> Since this is late-stage polish definitely wait for more feedback and for
> the design to fully settle first.
> -Daniel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Ville Syrjälä]

On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> 
> AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?

I can't recall the specifics for all of our supported platforms, but at
least I have the recollection that it would be the case yes.

The other problem wiyh this apporach is that even if you don't need the
crtc, you still need the link itself. What happens if the link is still
active since userspace just didn't bother to shut it down when the cable
was yanked? Can you keep the crtc going but stop it from feeding the
link in a way that userspace won't be able to notice? The kms design has
always been pretty much that policy is in userspace, and thus the kernel
shouldn't shut down crtcs unless explicitly asked to do so.

> 
> We can also past DP1.2 link layer compliance with this approach.
> 
> -----Original Message-----
> From: Deucher, Alexander 
> Sent: Thursday, November 10, 2016 1:43 PM
> To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin <martin.peres@intel.com>
> Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset
> 
> Adding Harry and Tony from our display team to review.
> 
> > -----Original Message-----
> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > Sent: Thursday, November 10, 2016 1:20 PM
> > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > gfx@lists.freedesktop.org
> > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > Link training failure is handled by lowering the link rate first 
> > > until it reaches the minimum and keeping the lane count maximum and 
> > > then lowering the lane count until it reaches minimim. These 
> > > fallback values are saved and hotplug uevent is sent to the 
> > > userspace after setting the connector link status property to BAD. 
> > > Userspace should triiger another modeset on a uevent and if link 
> > > status property is BAD. This will retrain the link at fallback values.
> > > This is repeated until the link is successfully trained.
> > >
> > > This has been validated to pass DP compliance.
> > 
> > This cover letter and the commit messages do a good job of explaining 
> > what the patches do. However, you're lacking the crucial information 
> > of
> > *why* we need userspace cooperation to handle link training failures 
> > on DP mode setting, and *why* a new connector property is a good 
> > solution for this.
> > 
> > Here goes, including some alternative approaches we've considered (and 
> > even tried):
> > 
> > At the time userspace does setcrtc, we've already promised the mode 
> > would work. The promise is based on the theoretical capabilities of 
> > the link, but it's possible we can't reach this in practice. The DP 
> > spec describes how the link should be reduced, but we can't reduce the 
> > link below the requirements of the mode. Black screen follows.
> > 
> > One idea would be to have setcrtc return a failure. However, it is my 
> > understanding that it already should not fail as the atomic checks 
> > have passed [citation needed]. It would also conflict with the idea of 
> > making setcrtc asynchronous in the future, returning before the actual 
> > mode setting and link training.
> > 
> > Another idea is to train the link "upfront" at hotplug time, before 
> > pruning the mode list, so that we can do the pruning based on 
> > practical not theoretical capabilities. However, the changes for link 
> > training are pretty drastic, all for the sake of error handling and DP 
> > compliance, when the most common happy day scenario is the current 
> > approach of link training at mode setting time, using the optimal 
> > parameters for the mode. It is also not certain all hardware could do 
> > this without the pipe on; not even all our hardware can do this. Some 
> > of this can be solved, but not trivially.
> > 
> > Both of the above ideas also fail to address link degradation *during* 
> > operation.
> > 
> > So the idea presented in these patches is to do this in a way that a) 
> > changes the current happy day scenario as little as possible, to avoid 
> > regressions, b) can be implemented the same way by all drm drivers, c) 
> > is still opt-in for the drivers and userspace, and opting out doesn't 
> > regress the user experience, d) doesn't prevent drivers from 
> > implementing better or alternate approaches, possibly without 
> > userspace involvement. And, of course, handles all the issues presented.
> > 
> > The solution is to add a "link status" connector property. In the 
> > usual happy day scenario, this is always "good". If something fails 
> > during or after a mode set, the kernel driver can set the link status 
> > to "bad", prune the mode list based on new information as necessary, 
> > and send a hotplug uevent for userspace to have it re-check the valid 
> > modes through getconnector, and try again. If the theoretical 
> > capabilities of the link can't be reached, the mode list is trimmed based on that.
> > 
> > If the userspace is not aware of the property, the user experience is 
> > the same as it currently is. If the userspace is aware of the 
> > property, it has a chance to improve user experience. If a drm driver 
> > does not modify the property (it stays "good"), the user experience is 
> > the same as it currently is. A drm driver can also choose to try to 
> > handle more of the failures in kernel, hardware not limiting, or it 
> > can choose to involve userspace more. Up to the drivers.
> > 
> > The reason for adding the property is to handle link training 
> > failures, but it is not limited to DP or link training. For example, 
> > if we implement asynchronous setcrtc, we can use this to report any 
> > failures in that.
> > 
> > Finally, while DP CTS compliance is advertized (which is great, and 
> > could be made to work similarly for all drm drivers), this can be used 
> > for the more important goal of improving user experience on link 
> > training failures, by avoiding black screens.
> > 
> > 
> > BR,
> > Jani.
> > 
> > 
> > >
> > > Manasi Navare (5):
> > >   drm: Add a new connector property for link status
> > >   drm/i915: Set link status property for DP connector
> > >   drm/i915: Update CRTC state if connector link status property 
> > > changed
> >     ^^^^^^^^
> > 
> > This is really drm, not i915.
> > 
> > 
> > >   drm/i915: Find fallback link rate/lane count
> > >   drm/i915: Implement Link Rate fallback on Link training failure
> > >
> > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > +++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > >  include/drm/drm_connector.h                   |   7 +-
> > >  include/drm/drm_crtc.h                        |   5 +
> > >  include/uapi/drm/drm_mode.h                   |   4 +
> > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > 
> > --
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Ville Syrjälä]

On Thu, Nov 10, 2016 at 09:58:31PM +0100, Daniel Vetter wrote:
> On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
> > @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	return false;
> >  }
> >  
> > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > +{
> > +	struct intel_connector *intel_connector;
> > +	struct drm_connector *connector;
> > +	struct drm_display_mode *mode;
> > +	bool verbose_prune = true;
> > +
> > +	intel_connector = container_of(work, typeof(*intel_connector),
> > +				       modeset_retry_work);
> > +	connector = &intel_connector->base;
> > +
> > +	/* Grab the locks before changing connector property*/
> > +	mutex_lock(&connector->dev->mode_config.mutex);
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > +		      connector->name);
> > +	list_for_each_entry(mode, &connector->modes, head) {
> > +		mode->status = intel_dp_mode_valid(connector,
> > +						   mode);
> > +	}
> > +	drm_mode_prune_invalid(connector->dev, &connector->modes,
> > +			       verbose_prune);
> > +
> > +	/* Set connector link status to BAD and send a Uevent to notify
> > +	 * userspace to do a modeset.
> > +	 */
> > +	intel_dp_set_link_status_property(connector,
> > +					  DRM_MODE_LINK_STATUS_BAD);
> > +	mutex_unlock(&connector->dev->mode_config.mutex);
> > +
> > +	/* Send Hotplug uevent so userspace can reprobe */
> > +	drm_kms_helper_hotplug_event(connector->dev);
> 
> One downside of keeping all the signalling logic here in i915 is that we
> don't have a good spot to put the kerneldoc for this new link status
> property within drm_connector.c for others to easily spot. My suggestion
> would be to extract function with the following rough pseudo-code:
> 
> drm_connector_set_link_status(connector, status)
> {
> 	mutex_lock(mode_config.mutex);
> 
> 	/* what intel_dp_set_link_status_property() does */
> 	
> 	mutex_unlock(mode_config.mutex);
> 	drm_kms_helper_hotplug_event()
> }
> 
> Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
> lock, and then call this function. The lock drop&reacquire is a bit ugly,
> but:

The mode re-validation should be done in the core as well. Not sure if
we could just stuff it all into one place, and then there would be no
need for any lock dropping.

> - it doesn't matter from a performance pov, this is a slow, asynchronous
>   work.
> - it doesn't matter for correctnes, the only thing we need is to drop the
>   lock before calling drm_kms_helper_hotplug_event, and that we update the
>   link status and the mode list before that too.
> - long term I expect that properties will get separate locks to protect
>   their values, and restrict mode_config.mutex to just mode probing. Which
>   means drm_connnector_set_link_status will use a different lock anyway.
> - it nicely encapsulates stuff imo.
> 
> Kerneldoc for drm_connector_set_link_status should mention that this needs
> to be run from some async work item, and ofc have the full explanation
> (maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
> of how this should be used.
> 
> Since this is late-stage polish definitely wait for more feedback and for
> the design to fully settle first.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Manasi Navare]

On Fri, Nov 11, 2016 at 04:08:26PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 10, 2016 at 09:58:31PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
> > > @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > >  	return false;
> > >  }
> > >  
> > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > > +{
> > > +	struct intel_connector *intel_connector;
> > > +	struct drm_connector *connector;
> > > +	struct drm_display_mode *mode;
> > > +	bool verbose_prune = true;
> > > +
> > > +	intel_connector = container_of(work, typeof(*intel_connector),
> > > +				       modeset_retry_work);
> > > +	connector = &intel_connector->base;
> > > +
> > > +	/* Grab the locks before changing connector property*/
> > > +	mutex_lock(&connector->dev->mode_config.mutex);
> > > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > > +		      connector->name);
> > > +	list_for_each_entry(mode, &connector->modes, head) {
> > > +		mode->status = intel_dp_mode_valid(connector,
> > > +						   mode);
> > > +	}
> > > +	drm_mode_prune_invalid(connector->dev, &connector->modes,
> > > +			       verbose_prune);
> > > +
> > > +	/* Set connector link status to BAD and send a Uevent to notify
> > > +	 * userspace to do a modeset.
> > > +	 */
> > > +	intel_dp_set_link_status_property(connector,
> > > +					  DRM_MODE_LINK_STATUS_BAD);
> > > +	mutex_unlock(&connector->dev->mode_config.mutex);
> > > +
> > > +	/* Send Hotplug uevent so userspace can reprobe */
> > > +	drm_kms_helper_hotplug_event(connector->dev);
> > 
> > One downside of keeping all the signalling logic here in i915 is that we
> > don't have a good spot to put the kerneldoc for this new link status
> > property within drm_connector.c for others to easily spot. My suggestion
> > would be to extract function with the following rough pseudo-code:
> > 
> > drm_connector_set_link_status(connector, status)
> > {
> > 	mutex_lock(mode_config.mutex);
> > 
> > 	/* what intel_dp_set_link_status_property() does */
> > 	
> > 	mutex_unlock(mode_config.mutex);
> > 	drm_kms_helper_hotplug_event()
> > }
> > 
> > Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
> > lock, and then call this function. The lock drop&reacquire is a bit ugly,
> > but:
> 
> The mode re-validation should be done in the core as well. Not sure if
> we could just stuff it all into one place, and then there would be no
> need for any lock dropping.
>

I can move the moderevalidation to the core as well, but then the function name
has to be something else than just drm_set_link_status_property(),cant think
of a name that combines mode revalidation and setting link sttaus property,
any suggestions?

Manasi 
> > - it doesn't matter from a performance pov, this is a slow, asynchronous
> >   work.
> > - it doesn't matter for correctnes, the only thing we need is to drop the
> >   lock before calling drm_kms_helper_hotplug_event, and that we update the
> >   link status and the mode list before that too.
> > - long term I expect that properties will get separate locks to protect
> >   their values, and restrict mode_config.mutex to just mode probing. Which
> >   means drm_connnector_set_link_status will use a different lock anyway.
> > - it nicely encapsulates stuff imo.
> > 
> > Kerneldoc for drm_connector_set_link_status should mention that this needs
> > to be run from some async work item, and ofc have the full explanation
> > (maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
> > of how this should be used.
> > 
> > Since this is late-stage polish definitely wait for more feedback and for
> > the design to fully settle first.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Manasi Navare]

On Fri, Nov 11, 2016 at 11:41:22AM +0200, Jani Nikula wrote:
> On Thu, 10 Nov 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
> >> @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >>  	return false;
> >>  }
> >>  
> >> +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> >> +{
> >> +	struct intel_connector *intel_connector;
> >> +	struct drm_connector *connector;
> >> +	struct drm_display_mode *mode;
> >> +	bool verbose_prune = true;
> >> +
> >> +	intel_connector = container_of(work, typeof(*intel_connector),
> >> +				       modeset_retry_work);
> >> +	connector = &intel_connector->base;
> >> +
> >> +	/* Grab the locks before changing connector property*/
> >> +	mutex_lock(&connector->dev->mode_config.mutex);
> >> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> >> +		      connector->name);
> >> +	list_for_each_entry(mode, &connector->modes, head) {
> >> +		mode->status = intel_dp_mode_valid(connector,
> >> +						   mode);
> >> +	}
> >> +	drm_mode_prune_invalid(connector->dev, &connector->modes,
> >> +			       verbose_prune);
> >> +
> >> +	/* Set connector link status to BAD and send a Uevent to notify
> >> +	 * userspace to do a modeset.
> >> +	 */
> >> +	intel_dp_set_link_status_property(connector,
> >> +					  DRM_MODE_LINK_STATUS_BAD);
> >> +	mutex_unlock(&connector->dev->mode_config.mutex);
> >> +
> >> +	/* Send Hotplug uevent so userspace can reprobe */
> >> +	drm_kms_helper_hotplug_event(connector->dev);
> >
> > One downside of keeping all the signalling logic here in i915 is that we
> > don't have a good spot to put the kerneldoc for this new link status
> > property within drm_connector.c for others to easily spot. My suggestion
> > would be to extract function with the following rough pseudo-code:
> 
> Thanks for this. I wanted Manasi to keep the work struct and function
> within i915, but I fell short of coming up with this division.
> 
> BR,
> Jani.
> 
>

Jani, so we ar elooking at two functions going in core,
1. that does mode validation and pruning
2. set link status property

These should be in a separate patch right after declaring the
drm connector property, what do you think?

Regards
Manasi

 
> 
> >
> > drm_connector_set_link_status(connector, status)
> > {
> > 	mutex_lock(mode_config.mutex);
> >
> > 	/* what intel_dp_set_link_status_property() does */
> > 	
> > 	mutex_unlock(mode_config.mutex);
> > 	drm_kms_helper_hotplug_event()
> > }
> >
> > Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
> > lock, and then call this function. The lock drop&reacquire is a bit ugly,
> > but:
> > - it doesn't matter from a performance pov, this is a slow, asynchronous
> >   work.
> > - it doesn't matter for correctnes, the only thing we need is to drop the
> >   lock before calling drm_kms_helper_hotplug_event, and that we update the
> >   link status and the mode list before that too.
> > - long term I expect that properties will get separate locks to protect
> >   their values, and restrict mode_config.mutex to just mode probing. Which
> >   means drm_connnector_set_link_status will use a different lock anyway.
> > - it nicely encapsulates stuff imo.
> >
> > Kerneldoc for drm_connector_set_link_status should mention that this needs
> > to be run from some async work item, and ofc have the full explanation
> > (maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
> > of how this should be used.
> >
> > Since this is late-stage polish definitely wait for more feedback and for
> > the design to fully settle first.
> > -Daniel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Cheng, Tony]

For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.

For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).

From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.

windows does not know link state and all link management is hidden behind kernel driver.   

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, November 11, 2016 9:05 AM
To: Cheng, Tony <Tony.Cheng@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> 
> AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?

I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.

The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.

> 
> We can also past DP1.2 link layer compliance with this approach.
> 
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Thursday, November 10, 2016 1:43 PM
> To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> intel-gfx@lists.freedesktop.org; Wentland, Harry 
> <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> <martin.peres@intel.com>
> Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> during modeset
> 
> Adding Harry and Tony from our display team to review.
> 
> > -----Original Message-----
> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > Sent: Thursday, November 10, 2016 1:20 PM
> > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > gfx@lists.freedesktop.org
> > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > Link training failure is handled by lowering the link rate first 
> > > until it reaches the minimum and keeping the lane count maximum 
> > > and then lowering the lane count until it reaches minimim. These 
> > > fallback values are saved and hotplug uevent is sent to the 
> > > userspace after setting the connector link status property to BAD.
> > > Userspace should triiger another modeset on a uevent and if link 
> > > status property is BAD. This will retrain the link at fallback values.
> > > This is repeated until the link is successfully trained.
> > >
> > > This has been validated to pass DP compliance.
> > 
> > This cover letter and the commit messages do a good job of 
> > explaining what the patches do. However, you're lacking the crucial 
> > information of
> > *why* we need userspace cooperation to handle link training failures 
> > on DP mode setting, and *why* a new connector property is a good 
> > solution for this.
> > 
> > Here goes, including some alternative approaches we've considered 
> > (and even tried):
> > 
> > At the time userspace does setcrtc, we've already promised the mode 
> > would work. The promise is based on the theoretical capabilities of 
> > the link, but it's possible we can't reach this in practice. The DP 
> > spec describes how the link should be reduced, but we can't reduce 
> > the link below the requirements of the mode. Black screen follows.
> > 
> > One idea would be to have setcrtc return a failure. However, it is 
> > my understanding that it already should not fail as the atomic 
> > checks have passed [citation needed]. It would also conflict with 
> > the idea of making setcrtc asynchronous in the future, returning 
> > before the actual mode setting and link training.
> > 
> > Another idea is to train the link "upfront" at hotplug time, before 
> > pruning the mode list, so that we can do the pruning based on 
> > practical not theoretical capabilities. However, the changes for 
> > link training are pretty drastic, all for the sake of error handling 
> > and DP compliance, when the most common happy day scenario is the 
> > current approach of link training at mode setting time, using the 
> > optimal parameters for the mode. It is also not certain all hardware 
> > could do this without the pipe on; not even all our hardware can do 
> > this. Some of this can be solved, but not trivially.
> > 
> > Both of the above ideas also fail to address link degradation 
> > *during* operation.
> > 
> > So the idea presented in these patches is to do this in a way that 
> > a) changes the current happy day scenario as little as possible, to 
> > avoid regressions, b) can be implemented the same way by all drm 
> > drivers, c) is still opt-in for the drivers and userspace, and 
> > opting out doesn't regress the user experience, d) doesn't prevent 
> > drivers from implementing better or alternate approaches, possibly 
> > without userspace involvement. And, of course, handles all the issues presented.
> > 
> > The solution is to add a "link status" connector property. In the 
> > usual happy day scenario, this is always "good". If something fails 
> > during or after a mode set, the kernel driver can set the link 
> > status to "bad", prune the mode list based on new information as 
> > necessary, and send a hotplug uevent for userspace to have it 
> > re-check the valid modes through getconnector, and try again. If the 
> > theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > 
> > If the userspace is not aware of the property, the user experience 
> > is the same as it currently is. If the userspace is aware of the 
> > property, it has a chance to improve user experience. If a drm 
> > driver does not modify the property (it stays "good"), the user 
> > experience is the same as it currently is. A drm driver can also 
> > choose to try to handle more of the failures in kernel, hardware not 
> > limiting, or it can choose to involve userspace more. Up to the drivers.
> > 
> > The reason for adding the property is to handle link training 
> > failures, but it is not limited to DP or link training. For example, 
> > if we implement asynchronous setcrtc, we can use this to report any 
> > failures in that.
> > 
> > Finally, while DP CTS compliance is advertized (which is great, and 
> > could be made to work similarly for all drm drivers), this can be 
> > used for the more important goal of improving user experience on 
> > link training failures, by avoiding black screens.
> > 
> > 
> > BR,
> > Jani.
> > 
> > 
> > >
> > > Manasi Navare (5):
> > >   drm: Add a new connector property for link status
> > >   drm/i915: Set link status property for DP connector
> > >   drm/i915: Update CRTC state if connector link status property 
> > > changed
> >     ^^^^^^^^
> > 
> > This is really drm, not i915.
> > 
> > 
> > >   drm/i915: Find fallback link rate/lane count
> > >   drm/i915: Implement Link Rate fallback on Link training failure
> > >
> > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > +++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > >  include/drm/drm_connector.h                   |   7 +-
> > >  include/drm/drm_crtc.h                        |   5 +
> > >  include/uapi/drm/drm_mode.h                   |   4 +
> > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > 
> > --
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> 
> For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).

The link should get retrained as the driver should check the link
state on HPD and retrain if it's not good. At least that's what we do in
i915. Of course if it's not the same display that got plugged back,
the retraining might fail. The new property can help with that
case as userspace would then attempt a full modeset after the failed
link training.

> 
> >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> 
> windows does not know link state and all link management is hidden behind kernel driver.   

If the user visible state doesn't change in any way, then the kernel could
try to manage it all internally.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, November 11, 2016 9:05 AM
> To: Cheng, Tony <Tony.Cheng@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset
> 
> On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > 
> > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> 
> I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> 
> The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> 
> > 
> > We can also past DP1.2 link layer compliance with this approach.
> > 
> > -----Original Message-----
> > From: Deucher, Alexander
> > Sent: Thursday, November 10, 2016 1:43 PM
> > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > <martin.peres@intel.com>
> > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > Adding Harry and Tony from our display team to review.
> > 
> > > -----Original Message-----
> > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > Sent: Thursday, November 10, 2016 1:20 PM
> > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > gfx@lists.freedesktop.org
> > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > Link training failure is handled by lowering the link rate first 
> > > > until it reaches the minimum and keeping the lane count maximum 
> > > > and then lowering the lane count until it reaches minimim. These 
> > > > fallback values are saved and hotplug uevent is sent to the 
> > > > userspace after setting the connector link status property to BAD.
> > > > Userspace should triiger another modeset on a uevent and if link 
> > > > status property is BAD. This will retrain the link at fallback values.
> > > > This is repeated until the link is successfully trained.
> > > >
> > > > This has been validated to pass DP compliance.
> > > 
> > > This cover letter and the commit messages do a good job of 
> > > explaining what the patches do. However, you're lacking the crucial 
> > > information of
> > > *why* we need userspace cooperation to handle link training failures 
> > > on DP mode setting, and *why* a new connector property is a good 
> > > solution for this.
> > > 
> > > Here goes, including some alternative approaches we've considered 
> > > (and even tried):
> > > 
> > > At the time userspace does setcrtc, we've already promised the mode 
> > > would work. The promise is based on the theoretical capabilities of 
> > > the link, but it's possible we can't reach this in practice. The DP 
> > > spec describes how the link should be reduced, but we can't reduce 
> > > the link below the requirements of the mode. Black screen follows.
> > > 
> > > One idea would be to have setcrtc return a failure. However, it is 
> > > my understanding that it already should not fail as the atomic 
> > > checks have passed [citation needed]. It would also conflict with 
> > > the idea of making setcrtc asynchronous in the future, returning 
> > > before the actual mode setting and link training.
> > > 
> > > Another idea is to train the link "upfront" at hotplug time, before 
> > > pruning the mode list, so that we can do the pruning based on 
> > > practical not theoretical capabilities. However, the changes for 
> > > link training are pretty drastic, all for the sake of error handling 
> > > and DP compliance, when the most common happy day scenario is the 
> > > current approach of link training at mode setting time, using the 
> > > optimal parameters for the mode. It is also not certain all hardware 
> > > could do this without the pipe on; not even all our hardware can do 
> > > this. Some of this can be solved, but not trivially.
> > > 
> > > Both of the above ideas also fail to address link degradation 
> > > *during* operation.
> > > 
> > > So the idea presented in these patches is to do this in a way that 
> > > a) changes the current happy day scenario as little as possible, to 
> > > avoid regressions, b) can be implemented the same way by all drm 
> > > drivers, c) is still opt-in for the drivers and userspace, and 
> > > opting out doesn't regress the user experience, d) doesn't prevent 
> > > drivers from implementing better or alternate approaches, possibly 
> > > without userspace involvement. And, of course, handles all the issues presented.
> > > 
> > > The solution is to add a "link status" connector property. In the 
> > > usual happy day scenario, this is always "good". If something fails 
> > > during or after a mode set, the kernel driver can set the link 
> > > status to "bad", prune the mode list based on new information as 
> > > necessary, and send a hotplug uevent for userspace to have it 
> > > re-check the valid modes through getconnector, and try again. If the 
> > > theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > 
> > > If the userspace is not aware of the property, the user experience 
> > > is the same as it currently is. If the userspace is aware of the 
> > > property, it has a chance to improve user experience. If a drm 
> > > driver does not modify the property (it stays "good"), the user 
> > > experience is the same as it currently is. A drm driver can also 
> > > choose to try to handle more of the failures in kernel, hardware not 
> > > limiting, or it can choose to involve userspace more. Up to the drivers.
> > > 
> > > The reason for adding the property is to handle link training 
> > > failures, but it is not limited to DP or link training. For example, 
> > > if we implement asynchronous setcrtc, we can use this to report any 
> > > failures in that.
> > > 
> > > Finally, while DP CTS compliance is advertized (which is great, and 
> > > could be made to work similarly for all drm drivers), this can be 
> > > used for the more important goal of improving user experience on 
> > > link training failures, by avoiding black screens.
> > > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > >
> > > > Manasi Navare (5):
> > > >   drm: Add a new connector property for link status
> > > >   drm/i915: Set link status property for DP connector
> > > >   drm/i915: Update CRTC state if connector link status property 
> > > > changed
> > >     ^^^^^^^^
> > > 
> > > This is really drm, not i915.
> > > 
> > > 
> > > >   drm/i915: Find fallback link rate/lane count
> > > >   drm/i915: Implement Link Rate fallback on Link training failure
> > > >
> > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > +++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > >  include/drm/drm_connector.h                   |   7 +-
> > > >  include/drm/drm_crtc.h                        |   5 +
> > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > 
> > > --
> > > Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Cheng, Tony]

In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?

For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, November 11, 2016 11:44 AM
To: Cheng, Tony <Tony.Cheng@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> 
> For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).

The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.

> 
> >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> 
> windows does not know link state and all link management is hidden behind kernel driver.   

If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, November 11, 2016 9:05 AM
> To: Cheng, Tony <Tony.Cheng@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> <jani.nikula@linux.intel.com>; Manasi Navare 
> <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> intel-gfx@lists.freedesktop.org; Wentland, Harry 
> <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> during modeset
> 
> On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > 
> > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> 
> I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> 
> The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> 
> > 
> > We can also past DP1.2 link layer compliance with this approach.
> > 
> > -----Original Message-----
> > From: Deucher, Alexander
> > Sent: Thursday, November 10, 2016 1:43 PM
> > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > <martin.peres@intel.com>
> > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > Adding Harry and Tony from our display team to review.
> > 
> > > -----Original Message-----
> > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > Sent: Thursday, November 10, 2016 1:20 PM
> > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > gfx@lists.freedesktop.org
> > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > Link training failure is handled by lowering the link rate first 
> > > > until it reaches the minimum and keeping the lane count maximum 
> > > > and then lowering the lane count until it reaches minimim. These 
> > > > fallback values are saved and hotplug uevent is sent to the 
> > > > userspace after setting the connector link status property to BAD.
> > > > Userspace should triiger another modeset on a uevent and if link 
> > > > status property is BAD. This will retrain the link at fallback values.
> > > > This is repeated until the link is successfully trained.
> > > >
> > > > This has been validated to pass DP compliance.
> > > 
> > > This cover letter and the commit messages do a good job of 
> > > explaining what the patches do. However, you're lacking the 
> > > crucial information of
> > > *why* we need userspace cooperation to handle link training 
> > > failures on DP mode setting, and *why* a new connector property is 
> > > a good solution for this.
> > > 
> > > Here goes, including some alternative approaches we've considered 
> > > (and even tried):
> > > 
> > > At the time userspace does setcrtc, we've already promised the 
> > > mode would work. The promise is based on the theoretical 
> > > capabilities of the link, but it's possible we can't reach this in 
> > > practice. The DP spec describes how the link should be reduced, 
> > > but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > 
> > > One idea would be to have setcrtc return a failure. However, it is 
> > > my understanding that it already should not fail as the atomic 
> > > checks have passed [citation needed]. It would also conflict with 
> > > the idea of making setcrtc asynchronous in the future, returning 
> > > before the actual mode setting and link training.
> > > 
> > > Another idea is to train the link "upfront" at hotplug time, 
> > > before pruning the mode list, so that we can do the pruning based 
> > > on practical not theoretical capabilities. However, the changes 
> > > for link training are pretty drastic, all for the sake of error 
> > > handling and DP compliance, when the most common happy day 
> > > scenario is the current approach of link training at mode setting 
> > > time, using the optimal parameters for the mode. It is also not 
> > > certain all hardware could do this without the pipe on; not even 
> > > all our hardware can do this. Some of this can be solved, but not trivially.
> > > 
> > > Both of the above ideas also fail to address link degradation
> > > *during* operation.
> > > 
> > > So the idea presented in these patches is to do this in a way that
> > > a) changes the current happy day scenario as little as possible, 
> > > to avoid regressions, b) can be implemented the same way by all 
> > > drm drivers, c) is still opt-in for the drivers and userspace, and 
> > > opting out doesn't regress the user experience, d) doesn't prevent 
> > > drivers from implementing better or alternate approaches, possibly 
> > > without userspace involvement. And, of course, handles all the issues presented.
> > > 
> > > The solution is to add a "link status" connector property. In the 
> > > usual happy day scenario, this is always "good". If something 
> > > fails during or after a mode set, the kernel driver can set the 
> > > link status to "bad", prune the mode list based on new information 
> > > as necessary, and send a hotplug uevent for userspace to have it 
> > > re-check the valid modes through getconnector, and try again. If 
> > > the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > 
> > > If the userspace is not aware of the property, the user experience 
> > > is the same as it currently is. If the userspace is aware of the 
> > > property, it has a chance to improve user experience. If a drm 
> > > driver does not modify the property (it stays "good"), the user 
> > > experience is the same as it currently is. A drm driver can also 
> > > choose to try to handle more of the failures in kernel, hardware 
> > > not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > 
> > > The reason for adding the property is to handle link training 
> > > failures, but it is not limited to DP or link training. For 
> > > example, if we implement asynchronous setcrtc, we can use this to 
> > > report any failures in that.
> > > 
> > > Finally, while DP CTS compliance is advertized (which is great, 
> > > and could be made to work similarly for all drm drivers), this can 
> > > be used for the more important goal of improving user experience 
> > > on link training failures, by avoiding black screens.
> > > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > >
> > > > Manasi Navare (5):
> > > >   drm: Add a new connector property for link status
> > > >   drm/i915: Set link status property for DP connector
> > > >   drm/i915: Update CRTC state if connector link status property 
> > > > changed
> > >     ^^^^^^^^
> > > 
> > > This is really drm, not i915.
> > > 
> > > 
> > > >   drm/i915: Find fallback link rate/lane count
> > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > failure
> > > >
> > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > +++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > >  include/drm/drm_connector.h                   |   7 +-
> > > >  include/drm/drm_crtc.h                        |   5 +
> > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > 
> > > --
> > > Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC

--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Manasi Navare]

On Fri, Nov 11, 2016 at 07:42:16PM +0000, Cheng, Tony wrote:
> In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?
> 
> For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.
>

Hi Tony,

So in case of link training failure, we first call mode_valid that will use the lower link rate/lane count
values based on the link training fallback to validate the modes and prune the modes that are higher than
the available link. After this is done, then we send the uevent to userspace indicating that link status is BAD and 
it will redo a probe and trigger a modeset for next possible lower resolution.

Manasi

 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, November 11, 2016 11:44 AM
> To: Cheng, Tony <Tony.Cheng@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset
> 
> On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> > For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> > 
> > For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).
> 
> The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.
> 
> > 
> > >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> > 
> > windows does not know link state and all link management is hidden behind kernel driver.   
> 
> If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.
> 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, November 11, 2016 9:05 AM
> > To: Cheng, Tony <Tony.Cheng@amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > <jani.nikula@linux.intel.com>; Manasi Navare 
> > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > > 
> > > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> > 
> > I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> > 
> > The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> > 
> > > 
> > > We can also past DP1.2 link layer compliance with this approach.
> > > 
> > > -----Original Message-----
> > > From: Deucher, Alexander
> > > Sent: Thursday, November 10, 2016 1:43 PM
> > > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > > <martin.peres@intel.com>
> > > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > Adding Harry and Tony from our display team to review.
> > > 
> > > > -----Original Message-----
> > > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > > Sent: Thursday, November 10, 2016 1:20 PM
> > > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > > gfx@lists.freedesktop.org
> > > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > > during modeset
> > > > 
> > > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > Link training failure is handled by lowering the link rate first 
> > > > > until it reaches the minimum and keeping the lane count maximum 
> > > > > and then lowering the lane count until it reaches minimim. These 
> > > > > fallback values are saved and hotplug uevent is sent to the 
> > > > > userspace after setting the connector link status property to BAD.
> > > > > Userspace should triiger another modeset on a uevent and if link 
> > > > > status property is BAD. This will retrain the link at fallback values.
> > > > > This is repeated until the link is successfully trained.
> > > > >
> > > > > This has been validated to pass DP compliance.
> > > > 
> > > > This cover letter and the commit messages do a good job of 
> > > > explaining what the patches do. However, you're lacking the 
> > > > crucial information of
> > > > *why* we need userspace cooperation to handle link training 
> > > > failures on DP mode setting, and *why* a new connector property is 
> > > > a good solution for this.
> > > > 
> > > > Here goes, including some alternative approaches we've considered 
> > > > (and even tried):
> > > > 
> > > > At the time userspace does setcrtc, we've already promised the 
> > > > mode would work. The promise is based on the theoretical 
> > > > capabilities of the link, but it's possible we can't reach this in 
> > > > practice. The DP spec describes how the link should be reduced, 
> > > > but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > > 
> > > > One idea would be to have setcrtc return a failure. However, it is 
> > > > my understanding that it already should not fail as the atomic 
> > > > checks have passed [citation needed]. It would also conflict with 
> > > > the idea of making setcrtc asynchronous in the future, returning 
> > > > before the actual mode setting and link training.
> > > > 
> > > > Another idea is to train the link "upfront" at hotplug time, 
> > > > before pruning the mode list, so that we can do the pruning based 
> > > > on practical not theoretical capabilities. However, the changes 
> > > > for link training are pretty drastic, all for the sake of error 
> > > > handling and DP compliance, when the most common happy day 
> > > > scenario is the current approach of link training at mode setting 
> > > > time, using the optimal parameters for the mode. It is also not 
> > > > certain all hardware could do this without the pipe on; not even 
> > > > all our hardware can do this. Some of this can be solved, but not trivially.
> > > > 
> > > > Both of the above ideas also fail to address link degradation
> > > > *during* operation.
> > > > 
> > > > So the idea presented in these patches is to do this in a way that
> > > > a) changes the current happy day scenario as little as possible, 
> > > > to avoid regressions, b) can be implemented the same way by all 
> > > > drm drivers, c) is still opt-in for the drivers and userspace, and 
> > > > opting out doesn't regress the user experience, d) doesn't prevent 
> > > > drivers from implementing better or alternate approaches, possibly 
> > > > without userspace involvement. And, of course, handles all the issues presented.
> > > > 
> > > > The solution is to add a "link status" connector property. In the 
> > > > usual happy day scenario, this is always "good". If something 
> > > > fails during or after a mode set, the kernel driver can set the 
> > > > link status to "bad", prune the mode list based on new information 
> > > > as necessary, and send a hotplug uevent for userspace to have it 
> > > > re-check the valid modes through getconnector, and try again. If 
> > > > the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > > 
> > > > If the userspace is not aware of the property, the user experience 
> > > > is the same as it currently is. If the userspace is aware of the 
> > > > property, it has a chance to improve user experience. If a drm 
> > > > driver does not modify the property (it stays "good"), the user 
> > > > experience is the same as it currently is. A drm driver can also 
> > > > choose to try to handle more of the failures in kernel, hardware 
> > > > not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > > 
> > > > The reason for adding the property is to handle link training 
> > > > failures, but it is not limited to DP or link training. For 
> > > > example, if we implement asynchronous setcrtc, we can use this to 
> > > > report any failures in that.
> > > > 
> > > > Finally, while DP CTS compliance is advertized (which is great, 
> > > > and could be made to work similarly for all drm drivers), this can 
> > > > be used for the more important goal of improving user experience 
> > > > on link training failures, by avoiding black screens.
> > > > 
> > > > 
> > > > BR,
> > > > Jani.
> > > > 
> > > > 
> > > > >
> > > > > Manasi Navare (5):
> > > > >   drm: Add a new connector property for link status
> > > > >   drm/i915: Set link status property for DP connector
> > > > >   drm/i915: Update CRTC state if connector link status property 
> > > > > changed
> > > >     ^^^^^^^^
> > > > 
> > > > This is really drm, not i915.
> > > > 
> > > > 
> > > > >   drm/i915: Find fallback link rate/lane count
> > > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > > failure
> > > > >
> > > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > > +++++++++++++++++++++++++-
> > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > > >  include/drm/drm_connector.h                   |   7 +-
> > > > >  include/drm/drm_crtc.h                        |   5 +
> > > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > > 
> > > > --
> > > > Jani Nikula, Intel Open Source Technology Center
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Daniel Vetter]

On Sun, Nov 13, 2016 at 11:43:01PM -0800, Manasi Navare wrote:
> On Fri, Nov 11, 2016 at 07:42:16PM +0000, Cheng, Tony wrote:
> > In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?
> > 
> > For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.
> >
> 
> Hi Tony,
> 
> So in case of link training failure, we first call mode_valid that will use the lower link rate/lane count
> values based on the link training fallback to validate the modes and prune the modes that are higher than
> the available link. After this is done, then we send the uevent to userspace indicating that link status is BAD and 
> it will redo a probe and trigger a modeset for next possible lower resolution.

Just in case it's not clear: This entire discussion here is only about the
userspace-visible abi changes. And I think for the worst-case we need
something like this (and Harry at least said on irc that on other os you
do something similar already). It of course does not preclude at all that
you do additional link-training/probe on initial hotplug. That part is
entirely up to drivers (and we might get there on some platforms, but on
others it's a real pain to get a link up&running unfortunately for
training, since we need to steal a crtc and do a full modeset).
-Daniel

> 
> Manasi
> 
>  
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> > Sent: Friday, November 11, 2016 11:44 AM
> > To: Cheng, Tony <Tony.Cheng@amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset
> > 
> > On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> > > For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> > > 
> > > For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).
> > 
> > The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.
> > 
> > > 
> > > >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> > > 
> > > windows does not know link state and all link management is hidden behind kernel driver.   
> > 
> > If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.
> > 
> > > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, November 11, 2016 9:05 AM
> > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > > > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > > > 
> > > > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> > > 
> > > I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> > > 
> > > The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> > > 
> > > > 
> > > > We can also past DP1.2 link layer compliance with this approach.
> > > > 
> > > > -----Original Message-----
> > > > From: Deucher, Alexander
> > > > Sent: Thursday, November 10, 2016 1:43 PM
> > > > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > > > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > > > <martin.peres@intel.com>
> > > > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > > during modeset
> > > > 
> > > > Adding Harry and Tony from our display team to review.
> > > > 
> > > > > -----Original Message-----
> > > > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > > > Sent: Thursday, November 10, 2016 1:20 PM
> > > > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > > > gfx@lists.freedesktop.org
> > > > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > > > during modeset
> > > > > 
> > > > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > > Link training failure is handled by lowering the link rate first 
> > > > > > until it reaches the minimum and keeping the lane count maximum 
> > > > > > and then lowering the lane count until it reaches minimim. These 
> > > > > > fallback values are saved and hotplug uevent is sent to the 
> > > > > > userspace after setting the connector link status property to BAD.
> > > > > > Userspace should triiger another modeset on a uevent and if link 
> > > > > > status property is BAD. This will retrain the link at fallback values.
> > > > > > This is repeated until the link is successfully trained.
> > > > > >
> > > > > > This has been validated to pass DP compliance.
> > > > > 
> > > > > This cover letter and the commit messages do a good job of 
> > > > > explaining what the patches do. However, you're lacking the 
> > > > > crucial information of
> > > > > *why* we need userspace cooperation to handle link training 
> > > > > failures on DP mode setting, and *why* a new connector property is 
> > > > > a good solution for this.
> > > > > 
> > > > > Here goes, including some alternative approaches we've considered 
> > > > > (and even tried):
> > > > > 
> > > > > At the time userspace does setcrtc, we've already promised the 
> > > > > mode would work. The promise is based on the theoretical 
> > > > > capabilities of the link, but it's possible we can't reach this in 
> > > > > practice. The DP spec describes how the link should be reduced, 
> > > > > but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > > > 
> > > > > One idea would be to have setcrtc return a failure. However, it is 
> > > > > my understanding that it already should not fail as the atomic 
> > > > > checks have passed [citation needed]. It would also conflict with 
> > > > > the idea of making setcrtc asynchronous in the future, returning 
> > > > > before the actual mode setting and link training.
> > > > > 
> > > > > Another idea is to train the link "upfront" at hotplug time, 
> > > > > before pruning the mode list, so that we can do the pruning based 
> > > > > on practical not theoretical capabilities. However, the changes 
> > > > > for link training are pretty drastic, all for the sake of error 
> > > > > handling and DP compliance, when the most common happy day 
> > > > > scenario is the current approach of link training at mode setting 
> > > > > time, using the optimal parameters for the mode. It is also not 
> > > > > certain all hardware could do this without the pipe on; not even 
> > > > > all our hardware can do this. Some of this can be solved, but not trivially.
> > > > > 
> > > > > Both of the above ideas also fail to address link degradation
> > > > > *during* operation.
> > > > > 
> > > > > So the idea presented in these patches is to do this in a way that
> > > > > a) changes the current happy day scenario as little as possible, 
> > > > > to avoid regressions, b) can be implemented the same way by all 
> > > > > drm drivers, c) is still opt-in for the drivers and userspace, and 
> > > > > opting out doesn't regress the user experience, d) doesn't prevent 
> > > > > drivers from implementing better or alternate approaches, possibly 
> > > > > without userspace involvement. And, of course, handles all the issues presented.
> > > > > 
> > > > > The solution is to add a "link status" connector property. In the 
> > > > > usual happy day scenario, this is always "good". If something 
> > > > > fails during or after a mode set, the kernel driver can set the 
> > > > > link status to "bad", prune the mode list based on new information 
> > > > > as necessary, and send a hotplug uevent for userspace to have it 
> > > > > re-check the valid modes through getconnector, and try again. If 
> > > > > the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > > > 
> > > > > If the userspace is not aware of the property, the user experience 
> > > > > is the same as it currently is. If the userspace is aware of the 
> > > > > property, it has a chance to improve user experience. If a drm 
> > > > > driver does not modify the property (it stays "good"), the user 
> > > > > experience is the same as it currently is. A drm driver can also 
> > > > > choose to try to handle more of the failures in kernel, hardware 
> > > > > not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > > > 
> > > > > The reason for adding the property is to handle link training 
> > > > > failures, but it is not limited to DP or link training. For 
> > > > > example, if we implement asynchronous setcrtc, we can use this to 
> > > > > report any failures in that.
> > > > > 
> > > > > Finally, while DP CTS compliance is advertized (which is great, 
> > > > > and could be made to work similarly for all drm drivers), this can 
> > > > > be used for the more important goal of improving user experience 
> > > > > on link training failures, by avoiding black screens.
> > > > > 
> > > > > 
> > > > > BR,
> > > > > Jani.
> > > > > 
> > > > > 
> > > > > >
> > > > > > Manasi Navare (5):
> > > > > >   drm: Add a new connector property for link status
> > > > > >   drm/i915: Set link status property for DP connector
> > > > > >   drm/i915: Update CRTC state if connector link status property 
> > > > > > changed
> > > > >     ^^^^^^^^
> > > > > 
> > > > > This is really drm, not i915.
> > > > > 
> > > > > 
> > > > > >   drm/i915: Find fallback link rate/lane count
> > > > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > > > failure
> > > > > >
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > > > +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > > > >  include/drm/drm_connector.h                   |   7 +-
> > > > > >  include/drm/drm_crtc.h                        |   5 +
> > > > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > > > 
> > > > > --
> > > > > Jani Nikula, Intel Open Source Technology Center
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Cheng, Tony]

I see.  As long as amd can still just have kernel hiding unsupported mode by doing a pre-train I am okay with the proposal.  Just to caution you the link training fallback we implemented on windows in old dal architecture is very painful to get right.  Windows 7, 8.1 and 10 all behave differently.  Not all apps handles mode change failure gracefully and worse case we get stuck in infinite mode set.  This is why for future generations asic on windows we are also going to just hide unsupported mode by doing pre-train.

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, November 14, 2016 3:04 AM
To: Manasi Navare <manasi.d.navare@intel.com>
Cc: Cheng, Tony <Tony.Cheng@amd.com>; intel-gfx@lists.freedesktop.org; Peres, Martin <martin.peres@intel.com>; dri-devel@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

On Sun, Nov 13, 2016 at 11:43:01PM -0800, Manasi Navare wrote:
> On Fri, Nov 11, 2016 at 07:42:16PM +0000, Cheng, Tony wrote:
> > In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?
> > 
> > For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.
> >
> 
> Hi Tony,
> 
> So in case of link training failure, we first call mode_valid that 
> will use the lower link rate/lane count values based on the link 
> training fallback to validate the modes and prune the modes that are 
> higher than the available link. After this is done, then we send the uevent to userspace indicating that link status is BAD and it will redo a probe and trigger a modeset for next possible lower resolution.

Just in case it's not clear: This entire discussion here is only about the userspace-visible abi changes. And I think for the worst-case we need something like this (and Harry at least said on irc that on other os you do something similar already). It of course does not preclude at all that you do additional link-training/probe on initial hotplug. That part is entirely up to drivers (and we might get there on some platforms, but on others it's a real pain to get a link up&running unfortunately for training, since we need to steal a crtc and do a full modeset).
-Daniel

> 
> Manasi
> 
>  
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, November 11, 2016 11:44 AM
> > To: Cheng, Tony <Tony.Cheng@amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > <jani.nikula@linux.intel.com>; Manasi Navare 
> > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> > > For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> > > 
> > > For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).
> > 
> > The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.
> > 
> > > 
> > > >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> > > 
> > > windows does not know link state and all link management is hidden behind kernel driver.   
> > 
> > If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.
> > 
> > > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, November 11, 2016 9:05 AM
> > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > > > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > > > 
> > > > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> > > 
> > > I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> > > 
> > > The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> > > 
> > > > 
> > > > We can also past DP1.2 link layer compliance with this approach.
> > > > 
> > > > -----Original Message-----
> > > > From: Deucher, Alexander
> > > > Sent: Thursday, November 10, 2016 1:43 PM
> > > > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > > > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > > > <martin.peres@intel.com>
> > > > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > Failure during modeset
> > > > 
> > > > Adding Harry and Tony from our display team to review.
> > > > 
> > > > > -----Original Message-----
> > > > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > > > Sent: Thursday, November 10, 2016 1:20 PM
> > > > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > > > gfx@lists.freedesktop.org
> > > > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > > Failure during modeset
> > > > > 
> > > > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > > Link training failure is handled by lowering the link rate 
> > > > > > first until it reaches the minimum and keeping the lane 
> > > > > > count maximum and then lowering the lane count until it 
> > > > > > reaches minimim. These fallback values are saved and hotplug 
> > > > > > uevent is sent to the userspace after setting the connector link status property to BAD.
> > > > > > Userspace should triiger another modeset on a uevent and if 
> > > > > > link status property is BAD. This will retrain the link at fallback values.
> > > > > > This is repeated until the link is successfully trained.
> > > > > >
> > > > > > This has been validated to pass DP compliance.
> > > > > 
> > > > > This cover letter and the commit messages do a good job of 
> > > > > explaining what the patches do. However, you're lacking the 
> > > > > crucial information of
> > > > > *why* we need userspace cooperation to handle link training 
> > > > > failures on DP mode setting, and *why* a new connector 
> > > > > property is a good solution for this.
> > > > > 
> > > > > Here goes, including some alternative approaches we've 
> > > > > considered (and even tried):
> > > > > 
> > > > > At the time userspace does setcrtc, we've already promised the 
> > > > > mode would work. The promise is based on the theoretical 
> > > > > capabilities of the link, but it's possible we can't reach 
> > > > > this in practice. The DP spec describes how the link should be 
> > > > > reduced, but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > > > 
> > > > > One idea would be to have setcrtc return a failure. However, 
> > > > > it is my understanding that it already should not fail as the 
> > > > > atomic checks have passed [citation needed]. It would also 
> > > > > conflict with the idea of making setcrtc asynchronous in the 
> > > > > future, returning before the actual mode setting and link training.
> > > > > 
> > > > > Another idea is to train the link "upfront" at hotplug time, 
> > > > > before pruning the mode list, so that we can do the pruning 
> > > > > based on practical not theoretical capabilities. However, the 
> > > > > changes for link training are pretty drastic, all for the sake 
> > > > > of error handling and DP compliance, when the most common 
> > > > > happy day scenario is the current approach of link training at 
> > > > > mode setting time, using the optimal parameters for the mode. 
> > > > > It is also not certain all hardware could do this without the 
> > > > > pipe on; not even all our hardware can do this. Some of this can be solved, but not trivially.
> > > > > 
> > > > > Both of the above ideas also fail to address link degradation
> > > > > *during* operation.
> > > > > 
> > > > > So the idea presented in these patches is to do this in a way 
> > > > > that
> > > > > a) changes the current happy day scenario as little as 
> > > > > possible, to avoid regressions, b) can be implemented the same 
> > > > > way by all drm drivers, c) is still opt-in for the drivers and 
> > > > > userspace, and opting out doesn't regress the user experience, 
> > > > > d) doesn't prevent drivers from implementing better or 
> > > > > alternate approaches, possibly without userspace involvement. And, of course, handles all the issues presented.
> > > > > 
> > > > > The solution is to add a "link status" connector property. In 
> > > > > the usual happy day scenario, this is always "good". If 
> > > > > something fails during or after a mode set, the kernel driver 
> > > > > can set the link status to "bad", prune the mode list based on 
> > > > > new information as necessary, and send a hotplug uevent for 
> > > > > userspace to have it re-check the valid modes through 
> > > > > getconnector, and try again. If the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > > > 
> > > > > If the userspace is not aware of the property, the user 
> > > > > experience is the same as it currently is. If the userspace is 
> > > > > aware of the property, it has a chance to improve user 
> > > > > experience. If a drm driver does not modify the property (it 
> > > > > stays "good"), the user experience is the same as it currently 
> > > > > is. A drm driver can also choose to try to handle more of the 
> > > > > failures in kernel, hardware not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > > > 
> > > > > The reason for adding the property is to handle link training 
> > > > > failures, but it is not limited to DP or link training. For 
> > > > > example, if we implement asynchronous setcrtc, we can use this 
> > > > > to report any failures in that.
> > > > > 
> > > > > Finally, while DP CTS compliance is advertized (which is 
> > > > > great, and could be made to work similarly for all drm 
> > > > > drivers), this can be used for the more important goal of 
> > > > > improving user experience on link training failures, by avoiding black screens.
> > > > > 
> > > > > 
> > > > > BR,
> > > > > Jani.
> > > > > 
> > > > > 
> > > > > >
> > > > > > Manasi Navare (5):
> > > > > >   drm: Add a new connector property for link status
> > > > > >   drm/i915: Set link status property for DP connector
> > > > > >   drm/i915: Update CRTC state if connector link status 
> > > > > > property changed
> > > > >     ^^^^^^^^
> > > > > 
> > > > > This is really drm, not i915.
> > > > > 
> > > > > 
> > > > > >   drm/i915: Find fallback link rate/lane count
> > > > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > > > failure
> > > > > >
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > > > +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > > > >  include/drm/drm_connector.h                   |   7 +-
> > > > > >  include/drm/drm_crtc.h                        |   5 +
> > > > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > > > 
> > > > > --
> > > > > Jani Nikula, Intel Open Source Technology Center
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

[Manasi Navare]

Yes driver can chose to have the pre train for kernel hiding unsupported mode, that is completely still possible for the amd driver and it would never use the link_status connector property and no interaction with userspace.
Could you please give your ACKs for the patches if you are okay with this porposal?

Regards
Manasi



On Mon, Nov 14, 2016 at 02:45:55PM +0000, Cheng, Tony wrote:
> I see.  As long as amd can still just have kernel hiding unsupported mode by doing a pre-train I am okay with the proposal.  Just to caution you the link training fallback we implemented on windows in old dal architecture is very painful to get right.  Windows 7, 8.1 and 10 all behave differently.  Not all apps handles mode change failure gracefully and worse case we get stuck in infinite mode set.  This is why for future generations asic on windows we are also going to just hide unsupported mode by doing pre-train.
> 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, November 14, 2016 3:04 AM
> To: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Cheng, Tony <Tony.Cheng@amd.com>; intel-gfx@lists.freedesktop.org; Peres, Martin <martin.peres@intel.com>; dri-devel@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset
> 
> On Sun, Nov 13, 2016 at 11:43:01PM -0800, Manasi Navare wrote:
> > On Fri, Nov 11, 2016 at 07:42:16PM +0000, Cheng, Tony wrote:
> > > In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?
> > > 
> > > For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.
> > >
> > 
> > Hi Tony,
> > 
> > So in case of link training failure, we first call mode_valid that 
> > will use the lower link rate/lane count values based on the link 
> > training fallback to validate the modes and prune the modes that are 
> > higher than the available link. After this is done, then we send the uevent to userspace indicating that link status is BAD and it will redo a probe and trigger a modeset for next possible lower resolution.
> 
> Just in case it's not clear: This entire discussion here is only about the userspace-visible abi changes. And I think for the worst-case we need something like this (and Harry at least said on irc that on other os you do something similar already). It of course does not preclude at all that you do additional link-training/probe on initial hotplug. That part is entirely up to drivers (and we might get there on some platforms, but on others it's a real pain to get a link up&running unfortunately for training, since we need to steal a crtc and do a full modeset).
> -Daniel
> 
> > 
> > Manasi
> > 
> >  
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, November 11, 2016 11:44 AM
> > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> > > > For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> > > > 
> > > > For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).
> > > 
> > > The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.
> > > 
> > > > 
> > > > >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> > > > 
> > > > windows does not know link state and all link management is hidden behind kernel driver.   
> > > 
> > > If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Friday, November 11, 2016 9:05 AM
> > > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > > during modeset
> > > > 
> > > > On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > > > > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > > > > 
> > > > > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> > > > 
> > > > I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> > > > 
> > > > The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> > > > 
> > > > > 
> > > > > We can also past DP1.2 link layer compliance with this approach.
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Deucher, Alexander
> > > > > Sent: Thursday, November 10, 2016 1:43 PM
> > > > > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > > > > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > > > > <martin.peres@intel.com>
> > > > > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > > Failure during modeset
> > > > > 
> > > > > Adding Harry and Tony from our display team to review.
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > > > > Sent: Thursday, November 10, 2016 1:20 PM
> > > > > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > > > > gfx@lists.freedesktop.org
> > > > > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > > > Failure during modeset
> > > > > > 
> > > > > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > > > Link training failure is handled by lowering the link rate 
> > > > > > > first until it reaches the minimum and keeping the lane 
> > > > > > > count maximum and then lowering the lane count until it 
> > > > > > > reaches minimim. These fallback values are saved and hotplug 
> > > > > > > uevent is sent to the userspace after setting the connector link status property to BAD.
> > > > > > > Userspace should triiger another modeset on a uevent and if 
> > > > > > > link status property is BAD. This will retrain the link at fallback values.
> > > > > > > This is repeated until the link is successfully trained.
> > > > > > >
> > > > > > > This has been validated to pass DP compliance.
> > > > > > 
> > > > > > This cover letter and the commit messages do a good job of 
> > > > > > explaining what the patches do. However, you're lacking the 
> > > > > > crucial information of
> > > > > > *why* we need userspace cooperation to handle link training 
> > > > > > failures on DP mode setting, and *why* a new connector 
> > > > > > property is a good solution for this.
> > > > > > 
> > > > > > Here goes, including some alternative approaches we've 
> > > > > > considered (and even tried):
> > > > > > 
> > > > > > At the time userspace does setcrtc, we've already promised the 
> > > > > > mode would work. The promise is based on the theoretical 
> > > > > > capabilities of the link, but it's possible we can't reach 
> > > > > > this in practice. The DP spec describes how the link should be 
> > > > > > reduced, but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > > > > 
> > > > > > One idea would be to have setcrtc return a failure. However, 
> > > > > > it is my understanding that it already should not fail as the 
> > > > > > atomic checks have passed [citation needed]. It would also 
> > > > > > conflict with the idea of making setcrtc asynchronous in the 
> > > > > > future, returning before the actual mode setting and link training.
> > > > > > 
> > > > > > Another idea is to train the link "upfront" at hotplug time, 
> > > > > > before pruning the mode list, so that we can do the pruning 
> > > > > > based on practical not theoretical capabilities. However, the 
> > > > > > changes for link training are pretty drastic, all for the sake 
> > > > > > of error handling and DP compliance, when the most common 
> > > > > > happy day scenario is the current approach of link training at 
> > > > > > mode setting time, using the optimal parameters for the mode. 
> > > > > > It is also not certain all hardware could do this without the 
> > > > > > pipe on; not even all our hardware can do this. Some of this can be solved, but not trivially.
> > > > > > 
> > > > > > Both of the above ideas also fail to address link degradation
> > > > > > *during* operation.
> > > > > > 
> > > > > > So the idea presented in these patches is to do this in a way 
> > > > > > that
> > > > > > a) changes the current happy day scenario as little as 
> > > > > > possible, to avoid regressions, b) can be implemented the same 
> > > > > > way by all drm drivers, c) is still opt-in for the drivers and 
> > > > > > userspace, and opting out doesn't regress the user experience, 
> > > > > > d) doesn't prevent drivers from implementing better or 
> > > > > > alternate approaches, possibly without userspace involvement. And, of course, handles all the issues presented.
> > > > > > 
> > > > > > The solution is to add a "link status" connector property. In 
> > > > > > the usual happy day scenario, this is always "good". If 
> > > > > > something fails during or after a mode set, the kernel driver 
> > > > > > can set the link status to "bad", prune the mode list based on 
> > > > > > new information as necessary, and send a hotplug uevent for 
> > > > > > userspace to have it re-check the valid modes through 
> > > > > > getconnector, and try again. If the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > > > > 
> > > > > > If the userspace is not aware of the property, the user 
> > > > > > experience is the same as it currently is. If the userspace is 
> > > > > > aware of the property, it has a chance to improve user 
> > > > > > experience. If a drm driver does not modify the property (it 
> > > > > > stays "good"), the user experience is the same as it currently 
> > > > > > is. A drm driver can also choose to try to handle more of the 
> > > > > > failures in kernel, hardware not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > > > > 
> > > > > > The reason for adding the property is to handle link training 
> > > > > > failures, but it is not limited to DP or link training. For 
> > > > > > example, if we implement asynchronous setcrtc, we can use this 
> > > > > > to report any failures in that.
> > > > > > 
> > > > > > Finally, while DP CTS compliance is advertized (which is 
> > > > > > great, and could be made to work similarly for all drm 
> > > > > > drivers), this can be used for the more important goal of 
> > > > > > improving user experience on link training failures, by avoiding black screens.
> > > > > > 
> > > > > > 
> > > > > > BR,
> > > > > > Jani.
> > > > > > 
> > > > > > 
> > > > > > >
> > > > > > > Manasi Navare (5):
> > > > > > >   drm: Add a new connector property for link status
> > > > > > >   drm/i915: Set link status property for DP connector
> > > > > > >   drm/i915: Update CRTC state if connector link status 
> > > > > > > property changed
> > > > > >     ^^^^^^^^
> > > > > > 
> > > > > > This is really drm, not i915.
> > > > > > 
> > > > > > 
> > > > > > >   drm/i915: Find fallback link rate/lane count
> > > > > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > > > > failure
> > > > > > >
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > > > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > > > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > > > > +++++++++++++++++++++++++-
> > > > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > > > > >  include/drm/drm_connector.h                   |   7 +-
> > > > > > >  include/drm/drm_crtc.h                        |   5 +
> > > > > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > > > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > --
> > > > > > Jani Nikula, Intel Open Source Technology Center
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Cheng, Tony]

Acked-by: Tony Cheng <tony.cheng@amd.com>

-----Original Message-----
From: Manasi Navare [mailto:manasi.d.navare@intel.com] 
Sent: Monday, November 14, 2016 12:52 PM
To: Cheng, Tony <Tony.Cheng@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; Peres, Martin <martin.peres@intel.com>; dri-devel@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

Yes driver can chose to have the pre train for kernel hiding unsupported mode, that is completely still possible for the amd driver and it would never use the link_status connector property and no interaction with userspace.
Could you please give your ACKs for the patches if you are okay with this porposal?

Regards
Manasi



On Mon, Nov 14, 2016 at 02:45:55PM +0000, Cheng, Tony wrote:
> I see.  As long as amd can still just have kernel hiding unsupported mode by doing a pre-train I am okay with the proposal.  Just to caution you the link training fallback we implemented on windows in old dal architecture is very painful to get right.  Windows 7, 8.1 and 10 all behave differently.  Not all apps handles mode change failure gracefully and worse case we get stuck in infinite mode set.  This is why for future generations asic on windows we are also going to just hide unsupported mode by doing pre-train.
> 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Monday, November 14, 2016 3:04 AM
> To: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Cheng, Tony <Tony.Cheng@amd.com>; intel-gfx@lists.freedesktop.org; 
> Peres, Martin <martin.peres@intel.com>; 
> dri-devel@lists.freedesktop.org; Deucher, Alexander 
> <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> during modeset
> 
> On Sun, Nov 13, 2016 at 11:43:01PM -0800, Manasi Navare wrote:
> > On Fri, Nov 11, 2016 at 07:42:16PM +0000, Cheng, Tony wrote:
> > > In case of link training failure and requiring user space to set a lower mode, would full mode set address it?  How do we make user mode select lower resolution?
> > > 
> > > For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing user mode from seeing 4K@60Hz from the start is a easier and more robust solution and requiring user mode to have logic to decide how to fallback.
> > >
> > 
> > Hi Tony,
> > 
> > So in case of link training failure, we first call mode_valid that 
> > will use the lower link rate/lane count values based on the link 
> > training fallback to validate the modes and prune the modes that are 
> > higher than the available link. After this is done, then we send the uevent to userspace indicating that link status is BAD and it will redo a probe and trigger a modeset for next possible lower resolution.
> 
> Just in case it's not clear: This entire discussion here is only about the userspace-visible abi changes. And I think for the worst-case we need something like this (and Harry at least said on irc that on other os you do something similar already). It of course does not preclude at all that you do additional link-training/probe on initial hotplug. That part is entirely up to drivers (and we might get there on some platforms, but on others it's a real pain to get a link up&running unfortunately for training, since we need to steal a crtc and do a full modeset).
> -Daniel
> 
> > 
> > Manasi
> > 
> >  
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, November 11, 2016 11:44 AM
> > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > > during modeset
> > > 
> > > On Fri, Nov 11, 2016 at 04:21:58PM +0000, Cheng, Tony wrote:
> > > > For HDMI, you can yank the cable, plug back in, HDMI will light up without user mode or kernel mode doing anything.
> > > > 
> > > > For DP this is not possible, someone will have to retrain the link when plugging back in or DP will not light up.  We see that on Ubuntu if someone unplug display and plug it back into same connector, we do not get KMS so in this case.  It appears there is some optimizations somewhere in user mode stack which I am not familiar with yet.  dal added workaround to retrain the link to light it back up (after the test training at end of detection).
> > > 
> > > The link should get retrained as the driver should check the link state on HPD and retrain if it's not good. At least that's what we do in i915. Of course if it's not the same display that got plugged back, the retraining might fail. The new property can help with that case as userspace would then attempt a full modeset after the failed link training.
> > > 
> > > > 
> > > > >From user mode perspective I think it make sense to keep CRTC running, so vblank is still going so UMD isn't impacted.   As regard to connector and encoder does it matter if kernel mode change state without user mode knowing?  It seems to me those are more informational to UMD as UMD doesn't act on them.
> > > > 
> > > > windows does not know link state and all link management is hidden behind kernel driver.   
> > > 
> > > If the user visible state doesn't change in any way, then the kernel could try to manage it all internally.
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Friday, November 11, 2016 9:05 AM
> > > > To: Cheng, Tony <Tony.Cheng@amd.com>
> > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; 'Jani Nikula' 
> > > > <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > <Harry.Wentland@amd.com>; Peres, Martin <martin.peres@intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > Failure during modeset
> > > > 
> > > > On Thu, Nov 10, 2016 at 06:51:40PM +0000, Cheng, Tony wrote:
> > > > > Amdgpu dal implementation will do a test link training at end of detection to verify we can achieve the capability reported in DPCD.  We then report mode base on result of test training.
> > > > > 
> > > > > AMD hardware (at least the generations supported by amdgpu) is able to link train without timing being setup (DP encoder and CRTC is decoupled).  Do we have limitation from other vendors where you need timing to be there before you can link train?
> > > > 
> > > > I can't recall the specifics for all of our supported platforms, but at least I have the recollection that it would be the case yes.
> > > > 
> > > > The other problem wiyh this apporach is that even if you don't need the crtc, you still need the link itself. What happens if the link is still active since userspace just didn't bother to shut it down when the cable was yanked? Can you keep the crtc going but stop it from feeding the link in a way that userspace won't be able to notice? The kms design has always been pretty much that policy is in userspace, and thus the kernel shouldn't shut down crtcs unless explicitly asked to do so.
> > > > 
> > > > > 
> > > > > We can also past DP1.2 link layer compliance with this approach.
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Deucher, Alexander
> > > > > Sent: Thursday, November 10, 2016 1:43 PM
> > > > > To: 'Jani Nikula' <jani.nikula@linux.intel.com>; Manasi Navare 
> > > > > <manasi.d.navare@intel.com>; dri-devel@lists.freedesktop.org; 
> > > > > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > > > > <Harry.Wentland@amd.com>; Cheng, Tony <Tony.Cheng@amd.com>
> > > > > Cc: Dave Airlie <airlied@gmail.com>; Peres, Martin 
> > > > > <martin.peres@intel.com>
> > > > > Subject: RE: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > > Failure during modeset
> > > > > 
> > > > > Adding Harry and Tony from our display team to review.
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > > > > Sent: Thursday, November 10, 2016 1:20 PM
> > > > > > To: Manasi Navare; dri-devel@lists.freedesktop.org; intel- 
> > > > > > gfx@lists.freedesktop.org
> > > > > > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> > > > > > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training 
> > > > > > Failure during modeset
> > > > > > 
> > > > > > On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > > > Link training failure is handled by lowering the link rate 
> > > > > > > first until it reaches the minimum and keeping the lane 
> > > > > > > count maximum and then lowering the lane count until it 
> > > > > > > reaches minimim. These fallback values are saved and 
> > > > > > > hotplug uevent is sent to the userspace after setting the connector link status property to BAD.
> > > > > > > Userspace should triiger another modeset on a uevent and 
> > > > > > > if link status property is BAD. This will retrain the link at fallback values.
> > > > > > > This is repeated until the link is successfully trained.
> > > > > > >
> > > > > > > This has been validated to pass DP compliance.
> > > > > > 
> > > > > > This cover letter and the commit messages do a good job of 
> > > > > > explaining what the patches do. However, you're lacking the 
> > > > > > crucial information of
> > > > > > *why* we need userspace cooperation to handle link training 
> > > > > > failures on DP mode setting, and *why* a new connector 
> > > > > > property is a good solution for this.
> > > > > > 
> > > > > > Here goes, including some alternative approaches we've 
> > > > > > considered (and even tried):
> > > > > > 
> > > > > > At the time userspace does setcrtc, we've already promised 
> > > > > > the mode would work. The promise is based on the theoretical 
> > > > > > capabilities of the link, but it's possible we can't reach 
> > > > > > this in practice. The DP spec describes how the link should 
> > > > > > be reduced, but we can't reduce the link below the requirements of the mode. Black screen follows.
> > > > > > 
> > > > > > One idea would be to have setcrtc return a failure. However, 
> > > > > > it is my understanding that it already should not fail as 
> > > > > > the atomic checks have passed [citation needed]. It would 
> > > > > > also conflict with the idea of making setcrtc asynchronous 
> > > > > > in the future, returning before the actual mode setting and link training.
> > > > > > 
> > > > > > Another idea is to train the link "upfront" at hotplug time, 
> > > > > > before pruning the mode list, so that we can do the pruning 
> > > > > > based on practical not theoretical capabilities. However, 
> > > > > > the changes for link training are pretty drastic, all for 
> > > > > > the sake of error handling and DP compliance, when the most 
> > > > > > common happy day scenario is the current approach of link 
> > > > > > training at mode setting time, using the optimal parameters for the mode.
> > > > > > It is also not certain all hardware could do this without 
> > > > > > the pipe on; not even all our hardware can do this. Some of this can be solved, but not trivially.
> > > > > > 
> > > > > > Both of the above ideas also fail to address link 
> > > > > > degradation
> > > > > > *during* operation.
> > > > > > 
> > > > > > So the idea presented in these patches is to do this in a 
> > > > > > way that
> > > > > > a) changes the current happy day scenario as little as 
> > > > > > possible, to avoid regressions, b) can be implemented the 
> > > > > > same way by all drm drivers, c) is still opt-in for the 
> > > > > > drivers and userspace, and opting out doesn't regress the 
> > > > > > user experience,
> > > > > > d) doesn't prevent drivers from implementing better or 
> > > > > > alternate approaches, possibly without userspace involvement. And, of course, handles all the issues presented.
> > > > > > 
> > > > > > The solution is to add a "link status" connector property. 
> > > > > > In the usual happy day scenario, this is always "good". If 
> > > > > > something fails during or after a mode set, the kernel 
> > > > > > driver can set the link status to "bad", prune the mode list 
> > > > > > based on new information as necessary, and send a hotplug 
> > > > > > uevent for userspace to have it re-check the valid modes 
> > > > > > through getconnector, and try again. If the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
> > > > > > 
> > > > > > If the userspace is not aware of the property, the user 
> > > > > > experience is the same as it currently is. If the userspace 
> > > > > > is aware of the property, it has a chance to improve user 
> > > > > > experience. If a drm driver does not modify the property (it 
> > > > > > stays "good"), the user experience is the same as it 
> > > > > > currently is. A drm driver can also choose to try to handle 
> > > > > > more of the failures in kernel, hardware not limiting, or it can choose to involve userspace more. Up to the drivers.
> > > > > > 
> > > > > > The reason for adding the property is to handle link 
> > > > > > training failures, but it is not limited to DP or link 
> > > > > > training. For example, if we implement asynchronous setcrtc, 
> > > > > > we can use this to report any failures in that.
> > > > > > 
> > > > > > Finally, while DP CTS compliance is advertized (which is 
> > > > > > great, and could be made to work similarly for all drm 
> > > > > > drivers), this can be used for the more important goal of 
> > > > > > improving user experience on link training failures, by avoiding black screens.
> > > > > > 
> > > > > > 
> > > > > > BR,
> > > > > > Jani.
> > > > > > 
> > > > > > 
> > > > > > >
> > > > > > > Manasi Navare (5):
> > > > > > >   drm: Add a new connector property for link status
> > > > > > >   drm/i915: Set link status property for DP connector
> > > > > > >   drm/i915: Update CRTC state if connector link status 
> > > > > > > property changed
> > > > > >     ^^^^^^^^
> > > > > > 
> > > > > > This is really drm, not i915.
> > > > > > 
> > > > > > 
> > > > > > >   drm/i915: Find fallback link rate/lane count
> > > > > > >   drm/i915: Implement Link Rate fallback on Link training 
> > > > > > > failure
> > > > > > >
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> > > > > > >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> > > > > > >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c               | 138
> > > > > > +++++++++++++++++++++++++-
> > > > > > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> > > > > > >  include/drm/drm_connector.h                   |   7 +-
> > > > > > >  include/drm/drm_crtc.h                        |   5 +
> > > > > > >  include/uapi/drm/drm_mode.h                   |   4 +
> > > > > > >  9 files changed, 214 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > --
> > > > > > Jani Nikula, Intel Open Source Technology Center
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org 
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Harry Wentland]

Overall this patch set makes sense but I think it would be good to move 
some stuff to common code instead of leaving it in i915. I'm thinking 
about patch 2 (setting the link status property) in particular but also 
tend to agree with Ville and Daniel about their comments for patch 5.

That said, should another driver need it it shouldn't be hard to pull 
that out.

Patch 1: Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Patch 2-5: Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry


On 2016-11-09 11:42 PM, Manasi Navare wrote:
> Link training failure is handled by lowering the link rate first
> until it reaches the minimum and keeping the lane count maximum
> and then lowering the lane count until it reaches minimim. These
> fallback values are saved and hotplug uevent is sent to the userspace
> after setting the connector link status property to BAD. Userspace
> should triiger another modeset on a uevent and if link status property
> is BAD. This will retrain the link at fallback values.
> This is repeated until the link is successfully trained.
>
> This has been validated to pass DP compliance.
>
> Manasi Navare (5):
>    drm: Add a new connector property for link status
>    drm/i915: Set link status property for DP connector
>    drm/i915: Update CRTC state if connector link status property changed
>    drm/i915: Find fallback link rate/lane count
>    drm/i915: Implement Link Rate fallback on Link training failure
>
>   drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
>   drivers/gpu/drm/drm_connector.c               |  17 ++++
>   drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
>   drivers/gpu/drm/i915/intel_dp.c               | 138 +++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
>   drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
>   include/drm/drm_connector.h                   |   7 +-
>   include/drm/drm_crtc.h                        |   5 +
>   include/uapi/drm/drm_mode.h                   |   4 +
>   9 files changed, 214 insertions(+), 9 deletions(-)
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] Handle Link Training Failure during modeset

[Manasi Navare]

On Mon, Nov 14, 2016 at 08:07:39PM -0500, Harry Wentland wrote:
> Overall this patch set makes sense but I think it would be good to
> move some stuff to common code instead of leaving it in i915. I'm
> thinking about patch 2 (setting the link status property) in
> particular but also tend to agree with Ville and Daniel about their
> comments for patch 5.
> 
> That said, should another driver need it it shouldn't be hard to
> pull that out.
> 
> Patch 1: Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> Patch 2-5: Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> Harry
> 
>

Thanks for the review.
I am already moving Patch 2 set_link_status to the drm core as
per Daniel and Ville's suggestion. I will make those changes
and submit the new patches by EOD today.

Regards
Manasi
 
> On 2016-11-09 11:42 PM, Manasi Navare wrote:
> >Link training failure is handled by lowering the link rate first
> >until it reaches the minimum and keeping the lane count maximum
> >and then lowering the lane count until it reaches minimim. These
> >fallback values are saved and hotplug uevent is sent to the userspace
> >after setting the connector link status property to BAD. Userspace
> >should triiger another modeset on a uevent and if link status property
> >is BAD. This will retrain the link at fallback values.
> >This is repeated until the link is successfully trained.
> >
> >This has been validated to pass DP compliance.
> >
> >Manasi Navare (5):
> >   drm: Add a new connector property for link status
> >   drm/i915: Set link status property for DP connector
> >   drm/i915: Update CRTC state if connector link status property changed
> >   drm/i915: Find fallback link rate/lane count
> >   drm/i915: Implement Link Rate fallback on Link training failure
> >
> >  drivers/gpu/drm/drm_atomic_helper.c           |   7 ++
> >  drivers/gpu/drm/drm_connector.c               |  17 ++++
> >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++-
> >  drivers/gpu/drm/i915/intel_dp.c               | 138 +++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> >  drivers/gpu/drm/i915/intel_drv.h              |  12 ++-
> >  include/drm/drm_connector.h                   |   7 +-
> >  include/drm/drm_crtc.h                        |   5 +
> >  include/uapi/drm/drm_mode.h                   |   4 +
> >  9 files changed, 214 insertions(+), 9 deletions(-)
> >
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel