diff mbox

[v2] drm/i915/dp: Read link status more times when EQ not done

Message ID 1488443763-26709-1-git-send-email-shawn.c.lee@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lee, Shawn C March 2, 2017, 8:36 a.m. UTC
From: "Lee, Shawn C" <shawn.c.lee@intel.com>

Display driver read DPCD register 0x202, 0x203 and 0x204 to identify
eDP sink status.If PSR exit is ongoing at eDP sink, and eDP source
read these registers at the same time. Panel will report EQ & symbol
lock not done. It will cause panel display flicking.

Try to read link status more times if eDP EQ not done. Panel side
request at least 1000us for fast link train while doing PSR exit.
So wait more than 1000us then retrieve sink's status again.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99639
TEST=Reboot DUT and no flicking on local display at login screen

Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Wei Shun Chen <wei.shun.chang@intel.com>
Cc: Gary C Wang <gary.c.wang@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Jani Nikula March 7, 2017, 8 p.m. UTC | #1
On Thu, 02 Mar 2017, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote:
> From: "Lee, Shawn C" <shawn.c.lee@intel.com>
>
> Display driver read DPCD register 0x202, 0x203 and 0x204 to identify
> eDP sink status.If PSR exit is ongoing at eDP sink, and eDP source
> read these registers at the same time. Panel will report EQ & symbol
> lock not done. It will cause panel display flicking.
>
> Try to read link status more times if eDP EQ not done. Panel side
> request at least 1000us for fast link train while doing PSR exit.
> So wait more than 1000us then retrieve sink's status again.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99639
> TEST=Reboot DUT and no flicking on local display at login screen
>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Wei Shun Chen <wei.shun.chang@intel.com>
> Cc: Gary C Wang <gary.c.wang@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
>
> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 024798a9c016..f6229d616971 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4225,6 +4225,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  {
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> @@ -4247,6 +4248,27 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  
>  	/* Retrain if Channel EQ or CR not ok */
>  	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> +		if (is_edp(intel_dp) && dev_priv->psr.enabled) {
> +			u8 retry;
> +
> +			for (retry = 0; retry < 3; retry++) {
> +				/*
> +				 * EQ not ok may caused by fast link train while exit PSR active,
> +				 * wait at least 1000 us then read it again.
> +				 */
> +				DRM_DEBUG_KMS("%s: channel EQ not ok, retry = %d, DPCD 0x202 = 0x%x, 0x203 = 0x%x, 0x204 = 0x%x\n",
> +					intel_encoder->base.name, retry, link_status[0], link_status[1], link_status[2]);
> +				usleep_range(1000, 1500);
> +				if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +					DRM_ERROR("Failed to get link status\n");
> +					return;
> +				}
> +
> +				if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
> +					return;
> +			}
> +		}

Sorry, I'm really not fond of how this basically duplicates the entire
intel_dp_check_link_status() function for the retry. Looks like you
should move the intel_dp_get_link_status() call closer to the
drm_dp_channel_eq_ok() call in the function, and abstract them into a
separate helper. Then call the helper in a loop, but only really repeat
for eDP and PSR active, settle for one try otherwise.

But before you do, please check with Rodrigo (Cc'd) if he thinks the
idea is sane to begin with.

BR,
Jani.



> +
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 024798a9c016..f6229d616971 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4225,6 +4225,7 @@  static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
@@ -4247,6 +4248,27 @@  static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 
 	/* Retrain if Channel EQ or CR not ok */
 	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
+		if (is_edp(intel_dp) && dev_priv->psr.enabled) {
+			u8 retry;
+
+			for (retry = 0; retry < 3; retry++) {
+				/*
+				 * EQ not ok may caused by fast link train while exit PSR active,
+				 * wait at least 1000 us then read it again.
+				 */
+				DRM_DEBUG_KMS("%s: channel EQ not ok, retry = %d, DPCD 0x202 = 0x%x, 0x203 = 0x%x, 0x204 = 0x%x\n",
+					intel_encoder->base.name, retry, link_status[0], link_status[1], link_status[2]);
+				usleep_range(1000, 1500);
+				if (!intel_dp_get_link_status(intel_dp, link_status)) {
+					DRM_ERROR("Failed to get link status\n");
+					return;
+				}
+
+				if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
+					return;
+			}
+		}
+
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);