[CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Paulo Zanoni]

From: Manasi Navare <manasi.d.navare@intel.com>

For ICL, on Combo PHY the allowed max rates are:
 - HBR3 8.1 eDP (DDIA)
 - HBR2 5.4 DisplayPort (DDIB)
and for MG PHY/TC DDI Ports allowed DP rates are:
 - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
 - DP on legacy connector - DDIC/D/E/F)

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: James Ausmus <james.ausmus@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
[Paulo: bikeshed to keep future platforms on "else".]
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 37b9f62aeb6e..8371159cc192 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp)
 	return 810000;
 }
 
+static int icl_max_source_rate(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	enum port port = dig_port->base.port;
+
+	/* On Combo PHY port A max speed is HBR3 for all Vccio voltages
+	 * and on Combo PHY Port B the maximum supported is HBR2.
+	 */
+	if (port == PORT_B)
+		return 540000;
+
+	return 810000;
+}
+
 static void
 intel_dp_set_source_rates(struct intel_dp *intel_dp)
 {
@@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp)
 	/* This should only be done once */
 	WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates);
 
-	if (IS_CANNONLAKE(dev_priv)) {
+	if (INTEL_GEN(dev_priv) >= 10) {
 		source_rates = cnl_rates;
 		size = ARRAY_SIZE(cnl_rates);
-		max_rate = cnl_max_source_rate(intel_dp);
+		if (INTEL_GEN(dev_priv) == 10)
+			max_rate = cnl_max_source_rate(intel_dp);
+		else
+			max_rate = icl_max_source_rate(intel_dp);
 	} else if (IS_GEN9_LP(dev_priv)) {
 		source_rates = bxt_rates;
 		size = ARRAY_SIZE(bxt_rates);
-- 
2.14.4

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

[CI 2/2] drm/i915/dp: Add support for HBR3 and TPS4 during link training

[Paulo Zanoni]

From: Manasi Navare <manasi.d.navare@intel.com>

DP spec 1.4 supports training pattern set 4 (TPS4) for HBR3 link
rate. This will be used in link training's channel equalization
phase if supported by both source and sink.
This patch adds the helpers to check if HBR3 is supported and uses
TPS4 in training pattern selection during link training.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: James Ausmus <james.ausmus@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h               |  1 +
 drivers/gpu/drm/i915/intel_dp.c               | 17 +++++++++---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 39 +++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h              |  1 +
 4 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 311c2a15f662..5828a1bdee21 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8692,6 +8692,7 @@ enum skl_power_gate {
 #define  DP_TP_CTL_LINK_TRAIN_PAT1		(0<<8)
 #define  DP_TP_CTL_LINK_TRAIN_PAT2		(1<<8)
 #define  DP_TP_CTL_LINK_TRAIN_PAT3		(4<<8)
+#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)
 #define  DP_TP_CTL_LINK_TRAIN_IDLE		(2<<8)
 #define  DP_TP_CTL_LINK_TRAIN_NORMAL		(3<<8)
 #define  DP_TP_CTL_SCRAMBLE_DISABLE		(1<<7)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8371159cc192..7e271cb6349c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1558,6 +1558,13 @@ bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
 	return max_rate >= 540000;
 }
 
+bool intel_dp_source_supports_hbr3(struct intel_dp *intel_dp)
+{
+	int max_rate = intel_dp->source_rates[intel_dp->num_source_rates - 1];
+
+	return max_rate >= 810000;
+}
+
 static void
 intel_dp_set_clock(struct intel_encoder *encoder,
 		   struct intel_crtc_state *pipe_config)
@@ -2883,10 +2890,11 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	enum port port = intel_dig_port->base.port;
+	uint8_t train_pat_mask = drm_dp_training_pattern_mask(intel_dp->dpcd);
 
-	if (dp_train_pat & DP_TRAINING_PATTERN_MASK)
+	if (dp_train_pat & train_pat_mask)
 		DRM_DEBUG_KMS("Using DP training pattern TPS%d\n",
-			      dp_train_pat & DP_TRAINING_PATTERN_MASK);
+			      dp_train_pat & train_pat_mask);
 
 	if (HAS_DDI(dev_priv)) {
 		uint32_t temp = I915_READ(DP_TP_CTL(port));
@@ -2897,7 +2905,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 			temp &= ~DP_TP_CTL_SCRAMBLE_DISABLE;
 
 		temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
-		switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
+		switch (dp_train_pat & train_pat_mask) {
 		case DP_TRAINING_PATTERN_DISABLE:
 			temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
 
@@ -2911,6 +2919,9 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 		case DP_TRAINING_PATTERN_3:
 			temp |= DP_TP_CTL_LINK_TRAIN_PAT3;
 			break;
+		case DP_TRAINING_PATTERN_4:
+			temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
+			break;
 		}
 		I915_WRITE(DP_TP_CTL(port), temp);
 
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 3fcaa98b9055..4da6e33c7fa1 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -219,14 +219,30 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 }
 
 /*
- * Pick training pattern for channel equalization. Training Pattern 3 for HBR2
+ * Pick training pattern for channel equalization. Training pattern 4 for HBR3
+ * or for 1.4 devices that support it, training Pattern 3 for HBR2
  * or 1.2 devices that support it, Training Pattern 2 otherwise.
  */
 static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
 {
-	u32 training_pattern = DP_TRAINING_PATTERN_2;
-	bool source_tps3, sink_tps3;
+	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
 
+	/*
+	 * Intel platforms that support HBR3 also support TPS4. It is mandatory
+	 * for all downstream devices that support HBR3. There are no known eDP
+	 * panels that support TPS4 as of Feb 2018 as per VESA eDP_v1.4b_E1
+	 * specification.
+	 */
+	source_tps4 = intel_dp_source_supports_hbr3(intel_dp);
+	sink_tps4 = drm_dp_tps4_supported(intel_dp->dpcd);
+	if (source_tps4 && sink_tps4) {
+		return DP_TRAINING_PATTERN_4;
+	} else if (intel_dp->link_rate == 810000) {
+		if (!source_tps4)
+			DRM_DEBUG_KMS("8.1 Gbps link rate without source HBR3/TPS4 support\n");
+		if (!sink_tps4)
+			DRM_DEBUG_KMS("8.1 Gbps link rate without sink TPS4 support\n");
+	}
 	/*
 	 * Intel platforms that support HBR2 also support TPS3. TPS3 support is
 	 * also mandatory for downstream devices that support HBR2. However, not
@@ -234,17 +250,16 @@ static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
 	 */
 	source_tps3 = intel_dp_source_supports_hbr2(intel_dp);
 	sink_tps3 = drm_dp_tps3_supported(intel_dp->dpcd);
-
 	if (source_tps3 && sink_tps3) {
-		training_pattern = DP_TRAINING_PATTERN_3;
-	} else if (intel_dp->link_rate == 540000) {
+		return  DP_TRAINING_PATTERN_3;
+	} else if (intel_dp->link_rate >= 540000) {
 		if (!source_tps3)
-			DRM_DEBUG_KMS("5.4 Gbps link rate without source HBR2/TPS3 support\n");
+			DRM_DEBUG_KMS(">=5.4/6.48 Gbps link rate without source HBR2/TPS3 support\n");
 		if (!sink_tps3)
-			DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3 support\n");
+			DRM_DEBUG_KMS(">=5.4/6.48 Gbps link rate without sink TPS3 support\n");
 	}
 
-	return training_pattern;
+	return DP_TRAINING_PATTERN_2;
 }
 
 static bool
@@ -256,11 +271,13 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	bool channel_eq = false;
 
 	training_pattern = intel_dp_training_pattern(intel_dp);
+	/* Scrambling is disabled for TPS2/3 and enabled for TPS4 */
+	if (training_pattern != DP_TRAINING_PATTERN_4)
+		training_pattern |= DP_LINK_SCRAMBLING_DISABLE;
 
 	/* channel equalization */
 	if (!intel_dp_set_link_train(intel_dp,
-				     training_pattern |
-				     DP_LINK_SCRAMBLING_DISABLE)) {
+				     training_pattern)) {
 		DRM_ERROR("failed to start channel equalization\n");
 		return false;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8840108749a5..04d61897b044 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1716,6 +1716,7 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select);
 bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
+bool intel_dp_source_supports_hbr3(struct intel_dp *intel_dp);
 bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
 
-- 
2.14.4

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake
URL   : https://patchwork.freedesktop.org/series/44595/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake
3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link training
-:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694:
+#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)
                                   		  ^

total: 0 errors, 0 warnings, 1 checks, 127 lines checked

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

✓ Fi.CI.BAT: success for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake
URL   : https://patchwork.freedesktop.org/series/44595/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4304 -> Patchwork_9265 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_mmap_gtt@basic-read-write:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

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

    
    ==== Possible fixes ====

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS +1

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097


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

  Missing    (5): fi-ctg-p8600 fi-bsw-cyan fi-ilk-m540 fi-skl-gvtdvm fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4304 -> Patchwork_9265

  CI_DRM_4304: 2313a1dc588ef63d9650ccbaaf576bc4b47dc255 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4515: a0f2d23b7d3d4226a0a7637a9240bfa86f08c1d3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9265: 3fe43cb729fe6424118ba865c3e0cbbeeb880215 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link training
e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake

== Logs ==

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

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Paulo Zanoni]

Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu:
> == Series Details ==
> 
> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP
> rates for Icelake
> URL   : https://patchwork.freedesktop.org/series/44595/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake
> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link
> training
> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694:
> +#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)

Dear maintainers,

I get this type of error way too often. What's the most desirable thing
here?

1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave
the coding style inconsistent with the surrounding lines.

2 - Drive-by fix all the bits around it so everybody in the same
definition has nice spaces, 2.a: in the same patch, 2.b: in a separate
patch.

3 - Just ignore the checkpatch message, push code as-is.

4 - Blacklist this check from checkpatch.

5 - Submit a separate patch fixing all the spacing errors on i915_reg.h
once and for all. Live happily ever after.

6 - Submit a separate patch converting everything to BIT() on
i915_reg.h.


Thanks,
Paulo

>                                    		  ^
> 
> total: 0 errors, 0 warnings, 1 checks, 127 lines checked
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake
URL   : https://patchwork.freedesktop.org/series/44595/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4304_full -> Patchwork_9265_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9265_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9265_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_9265_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-apl:          PASS -> FAIL (fdo#105347)

    igt@drv_selftest@live_hangcheck:
      shard-apl:          PASS -> DMESG-FAIL (fdo#106560)

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

    igt@kms_flip@2x-flip-vs-wf_vblank-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#103822, fdo#104724)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

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

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          FAIL (fdo#105347) -> PASS

    igt@drv_suspend@shrink:
      shard-hsw:          INCOMPLETE (fdo#103540) -> PASS

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-hsw:          FAIL (fdo#104724, fdo#103925) -> PASS

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

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  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_4304 -> Patchwork_9265

  CI_DRM_4304: 2313a1dc588ef63d9650ccbaaf576bc4b47dc255 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4515: a0f2d23b7d3d4226a0a7637a9240bfa86f08c1d3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9265: 3fe43cb729fe6424118ba865c3e0cbbeeb880215 @ 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_9265/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Jani Nikula]

On Mon, 11 Jun 2018, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu:
>> == Series Details ==
>> 
>> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP
>> rates for Icelake
>> URL   : https://patchwork.freedesktop.org/series/44595/
>> State : warning
>> 
>> == Summary ==
>> 
>> $ dim checkpatch origin/drm-tip
>> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake
>> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link
>> training
>> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694:
>> +#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)
>
> Dear maintainers,
>
> I get this type of error way too often. What's the most desirable thing
> here?
>
> 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave
> the coding style inconsistent with the surrounding lines.

I don't like the inconsistency.

> 2 - Drive-by fix all the bits around it so everybody in the same
> definition has nice spaces, 2.a: in the same patch, 2.b: in a separate
> patch.

Fine by me. Both a and b. I was kind of hoping this would have happened
more.

> 3 - Just ignore the checkpatch message, push code as-is.

Also fine by me.

> 4 - Blacklist this check from checkpatch.

Unfortunately the SPACING class in checkpatch would silence much, much
more than just this specific thing, so it would be a net negative.

> 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h
> once and for all. Live happily ever after.

It would be annoying for a while with conflicts, but I'd be fine. Not
sure if it would be better to do it in some arbitrary chunks rather than
mass change.

> 6 - Submit a separate patch converting everything to BIT() on
> i915_reg.h.

Same as above.

BR,
Jani.

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

Re: [CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Ville Syrjälä]

On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote:
> From: Manasi Navare <manasi.d.navare@intel.com>
> 
> For ICL, on Combo PHY the allowed max rates are:
>  - HBR3 8.1 eDP (DDIA)
>  - HBR2 5.4 DisplayPort (DDIB)
> and for MG PHY/TC DDI Ports allowed DP rates are:
>  - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
>  - DP on legacy connector - DDIC/D/E/F)
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: James Ausmus <james.ausmus@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: James Ausmus <james.ausmus@intel.com>
> [Paulo: bikeshed to keep future platforms on "else".]
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 37b9f62aeb6e..8371159cc192 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp)
>  	return 810000;
>  }
>  
> +static int icl_max_source_rate(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	enum port port = dig_port->base.port;
> +
> +	/* On Combo PHY port A max speed is HBR3 for all Vccio voltages
> +	 * and on Combo PHY Port B the maximum supported is HBR2.
> +	 */

And what about the other ports? If port B is the only
exception why are we even discussing port A specifically
here?

> +	if (port == PORT_B)
> +		return 540000;
> +
> +	return 810000;
> +}
> +
>  static void
>  intel_dp_set_source_rates(struct intel_dp *intel_dp)
>  {
> @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp)
>  	/* This should only be done once */
>  	WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates);
>  
> -	if (IS_CANNONLAKE(dev_priv)) {
> +	if (INTEL_GEN(dev_priv) >= 10) {
>  		source_rates = cnl_rates;
>  		size = ARRAY_SIZE(cnl_rates);
> -		max_rate = cnl_max_source_rate(intel_dp);
> +		if (INTEL_GEN(dev_priv) == 10)
> +			max_rate = cnl_max_source_rate(intel_dp);
> +		else
> +			max_rate = icl_max_source_rate(intel_dp);
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		source_rates = bxt_rates;
>  		size = ARRAY_SIZE(bxt_rates);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Manasi Navare]

On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote:
> > From: Manasi Navare <manasi.d.navare@intel.com>
> > 
> > For ICL, on Combo PHY the allowed max rates are:
> >  - HBR3 8.1 eDP (DDIA)
> >  - HBR2 5.4 DisplayPort (DDIB)
> > and for MG PHY/TC DDI Ports allowed DP rates are:
> >  - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
> >  - DP on legacy connector - DDIC/D/E/F)
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Reviewed-by: James Ausmus <james.ausmus@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > [Paulo: bikeshed to keep future platforms on "else".]
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 37b9f62aeb6e..8371159cc192 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp)
> >  	return 810000;
> >  }
> >  
> > +static int icl_max_source_rate(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	enum port port = dig_port->base.port;
> > +
> > +	/* On Combo PHY port A max speed is HBR3 for all Vccio voltages
> > +	 * and on Combo PHY Port B the maximum supported is HBR2.
> > +	 */
> 
> And what about the other ports? If port B is the only
> exception why are we even discussing port A specifically
> here?

All the MG PHY ports (C/D/E/F)  support a max of HBR3 that is 810000 but for
Combo PHY ports which is Port A or B, HBR3 only supported for Port A
but for Port B it is max of HBR2 which is 540000 hence the comment for Combo PHY
ports and if port B then just return HBR2

Manasi

> 
> > +	if (port == PORT_B)
> > +		return 540000;
> > +
> > +	return 810000;
> > +}
> > +
> >  static void
> >  intel_dp_set_source_rates(struct intel_dp *intel_dp)
> >  {
> > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp)
> >  	/* This should only be done once */
> >  	WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates);
> >  
> > -	if (IS_CANNONLAKE(dev_priv)) {
> > +	if (INTEL_GEN(dev_priv) >= 10) {
> >  		source_rates = cnl_rates;
> >  		size = ARRAY_SIZE(cnl_rates);
> > -		max_rate = cnl_max_source_rate(intel_dp);
> > +		if (INTEL_GEN(dev_priv) == 10)
> > +			max_rate = cnl_max_source_rate(intel_dp);
> > +		else
> > +			max_rate = icl_max_source_rate(intel_dp);
> >  	} else if (IS_GEN9_LP(dev_priv)) {
> >  		source_rates = bxt_rates;
> >  		size = ARRAY_SIZE(bxt_rates);
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> 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

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Rodrigo Vivi]

On Tue, Jun 12, 2018 at 11:46:08AM +0300, Jani Nikula wrote:
> On Mon, 11 Jun 2018, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu:
> >> == Series Details ==
> >> 
> >> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP
> >> rates for Icelake
> >> URL   : https://patchwork.freedesktop.org/series/44595/
> >> State : warning
> >> 
> >> == Summary ==
> >> 
> >> $ dim checkpatch origin/drm-tip
> >> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake
> >> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link
> >> training
> >> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> >> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694:
> >> +#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)
> >
> > Dear maintainers,
> >
> > I get this type of error way too often. What's the most desirable thing
> > here?
> >
> > 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave
> > the coding style inconsistent with the surrounding lines.
> 
> I don't like the inconsistency.

me neither...

> 
> > 2 - Drive-by fix all the bits around it so everybody in the same
> > definition has nice spaces, 2.a: in the same patch, 2.b: in a separate
> > patch.
> 
> Fine by me. Both a and b. I was kind of hoping this would have happened
> more.
> 
> > 3 - Just ignore the checkpatch message, push code as-is.
> 
> Also fine by me.

what I'm currently doing...

> 
> > 4 - Blacklist this check from checkpatch.
> 
> Unfortunately the SPACING class in checkpatch would silence much, much
> more than just this specific thing, so it would be a net negative.

Let's keep the style we want there even if this cause warnings while we
haven't finished the standardization.

> 
> > 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h
> > once and for all. Live happily ever after.
> 
> It would be annoying for a while with conflicts, but I'd be fine. Not
> sure if it would be better to do it in some arbitrary chunks rather than
> mass change.

I believe I prefer one mass commit. So we convert once for all
and cause rebase conflict on internal branch only once. So we solve all
at one and be happy...

> 
> > 6 - Submit a separate patch converting everything to BIT() on
> > i915_reg.h.
> 
> Same as above.

Do we really want BIT everywhere?!

Thanks,
Rodrigo.

> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> 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

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Jani Nikula]

On Tue, 12 Jun 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Do we really want BIT everywhere?!

I think I'd go for everywhere except part of a register field value:

#define SINGLE_BIT_OKAY		BIT(25)
#define FIELD_SHIFT		20
#define FIELD_MASK		(0xf << 20)
#define FIELD_FOO_PLEASE_NO	BIT(20)		/* Don't do this */
#define FIELD_FOO		(1 << 20)	/* This is consistent */
#define FIELD_BAR		(2 << 20)
#define FIELD_BAZ		(3 << 20)


BR,
Jani.

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

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Paulo Zanoni]

Em Qua, 2018-06-13 às 11:07 +0300, Jani Nikula escreveu:
> On Tue, 12 Jun 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Do we really want BIT everywhere?!
> 
> I think I'd go for everywhere except part of a register field value:
> 

While I completely agree with your reasoning, this means we'll kinda
always want to blacklist the BIT_MACRO checkpath type because
checkpatch won't know about these exceptions, which means we won't
actually need to convert everything to BIT() since no false negative
emails anyway.

Anyway, I submitted a patch to fix the spacing issues, I'd love to have
some comments from the maintainers on it.

Thanks,
Paulo

> #define SINGLE_BIT_OKAY		BIT(25)
> #define FIELD_SHIFT		20
> #define FIELD_MASK		(0xf << 20)
> #define FIELD_FOO_PLEASE_NO	BIT(20)		/* Don't do
> this */
> #define FIELD_FOO		(1 << 20)	/* This is
> consistent */
> #define FIELD_BAR		(2 << 20)
> #define FIELD_BAZ		(3 << 20)
> 
> 
> BR,
> Jani.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Paulo Zanoni]

Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu:
> On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote:
> > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote:
> > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > 
> > > For ICL, on Combo PHY the allowed max rates are:
> > >  - HBR3 8.1 eDP (DDIA)
> > >  - HBR2 5.4 DisplayPort (DDIB)
> > > and for MG PHY/TC DDI Ports allowed DP rates are:
> > >  - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
> > >  - DP on legacy connector - DDIC/D/E/F)
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Reviewed-by: James Ausmus <james.ausmus@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > > [Paulo: bikeshed to keep future platforms on "else".]
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 37b9f62aeb6e..8371159cc192 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct
> > > intel_dp *intel_dp)
> > >  	return 810000;
> > >  }
> > >  
> > > +static int icl_max_source_rate(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	enum port port = dig_port->base.port;
> > > +
> > > +	/* On Combo PHY port A max speed is HBR3 for all Vccio
> > > voltages
> > > +	 * and on Combo PHY Port B the maximum supported is
> > > HBR2.
> > > +	 */
> > 
> > And what about the other ports? If port B is the only
> > exception why are we even discussing port A specifically
> > here?
> 
> All the MG PHY ports (C/D/E/F)  support a max of HBR3 that is 810000
> but for
> Combo PHY ports which is Port A or B, HBR3 only supported for Port A
> but for Port B it is max of HBR2 which is 540000 hence the comment
> for Combo PHY
> ports and if port B then just return HBR2

I think Ville's point was that having a comment that only discusses
ports A and B on code that handles all ports gives the impression that
perhaps the code is "forgetting" to consider the other ports, or
something like that. Which makes sense to me.

Perhaps to address this issue we could either reword the comment to
include C-F or simply just remove it, since the commit message should
be enough and the comment only says what the code says.

> 
> Manasi
> 
> > 
> > > +	if (port == PORT_B)
> > > +		return 540000;
> > > +
> > > +	return 810000;
> > > +}
> > > +
> > >  static void
> > >  intel_dp_set_source_rates(struct intel_dp *intel_dp)
> > >  {
> > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp
> > > *intel_dp)
> > >  	/* This should only be done once */
> > >  	WARN_ON(intel_dp->source_rates || intel_dp-
> > > >num_source_rates);
> > >  
> > > -	if (IS_CANNONLAKE(dev_priv)) {
> > > +	if (INTEL_GEN(dev_priv) >= 10) {
> > >  		source_rates = cnl_rates;
> > >  		size = ARRAY_SIZE(cnl_rates);
> > > -		max_rate = cnl_max_source_rate(intel_dp);
> > > +		if (INTEL_GEN(dev_priv) == 10)
> > > +			max_rate =
> > > cnl_max_source_rate(intel_dp);
> > > +		else
> > > +			max_rate =
> > > icl_max_source_rate(intel_dp);
> > >  	} else if (IS_GEN9_LP(dev_priv)) {
> > >  		source_rates = bxt_rates;
> > >  		size = ARRAY_SIZE(bxt_rates);
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Manasi Navare]

On Wed, Jun 13, 2018 at 12:42:20PM -0700, Paulo Zanoni wrote:
> Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu:
> > On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote:
> > > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote:
> > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > 
> > > > For ICL, on Combo PHY the allowed max rates are:
> > > >  - HBR3 8.1 eDP (DDIA)
> > > >  - HBR2 5.4 DisplayPort (DDIB)
> > > > and for MG PHY/TC DDI Ports allowed DP rates are:
> > > >  - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
> > > >  - DP on legacy connector - DDIC/D/E/F)
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Reviewed-by: James Ausmus <james.ausmus@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > > > [Paulo: bikeshed to keep future platforms on "else".]
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
> > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 37b9f62aeb6e..8371159cc192 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct
> > > > intel_dp *intel_dp)
> > > >  	return 810000;
> > > >  }
> > > >  
> > > > +static int icl_max_source_rate(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	enum port port = dig_port->base.port;
> > > > +
> > > > +	/* On Combo PHY port A max speed is HBR3 for all Vccio
> > > > voltages
> > > > +	 * and on Combo PHY Port B the maximum supported is
> > > > HBR2.
> > > > +	 */
> > > 
> > > And what about the other ports? If port B is the only
> > > exception why are we even discussing port A specifically
> > > here?
> > 
> > All the MG PHY ports (C/D/E/F)  support a max of HBR3 that is 810000
> > but for
> > Combo PHY ports which is Port A or B, HBR3 only supported for Port A
> > but for Port B it is max of HBR2 which is 540000 hence the comment
> > for Combo PHY
> > ports and if port B then just return HBR2
> 
> I think Ville's point was that having a comment that only discusses
> ports A and B on code that handles all ports gives the impression that
> perhaps the code is "forgetting" to consider the other ports, or
> something like that. Which makes sense to me.
> 
> Perhaps to address this issue we could either reword the comment to
> include C-F or simply just remove it, since the commit message should
> be enough and the comment only says what the code says.
>

Yes I agree. Lets remove the comment in the code since the commit message
covers that part.
Should I send a new revision for this with comment removed?

Manasi 

> > 
> > Manasi
> > 
> > > 
> > > > +	if (port == PORT_B)
> > > > +		return 540000;
> > > > +
> > > > +	return 810000;
> > > > +}
> > > > +
> > > >  static void
> > > >  intel_dp_set_source_rates(struct intel_dp *intel_dp)
> > > >  {
> > > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct intel_dp
> > > > *intel_dp)
> > > >  	/* This should only be done once */
> > > >  	WARN_ON(intel_dp->source_rates || intel_dp-
> > > > >num_source_rates);
> > > >  
> > > > -	if (IS_CANNONLAKE(dev_priv)) {
> > > > +	if (INTEL_GEN(dev_priv) >= 10) {
> > > >  		source_rates = cnl_rates;
> > > >  		size = ARRAY_SIZE(cnl_rates);
> > > > -		max_rate = cnl_max_source_rate(intel_dp);
> > > > +		if (INTEL_GEN(dev_priv) == 10)
> > > > +			max_rate =
> > > > cnl_max_source_rate(intel_dp);
> > > > +		else
> > > > +			max_rate =
> > > > icl_max_source_rate(intel_dp);
> > > >  	} else if (IS_GEN9_LP(dev_priv)) {
> > > >  		source_rates = bxt_rates;
> > > >  		size = ARRAY_SIZE(bxt_rates);
> > > > -- 
> > > > 2.14.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > > _______________________________________________
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 1/2] drm/i915/icl: Add allowed DP rates for Icelake

[Paulo Zanoni]

Em Qua, 2018-06-13 às 13:15 -0700, Manasi Navare escreveu:
> On Wed, Jun 13, 2018 at 12:42:20PM -0700, Paulo Zanoni wrote:
> > Em Ter, 2018-06-12 às 11:37 -0700, Manasi Navare escreveu:
> > > On Tue, Jun 12, 2018 at 03:15:53PM +0300, Ville Syrjälä wrote:
> > > > On Mon, Jun 11, 2018 at 03:26:54PM -0700, Paulo Zanoni wrote:
> > > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > > 
> > > > > For ICL, on Combo PHY the allowed max rates are:
> > > > >  - HBR3 8.1 eDP (DDIA)
> > > > >  - HBR2 5.4 DisplayPort (DDIB)
> > > > > and for MG PHY/TC DDI Ports allowed DP rates are:
> > > > >  - HBR3 8.1 DisplayPort (DP alternate mode, DP over TBT,
> > > > >  - DP on legacy connector - DDIC/D/E/F)
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Reviewed-by: James Ausmus <james.ausmus@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > > > > [Paulo: bikeshed to keep future platforms on "else".]
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++++++--
> > > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 37b9f62aeb6e..8371159cc192 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -256,6 +256,20 @@ static int cnl_max_source_rate(struct
> > > > > intel_dp *intel_dp)
> > > > >  	return 810000;
> > > > >  }
> > > > >  
> > > > > +static int icl_max_source_rate(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *dig_port =
> > > > > dp_to_dig_port(intel_dp);
> > > > > +	enum port port = dig_port->base.port;
> > > > > +
> > > > > +	/* On Combo PHY port A max speed is HBR3 for all
> > > > > Vccio
> > > > > voltages
> > > > > +	 * and on Combo PHY Port B the maximum supported is
> > > > > HBR2.
> > > > > +	 */
> > > > 
> > > > And what about the other ports? If port B is the only
> > > > exception why are we even discussing port A specifically
> > > > here?
> > > 
> > > All the MG PHY ports (C/D/E/F)  support a max of HBR3 that is
> > > 810000
> > > but for
> > > Combo PHY ports which is Port A or B, HBR3 only supported for
> > > Port A
> > > but for Port B it is max of HBR2 which is 540000 hence the
> > > comment
> > > for Combo PHY
> > > ports and if port B then just return HBR2
> > 
> > I think Ville's point was that having a comment that only discusses
> > ports A and B on code that handles all ports gives the impression
> > that
> > perhaps the code is "forgetting" to consider the other ports, or
> > something like that. Which makes sense to me.
> > 
> > Perhaps to address this issue we could either reword the comment to
> > include C-F or simply just remove it, since the commit message
> > should
> > be enough and the comment only says what the code says.
> > 
> 
> Yes I agree. Lets remove the comment in the code since the commit
> message
> covers that part.
> Should I send a new revision for this with comment removed?

Feel free to do it, otherwise I can do it while applying since it's a
trivial change.

Thanks,
Paulo

> 
> Manasi 
> 
> > > 
> > > Manasi
> > > 
> > > > 
> > > > > +	if (port == PORT_B)
> > > > > +		return 540000;
> > > > > +
> > > > > +	return 810000;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  intel_dp_set_source_rates(struct intel_dp *intel_dp)
> > > > >  {
> > > > > @@ -285,10 +299,13 @@ intel_dp_set_source_rates(struct
> > > > > intel_dp
> > > > > *intel_dp)
> > > > >  	/* This should only be done once */
> > > > >  	WARN_ON(intel_dp->source_rates || intel_dp-
> > > > > > num_source_rates);
> > > > > 
> > > > >  
> > > > > -	if (IS_CANNONLAKE(dev_priv)) {
> > > > > +	if (INTEL_GEN(dev_priv) >= 10) {
> > > > >  		source_rates = cnl_rates;
> > > > >  		size = ARRAY_SIZE(cnl_rates);
> > > > > -		max_rate = cnl_max_source_rate(intel_dp);
> > > > > +		if (INTEL_GEN(dev_priv) == 10)
> > > > > +			max_rate =
> > > > > cnl_max_source_rate(intel_dp);
> > > > > +		else
> > > > > +			max_rate =
> > > > > icl_max_source_rate(intel_dp);
> > > > >  	} else if (IS_GEN9_LP(dev_priv)) {
> > > > >  		source_rates = bxt_rates;
> > > > >  		size = ARRAY_SIZE(bxt_rates);
> > > > > -- 
> > > > > 2.14.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > > > _______________________________________________
> > > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx