All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
@ 2018-09-05  9:53 Jani Nikula
  2018-09-05 10:28 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Jani Nikula @ 2018-09-05  9:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Rodrigo Vivi

We've opted to use the maximum link rate and lane count for eDP panels,
because typically the maximum supported configuration reported by the
panel has matched the native resolution requirements of the panel, and
optimizing the link has lead to problems.

With eDP 1.4 rate select method and DSC features, this is decreasingly
the case. There's a need to optimize the link parameters. Moreover,
already eDP 1.3 states fast link with fewer lanes is preferred over the
wide and slow. (Wide and slow should still be more reliable for longer
cable lengths.)

Additionally, there have been reports of panels failing on arbitrary
link configurations, although arguably all configurations they claim to
support should work.

Optimize eDP 1.4+ link config fast and narrow.

Side note: The implementation has a near duplicate of the link config
function, with just the two inner for loops turned inside out. Perhaps
there'd be a way to make this, say, more table driven to reduce the
duplication, but seems like that would lead to duplication in the table
generation. We'll also have to see how the link config optimization for
DSC turns out.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Matt Atwood <matthew.s.atwood@intel.com>
Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com

Untested. It's possible this helps the referenced bug. The downside is
that this patch has a bunch of dependencies that are too much to
backport to stable kernels. If the patch works, we may need to consider
hacking together an uglier backport.
---
 drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 436c22de33b6..bf7b91832c8a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
 	return false;
 }
 
+/* Optimize link config in order: max bpp, min lanes, min clock */
+static bool
+intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
+				  struct intel_crtc_state *pipe_config,
+				  const struct link_config_limits *limits)
+{
+	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	int bpp, clock, lane_count;
+	int mode_rate, link_clock, link_avail;
+
+	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
+		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
+						   bpp);
+
+		for (lane_count = limits->min_lane_count;
+		     lane_count <= limits->max_lane_count;
+		     lane_count <<= 1) {
+			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
+				link_clock = intel_dp->common_rates[clock];
+				link_avail = intel_dp_max_data_rate(link_clock,
+								    lane_count);
+
+				if (mode_rate <= link_avail) {
+					pipe_config->lane_count = lane_count;
+					pipe_config->pipe_bpp = bpp;
+					pipe_config->port_clock = link_clock;
+
+					return true;
+				}
+			}
+		}
+	}
+
+	return false;
+}
+
 static bool
 intel_dp_compute_link_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config)
@@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 	limits.min_bpp = 6 * 3;
 	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 
-	if (intel_dp_is_edp(intel_dp)) {
+	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
 		/*
 		 * Use the maximum clock and number of lanes the eDP panel
-		 * advertizes being capable of. The panels are generally
-		 * designed to support only a single clock and lane
-		 * configuration, and typically these values correspond to the
-		 * native resolution of the panel.
+		 * advertizes being capable of. The eDP 1.3 and earlier panels
+		 * are generally designed to support only a single clock and
+		 * lane configuration, and typically these values correspond to
+		 * the native resolution of the panel. With eDP 1.4 rate select
+		 * and DSC, this is decreasingly the case, and we need to be
+		 * able to select less than maximum link config.
 		 */
 		limits.min_lane_count = limits.max_lane_count;
 		limits.min_clock = limits.max_clock;
@@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 		      intel_dp->common_rates[limits.max_clock],
 		      limits.max_bpp, adjusted_mode->crtc_clock);
 
-	/*
-	 * Optimize for slow and wide. This is the place to add alternative
-	 * optimization policy.
-	 */
-	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
-		return false;
+	if (intel_dp_is_edp(intel_dp)) {
+		/*
+		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
+		 * section A.1: "It is recommended that the minimum number of
+		 * lanes be used, using the minimum link rate allowed for that
+		 * lane configuration."
+		 *
+		 * Note that we use the max clock and lane count for eDP 1.3 and
+		 * earlier, and fast vs. wide is irrelevant.
+		 */
+		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
+						       &limits))
+			return false;
+	} else {
+		/* Optimize for slow and wide. */
+		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
+						       &limits))
+			return false;
+	}
 
 	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
 		      pipe_config->lane_count, pipe_config->port_clock,
-- 
2.11.0

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

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

* ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
@ 2018-09-05 10:28 ` Patchwork
  2018-09-05 12:02 ` ✓ Fi.CI.IGT: " Patchwork
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-05 10:28 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
URL   : https://patchwork.freedesktop.org/series/42923/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4770 -> Patchwork_10090 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10090 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10090, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42923/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10090:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rpm@module-reload:
      fi-hsw-4770r:       SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-skl-caroline:    NOTRUN -> INCOMPLETE (fdo#104108, fdo#107556)

    igt@kms_psr@primary_page_flip:
      fi-icl-u:           NOTRUN -> FAIL (fdo#107383) +3

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       DMESG-WARN (fdo#107425) -> PASS

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

    igt@kms_psr@primary_page_flip:
      fi-cnl-psr:         FAIL (fdo#107336) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383
  fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556


== Participating hosts (50 -> 49) ==

  Additional (4): fi-glk-j4005 fi-skl-caroline fi-icl-u fi-elk-e7500 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4770 -> Patchwork_10090

  CI_DRM_4770: 0c3535cf60140d017a5df73d84d06e8b1a5b5d3b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4627: e0c3033a57d85c0d2eb33af0451afa16edc79f10 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10090: 68150174d8a1d495bcbaa1c737cc447ef4b79a49 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

68150174d8a1 drm/i915/dp: optimize eDP 1.4+ link config fast and narrow

== Logs ==

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

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

* ✓ Fi.CI.IGT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
  2018-09-05 10:28 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
@ 2018-09-05 12:02 ` Patchwork
  2018-09-05 16:17 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-05 12:02 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
URL   : https://patchwork.freedesktop.org/series/42923/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4770_full -> Patchwork_10090_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-gtt:
      shard-glk:          PASS -> FAIL (fdo#103167)

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

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#103665, fdo#106023) -> PASS

    igt@kms_busy@extended-modeset-hang-newfb-render-b:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS

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

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4770 -> Patchwork_10090

  CI_DRM_4770: 0c3535cf60140d017a5df73d84d06e8b1a5b5d3b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4627: e0c3033a57d85c0d2eb33af0451afa16edc79f10 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10090: 68150174d8a1d495bcbaa1c737cc447ef4b79a49 @ 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_10090/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
  2018-09-05 10:28 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
  2018-09-05 12:02 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-09-05 16:17 ` Manasi Navare
  2018-09-24 10:35 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Manasi Navare @ 2018-09-05 16:17 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, Rodrigo Vivi

On Wed, Sep 05, 2018 at 12:53:21PM +0300, Jani Nikula wrote:
> We've opted to use the maximum link rate and lane count for eDP panels,
> because typically the maximum supported configuration reported by the
> panel has matched the native resolution requirements of the panel, and
> optimizing the link has lead to problems.
> 
> With eDP 1.4 rate select method and DSC features, this is decreasingly
> the case. There's a need to optimize the link parameters. Moreover,
> already eDP 1.3 states fast link with fewer lanes is preferred over the
> wide and slow. (Wide and slow should still be more reliable for longer
> cable lengths.)
> 
> Additionally, there have been reports of panels failing on arbitrary
> link configurations, although arguably all configurations they claim to
> support should work.
> 
> Optimize eDP 1.4+ link config fast and narrow.
> 
> Side note: The implementation has a near duplicate of the link config
> function, with just the two inner for loops turned inside out. Perhaps
> there'd be a way to make this, say, more table driven to reduce the
> duplication, but seems like that would lead to duplication in the table
> generation. We'll also have to see how the link config optimization for
> DSC turns out.

In case of DSC, currently we only compute DSC params and enable DSC if
intel_dp_compute_link_config() returns a false. So since the fast_narrow
link calculation is embedded within the intel_dp_compute_link_config(),
DSC can still be enabled if link config returns false with either fast_narrow
or wide approach.

However ideally, for power savings for eDP we could eventually enable DSC first
if supported by the panel, find the mode rate with compressed bpp and then
optimize the link config with that compressed bpp. So reverse the order
of dsc_compute_config and intel_dp_compute_link_config()

Everything else in the patch looks good to me.

Acked-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi


> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matt Atwood <matthew.s.atwood@intel.com>
> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com
> 
> Untested. It's possible this helps the referenced bug. The downside is
> that this patch has a bunch of dependencies that are too much to
> backport to stable kernels. If the patch works, we may need to consider
> hacking together an uglier backport.
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..bf7b91832c8a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +/* Optimize link config in order: max bpp, min lanes, min clock */
> +static bool
> +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *pipe_config,
> +				  const struct link_config_limits *limits)
> +{
> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	int bpp, clock, lane_count;
> +	int mode_rate, link_clock, link_avail;
> +
> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> +						   bpp);
> +
> +		for (lane_count = limits->min_lane_count;
> +		     lane_count <= limits->max_lane_count;
> +		     lane_count <<= 1) {
> +			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> +				link_clock = intel_dp->common_rates[clock];
> +				link_avail = intel_dp_max_data_rate(link_clock,
> +								    lane_count);
> +
> +				if (mode_rate <= link_avail) {
> +					pipe_config->lane_count = lane_count;
> +					pipe_config->pipe_bpp = bpp;
> +					pipe_config->port_clock = link_clock;
> +
> +					return true;
> +				}
> +			}
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static bool
>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config)
> @@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	limits.min_bpp = 6 * 3;
>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  
> -	if (intel_dp_is_edp(intel_dp)) {
> +	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
>  		/*
>  		 * Use the maximum clock and number of lanes the eDP panel
> -		 * advertizes being capable of. The panels are generally
> -		 * designed to support only a single clock and lane
> -		 * configuration, and typically these values correspond to the
> -		 * native resolution of the panel.
> +		 * advertizes being capable of. The eDP 1.3 and earlier panels
> +		 * are generally designed to support only a single clock and
> +		 * lane configuration, and typically these values correspond to
> +		 * the native resolution of the panel. With eDP 1.4 rate select
> +		 * and DSC, this is decreasingly the case, and we need to be
> +		 * able to select less than maximum link config.
>  		 */
>  		limits.min_lane_count = limits.max_lane_count;
>  		limits.min_clock = limits.max_clock;
> @@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		      intel_dp->common_rates[limits.max_clock],
>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>  
> -	/*
> -	 * Optimize for slow and wide. This is the place to add alternative
> -	 * optimization policy.
> -	 */
> -	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> -		return false;
> +	if (intel_dp_is_edp(intel_dp)) {
> +		/*
> +		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> +		 * section A.1: "It is recommended that the minimum number of
> +		 * lanes be used, using the minimum link rate allowed for that
> +		 * lane configuration."
> +		 *
> +		 * Note that we use the max clock and lane count for eDP 1.3 and
> +		 * earlier, and fast vs. wide is irrelevant.
> +		 */
> +		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> +						       &limits))
> +			return false;
> +	} else {
> +		/* Optimize for slow and wide. */
> +		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> +						       &limits))
> +			return false;
> +	}
>  
>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
>  		      pipe_config->lane_count, pipe_config->port_clock,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
                   ` (2 preceding siblings ...)
  2018-09-05 16:17 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
@ 2018-09-24 10:35 ` Patchwork
  2018-09-24 11:53 ` ✓ Fi.CI.IGT: " Patchwork
  2018-09-27 10:10 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-24 10:35 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
URL   : https://patchwork.freedesktop.org/series/42923/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4867 -> Patchwork_10261 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42923/revisions/2/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107556, fdo#107774)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      {fi-skl-caroline}:  PASS -> INCOMPLETE (fdo#107556, fdo#104108)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774


== Participating hosts (46 -> 42) ==

  Additional (1): fi-kbl-soraka 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4867 -> Patchwork_10261

  CI_DRM_4867: d3bc4f8ea48e074e21aac5dbd0805630ecea9b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10261: c3f83dcadc38bd1212e596388aa861326f843d14 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c3f83dcadc38 drm/i915/dp: optimize eDP 1.4+ link config fast and narrow

== Logs ==

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

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

* ✓ Fi.CI.IGT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
                   ` (3 preceding siblings ...)
  2018-09-24 10:35 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
@ 2018-09-24 11:53 ` Patchwork
  2018-09-27 10:10 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-24 11:53 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2)
URL   : https://patchwork.freedesktop.org/series/42923/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4867_full -> Patchwork_10261_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10261_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10261_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10261_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
      shard-apl:          PASS -> SKIP +13

    igt@perf_pmu@rc6:
      shard-kbl:          SKIP -> PASS

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106886)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-glk:          NOTRUN -> FAIL (fdo#103158) +3

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +13

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-glk:          NOTRUN -> FAIL (fdo#106641)

    igt@kms_busy@extended-modeset-hang-newfb-render-b:
      shard-kbl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-glk:          NOTRUN -> DMESG-WARN (fdo#107956) +7

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956) +2
      shard-kbl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          NOTRUN -> FAIL (fdo#105454, fdo#106509)

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          NOTRUN -> DMESG-WARN (fdo#105763, fdo#106538)

    igt@kms_frontbuffer_tracking@fbc-1p-rte:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558)

    igt@kms_setmode@basic:
      shard-glk:          NOTRUN -> FAIL (fdo#99912)
      shard-snb:          NOTRUN -> FAIL (fdo#99912)

    igt@kms_sysfs_edid_timing:
      shard-kbl:          NOTRUN -> FAIL (fdo#100047)

    
    ==== Possible fixes ====

    igt@gem_exec_big:
      shard-hsw:          TIMEOUT (fdo#107937) -> PASS

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

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  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_4867 -> Patchwork_10261

  CI_DRM_4867: d3bc4f8ea48e074e21aac5dbd0805630ecea9b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10261: c3f83dcadc38bd1212e596388aa861326f843d14 @ 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_10261/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
  2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
                   ` (4 preceding siblings ...)
  2018-09-24 11:53 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-09-27 10:10 ` Manasi Navare
  2018-09-27 11:15   ` Jani Nikula
  5 siblings, 1 reply; 10+ messages in thread
From: Manasi Navare @ 2018-09-27 10:10 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, Rodrigo Vivi

On Wed, Sep 05, 2018 at 12:53:21PM +0300, Jani Nikula wrote:
> We've opted to use the maximum link rate and lane count for eDP panels,
> because typically the maximum supported configuration reported by the
> panel has matched the native resolution requirements of the panel, and
> optimizing the link has lead to problems.
> 
> With eDP 1.4 rate select method and DSC features, this is decreasingly
> the case. There's a need to optimize the link parameters. Moreover,
> already eDP 1.3 states fast link with fewer lanes is preferred over the
> wide and slow. (Wide and slow should still be more reliable for longer
> cable lengths.)
> 
> Additionally, there have been reports of panels failing on arbitrary
> link configurations, although arguably all configurations they claim to
> support should work.
> 
> Optimize eDP 1.4+ link config fast and narrow.
> 
> Side note: The implementation has a near duplicate of the link config
> function, with just the two inner for loops turned inside out. Perhaps
> there'd be a way to make this, say, more table driven to reduce the
> duplication, but seems like that would lead to duplication in the table
> generation. We'll also have to see how the link config optimization for
> DSC turns out.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matt Atwood <matthew.s.atwood@intel.com>
> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com
> 
> Untested. It's possible this helps the referenced bug. The downside is
> that this patch has a bunch of dependencies that are too much to
> backport to stable kernels. If the patch works, we may need to consider
> hacking together an uglier backport.
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..bf7b91832c8a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +/* Optimize link config in order: max bpp, min lanes, min clock */
> +static bool
> +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *pipe_config,
> +				  const struct link_config_limits *limits)
> +{
> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	int bpp, clock, lane_count;
> +	int mode_rate, link_clock, link_avail;
> +
> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> +						   bpp);
> +
> +		for (lane_count = limits->min_lane_count;
> +		     lane_count <= limits->max_lane_count;
> +		     lane_count <<= 1) {
> +			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> +				link_clock = intel_dp->common_rates[clock];
> +				link_avail = intel_dp_max_data_rate(link_clock,
> +								    lane_count);
> +
> +				if (mode_rate <= link_avail) {
> +					pipe_config->lane_count = lane_count;
> +					pipe_config->pipe_bpp = bpp;
> +					pipe_config->port_clock = link_clock;
> +
> +					return true;
> +				}
> +			}
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static bool
>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config)
> @@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	limits.min_bpp = 6 * 3;
>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  
> -	if (intel_dp_is_edp(intel_dp)) {
> +	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
>  		/*
>  		 * Use the maximum clock and number of lanes the eDP panel
> -		 * advertizes being capable of. The panels are generally
> -		 * designed to support only a single clock and lane
> -		 * configuration, and typically these values correspond to the
> -		 * native resolution of the panel.
> +		 * advertizes being capable of. The eDP 1.3 and earlier panels
> +		 * are generally designed to support only a single clock and
> +		 * lane configuration, and typically these values correspond to
> +		 * the native resolution of the panel. With eDP 1.4 rate select
> +		 * and DSC, this is decreasingly the case, and we need to be
> +		 * able to select less than maximum link config.
>  		 */
>  		limits.min_lane_count = limits.max_lane_count;
>  		limits.min_clock = limits.max_clock;
> @@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		      intel_dp->common_rates[limits.max_clock],
>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>  
> -	/*
> -	 * Optimize for slow and wide. This is the place to add alternative
> -	 * optimization policy.
> -	 */
> -	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> -		return false;
> +	if (intel_dp_is_edp(intel_dp)) {

Nitpick below:
Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below:

		if (intel_dp->edp_dpcd[0] < DP_EDP_14) {
			limits.min_lane_count = limits.max_lane_count;
			limits.min_clock = limits.max_clock;
		}

With that,

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> +		/*
> +		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> +		 * section A.1: "It is recommended that the minimum number of
> +		 * lanes be used, using the minimum link rate allowed for that
> +		 * lane configuration."
> +		 *
> +		 * Note that we use the max clock and lane count for eDP 1.3 and
> +		 * earlier, and fast vs. wide is irrelevant.
> +		 */
> +		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> +						       &limits))
> +			return false;
> +	} else {
> +		/* Optimize for slow and wide. */
> +		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> +						       &limits))
> +			return false;
> +	}
>  
>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
>  		      pipe_config->lane_count, pipe_config->port_clock,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
  2018-09-27 10:10 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
@ 2018-09-27 11:15   ` Jani Nikula
  2018-09-27 13:03     ` Manasi Navare
  0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2018-09-27 11:15 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx, Rodrigo Vivi

On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Wed, Sep 05, 2018 at 12:53:21PM +0300, Jani Nikula wrote:
>> We've opted to use the maximum link rate and lane count for eDP panels,
>> because typically the maximum supported configuration reported by the
>> panel has matched the native resolution requirements of the panel, and
>> optimizing the link has lead to problems.
>> 
>> With eDP 1.4 rate select method and DSC features, this is decreasingly
>> the case. There's a need to optimize the link parameters. Moreover,
>> already eDP 1.3 states fast link with fewer lanes is preferred over the
>> wide and slow. (Wide and slow should still be more reliable for longer
>> cable lengths.)
>> 
>> Additionally, there have been reports of panels failing on arbitrary
>> link configurations, although arguably all configurations they claim to
>> support should work.
>> 
>> Optimize eDP 1.4+ link config fast and narrow.
>> 
>> Side note: The implementation has a near duplicate of the link config
>> function, with just the two inner for loops turned inside out. Perhaps
>> there'd be a way to make this, say, more table driven to reduce the
>> duplication, but seems like that would lead to duplication in the table
>> generation. We'll also have to see how the link config optimization for
>> DSC turns out.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Matt Atwood <matthew.s.atwood@intel.com>
>> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
>> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com
>> 
>> Untested. It's possible this helps the referenced bug. The downside is
>> that this patch has a bunch of dependencies that are too much to
>> backport to stable kernels. If the patch works, we may need to consider
>> hacking together an uglier backport.
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 62 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 436c22de33b6..bf7b91832c8a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>>  	return false;
>>  }
>>  
>> +/* Optimize link config in order: max bpp, min lanes, min clock */
>> +static bool
>> +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>> +				  struct intel_crtc_state *pipe_config,
>> +				  const struct link_config_limits *limits)
>> +{
>> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>> +	int bpp, clock, lane_count;
>> +	int mode_rate, link_clock, link_avail;
>> +
>> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
>> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> +						   bpp);
>> +
>> +		for (lane_count = limits->min_lane_count;
>> +		     lane_count <= limits->max_lane_count;
>> +		     lane_count <<= 1) {
>> +			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
>> +				link_clock = intel_dp->common_rates[clock];
>> +				link_avail = intel_dp_max_data_rate(link_clock,
>> +								    lane_count);
>> +
>> +				if (mode_rate <= link_avail) {
>> +					pipe_config->lane_count = lane_count;
>> +					pipe_config->pipe_bpp = bpp;
>> +					pipe_config->port_clock = link_clock;
>> +
>> +					return true;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>  static bool
>>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  			     struct intel_crtc_state *pipe_config)
>> @@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  	limits.min_bpp = 6 * 3;
>>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>>  
>> -	if (intel_dp_is_edp(intel_dp)) {
>> +	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
>>  		/*
>>  		 * Use the maximum clock and number of lanes the eDP panel
>> -		 * advertizes being capable of. The panels are generally
>> -		 * designed to support only a single clock and lane
>> -		 * configuration, and typically these values correspond to the
>> -		 * native resolution of the panel.
>> +		 * advertizes being capable of. The eDP 1.3 and earlier panels
>> +		 * are generally designed to support only a single clock and
>> +		 * lane configuration, and typically these values correspond to
>> +		 * the native resolution of the panel. With eDP 1.4 rate select
>> +		 * and DSC, this is decreasingly the case, and we need to be
>> +		 * able to select less than maximum link config.
>>  		 */
>>  		limits.min_lane_count = limits.max_lane_count;
>>  		limits.min_clock = limits.max_clock;
>> @@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  		      intel_dp->common_rates[limits.max_clock],
>>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>>  
>> -	/*
>> -	 * Optimize for slow and wide. This is the place to add alternative
>> -	 * optimization policy.
>> -	 */
>> -	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
>> -		return false;
>> +	if (intel_dp_is_edp(intel_dp)) {
>
> Nitpick below:
> Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below:
>
> 		if (intel_dp->edp_dpcd[0] < DP_EDP_14) {
> 			limits.min_lane_count = limits.max_lane_count;
> 			limits.min_clock = limits.max_clock;
> 		}

Doing that would require duplicating the debug logging to both branches
of the if (intel_dp_is_edp(intel_dp)) condition.

BR,
Jani.

>
> With that,
>
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
>
> Manasi
>
>> +		/*
>> +		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
>> +		 * section A.1: "It is recommended that the minimum number of
>> +		 * lanes be used, using the minimum link rate allowed for that
>> +		 * lane configuration."
>> +		 *
>> +		 * Note that we use the max clock and lane count for eDP 1.3 and
>> +		 * earlier, and fast vs. wide is irrelevant.
>> +		 */
>> +		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
>> +						       &limits))
>> +			return false;
>> +	} else {
>> +		/* Optimize for slow and wide. */
>> +		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
>> +						       &limits))
>> +			return false;
>> +	}
>>  
>>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
>>  		      pipe_config->lane_count, pipe_config->port_clock,
>> -- 
>> 2.11.0
>> 

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

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

* Re: [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
  2018-09-27 11:15   ` Jani Nikula
@ 2018-09-27 13:03     ` Manasi Navare
  2018-09-28  6:52       ` Jani Nikula
  0 siblings, 1 reply; 10+ messages in thread
From: Manasi Navare @ 2018-09-27 13:03 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, Rodrigo Vivi

On Thu, Sep 27, 2018 at 02:15:26PM +0300, Jani Nikula wrote:
> On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Wed, Sep 05, 2018 at 12:53:21PM +0300, Jani Nikula wrote:
> >> We've opted to use the maximum link rate and lane count for eDP panels,
> >> because typically the maximum supported configuration reported by the
> >> panel has matched the native resolution requirements of the panel, and
> >> optimizing the link has lead to problems.
> >> 
> >> With eDP 1.4 rate select method and DSC features, this is decreasingly
> >> the case. There's a need to optimize the link parameters. Moreover,
> >> already eDP 1.3 states fast link with fewer lanes is preferred over the
> >> wide and slow. (Wide and slow should still be more reliable for longer
> >> cable lengths.)
> >> 
> >> Additionally, there have been reports of panels failing on arbitrary
> >> link configurations, although arguably all configurations they claim to
> >> support should work.
> >> 
> >> Optimize eDP 1.4+ link config fast and narrow.
> >> 
> >> Side note: The implementation has a near duplicate of the link config
> >> function, with just the two inner for loops turned inside out. Perhaps
> >> there'd be a way to make this, say, more table driven to reduce the
> >> duplication, but seems like that would lead to duplication in the table
> >> generation. We'll also have to see how the link config optimization for
> >> DSC turns out.
> >> 
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Manasi Navare <manasi.d.navare@intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: Matt Atwood <matthew.s.atwood@intel.com>
> >> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
> >> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

 
> >> ---
> >> 
> >> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com
> >> 
> >> Untested. It's possible this helps the referenced bug. The downside is
> >> that this patch has a bunch of dependencies that are too much to
> >> backport to stable kernels. If the patch works, we may need to consider
> >> hacking together an uglier backport.
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 62 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 436c22de33b6..bf7b91832c8a 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >>  	return false;
> >>  }
> >>  
> >> +/* Optimize link config in order: max bpp, min lanes, min clock */
> >> +static bool
> >> +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >> +				  struct intel_crtc_state *pipe_config,
> >> +				  const struct link_config_limits *limits)
> >> +{
> >> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >> +	int bpp, clock, lane_count;
> >> +	int mode_rate, link_clock, link_avail;
> >> +
> >> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> >> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> +						   bpp);
> >> +
> >> +		for (lane_count = limits->min_lane_count;
> >> +		     lane_count <= limits->max_lane_count;
> >> +		     lane_count <<= 1) {
> >> +			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> >> +				link_clock = intel_dp->common_rates[clock];
> >> +				link_avail = intel_dp_max_data_rate(link_clock,
> >> +								    lane_count);
> >> +
> >> +				if (mode_rate <= link_avail) {
> >> +					pipe_config->lane_count = lane_count;
> >> +					pipe_config->pipe_bpp = bpp;
> >> +					pipe_config->port_clock = link_clock;
> >> +
> >> +					return true;
> >> +				}
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >>  static bool
> >>  intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  			     struct intel_crtc_state *pipe_config)
> >> @@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  	limits.min_bpp = 6 * 3;
> >>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> >>  
> >> -	if (intel_dp_is_edp(intel_dp)) {
> >> +	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
> >>  		/*
> >>  		 * Use the maximum clock and number of lanes the eDP panel
> >> -		 * advertizes being capable of. The panels are generally
> >> -		 * designed to support only a single clock and lane
> >> -		 * configuration, and typically these values correspond to the
> >> -		 * native resolution of the panel.
> >> +		 * advertizes being capable of. The eDP 1.3 and earlier panels
> >> +		 * are generally designed to support only a single clock and
> >> +		 * lane configuration, and typically these values correspond to
> >> +		 * the native resolution of the panel. With eDP 1.4 rate select
> >> +		 * and DSC, this is decreasingly the case, and we need to be
> >> +		 * able to select less than maximum link config.
> >>  		 */
> >>  		limits.min_lane_count = limits.max_lane_count;
> >>  		limits.min_clock = limits.max_clock;
> >> @@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  		      intel_dp->common_rates[limits.max_clock],
> >>  		      limits.max_bpp, adjusted_mode->crtc_clock);
> >>  
> >> -	/*
> >> -	 * Optimize for slow and wide. This is the place to add alternative
> >> -	 * optimization policy.
> >> -	 */
> >> -	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> >> -		return false;
> >> +	if (intel_dp_is_edp(intel_dp)) {
> >
> > Nitpick below:
> > Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below:
> >
> > 		if (intel_dp->edp_dpcd[0] < DP_EDP_14) {
> > 			limits.min_lane_count = limits.max_lane_count;
> > 			limits.min_clock = limits.max_clock;
> > 		}
> 
> Doing that would require duplicating the debug logging to both branches
> of the if (intel_dp_is_edp(intel_dp)) condition.
> 
> BR,
> Jani.
>

Ok yes makes sense, now its common to both eDP and DP cases.

Manasi
 
> >
> > With that,
> >
> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> >
> > Manasi
> >
> >> +		/*
> >> +		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> >> +		 * section A.1: "It is recommended that the minimum number of
> >> +		 * lanes be used, using the minimum link rate allowed for that
> >> +		 * lane configuration."
> >> +		 *
> >> +		 * Note that we use the max clock and lane count for eDP 1.3 and
> >> +		 * earlier, and fast vs. wide is irrelevant.
> >> +		 */
> >> +		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> >> +						       &limits))
> >> +			return false;
> >> +	} else {
> >> +		/* Optimize for slow and wide. */
> >> +		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> >> +						       &limits))
> >> +			return false;
> >> +	}
> >>  
> >>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> >>  		      pipe_config->lane_count, pipe_config->port_clock,
> >> -- 
> >> 2.11.0
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow
  2018-09-27 13:03     ` Manasi Navare
@ 2018-09-28  6:52       ` Jani Nikula
  0 siblings, 0 replies; 10+ messages in thread
From: Jani Nikula @ 2018-09-28  6:52 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx, Rodrigo Vivi

On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Thu, Sep 27, 2018 at 02:15:26PM +0300, Jani Nikula wrote:
>> On Thu, 27 Sep 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > On Wed, Sep 05, 2018 at 12:53:21PM +0300, Jani Nikula wrote:
>> >> We've opted to use the maximum link rate and lane count for eDP panels,
>> >> because typically the maximum supported configuration reported by the
>> >> panel has matched the native resolution requirements of the panel, and
>> >> optimizing the link has lead to problems.
>> >> 
>> >> With eDP 1.4 rate select method and DSC features, this is decreasingly
>> >> the case. There's a need to optimize the link parameters. Moreover,
>> >> already eDP 1.3 states fast link with fewer lanes is preferred over the
>> >> wide and slow. (Wide and slow should still be more reliable for longer
>> >> cable lengths.)
>> >> 
>> >> Additionally, there have been reports of panels failing on arbitrary
>> >> link configurations, although arguably all configurations they claim to
>> >> support should work.
>> >> 
>> >> Optimize eDP 1.4+ link config fast and narrow.
>> >> 
>> >> Side note: The implementation has a near duplicate of the link config
>> >> function, with just the two inner for loops turned inside out. Perhaps
>> >> there'd be a way to make this, say, more table driven to reduce the
>> >> duplication, but seems like that would lead to duplication in the table
>> >> generation. We'll also have to see how the link config optimization for
>> >> DSC turns out.
>> >> 
>> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >> Cc: Matt Atwood <matthew.s.atwood@intel.com>
>> >> Cc: "Lee, Shawn C" <shawn.c.lee@intel.com>
>> >> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105267
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >>
>
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Thanks for the review, pushed to dinq.

BR,
Jani.


>
>  
>> >> ---
>> >> 
>> >> v2 of http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@intel.com
>> >> 
>> >> Untested. It's possible this helps the referenced bug. The downside is
>> >> that this patch has a bunch of dependencies that are too much to
>> >> backport to stable kernels. If the patch works, we may need to consider
>> >> hacking together an uglier backport.
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++++-------
>> >>  1 file changed, 62 insertions(+), 11 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> >> index 436c22de33b6..bf7b91832c8a 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> @@ -1921,6 +1921,42 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>> >>  	return false;
>> >>  }
>> >>  
>> >> +/* Optimize link config in order: max bpp, min lanes, min clock */
>> >> +static bool
>> >> +intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>> >> +				  struct intel_crtc_state *pipe_config,
>> >> +				  const struct link_config_limits *limits)
>> >> +{
>> >> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>> >> +	int bpp, clock, lane_count;
>> >> +	int mode_rate, link_clock, link_avail;
>> >> +
>> >> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
>> >> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> >> +						   bpp);
>> >> +
>> >> +		for (lane_count = limits->min_lane_count;
>> >> +		     lane_count <= limits->max_lane_count;
>> >> +		     lane_count <<= 1) {
>> >> +			for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
>> >> +				link_clock = intel_dp->common_rates[clock];
>> >> +				link_avail = intel_dp_max_data_rate(link_clock,
>> >> +								    lane_count);
>> >> +
>> >> +				if (mode_rate <= link_avail) {
>> >> +					pipe_config->lane_count = lane_count;
>> >> +					pipe_config->pipe_bpp = bpp;
>> >> +					pipe_config->port_clock = link_clock;
>> >> +
>> >> +					return true;
>> >> +				}
>> >> +			}
>> >> +		}
>> >> +	}
>> >> +
>> >> +	return false;
>> >> +}
>> >> +
>> >>  static bool
>> >>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>> >>  			     struct intel_crtc_state *pipe_config)
>> >> @@ -1945,13 +1981,15 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>> >>  	limits.min_bpp = 6 * 3;
>> >>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>> >>  
>> >> -	if (intel_dp_is_edp(intel_dp)) {
>> >> +	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
>> >>  		/*
>> >>  		 * Use the maximum clock and number of lanes the eDP panel
>> >> -		 * advertizes being capable of. The panels are generally
>> >> -		 * designed to support only a single clock and lane
>> >> -		 * configuration, and typically these values correspond to the
>> >> -		 * native resolution of the panel.
>> >> +		 * advertizes being capable of. The eDP 1.3 and earlier panels
>> >> +		 * are generally designed to support only a single clock and
>> >> +		 * lane configuration, and typically these values correspond to
>> >> +		 * the native resolution of the panel. With eDP 1.4 rate select
>> >> +		 * and DSC, this is decreasingly the case, and we need to be
>> >> +		 * able to select less than maximum link config.
>> >>  		 */
>> >>  		limits.min_lane_count = limits.max_lane_count;
>> >>  		limits.min_clock = limits.max_clock;
>> >> @@ -1965,12 +2003,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>> >>  		      intel_dp->common_rates[limits.max_clock],
>> >>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>> >>  
>> >> -	/*
>> >> -	 * Optimize for slow and wide. This is the place to add alternative
>> >> -	 * optimization policy.
>> >> -	 */
>> >> -	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
>> >> -		return false;
>> >> +	if (intel_dp_is_edp(intel_dp)) {
>> >
>> > Nitpick below:
>> > Can we nest the edp_rev < DP_EDP_14 condition within this if condition? Like below:
>> >
>> > 		if (intel_dp->edp_dpcd[0] < DP_EDP_14) {
>> > 			limits.min_lane_count = limits.max_lane_count;
>> > 			limits.min_clock = limits.max_clock;
>> > 		}
>> 
>> Doing that would require duplicating the debug logging to both branches
>> of the if (intel_dp_is_edp(intel_dp)) condition.
>> 
>> BR,
>> Jani.
>>
>
> Ok yes makes sense, now its common to both eDP and DP cases.
>
> Manasi
>  
>> >
>> > With that,
>> >
>> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
>> >
>> > Manasi
>> >
>> >> +		/*
>> >> +		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
>> >> +		 * section A.1: "It is recommended that the minimum number of
>> >> +		 * lanes be used, using the minimum link rate allowed for that
>> >> +		 * lane configuration."
>> >> +		 *
>> >> +		 * Note that we use the max clock and lane count for eDP 1.3 and
>> >> +		 * earlier, and fast vs. wide is irrelevant.
>> >> +		 */
>> >> +		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
>> >> +						       &limits))
>> >> +			return false;
>> >> +	} else {
>> >> +		/* Optimize for slow and wide. */
>> >> +		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
>> >> +						       &limits))
>> >> +			return false;
>> >> +	}
>> >>  
>> >>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
>> >>  		      pipe_config->lane_count, pipe_config->port_clock,
>> >> -- 
>> >> 2.11.0
>> >> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

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

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

end of thread, other threads:[~2018-09-28  6:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-05  9:53 [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Jani Nikula
2018-09-05 10:28 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
2018-09-05 12:02 ` ✓ Fi.CI.IGT: " Patchwork
2018-09-05 16:17 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
2018-09-24 10:35 ` ✓ Fi.CI.BAT: success for drm/i915/dp: optimize eDP 1.4+ link config fast and narrow (rev2) Patchwork
2018-09-24 11:53 ` ✓ Fi.CI.IGT: " Patchwork
2018-09-27 10:10 ` [PATCH] drm/i915/dp: optimize eDP 1.4+ link config fast and narrow Manasi Navare
2018-09-27 11:15   ` Jani Nikula
2018-09-27 13:03     ` Manasi Navare
2018-09-28  6:52       ` Jani Nikula

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.