All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
@ 2018-09-12 22:57 Manasi Navare
  2018-09-12 22:57 ` [PATCH v2 2/4] drm/i915/dp: Disconnect eDP downclock mode from DRRS Manasi Navare
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Manasi Navare @ 2018-09-12 22:57 UTC (permalink / raw)
  To: intel-gfx; +Cc: Daniel Vetter, Lucas De Marchi

This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
Do not do link training fallback or prune modes on EDP") that causes
a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
where first link training fails and a retraining is required by falling
back to lower link rate/lane count.
In case of some panels they advertise higher link rate/lane count
than whats required for supporting the panel's native mode.
But we always link train at highest link rate/lane count for eDP
and if that fails we can still fallback to lower link rate/lane count
as long as the fallback link BW still fits the native mode to avoid
pruning the panel's native mode yet retraining at fallback values
to recover from a blank screen.

v2:
* Send uevent if link failure on eDP unconditionally

Cc: Clinton Taylor <clinton.a.taylor@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
---
 drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 436c22de33b6..e4de5257cd87 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
 	return true;
 }
 
+static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
+						     int link_rate,
+						     uint8_t lane_count)
+{
+	struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
+	int mode_rate, max_rate;
+
+	mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
+	max_rate = intel_dp_max_data_rate(link_rate, lane_count);
+	if (mode_rate > max_rate)
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {
@@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 				    intel_dp->num_common_rates,
 				    link_rate);
 	if (index > 0) {
+		if (intel_dp_is_edp(intel_dp) &&
+		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
+							      intel_dp->common_rates[index - 1],
+							      lane_count)) {
+			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
+			return 0;
+		}
 		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
 		intel_dp->max_link_lane_count = lane_count;
 	} else if (lane_count > 1) {
+		if (intel_dp_is_edp(intel_dp) &&
+		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
+							      intel_dp_max_common_rate(intel_dp),
+							      lane_count >> 1)) {
+			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
+			return 0;
+		}
 		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
 		intel_dp->max_link_lane_count = lane_count >> 1;
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index a9f40985a621..30be0e39bd5f 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	return;
 
  failure_handling:
-	/* Dont fallback and prune modes if its eDP */
-	if (!intel_dp_is_edp(intel_dp)) {
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
-			      intel_connector->base.base.id,
-			      intel_connector->base.name,
-			      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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
-			  intel_connector->base.base.id,
-			  intel_connector->base.name,
-			  intel_dp->link_rate, intel_dp->lane_count);
-	}
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
+		      intel_connector->base.base.id,
+		      intel_connector->base.name,
+		      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);
 	return;
 }
-- 
2.18.0

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] drm/i915/dp: Disconnect eDP downclock mode from DRRS
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
@ 2018-09-12 22:57 ` Manasi Navare
  2018-09-12 22:57 ` [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists Manasi Navare
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Manasi Navare @ 2018-09-12 22:57 UTC (permalink / raw)
  To: intel-gfx

Downclock mode of the panel needs to be set independently
of DRRS so that the user can set the downclock mode on the panel
without enabling DRRS.
This patch sets the downclock mode in edp_init as opposed to
drrs_init to achieve this.

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.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_dp.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e4de5257cd87..30526c02b343 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6371,44 +6371,30 @@ void intel_edp_drrs_flush(struct drm_i915_private *dev_priv,
  * This function is  called only once at driver load to initialize basic
  * DRRS stuff.
  *
- * Returns:
- * Downclock mode if panel supports it, else return NULL.
- * DRRS support is determined by the presence of downclock mode (apart
- * from VBT setting).
  */
-static struct drm_display_mode *
+static void
 intel_dp_drrs_init(struct intel_connector *connector,
 		   struct drm_display_mode *fixed_mode)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	struct drm_display_mode *downclock_mode = NULL;
 
 	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work);
 	mutex_init(&dev_priv->drrs.mutex);
 
 	if (INTEL_GEN(dev_priv) <= 6) {
 		DRM_DEBUG_KMS("DRRS supported for Gen7 and above\n");
-		return NULL;
+		return;
 	}
 
 	if (dev_priv->vbt.drrs_type != SEAMLESS_DRRS_SUPPORT) {
 		DRM_DEBUG_KMS("VBT doesn't support DRRS\n");
-		return NULL;
-	}
-
-	downclock_mode = intel_find_panel_downclock(dev_priv, fixed_mode,
-						    &connector->base);
-
-	if (!downclock_mode) {
-		DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
-		return NULL;
+		return;
 	}
 
 	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
 
 	dev_priv->drrs.refresh_rate_type = DRRS_HIGH_RR;
 	DRM_DEBUG_KMS("seamless DRRS supported for eDP panel.\n");
-	return downclock_mode;
 }
 
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
@@ -6476,8 +6462,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	list_for_each_entry(scan, &connector->probed_modes, head) {
 		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
 			fixed_mode = drm_mode_duplicate(dev, scan);
-			downclock_mode = intel_dp_drrs_init(
-						intel_connector, fixed_mode);
+			downclock_mode = intel_find_panel_downclock(dev_priv,
+								    fixed_mode,
+								    connector);
+			if (downclock_mode)
+				intel_dp_drrs_init(intel_connector, fixed_mode);
+			else
+				DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
 			break;
 		}
 	}
-- 
2.18.0

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
  2018-09-12 22:57 ` [PATCH v2 2/4] drm/i915/dp: Disconnect eDP downclock mode from DRRS Manasi Navare
@ 2018-09-12 22:57 ` Manasi Navare
  2018-10-01 16:21   ` Ville Syrjälä
  2018-09-12 22:57 ` [PATCH v2 4/4] drm/i915/dp: Check eDP fallback link BW against downclock mode Manasi Navare
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Manasi Navare @ 2018-09-12 22:57 UTC (permalink / raw)
  To: intel-gfx

With downclock mode and DRRS now operating independently user can
request a modeset on downclock mode. In that case intel_dp_mode_valid()
should validate the downclock mode as well if that is requested.

Cc: Jani Nikula <jani.nikula@linux.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_dp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 30526c02b343..0e400629e85c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -615,6 +615,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
+	struct drm_display_mode *downclock_mode =
+		intel_connector->panel.downclock_mode;
 	int target_clock = mode->clock;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 	int max_dotclk;
@@ -631,7 +633,10 @@ intel_dp_mode_valid(struct drm_connector *connector,
 		if (mode->vdisplay > fixed_mode->vdisplay)
 			return MODE_PANEL;
 
-		target_clock = fixed_mode->clock;
+		if (target_clock < fixed_mode->clock && downclock_mode)
+			target_clock = downclock_mode->clock;
+		else
+			target_clock = fixed_mode->clock;
 	}
 
 	max_link_clock = intel_dp_max_link_rate(intel_dp);
-- 
2.18.0

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] drm/i915/dp: Check eDP fallback link BW against downclock mode
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
  2018-09-12 22:57 ` [PATCH v2 2/4] drm/i915/dp: Disconnect eDP downclock mode from DRRS Manasi Navare
  2018-09-12 22:57 ` [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists Manasi Navare
@ 2018-09-12 22:57 ` Manasi Navare
  2018-09-12 23:42 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Manasi Navare @ 2018-09-12 22:57 UTC (permalink / raw)
  To: intel-gfx

If link training fails on eDP then we fallback to lower link rate
and lane count. If the fallback link BW cannot fit the panel's native
mode and if the downclock mode exists then we should check if the fallback
BW can fit this downclock mode.

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.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_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0e400629e85c..bfc56e778d0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -562,12 +562,21 @@ static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
 						     uint8_t lane_count)
 {
 	struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
+	struct drm_display_mode *downclock_mode =
+		intel_dp->attached_connector->panel.downclock_mode;
 	int mode_rate, max_rate;
 
 	mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
 	max_rate = intel_dp_max_data_rate(link_rate, lane_count);
-	if (mode_rate > max_rate)
+	if (mode_rate > max_rate) {
+		if (downclock_mode) {
+			mode_rate = intel_dp_link_required(downclock_mode->clock,
+							   18);
+			if (mode_rate > max_rate)
+				return false;
+		}
 		return false;
+	}
 
 	return true;
 }
-- 
2.18.0

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
                   ` (2 preceding siblings ...)
  2018-09-12 22:57 ` [PATCH v2 4/4] drm/i915/dp: Check eDP fallback link BW against downclock mode Manasi Navare
@ 2018-09-12 23:42 ` Patchwork
  2018-09-13  0:03 ` [PATCH v2 1/4] " Lucas De Marchi
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-09-12 23:42 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
URL   : https://patchwork.freedesktop.org/series/49596/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4813 -> Patchwork_10163 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49596/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10163 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       PASS -> INCOMPLETE (fdo#107773)

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-cfl-guc:         DMESG-FAIL (fdo#107710) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#107139, fdo#105128) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107710 https://bugs.freedesktop.org/show_bug.cgi?id=107710
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


== Participating hosts (51 -> 45) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4813 -> Patchwork_10163

  CI_DRM_4813: 3c13515b12339366b414637b69227a4e3cbe21ae @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4640: 9a8da36e708f9ed15b20689dfe305e41f9a19008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10163: c20815b7fb4e8a7f36d8f318b5b5090b70ee39ac @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c20815b7fb4e drm/i915/dp: Check eDP fallback link BW against downclock mode
b0a8b896b0af drm/i915/dp: Validate the downclock mode for eDP if it exists
07fbd2a99d01 drm/i915/dp: Disconnect eDP downclock mode from DRRS
fdbd4338b849 drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10163/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
                   ` (3 preceding siblings ...)
  2018-09-12 23:42 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Patchwork
@ 2018-09-13  0:03 ` Lucas De Marchi
  2018-10-09 20:49   ` Manasi Navare
  2018-09-13  4:58 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/4] " Patchwork
  2018-10-01 16:03 ` [PATCH v2 1/4] " Ville Syrjälä
  6 siblings, 1 reply; 12+ messages in thread
From: Lucas De Marchi @ 2018-09-13  0:03 UTC (permalink / raw)
  To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, Lucas De Marchi

On Wed, Sep 12, 2018 at 3:55 PM Manasi Navare <manasi.d.navare@intel.com> wrote:
>
> This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
> Do not do link training fallback or prune modes on EDP") that causes
> a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
> where first link training fails and a retraining is required by falling
> back to lower link rate/lane count.
> In case of some panels they advertise higher link rate/lane count
> than whats required for supporting the panel's native mode.
> But we always link train at highest link rate/lane count for eDP
> and if that fails we can still fallback to lower link rate/lane count
> as long as the fallback link BW still fits the native mode to avoid
> pruning the panel's native mode yet retraining at fallback values
> to recover from a blank screen.

I suspect something went bad on my laptop or the cause is something else.
I can't reproduce this problem anymore, even if I go back to old kernels.
And, in contrary to what I had previously, now I have:

# dd if=/dev/drm_dp_aux0 bs=1 | hexdump -C
...
00000070  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
...

Lucas De Marchi

>
> v2:
> * Send uevent if link failure on eDP unconditionally
>
> Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
> ---
>  drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
>  2 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..e4de5257cd87 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
>         return true;
>  }
>
> +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> +                                                    int link_rate,
> +                                                    uint8_t lane_count)
> +{
> +       struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
> +       int mode_rate, max_rate;
> +
> +       mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
> +       max_rate = intel_dp_max_data_rate(link_rate, lane_count);
> +       if (mode_rate > max_rate)
> +               return false;
> +
> +       return true;
> +}
> +
>  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>                                             int link_rate, uint8_t lane_count)
>  {
> @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>                                     intel_dp->num_common_rates,
>                                     link_rate);
>         if (index > 0) {
> +               if (intel_dp_is_edp(intel_dp) &&
> +                   !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> +                                                             intel_dp->common_rates[index - 1],
> +                                                             lane_count)) {
> +                       DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> +                       return 0;
> +               }
>                 intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
>                 intel_dp->max_link_lane_count = lane_count;
>         } else if (lane_count > 1) {
> +               if (intel_dp_is_edp(intel_dp) &&
> +                   !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> +                                                             intel_dp_max_common_rate(intel_dp),
> +                                                             lane_count >> 1)) {
> +                       DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> +                       return 0;
> +               }
>                 intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
>                 intel_dp->max_link_lane_count = lane_count >> 1;
>         } else {
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index a9f40985a621..30be0e39bd5f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>         return;
>
>   failure_handling:
> -       /* Dont fallback and prune modes if its eDP */
> -       if (!intel_dp_is_edp(intel_dp)) {
> -               DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> -                             intel_connector->base.base.id,
> -                             intel_connector->base.name,
> -                             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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> -                         intel_connector->base.base.id,
> -                         intel_connector->base.name,
> -                         intel_dp->link_rate, intel_dp->lane_count);
> -       }
> +       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> +                     intel_connector->base.base.id,
> +                     intel_connector->base.name,
> +                     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);
>         return;
>  }
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
                   ` (4 preceding siblings ...)
  2018-09-13  0:03 ` [PATCH v2 1/4] " Lucas De Marchi
@ 2018-09-13  4:58 ` Patchwork
  2018-10-01 16:03 ` [PATCH v2 1/4] " Ville Syrjälä
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-09-13  4:58 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
URL   : https://patchwork.freedesktop.org/series/49596/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4813_full -> Patchwork_10163_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10163_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    
    ==== Possible fixes ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          FAIL (fdo#106680) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4813 -> Patchwork_10163

  CI_DRM_4813: 3c13515b12339366b414637b69227a4e3cbe21ae @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4640: 9a8da36e708f9ed15b20689dfe305e41f9a19008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10163: c20815b7fb4e8a7f36d8f318b5b5090b70ee39ac @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10163/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
                   ` (5 preceding siblings ...)
  2018-09-13  4:58 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/4] " Patchwork
@ 2018-10-01 16:03 ` Ville Syrjälä
  2018-10-08 19:23   ` Manasi Navare
  6 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2018-10-01 16:03 UTC (permalink / raw)
  To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, Lucas De Marchi

On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote:
> This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
> Do not do link training fallback or prune modes on EDP") that causes
> a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
> where first link training fails and a retraining is required by falling
> back to lower link rate/lane count.
> In case of some panels they advertise higher link rate/lane count
> than whats required for supporting the panel's native mode.
> But we always link train at highest link rate/lane count for eDP
> and if that fails we can still fallback to lower link rate/lane count
> as long as the fallback link BW still fits the native mode to avoid
> pruning the panel's native mode yet retraining at fallback values
> to recover from a blank screen.
> 
> v2:
> * Send uevent if link failure on eDP unconditionally
> 
> Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
> ---
>  drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
>  2 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..e4de5257cd87 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
>  	return true;
>  }
>  
> +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> +						     int link_rate,
> +						     uint8_t lane_count)
> +{
> +	struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;

const

> +	int mode_rate, max_rate;
> +
> +	mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
> +	max_rate = intel_dp_max_data_rate(link_rate, lane_count);
> +	if (mode_rate > max_rate)
> +		return false;

I wonder if we should extract this into a helper and share with
mode_valid(). Then again, it's just a few lines so maybe not worth it.

> +
> +	return true;
> +}
> +
>  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  					    int link_rate, uint8_t lane_count)
>  {
> @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  				    intel_dp->num_common_rates,
>  				    link_rate);
>  	if (index > 0) {
> +		if (intel_dp_is_edp(intel_dp) &&
> +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> +							      intel_dp->common_rates[index - 1],
> +							      lane_count)) {
> +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> +			return 0;
> +		}
>  		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
>  		intel_dp->max_link_lane_count = lane_count;
>  	} else if (lane_count > 1) {
> +		if (intel_dp_is_edp(intel_dp) &&
> +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> +							      intel_dp_max_common_rate(intel_dp),
> +							      lane_count >> 1)) {
> +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> +			return 0;
> +		}
>  		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
>  		intel_dp->max_link_lane_count = lane_count >> 1;

This whole thing is getting a bit messy. I think it would worthwile to
rewrite this as something like:

intel_dp_update_link_train_fallback_values(intel_dp)
{
	int link_rate = intel_dp->link_rate;
	int lane_count = intel_dp->lane_count;

	if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count))
		return -1;

	if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) {
		DRM_DEBUG_KMS("...");
		return 0;
	}

	intel_dp->max_link_rate = link_rate;
	intel_dp->max_link_lane_count = lane_count;

	return 0;
}

But that can be done later. With the missing const added this patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index a9f40985a621..30be0e39bd5f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  	return;
>  
>   failure_handling:
> -	/* Dont fallback and prune modes if its eDP */
> -	if (!intel_dp_is_edp(intel_dp)) {
> -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> -			      intel_connector->base.base.id,
> -			      intel_connector->base.name,
> -			      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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> -			  intel_connector->base.base.id,
> -			  intel_connector->base.name,
> -			  intel_dp->link_rate, intel_dp->lane_count);
> -	}
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> +		      intel_connector->base.base.id,
> +		      intel_connector->base.name,
> +		      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);
>  	return;
>  }
> -- 
> 2.18.0

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists
  2018-09-12 22:57 ` [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists Manasi Navare
@ 2018-10-01 16:21   ` Ville Syrjälä
  0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2018-10-01 16:21 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Wed, Sep 12, 2018 at 03:57:18PM -0700, Manasi Navare wrote:
> With downclock mode and DRRS now operating independently user can
> request a modeset on downclock mode. In that case intel_dp_mode_valid()
> should validate the downclock mode as well if that is requested.
> 
> Cc: Jani Nikula <jani.nikula@linux.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_dp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 30526c02b343..0e400629e85c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -615,6 +615,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
> +	struct drm_display_mode *downclock_mode =
> +		intel_connector->panel.downclock_mode;
>  	int target_clock = mode->clock;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  	int max_dotclk;
> @@ -631,7 +633,10 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  		if (mode->vdisplay > fixed_mode->vdisplay)
>  			return MODE_PANEL;
>  
> -		target_clock = fixed_mode->clock;
> +		if (target_clock < fixed_mode->clock && downclock_mode)
> +			target_clock = downclock_mode->clock;
> +		else
> +			target_clock = fixed_mode->clock;

The whole mode_valid() thing is a bit of mess for internal displays.
I tried to start fixing it here https://patchwork.freedesktop.org/series/45610/
but it got stuck in a bit of a bikeshed.

So without going all in and fixing the vrefresh stuff, and probably
also digging through the whole rat's nest of drm_mode_equal() clock
comparison accuracy (if we want to deal with the whole 59.97 v. 60
hz stuff fully) I think this can be simplified into:

intel_edp_min_dotclock()
{
	return downclock_mode ? downclock_mode->clock : fixed_mode->clock;
}

Anyways, none of that is helpful unless you actually start to use the
downlock mode instead of the fixed mode. Which I didn't spot anywhere
in this series.

>  	}
>  
>  	max_link_clock = intel_dp_max_link_rate(intel_dp);
> -- 
> 2.18.0

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-10-01 16:03 ` [PATCH v2 1/4] " Ville Syrjälä
@ 2018-10-08 19:23   ` Manasi Navare
  2018-10-09 13:00     ` Ville Syrjälä
  0 siblings, 1 reply; 12+ messages in thread
From: Manasi Navare @ 2018-10-08 19:23 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, Lucas De Marchi

On Mon, Oct 01, 2018 at 07:03:52PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote:
> > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
> > Do not do link training fallback or prune modes on EDP") that causes
> > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
> > where first link training fails and a retraining is required by falling
> > back to lower link rate/lane count.
> > In case of some panels they advertise higher link rate/lane count
> > than whats required for supporting the panel's native mode.
> > But we always link train at highest link rate/lane count for eDP
> > and if that fails we can still fallback to lower link rate/lane count
> > as long as the fallback link BW still fits the native mode to avoid
> > pruning the panel's native mode yet retraining at fallback values
> > to recover from a blank screen.
> > 
> > v2:
> > * Send uevent if link failure on eDP unconditionally
> > 
> > Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
> >  2 files changed, 38 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 436c22de33b6..e4de5257cd87 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
> >  	return true;
> >  }
> >  
> > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> > +						     int link_rate,
> > +						     uint8_t lane_count)
> > +{
> > +	struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
> 
> const
>

Will add const to the fixed_mode.
 
> > +	int mode_rate, max_rate;
> > +
> > +	mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
> > +	max_rate = intel_dp_max_data_rate(link_rate, lane_count);
> > +	if (mode_rate > max_rate)
> > +		return false;
> 
> I wonder if we should extract this into a helper and share with
> mode_valid(). Then again, it's just a few lines so maybe not worth it.
> 
> > +
> > +	return true;
> > +}
> > +
> >  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  					    int link_rate, uint8_t lane_count)
> >  {
> > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  				    intel_dp->num_common_rates,
> >  				    link_rate);
> >  	if (index > 0) {
> > +		if (intel_dp_is_edp(intel_dp) &&
> > +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > +							      intel_dp->common_rates[index - 1],
> > +							      lane_count)) {
> > +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > +			return 0;
> > +		}
> >  		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
> >  		intel_dp->max_link_lane_count = lane_count;
> >  	} else if (lane_count > 1) {
> > +		if (intel_dp_is_edp(intel_dp) &&
> > +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > +							      intel_dp_max_common_rate(intel_dp),
> > +							      lane_count >> 1)) {
> > +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > +			return 0;
> > +		}
> >  		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> >  		intel_dp->max_link_lane_count = lane_count >> 1;
> 
> This whole thing is getting a bit messy. I think it would worthwile to
> rewrite this as something like:
> 
> intel_dp_update_link_train_fallback_values(intel_dp)
> {
> 	int link_rate = intel_dp->link_rate;
> 	int lane_count = intel_dp->lane_count;
> 
> 	if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count))
> 		return -1;
> 
> 	if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) {
> 		DRM_DEBUG_KMS("...");
> 		return 0;
> 	}
> 
> 	intel_dp->max_link_rate = link_rate;
> 	intel_dp->max_link_lane_count = lane_count;
> 
> 	return 0;
> }
> 
> But that can be done later. With the missing const added this patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>

Yes this can be refactored/reorganized as a follow up patch, also when downclock mode
checks get in.
So until then thsi fix is good to be merged right?

Manasi
 
> >  	} else {
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index a9f40985a621..30be0e39bd5f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> >  	return;
> >  
> >   failure_handling:
> > -	/* Dont fallback and prune modes if its eDP */
> > -	if (!intel_dp_is_edp(intel_dp)) {
> > -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > -			      intel_connector->base.base.id,
> > -			      intel_connector->base.name,
> > -			      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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > -			  intel_connector->base.base.id,
> > -			  intel_connector->base.name,
> > -			  intel_dp->link_rate, intel_dp->lane_count);
> > -	}
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > +		      intel_connector->base.base.id,
> > +		      intel_connector->base.name,
> > +		      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);
> >  	return;
> >  }
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-10-08 19:23   ` Manasi Navare
@ 2018-10-09 13:00     ` Ville Syrjälä
  0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2018-10-09 13:00 UTC (permalink / raw)
  To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, Lucas De Marchi

On Mon, Oct 08, 2018 at 12:23:32PM -0700, Manasi Navare wrote:
> On Mon, Oct 01, 2018 at 07:03:52PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote:
> > > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
> > > Do not do link training fallback or prune modes on EDP") that causes
> > > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
> > > where first link training fails and a retraining is required by falling
> > > back to lower link rate/lane count.
> > > In case of some panels they advertise higher link rate/lane count
> > > than whats required for supporting the panel's native mode.
> > > But we always link train at highest link rate/lane count for eDP
> > > and if that fails we can still fallback to lower link rate/lane count
> > > as long as the fallback link BW still fits the native mode to avoid
> > > pruning the panel's native mode yet retraining at fallback values
> > > to recover from a blank screen.
> > > 
> > > v2:
> > > * Send uevent if link failure on eDP unconditionally
> > > 
> > > Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
> > >  2 files changed, 38 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 436c22de33b6..e4de5257cd87 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
> > >  	return true;
> > >  }
> > >  
> > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> > > +						     int link_rate,
> > > +						     uint8_t lane_count)
> > > +{
> > > +	struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
> > 
> > const
> >
> 
> Will add const to the fixed_mode.
>  
> > > +	int mode_rate, max_rate;
> > > +
> > > +	mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
> > > +	max_rate = intel_dp_max_data_rate(link_rate, lane_count);
> > > +	if (mode_rate > max_rate)
> > > +		return false;
> > 
> > I wonder if we should extract this into a helper and share with
> > mode_valid(). Then again, it's just a few lines so maybe not worth it.
> > 
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > >  					    int link_rate, uint8_t lane_count)
> > >  {
> > > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > >  				    intel_dp->num_common_rates,
> > >  				    link_rate);
> > >  	if (index > 0) {
> > > +		if (intel_dp_is_edp(intel_dp) &&
> > > +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > > +							      intel_dp->common_rates[index - 1],
> > > +							      lane_count)) {
> > > +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > > +			return 0;
> > > +		}
> > >  		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
> > >  		intel_dp->max_link_lane_count = lane_count;
> > >  	} else if (lane_count > 1) {
> > > +		if (intel_dp_is_edp(intel_dp) &&
> > > +		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > > +							      intel_dp_max_common_rate(intel_dp),
> > > +							      lane_count >> 1)) {
> > > +			DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > > +			return 0;
> > > +		}
> > >  		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> > >  		intel_dp->max_link_lane_count = lane_count >> 1;
> > 
> > This whole thing is getting a bit messy. I think it would worthwile to
> > rewrite this as something like:
> > 
> > intel_dp_update_link_train_fallback_values(intel_dp)
> > {
> > 	int link_rate = intel_dp->link_rate;
> > 	int lane_count = intel_dp->lane_count;
> > 
> > 	if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count))
> > 		return -1;
> > 
> > 	if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) {
> > 		DRM_DEBUG_KMS("...");
> > 		return 0;
> > 	}
> > 
> > 	intel_dp->max_link_rate = link_rate;
> > 	intel_dp->max_link_lane_count = lane_count;
> > 
> > 	return 0;
> > }
> > 
> > But that can be done later. With the missing const added this patch is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> 
> Yes this can be refactored/reorganized as a follow up patch, also when downclock mode
> checks get in.
> So until then thsi fix is good to be merged right?

Yes.

> 
> Manasi
>  
> > >  	} else {
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index a9f40985a621..30be0e39bd5f 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> > >  	return;
> > >  
> > >   failure_handling:
> > > -	/* Dont fallback and prune modes if its eDP */
> > > -	if (!intel_dp_is_edp(intel_dp)) {
> > > -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > > -			      intel_connector->base.base.id,
> > > -			      intel_connector->base.name,
> > > -			      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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > > -			  intel_connector->base.base.id,
> > > -			  intel_connector->base.name,
> > > -			  intel_dp->link_rate, intel_dp->lane_count);
> > > -	}
> > > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > > +		      intel_connector->base.base.id,
> > > +		      intel_connector->base.name,
> > > +		      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);
> > >  	return;
> > >  }
> > > -- 
> > > 2.18.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode
  2018-09-13  0:03 ` [PATCH v2 1/4] " Lucas De Marchi
@ 2018-10-09 20:49   ` Manasi Navare
  0 siblings, 0 replies; 12+ messages in thread
From: Manasi Navare @ 2018-10-09 20:49 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: Daniel Vetter, intel-gfx, Lucas De Marchi

On Wed, Sep 12, 2018 at 05:03:15PM -0700, Lucas De Marchi wrote:
> On Wed, Sep 12, 2018 at 3:55 PM Manasi Navare <manasi.d.navare@intel.com> wrote:
> >
> > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp:
> > Do not do link training fallback or prune modes on EDP") that causes
> > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350)
> > where first link training fails and a retraining is required by falling
> > back to lower link rate/lane count.
> > In case of some panels they advertise higher link rate/lane count
> > than whats required for supporting the panel's native mode.
> > But we always link train at highest link rate/lane count for eDP
> > and if that fails we can still fallback to lower link rate/lane count
> > as long as the fallback link BW still fits the native mode to avoid
> > pruning the panel's native mode yet retraining at fallback values
> > to recover from a blank screen.
> 
> I suspect something went bad on my laptop or the cause is something else.
> I can't reproduce this problem anymore, even if I go back to old kernels.
> And, in contrary to what I had previously, now I have:
> 
> # dd if=/dev/drm_dp_aux0 bs=1 | hexdump -C
> ...
> 00000070  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> ...
>

Thanks for testing this scenario again. I think this is one of those panels
where sometimes it works without a retrain and sometimes requires retraining
which was the cause of the black screen on your laptop earlier.
But regardless there are other bugs seen where where we need retraining on eDP
with reduced fallback parameters or the same parameters.
And this patch still fixes those bugs.

Manasi
 
> Lucas De Marchi
> 
> >
> > v2:
> > * Send uevent if link failure on eDP unconditionally
> >
> > Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c               | 29 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++-----------
> >  2 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 436c22de33b6..e4de5257cd87 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
> >         return true;
> >  }
> >
> > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> > +                                                    int link_rate,
> > +                                                    uint8_t lane_count)
> > +{
> > +       struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode;
> > +       int mode_rate, max_rate;
> > +
> > +       mode_rate = intel_dp_link_required(fixed_mode->clock, 18);
> > +       max_rate = intel_dp_max_data_rate(link_rate, lane_count);
> > +       if (mode_rate > max_rate)
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> >  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >                                             int link_rate, uint8_t lane_count)
> >  {
> > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >                                     intel_dp->num_common_rates,
> >                                     link_rate);
> >         if (index > 0) {
> > +               if (intel_dp_is_edp(intel_dp) &&
> > +                   !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > +                                                             intel_dp->common_rates[index - 1],
> > +                                                             lane_count)) {
> > +                       DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > +                       return 0;
> > +               }
> >                 intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
> >                 intel_dp->max_link_lane_count = lane_count;
> >         } else if (lane_count > 1) {
> > +               if (intel_dp_is_edp(intel_dp) &&
> > +                   !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > +                                                             intel_dp_max_common_rate(intel_dp),
> > +                                                             lane_count >> 1)) {
> > +                       DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n");
> > +                       return 0;
> > +               }
> >                 intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> >                 intel_dp->max_link_lane_count = lane_count >> 1;
> >         } else {
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index a9f40985a621..30be0e39bd5f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> >         return;
> >
> >   failure_handling:
> > -       /* Dont fallback and prune modes if its eDP */
> > -       if (!intel_dp_is_edp(intel_dp)) {
> > -               DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > -                             intel_connector->base.base.id,
> > -                             intel_connector->base.name,
> > -                             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_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > -                         intel_connector->base.base.id,
> > -                         intel_connector->base.name,
> > -                         intel_dp->link_rate, intel_dp->lane_count);
> > -       }
> > +       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d",
> > +                     intel_connector->base.base.id,
> > +                     intel_connector->base.name,
> > +                     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);
> >         return;
> >  }
> > --
> > 2.18.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-10-09 20:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-12 22:57 [PATCH v2 1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Manasi Navare
2018-09-12 22:57 ` [PATCH v2 2/4] drm/i915/dp: Disconnect eDP downclock mode from DRRS Manasi Navare
2018-09-12 22:57 ` [PATCH v2 3/4] drm/i915/dp: Validate the downclock mode for eDP if it exists Manasi Navare
2018-10-01 16:21   ` Ville Syrjälä
2018-09-12 22:57 ` [PATCH v2 4/4] drm/i915/dp: Check eDP fallback link BW against downclock mode Manasi Navare
2018-09-12 23:42 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode Patchwork
2018-09-13  0:03 ` [PATCH v2 1/4] " Lucas De Marchi
2018-10-09 20:49   ` Manasi Navare
2018-09-13  4:58 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/4] " Patchwork
2018-10-01 16:03 ` [PATCH v2 1/4] " Ville Syrjälä
2018-10-08 19:23   ` Manasi Navare
2018-10-09 13:00     ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.