[PATCH 0/8] Haswell HDMI fixes

[PATCH 0/8] Haswell HDMI fixes

[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

[PATCH 1/8] drm/i915: fix pipe DDI mode select

[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

[PATCH 2/8] drm/i915: set the DDI sync polarity bits

[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

[PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field

[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

Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field

[Jani Nikula]

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

Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field

[Daniel Vetter]

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

Re: [PATCH 3/8] drm/i915: correctly set the DDI_FUNC_CTL bpc field

[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

[PATCH 4/8] drm/i915: completely reset the value of DDI_FUNC_CTL

[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

[PATCH 5/8] drm/i915: reindent Haswell register definitions

[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

[PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions

[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

Re: [PATCH 6/8] drm/i915: add parentheses around PIXCLK_GATE definitions

[Daniel Vetter]

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

[PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[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

Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[Jani Nikula]

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

Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[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

Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[Daniel Vetter]

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

[PATCH 8/8] drm/i915: try to use WR PLL 2

[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

Re: [PATCH 8/8] drm/i915: try to use WR PLL 2

[Jani Nikula]

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

Re: [PATCH 0/8] Haswell HDMI fixes

[Jani Nikula]

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

[PATCH] drm/i915: try harder to find WR PLL clock settings

[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

Re: [PATCH] drm/i915: try harder to find WR PLL clock settings

[Jani Nikula]

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

Re: [PATCH] drm/i915: try harder to find WR PLL clock settings

[Daniel Vetter]

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