* [PATCH 0/8] Haswell HDMI fixes
@ 2012-08-08 17:15 Paulo Zanoni
2012-08-08 17:15 ` [PATCH 1/8] drm/i915: fix pipe DDI mode select Paulo Zanoni
` (9 more replies)
0 siblings, 10 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
HDMI already works fine on Haswell, but we still have room for improvements.
This series will make us less dependent on the bits set by the BIOS, will fix
cases where DVI was not working and will also improve the cases where we have 2
HDMI monitors.
- Patches 1-4 are all about the DDI_FUNC_CTL register.
- Patch 5 is to satisfy my OCD.
- Patch 6 was spotted while writing patch 5.
- Patches 7-8 are about setting PLLs.
Paulo Zanoni (8):
drm/i915: fix pipe DDI mode select
drm/i915: set the DDI sync polarity bits
drm/i915: correctly set the DDI_FUNC_CTL bpc field
drm/i915: completely reset the value of DDI_FUNC_CTL
drm/i915: reindent Haswell register definitions
drm/i915: add parentheses around PIXCLK_GATE definitions
drm/i915: try harder to find WR PLL clock settings
drm/i915: try to use WR PLL 2
drivers/gpu/drm/i915/i915_reg.h | 184 ++++++++++++++++++---------------------
drivers/gpu/drm/i915/intel_ddi.c | 108 ++++++++++++++++-------
2 files changed, 163 insertions(+), 129 deletions(-)
--
1.7.11.2
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/8] drm/i915: fix pipe DDI mode select
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-08 17:15 ` [PATCH 2/8] drm/i915: set the DDI sync polarity bits Paulo Zanoni
` (8 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Mask the value before changing it and also select DVI when needed.
DVI was working in cases where the BIOS was setting the correct value
because we were not masking the value before changing it.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ddi.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1310caa..97f00fb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4302,6 +4302,7 @@
/* Those bits are ignored by pipe EDP since it can only connect to DDI A */
#define PIPE_DDI_PORT_MASK (7<<28)
#define PIPE_DDI_SELECT_PORT(x) ((x)<<28)
+#define PIPE_DDI_MODE_SELECT_MASK (7<<24)
#define PIPE_DDI_MODE_SELECT_HDMI (0<<24)
#define PIPE_DDI_MODE_SELECT_DVI (1<<24)
#define PIPE_DDI_MODE_SELECT_DP_SST (2<<24)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 32604ac..0d7acd7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -726,13 +726,18 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
temp = I915_READ(DDI_FUNC_CTL(pipe));
temp &= ~PIPE_DDI_PORT_MASK;
temp &= ~PIPE_DDI_BPC_12;
+ temp &= ~PIPE_DDI_MODE_SELECT_MASK;
temp |= PIPE_DDI_SELECT_PORT(port) |
- PIPE_DDI_MODE_SELECT_HDMI |
((intel_crtc->bpp > 24) ?
PIPE_DDI_BPC_12 :
PIPE_DDI_BPC_8) |
PIPE_DDI_FUNC_ENABLE;
+ if (intel_hdmi->has_hdmi_sink)
+ temp |= PIPE_DDI_MODE_SELECT_HDMI;
+ else
+ temp |= PIPE_DDI_MODE_SELECT_DVI;
+
I915_WRITE(DDI_FUNC_CTL(pipe), temp);
intel_hdmi->set_infoframes(encoder, adjusted_mode);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/8] drm/i915: set the DDI sync polarity bits
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
2012-08-08 17:15 ` [PATCH 1/8] drm/i915: fix pipe DDI mode select Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-08 17:15 ` [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field Paulo Zanoni
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
During my tests, everything worked even if the wrong polarity was set.
Still, we should try to set the correct values.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 97f00fb..896b279 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4312,6 +4312,8 @@
#define PIPE_DDI_BPC_10 (1<<20)
#define PIPE_DDI_BPC_6 (2<<20)
#define PIPE_DDI_BPC_12 (3<<20)
+#define PIPE_DDI_PVSYNC (1<<17)
+#define PIPE_DDI_PHSYNC (1<<16)
#define PIPE_DDI_BFI_ENABLE (1<<4)
#define PIPE_DDI_PORT_WIDTH_X1 (0<<1)
#define PIPE_DDI_PORT_WIDTH_X2 (1<<1)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0d7acd7..1fbd67c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -727,6 +727,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
temp &= ~PIPE_DDI_PORT_MASK;
temp &= ~PIPE_DDI_BPC_12;
temp &= ~PIPE_DDI_MODE_SELECT_MASK;
+ temp &= ~(PIPE_DDI_PVSYNC | PIPE_DDI_PHSYNC);
temp |= PIPE_DDI_SELECT_PORT(port) |
((intel_crtc->bpp > 24) ?
PIPE_DDI_BPC_12 :
@@ -738,6 +739,11 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
else
temp |= PIPE_DDI_MODE_SELECT_DVI;
+ if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
+ temp |= PIPE_DDI_PVSYNC;
+ if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
+ temp |= PIPE_DDI_PHSYNC;
+
I915_WRITE(DDI_FUNC_CTL(pipe), temp);
intel_hdmi->set_infoframes(encoder, adjusted_mode);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
2012-08-08 17:15 ` [PATCH 1/8] drm/i915: fix pipe DDI mode select Paulo Zanoni
2012-08-08 17:15 ` [PATCH 2/8] drm/i915: set the DDI sync polarity bits Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-09 9:55 ` Jani Nikula
2012-08-08 17:15 ` [PATCH 4/8] drm/i915: completely reset the value of DDI_FUNC_CTL Paulo Zanoni
` (6 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Correctly erase the values previously set and also check for 6pbc and
10bpc.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ddi.c | 26 ++++++++++++++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 896b279..f3fafb8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4308,6 +4308,7 @@
#define PIPE_DDI_MODE_SELECT_DP_SST (2<<24)
#define PIPE_DDI_MODE_SELECT_DP_MST (3<<24)
#define PIPE_DDI_MODE_SELECT_FDI (4<<24)
+#define PIPE_DDI_BPC_MASK (7<<20)
#define PIPE_DDI_BPC_8 (0<<20)
#define PIPE_DDI_BPC_10 (1<<20)
#define PIPE_DDI_BPC_6 (2<<20)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1fbd67c..8b38359 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -725,14 +725,28 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
/* Enable PIPE_DDI_FUNC_CTL for the pipe to work in HDMI mode */
temp = I915_READ(DDI_FUNC_CTL(pipe));
temp &= ~PIPE_DDI_PORT_MASK;
- temp &= ~PIPE_DDI_BPC_12;
+ temp &= ~PIPE_DDI_BPC_MASK;
temp &= ~PIPE_DDI_MODE_SELECT_MASK;
temp &= ~(PIPE_DDI_PVSYNC | PIPE_DDI_PHSYNC);
- temp |= PIPE_DDI_SELECT_PORT(port) |
- ((intel_crtc->bpp > 24) ?
- PIPE_DDI_BPC_12 :
- PIPE_DDI_BPC_8) |
- PIPE_DDI_FUNC_ENABLE;
+ temp |= PIPE_DDI_FUNC_ENABLE | PIPE_DDI_SELECT_PORT(port);
+
+ switch (intel_crtc->bpp) {
+ case 18:
+ temp |= PIPE_DDI_BPC_6;
+ break;
+ case 24:
+ temp |= PIPE_DDI_BPC_8;
+ break;
+ case 30:
+ temp |= PIPE_DDI_BPC_10;
+ break;
+ case 36:
+ temp |= PIPE_DDI_BPC_12;
+ break;
+ default:
+ WARN(1, "%d bpp unsupported by pipe DDI function\n",
+ intel_crtc->bpp);
+ }
if (intel_hdmi->has_hdmi_sink)
temp |= PIPE_DDI_MODE_SELECT_HDMI;
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/8] drm/i915: completely reset the value of DDI_FUNC_CTL
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (2 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-08 17:15 ` [PATCH 5/8] drm/i915: reindent Haswell register definitions Paulo Zanoni
` (5 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Don't rely on previous values already set on the register. Everything
we're not explicitly setting should be zero for now.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8b38359..ff03a3a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -723,12 +723,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
}
/* Enable PIPE_DDI_FUNC_CTL for the pipe to work in HDMI mode */
- temp = I915_READ(DDI_FUNC_CTL(pipe));
- temp &= ~PIPE_DDI_PORT_MASK;
- temp &= ~PIPE_DDI_BPC_MASK;
- temp &= ~PIPE_DDI_MODE_SELECT_MASK;
- temp &= ~(PIPE_DDI_PVSYNC | PIPE_DDI_PHSYNC);
- temp |= PIPE_DDI_FUNC_ENABLE | PIPE_DDI_SELECT_PORT(port);
+ temp = PIPE_DDI_FUNC_ENABLE | PIPE_DDI_SELECT_PORT(port);
switch (intel_crtc->bpp) {
case 18:
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/8] drm/i915: reindent Haswell register definitions
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (3 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 4/8] drm/i915: completely reset the value of DDI_FUNC_CTL Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-08 17:15 ` [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions Paulo Zanoni
` (4 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
It's the only part of the i915_reg.h file that looks totally wrongly
indented, so I assume my editor config is the correct one.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 184 +++++++++++++++++++---------------------
1 file changed, 85 insertions(+), 99 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f3fafb8..af41d03 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4278,198 +4278,184 @@
#define AUD_CONFIG_DISABLE_NCTS (1 << 3)
/* HSW Power Wells */
-#define HSW_PWR_WELL_CTL1 0x45400 /* BIOS */
-#define HSW_PWR_WELL_CTL2 0x45404 /* Driver */
-#define HSW_PWR_WELL_CTL3 0x45408 /* KVMR */
-#define HSW_PWR_WELL_CTL4 0x4540C /* Debug */
-#define HSW_PWR_WELL_ENABLE (1<<31)
-#define HSW_PWR_WELL_STATE (1<<30)
-#define HSW_PWR_WELL_CTL5 0x45410
+#define HSW_PWR_WELL_CTL1 0x45400 /* BIOS */
+#define HSW_PWR_WELL_CTL2 0x45404 /* Driver */
+#define HSW_PWR_WELL_CTL3 0x45408 /* KVMR */
+#define HSW_PWR_WELL_CTL4 0x4540C /* Debug */
+#define HSW_PWR_WELL_ENABLE (1<<31)
+#define HSW_PWR_WELL_STATE (1<<30)
+#define HSW_PWR_WELL_CTL5 0x45410
#define HSW_PWR_WELL_ENABLE_SINGLE_STEP (1<<31)
#define HSW_PWR_WELL_PWR_GATE_OVERRIDE (1<<20)
-#define HSW_PWR_WELL_FORCE_ON (1<<19)
-#define HSW_PWR_WELL_CTL6 0x45414
+#define HSW_PWR_WELL_FORCE_ON (1<<19)
+#define HSW_PWR_WELL_CTL6 0x45414
/* Per-pipe DDI Function Control */
-#define PIPE_DDI_FUNC_CTL_A 0x60400
-#define PIPE_DDI_FUNC_CTL_B 0x61400
-#define PIPE_DDI_FUNC_CTL_C 0x62400
+#define PIPE_DDI_FUNC_CTL_A 0x60400
+#define PIPE_DDI_FUNC_CTL_B 0x61400
+#define PIPE_DDI_FUNC_CTL_C 0x62400
#define PIPE_DDI_FUNC_CTL_EDP 0x6F400
-#define DDI_FUNC_CTL(pipe) _PIPE(pipe, \
- PIPE_DDI_FUNC_CTL_A, \
- PIPE_DDI_FUNC_CTL_B)
+#define DDI_FUNC_CTL(pipe) _PIPE(pipe, PIPE_DDI_FUNC_CTL_A, \
+ PIPE_DDI_FUNC_CTL_B)
#define PIPE_DDI_FUNC_ENABLE (1<<31)
/* Those bits are ignored by pipe EDP since it can only connect to DDI A */
-#define PIPE_DDI_PORT_MASK (7<<28)
-#define PIPE_DDI_SELECT_PORT(x) ((x)<<28)
-#define PIPE_DDI_MODE_SELECT_MASK (7<<24)
-#define PIPE_DDI_MODE_SELECT_HDMI (0<<24)
-#define PIPE_DDI_MODE_SELECT_DVI (1<<24)
+#define PIPE_DDI_PORT_MASK (7<<28)
+#define PIPE_DDI_SELECT_PORT(x) ((x)<<28)
+#define PIPE_DDI_MODE_SELECT_MASK (7<<24)
+#define PIPE_DDI_MODE_SELECT_HDMI (0<<24)
+#define PIPE_DDI_MODE_SELECT_DVI (1<<24)
#define PIPE_DDI_MODE_SELECT_DP_SST (2<<24)
#define PIPE_DDI_MODE_SELECT_DP_MST (3<<24)
-#define PIPE_DDI_MODE_SELECT_FDI (4<<24)
-#define PIPE_DDI_BPC_MASK (7<<20)
-#define PIPE_DDI_BPC_8 (0<<20)
-#define PIPE_DDI_BPC_10 (1<<20)
-#define PIPE_DDI_BPC_6 (2<<20)
-#define PIPE_DDI_BPC_12 (3<<20)
-#define PIPE_DDI_PVSYNC (1<<17)
-#define PIPE_DDI_PHSYNC (1<<16)
-#define PIPE_DDI_BFI_ENABLE (1<<4)
-#define PIPE_DDI_PORT_WIDTH_X1 (0<<1)
-#define PIPE_DDI_PORT_WIDTH_X2 (1<<1)
-#define PIPE_DDI_PORT_WIDTH_X4 (3<<1)
+#define PIPE_DDI_MODE_SELECT_FDI (4<<24)
+#define PIPE_DDI_BPC_MASK (7<<20)
+#define PIPE_DDI_BPC_8 (0<<20)
+#define PIPE_DDI_BPC_10 (1<<20)
+#define PIPE_DDI_BPC_6 (2<<20)
+#define PIPE_DDI_BPC_12 (3<<20)
+#define PIPE_DDI_PVSYNC (1<<17)
+#define PIPE_DDI_PHSYNC (1<<16)
+#define PIPE_DDI_BFI_ENABLE (1<<4)
+#define PIPE_DDI_PORT_WIDTH_X1 (0<<1)
+#define PIPE_DDI_PORT_WIDTH_X2 (1<<1)
+#define PIPE_DDI_PORT_WIDTH_X4 (3<<1)
/* DisplayPort Transport Control */
#define DP_TP_CTL_A 0x64040
#define DP_TP_CTL_B 0x64140
-#define DP_TP_CTL(port) _PORT(port, \
- DP_TP_CTL_A, \
- DP_TP_CTL_B)
-#define DP_TP_CTL_ENABLE (1<<31)
-#define DP_TP_CTL_MODE_SST (0<<27)
-#define DP_TP_CTL_MODE_MST (1<<27)
+#define DP_TP_CTL(port) _PORT(port, DP_TP_CTL_A, DP_TP_CTL_B)
+#define DP_TP_CTL_ENABLE (1<<31)
+#define DP_TP_CTL_MODE_SST (0<<27)
+#define DP_TP_CTL_MODE_MST (1<<27)
#define DP_TP_CTL_ENHANCED_FRAME_ENABLE (1<<18)
-#define DP_TP_CTL_FDI_AUTOTRAIN (1<<15)
+#define DP_TP_CTL_FDI_AUTOTRAIN (1<<15)
#define DP_TP_CTL_LINK_TRAIN_MASK (7<<8)
#define DP_TP_CTL_LINK_TRAIN_PAT1 (0<<8)
#define DP_TP_CTL_LINK_TRAIN_PAT2 (1<<8)
-#define DP_TP_CTL_LINK_TRAIN_NORMAL (3<<8)
+#define DP_TP_CTL_LINK_TRAIN_NORMAL (3<<8)
/* DisplayPort Transport Status */
#define DP_TP_STATUS_A 0x64044
#define DP_TP_STATUS_B 0x64144
-#define DP_TP_STATUS(port) _PORT(port, \
- DP_TP_STATUS_A, \
- DP_TP_STATUS_B)
+#define DP_TP_STATUS(port) _PORT(port, DP_TP_STATUS_A, DP_TP_STATUS_B)
#define DP_TP_STATUS_AUTOTRAIN_DONE (1<<12)
/* DDI Buffer Control */
#define DDI_BUF_CTL_A 0x64000
#define DDI_BUF_CTL_B 0x64100
-#define DDI_BUF_CTL(port) _PORT(port, \
- DDI_BUF_CTL_A, \
- DDI_BUF_CTL_B)
-#define DDI_BUF_CTL_ENABLE (1<<31)
+#define DDI_BUF_CTL(port) _PORT(port, DDI_BUF_CTL_A, DDI_BUF_CTL_B)
+#define DDI_BUF_CTL_ENABLE (1<<31)
#define DDI_BUF_EMP_400MV_0DB_HSW (0<<24) /* Sel0 */
-#define DDI_BUF_EMP_400MV_3_5DB_HSW (1<<24) /* Sel1 */
+#define DDI_BUF_EMP_400MV_3_5DB_HSW (1<<24) /* Sel1 */
#define DDI_BUF_EMP_400MV_6DB_HSW (2<<24) /* Sel2 */
-#define DDI_BUF_EMP_400MV_9_5DB_HSW (3<<24) /* Sel3 */
+#define DDI_BUF_EMP_400MV_9_5DB_HSW (3<<24) /* Sel3 */
#define DDI_BUF_EMP_600MV_0DB_HSW (4<<24) /* Sel4 */
-#define DDI_BUF_EMP_600MV_3_5DB_HSW (5<<24) /* Sel5 */
+#define DDI_BUF_EMP_600MV_3_5DB_HSW (5<<24) /* Sel5 */
#define DDI_BUF_EMP_600MV_6DB_HSW (6<<24) /* Sel6 */
#define DDI_BUF_EMP_800MV_0DB_HSW (7<<24) /* Sel7 */
-#define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */
-#define DDI_BUF_EMP_MASK (0xf<<24)
-#define DDI_BUF_IS_IDLE (1<<7)
-#define DDI_PORT_WIDTH_X1 (0<<1)
-#define DDI_PORT_WIDTH_X2 (1<<1)
-#define DDI_PORT_WIDTH_X4 (3<<1)
+#define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */
+#define DDI_BUF_EMP_MASK (0xf<<24)
+#define DDI_BUF_IS_IDLE (1<<7)
+#define DDI_PORT_WIDTH_X1 (0<<1)
+#define DDI_PORT_WIDTH_X2 (1<<1)
+#define DDI_PORT_WIDTH_X4 (3<<1)
#define DDI_INIT_DISPLAY_DETECTED (1<<0)
/* DDI Buffer Translations */
#define DDI_BUF_TRANS_A 0x64E00
#define DDI_BUF_TRANS_B 0x64E60
-#define DDI_BUF_TRANS(port) _PORT(port, \
- DDI_BUF_TRANS_A, \
- DDI_BUF_TRANS_B)
+#define DDI_BUF_TRANS(port) _PORT(port, DDI_BUF_TRANS_A, DDI_BUF_TRANS_B)
/* Sideband Interface (SBI) is programmed indirectly, via
* SBI_ADDR, which contains the register offset; and SBI_DATA,
* which contains the payload */
-#define SBI_ADDR 0xC6000
-#define SBI_DATA 0xC6004
+#define SBI_ADDR 0xC6000
+#define SBI_DATA 0xC6004
#define SBI_CTL_STAT 0xC6008
#define SBI_CTL_OP_CRRD (0x6<<8)
#define SBI_CTL_OP_CRWR (0x7<<8)
#define SBI_RESPONSE_FAIL (0x1<<1)
-#define SBI_RESPONSE_SUCCESS (0x0<<1)
-#define SBI_BUSY (0x1<<0)
-#define SBI_READY (0x0<<0)
+#define SBI_RESPONSE_SUCCESS (0x0<<1)
+#define SBI_BUSY (0x1<<0)
+#define SBI_READY (0x0<<0)
/* SBI offsets */
-#define SBI_SSCDIVINTPHASE6 0x0600
+#define SBI_SSCDIVINTPHASE6 0x0600
#define SBI_SSCDIVINTPHASE_DIVSEL_MASK ((0x7f)<<1)
#define SBI_SSCDIVINTPHASE_DIVSEL(x) ((x)<<1)
#define SBI_SSCDIVINTPHASE_INCVAL_MASK ((0x7f)<<8)
#define SBI_SSCDIVINTPHASE_INCVAL(x) ((x)<<8)
-#define SBI_SSCDIVINTPHASE_DIR(x) ((x)<<15)
+#define SBI_SSCDIVINTPHASE_DIR(x) ((x)<<15)
#define SBI_SSCDIVINTPHASE_PROPAGATE (1<<0)
-#define SBI_SSCCTL 0x020c
+#define SBI_SSCCTL 0x020c
#define SBI_SSCCTL6 0x060C
-#define SBI_SSCCTL_DISABLE (1<<0)
+#define SBI_SSCCTL_DISABLE (1<<0)
#define SBI_SSCAUXDIV6 0x0610
#define SBI_SSCAUXDIV_FINALDIV2SEL(x) ((x)<<4)
-#define SBI_DBUFF0 0x2a00
+#define SBI_DBUFF0 0x2a00
/* LPT PIXCLK_GATE */
-#define PIXCLK_GATE 0xC6020
+#define PIXCLK_GATE 0xC6020
#define PIXCLK_GATE_UNGATE 1<<0
#define PIXCLK_GATE_GATE 0<<0
/* SPLL */
-#define SPLL_CTL 0x46020
+#define SPLL_CTL 0x46020
#define SPLL_PLL_ENABLE (1<<31)
#define SPLL_PLL_SCC (1<<28)
#define SPLL_PLL_NON_SCC (2<<28)
-#define SPLL_PLL_FREQ_810MHz (0<<26)
-#define SPLL_PLL_FREQ_1350MHz (1<<26)
+#define SPLL_PLL_FREQ_810MHz (0<<26)
+#define SPLL_PLL_FREQ_1350MHz (1<<26)
/* WRPLL */
-#define WRPLL_CTL1 0x46040
-#define WRPLL_CTL2 0x46060
-#define WRPLL_PLL_ENABLE (1<<31)
-#define WRPLL_PLL_SELECT_SSC (0x01<<28)
-#define WRPLL_PLL_SELECT_NON_SCC (0x02<<28)
+#define WRPLL_CTL1 0x46040
+#define WRPLL_CTL2 0x46060
+#define WRPLL_PLL_ENABLE (1<<31)
+#define WRPLL_PLL_SELECT_SSC (0x01<<28)
+#define WRPLL_PLL_SELECT_NON_SCC (0x02<<28)
#define WRPLL_PLL_SELECT_LCPLL_2700 (0x03<<28)
/* WRPLL divider programming */
-#define WRPLL_DIVIDER_REFERENCE(x) ((x)<<0)
-#define WRPLL_DIVIDER_POST(x) ((x)<<8)
-#define WRPLL_DIVIDER_FEEDBACK(x) ((x)<<16)
+#define WRPLL_DIVIDER_REFERENCE(x) ((x)<<0)
+#define WRPLL_DIVIDER_POST(x) ((x)<<8)
+#define WRPLL_DIVIDER_FEEDBACK(x) ((x)<<16)
/* Port clock selection */
#define PORT_CLK_SEL_A 0x46100
#define PORT_CLK_SEL_B 0x46104
-#define PORT_CLK_SEL(port) _PORT(port, \
- PORT_CLK_SEL_A, \
- PORT_CLK_SEL_B)
+#define PORT_CLK_SEL(port) _PORT(port, PORT_CLK_SEL_A, PORT_CLK_SEL_B)
#define PORT_CLK_SEL_LCPLL_2700 (0<<29)
#define PORT_CLK_SEL_LCPLL_1350 (1<<29)
#define PORT_CLK_SEL_LCPLL_810 (2<<29)
-#define PORT_CLK_SEL_SPLL (3<<29)
+#define PORT_CLK_SEL_SPLL (3<<29)
#define PORT_CLK_SEL_WRPLL1 (4<<29)
#define PORT_CLK_SEL_WRPLL2 (5<<29)
/* Pipe clock selection */
#define PIPE_CLK_SEL_A 0x46140
#define PIPE_CLK_SEL_B 0x46144
-#define PIPE_CLK_SEL(pipe) _PIPE(pipe, \
- PIPE_CLK_SEL_A, \
- PIPE_CLK_SEL_B)
+#define PIPE_CLK_SEL(pipe) _PIPE(pipe, PIPE_CLK_SEL_A, PIPE_CLK_SEL_B)
/* For each pipe, we need to select the corresponding port clock */
-#define PIPE_CLK_SEL_DISABLED (0x0<<29)
-#define PIPE_CLK_SEL_PORT(x) ((x+1)<<29)
+#define PIPE_CLK_SEL_DISABLED (0x0<<29)
+#define PIPE_CLK_SEL_PORT(x) ((x+1)<<29)
/* LCPLL Control */
-#define LCPLL_CTL 0x130040
+#define LCPLL_CTL 0x130040
#define LCPLL_PLL_DISABLE (1<<31)
#define LCPLL_PLL_LOCK (1<<30)
-#define LCPLL_CD_CLOCK_DISABLE (1<<25)
+#define LCPLL_CD_CLOCK_DISABLE (1<<25)
#define LCPLL_CD2X_CLOCK_DISABLE (1<<23)
/* Pipe WM_LINETIME - watermark line time */
#define PIPE_WM_LINETIME_A 0x45270
#define PIPE_WM_LINETIME_B 0x45274
-#define PIPE_WM_LINETIME(pipe) _PIPE(pipe, \
- PIPE_WM_LINETIME_A, \
- PIPE_WM_LINETIME_B)
-#define PIPE_WM_LINETIME_MASK (0x1ff)
-#define PIPE_WM_LINETIME_TIME(x) ((x))
+#define PIPE_WM_LINETIME(pipe) _PIPE(pipe, PIPE_WM_LINETIME_A, \
+ PIPE_WM_LINETIME_B)
+#define PIPE_WM_LINETIME_MASK (0x1ff)
+#define PIPE_WM_LINETIME_TIME(x) ((x))
#define PIPE_WM_LINETIME_IPS_LINETIME_MASK (0x1ff<<16)
-#define PIPE_WM_LINETIME_IPS_LINETIME(x) ((x)<<16)
+#define PIPE_WM_LINETIME_IPS_LINETIME(x) ((x)<<16)
/* SFUSE_STRAP */
-#define SFUSE_STRAP 0xc2014
+#define SFUSE_STRAP 0xc2014
#define SFUSE_STRAP_DDIB_DETECTED (1<<2)
#define SFUSE_STRAP_DDIC_DETECTED (1<<1)
#define SFUSE_STRAP_DDID_DETECTED (1<<0)
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (4 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 5/8] drm/i915: reindent Haswell register definitions Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-09 16:43 ` Daniel Vetter
2012-08-08 17:15 ` [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
` (3 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
By looking at the current way we're using these definitions I don't
think this commit will fix any bug, but programmers from the future
are evil and will certainly find ways to combine macro expansion with
operator precedence to introduce bugs that are hard to find.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index af41d03..321ae72 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4395,8 +4395,8 @@
/* LPT PIXCLK_GATE */
#define PIXCLK_GATE 0xC6020
-#define PIXCLK_GATE_UNGATE 1<<0
-#define PIXCLK_GATE_GATE 0<<0
+#define PIXCLK_GATE_UNGATE (1<<0)
+#define PIXCLK_GATE_GATE (0<<0)
/* SPLL */
#define SPLL_CTL 0x46020
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (5 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-09 10:56 ` Jani Nikula
2012-08-08 17:15 ` [PATCH 8/8] drm/i915: try to use WR PLL 2 Paulo Zanoni
` (2 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
If we don't find the exact refresh rate, go with the next one. This
makes some modes work for me. They won't have the best settings, but
will at least have something. Just returning from this function when
we don't find the perfect settings does not help us at all.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ff03a3a..db242cf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
u16 r2; /* Reference divider */
};
-/* Table of matching values for WRPLL clocks programming for each frequency */
+/* Table of matching values for WRPLL clocks programming for each frequency.
+ * The code assumes this table is sorted. */
static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
{19750, 38, 25, 18},
{20000, 48, 32, 18},
@@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
int port = intel_hdmi->ddi_port;
int pipe = intel_crtc->pipe;
- int p, n2, r2, valid=0;
+ int p, n2, r2;
u32 temp, i;
/* On Haswell, we need to enable the clocks and prepare DDI function to
@@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
*/
DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
- for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
- if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
- p = wrpll_tmds_clock_table[i].p;
- n2 = wrpll_tmds_clock_table[i].n2;
- r2 = wrpll_tmds_clock_table[i].r2;
+ for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
+ if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
+ break;
- DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
- crtc->mode.clock,
- p, n2, r2);
+ if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
+ i--;
- valid = 1;
- break;
- }
- }
+ if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
+ DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
+ wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
- if (!valid) {
- DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
- crtc->mode.clock);
- return;
- }
+ p = wrpll_tmds_clock_table[i].p;
+ n2 = wrpll_tmds_clock_table[i].n2;
+ r2 = wrpll_tmds_clock_table[i].r2;
/* Enable LCPLL if disabled */
temp = I915_READ(LCPLL_CTL);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 8/8] drm/i915: try to use WR PLL 2
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (6 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
@ 2012-08-08 17:15 ` Paulo Zanoni
2012-08-09 11:32 ` Jani Nikula
2012-08-09 11:40 ` [PATCH 0/8] Haswell HDMI fixes Jani Nikula
2012-08-10 13:03 ` [PATCH] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
9 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-08 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
The current situation is: we use WR PLL1 for everything, so if we have
2 HDMI outputs they will share the same PLL. As a consequence, when
you set a mode on HDMI2, HDMI1 will change its refresh rate. If the
modes are too different, setting a mode on HDMI2 may make the HDMI1
screen blank (with one of those error messages from your monitor).
So now we at least try to use WR PLL 2. This will improve the case
where we have 2 HDMI outputs and don't keep crazily changing ports,
but it's still not the perfect solution:
- We need to select PORT_CLK_SEL_NONE when we stop using a port, so we
will be able to reuse its PLL.
- We need to move the whole DDI PLL selection code to a place where we
can properly fail and return an error code, possibly undoing the
mode set. Right now, instead of failing we're hijacking WR PLL 2.
This patch is not the perfect solution, but it's at least better than
the current one. Future patches will fix the remaining problems.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 41 ++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index db242cf..80b9902 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -659,7 +659,10 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
int port = intel_hdmi->ddi_port;
int pipe = intel_crtc->pipe;
- int p, n2, r2;
+ uint32_t pll_reg[] = {WRPLL_CTL1, WRPLL_CTL2};
+ uint32_t pll_sel[] = {PORT_CLK_SEL_WRPLL1, PORT_CLK_SEL_WRPLL2};
+ int p, n2, r2, pll;
+ bool used;
u32 temp, i;
/* On Haswell, we need to enable the clocks and prepare DDI function to
@@ -667,6 +670,35 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
*/
DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
+ for (pll = 0; pll < 2; pll++) {
+ used = false;
+ for (i = PORT_A; i <= PORT_E; i++) {
+ if (i == port)
+ continue;
+
+ if (I915_READ(PORT_CLK_SEL(i)) == pll_sel[pll]) {
+ used = true;
+ break;
+ }
+ }
+ if (!used)
+ break;
+ }
+ if (pll == 2) {
+ /* FIXME: just claiming failure and returning from this function
+ * won't help us at all. So instead we hijack WR PLL 2 and hope
+ * we don't break the other output (if the refresh rates are
+ * similar we may survive). We should definitely move the PLL
+ * code to somewhere else, where we may be able to properly
+ * fail. Also, we should write code to select PORT_CLK_SEL_NONE
+ * when we stop using the port, so other ports will be able to
+ * reuse the WR PLL. */
+ DRM_ERROR("No WR PLL available\n");
+ pll = 1;
+ }
+
+ DRM_DEBUG_KMS("Using WR PLL %d\n", pll+1);
+
for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
break;
@@ -688,9 +720,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
I915_WRITE(LCPLL_CTL,
temp & ~LCPLL_PLL_DISABLE);
- /* Configure WR PLL 1, program the correct divider values for
- * the desired frequency and wait for warmup */
- I915_WRITE(WRPLL_CTL1,
+ I915_WRITE(pll_reg[pll],
WRPLL_PLL_ENABLE |
WRPLL_PLL_SELECT_LCPLL_2700 |
WRPLL_DIVIDER_REFERENCE(r2) |
@@ -702,8 +732,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
/* Use WRPLL1 clock to drive the output to the port, and tell the pipe to use
* this port for connection.
*/
- I915_WRITE(PORT_CLK_SEL(port),
- PORT_CLK_SEL_WRPLL1);
+ I915_WRITE(PORT_CLK_SEL(port), pll_sel[pll]);
I915_WRITE(PIPE_CLK_SEL(pipe),
PIPE_CLK_SEL_PORT(port));
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field
2012-08-08 17:15 ` [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field Paulo Zanoni
@ 2012-08-09 9:55 ` Jani Nikula
2012-08-09 16:40 ` Daniel Vetter
0 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2012-08-09 9:55 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Correctly erase the values previously set and also check for 6pbc and
> 10bpc.
6 *bpc*. But is the 6 or 10 bpc usage below correct anyway, as the spec
says they are not supported by HDMI or DVI? (Either way, the erase part
of the patch is valid.)
BR,
Jani.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_ddi.c | 26 ++++++++++++++++++++------
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 896b279..f3fafb8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4308,6 +4308,7 @@
> #define PIPE_DDI_MODE_SELECT_DP_SST (2<<24)
> #define PIPE_DDI_MODE_SELECT_DP_MST (3<<24)
> #define PIPE_DDI_MODE_SELECT_FDI (4<<24)
> +#define PIPE_DDI_BPC_MASK (7<<20)
> #define PIPE_DDI_BPC_8 (0<<20)
> #define PIPE_DDI_BPC_10 (1<<20)
> #define PIPE_DDI_BPC_6 (2<<20)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1fbd67c..8b38359 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -725,14 +725,28 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> /* Enable PIPE_DDI_FUNC_CTL for the pipe to work in HDMI mode */
> temp = I915_READ(DDI_FUNC_CTL(pipe));
> temp &= ~PIPE_DDI_PORT_MASK;
> - temp &= ~PIPE_DDI_BPC_12;
> + temp &= ~PIPE_DDI_BPC_MASK;
> temp &= ~PIPE_DDI_MODE_SELECT_MASK;
> temp &= ~(PIPE_DDI_PVSYNC | PIPE_DDI_PHSYNC);
> - temp |= PIPE_DDI_SELECT_PORT(port) |
> - ((intel_crtc->bpp > 24) ?
> - PIPE_DDI_BPC_12 :
> - PIPE_DDI_BPC_8) |
> - PIPE_DDI_FUNC_ENABLE;
> + temp |= PIPE_DDI_FUNC_ENABLE | PIPE_DDI_SELECT_PORT(port);
> +
> + switch (intel_crtc->bpp) {
> + case 18:
> + temp |= PIPE_DDI_BPC_6;
> + break;
> + case 24:
> + temp |= PIPE_DDI_BPC_8;
> + break;
> + case 30:
> + temp |= PIPE_DDI_BPC_10;
> + break;
> + case 36:
> + temp |= PIPE_DDI_BPC_12;
> + break;
> + default:
> + WARN(1, "%d bpp unsupported by pipe DDI function\n",
> + intel_crtc->bpp);
> + }
>
> if (intel_hdmi->has_hdmi_sink)
> temp |= PIPE_DDI_MODE_SELECT_HDMI;
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings
2012-08-08 17:15 ` [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
@ 2012-08-09 10:56 ` Jani Nikula
2012-08-09 17:30 ` Paulo Zanoni
0 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2012-08-09 10:56 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If we don't find the exact refresh rate, go with the next one. This
> makes some modes work for me. They won't have the best settings, but
> will at least have something. Just returning from this function when
> we don't find the perfect settings does not help us at all.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ff03a3a..db242cf 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
> u16 r2; /* Reference divider */
> };
>
> -/* Table of matching values for WRPLL clocks programming for each frequency */
> +/* Table of matching values for WRPLL clocks programming for each frequency.
> + * The code assumes this table is sorted. */
I spotted some duplicate lines in the table. Perhaps you could remove
them while at it?
> static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> {19750, 38, 25, 18},
> {20000, 48, 32, 18},
> @@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> int port = intel_hdmi->ddi_port;
> int pipe = intel_crtc->pipe;
> - int p, n2, r2, valid=0;
> + int p, n2, r2;
> u32 temp, i;
>
> /* On Haswell, we need to enable the clocks and prepare DDI function to
> @@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> */
> DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>
> - for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
> - if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
> - p = wrpll_tmds_clock_table[i].p;
> - n2 = wrpll_tmds_clock_table[i].n2;
> - r2 = wrpll_tmds_clock_table[i].r2;
> + for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
> + if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
> + break;
>
> - DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
> - crtc->mode.clock,
> - p, n2, r2);
You drop this debug message. Is it intentional? The below DRM_INFO will
only be printed if an exact match is not found.
BR,
Jani.
> + if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
> + i--;
>
> - valid = 1;
> - break;
> - }
> - }
> + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
> + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
> + wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
>
> - if (!valid) {
> - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
> - crtc->mode.clock);
> - return;
> - }
> + p = wrpll_tmds_clock_table[i].p;
> + n2 = wrpll_tmds_clock_table[i].n2;
> + r2 = wrpll_tmds_clock_table[i].r2;
>
> /* Enable LCPLL if disabled */
> temp = I915_READ(LCPLL_CTL);
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 8/8] drm/i915: try to use WR PLL 2
2012-08-08 17:15 ` [PATCH 8/8] drm/i915: try to use WR PLL 2 Paulo Zanoni
@ 2012-08-09 11:32 ` Jani Nikula
0 siblings, 0 replies; 21+ messages in thread
From: Jani Nikula @ 2012-08-09 11:32 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The current situation is: we use WR PLL1 for everything, so if we have
> 2 HDMI outputs they will share the same PLL. As a consequence, when
> you set a mode on HDMI2, HDMI1 will change its refresh rate. If the
> modes are too different, setting a mode on HDMI2 may make the HDMI1
> screen blank (with one of those error messages from your monitor).
>
> So now we at least try to use WR PLL 2. This will improve the case
> where we have 2 HDMI outputs and don't keep crazily changing ports,
> but it's still not the perfect solution:
>
> - We need to select PORT_CLK_SEL_NONE when we stop using a port, so we
> will be able to reuse its PLL.
> - We need to move the whole DDI PLL selection code to a place where we
> can properly fail and return an error code, possibly undoing the
> mode set. Right now, instead of failing we're hijacking WR PLL 2.
>
> This patch is not the perfect solution, but it's at least better than
> the current one. Future patches will fix the remaining problems.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 41 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index db242cf..80b9902 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -659,7 +659,10 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> int port = intel_hdmi->ddi_port;
> int pipe = intel_crtc->pipe;
> - int p, n2, r2;
> + uint32_t pll_reg[] = {WRPLL_CTL1, WRPLL_CTL2};
> + uint32_t pll_sel[] = {PORT_CLK_SEL_WRPLL1, PORT_CLK_SEL_WRPLL2};
> + int p, n2, r2, pll;
> + bool used;
> u32 temp, i;
>
> /* On Haswell, we need to enable the clocks and prepare DDI function to
> @@ -667,6 +670,35 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> */
> DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>
> + for (pll = 0; pll < 2; pll++) {
Personally I'd prefer to use ARRAY_SIZE() even for small arrays like
this. YMMV.
> + used = false;
> + for (i = PORT_A; i <= PORT_E; i++) {
> + if (i == port)
> + continue;
> +
> + if (I915_READ(PORT_CLK_SEL(i)) == pll_sel[pll]) {
> + used = true;
> + break;
> + }
> + }
> + if (!used)
> + break;
> + }
I wonder if the logic (esp. the use of the "used" variable) could be
simplified by putting the inner for loop into a function. But this is
just bikeshedding, really.
> + if (pll == 2) {
> + /* FIXME: just claiming failure and returning from this function
> + * won't help us at all. So instead we hijack WR PLL 2 and hope
> + * we don't break the other output (if the refresh rates are
> + * similar we may survive). We should definitely move the PLL
> + * code to somewhere else, where we may be able to properly
> + * fail. Also, we should write code to select PORT_CLK_SEL_NONE
> + * when we stop using the port, so other ports will be able to
> + * reuse the WR PLL. */
> + DRM_ERROR("No WR PLL available\n");
> + pll = 1;
> + }
> +
> + DRM_DEBUG_KMS("Using WR PLL %d\n", pll+1);
> +
> for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
> if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
> break;
> @@ -688,9 +720,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> I915_WRITE(LCPLL_CTL,
> temp & ~LCPLL_PLL_DISABLE);
>
> - /* Configure WR PLL 1, program the correct divider values for
> - * the desired frequency and wait for warmup */
> - I915_WRITE(WRPLL_CTL1,
> + I915_WRITE(pll_reg[pll],
> WRPLL_PLL_ENABLE |
> WRPLL_PLL_SELECT_LCPLL_2700 |
> WRPLL_DIVIDER_REFERENCE(r2) |
> @@ -702,8 +732,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> /* Use WRPLL1 clock to drive the output to the port, and tell the pipe to use
> * this port for connection.
> */
> - I915_WRITE(PORT_CLK_SEL(port),
> - PORT_CLK_SEL_WRPLL1);
> + I915_WRITE(PORT_CLK_SEL(port), pll_sel[pll]);
> I915_WRITE(PIPE_CLK_SEL(pipe),
> PIPE_CLK_SEL_PORT(port));
>
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/8] Haswell HDMI fixes
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (7 preceding siblings ...)
2012-08-08 17:15 ` [PATCH 8/8] drm/i915: try to use WR PLL 2 Paulo Zanoni
@ 2012-08-09 11:40 ` Jani Nikula
2012-08-10 13:03 ` [PATCH] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
9 siblings, 0 replies; 21+ messages in thread
From: Jani Nikula @ 2012-08-09 11:40 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> HDMI already works fine on Haswell, but we still have room for improvements.
> This series will make us less dependent on the bits set by the BIOS, will fix
> cases where DVI was not working and will also improve the cases where we have 2
> HDMI monitors.
Hi Paulo, please address the bpc comment regarding patch 3/8; other than
that,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Whether to do anything about the other comments I leave at your
discretion.
>
> - Patches 1-4 are all about the DDI_FUNC_CTL register.
> - Patch 5 is to satisfy my OCD.
> - Patch 6 was spotted while writing patch 5.
> - Patches 7-8 are about setting PLLs.
>
> Paulo Zanoni (8):
> drm/i915: fix pipe DDI mode select
> drm/i915: set the DDI sync polarity bits
> drm/i915: correctly set the DDI_FUNC_CTL bpc field
> drm/i915: completely reset the value of DDI_FUNC_CTL
> drm/i915: reindent Haswell register definitions
> drm/i915: add parentheses around PIXCLK_GATE definitions
> drm/i915: try harder to find WR PLL clock settings
> drm/i915: try to use WR PLL 2
>
> drivers/gpu/drm/i915/i915_reg.h | 184 ++++++++++++++++++---------------------
> drivers/gpu/drm/i915/intel_ddi.c | 108 ++++++++++++++++-------
> 2 files changed, 163 insertions(+), 129 deletions(-)
>
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field
2012-08-09 9:55 ` Jani Nikula
@ 2012-08-09 16:40 ` Daniel Vetter
2012-08-09 16:46 ` Paulo Zanoni
0 siblings, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2012-08-09 16:40 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
On Thu, Aug 09, 2012 at 12:55:41PM +0300, Jani Nikula wrote:
> On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Correctly erase the values previously set and also check for 6pbc and
> > 10bpc.
>
> 6 *bpc*. But is the 6 or 10 bpc usage below correct anyway, as the spec
> says they are not supported by HDMI or DVI? (Either way, the erase part
> of the patch is valid.)
Iirc the intel_crtc->bpp computation should take these constraints into
account. On a quick look intel_choose_pipe_bpp_dither seems to dtrt.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions
2012-08-08 17:15 ` [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions Paulo Zanoni
@ 2012-08-09 16:43 ` Daniel Vetter
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2012-08-09 16:43 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Wed, Aug 08, 2012 at 02:15:32PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> By looking at the current way we're using these definitions I don't
> think this commit will fix any bug, but programmers from the future
> are evil and will certainly find ways to combine macro expansion with
> operator precedence to introduce bugs that are hard to find.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
I've applied all patches up to and including this one here (with the pbc
mispell fixed). I agree with Jani's bikeshed on patch 7, and I think we
need to discuss patch 8 some more on irc. I think something similar to the
pch pll sharing is required here.
Thanks for the patches&review.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field
2012-08-09 16:40 ` Daniel Vetter
@ 2012-08-09 16:46 ` Paulo Zanoni
0 siblings, 0 replies; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-09 16:46 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Paulo Zanoni
Hi
2012/8/9 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Aug 09, 2012 at 12:55:41PM +0300, Jani Nikula wrote:
>> On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Correctly erase the values previously set and also check for 6pbc and
>> > 10bpc.
>>
>> 6 *bpc*. But is the 6 or 10 bpc usage below correct anyway, as the spec
>> says they are not supported by HDMI or DVI? (Either way, the erase part
>> of the patch is valid.)
>
> Iirc the intel_crtc->bpp computation should take these constraints into
> account. On a quick look intel_choose_pipe_bpp_dither seems to dtrt.
Yes, that's exactly what I was checking right now... Also, a quick
check on the HDMI spec shows that is does support 30bpp, so 10bpc
might be allowed...
I really think that inside encoder_mode_set we really shouldn't be
doing failure checks anymore since we can't fail and return error
codes. Also, my evil plans include using this code for DP too, that's
why I covered all cases.
Still, I did some checks with testdisplay and it seems our non-24-bpp
Kernel code could use some love.
> -Daniel
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings
2012-08-09 10:56 ` Jani Nikula
@ 2012-08-09 17:30 ` Paulo Zanoni
2012-08-09 17:38 ` Daniel Vetter
0 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-09 17:30 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
Hi
2012/8/9 Jani Nikula <jani.nikula@linux.intel.com>:
> On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we don't find the exact refresh rate, go with the next one. This
>> makes some modes work for me. They won't have the best settings, but
>> will at least have something. Just returning from this function when
>> we don't find the perfect settings does not help us at all.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
>> 1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index ff03a3a..db242cf 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
>> u16 r2; /* Reference divider */
>> };
>>
>> -/* Table of matching values for WRPLL clocks programming for each frequency */
>> +/* Table of matching values for WRPLL clocks programming for each frequency.
>> + * The code assumes this table is sorted. */
>
> I spotted some duplicate lines in the table. Perhaps you could remove
> them while at it?
Good catch. I will write a V2 removing the 3 duplicated lines.
>
>> static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
>> {19750, 38, 25, 18},
>> {20000, 48, 32, 18},
>> @@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>> int port = intel_hdmi->ddi_port;
>> int pipe = intel_crtc->pipe;
>> - int p, n2, r2, valid=0;
>> + int p, n2, r2;
>> u32 temp, i;
>>
>> /* On Haswell, we need to enable the clocks and prepare DDI function to
>> @@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>> */
>> DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>>
>> - for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
>> - if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
>> - p = wrpll_tmds_clock_table[i].p;
>> - n2 = wrpll_tmds_clock_table[i].n2;
>> - r2 = wrpll_tmds_clock_table[i].r2;
>> + for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
>> + if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
>> + break;
>>
>> - DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
>> - crtc->mode.clock,
>> - p, n2, r2);
>
> You drop this debug message. Is it intentional? The below DRM_INFO will
> only be printed if an exact match is not found.
Yes. It had way more than 80 columns so I started feeling extremely
uncomfortable while reading the code and I didn't know why, until I
realized it :) </joke>
The thing is that if we actually find the mode on the table it means
we're on the "happy case", so we don't really need to pollute dmesg
even more. The refresh rate is print by drm_mode_debug_printmodeline
(or by the DRM_INFO in the unhappy case) and in case you really need
to know the extremely meaningful p, n2 and r2 values you can always
check the code. If we really need this I can always add it back... But
leaving only the "bad case" for dmesg makes it easier to spot while
reading the tons of messages we print.
>
> BR,
> Jani.
>
>> + if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
>> + i--;
>>
>> - valid = 1;
>> - break;
>> - }
>> - }
>> + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
>> + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
>> + wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
>>
>> - if (!valid) {
>> - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
>> - crtc->mode.clock);
>> - return;
>> - }
>> + p = wrpll_tmds_clock_table[i].p;
>> + n2 = wrpll_tmds_clock_table[i].n2;
>> + r2 = wrpll_tmds_clock_table[i].r2;
>>
>> /* Enable LCPLL if disabled */
>> temp = I915_READ(LCPLL_CTL);
>> --
>> 1.7.11.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings
2012-08-09 17:30 ` Paulo Zanoni
@ 2012-08-09 17:38 ` Daniel Vetter
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2012-08-09 17:38 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Aug 09, 2012 at 02:30:21PM -0300, Paulo Zanoni wrote:
> Hi
>
> 2012/8/9 Jani Nikula <jani.nikula@linux.intel.com>:
> > On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we don't find the exact refresh rate, go with the next one. This
> >> makes some modes work for me. They won't have the best settings, but
> >> will at least have something. Just returning from this function when
> >> we don't find the perfect settings does not help us at all.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
> >> 1 file changed, 14 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index ff03a3a..db242cf 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
> >> u16 r2; /* Reference divider */
> >> };
> >>
> >> -/* Table of matching values for WRPLL clocks programming for each frequency */
> >> +/* Table of matching values for WRPLL clocks programming for each frequency.
> >> + * The code assumes this table is sorted. */
> >
> > I spotted some duplicate lines in the table. Perhaps you could remove
> > them while at it?
>
> Good catch. I will write a V2 removing the 3 duplicated lines.
>
> >
> >> static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> >> {19750, 38, 25, 18},
> >> {20000, 48, 32, 18},
> >> @@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> >> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >> int port = intel_hdmi->ddi_port;
> >> int pipe = intel_crtc->pipe;
> >> - int p, n2, r2, valid=0;
> >> + int p, n2, r2;
> >> u32 temp, i;
> >>
> >> /* On Haswell, we need to enable the clocks and prepare DDI function to
> >> @@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> >> */
> >> DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
> >>
> >> - for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
> >> - if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
> >> - p = wrpll_tmds_clock_table[i].p;
> >> - n2 = wrpll_tmds_clock_table[i].n2;
> >> - r2 = wrpll_tmds_clock_table[i].r2;
> >> + for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
> >> + if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
> >> + break;
> >>
> >> - DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
> >> - crtc->mode.clock,
> >> - p, n2, r2);
> >
> > You drop this debug message. Is it intentional? The below DRM_INFO will
> > only be printed if an exact match is not found.
>
> Yes. It had way more than 80 columns so I started feeling extremely
> uncomfortable while reading the code and I didn't know why, until I
> realized it :) </joke>
>
> The thing is that if we actually find the mode on the table it means
> we're on the "happy case", so we don't really need to pollute dmesg
> even more. The refresh rate is print by drm_mode_debug_printmodeline
> (or by the DRM_INFO in the unhappy case) and in case you really need
> to know the extremely meaningful p, n2 and r2 values you can always
> check the code. If we really need this I can always add it back... But
> leaving only the "bad case" for dmesg makes it easier to spot while
> reading the tons of messages we print.
For tricky code that has different ways to get to a working state (or
different reasons to fail) I like it when every case has a debug output.
Since users tend to report bugs with all kinds of funny kernels, it's
easier to be sure what's going on if the dmesg contains a log entry to
confirm things. Maybe differentiate the two with "found exact settings"
and "using approximate mode" ...
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/i915: try harder to find WR PLL clock settings
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
` (8 preceding siblings ...)
2012-08-09 11:40 ` [PATCH 0/8] Haswell HDMI fixes Jani Nikula
@ 2012-08-10 13:03 ` Paulo Zanoni
2012-08-10 13:18 ` Jani Nikula
9 siblings, 1 reply; 21+ messages in thread
From: Paulo Zanoni @ 2012-08-10 13:03 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
If we don't find the exact refresh rate, go with the next one. This
makes some modes work for me. They won't have the best settings, but
will at least have something. Just returning from this function when
we don't find the perfect settings does not help us at all.
Version 2:
- Remove duplicate lines on the clock table.
- Add back debug message with refresh, p, n2 and r2.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ff03a3a..9584226 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
u16 r2; /* Reference divider */
};
-/* Table of matching values for WRPLL clocks programming for each frequency */
+/* Table of matching values for WRPLL clocks programming for each frequency.
+ * The code assumes this table is sorted. */
static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
{19750, 38, 25, 18},
{20000, 48, 32, 18},
@@ -277,7 +278,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
{23000, 36, 23, 15},
{23500, 40, 40, 23},
{23750, 26, 16, 14},
- {23750, 26, 16, 14},
{24000, 36, 24, 15},
{25000, 36, 25, 15},
{25175, 26, 40, 33},
@@ -437,7 +437,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
{108000, 8, 24, 15},
{108108, 8, 173, 108},
{109000, 6, 23, 19},
- {109000, 6, 23, 19},
{110000, 6, 22, 18},
{110013, 6, 22, 18},
{110250, 8, 49, 30},
@@ -614,7 +613,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
{218250, 4, 42, 26},
{218750, 4, 34, 21},
{219000, 4, 47, 29},
- {219000, 4, 47, 29},
{220000, 4, 44, 27},
{220640, 4, 49, 30},
{220750, 4, 36, 22},
@@ -658,7 +656,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
int port = intel_hdmi->ddi_port;
int pipe = intel_crtc->pipe;
- int p, n2, r2, valid=0;
+ int p, n2, r2;
u32 temp, i;
/* On Haswell, we need to enable the clocks and prepare DDI function to
@@ -666,26 +664,23 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
*/
DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
- for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
- if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
- p = wrpll_tmds_clock_table[i].p;
- n2 = wrpll_tmds_clock_table[i].n2;
- r2 = wrpll_tmds_clock_table[i].r2;
+ for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
+ if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
+ break;
- DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
- crtc->mode.clock,
- p, n2, r2);
+ if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
+ i--;
- valid = 1;
- break;
- }
- }
+ p = wrpll_tmds_clock_table[i].p;
+ n2 = wrpll_tmds_clock_table[i].n2;
+ r2 = wrpll_tmds_clock_table[i].r2;
- if (!valid) {
- DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
- crtc->mode.clock);
- return;
- }
+ if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
+ DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
+ wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
+
+ DRM_DEBUG_KMS("WR PLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n",
+ crtc->mode.clock, p, n2, r2);
/* Enable LCPLL if disabled */
temp = I915_READ(LCPLL_CTL);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/i915: try harder to find WR PLL clock settings
2012-08-10 13:03 ` [PATCH] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
@ 2012-08-10 13:18 ` Jani Nikula
2012-08-10 16:40 ` Daniel Vetter
0 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2012-08-10 13:18 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Fri, 10 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If we don't find the exact refresh rate, go with the next one. This
> makes some modes work for me. They won't have the best settings, but
> will at least have something. Just returning from this function when
> we don't find the perfect settings does not help us at all.
>
> Version 2:
> - Remove duplicate lines on the clock table.
> - Add back debug message with refresh, p, n2 and r2.
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ff03a3a..9584226 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
> u16 r2; /* Reference divider */
> };
>
> -/* Table of matching values for WRPLL clocks programming for each frequency */
> +/* Table of matching values for WRPLL clocks programming for each frequency.
> + * The code assumes this table is sorted. */
> static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> {19750, 38, 25, 18},
> {20000, 48, 32, 18},
> @@ -277,7 +278,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> {23000, 36, 23, 15},
> {23500, 40, 40, 23},
> {23750, 26, 16, 14},
> - {23750, 26, 16, 14},
> {24000, 36, 24, 15},
> {25000, 36, 25, 15},
> {25175, 26, 40, 33},
> @@ -437,7 +437,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> {108000, 8, 24, 15},
> {108108, 8, 173, 108},
> {109000, 6, 23, 19},
> - {109000, 6, 23, 19},
> {110000, 6, 22, 18},
> {110013, 6, 22, 18},
> {110250, 8, 49, 30},
> @@ -614,7 +613,6 @@ static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> {218250, 4, 42, 26},
> {218750, 4, 34, 21},
> {219000, 4, 47, 29},
> - {219000, 4, 47, 29},
> {220000, 4, 44, 27},
> {220640, 4, 49, 30},
> {220750, 4, 36, 22},
> @@ -658,7 +656,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> int port = intel_hdmi->ddi_port;
> int pipe = intel_crtc->pipe;
> - int p, n2, r2, valid=0;
> + int p, n2, r2;
> u32 temp, i;
>
> /* On Haswell, we need to enable the clocks and prepare DDI function to
> @@ -666,26 +664,23 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> */
> DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>
> - for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
> - if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
> - p = wrpll_tmds_clock_table[i].p;
> - n2 = wrpll_tmds_clock_table[i].n2;
> - r2 = wrpll_tmds_clock_table[i].r2;
> + for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
> + if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
> + break;
>
> - DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
> - crtc->mode.clock,
> - p, n2, r2);
> + if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
> + i--;
>
> - valid = 1;
> - break;
> - }
> - }
> + p = wrpll_tmds_clock_table[i].p;
> + n2 = wrpll_tmds_clock_table[i].n2;
> + r2 = wrpll_tmds_clock_table[i].r2;
>
> - if (!valid) {
> - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
> - crtc->mode.clock);
> - return;
> - }
> + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
> + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
> + wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
> +
> + DRM_DEBUG_KMS("WR PLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n",
> + crtc->mode.clock, p, n2, r2);
>
> /* Enable LCPLL if disabled */
> temp = I915_READ(LCPLL_CTL);
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/i915: try harder to find WR PLL clock settings
2012-08-10 13:18 ` Jani Nikula
@ 2012-08-10 16:40 ` Daniel Vetter
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2012-08-10 16:40 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
On Fri, Aug 10, 2012 at 04:18:45PM +0300, Jani Nikula wrote:
> On Fri, 10 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > If we don't find the exact refresh rate, go with the next one. This
> > makes some modes work for me. They won't have the best settings, but
> > will at least have something. Just returning from this function when
> > we don't find the perfect settings does not help us at all.
> >
> > Version 2:
> > - Remove duplicate lines on the clock table.
> > - Add back debug message with refresh, p, n2 and r2.
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Queued for -next, thanks for the review.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-08-10 16:40 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-08 17:15 [PATCH 0/8] Haswell HDMI fixes Paulo Zanoni
2012-08-08 17:15 ` [PATCH 1/8] drm/i915: fix pipe DDI mode select Paulo Zanoni
2012-08-08 17:15 ` [PATCH 2/8] drm/i915: set the DDI sync polarity bits Paulo Zanoni
2012-08-08 17:15 ` [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field Paulo Zanoni
2012-08-09 9:55 ` Jani Nikula
2012-08-09 16:40 ` Daniel Vetter
2012-08-09 16:46 ` Paulo Zanoni
2012-08-08 17:15 ` [PATCH 4/8] drm/i915: completely reset the value of DDI_FUNC_CTL Paulo Zanoni
2012-08-08 17:15 ` [PATCH 5/8] drm/i915: reindent Haswell register definitions Paulo Zanoni
2012-08-08 17:15 ` [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions Paulo Zanoni
2012-08-09 16:43 ` Daniel Vetter
2012-08-08 17:15 ` [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
2012-08-09 10:56 ` Jani Nikula
2012-08-09 17:30 ` Paulo Zanoni
2012-08-09 17:38 ` Daniel Vetter
2012-08-08 17:15 ` [PATCH 8/8] drm/i915: try to use WR PLL 2 Paulo Zanoni
2012-08-09 11:32 ` Jani Nikula
2012-08-09 11:40 ` [PATCH 0/8] Haswell HDMI fixes Jani Nikula
2012-08-10 13:03 ` [PATCH] drm/i915: try harder to find WR PLL clock settings Paulo Zanoni
2012-08-10 13:18 ` Jani Nikula
2012-08-10 16:40 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).