Enable PC8+ states

From: Paulo Zanoni <[email protected]>

Hi

So this series is a second version of the patch I sent on April 16th. Daniel asked to write some patches to properly initialize the interrupts we're touching on the PC8+ patches, so the first 5 patches should do this.

The 6th patch is the big one enabling PC8+ and even though I still didn't address all of Daniel's concerns from his first review, the patch already works:
we get into PC10 and when we get back to PC7 things are still working.

Cheers,
Paulo

Paulo Zanoni (6):
drm/i915: add ibx_irq_preinstall
drm/i915: initialize Haswell audio interrupts drm/i915: initialize FDI RX interrupts
drm/i915: initialize the Haswell SRD interrupts drm/i915: initialize the PCH GTC interrupts drm/i915: implement HSW display sequences for package C8+

 drivers/gpu/drm/i915/i915_dma.c      |   4 +
 drivers/gpu/drm/i915/i915_drv.h      |  28 +++
 drivers/gpu/drm/i915/i915_irq.c      | 111 +++++++--
 drivers/gpu/drm/i915/i915_reg.h      |  27 +++
 drivers/gpu/drm/i915/intel_crt.c     |  26 ++-
 drivers/gpu/drm/i915/intel_display.c | 430 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      |  11 +-
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_hdmi.c    |   3 +
 9 files changed, 609 insertions(+), 34 deletions(-)

1.8.1.2

From: Paulo Zanoni <[email protected]>

So we can remove some duplicate code. All the PCHs are very similar and right now the code is the same. I plan to add more code, so we would have more duplicated code.

---
 drivers/gpu/drm/i915/i915_irq.c | 44 ++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

index 63996aa..c482e8a 100644
@@ -2472,6 +2472,25 @@ void i915_hangcheck_elapsed(unsigned long data)
 					   DRM_I915_HANGCHECK_JIFFIES));
 }
 
+static void ibx_irq_preinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_PCH_NOP(dev))
+		return;
+
+	/* south display irq */
+	I915_WRITE(SDEIMR, 0xffffffff);
+	/*
+	 * SDEIER is also touched by the interrupt handler to work around missed
+	 * PCH interrupts. Hence we can't update it after the interrupt handler
+	 * is enabled - instead we unconditionally enable all PCH interrupt
+	 * sources here, but then only unmask them as needed with SDEIMR.
+	 */
+	I915_WRITE(SDEIER, 0xffffffff);
+	POSTING_READ(SDEIER);
+}
+
 /* drm_dma.h hooks
 */
 static void ironlake_irq_preinstall(struct drm_device *dev)

@@ -2493,16 +2512,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GTIER, 0x0);
 	POSTING_READ(GTIER);
 
-	/* south display irq */
-	I915_WRITE(SDEIMR, 0xffffffff);
-	/*
-	 * SDEIER is also touched by the interrupt handler to work around missed
-	 * PCH interrupts. Hence we can't update it after the interrupt handler
-	 * is enabled - instead we unconditionally enable all PCH interrupt
-	 * sources here, but then only unmask them as needed with SDEIMR.
-	 */
-	I915_WRITE(SDEIER, 0xffffffff);
-	POSTING_READ(SDEIER);
+	ibx_irq_preinstall(dev);
 }
 
 static void ivybridge_irq_preinstall(struct drm_device *dev)

@@ -2529,19 +2539,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIER, 0x0);
 	POSTING_READ(GEN6_PMIER);
 
-	if (HAS_PCH_NOP(dev))
-		return;
-
-	/* south display irq */
-	I915_WRITE(SDEIMR, 0xffffffff);
-	/*
-	 * SDEIER is also touched by the interrupt handler to work around missed
-	 * PCH interrupts. Hence we can't update it after the interrupt handler
-	 * is enabled - instead we unconditionally enable all PCH interrupt
-	 * sources here, but then only unmask them as needed with SDEIMR.
-	 */
-	I915_WRITE(SDEIER, 0xffffffff);
-	POSTING_READ(SDEIER);
+	ibx_irq_preinstall(dev);
 }
 
 static void valleyview_irq_preinstall(struct drm_device *dev)

1.8.1.2

Queued for -next, thanks for the patch. For the other prep patches I'd like to review our interrupt code first a bit, like describe in my reply to the vecs enabling patches.
-Daniel

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

From: Paulo Zanoni <[email protected]>

These interrupts are specific to Haswell and we don't use them anywhere inside our code. If we want to allow package C8+ states we need to make sure we don't have any pending interrupts, so let's properly initialize the interrupt registers here, so when we allow C8+ states we'll only need to check if these interrupts are still disabled.

---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |  4 ++++
 2 files changed, 22 insertions(+)

index c482e8a..6706d89 100644
@@ -2539,6 +2539,13 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIER, 0x0);
 	POSTING_READ(GEN6_PMIER);
 
+	/* Audio */
+	if (IS_HASWELL(dev)) {
+		I915_WRITE(AUDIMR, 0xffffffff);
+		I915_WRITE(AUDIER, 0x0);
+		POSTING_READ(AUDIER);
+	}
+
 	ibx_irq_preinstall(dev);
 }
 

@@ -2730,6 +2737,11 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
 	POSTING_READ(GEN6_PMIER);
 
+	if (IS_HASWELL(dev)) {
+		I915_WRITE(AUDIIR, I915_READ(AUDIIR));
+		POSTING_READ(AUDIIR);
+	}
+
 	ibx_irq_postinstall(dev);
 
 	return 0;

@@ -2837,6 +2849,12 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(GTIER, 0x0);
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 
+	if (IS_HASWELL(dev)) {
+		I915_WRITE(AUDIMR, 0xffffffff);
+		I915_WRITE(AUDIER, 0x0);
+		I915_WRITE(AUDIIR, I915_READ(AUDIIR));
+	}
+
 	if (HAS_PCH_NOP(dev))
 		return;
 

index 47a9de0..d7f272a 100644
@@ -3733,6 +3733,10 @@
 #define GTIIR   0x44018
 #define GTIER   0x4401c
 
+#define AUDIMR	0x44084
+#define AUDIIR	0x44088
+#define AUDIER	0x4408c
+
 #define ILK_DISPLAY_CHICKEN2	0x42004
 /* Required on all Ironlake and Sandybridge according to the B-Spec. */
 #define  ILK_ELPIN_409_SELECT	(1 << 25)

1.8.1.2

From: Paulo Zanoni <[email protected]>

Initialize (disable) the FDI RX interrupts when initializing the driver. The code that uses those interrupts already takes care of properly unmasking the needed ones before using them.

This patch will make sure we never get undesired FDI RX interrupts, and then when we enable package C8+ states on Haswell all we'll need to do is to check that we still don't have any pending interrupt, since Haswell doesn't use the FDI RX interrupts.

---
 drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

index 6706d89..33f404e 100644
@@ -2475,6 +2475,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 static void ibx_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe;
 
 	if (HAS_PCH_NOP(dev))
 		return;

@@ -2489,6 +2490,14 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	 */
 	I915_WRITE(SDEIER, 0xffffffff);
 	POSTING_READ(SDEIER);
+
+	for_each_pipe(pipe) {
+		I915_WRITE(FDI_RX_IMR(pipe), 0xffffffff);
+		POSTING_READ(FDI_RX_IMR(pipe));
+		/* Only 1 PCH transcoder. */
+		if (HAS_PCH_LPT(dev))
+			break;
+	}
 }
 
 /* drm_dma.h hooks

@@ -2621,6 +2630,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 mask;
+	enum pipe pipe;
 
 	if (HAS_PCH_NOP(dev))
 		return;

@@ -2636,6 +2646,14 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 	I915_WRITE(SDEIMR, ~mask);
+
+	for_each_pipe(pipe) {
+		I915_WRITE(FDI_RX_IMR(pipe), 0xffffffff);
+		I915_WRITE(FDI_RX_IIR(pipe), I915_READ(FDI_RX_IIR(pipe)));
+		/* Only 1 PCH transcoder. */
+		if (HAS_PCH_LPT(dev))
+			break;
+	}
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)

@@ -2831,6 +2849,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 static void ironlake_irq_uninstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	enum pipe pipe;
 
 	if (!dev_priv)
 		return;

@@ -2863,6 +2882,14 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+
+	for_each_pipe(pipe) {
+		I915_WRITE(FDI_RX_IMR(pipe), 0xffffffff);
+		I915_WRITE(FDI_RX_IIR(pipe), I915_READ(FDI_RX_IIR(pipe)));
+		/* Only 1 PCH transcoder. */
+		if (HAS_PCH_LPT(dev))
+			break;
+	}
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

1.8.1.2

From: Paulo Zanoni <[email protected]>

Same reasons as the other ones: we're not using them so we don't want them, and this is a preparation for when we enable package C8+ states.

---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

index 33f404e..c21055e 100644
@@ -2548,11 +2548,14 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIER, 0x0);
 	POSTING_READ(GEN6_PMIER);
 
-	/* Audio */
+	/* Audio and PSR */
 	if (IS_HASWELL(dev)) {
 		I915_WRITE(AUDIMR, 0xffffffff);
 		I915_WRITE(AUDIER, 0x0);
 		POSTING_READ(AUDIER);
+
+		I915_WRITE(SRDIMR, 0xffffffff);
+		POSTING_READ(SRDIMR);
 	}
 
 	ibx_irq_preinstall(dev);

@@ -2758,6 +2761,9 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	if (IS_HASWELL(dev)) {
 		I915_WRITE(AUDIIR, I915_READ(AUDIIR));
 		POSTING_READ(AUDIIR);
+
+		I915_WRITE(SRDIIR, I915_READ(SRDIIR));
+		POSTING_READ(SRDIIR);
 	}
 
 	ibx_irq_postinstall(dev);

@@ -2872,6 +2878,9 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 		I915_WRITE(AUDIMR, 0xffffffff);
 		I915_WRITE(AUDIER, 0x0);
 		I915_WRITE(AUDIIR, I915_READ(AUDIIR));
+
+		I915_WRITE(SRDIMR, 0xffffffff);
+		I915_WRITE(SRDIIR, I915_READ(SRDIIR));
 	}
 
 	if (HAS_PCH_NOP(dev))

index d7f272a..f996e9f 100644
@@ -3737,6 +3737,9 @@
 #define AUDIIR	0x44088
 #define AUDIER	0x4408c
 
+#define SRDIMR	0x64834
+#define SRDIIR	0x64838
+
 #define ILK_DISPLAY_CHICKEN2	0x42004
 /* Required on all Ironlake and Sandybridge according to the B-Spec. */
 #define  ILK_ELPIN_409_SELECT	(1 << 25)

1.8.1.2

From: Paulo Zanoni <[email protected]>

These regsiters only exist on LPT and we still don't use them. Initialize them for the same reason as we initialize the other interrupts we don't use (SRD, FDI_RX, AUD).

Notice that we also have CPU GTC registers, but these registers are disabled when the power well is disabled, so they must be handled differently. Also, they don't affect the code for package C8+ since we need the power well disabled to enter PC8+.

---
 drivers/gpu/drm/i915/i915_irq.c | 15 +++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 2 files changed, 18 insertions(+)

index c21055e..dc2658c 100644
@@ -2498,6 +2498,11 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 		if (HAS_PCH_LPT(dev))
 			break;
 	}
+
+	if (HAS_PCH_LPT(dev)) {
+		I915_WRITE(PCH_GTCIMR, 0xffffffff);
+		POSTING_READ(PCH_GTCIMR);
+	}
 }
 
 /* drm_dma.h hooks

@@ -2657,6 +2662,11 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 		if (HAS_PCH_LPT(dev))
 			break;
 	}
+
+	if (HAS_PCH_LPT(dev)) {
+		I915_WRITE(PCH_GTCIMR, 0xffffffff);
+		I915_WRITE(PCH_GTCIIR, I915_READ(PCH_GTCIIR));
+	}
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)

@@ -2899,6 +2909,11 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 		if (HAS_PCH_LPT(dev))
 			break;
 	}
+
+	if (HAS_PCH_LPT(dev)) {
+		I915_WRITE(PCH_GTCIMR, 0xffffffff);
+		I915_WRITE(PCH_GTCIIR, I915_READ(PCH_GTCIIR));
+	}
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

index f996e9f..6a977ce 100644
@@ -3740,6 +3740,9 @@
 #define SRDIMR	0x64834
 #define SRDIIR	0x64838
 
+#define PCH_GTCIMR	0xe7054
+#define PCH_GTCIIR	0xe7058
+
 #define ILK_DISPLAY_CHICKEN2	0x42004
 /* Required on all Ironlake and Sandybridge according to the B-Spec. */
 #define  ILK_ELPIN_409_SELECT	(1 << 25)

1.8.1.2

From: Paulo Zanoni <[email protected]>

This patch implements "Display Sequences for Package C8", from the "Display Mode Set Sequence" section from the Haswell documentation.

Notice that even if we allow PC8+, there's no guarantee that we will actually enter PC8+, since the other devices also need to allow it. Also notice that we need i915.disable_power_well=1 in order to test this feature: we don't allow PC8+ if the power well is still enabled.

v2: - Rebase
- Implement many review comments form Daniel Vetter
- Call intel_prepare_ddi and i915_gem_init_swizzling when returning from C8+ so we can actually see the screen contents

---
 drivers/gpu/drm/i915/i915_dma.c      |   4 +
 drivers/gpu/drm/i915/i915_drv.h      |  28 +++
 drivers/gpu/drm/i915/i915_reg.h      |  17 ++
 drivers/gpu/drm/i915/intel_crt.c     |  26 ++-
 drivers/gpu/drm/i915/intel_display.c | 430 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      |  11 +-
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_hdmi.c    |   3 +
 8 files changed, 510 insertions(+), 12 deletions(-)

index 22534c3..fe2bd7f 100644
@@ -1639,10 +1639,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->rps.lock);
 	spin_lock_init(&dev_priv->backlight.lock);
 	mutex_init(&dev_priv->dpio_lock);
+	mutex_init(&dev_priv->c8_lock);
 
 	mutex_init(&dev_priv->rps.hw_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	dev_priv->allowing_package_c8 = false;
+	dev_priv->c8_forbid_refcnt = 1;
+
 	dev_priv->num_plane = 1;
 	if (IS_VALLEYVIEW(dev))
 		dev_priv->num_plane = 2;

index 215aa63..46b1f70 100644
@@ -538,6 +538,29 @@ struct intel_gmbus {
 	struct drm_i915_private *dev_priv;
 };
 
+struct i915_c8_saved_registers {
+	u32 de_imr;
+	u32 de_ier;
+	u32 aud_imr;
+	u32 aud_ier;
+	u32 gt_imr;
+	u32 gt_ier;
+	u32 pm_imr;
+	u32 pm_ier;
+	u32 srd_imr;
+	u32 hotplug_ctl;
+	u32 err_int;
+
+	u32 sde_imr;
+	u32 sde_ier;
+	u32 fdirx_imr;
+	u32 gtcpch_imr;
+	u32 shotplug_ctl;
+	u32 serr_int;
+
+	u32 lcpll_freq;
+};
+
 struct i915_suspend_saved_registers {
 	u8 saveLBB;
 	u32 saveDSPACNTR;

@@ -1127,6 +1150,11 @@ typedef struct drm_i915_private {
 
 	struct i915_suspend_saved_registers regfile;
 
+	struct i915_c8_saved_registers c8_regfile;
+	bool allowing_package_c8;
+	int c8_forbid_refcnt;
+	struct mutex c8_lock;
+
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;

index 6a977ce..ba73e00 100644
@@ -2194,6 +2194,8 @@
 #define BLC_PWM_CPU_CTL2	0x48250
 #define BLC_PWM_CPU_CTL		0x48254
 
+#define HSW_BLC_PWM2_CTL	0x48350
+
 /* PCH CTL1 is totally different, all but the below bits are reserved. CTL2 is
  * like the normal CTL from gen4 and earlier. Hooray for confusing naming. */
 #define BLC_PWM_PCH_CTL1	0xc8250

@@ -2202,6 +2204,12 @@
 #define   BLM_PCH_POLARITY			(1 << 29)
 #define BLC_PWM_PCH_CTL2	0xc8254
 
+#define UTIL_PIN_CTL		0x48400
+#define   UTIL_PIN_ENABLE	(1 << 31)
+
+#define PCH_GTC_CTL		0xe7000
+#define   PCH_GTC_ENABLE	(1 << 31)
+
 /* TV port control */
 #define TV_CTL			0x68000
 /** Enables the TV encoder */

@@ -4877,6 +4885,8 @@
 #define   SBI_SSCAUXDIV_FINALDIV2SEL(x)		((x)<<4)
 #define  SBI_DBUFF0				0x2a00
 #define   SBI_DBUFF0_ENABLE			(1<<0)
+#define  SBI_GEN0				0x1f00
+#define   SBI_GEN0_ENABLE			(1<<0)
 
 /* LPT PIXCLK_GATE */
 #define PIXCLK_GATE			0xC6020

@@ -4942,7 +4952,14 @@
 #define  LCPLL_CLK_FREQ_450		(0<<26)
 #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
 #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
+#define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
+#define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)
+
+#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
+#define  D_COMP_COMP_FORCE		(1<<8)
+#define  D_COMP_COMP_DISABLE		(1<<0)
 
 /* Pipe WM_LINETIME - watermark line time */
 #define PIPE_WM_LINETIME_A		0x45270

index 3acec8c..6808a31 100644
@@ -609,10 +609,13 @@ static enum drm_connector_status
 intel_crt_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crt *crt = intel_attached_crt(connector);
 	enum drm_connector_status status;
 	struct intel_load_detect_pipe tmp;
 
+	intel_aux_display_shutdown_forbid(dev_priv);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		/* We can not rely on the HPD pin always being correctly wired
 		 * up, for example many KVM do not pass it through, and so

@@ -620,23 +623,30 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		 */
 		if (intel_crt_detect_hotplug(connector)) {
 			DRM_DEBUG_KMS("CRT detected via hotplug\n");
-			return connector_status_connected;
+			status = connector_status_connected;
+			goto out;
 		} else
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
 	}
 
-	if (intel_crt_detect_ddc(connector))
-		return connector_status_connected;
+	if (intel_crt_detect_ddc(connector)) {
+		status = connector_status_connected;
+		goto out;
+	}
 
 	/* Load detection is broken on HPD capable machines. Whoever wants a
 	 * broken monitor (without edid) to work behind a broken kvm (that fails
 	 * to have the right resistors for HP detection) needs to fix this up.
 	 * For now just bail out. */
-	if (I915_HAS_HOTPLUG(dev))
-		return connector_status_disconnected;
+	if (I915_HAS_HOTPLUG(dev)) {
+		status = connector_status_disconnected;
+		goto out;
+	}
 
-	if (!force)
-		return connector->status;
+	if (!force) {
+		status = connector->status;
+		goto out;
+	}
 
 	/* for pre-945g platforms use load detect */
 	if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {

@@ -648,6 +658,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	} else
 		status = connector_status_unknown;
 
+out:
+	intel_aux_display_shutdown_allow(dev_priv);
 	return status;
 }
 

index 759a1c5..0de82bb 100644
@@ -2519,9 +2519,12 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
 	struct edid *edid = NULL;
 
+	intel_aux_display_shutdown_forbid(dev_priv);
+
 	intel_dp->has_audio = false;
 
 	if (HAS_PCH_SPLIT(dev))

@@ -2530,7 +2533,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		status = g4x_dp_detect(intel_dp);
 
 	if (status != connector_status_connected)
-		return status;
+		goto out;
 
 	intel_dp_probe_oui(intel_dp);
 

@@ -2546,7 +2549,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP)
 		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-	return connector_status_connected;
+	status = connector_status_connected;
+
+out:
+	intel_aux_display_shutdown_allow(dev_priv);
+	return status;
 }
 
 static int intel_dp_get_modes(struct drm_connector *connector)

index eae3dbc..50ca1bf 100644
@@ -767,6 +767,9 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
+extern void
+intel_aux_display_shutdown_forbid(struct drm_i915_private *dev_priv);
+extern void intel_aux_display_shutdown_allow(struct drm_i915_private *dev_priv);
 extern bool intel_display_power_enabled(struct drm_device *dev,
 					enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);

index bc12518..7fd186d 100644
@@ -866,6 +866,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	struct edid *edid;
 	enum drm_connector_status status = connector_status_disconnected;
 
+	intel_aux_display_shutdown_forbid(dev_priv);
+
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;

@@ -893,6 +895,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 		intel_encoder->type = INTEL_OUTPUT_HDMI;
 	}
 
+	intel_aux_display_shutdown_allow(dev_priv);
 	return status;
 }
 

1.8.1.2

Really don't like the colour you've chosen in places, and there seem to be a lot of placeholder code.

Highlights:
- intel_aux_display_shutdown_forbid / allow ->
intel_aux_display_get / put

Not convinced by "aux" there either. Perhaps, intel_display_wakelock_get() instead. Or we could go with intel_display_forcewake_get(). Hmm, forcewake is better as we already have that concept in our code (and wakelock may end up being confused with the system-wide wakelocks.)

- move your c8 state into a struct:

   struct i915_package_c8 {
           struct mutex mutex;
	   union {
		   struct hsw_package_c8_registers hsw_regfile;
	   };
	   bool enabled; /* was allowing_package_c8 */
	   int forcewake; /* was c8_forbid_refcnt */

};

- The interrupt register handling is messy. We already track the values of the ones that we change, the others should be static. Looks mostly correct (barring a race with an interrupt!).

- Hooking into modeset_global_resources seems strange and never explained.
-Chris

Chris Wilson, Intel Open Source Technology Centre

aux_display is my bikeshed ;-) The idea was that we have a russian doll stack of display power knobs:
- the power well to shut of everything but edp on pipe A
- the power well for all display pipes (not yet on haswell)
- power control for aux stuff like gmbus/dp aux
- the real pci device

In reality it's probably more a tree since rc6 forcewake also hangs in there somewhere. Maybe we could also track a few more things explicitly, e.g. the refclock plls could probably be refcounted by their users (e.g. ddi plls).

In the end I guess most of these need to be refcounted, and child power domains need to also grab a reference on their parents on the 0->1 transition. One idea I'm pondering is whether we should make all those power domains explicit by mapping them to platform devices we create. Upsides:
- Someone clearly thought about funy races and locking issues when writing the pm runtime stuff. Using it would avoid us reinventing that wheel.
- struct device is the main object for pm in linux, so if we can make our aux device (like i2c) childs of the correct power domain (what I've called "aux display") instead of the pci device we might gain a bit in integration.
- Full runtime PM at the pci level seems to be on the table anyway.

I've inked in today for starting my irq review (thanks to vecs), I think we should do that first and then look again at the irq prep patches. But I agree that in theory we should only need to clear out the unused interrupts once in preinstall. For paranoia we could add a few WARNs in the uninstall hooks.

Modeset is a bit ugly for power domain refcounting (it's the same mess for the power well as for pc8+ here). The issue I think is that we don't have a resource acquire/release stage where platform code could hook in. crtc_disable would be at the right spot, but is also called with the dpms off stuff (so not suitable for everything). Off isn't called unconditionally when shutting down a pipe for real (if we reuse the pipe the cleanup is done in ->mode_set). And crtc_enable is too late for some cases since global_modeset_resources and crtc mode_set already need to touch the hw.

Maybe we could use the for_each_crtc loop we already have and grab a refcount for each crtc. If we switch the power well code to refcounting that would nicely unify things, too.

Another issue I'm not yet clear on is dpms handling. Atm we ignore all that stuff for it (since neither modeset_global_resources nor mode_set hooks are called, and the hw seems to completely forget register contents, so this must be done). Probably the simplest solution is to call these hooks in the dpms on path, too. But atm that plan runs afoul of the resource accounting we still have in there (pll sharing and refcounting mostly).

Tbh I don't have a clear overall idea for this yet. -Daniel

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

Ok, so real review now, not just function name bikeshedding ;-)

* I'd really like to see a testcase, but not per se to exercise pc8+ but to check that our prepare/exit code and especially the interrupt disabling/enabling doesn't break anything. So the overall structure of the test should probably be something like

1. Put the console into raw mode (so that fbcon doesn't interfere), Imre's recent infrastructure patches should make this really simple.

2. Disable all outputs with setcrtc(fb=NULL).

3. Wait a bit until we're sure that our code has prepared for pc8+, i.e. all the interrupts are shut off, lcpll is disabled ...

4. Run a subtest.

5. Repeat.

For subtests we need quite a pile of them I think:
- Test getconnectors ioctl to make sure both our ->detec and ->get_modes callbacks have the proper aux_display runtime pm get/put calls. On a quick read through your patch the get/put (currently still call forbid/allow) in the ->get_modes hooks seem to be missing.
- Direct i2c access from userspace. This happens both from random tools probing the i2c bus, but also for real uses-cases, like the ddccontrol tool or e.g. the Chromebook Pixie which uses gmbus as the hw i2c engine for the touchpad.
- Submit a patch (this might just work when waking the gt automatically wakes up everything) _and_ do a blocking wait for it (this will break if interrupts are dead).

It's probably good to have some correctness checks for these, e.g. compare the edid property while outputs are still on (so pc8+ is impossible) with what happens when they're off. Execbuf could be tested with a simple fill blt. Imo it's also important that we have a wait before each new subtest, to make sure we really teste everything. E.g. a subtest only checks EDID for one output, then waits again. Otherwise it could be that the working refcounting in e.g. HDMI could shadow broken runtime pm refcounting for dp aux if it's done right afterwards.

* spin_lock_irqsave isn't that irq-save ;-) It only blocks interrupt on the local cpu. See Rusty's unreliable guide to kernel locking in Documentation/DocBook/kernel-locking. If you don't understand what spin_lock_irqsave is useful for, how our current locking works or why the one in your patch is broken I think we should discuss this afterwards. Reading my irq locking review patches and comparing it to the locking guid could also be interesting.

I haven't yet done the full review, but probably the safest route is to fully disable our interrupt handler with disable_irq while we do the pc8+ prepare sequence. On the exit sequence we probably don't need it. In any case we can reenable interrupts again with enable_irq. Maybe there's a possible race there (I need to review the core irq code first), but now I think having a tiny race there where we could miss hpd events is less evil than adding complex locking just for pc8+ entry. Even more so with the next point, which aims to unify pc8+ entry/exit with our existing suspend/resume code. And s/r runs in a non-reentrant enviroment, so if we can keep those basic assumptions that's imo worthwile.

* Unifying irq setup/teardown code. I'm not really happy with the setup/teardown code, and I think we should take a good look at trying to unify it with the existing driver load/unload/suspend/resume code. Massive register save/restore code tends to get out of sync with other code changes way too easily.

Currently our irq enable sequence is:
1. call ->irq_preinstall hook
2. enable irq handler
3. call ->irq_postinstall hook
and somewhen later
4. enable hotplug interrupt handling with our own ->hpd_irq_setup hook.

The disabling sequence is:
1. disable irq handler
2. call ->irq_uninstall hook

Now for pc8+ we want to disable/enable almost everything _but_ the hpd interrupts. I think with some code wrestling we should be able to do that with the existing functions:
- Split the irq_unistall into two parts, irq_unistall which disables all interrupt sources _but_ hotplug, and a hpd_irq_teardown which disables hotplug, too. For that we'd need an i915_irq_unistall functions which first calls drm_irq_uninstall and the new ->hpd_irq_teardown hook. That would also allow us to unify the hotplug reenable timer teardown a bit.
- Rework ->irq_postinstall and ->irq_uninstall to not frob hpd settings. Since we control hpd interrupts all through SDEIMR it should be fairly simple to disable everything but hotplug interrupts, even more so with the new helpers I've added in my irq locking review series.
- Important is that the new ->irq_uninstall hook won't disable the master interrupt sources, that'd would all be moved into the ->hpd_irq_teardown code.

With the pc8+ prepare sequence would look like
1. disable irq handler
2. call ->irq_unistall hook directly
-> hpd interrupts are still working
3. re-enable irq handler

And the exit sequence would be
1. disable irq handler
2. call ->irq_postinstall
3. re-enable irq handler

* As you might have guessed I see the biggest risk in us screwing up the interrupt handling and especially someone sneaking in while we're in pc8+ mode and changing the irq registers. Safe for the hotplug bits which might need masking due to a hotplug storm I expect all such changes to be bugs.

The upshot of the above proposal now is that we can make sure that for all the pc8+ relevant interrupt bits calling ->irq_uninstall leaves behind the same state as ->irq_preinstall. So we could sprinkle massive amounts of WARNs into ->irq_postinstall to check that all the interupt registers are still in the correct state and no one changed them.

* Finally (and this part is really just an idea I'll throw out here) our driver setup/teardown sequences are getting pretty complicated, and different parts (load, s/r, pc8+, ...) will use them ever so slightly. I think adding an

atomic_t dev_priv->driver_stage_ready

bit mask would be useful. Every time a driver stage is loaded/setup/resumed it sets the bit, if we unload/suspend/teardown it gets cleared. And every driver setup stage can check at the beginning whether all the required pieces are present. This should help us in documenting the sometimes tricky ordering constraints in general.

For this case specifically I'm thinking of 2 driver stages:
- general_irq_ready
- hpd_irq_ready
Since atomic_read is as cheap as any unordered load we could sprinkle a few of these checks over the code, e.g. in the gmbus or dp aux irq-based wait code. Similar e.g. for gem in the __wait_seqno function. This should help a lot in debugging bugs due to missing refcounting.

For priorities I think the best approach is to start with the testcase. I think currently you're code is missing quite a few runtime pm get/put calls, so I'm pretty sure it'll blow up. And if it doesn't my understanding of how pc8+ works is seriously wrong.

And once we have a solid testcase it should be much easier to move the code piece around a bit so that we have a sane pc8+ enter/exit sequence.

Cheers, Daniel

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