All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/psr: vbt change for psr
@ 2018-05-02  9:13 vathsala nagaraju
  2018-05-02 12:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: vbt change for psr (rev4) Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: vathsala nagaraju @ 2018-05-02  9:13 UTC (permalink / raw)
  To: rodrigo.vivi, jani.nikula, intel-gfx
  Cc: Puthikorn Voravootivat, Maulik V Vaghela

From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

For psr block #9, the vbt description has moved to options [0-3] for
TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
structure. Since spec does not  mention from which VBT version this
change was added to vbt.bsf file, we cannot depend on bdb->version check
to change for all the platforms.

There is RCR inplace for GOP team to  provide the version number
to make generic change. Since Kabylake with bdb version 209 is having this
change, limiting this change to kbl and version 209+ to unblock google.

Tested on skl(bdb version 203,without options) and
kabylake(bdb version 209,212) having new options.

bspec 20131

v2: (Jani and Rodrigo)
    move the 165 version check to intel_bios.c
v3: Jani
    Move the abstraction to intel_bios.
v4: Jani
    Rename tp*_wakeup_time to have "us" suffix.
    For values outside range[0-3],default to max 2500us.
    Old decimal value was wake up time in multiples of 100us.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Puthikorn Voravootivat <puthik@chromium.org>

Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
 drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
 drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
 4 files changed, 68 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6268a51..a189382 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1077,8 +1077,8 @@ struct intel_vbt_data {
 		bool require_aux_wakeup;
 		int idle_frames;
 		enum psr_lines_to_wait lines_to_wait;
-		int tp1_wakeup_time;
-		int tp2_tp3_wakeup_time;
+		int tp1_wakeup_time_us;
+		int tp2_tp3_wakeup_time_us;
 	} psr;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 197c966..6bbd0b4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4084,10 +4084,10 @@ enum {
 #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
 #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
-#define   EDP_PSR2_TP2_TIME_500		(0<<8)
-#define   EDP_PSR2_TP2_TIME_100		(1<<8)
-#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
-#define   EDP_PSR2_TP2_TIME_50		(3<<8)
+#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
+#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
+#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
+#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
 #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
 #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
 #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 702d3fa..a246b6b 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
 		break;
 	}
 
-	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
-	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
+	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
+	 * Old decimal value is wake up time in multiples of 100 us.
+	 */
+	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {
+		switch (psr_table->tp1_wakeup_time) {
+		case 0:
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
+			break;
+		case 1:
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
+			break;
+		case 3:
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
+			break;
+		case 2:
+		default:
+			if (psr_table->tp1_wakeup_time != 2)
+				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
+			break;
+		}
+
+		switch (psr_table->tp2_tp3_wakeup_time) {
+		case 0:
+			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
+			break;
+		case 1:
+			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
+			break;
+		case 3:
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
+			break;
+		case 2:
+		default:
+			if (psr_table->tp2_tp3_wakeup_time != 2)
+				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
+			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
+		break;
+		}
+	} else {
+		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
+		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
+	}
 }
 
 static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6233a32..746c5ac 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	if (dev_priv->psr.link_standby)
 		val |= EDP_PSR_LINK_STANDBY;
 
-	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
-		val |= EDP_PSR_TP1_TIME_2500us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
-		val |= EDP_PSR_TP1_TIME_500us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
+	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)
+		val |=  EDP_PSR_TP1_TIME_0us;
+	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
 		val |= EDP_PSR_TP1_TIME_100us;
+	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
+		val |= EDP_PSR_TP1_TIME_500us;
 	else
-		val |= EDP_PSR_TP1_TIME_0us;
+		val |= EDP_PSR_TP1_TIME_2500us;
 
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
-		val |= EDP_PSR_TP2_TP3_TIME_2500us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
-		val |= EDP_PSR_TP2_TP3_TIME_500us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
+	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)
+		val |=  EDP_PSR_TP2_TP3_TIME_0us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
 		val |= EDP_PSR_TP2_TP3_TIME_100us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
+		val |= EDP_PSR_TP2_TP3_TIME_500us;
 	else
-		val |= EDP_PSR_TP2_TP3_TIME_0us;
+		val |= EDP_PSR_TP2_TP3_TIME_2500us;
 
 	if (intel_dp_source_supports_hbr2(intel_dp) &&
 	    drm_dp_tps3_supported(intel_dp->dpcd))
@@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
 
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
-		val |= EDP_PSR2_TP2_TIME_2500;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
-		val |= EDP_PSR2_TP2_TIME_500;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
-		val |= EDP_PSR2_TP2_TIME_100;
+	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
+		val |= EDP_PSR2_TP2_TIME_50us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
+		val |= EDP_PSR2_TP2_TIME_100us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
+		val |= EDP_PSR2_TP2_TIME_500us;
 	else
-		val |= EDP_PSR2_TP2_TIME_50;
+		val |= EDP_PSR2_TP2_TIME_2500us;
 
 	I915_WRITE(EDP_PSR2_CTL, val);
 }
-- 
1.9.1

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

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

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: vbt change for psr (rev4)
  2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
@ 2018-05-02 12:44 ` Patchwork
  2018-05-02 13:04 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-05-02 12:44 UTC (permalink / raw)
  To: vathsala nagaraju; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: vbt change for psr (rev4)
URL   : https://patchwork.freedesktop.org/series/41289/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3650bcd5ccb9 drm/i915/psr: vbt change for psr
-:63: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#63: FILE: drivers/gpu/drm/i915/i915_reg.h:4087:
+#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
                                  	  ^

-:64: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#64: FILE: drivers/gpu/drm/i915/i915_reg.h:4088:
+#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
                                  	  ^

-:65: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#65: FILE: drivers/gpu/drm/i915/i915_reg.h:4089:
+#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
                                   	  ^

-:66: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#66: FILE: drivers/gpu/drm/i915/i915_reg.h:4090:
+#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
                                 	  ^

total: 0 errors, 0 warnings, 4 checks, 131 lines checked

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

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

* ✓ Fi.CI.BAT: success for drm/i915/psr: vbt change for psr (rev4)
  2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
  2018-05-02 12:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: vbt change for psr (rev4) Patchwork
@ 2018-05-02 13:04 ` Patchwork
  2018-05-02 17:04 ` ✓ Fi.CI.IGT: " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-05-02 13:04 UTC (permalink / raw)
  To: vathsala nagaraju; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: vbt change for psr (rev4)
URL   : https://patchwork.freedesktop.org/series/41289/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4121 -> Patchwork_8870 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8870 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8870, 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/41289/revisions/4/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL (fdo#100368)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-cnl-y3:          PASS -> DMESG-WARN (fdo#104951)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951


== Participating hosts (40 -> 37) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8870

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8870: 3650bcd5ccb9c37d1a01c6791448b0e169925e74 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

3650bcd5ccb9 drm/i915/psr: vbt change for psr

== Logs ==

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

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

* ✓ Fi.CI.IGT: success for drm/i915/psr: vbt change for psr (rev4)
  2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
  2018-05-02 12:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: vbt change for psr (rev4) Patchwork
  2018-05-02 13:04 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-05-02 17:04 ` Patchwork
  2018-05-02 21:15 ` [PATCH] drm/i915/psr: vbt change for psr Rodrigo Vivi
  2018-05-03  7:07 ` Jani Nikula
  4 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-05-02 17:04 UTC (permalink / raw)
  To: vathsala nagaraju; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: vbt change for psr (rev4)
URL   : https://patchwork.freedesktop.org/series/41289/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4121_full -> Patchwork_8870_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8870_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8870_full, 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/41289/revisions/4/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-apl:          PASS -> FAIL (fdo#103375)

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          PASS -> FAIL (fdo#103060)

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

    igt@kms_rotation_crc@primary-rotation-270:
      shard-apl:          PASS -> DMESG-WARN (fdo#105127)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@modeset-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-cpu:
      shard-apl:          FAIL (fdo#103167) -> PASS

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#105127 https://bugs.freedesktop.org/show_bug.cgi?id=105127
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363


== Participating hosts (7 -> 6) ==

  Missing    (1): shard-kbl 


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8870

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8870: 3650bcd5ccb9c37d1a01c6791448b0e169925e74 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

* Re: [PATCH] drm/i915/psr: vbt change for psr
  2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
                   ` (2 preceding siblings ...)
  2018-05-02 17:04 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-05-02 21:15 ` Rodrigo Vivi
  2018-05-03  3:21   ` vathsala nagaraju
  2018-05-03  6:59   ` Jani Nikula
  2018-05-03  7:07 ` Jani Nikula
  4 siblings, 2 replies; 8+ messages in thread
From: Rodrigo Vivi @ 2018-05-02 21:15 UTC (permalink / raw)
  To: vathsala nagaraju; +Cc: Puthikorn Voravootivat, intel-gfx, Maulik V Vaghela

On Wed, May 02, 2018 at 02:43:29PM +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> For psr block #9, the vbt description has moved to options [0-3] for
> TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
> structure. Since spec does not  mention from which VBT version this
> change was added to vbt.bsf file, we cannot depend on bdb->version check
> to change for all the platforms.
> 
> There is RCR inplace for GOP team to  provide the version number
> to make generic change. Since Kabylake with bdb version 209 is having this
> change, limiting this change to kbl and version 209+ to unblock google.
> 
> Tested on skl(bdb version 203,without options) and
> kabylake(bdb version 209,212) having new options.
> 
> bspec 20131
> 
> v2: (Jani and Rodrigo)
>     move the 165 version check to intel_bios.c
> v3: Jani
>     Move the abstraction to intel_bios.
> v4: Jani
>     Rename tp*_wakeup_time to have "us" suffix.
>     For values outside range[0-3],default to max 2500us.
>     Old decimal value was wake up time in multiples of 100us.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Puthikorn Voravootivat <puthik@chromium.org>
> 
> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
>  drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
>  drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
>  4 files changed, 68 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6268a51..a189382 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
>  		bool require_aux_wakeup;
>  		int idle_frames;
>  		enum psr_lines_to_wait lines_to_wait;
> -		int tp1_wakeup_time;
> -		int tp2_tp3_wakeup_time;
> +		int tp1_wakeup_time_us;
> +		int tp2_tp3_wakeup_time_us;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 197c966..6bbd0b4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4084,10 +4084,10 @@ enum {
>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
> -#define   EDP_PSR2_TP2_TIME_500		(0<<8)
> -#define   EDP_PSR2_TP2_TIME_100		(1<<8)
> -#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
> -#define   EDP_PSR2_TP2_TIME_50		(3<<8)
> +#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
> +#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
> +#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
> +#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
>  #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 702d3fa..a246b6b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
> -	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
> +	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
> +	 * Old decimal value is wake up time in multiples of 100 us.
> +	 */

Please follow this style with first empty line:

/*
 * Text here
 */

Start with capital N on New.

> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {

Why are we filtering per platform?
What about Coffelake? And Geminilake? And Cannonlake? and on?

Btw, where is this spec? I just checked predator and it still
only contains the old multiple of 100 one... :(

> +		switch (psr_table->tp1_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:

this out of order case is not good imho...
confusing... but well... 0 -> 2500 -> 100 -> 500 is already confusing by itself
honestly...

> +		default:

why default is 2? higher?
I always thought this awkward order was to make the 0 the default with 500
or at least I was trying to make some sense on that order :/

> +			if (psr_table->tp1_wakeup_time != 2)
> +				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
> +		switch (psr_table->tp2_tp3_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:
> +		default:
> +			if (psr_table->tp2_tp3_wakeup_time != 2)
> +				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
> +		break;
> +		}
> +	} else {
> +		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
> +		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
> +	}
>  }
>  
>  static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6233a32..746c5ac 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	if (dev_priv->psr.link_standby)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
> -	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
> -		val |= EDP_PSR_TP1_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
> -		val |= EDP_PSR_TP1_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)
> +		val |=  EDP_PSR_TP1_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP1_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP1_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP1_TIME_0us;
> +		val |= EDP_PSR_TP1_TIME_2500us;
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)
> +		val |=  EDP_PSR_TP2_TP3_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP2_TP3_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>  
>  	if (intel_dp_source_supports_hbr2(intel_dp) &&
>  	    drm_dp_tps3_supported(intel_dp->dpcd))
> @@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR2_TP2_TIME_2500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR2_TP2_TIME_500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> -		val |= EDP_PSR2_TP2_TIME_100;
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> +		val |= EDP_PSR2_TP2_TIME_50us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
> +		val |= EDP_PSR2_TP2_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR2_TP2_TIME_500us;
>  	else
> -		val |= EDP_PSR2_TP2_TIME_50;
> +		val |= EDP_PSR2_TP2_TIME_2500us;
>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: vbt change for psr
  2018-05-02 21:15 ` [PATCH] drm/i915/psr: vbt change for psr Rodrigo Vivi
@ 2018-05-03  3:21   ` vathsala nagaraju
  2018-05-03  6:59   ` Jani Nikula
  1 sibling, 0 replies; 8+ messages in thread
From: vathsala nagaraju @ 2018-05-03  3:21 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: Puthikorn Voravootivat, intel-gfx, Maulik V Vaghela

On Thursday 03 May 2018 02:45 AM, Rodrigo Vivi wrote:
> On Wed, May 02, 2018 at 02:43:29PM +0530, vathsala nagaraju wrote:
>> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>
>> For psr block #9, the vbt description has moved to options [0-3] for
>> TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
>> structure. Since spec does not  mention from which VBT version this
>> change was added to vbt.bsf file, we cannot depend on bdb->version check
>> to change for all the platforms.
>>
>> There is RCR inplace for GOP team to  provide the version number
>> to make generic change. Since Kabylake with bdb version 209 is having this
>> change, limiting this change to kbl and version 209+ to unblock google.
>>
>> Tested on skl(bdb version 203,without options) and
>> kabylake(bdb version 209,212) having new options.
>>
>> bspec 20131
>>
>> v2: (Jani and Rodrigo)
>>      move the 165 version check to intel_bios.c
>> v3: Jani
>>      Move the abstraction to intel_bios.
>> v4: Jani
>>      Rename tp*_wakeup_time to have "us" suffix.
>>      For values outside range[0-3],default to max 2500us.
>>      Old decimal value was wake up time in multiples of 100us.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> CC: Puthikorn Voravootivat <puthik@chromium.org>
>>
>> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
>>   drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
>>   drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
>>   4 files changed, 68 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6268a51..a189382 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
>>   		bool require_aux_wakeup;
>>   		int idle_frames;
>>   		enum psr_lines_to_wait lines_to_wait;
>> -		int tp1_wakeup_time;
>> -		int tp2_tp3_wakeup_time;
>> +		int tp1_wakeup_time_us;
>> +		int tp2_tp3_wakeup_time_us;
>>   	} psr;
>>   
>>   	struct {
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 197c966..6bbd0b4 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4084,10 +4084,10 @@ enum {
>>   #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>>   #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>>   #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
>> -#define   EDP_PSR2_TP2_TIME_500		(0<<8)
>> -#define   EDP_PSR2_TP2_TIME_100		(1<<8)
>> -#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
>> -#define   EDP_PSR2_TP2_TIME_50		(3<<8)
>> +#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
>> +#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
>> +#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
>> +#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
>>   #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>>   #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>>   #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 702d3fa..a246b6b 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>>   		break;
>>   	}
>>   
>> -	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
>> -	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
>> +	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
>> +	 * Old decimal value is wake up time in multiples of 100 us.
>> +	 */
> Please follow this style with first empty line:
>
> /*
>   * Text here
>   */
>
> Start with capital N on New.
Thanks, will make the changes.
>
>> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {
> Why are we filtering per platform?

What about Coffelake? And Geminilake? And Cannonlake? and on?

Once GOP team confirms the exact versions for other platforms, we will 
add them.
Please check my commit message.
>
> Btw, where is this spec? I just checked predator and it still
> only contains the old multiple of 100 one... :(
please check bspec 20131
>
>> +		switch (psr_table->tp1_wakeup_time) {
>> +		case 0:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
>> +			break;
>> +		case 1:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
>> +			break;
>> +		case 3:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
>> +			break;
>> +		case 2:
> this out of order case is not good imho...
> confusing... but well... 0 -> 2500 -> 100 -> 500 is already confusing by itself
> honestly...
>
>> +		default:
> why default is 2? higher?
> I always thought this awkward order was to make the 0 the default with 500
> or at least I was trying to make some sense on that order :/
Jani  suggested for 2500 for anything out of range [0-3]. Please check 
the thread https://patchwork.freedesktop.org/patch/217851/ , last 
message from Jani.
Since 2=2500 and outside range = 2500, I have ordered it that way in the 
code.

>
>> +			if (psr_table->tp1_wakeup_time != 2)
>> +				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
>> +			break;
>> +		}
>> +
>> +		switch (psr_table->tp2_tp3_wakeup_time) {
>> +		case 0:
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
>> +			break;
>> +		case 1:
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
>> +			break;
>> +		case 3:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
>> +			break;
>> +		case 2:
>> +		default:
>> +			if (psr_table->tp2_tp3_wakeup_time != 2)
>> +				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
>> +		break;
>> +		}
>> +	} else {
>> +		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
>> +		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
>> +	}
>>   }
>>   
>>   static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 6233a32..746c5ac 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>>   	if (dev_priv->psr.link_standby)
>>   		val |= EDP_PSR_LINK_STANDBY;
>>   
>> -	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
>> -		val |= EDP_PSR_TP1_TIME_2500us;
>> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
>> -		val |= EDP_PSR_TP1_TIME_500us;
>> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
>> +	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)
>> +		val |=  EDP_PSR_TP1_TIME_0us;
>> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
>>   		val |= EDP_PSR_TP1_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
>> +		val |= EDP_PSR_TP1_TIME_500us;
>>   	else
>> -		val |= EDP_PSR_TP1_TIME_0us;
>> +		val |= EDP_PSR_TP1_TIME_2500us;
>>   
>> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
>> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
>> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)
>> +		val |=  EDP_PSR_TP2_TP3_TIME_0us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>>   		val |= EDP_PSR_TP2_TP3_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
>> +		val |= EDP_PSR_TP2_TP3_TIME_500us;
>>   	else
>> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
>> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>>   
>>   	if (intel_dp_source_supports_hbr2(intel_dp) &&
>>   	    drm_dp_tps3_supported(intel_dp->dpcd))
>> @@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>>   
>>   	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>>   
>> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>> -		val |= EDP_PSR2_TP2_TIME_2500;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
>> -		val |= EDP_PSR2_TP2_TIME_500;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
>> -		val |= EDP_PSR2_TP2_TIME_100;
>> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
>> +		val |= EDP_PSR2_TP2_TIME_50us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>> +		val |= EDP_PSR2_TP2_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
>> +		val |= EDP_PSR2_TP2_TIME_500us;
>>   	else
>> -		val |= EDP_PSR2_TP2_TIME_50;
>> +		val |= EDP_PSR2_TP2_TIME_2500us;
>>   
>>   	I915_WRITE(EDP_PSR2_CTL, val);
>>   }
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

* Re: [PATCH] drm/i915/psr: vbt change for psr
  2018-05-02 21:15 ` [PATCH] drm/i915/psr: vbt change for psr Rodrigo Vivi
  2018-05-03  3:21   ` vathsala nagaraju
@ 2018-05-03  6:59   ` Jani Nikula
  1 sibling, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2018-05-03  6:59 UTC (permalink / raw)
  To: Rodrigo Vivi, vathsala nagaraju
  Cc: Puthikorn Voravootivat, intel-gfx, Maulik V Vaghela

On Wed, 02 May 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Wed, May 02, 2018 at 02:43:29PM +0530, vathsala nagaraju wrote:
>> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> 
>> For psr block #9, the vbt description has moved to options [0-3] for
>> TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
>> structure. Since spec does not  mention from which VBT version this
>> change was added to vbt.bsf file, we cannot depend on bdb->version check
>> to change for all the platforms.
>> 
>> There is RCR inplace for GOP team to  provide the version number
>> to make generic change. Since Kabylake with bdb version 209 is having this
>> change, limiting this change to kbl and version 209+ to unblock google.
>> 
>> Tested on skl(bdb version 203,without options) and
>> kabylake(bdb version 209,212) having new options.
>> 
>> bspec 20131
>> 
>> v2: (Jani and Rodrigo)
>>     move the 165 version check to intel_bios.c
>> v3: Jani
>>     Move the abstraction to intel_bios.
>> v4: Jani
>>     Rename tp*_wakeup_time to have "us" suffix.
>>     For values outside range[0-3],default to max 2500us.
>>     Old decimal value was wake up time in multiples of 100us.
>> 
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> CC: Puthikorn Voravootivat <puthik@chromium.org>
>> 
>> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
>>  drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
>>  drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
>>  4 files changed, 68 insertions(+), 27 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6268a51..a189382 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
>>  		bool require_aux_wakeup;
>>  		int idle_frames;
>>  		enum psr_lines_to_wait lines_to_wait;
>> -		int tp1_wakeup_time;
>> -		int tp2_tp3_wakeup_time;
>> +		int tp1_wakeup_time_us;
>> +		int tp2_tp3_wakeup_time_us;
>>  	} psr;
>>  
>>  	struct {
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 197c966..6bbd0b4 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4084,10 +4084,10 @@ enum {
>>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
>> -#define   EDP_PSR2_TP2_TIME_500		(0<<8)
>> -#define   EDP_PSR2_TP2_TIME_100		(1<<8)
>> -#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
>> -#define   EDP_PSR2_TP2_TIME_50		(3<<8)
>> +#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
>> +#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
>> +#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
>> +#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
>>  #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 702d3fa..a246b6b 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>>  		break;
>>  	}
>>  
>> -	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
>> -	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
>> +	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
>> +	 * Old decimal value is wake up time in multiples of 100 us.
>> +	 */
>
> Please follow this style with first empty line:
>
> /*
>  * Text here
>  */
>
> Start with capital N on New.
>
>> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {
>
> Why are we filtering per platform?
> What about Coffelake? And Geminilake? And Cannonlake? and on?

It's a mess. I hate it, but I guess no can do.

>
> Btw, where is this spec? I just checked predator and it still
> only contains the old multiple of 100 one... :(
>
>> +		switch (psr_table->tp1_wakeup_time) {
>> +		case 0:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
>> +			break;
>> +		case 1:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
>> +			break;
>> +		case 3:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
>> +			break;
>> +		case 2:
>
> this out of order case is not good imho...
> confusing... but well... 0 -> 2500 -> 100 -> 500 is already confusing by itself
> honestly...
>
>> +		default:
>
> why default is 2? higher?
> I always thought this awkward order was to make the 0 the default with 500
> or at least I was trying to make some sense on that order :/

The spec is silly. IMO they should have stuck to using usecs
directly. If the driver has to do a per platform mapping *anyway*, it's
better to do it from usecs directly than from some platform specific
index values.

Defaulting to longest when out of range is per my instruction. It should
handle the cases where the VBT actually has the usecs/100 value but
should have the new style. Did I mention this is a mess already?

BR,
Jani.

>
>> +			if (psr_table->tp1_wakeup_time != 2)
>> +				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
>> +			break;
>> +		}
>> +
>> +		switch (psr_table->tp2_tp3_wakeup_time) {
>> +		case 0:
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
>> +			break;
>> +		case 1:
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
>> +			break;
>> +		case 3:
>> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
>> +			break;
>> +		case 2:
>> +		default:
>> +			if (psr_table->tp2_tp3_wakeup_time != 2)
>> +				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
>> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
>> +		break;
>> +		}
>> +	} else {
>> +		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
>> +		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
>> +	}
>>  }
>>  
>>  static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 6233a32..746c5ac 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>>  	if (dev_priv->psr.link_standby)
>>  		val |= EDP_PSR_LINK_STANDBY;
>>  
>> -	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
>> -		val |= EDP_PSR_TP1_TIME_2500us;
>> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
>> -		val |= EDP_PSR_TP1_TIME_500us;
>> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
>> +	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)
>> +		val |=  EDP_PSR_TP1_TIME_0us;
>> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
>>  		val |= EDP_PSR_TP1_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
>> +		val |= EDP_PSR_TP1_TIME_500us;
>>  	else
>> -		val |= EDP_PSR_TP1_TIME_0us;
>> +		val |= EDP_PSR_TP1_TIME_2500us;
>>  
>> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
>> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
>> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)
>> +		val |=  EDP_PSR_TP2_TP3_TIME_0us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
>> +		val |= EDP_PSR_TP2_TP3_TIME_500us;
>>  	else
>> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
>> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>>  
>>  	if (intel_dp_source_supports_hbr2(intel_dp) &&
>>  	    drm_dp_tps3_supported(intel_dp->dpcd))
>> @@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>>  
>>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>>  
>> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>> -		val |= EDP_PSR2_TP2_TIME_2500;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
>> -		val |= EDP_PSR2_TP2_TIME_500;
>> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
>> -		val |= EDP_PSR2_TP2_TIME_100;
>> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
>> +		val |= EDP_PSR2_TP2_TIME_50us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>> +		val |= EDP_PSR2_TP2_TIME_100us;
>> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
>> +		val |= EDP_PSR2_TP2_TIME_500us;
>>  	else
>> -		val |= EDP_PSR2_TP2_TIME_50;
>> +		val |= EDP_PSR2_TP2_TIME_2500us;
>>  
>>  	I915_WRITE(EDP_PSR2_CTL, val);
>>  }
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

* Re: [PATCH] drm/i915/psr: vbt change for psr
  2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
                   ` (3 preceding siblings ...)
  2018-05-02 21:15 ` [PATCH] drm/i915/psr: vbt change for psr Rodrigo Vivi
@ 2018-05-03  7:07 ` Jani Nikula
  4 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2018-05-03  7:07 UTC (permalink / raw)
  To: vathsala nagaraju, rodrigo.vivi, intel-gfx
  Cc: Puthikorn Voravootivat, Maulik V Vaghela

On Wed, 02 May 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>
> For psr block #9, the vbt description has moved to options [0-3] for
> TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
> structure. Since spec does not  mention from which VBT version this
> change was added to vbt.bsf file, we cannot depend on bdb->version check
> to change for all the platforms.
>
> There is RCR inplace for GOP team to  provide the version number
> to make generic change. Since Kabylake with bdb version 209 is having this
> change, limiting this change to kbl and version 209+ to unblock google.
>
> Tested on skl(bdb version 203,without options) and
> kabylake(bdb version 209,212) having new options.
>
> bspec 20131
>
> v2: (Jani and Rodrigo)
>     move the 165 version check to intel_bios.c
> v3: Jani
>     Move the abstraction to intel_bios.
> v4: Jani
>     Rename tp*_wakeup_time to have "us" suffix.
>     For values outside range[0-3],default to max 2500us.
>     Old decimal value was wake up time in multiples of 100us.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Puthikorn Voravootivat <puthik@chromium.org>
>
> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
>  drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
>  drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
>  4 files changed, 68 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6268a51..a189382 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
>  		bool require_aux_wakeup;
>  		int idle_frames;
>  		enum psr_lines_to_wait lines_to_wait;
> -		int tp1_wakeup_time;
> -		int tp2_tp3_wakeup_time;
> +		int tp1_wakeup_time_us;
> +		int tp2_tp3_wakeup_time_us;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 197c966..6bbd0b4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4084,10 +4084,10 @@ enum {
>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
> -#define   EDP_PSR2_TP2_TIME_500		(0<<8)
> -#define   EDP_PSR2_TP2_TIME_100		(1<<8)
> -#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
> -#define   EDP_PSR2_TP2_TIME_50		(3<<8)
> +#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
> +#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
> +#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
> +#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
>  #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 702d3fa..a246b6b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
> -	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
> +	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
> +	 * Old decimal value is wake up time in multiples of 100 us.
> +	 */
> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {
> +		switch (psr_table->tp1_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:
> +		default:
> +			if (psr_table->tp1_wakeup_time != 2)

You already have the "!= 2" and "== 2" differentiation above. Use
it. (Hint: fallthrough.)

> +				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");

If you complain about out of range value, please log the value.

> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
> +		switch (psr_table->tp2_tp3_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:
> +		default:
> +			if (psr_table->tp2_tp3_wakeup_time != 2)
> +				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
> +		break;
> +		}

That's the same thing copy-pasted twice. If you don't fix it now, I'm
pretty sure the next person to stumble on this will send a patch to fix
it.

> +	} else {
> +		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
> +		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
> +	}
>  }
>  
>  static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6233a32..746c5ac 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	if (dev_priv->psr.link_standby)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
> -	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
> -		val |= EDP_PSR_TP1_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
> -		val |= EDP_PSR_TP1_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)

== 0. It should never be < 0, and if it is, go for the longest delay.

> +		val |=  EDP_PSR_TP1_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP1_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP1_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP1_TIME_0us;
> +		val |= EDP_PSR_TP1_TIME_2500us;
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)

Same here.

> +		val |=  EDP_PSR_TP2_TP3_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP2_TP3_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>  
>  	if (intel_dp_source_supports_hbr2(intel_dp) &&
>  	    drm_dp_tps3_supported(intel_dp->dpcd))
> @@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR2_TP2_TIME_2500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR2_TP2_TIME_500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> -		val |= EDP_PSR2_TP2_TIME_100;
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> +		val |= EDP_PSR2_TP2_TIME_50us;

So I didn't actually look at bspec now. But the they've got to be
kidding me. They first move away from specifying usecs in the bspec,
because hardware only supports certain values. And this looks like they
didn't take into account the 50 us delay? Really? Ugh.

BR,
Jani.

> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
> +		val |= EDP_PSR2_TP2_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR2_TP2_TIME_500us;
>  	else
> -		val |= EDP_PSR2_TP2_TIME_50;
> +		val |= EDP_PSR2_TP2_TIME_2500us;
>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }

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

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

end of thread, other threads:[~2018-05-03  7:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-02  9:13 [PATCH] drm/i915/psr: vbt change for psr vathsala nagaraju
2018-05-02 12:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: vbt change for psr (rev4) Patchwork
2018-05-02 13:04 ` ✓ Fi.CI.BAT: success " Patchwork
2018-05-02 17:04 ` ✓ Fi.CI.IGT: " Patchwork
2018-05-02 21:15 ` [PATCH] drm/i915/psr: vbt change for psr Rodrigo Vivi
2018-05-03  3:21   ` vathsala nagaraju
2018-05-03  6:59   ` Jani Nikula
2018-05-03  7:07 ` 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.