[PATCH 0/9] some ivb fdi b/c fixes

[PATCH 0/9] some ivb fdi b/c fixes

[Daniel Vetter]

Hi all,

I've dug around in Bspec a bit to check out whether we're missing any
workarounds or have anything ordered the wrong way around in our setup sequence
for the fdi links. And indeed, there are a few small things. I have the illusion
that this patch series fixes fdi b/c issues for me, that will probably last
until the first test report.

There's still a funny w/a left that I'm working on - but it looks like it's a
voltage setting in a link somewhere that the bios should fix. So I'll likely
only add a check plus a debug printk.

The 2nd last patch introduces a new modeset hook and explains the big plans I
have for it. For this patch series it's overkill, but I'm already working on a
small haswell feature that also plugs into the same hook.

Comments, review and testing highly welcome.

Cheers, Daniel

[Now actually the right pile of patches ... ]

Daniel Vetter (9):
  drm/i915: shut up spurious message in intel_dp_get_hw_state
  drm/i915: Write the FDI RX TU size reg at the right time
  drm/i915: set FDI_RX_MISC to recommended values on CPT/PPT
  drm/i915: add comment about pch pll enabling rules
  drm/i915: CPT/PPT pch dp transcoder workaround
  drm/i915: BUG on impossible pch dp port
  drm/i915: drop unnecessary check from fdi_link_train code
  drm/i915: add ->display.modeset_global_resources callback
  drm/i915: check fdi B/C lane sharing constraint

 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/i915_reg.h      |  13 ++-
 drivers/gpu/drm/i915/intel_display.c | 175 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dp.c      |   5 +-
 4 files changed, 169 insertions(+), 25 deletions(-)

-- 
1.7.11.4

[PATCH 1/9] drm/i915: shut up spurious message in intel_dp_get_hw_state

[Daniel Vetter]

The debug message is only relevant on CPT/PPT PCH ports, so move
it into the correct if clause.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 831ef74..e8c2cd93 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1325,9 +1325,10 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 				return true;
 			}
 		}
-	}
 
-	DRM_DEBUG_KMS("No pipe for dp port 0x%x found\n", intel_dp->output_reg);
+		DRM_DEBUG_KMS("No pipe for dp port 0x%x found\n",
+			      intel_dp->output_reg);
+	}
 
 	return true;
 }
-- 
1.7.11.4

[PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Daniel Vetter]

According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
Display Mode Set Sequence" We need to write the TU size register
of the fdi RX unit _before_ starting to train the link.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0261d18..6cc9cb9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2681,9 +2681,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	/* Write the TU size bits so error detection works */
-	I915_WRITE(FDI_RX_TUSIZE1(pipe),
-		   I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
 
 	/* enable PCH FDI RX PLL, wait warmup plus DMI latency */
 	reg = FDI_RX_CTL(pipe);
@@ -2996,6 +2993,11 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	assert_transcoder_disabled(dev_priv, pipe);
 
+	/* Write the TU size bits before fdi link training, so that error
+	 * detection works. */
+	I915_WRITE(FDI_RX_TUSIZE1(pipe),
+		   I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
+
 	/* For PCH output, training FDI link */
 	dev_priv->display.fdi_link_train(crtc);
 
-- 
1.7.11.4

[PATCH 3/9] drm/i915: set FDI_RX_MISC to recommended values on CPT/PPT

[Daniel Vetter]

My machine here has the correct ones already, but better safe
than sorry. IBX has different settings for that register, and
on IBX the device defaults match the recommended values. Hence
I did not add the respective writes for IBX.

LPT needs the same settings, but that has been done already

commit 4acf518626cdad5bbf7aac9869bd4accbbfb4ad3
Author: Eugeni Dodonov <eugeni.dodonov@intel.com>
Date:   Wed Jul 4 20:15:16 2012 -0300

    drm/i915: program FDI_RX TP and FDI delays

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6cc9cb9..b19e3bb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2460,6 +2460,9 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
 	temp |= FDI_LINK_TRAIN_400MV_0DB_SNB_B;
 	I915_WRITE(reg, temp | FDI_TX_ENABLE);
 
+	I915_WRITE(FDI_RX_MISC(pipe),
+		   FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90);
+
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
 	if (HAS_PCH_CPT(dev)) {
@@ -2592,6 +2595,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 	temp |= FDI_COMPOSITE_SYNC;
 	I915_WRITE(reg, temp | FDI_TX_ENABLE);
 
+	I915_WRITE(FDI_RX_MISC(pipe),
+		   FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90);
+
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
 	temp &= ~FDI_LINK_TRAIN_AUTO;
-- 
1.7.11.4

[PATCH 4/9] drm/i915: add comment about pch pll enabling rules

[Daniel Vetter]

Atm we have a few funny issues where we enable/disable shared
pll clocks. To make it clear that we are not required to enable/
disable the pch plls together with the other pch resources (and
so should keep it running when it's used by another pipe in
a shared pll configuration) add a comment.

This note is lifted from "Graphics BSpec: vol4g North Display Engine
Registers [IVB], Display Mode Set Sequence", step 9.d. of the enable
sequence:

"Configure and enable PCH DPLL, wait for PCH DPLL warmup (Can be
done anytime before enabling PCH transcoder)."

Since fixing the pll sharing code to no longer disable shared plls
if they're still in use is more involved, let's just stick with the
comment for now.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b19e3bb..aa80ecb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3007,6 +3007,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	/* For PCH output, training FDI link */
 	dev_priv->display.fdi_link_train(crtc);
 
+	/* XXX: pch pll's can be enabled any time before we enable the PCH
+	 * transcoder, and we actually should do this to not upset any PCH
+	 * transcoder that already use the clock when we share it. */
 	intel_enable_pch_pll(intel_crtc);
 
 	if (HAS_PCH_LPT(dev)) {
-- 
1.7.11.4

[PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Daniel Vetter]

We need to set the timing override chicken bit after fdi link training
has completed and before we enable the dp transcoder. We also have to
clear that bit again after disabling the pch dp transcoder.

See "Graphics BSpec: vol4g North Display Engine Registers [IVB],
Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display
Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI
Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31:

"Workaround : Enable the override prior to enabling the transcoder.
Disable the override after disabling the transcoder."

While at it, use the _PIPE macro for the other TRANS_DP register.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h      |  8 +++++++-
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c7c4b96..84b09ee 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4062,7 +4062,7 @@
 #define TRANS_DP_CTL_A		0xe0300
 #define TRANS_DP_CTL_B		0xe1300
 #define TRANS_DP_CTL_C		0xe2300
-#define TRANS_DP_CTL(pipe)	(TRANS_DP_CTL_A + (pipe) * 0x01000)
+#define TRANS_DP_CTL(pipe)	_PIPE(pipe, TRANS_DP_CTL_A, TRANS_DP_CTL_B)
 #define  TRANS_DP_OUTPUT_ENABLE	(1<<31)
 #define  TRANS_DP_PORT_SEL_B	(0<<29)
 #define  TRANS_DP_PORT_SEL_C	(1<<29)
@@ -4082,6 +4082,12 @@
 #define  TRANS_DP_HSYNC_ACTIVE_LOW	0
 #define  TRANS_DP_SYNC_MASK	(3<<3)
 
+#define TRANS_CHICKEN_2_A	0xf0064
+#define TRANS_CHICKEN_2_B	0xf1064
+#define TRANS_CHICKEN_2_C	0xf2064
+#define TRANS_CHICKEN_2(pipe)	_PIPE(pipe, TRANS_CHICKEN_2_A, TRANS_CHICKEN_2_B)
+#define  TRANS_CHICKEN2_TIMING_OVERRIDE		(1<<31)
+
 /* SNB eDP training params */
 /* SNB A-stepping */
 #define  EDP_LINK_TRAIN_400MV_0DB_SNB_A		(0x38<<22)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index aa80ecb..8ab0fa5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2687,7 +2687,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-
 	/* enable PCH FDI RX PLL, wait warmup plus DMI latency */
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
@@ -3060,6 +3059,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
 	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
 		u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5;
+
+		/* Workaround: Set the timing override bit before enabling the
+		 * DP pch transcoder. */
+		reg = TRANS_CHICKEN2(pipe);
+		temp = I915_READ(reg);
+		temp |= TRANS_CHICKEN2_TIMING_OVERRIDE;
+		I915_WRITE(reg, temp);
+
 		reg = TRANS_DP_CTL(pipe);
 		temp = I915_READ(reg);
 		temp &= ~(TRANS_DP_PORT_SEL_MASK |
@@ -3365,6 +3372,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 			BUG(); /* wtf */
 		}
 		I915_WRITE(PCH_DPLL_SEL, temp);
+
+		/* Workaround: Clear the timing override chicken bit again. */
+		reg = TRANS_CHICKEN2(pipe);
+		temp = I915_READ(reg);
+		temp &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
+		I915_WRITE(reg, temp);
 	}
 
 	/* disable PCH DPLL */
-- 
1.7.11.4

[PATCH 6/9] drm/i915: BUG on impossible pch dp port

[Daniel Vetter]

Since it is one. We need to move this code to encoder specific callbacks
eventually, to kill all that inversion of control ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ab0fa5..991adc1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3092,9 +3092,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 			temp |= TRANS_DP_PORT_SEL_D;
 			break;
 		default:
-			DRM_DEBUG_KMS("Wrong PCH DP port return. Guess port B\n");
-			temp |= TRANS_DP_PORT_SEL_B;
-			break;
+			BUG();
 		}
 
 		I915_WRITE(reg, temp);
-- 
1.7.11.4

[PATCH 7/9] drm/i915: drop unnecessary check from fdi_link_train code

[Daniel Vetter]

They are all written for a specific north disaplay->pch combination.
So stop pretending otherwise.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 991adc1..7a9cfc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2367,11 +2367,9 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 	udelay(150);
 
 	/* Ironlake workaround, enable clock pointer after FDI enable*/
-	if (HAS_PCH_IBX(dev)) {
-		I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR);
-		I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR |
-			   FDI_RX_PHASE_SYNC_POINTER_EN);
-	}
+	I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR);
+	I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR |
+		   FDI_RX_PHASE_SYNC_POINTER_EN);
 
 	reg = FDI_RX_IIR(pipe);
 	for (tries = 0; tries < 5; tries++) {
@@ -2477,8 +2475,7 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
 	POSTING_READ(reg);
 	udelay(150);
 
-	if (HAS_PCH_CPT(dev))
-		cpt_phase_pointer_enable(dev, pipe);
+	cpt_phase_pointer_enable(dev, pipe);
 
 	for (i = 0; i < 4; i++) {
 		reg = FDI_TX_CTL(pipe);
@@ -2609,8 +2606,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 	POSTING_READ(reg);
 	udelay(150);
 
-	if (HAS_PCH_CPT(dev))
-		cpt_phase_pointer_enable(dev, pipe);
+	cpt_phase_pointer_enable(dev, pipe);
 
 	for (i = 0; i < 4; i++) {
 		reg = FDI_TX_CTL(pipe);
-- 
1.7.11.4

[PATCH 8/9] drm/i915: add ->display.modeset_global_resources callback

[Daniel Vetter]

After all relevant pipes are disabled and after we've updated all the
state with the staged state, but before we call the per-crtc
->mode_set functions there's a very natural point to set up any
shared/global resources like
- shared plls (obviously only the setup, the enabling needs to be
  separately handling with a separate refcount)
- global watermark state like the DSPARB on gmch platforms
- workaround bits that depend upon the exact global output
  configuration
- enabling the right set of refclocks
- enabling/disabling manual power wells.

Now for a lot of these things we can't move them into this function
yet, most often because we only compute the required information in
the per-crtc ->mode_set callback. Which is too late. But due to a
bunch of reasons (check-only atomic modeset, fastboot&hw state checks,
...) we need to separate the computation of that state from the actual
hw frobbery anyway. So we can move things into this new callback step-
by-step.

Others can't be moved here (or implemented at all) because our code
lacks the smarts to properly update them. E.g. the DSPARB can only be
updated when all pipes are disabled, so if we decide to change it's
value, we need to disable _all_ pipes. The infrastructure for that is
already in place (with the various pipe masks that driver the modeset
logic). But again we need to move a few things out of ->mode_set
first before we can even implement the correct decision making.

In any case, we need to start somewhere, so let's start with the
callback: Some small follow-up patches will make immediate good use of
it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b0ca786..6531d3e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -249,6 +249,7 @@ struct drm_i915_display_funcs {
 				 uint32_t sprite_width, int pixel_size);
 	void (*update_linetime_wm)(struct drm_device *dev, int pipe,
 				 struct drm_display_mode *mode);
+	void (*modeset_global_resources)(struct drm_device *dev);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_display_mode *adjusted_mode,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7a9cfc2..7009c0f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7403,6 +7403,9 @@ bool intel_set_mode(struct drm_crtc *crtc,
 	 * update the the output configuration. */
 	intel_modeset_update_state(dev, prepare_pipes);
 
+	if (dev_priv->display.modeset_global_resources)
+		dev_priv->display.modeset_global_resources(dev);
+
 	/* Set up the DPLL and any encoders state that needs to adjust or depend
 	 * on the DPLL.
 	 */
-- 
1.7.11.4

[PATCH 9/9] drm/i915: check fdi B/C lane sharing constraint

[Daniel Vetter]

And properly toggle the chicken bit in the pch to enable/disable fdi C
rx. If we don't set this bit correctly, the rx gets confused in link
training, which can result in an fdi link that silently fails to train
the link (since the corresponding register reports success). Note that
both fdi link B and C can suffer when this bit is not set correctly.

The code as-is has a few deficiencies:
- We presume all pipes use the pch which is not the case for cpu edp.
- We don't bother with disabling both pipes when we could make things
  work, e.g. when pipe B switched from 4 to 2 lanes due to a mode
  change, we don't bother updating the w/a bit.
- It's ugly.

All of these are because we compute ->fdi_lanes way too late, when
we're already setting up individual pipes. We need to have this
information in ->modeset_global_resources already, to set things up
correctly. But that is a much larger reorg of the code.

Note that we actually hit the 2 lanes limit in practice rather
quickly: Even though the 1920x1200 mode native mode of my screen fits
into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow
has much more blanking ...). Not obeying this restriction seems to
results in cute-looking digital noise.

v2: Only ever clear the chicken bit when both pipes are off.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

v3: Use the new ->modeset_global_resources callback.

v4: Move the WARNs to the right place. Oh how I hate hacks.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h      |   5 +-
 drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++--
 2 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 84b09ee..f681d01 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3810,8 +3810,9 @@
 #define SOUTH_CHICKEN1		0xc2000
 #define  FDIA_PHASE_SYNC_SHIFT_OVR	19
 #define  FDIA_PHASE_SYNC_SHIFT_EN	18
-#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
-#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
+#define  FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
+#define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
+#define  FDI_BC_BIFURCATION_SELECT	(1 << 12)
 #define SOUTH_CHICKEN2		0xc2004
 #define  DPLS_EDP_PPS_FIX_DIS	(1<<0)
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7009c0f..90617cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
 	POSTING_READ(SOUTH_CHICKEN1);
 }
 
+static void ivb_modeset_gloabl_resources(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *pipe_B_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
+	struct intel_crtc *pipe_C_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
+	uint32_t temp;
+
+	/* When everything is off disable fdi C so that we could enabled fdi B
+	 * with all lanes. XXX: This misses the case where a pipe is not using
+	 * any pch resources and so doesn't need any fdi lanes. */
+	if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) {
+		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
+		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
+
+		temp = I915_READ(SOUTH_CHICKEN1);
+		temp &= ~FDI_BC_BIFURCATION_SELECT;
+		DRM_DEBUG_KMS("disabling fdi C rx\n");
+		I915_WRITE(SOUTH_CHICKEN1, temp);
+	}
+}
+
 /* The FDI link training functions for ILK/Ibexpeak. */
 static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 {
@@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 	POSTING_READ(reg);
 	udelay(150);
 
+	DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n",
+		      I915_READ(FDI_RX_IIR(pipe)));
+
 	/* enable CPU FDI TX and PCH FDI RX */
 	reg = FDI_TX_CTL(pipe);
 	temp = I915_READ(reg);
@@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 		if (temp & FDI_RX_BIT_LOCK ||
 		    (I915_READ(reg) & FDI_RX_BIT_LOCK)) {
 			I915_WRITE(reg, temp | FDI_RX_BIT_LOCK);
-			DRM_DEBUG_KMS("FDI train 1 done.\n");
+			DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i);
 			break;
 		}
 	}
@@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 
 		if (temp & FDI_RX_SYMBOL_LOCK) {
 			I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK);
-			DRM_DEBUG_KMS("FDI train 2 done.\n");
+			DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i);
 			break;
 		}
 	}
@@ -4875,6 +4901,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	return true;
 }
 
+static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t temp;
+
+	temp = I915_READ(SOUTH_CHICKEN1);
+	if (temp & FDI_BC_BIFURCATION_SELECT)
+		return;
+
+	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
+	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
+
+	temp |= FDI_BC_BIFURCATION_SELECT;
+	DRM_DEBUG_KMS("enabling fdi C rx\n");
+	I915_WRITE(SOUTH_CHICKEN1, temp);
+	POSTING_READ(SOUTH_CHICKEN1);
+}
+
+static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *pipe_B_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
+
+	DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n",
+		      intel_crtc->pipe, intel_crtc->fdi_lanes);
+	if (intel_crtc->fdi_lanes > 4) {
+		DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n",
+			      intel_crtc->pipe, intel_crtc->fdi_lanes);
+		/* Clamp lanes to avoid programming the hw with bogus values. */
+		intel_crtc->fdi_lanes = 4;
+
+		return false;
+	}
+
+	if (dev_priv->num_pipe == 2)
+		return true;
+
+	switch (intel_crtc->pipe) {
+	case PIPE_A:
+		return true;
+	case PIPE_B:
+		if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled &&
+		    intel_crtc->fdi_lanes > 2) {
+			DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
+				      intel_crtc->pipe, intel_crtc->fdi_lanes);
+			/* Clamp lanes to avoid programming the hw with bogus values. */
+			intel_crtc->fdi_lanes = 2;
+
+			return false;
+		}
+
+		if (intel_crtc->fdi_lanes > 2)
+			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
+		else
+			cpt_enable_fdi_bc_bifurcation(dev);
+
+		return true;
+	case PIPE_C:
+		if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) {
+			if (intel_crtc->fdi_lanes > 2) {
+				DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
+					      intel_crtc->pipe, intel_crtc->fdi_lanes);
+				/* Clamp lanes to avoid programming the hw with bogus values. */
+				intel_crtc->fdi_lanes = 2;
+
+				return false;
+			}
+		} else {
+			DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
+			return false;
+		}
+
+		cpt_enable_fdi_bc_bifurcation(dev);
+
+		return true;
+	default:
+		BUG();
+	}
+}
+
 static void ironlake_set_m_n(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_display_mode *adjusted_mode)
@@ -5073,7 +5181,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	struct intel_encoder *encoder;
 	u32 temp;
 	int ret;
-	bool dither;
+	bool dither, fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5210,8 +5318,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
+	/* Note, this also computes intel_crtc->fdi_lanes which is used below in
+	 * ironlake_check_fdi_lanes. */
 	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
+	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
+
 	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
 
@@ -5229,7 +5341,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
 
-	return ret;
+	return fdi_config_ok ? ret : -EINVAL;
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
@@ -8185,6 +8297,8 @@ static void intel_init_display(struct drm_device *dev)
 			/* FIXME: detect B0+ stepping and use auto training */
 			dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
 			dev_priv->display.write_eld = ironlake_write_eld;
+			dev_priv->display.modeset_global_resources =
+				ivb_modeset_gloabl_resources;
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 			dev_priv->display.write_eld = haswell_write_eld;
-- 
1.7.11.4

Re: [PATCH 1/9] drm/i915: shut up spurious message in intel_dp_get_hw_state

[Paulo Zanoni]

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The debug message is only relevant on CPT/PPT PCH ports, so move
> it into the correct if clause.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 831ef74..e8c2cd93 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1325,9 +1325,10 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
>                                 return true;
>                         }
>                 }
> -       }
>
> -       DRM_DEBUG_KMS("No pipe for dp port 0x%x found\n", intel_dp->output_reg);
> +               DRM_DEBUG_KMS("No pipe for dp port 0x%x found\n",
> +                             intel_dp->output_reg);
> +       }
>
>         return true;
>  }
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> We need to set the timing override chicken bit after fdi link training
> has completed and before we enable the dp transcoder. We also have to
> clear that bit again after disabling the pch dp transcoder.
>
> See "Graphics BSpec: vol4g North Display Engine Registers [IVB],
> Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display
> Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI
> Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31:
>
> "Workaround : Enable the override prior to enabling the transcoder.
> Disable the override after disabling the transcoder."
>
> While at it, use the _PIPE macro for the other TRANS_DP register.

What confuses me is that the bit name is actually "Autotrain TimingGen
Override" and after a quick check it seems we don't use autotrain.
Also, by looking at the IVB mode set sequence it seems Auto Train is
the *recommended* method since manual is listed as "testing only". So
I'm not sure if we need this code now, but it seems we could try to
implement the auto train...

Also, I'd move the code that sets/unsets the chicken bit to
intel_enable_transcoder/intel_disable_transcoder.

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  8 +++++++-
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c7c4b96..84b09ee 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4062,7 +4062,7 @@
>  #define TRANS_DP_CTL_A         0xe0300
>  #define TRANS_DP_CTL_B         0xe1300
>  #define TRANS_DP_CTL_C         0xe2300
> -#define TRANS_DP_CTL(pipe)     (TRANS_DP_CTL_A + (pipe) * 0x01000)
> +#define TRANS_DP_CTL(pipe)     _PIPE(pipe, TRANS_DP_CTL_A, TRANS_DP_CTL_B)
>  #define  TRANS_DP_OUTPUT_ENABLE        (1<<31)
>  #define  TRANS_DP_PORT_SEL_B   (0<<29)
>  #define  TRANS_DP_PORT_SEL_C   (1<<29)
> @@ -4082,6 +4082,12 @@
>  #define  TRANS_DP_HSYNC_ACTIVE_LOW     0
>  #define  TRANS_DP_SYNC_MASK    (3<<3)
>
> +#define TRANS_CHICKEN_2_A      0xf0064
> +#define TRANS_CHICKEN_2_B      0xf1064
> +#define TRANS_CHICKEN_2_C      0xf2064
> +#define TRANS_CHICKEN_2(pipe)  _PIPE(pipe, TRANS_CHICKEN_2_A, TRANS_CHICKEN_2_B)
> +#define  TRANS_CHICKEN2_TIMING_OVERRIDE                (1<<31)
> +
>  /* SNB eDP training params */
>  /* SNB A-stepping */
>  #define  EDP_LINK_TRAIN_400MV_0DB_SNB_A                (0x38<<22)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index aa80ecb..8ab0fa5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2687,7 +2687,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
>         int pipe = intel_crtc->pipe;
>         u32 reg, temp;
>
> -
>         /* enable PCH FDI RX PLL, wait warmup plus DMI latency */
>         reg = FDI_RX_CTL(pipe);
>         temp = I915_READ(reg);
> @@ -3060,6 +3059,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>             (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>              intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
>                 u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5;
> +
> +               /* Workaround: Set the timing override bit before enabling the
> +                * DP pch transcoder. */
> +               reg = TRANS_CHICKEN2(pipe);
> +               temp = I915_READ(reg);
> +               temp |= TRANS_CHICKEN2_TIMING_OVERRIDE;
> +               I915_WRITE(reg, temp);
> +
>                 reg = TRANS_DP_CTL(pipe);
>                 temp = I915_READ(reg);
>                 temp &= ~(TRANS_DP_PORT_SEL_MASK |
> @@ -3365,6 +3372,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>                         BUG(); /* wtf */
>                 }
>                 I915_WRITE(PCH_DPLL_SEL, temp);
> +
> +               /* Workaround: Clear the timing override chicken bit again. */
> +               reg = TRANS_CHICKEN2(pipe);
> +               temp = I915_READ(reg);
> +               temp &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
> +               I915_WRITE(reg, temp);
>         }
>
>         /* disable PCH DPLL */
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 6/9] drm/i915: BUG on impossible pch dp port

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Since it is one. We need to move this code to encoder specific callbacks
> eventually, to kill all that inversion of control ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ab0fa5..991adc1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3092,9 +3092,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>                         temp |= TRANS_DP_PORT_SEL_D;
>                         break;
>                 default:
> -                       DRM_DEBUG_KMS("Wrong PCH DP port return. Guess port B\n");
> -                       temp |= TRANS_DP_PORT_SEL_B;
> -                       break;
> +                       BUG();
>                 }
>
>                 I915_WRITE(reg, temp);
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 7/9] drm/i915: drop unnecessary check from fdi_link_train code

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> They are all written for a specific north disaplay->pch combination.
> So stop pretending otherwise.
>

So how about we go deeper in this "stop pretending" thing and add some
checks inside intel_detect_pch printing loud messages in case the CPU
and the PCH are not soulmates?

I don't feel good about this, but, well, let's assume that soon we
will have the loud error message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 991adc1..7a9cfc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2367,11 +2367,9 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>         udelay(150);
>
>         /* Ironlake workaround, enable clock pointer after FDI enable*/
> -       if (HAS_PCH_IBX(dev)) {
> -               I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR);
> -               I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR |
> -                          FDI_RX_PHASE_SYNC_POINTER_EN);
> -       }
> +       I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR);
> +       I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR |
> +                  FDI_RX_PHASE_SYNC_POINTER_EN);
>
>         reg = FDI_RX_IIR(pipe);
>         for (tries = 0; tries < 5; tries++) {
> @@ -2477,8 +2475,7 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
>         POSTING_READ(reg);
>         udelay(150);
>
> -       if (HAS_PCH_CPT(dev))
> -               cpt_phase_pointer_enable(dev, pipe);
> +       cpt_phase_pointer_enable(dev, pipe);
>
>         for (i = 0; i < 4; i++) {
>                 reg = FDI_TX_CTL(pipe);
> @@ -2609,8 +2606,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>         POSTING_READ(reg);
>         udelay(150);
>
> -       if (HAS_PCH_CPT(dev))
> -               cpt_phase_pointer_enable(dev, pipe);
> +       cpt_phase_pointer_enable(dev, pipe);
>
>         for (i = 0; i < 4; i++) {
>                 reg = FDI_TX_CTL(pipe);
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Paulo Zanoni]

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
> Display Mode Set Sequence" We need to write the TU size register
> of the fdi RX unit _before_ starting to train the link.

Well, we are still writing it before training the link, but it's
waaaaay before :)
But I agree with the patch, it makes the code look more like our mode
set sequence docs.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0261d18..6cc9cb9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2681,9 +2681,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
>         int pipe = intel_crtc->pipe;
>         u32 reg, temp;
>
> -       /* Write the TU size bits so error detection works */
> -       I915_WRITE(FDI_RX_TUSIZE1(pipe),
> -                  I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
>
>         /* enable PCH FDI RX PLL, wait warmup plus DMI latency */
>         reg = FDI_RX_CTL(pipe);
> @@ -2996,6 +2993,11 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>
>         assert_transcoder_disabled(dev_priv, pipe);
>
> +       /* Write the TU size bits before fdi link training, so that error
> +        * detection works. */
> +       I915_WRITE(FDI_RX_TUSIZE1(pipe),
> +                  I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
> +
>         /* For PCH output, training FDI link */
>         dev_priv->display.fdi_link_train(crtc);
>
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/9] drm/i915: set FDI_RX_MISC to recommended values on CPT/PPT

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> My machine here has the correct ones already, but better safe
> than sorry. IBX has different settings for that register, and
> on IBX the device defaults match the recommended values. Hence
> I did not add the respective writes for IBX.
>
> LPT needs the same settings, but that has been done already
>
> commit 4acf518626cdad5bbf7aac9869bd4accbbfb4ad3
> Author: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Date:   Wed Jul 4 20:15:16 2012 -0300
>
>     drm/i915: program FDI_RX TP and FDI delays
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6cc9cb9..b19e3bb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2460,6 +2460,9 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
>         temp |= FDI_LINK_TRAIN_400MV_0DB_SNB_B;
>         I915_WRITE(reg, temp | FDI_TX_ENABLE);
>
> +       I915_WRITE(FDI_RX_MISC(pipe),
> +                  FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90);
> +

On my local HSW branch I changed this code on the LPT path to first
I915_READ the register, then mask and change the values we want, then
write again. This way we preserve the values set for the other bits. I
did this because I discovered I needed to change the other bits in
other places for some workarounds. So maybe you could do this for
CPT/PPT too?

>         reg = FDI_RX_CTL(pipe);
>         temp = I915_READ(reg);
>         if (HAS_PCH_CPT(dev)) {
> @@ -2592,6 +2595,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>         temp |= FDI_COMPOSITE_SYNC;
>         I915_WRITE(reg, temp | FDI_TX_ENABLE);
>
> +       I915_WRITE(FDI_RX_MISC(pipe),
> +                  FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90);
> +
>         reg = FDI_RX_CTL(pipe);
>         temp = I915_READ(reg);
>         temp &= ~FDI_LINK_TRAIN_AUTO;
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/9] drm/i915: add comment about pch pll enabling rules

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Atm we have a few funny issues where we enable/disable shared
> pll clocks. To make it clear that we are not required to enable/
> disable the pch plls together with the other pch resources (and
> so should keep it running when it's used by another pipe in
> a shared pll configuration) add a comment.
>
> This note is lifted from "Graphics BSpec: vol4g North Display Engine
> Registers [IVB], Display Mode Set Sequence", step 9.d. of the enable
> sequence:
>
> "Configure and enable PCH DPLL, wait for PCH DPLL warmup (Can be
> done anytime before enabling PCH transcoder)."
>
> Since fixing the pll sharing code to no longer disable shared plls
> if they're still in use is more involved, let's just stick with the
> comment for now.

I'm not sure what's the problem with the current code. Could you
please explain a little bit more? After a brief look at
intel_enable_pch_pll and intel_disable_pch_pll it seems our code does
try to check the pll refcount and behave correctly. I'm not an expert
of the ILK PCH pll sharing code, so I may be missing something.

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b19e3bb..aa80ecb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3007,6 +3007,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>         /* For PCH output, training FDI link */
>         dev_priv->display.fdi_link_train(crtc);
>
> +       /* XXX: pch pll's can be enabled any time before we enable the PCH
> +        * transcoder, and we actually should do this to not upset any PCH
> +        * transcoder that already use the clock when we share it. */
>         intel_enable_pch_pll(intel_crtc);
>
>         if (HAS_PCH_LPT(dev)) {
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 8/9] drm/i915: add ->display.modeset_global_resources callback

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> After all relevant pipes are disabled and after we've updated all the
> state with the staged state, but before we call the per-crtc
> ->mode_set functions there's a very natural point to set up any
> shared/global resources like
> - shared plls (obviously only the setup, the enabling needs to be
>   separately handling with a separate refcount)
> - global watermark state like the DSPARB on gmch platforms
> - workaround bits that depend upon the exact global output
>   configuration
> - enabling the right set of refclocks
> - enabling/disabling manual power wells.
>
> Now for a lot of these things we can't move them into this function
> yet, most often because we only compute the required information in
> the per-crtc ->mode_set callback. Which is too late. But due to a
> bunch of reasons (check-only atomic modeset, fastboot&hw state checks,
> ...) we need to separate the computation of that state from the actual
> hw frobbery anyway. So we can move things into this new callback step-
> by-step.
>
> Others can't be moved here (or implemented at all) because our code
> lacks the smarts to properly update them. E.g. the DSPARB can only be
> updated when all pipes are disabled, so if we decide to change it's
> value, we need to disable _all_ pipes. The infrastructure for that is
> already in place (with the various pipe masks that driver the modeset
> logic). But again we need to move a few things out of ->mode_set
> first before we can even implement the correct decision making.
>
> In any case, we need to start somewhere, so let's start with the
> callback: Some small follow-up patches will make immediate good use of
> it.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Yes, I can see cases where this is needed and even thought about doing
something similar for a specific task. In the future we might want a
global function that runs on all gens before calling the gen-specific
functions, but for now let's keep it this way.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 1 +
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b0ca786..6531d3e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -249,6 +249,7 @@ struct drm_i915_display_funcs {
>                                  uint32_t sprite_width, int pixel_size);
>         void (*update_linetime_wm)(struct drm_device *dev, int pipe,
>                                  struct drm_display_mode *mode);
> +       void (*modeset_global_resources)(struct drm_device *dev);
>         int (*crtc_mode_set)(struct drm_crtc *crtc,
>                              struct drm_display_mode *mode,
>                              struct drm_display_mode *adjusted_mode,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7a9cfc2..7009c0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7403,6 +7403,9 @@ bool intel_set_mode(struct drm_crtc *crtc,
>          * update the the output configuration. */
>         intel_modeset_update_state(dev, prepare_pipes);
>
> +       if (dev_priv->display.modeset_global_resources)
> +               dev_priv->display.modeset_global_resources(dev);
> +
>         /* Set up the DPLL and any encoders state that needs to adjust or depend
>          * on the DPLL.
>          */
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 9/9] drm/i915: check fdi B/C lane sharing constraint

[Paulo Zanoni]

Hi

2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> And properly toggle the chicken bit in the pch to enable/disable fdi C
> rx. If we don't set this bit correctly, the rx gets confused in link
> training, which can result in an fdi link that silently fails to train
> the link (since the corresponding register reports success). Note that
> both fdi link B and C can suffer when this bit is not set correctly.
>
> The code as-is has a few deficiencies:
> - We presume all pipes use the pch which is not the case for cpu edp.
> - We don't bother with disabling both pipes when we could make things
>   work, e.g. when pipe B switched from 4 to 2 lanes due to a mode
>   change, we don't bother updating the w/a bit.
> - It's ugly.
>
> All of these are because we compute ->fdi_lanes way too late, when
> we're already setting up individual pipes. We need to have this
> information in ->modeset_global_resources already, to set things up
> correctly. But that is a much larger reorg of the code.
>
> Note that we actually hit the 2 lanes limit in practice rather
> quickly: Even though the 1920x1200 mode native mode of my screen fits
> into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow
> has much more blanking ...). Not obeying this restriction seems to
> results in cute-looking digital noise.
>
> v2: Only ever clear the chicken bit when both pipes are off.
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

You have 2 Signed-off-by tags on this patch ^

>
> v3: Use the new ->modeset_global_resources callback.
>
> v4: Move the WARNs to the right place. Oh how I hate hacks.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   5 +-
>  drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++--
>  2 files changed, 121 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 84b09ee..f681d01 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3810,8 +3810,9 @@
>  #define SOUTH_CHICKEN1         0xc2000
>  #define  FDIA_PHASE_SYNC_SHIFT_OVR     19
>  #define  FDIA_PHASE_SYNC_SHIFT_EN      18
> -#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> -#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> +#define  FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> +#define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> +#define  FDI_BC_BIFURCATION_SELECT     (1 << 12)
>  #define SOUTH_CHICKEN2         0xc2004
>  #define  DPLS_EDP_PPS_FIX_DIS  (1<<0)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7009c0f..90617cf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
>         POSTING_READ(SOUTH_CHICKEN1);
>  }
>
> +static void ivb_modeset_gloabl_resources(struct drm_device *dev)

s/_gloabl_/_global_/

> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *pipe_B_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +       struct intel_crtc *pipe_C_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> +       uint32_t temp;
> +
> +       /* When everything is off disable fdi C so that we could enabled fdi B

s/could enabled/could enable/ ?

> +        * with all lanes. XXX: This misses the case where a pipe is not using
> +        * any pch resources and so doesn't need any fdi lanes. */
> +       if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) {
> +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> +
> +               temp = I915_READ(SOUTH_CHICKEN1);
> +               temp &= ~FDI_BC_BIFURCATION_SELECT;
> +               DRM_DEBUG_KMS("disabling fdi C rx\n");
> +               I915_WRITE(SOUTH_CHICKEN1, temp);
> +       }
> +}
> +
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>         POSTING_READ(reg);
>         udelay(150);
>
> +       DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n",
> +                     I915_READ(FDI_RX_IIR(pipe)));
> +
>         /* enable CPU FDI TX and PCH FDI RX */
>         reg = FDI_TX_CTL(pipe);
>         temp = I915_READ(reg);
> @@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>                 if (temp & FDI_RX_BIT_LOCK ||
>                     (I915_READ(reg) & FDI_RX_BIT_LOCK)) {
>                         I915_WRITE(reg, temp | FDI_RX_BIT_LOCK);
> -                       DRM_DEBUG_KMS("FDI train 1 done.\n");
> +                       DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i);
>                         break;
>                 }
>         }
> @@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>
>                 if (temp & FDI_RX_SYMBOL_LOCK) {
>                         I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK);
> -                       DRM_DEBUG_KMS("FDI train 2 done.\n");
> +                       DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i);
>                         break;
>                 }
>         }
> @@ -4875,6 +4901,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>         return true;
>  }
>
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       uint32_t temp;
> +
> +       temp = I915_READ(SOUTH_CHICKEN1);
> +       if (temp & FDI_BC_BIFURCATION_SELECT)
> +               return;
> +
> +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> +
> +       temp |= FDI_BC_BIFURCATION_SELECT;
> +       DRM_DEBUG_KMS("enabling fdi C rx\n");
> +       I915_WRITE(SOUTH_CHICKEN1, temp);
> +       POSTING_READ(SOUTH_CHICKEN1);
> +}
> +
> +static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
> +{
> +       struct drm_device *dev = intel_crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *pipe_B_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +
> +       DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n",
> +                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> +       if (intel_crtc->fdi_lanes > 4) {
> +               DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n",
> +                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> +               /* Clamp lanes to avoid programming the hw with bogus values. */
> +               intel_crtc->fdi_lanes = 4;
> +
> +               return false;
> +       }
> +
> +       if (dev_priv->num_pipe == 2)
> +               return true;
> +
> +       switch (intel_crtc->pipe) {
> +       case PIPE_A:
> +               return true;
> +       case PIPE_B:
> +               if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled &&
> +                   intel_crtc->fdi_lanes > 2) {
> +                       DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> +                                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> +                       /* Clamp lanes to avoid programming the hw with bogus values. */
> +                       intel_crtc->fdi_lanes = 2;
> +
> +                       return false;
> +               }
> +
> +               if (intel_crtc->fdi_lanes > 2)
> +                       WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +               else
> +                       cpt_enable_fdi_bc_bifurcation(dev);
> +
> +               return true;
> +       case PIPE_C:
> +               if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) {
> +                       if (intel_crtc->fdi_lanes > 2) {
> +                               DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> +                                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> +                               /* Clamp lanes to avoid programming the hw with bogus values. */
> +                               intel_crtc->fdi_lanes = 2;
> +
> +                               return false;
> +                       }
> +               } else {
> +                       DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
> +                       return false;
> +               }
> +
> +               cpt_enable_fdi_bc_bifurcation(dev);
> +
> +               return true;
> +       default:
> +               BUG();
> +       }
> +}
> +
>  static void ironlake_set_m_n(struct drm_crtc *crtc,
>                              struct drm_display_mode *mode,
>                              struct drm_display_mode *adjusted_mode)
> @@ -5073,7 +5181,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         struct intel_encoder *encoder;
>         u32 temp;
>         int ret;
> -       bool dither;
> +       bool dither, fdi_config_ok;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 switch (encoder->type) {
> @@ -5210,8 +5318,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
> +       /* Note, this also computes intel_crtc->fdi_lanes which is used below in
> +        * ironlake_check_fdi_lanes. */
>         ironlake_set_m_n(crtc, mode, adjusted_mode);
>
> +       fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
> +
>         if (is_cpu_edp)
>                 ironlake_set_pll_edp(crtc, adjusted_mode->clock);
>
> @@ -5229,7 +5341,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
>
> -       return ret;
> +       return fdi_config_ok ? ret : -EINVAL;

After reading your patch everything looks correct (even though it's
complicated, so a proper testing is better than 1000 reviews here). My
only worry is: do we properly disable all the resources we need to
disable when we fail here? I am assuming you tested this :)

My only last bikeshed would be to try to reuse "ret" instead of
"fdi_config_ok" :)

With the typos fixed, the double signed-off-by removed, the failure
path properly tested and, optionally, the fdi_config_ok variable
removed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  }
>
>  static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> @@ -8185,6 +8297,8 @@ static void intel_init_display(struct drm_device *dev)
>                         /* FIXME: detect B0+ stepping and use auto training */
>                         dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>                         dev_priv->display.write_eld = ironlake_write_eld;
> +                       dev_priv->display.modeset_global_resources =
> +                               ivb_modeset_gloabl_resources;
>                 } else if (IS_HASWELL(dev)) {
>                         dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>                         dev_priv->display.write_eld = haswell_write_eld;
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/9] drm/i915: add comment about pch pll enabling rules

[Daniel Vetter]

On Sat, Oct 27, 2012 at 2:15 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> I'm not sure what's the problem with the current code. Could you
> please explain a little bit more? After a brief look at
> intel_enable_pch_pll and intel_disable_pch_pll it seems our code does
> try to check the pll refcount and behave correctly. I'm not an expert
> of the ILK PCH pll sharing code, so I may be missing something.

Indeed, the comment is at the wrong place, but I _did_ remember the
code correctly: In get_pch_pll we unconditionally rewrite (and in
doing so, disable) the pch_pll and reset pll->on, even when the pll is
currently on and used already.

We can't just fix this, since the lvds enable code _requires_ that the
pll is off (otherwise the ldvs pin pair enabling will fail). So it's
not that easy to fix.

I'll update the comment.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Daniel Vetter]

On Sat, Oct 27, 2012 at 1:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
>> Display Mode Set Sequence" We need to write the TU size register
>> of the fdi RX unit _before_ starting to train the link.
>
> Well, we are still writing it before training the link, but it's
> waaaaay before :)
> But I agree with the patch, it makes the code look more like our mode
> set sequence docs.

Indeed, I've confused myself with the placement of the fdi pll code
quite a bit. I think that's actually a remnant of the pre-modeset
world, where we could potentially enter the modeset functions with
unknown states. I think I'll keep things as-is, and instead add a
comment to the fdi_pll function that we need to evetually move this to
the pch/fdi enabling ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Paulo Zanoni]

2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Sat, Oct 27, 2012 at 1:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>> According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
>>> Display Mode Set Sequence" We need to write the TU size register
>>> of the fdi RX unit _before_ starting to train the link.
>>
>> Well, we are still writing it before training the link, but it's
>> waaaaay before :)
>> But I agree with the patch, it makes the code look more like our mode
>> set sequence docs.
>
> Indeed, I've confused myself with the placement of the fdi pll code
> quite a bit. I think that's actually a remnant of the pre-modeset
> world, where we could potentially enter the modeset functions with
> unknown states. I think I'll keep things as-is, and instead add a
> comment to the fdi_pll function that we need to evetually move this to
> the pch/fdi enabling ...

I don't understand. Why not just apply the current patch?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 9/9] drm/i915: check fdi B/C lane sharing constraint

[Daniel Vetter]

On Sat, Oct 27, 2012 at 2:56 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> After reading your patch everything looks correct (even though it's
> complicated, so a proper testing is better than 1000 reviews here). My
> only worry is: do we properly disable all the resources we need to
> disable when we fail here? I am assuming you tested this :)

I've hoped to document/check the requirements with WARN_ONs. And I've
hit them until the code seemed right. So I think I'd be good if you
could review those, and if you see any spot that isn't properly
checked with WARNs.

> My only last bikeshed would be to try to reuse "ret" instead of
> "fdi_config_ok" :)

I've had that early, but then dropped it again. My fear is that we
can't just bail out in the middle of the modeset sequence, which is
why I've sanitized the fdi_lanes, and let the entire sequence run
through. But at the end we still need to check whether the mode fits
and return an error (so that the caller can restore things). Hence a
separate error value.

btw, I've tested this, and it seems to work as advertised.

> With the typos fixed, the double signed-off-by removed, the failure
> path properly tested and, optionally, the fdi_config_ok variable
> removed.

Will do.
-Daniel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Daniel Vetter]

On Sat, Oct 27, 2012 at 3:03 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> On Sat, Oct 27, 2012 at 1:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>>> According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
>>>> Display Mode Set Sequence" We need to write the TU size register
>>>> of the fdi RX unit _before_ starting to train the link.
>>>
>>> Well, we are still writing it before training the link, but it's
>>> waaaaay before :)
>>> But I agree with the patch, it makes the code look more like our mode
>>> set sequence docs.
>>
>> Indeed, I've confused myself with the placement of the fdi pll code
>> quite a bit. I think that's actually a remnant of the pre-modeset
>> world, where we could potentially enter the modeset functions with
>> unknown states. I think I'll keep things as-is, and instead add a
>> comment to the fdi_pll function that we need to evetually move this to
>> the pch/fdi enabling ...
>
> I don't understand. Why not just apply the current patch?

We could move the call to enable_fdi_pll to the right place, where we
enable all fdi/pch resources, instead of just the TU_SIZE write. Or do
you think that's worse?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Paulo Zanoni]

2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Sat, Oct 27, 2012 at 3:03 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>> On Sat, Oct 27, 2012 at 1:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>>> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>>>> According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
>>>>> Display Mode Set Sequence" We need to write the TU size register
>>>>> of the fdi RX unit _before_ starting to train the link.
>>>>
>>>> Well, we are still writing it before training the link, but it's
>>>> waaaaay before :)
>>>> But I agree with the patch, it makes the code look more like our mode
>>>> set sequence docs.
>>>
>>> Indeed, I've confused myself with the placement of the fdi pll code
>>> quite a bit. I think that's actually a remnant of the pre-modeset
>>> world, where we could potentially enter the modeset functions with
>>> unknown states. I think I'll keep things as-is, and instead add a
>>> comment to the fdi_pll function that we need to evetually move this to
>>> the pch/fdi enabling ...
>>
>> I don't understand. Why not just apply the current patch?
>
> We could move the call to enable_fdi_pll to the right place, where we
> enable all fdi/pch resources, instead of just the TU_SIZE write. Or do
> you think that's worse?

Our mode set sequence says the FDI PLL should be enabled way earlier
than the other FDI/PCH resources, and that's what we currently do, so
I believe it is currently being called at the "right place" or at
least close to the right place. It seems RX TU_SIZE enablement is not
part of the "FDI PLL enablement", but part of the "other FDI/PCH
resources" enablement (at least that's that the HSW mode set sequence
says), and that's why I agree with the patch.

(I have the feeling I am trying to explain to you why your patch is correct...)

Well, it's your patch, your choice :)

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

[PATCH] drm/i915: Write the FDI RX TU size reg at the right time

[Daniel Vetter]

According to "Graphics BSpec: vol4g North Display Engine Registers [IVB],
Display Mode Set Sequence" We need to write the TU size register
of the fdi RX unit _before_ starting to train the link.

v2: Paulo Zanoni pointed out that the current sequence is already
corret - I've been confused by the _very_ early call of fdi_pll_enable
in the enable sequence in our code. Hence just clarify the comment.

In the future we might want to move the fdi_pll_enable call to the
other fdi/pch resource enabling in enable_transcoder, but that's a
larger rework.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0261d18..78f8481 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2681,7 +2681,8 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	/* Write the TU size bits so error detection works */
+	/* Write the TU size bits so error detection works. This must be done
+	 * before we start to train the fdi links. */
 	I915_WRITE(FDI_RX_TUSIZE1(pipe),
 		   I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
 
-- 
1.7.11.4

Re: [PATCH 2/9] drm/i915: Write the FDI RX TU size reg at the right time

[Daniel Vetter]

On Sat, Oct 27, 2012 at 3:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Our mode set sequence says the FDI PLL should be enabled way earlier
> than the other FDI/PCH resources, and that's what we currently do, so
> I believe it is currently being called at the "right place" or at
> least close to the right place. It seems RX TU_SIZE enablement is not
> part of the "FDI PLL enablement", but part of the "other FDI/PCH
> resources" enablement (at least that's that the HSW mode set sequence
> says), and that's why I agree with the patch.
>
> (I have the feeling I am trying to explain to you why your patch is correct...)
>
> Well, it's your patch, your choice :)

Ok, I've rechecked the modeset sequence, and we need to indeed enable
fdi plls _before_ we enable the cpu pipe. I think I'll merge v1, plus
throw another patch on top to explain why fdi_pll_enable is so damn
early.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: check fdi B/C lane sharing constraint

[Daniel Vetter]

And properly toggle the chicken bit in the pch to enable/disable fdi C
rx. If we don't set this bit correctly, the rx gets confused in link
training, which can result in an fdi link that silently fails to train
the link (since the corresponding register reports success). Note that
both fdi link B and C can suffer when this bit is not set correctly.

The code as-is has a few deficiencies:
- We presume all pipes use the pch which is not the case for cpu edp.
- We don't bother with disabling both pipes when we could make things
  work, e.g. when pipe B switched from 4 to 2 lanes due to a mode
  change, we don't bother updating the w/a bit.
- It's ugly.

All of these are because we compute ->fdi_lanes way too late, when
we're already setting up individual pipes. We need to have this
information in ->modeset_global_resources already, to set things up
correctly. But that is a much larger reorg of the code.

Note that we actually hit the 2 lanes limit in practice rather
quickly: Even though the 1920x1200 mode native mode of my screen fits
into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow
has much more blanking ...). Not obeying this restriction seems to
results in cute-looking digital noise.

v2: Only ever clear the chicken bit when both pipes are off.

v3: Use the new ->modeset_global_resources callback.

v4: Move the WARNs to the right place. Oh how I hate hacks.

v5: Fix spelling, noticed by Paulo Zanoni.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h      |   5 +-
 drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++--
 2 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 84b09ee..f681d01 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3810,8 +3810,9 @@
 #define SOUTH_CHICKEN1		0xc2000
 #define  FDIA_PHASE_SYNC_SHIFT_OVR	19
 #define  FDIA_PHASE_SYNC_SHIFT_EN	18
-#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
-#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
+#define  FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
+#define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
+#define  FDI_BC_BIFURCATION_SELECT	(1 << 12)
 #define SOUTH_CHICKEN2		0xc2004
 #define  DPLS_EDP_PPS_FIX_DIS	(1<<0)
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43ac99b..befc06c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
 	POSTING_READ(SOUTH_CHICKEN1);
 }
 
+static void ivb_modeset_global_resources(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *pipe_B_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
+	struct intel_crtc *pipe_C_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
+	uint32_t temp;
+
+	/* When everything is off disable fdi C so that we could enable fdi B
+	 * with all lanes. XXX: This misses the case where a pipe is not using
+	 * any pch resources and so doesn't need any fdi lanes. */
+	if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) {
+		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
+		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
+
+		temp = I915_READ(SOUTH_CHICKEN1);
+		temp &= ~FDI_BC_BIFURCATION_SELECT;
+		DRM_DEBUG_KMS("disabling fdi C rx\n");
+		I915_WRITE(SOUTH_CHICKEN1, temp);
+	}
+}
+
 /* The FDI link training functions for ILK/Ibexpeak. */
 static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 {
@@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 	POSTING_READ(reg);
 	udelay(150);
 
+	DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n",
+		      I915_READ(FDI_RX_IIR(pipe)));
+
 	/* enable CPU FDI TX and PCH FDI RX */
 	reg = FDI_TX_CTL(pipe);
 	temp = I915_READ(reg);
@@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 		if (temp & FDI_RX_BIT_LOCK ||
 		    (I915_READ(reg) & FDI_RX_BIT_LOCK)) {
 			I915_WRITE(reg, temp | FDI_RX_BIT_LOCK);
-			DRM_DEBUG_KMS("FDI train 1 done.\n");
+			DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i);
 			break;
 		}
 	}
@@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 
 		if (temp & FDI_RX_SYMBOL_LOCK) {
 			I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK);
-			DRM_DEBUG_KMS("FDI train 2 done.\n");
+			DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i);
 			break;
 		}
 	}
@@ -4878,6 +4904,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	return true;
 }
 
+static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t temp;
+
+	temp = I915_READ(SOUTH_CHICKEN1);
+	if (temp & FDI_BC_BIFURCATION_SELECT)
+		return;
+
+	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
+	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
+
+	temp |= FDI_BC_BIFURCATION_SELECT;
+	DRM_DEBUG_KMS("enabling fdi C rx\n");
+	I915_WRITE(SOUTH_CHICKEN1, temp);
+	POSTING_READ(SOUTH_CHICKEN1);
+}
+
+static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *pipe_B_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
+
+	DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n",
+		      intel_crtc->pipe, intel_crtc->fdi_lanes);
+	if (intel_crtc->fdi_lanes > 4) {
+		DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n",
+			      intel_crtc->pipe, intel_crtc->fdi_lanes);
+		/* Clamp lanes to avoid programming the hw with bogus values. */
+		intel_crtc->fdi_lanes = 4;
+
+		return false;
+	}
+
+	if (dev_priv->num_pipe == 2)
+		return true;
+
+	switch (intel_crtc->pipe) {
+	case PIPE_A:
+		return true;
+	case PIPE_B:
+		if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled &&
+		    intel_crtc->fdi_lanes > 2) {
+			DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
+				      intel_crtc->pipe, intel_crtc->fdi_lanes);
+			/* Clamp lanes to avoid programming the hw with bogus values. */
+			intel_crtc->fdi_lanes = 2;
+
+			return false;
+		}
+
+		if (intel_crtc->fdi_lanes > 2)
+			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
+		else
+			cpt_enable_fdi_bc_bifurcation(dev);
+
+		return true;
+	case PIPE_C:
+		if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) {
+			if (intel_crtc->fdi_lanes > 2) {
+				DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
+					      intel_crtc->pipe, intel_crtc->fdi_lanes);
+				/* Clamp lanes to avoid programming the hw with bogus values. */
+				intel_crtc->fdi_lanes = 2;
+
+				return false;
+			}
+		} else {
+			DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
+			return false;
+		}
+
+		cpt_enable_fdi_bc_bifurcation(dev);
+
+		return true;
+	default:
+		BUG();
+	}
+}
+
 static void ironlake_set_m_n(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_display_mode *adjusted_mode)
@@ -5076,7 +5184,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	struct intel_encoder *encoder;
 	u32 temp;
 	int ret;
-	bool dither;
+	bool dither, fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5213,8 +5321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
+	/* Note, this also computes intel_crtc->fdi_lanes which is used below in
+	 * ironlake_check_fdi_lanes. */
 	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
+	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
+
 	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
 
@@ -5232,7 +5344,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
 
-	return ret;
+	return fdi_config_ok ? ret : -EINVAL;
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
@@ -8188,6 +8300,8 @@ static void intel_init_display(struct drm_device *dev)
 			/* FIXME: detect B0+ stepping and use auto training */
 			dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
 			dev_priv->display.write_eld = ironlake_write_eld;
+			dev_priv->display.modeset_global_resources =
+				ivb_modeset_global_resources;
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 			dev_priv->display.write_eld = haswell_write_eld;
-- 
1.7.11.4

[PATCH] drm/i915: add comment about pch pll enabling rules

[Daniel Vetter]

Atm we have a few funny issues where we enable/disable shared
pll clocks. To make it clear that we are not required to enable/
disable the pch plls together with the other pch resources (and
so should keep it running when it's used by another pipe in
a shared pll configuration) add a comment.

This note is lifted from "Graphics BSpec: vol4g North Display Engine
Registers [IVB], Display Mode Set Sequence", step 9.d. of the enable
sequence:

"Configure and enable PCH DPLL, wait for PCH DPLL warmup (Can be
done anytime before enabling PCH transcoder)."

Since fixing the pll sharing code to no longer disable shared plls
if they're still in use is more involved, let's just stick with the
comment for now.

v2: Make the comment in the code clearer, to address questions raised
by Paulo Zanoni in review.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b19e3bb..bf2356c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3007,6 +3007,13 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	/* For PCH output, training FDI link */
 	dev_priv->display.fdi_link_train(crtc);
 
+	/* XXX: pch pll's can be enabled any time before we enable the PCH
+	 * transcoder, and we actually should do this to not upset any PCH
+	 * transcoder that already use the clock when we share it.
+	 *
+	 * Note that enable_pch_pll tries to do the right thing, but get_pch_pll
+	 * unconditionally resets the pll - we need that to have the right LVDS
+	 * enable sequence. */
 	intel_enable_pch_pll(intel_crtc);
 
 	if (HAS_PCH_LPT(dev)) {
-- 
1.7.11.4

Re: [PATCH] drm/i915: add comment about pch pll enabling rules

[Paulo Zanoni]

Hi

2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Atm we have a few funny issues where we enable/disable shared
> pll clocks. To make it clear that we are not required to enable/
> disable the pch plls together with the other pch resources (and
> so should keep it running when it's used by another pipe in
> a shared pll configuration) add a comment.
>
> This note is lifted from "Graphics BSpec: vol4g North Display Engine
> Registers [IVB], Display Mode Set Sequence", step 9.d. of the enable
> sequence:
>
> "Configure and enable PCH DPLL, wait for PCH DPLL warmup (Can be
> done anytime before enabling PCH transcoder)."
>
> Since fixing the pll sharing code to no longer disable shared plls
> if they're still in use is more involved, let's just stick with the
> comment for now.
>
> v2: Make the comment in the code clearer, to address questions raised
> by Paulo Zanoni in review.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b19e3bb..bf2356c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3007,6 +3007,13 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>         /* For PCH output, training FDI link */
>         dev_priv->display.fdi_link_train(crtc);
>
> +       /* XXX: pch pll's can be enabled any time before we enable the PCH
> +        * transcoder, and we actually should do this to not upset any PCH
> +        * transcoder that already use the clock when we share it.
> +        *
> +        * Note that enable_pch_pll tries to do the right thing, but get_pch_pll
> +        * unconditionally resets the pll - we need that to have the right LVDS
> +        * enable sequence. */
>         intel_enable_pch_pll(intel_crtc);
>
>         if (HAS_PCH_LPT(dev)) {
> --
> 1.7.11.4
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: check fdi B/C lane sharing constraint

[Paulo Zanoni]

Hi

2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> And properly toggle the chicken bit in the pch to enable/disable fdi C
> rx. If we don't set this bit correctly, the rx gets confused in link
> training, which can result in an fdi link that silently fails to train
> the link (since the corresponding register reports success). Note that
> both fdi link B and C can suffer when this bit is not set correctly.
>
> The code as-is has a few deficiencies:
> - We presume all pipes use the pch which is not the case for cpu edp.
> - We don't bother with disabling both pipes when we could make things
>   work, e.g. when pipe B switched from 4 to 2 lanes due to a mode
>   change, we don't bother updating the w/a bit.
> - It's ugly.
>
> All of these are because we compute ->fdi_lanes way too late, when
> we're already setting up individual pipes. We need to have this
> information in ->modeset_global_resources already, to set things up
> correctly. But that is a much larger reorg of the code.
>
> Note that we actually hit the 2 lanes limit in practice rather
> quickly: Even though the 1920x1200 mode native mode of my screen fits
> into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow
> has much more blanking ...). Not obeying this restriction seems to
> results in cute-looking digital noise.
>
> v2: Only ever clear the chicken bit when both pipes are off.
>
> v3: Use the new ->modeset_global_resources callback.
>
> v4: Move the WARNs to the right place. Oh how I hate hacks.
>
> v5: Fix spelling, noticed by Paulo Zanoni.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   5 +-
>  drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++--
>  2 files changed, 121 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 84b09ee..f681d01 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3810,8 +3810,9 @@
>  #define SOUTH_CHICKEN1         0xc2000
>  #define  FDIA_PHASE_SYNC_SHIFT_OVR     19
>  #define  FDIA_PHASE_SYNC_SHIFT_EN      18
> -#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> -#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> +#define  FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> +#define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> +#define  FDI_BC_BIFURCATION_SELECT     (1 << 12)
>  #define SOUTH_CHICKEN2         0xc2004
>  #define  DPLS_EDP_PPS_FIX_DIS  (1<<0)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43ac99b..befc06c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
>         POSTING_READ(SOUTH_CHICKEN1);
>  }
>
> +static void ivb_modeset_global_resources(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *pipe_B_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +       struct intel_crtc *pipe_C_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> +       uint32_t temp;
> +
> +       /* When everything is off disable fdi C so that we could enable fdi B
> +        * with all lanes. XXX: This misses the case where a pipe is not using
> +        * any pch resources and so doesn't need any fdi lanes. */
> +       if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) {
> +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> +
> +               temp = I915_READ(SOUTH_CHICKEN1);
> +               temp &= ~FDI_BC_BIFURCATION_SELECT;
> +               DRM_DEBUG_KMS("disabling fdi C rx\n");
> +               I915_WRITE(SOUTH_CHICKEN1, temp);
> +       }
> +}
> +
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>         POSTING_READ(reg);
>         udelay(150);
>
> +       DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n",
> +                     I915_READ(FDI_RX_IIR(pipe)));
> +
>         /* enable CPU FDI TX and PCH FDI RX */
>         reg = FDI_TX_CTL(pipe);
>         temp = I915_READ(reg);
> @@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>                 if (temp & FDI_RX_BIT_LOCK ||
>                     (I915_READ(reg) & FDI_RX_BIT_LOCK)) {
>                         I915_WRITE(reg, temp | FDI_RX_BIT_LOCK);
> -                       DRM_DEBUG_KMS("FDI train 1 done.\n");
> +                       DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i);
>                         break;
>                 }
>         }
> @@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>
>                 if (temp & FDI_RX_SYMBOL_LOCK) {
>                         I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK);
> -                       DRM_DEBUG_KMS("FDI train 2 done.\n");
> +                       DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i);
>                         break;
>                 }
>         }
> @@ -4878,6 +4904,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>         return true;
>  }
>
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       uint32_t temp;
> +
> +       temp = I915_READ(SOUTH_CHICKEN1);
> +       if (temp & FDI_BC_BIFURCATION_SELECT)
> +               return;
> +
> +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> +
> +       temp |= FDI_BC_BIFURCATION_SELECT;
> +       DRM_DEBUG_KMS("enabling fdi C rx\n");
> +       I915_WRITE(SOUTH_CHICKEN1, temp);
> +       POSTING_READ(SOUTH_CHICKEN1);
> +}
> +
> +static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
> +{
> +       struct drm_device *dev = intel_crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *pipe_B_crtc =
> +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +
> +       DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n",
> +                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> +       if (intel_crtc->fdi_lanes > 4) {
> +               DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n",
> +                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> +               /* Clamp lanes to avoid programming the hw with bogus values. */
> +               intel_crtc->fdi_lanes = 4;
> +
> +               return false;
> +       }
> +
> +       if (dev_priv->num_pipe == 2)
> +               return true;
> +
> +       switch (intel_crtc->pipe) {
> +       case PIPE_A:
> +               return true;
> +       case PIPE_B:
> +               if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled &&
> +                   intel_crtc->fdi_lanes > 2) {
> +                       DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> +                                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> +                       /* Clamp lanes to avoid programming the hw with bogus values. */
> +                       intel_crtc->fdi_lanes = 2;
> +
> +                       return false;
> +               }
> +
> +               if (intel_crtc->fdi_lanes > 2)
> +                       WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +               else
> +                       cpt_enable_fdi_bc_bifurcation(dev);
> +
> +               return true;
> +       case PIPE_C:
> +               if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) {
> +                       if (intel_crtc->fdi_lanes > 2) {
> +                               DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> +                                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> +                               /* Clamp lanes to avoid programming the hw with bogus values. */
> +                               intel_crtc->fdi_lanes = 2;
> +
> +                               return false;
> +                       }
> +               } else {
> +                       DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
> +                       return false;
> +               }
> +
> +               cpt_enable_fdi_bc_bifurcation(dev);
> +
> +               return true;
> +       default:
> +               BUG();
> +       }
> +}
> +
>  static void ironlake_set_m_n(struct drm_crtc *crtc,
>                              struct drm_display_mode *mode,
>                              struct drm_display_mode *adjusted_mode)
> @@ -5076,7 +5184,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         struct intel_encoder *encoder;
>         u32 temp;
>         int ret;
> -       bool dither;
> +       bool dither, fdi_config_ok;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 switch (encoder->type) {
> @@ -5213,8 +5321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
> +       /* Note, this also computes intel_crtc->fdi_lanes which is used below in
> +        * ironlake_check_fdi_lanes. */
>         ironlake_set_m_n(crtc, mode, adjusted_mode);
>
> +       fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
> +
>         if (is_cpu_edp)
>                 ironlake_set_pll_edp(crtc, adjusted_mode->clock);
>
> @@ -5232,7 +5344,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
>
> -       return ret;
> +       return fdi_config_ok ? ret : -EINVAL;
>  }
>
>  static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> @@ -8188,6 +8300,8 @@ static void intel_init_display(struct drm_device *dev)
>                         /* FIXME: detect B0+ stepping and use auto training */
>                         dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>                         dev_priv->display.write_eld = ironlake_write_eld;
> +                       dev_priv->display.modeset_global_resources =
> +                               ivb_modeset_global_resources;
>                 } else if (IS_HASWELL(dev)) {
>                         dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>                         dev_priv->display.write_eld = haswell_write_eld;
> --
> 1.7.11.4
>



-- 
Paulo Zanoni

Re: [PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Daniel Vetter]

On Fri, Oct 26, 2012 at 12:21:11PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > We need to set the timing override chicken bit after fdi link training
> > has completed and before we enable the dp transcoder. We also have to
> > clear that bit again after disabling the pch dp transcoder.
> >
> > See "Graphics BSpec: vol4g North Display Engine Registers [IVB],
> > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display
> > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI
> > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31:
> >
> > "Workaround : Enable the override prior to enabling the transcoder.
> > Disable the override after disabling the transcoder."
> >
> > While at it, use the _PIPE macro for the other TRANS_DP register.
> 
> What confuses me is that the bit name is actually "Autotrain TimingGen
> Override" and after a quick check it seems we don't use autotrain.
> Also, by looking at the IVB mode set sequence it seems Auto Train is
> the *recommended* method since manual is listed as "testing only". So
> I'm not sure if we need this code now, but it seems we could try to
> implement the auto train...

Well, us using manual fdi train is a different matter altogether. And
since there's nothing in the docs indicating that we don't need this w/a
if we don't do autotraining, I prefer to just stick it into the code.

And Damien tried to do the auto training, but somehow it failed ...

> Also, I'd move the code that sets/unsets the chicken bit to
> intel_enable_transcoder/intel_disable_transcoder.

According to my reading of the modeset sequence we need to do this after
the fdi train is fully done, but before we enable the pch dp transcoder.
So I think this is the right place, and I can't actually move it anyplace
else. And since the pch dp transcoder is a cpt/ppt-only feature, it' fits
rather naturally.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Paulo Zanoni]

2012/10/29 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Oct 26, 2012 at 12:21:11PM -0200, Paulo Zanoni wrote:
>> Hi
>>
>> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> > We need to set the timing override chicken bit after fdi link training
>> > has completed and before we enable the dp transcoder. We also have to
>> > clear that bit again after disabling the pch dp transcoder.
>> >
>> > See "Graphics BSpec: vol4g North Display Engine Registers [IVB],
>> > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display
>> > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI
>> > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31:
>> >
>> > "Workaround : Enable the override prior to enabling the transcoder.
>> > Disable the override after disabling the transcoder."
>> >
>> > While at it, use the _PIPE macro for the other TRANS_DP register.
>>
>> What confuses me is that the bit name is actually "Autotrain TimingGen
>> Override" and after a quick check it seems we don't use autotrain.
>> Also, by looking at the IVB mode set sequence it seems Auto Train is
>> the *recommended* method since manual is listed as "testing only". So
>> I'm not sure if we need this code now, but it seems we could try to
>> implement the auto train...
>
> Well, us using manual fdi train is a different matter altogether. And
> since there's nothing in the docs indicating that we don't need this w/a
> if we don't do autotraining, I prefer to just stick it into the code.
>
> And Damien tried to do the auto training, but somehow it failed ...
>
>> Also, I'd move the code that sets/unsets the chicken bit to
>> intel_enable_transcoder/intel_disable_transcoder.
>
> According to my reading of the modeset sequence we need to do this after
> the fdi train is fully done, but before we enable the pch dp transcoder.

>From what I could understand your patch does the following:

Enable:
- Step 9.g (configure PCH timings)
- Apply workaround
- Step 9.h (configure TRANS_DP_CTL)
- Step 9.i (enable PCH transcoder)

Disable:
- Step 10.e (wait PCH transcoder off)
- Step 10.f (disable TRANS_DP_CTL)
- Apply workaround
- Step 10.g (disable transcoder DPLL)


My understand is that we should be doing:

Enable:
- Step 9.g (configure PCH timings)
- Step 9.h (configure TRANS_DP_CTL)
- Apply workaround
- Step 9.i (enable PCH transcoder)

Disable:
- Step 10.e (wait PCH transcoder off)
- Apply workaround
- Step 10.f (disable TRANS_DP_CTL)
- Step 10.g (disale transcoder DPLL)

The description of the chicken bit used in the workaround even says that:
"Workaround: Enable the override prior to enabling the transcoder.
Disable the override after disabling the transcoder".

That's why I said we should implement the workaround in
intel_enable_transcoder and intel_disable_transcoder: it matches the
register description and also matches what I understood from our mode
set sequence.


> So I think this is the right place, and I can't actually move it anyplace
> else. And since the pch dp transcoder is a cpt/ppt-only feature, it' fits
> rather naturally.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Daniel Vetter]

On Mon, Oct 29, 2012 at 6:02 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> The description of the chicken bit used in the workaround even says that:
> "Workaround: Enable the override prior to enabling the transcoder.
> Disable the override after disabling the transcoder".
>
> That's why I said we should implement the workaround in
> intel_enable_transcoder and intel_disable_transcoder: it matches the
> register description and also matches what I understood from our mode
> set sequence.

I think that's what my code does, we set it before/after we enable the
w/a. The difference between my code and your suggestion is that you
either have to enable this w/a unconditionally, whereas I think we
should only do this if we will use the dp transcoder on the pch, not
in general.

I agree that if we decide that this should be done in general, it's
better done together with the other pch enable/disable stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: check fdi B/C lane sharing constraint

[Daniel Vetter]

On Mon, Oct 29, 2012 at 10:06:56AM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/10/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > And properly toggle the chicken bit in the pch to enable/disable fdi C
> > rx. If we don't set this bit correctly, the rx gets confused in link
> > training, which can result in an fdi link that silently fails to train
> > the link (since the corresponding register reports success). Note that
> > both fdi link B and C can suffer when this bit is not set correctly.
> >
> > The code as-is has a few deficiencies:
> > - We presume all pipes use the pch which is not the case for cpu edp.
> > - We don't bother with disabling both pipes when we could make things
> >   work, e.g. when pipe B switched from 4 to 2 lanes due to a mode
> >   change, we don't bother updating the w/a bit.
> > - It's ugly.
> >
> > All of these are because we compute ->fdi_lanes way too late, when
> > we're already setting up individual pipes. We need to have this
> > information in ->modeset_global_resources already, to set things up
> > correctly. But that is a much larger reorg of the code.
> >
> > Note that we actually hit the 2 lanes limit in practice rather
> > quickly: Even though the 1920x1200 mode native mode of my screen fits
> > into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow
> > has much more blanking ...). Not obeying this restriction seems to
> > results in cute-looking digital noise.
> >
> > v2: Only ever clear the chicken bit when both pipes are off.
> >
> > v3: Use the new ->modeset_global_resources callback.
> >
> > v4: Move the WARNs to the right place. Oh how I hate hacks.
> >
> > v5: Fix spelling, noticed by Paulo Zanoni.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Ok, I've slurped in this patch series, safe for the 2 patches that still
need some more work. I'll resend those. I've also included the one follow
up patch with a clarifiying comment.

Thanks for the review, Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |   5 +-
> >  drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++--
> >  2 files changed, 121 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 84b09ee..f681d01 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3810,8 +3810,9 @@
> >  #define SOUTH_CHICKEN1         0xc2000
> >  #define  FDIA_PHASE_SYNC_SHIFT_OVR     19
> >  #define  FDIA_PHASE_SYNC_SHIFT_EN      18
> > -#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> > -#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> > +#define  FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2)))
> > +#define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
> > +#define  FDI_BC_BIFURCATION_SELECT     (1 << 12)
> >  #define SOUTH_CHICKEN2         0xc2004
> >  #define  DPLS_EDP_PPS_FIX_DIS  (1<<0)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 43ac99b..befc06c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
> >         POSTING_READ(SOUTH_CHICKEN1);
> >  }
> >
> > +static void ivb_modeset_global_resources(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct intel_crtc *pipe_B_crtc =
> > +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > +       struct intel_crtc *pipe_C_crtc =
> > +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > +       uint32_t temp;
> > +
> > +       /* When everything is off disable fdi C so that we could enable fdi B
> > +        * with all lanes. XXX: This misses the case where a pipe is not using
> > +        * any pch resources and so doesn't need any fdi lanes. */
> > +       if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) {
> > +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > +               WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > +
> > +               temp = I915_READ(SOUTH_CHICKEN1);
> > +               temp &= ~FDI_BC_BIFURCATION_SELECT;
> > +               DRM_DEBUG_KMS("disabling fdi C rx\n");
> > +               I915_WRITE(SOUTH_CHICKEN1, temp);
> > +       }
> > +}
> > +
> >  /* The FDI link training functions for ILK/Ibexpeak. */
> >  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> >  {
> > @@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
> >         POSTING_READ(reg);
> >         udelay(150);
> >
> > +       DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n",
> > +                     I915_READ(FDI_RX_IIR(pipe)));
> > +
> >         /* enable CPU FDI TX and PCH FDI RX */
> >         reg = FDI_TX_CTL(pipe);
> >         temp = I915_READ(reg);
> > @@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
> >                 if (temp & FDI_RX_BIT_LOCK ||
> >                     (I915_READ(reg) & FDI_RX_BIT_LOCK)) {
> >                         I915_WRITE(reg, temp | FDI_RX_BIT_LOCK);
> > -                       DRM_DEBUG_KMS("FDI train 1 done.\n");
> > +                       DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i);
> >                         break;
> >                 }
> >         }
> > @@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
> >
> >                 if (temp & FDI_RX_SYMBOL_LOCK) {
> >                         I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK);
> > -                       DRM_DEBUG_KMS("FDI train 2 done.\n");
> > +                       DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i);
> >                         break;
> >                 }
> >         }
> > @@ -4878,6 +4904,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> >         return true;
> >  }
> >
> > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       uint32_t temp;
> > +
> > +       temp = I915_READ(SOUTH_CHICKEN1);
> > +       if (temp & FDI_BC_BIFURCATION_SELECT)
> > +               return;
> > +
> > +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > +       WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > +
> > +       temp |= FDI_BC_BIFURCATION_SELECT;
> > +       DRM_DEBUG_KMS("enabling fdi C rx\n");
> > +       I915_WRITE(SOUTH_CHICKEN1, temp);
> > +       POSTING_READ(SOUTH_CHICKEN1);
> > +}
> > +
> > +static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
> > +{
> > +       struct drm_device *dev = intel_crtc->base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct intel_crtc *pipe_B_crtc =
> > +               to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > +
> > +       DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n",
> > +                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> > +       if (intel_crtc->fdi_lanes > 4) {
> > +               DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n",
> > +                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> > +               /* Clamp lanes to avoid programming the hw with bogus values. */
> > +               intel_crtc->fdi_lanes = 4;
> > +
> > +               return false;
> > +       }
> > +
> > +       if (dev_priv->num_pipe == 2)
> > +               return true;
> > +
> > +       switch (intel_crtc->pipe) {
> > +       case PIPE_A:
> > +               return true;
> > +       case PIPE_B:
> > +               if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled &&
> > +                   intel_crtc->fdi_lanes > 2) {
> > +                       DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> > +                                     intel_crtc->pipe, intel_crtc->fdi_lanes);
> > +                       /* Clamp lanes to avoid programming the hw with bogus values. */
> > +                       intel_crtc->fdi_lanes = 2;
> > +
> > +                       return false;
> > +               }
> > +
> > +               if (intel_crtc->fdi_lanes > 2)
> > +                       WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> > +               else
> > +                       cpt_enable_fdi_bc_bifurcation(dev);
> > +
> > +               return true;
> > +       case PIPE_C:
> > +               if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) {
> > +                       if (intel_crtc->fdi_lanes > 2) {
> > +                               DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n",
> > +                                             intel_crtc->pipe, intel_crtc->fdi_lanes);
> > +                               /* Clamp lanes to avoid programming the hw with bogus values. */
> > +                               intel_crtc->fdi_lanes = 2;
> > +
> > +                               return false;
> > +                       }
> > +               } else {
> > +                       DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
> > +                       return false;
> > +               }
> > +
> > +               cpt_enable_fdi_bc_bifurcation(dev);
> > +
> > +               return true;
> > +       default:
> > +               BUG();
> > +       }
> > +}
> > +
> >  static void ironlake_set_m_n(struct drm_crtc *crtc,
> >                              struct drm_display_mode *mode,
> >                              struct drm_display_mode *adjusted_mode)
> > @@ -5076,7 +5184,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >         struct intel_encoder *encoder;
> >         u32 temp;
> >         int ret;
> > -       bool dither;
> > +       bool dither, fdi_config_ok;
> >
> >         for_each_encoder_on_crtc(dev, crtc, encoder) {
> >                 switch (encoder->type) {
> > @@ -5213,8 +5321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >
> >         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> >
> > +       /* Note, this also computes intel_crtc->fdi_lanes which is used below in
> > +        * ironlake_check_fdi_lanes. */
> >         ironlake_set_m_n(crtc, mode, adjusted_mode);
> >
> > +       fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
> > +
> >         if (is_cpu_edp)
> >                 ironlake_set_pll_edp(crtc, adjusted_mode->clock);
> >
> > @@ -5232,7 +5344,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >
> >         intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
> >
> > -       return ret;
> > +       return fdi_config_ok ? ret : -EINVAL;
> >  }
> >
> >  static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> > @@ -8188,6 +8300,8 @@ static void intel_init_display(struct drm_device *dev)
> >                         /* FIXME: detect B0+ stepping and use auto training */
> >                         dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> >                         dev_priv->display.write_eld = ironlake_write_eld;
> > +                       dev_priv->display.modeset_global_resources =
> > +                               ivb_modeset_global_resources;
> >                 } else if (IS_HASWELL(dev)) {
> >                         dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >                         dev_priv->display.write_eld = haswell_write_eld;
> > --
> > 1.7.11.4
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/9] drm/i915: CPT/PPT pch dp transcoder workaround

[Paulo Zanoni]

Hi

2012/10/29 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Oct 29, 2012 at 6:02 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> The description of the chicken bit used in the workaround even says that:
>> "Workaround: Enable the override prior to enabling the transcoder.
>> Disable the override after disabling the transcoder".
>>
>> That's why I said we should implement the workaround in
>> intel_enable_transcoder and intel_disable_transcoder: it matches the
>> register description and also matches what I understood from our mode
>> set sequence.
>
> I think that's what my code does, we set it before/after we enable the
> w/a. The difference between my code and your suggestion is that you
> either have to enable this w/a unconditionally, whereas I think we
> should only do this if we will use the dp transcoder on the pch, not
> in general.
>
> I agree that if we decide that this should be done in general, it's
> better done together with the other pch enable/disable stuff.

LPT does not have DP and it still requires the workaround. Check HSW
doc "Sequence for CRT port", step 26 on enable path (inside "enable
transcoder") and step 11 on disable path (inside "disable
transcoder").

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni