* [PATCH 6/6] drm/i915: implement HSW display sequences for package C8+
2013-06-05 17:21 [PATCH 0/6] Enable PC8+ states Paulo Zanoni
` (4 preceding siblings ...)
2013-06-05 17:21 ` [PATCH 5/6] drm/i915: initialize the PCH GTC interrupts Paulo Zanoni
@ 2013-06-05 17:21 ` Paulo Zanoni
2013-06-06 6:49 ` Chris Wilson
2013-06-12 16:27 ` [PATCH 0/6] Enable PC8+ states Daniel Vetter
6 siblings, 1 reply; 11+ messages in thread
From: Paulo Zanoni @ 2013-06-05 17:21 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
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(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 22534c3..fe2bd7f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -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;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 215aa63..46b1f70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6a977ce..ba73e00 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 3acec8c..6808a31 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -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;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 827d7ca..4c8fcec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5284,6 +5284,66 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
mutex_unlock(&dev_priv->dpio_lock);
}
+/* Sequence to enable CLKOUT_DP */
+static void lpt_enable_clkout_dp(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ mutex_lock(&dev_priv->dpio_lock);
+
+ val = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+ val &= ~SBI_SSCCTL_DISABLE;
+ val |= SBI_SSCCTL_PATHALT;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, val, SBI_ICLK);
+
+ udelay(24);
+
+ val = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+ val &= ~SBI_SSCCTL_PATHALT;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, val, SBI_ICLK);
+
+ if (IS_ULT(dev_priv->dev)) {
+ val = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+ val |= SBI_GEN0_ENABLE;
+ intel_sbi_write(dev_priv, SBI_GEN0, val, SBI_ICLK);
+ } else {
+ val = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+ val |= SBI_DBUFF0_ENABLE;
+ intel_sbi_write(dev_priv, SBI_DBUFF0, val, SBI_ICLK);
+ }
+
+ mutex_unlock(&dev_priv->dpio_lock);
+}
+
+/* Sequence to disable CLKOUT_DP */
+static void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ mutex_lock(&dev_priv->dpio_lock);
+
+ if (IS_ULT(dev_priv->dev)) {
+ val = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+ val &= ~SBI_GEN0_ENABLE;
+ intel_sbi_write(dev_priv, SBI_GEN0, val, SBI_ICLK);
+ } else {
+ val = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+ val &= ~SBI_DBUFF0_ENABLE;
+ intel_sbi_write(dev_priv, SBI_DBUFF0, val, SBI_ICLK);
+ }
+
+ val = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+ if (!(val & SBI_SSCCTL_PATHALT)) {
+ val |= SBI_SSCCTL_PATHALT;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, val, SBI_ICLK);
+ udelay(32);
+ }
+ val |= SBI_SSCCTL_DISABLE;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, val, SBI_ICLK);
+
+ mutex_unlock(&dev_priv->dpio_lock);
+}
+
/*
* Initialize reference clocks when the driver loads
*/
@@ -5844,9 +5904,357 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
return true;
}
+static void hsw_disable_lcpll(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ val = I915_READ(LCPLL_CTL);
+
+ dev_priv->c8_regfile.lcpll_freq = val & LCPLL_CLK_FREQ_MASK;
+
+ val |= LCPLL_CD_SOURCE_FCLK;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+ DRM_ERROR("Switching to FCLK failed\n");
+
+ val |= LCPLL_PLL_DISABLE;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
+ DRM_ERROR("LCPLL still locked\n");
+
+ val = I915_READ(D_COMP);
+ val |= D_COMP_COMP_DISABLE;
+ I915_WRITE(D_COMP, val);
+ POSTING_READ(D_COMP);
+
+ udelay(2);
+
+ val = I915_READ(D_COMP);
+ if (val & D_COMP_RCOMP_IN_PROGRESS)
+ DRM_ERROR("D_COMP RCOMP still in progress\n");
+
+ val = I915_READ(LCPLL_CTL);
+ val |= LCPLL_POWER_DOWN_ALLOW;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+}
+
+static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ val = I915_READ(LCPLL_CTL);
+
+ if ((val & LCPLL_CLK_FREQ_MASK) != dev_priv->c8_regfile.lcpll_freq)
+ DRM_ERROR("LCPLL frequency changed\n");
+
+ if (val & LCPLL_POWER_DOWN_ALLOW) {
+ val &= ~LCPLL_POWER_DOWN_ALLOW;
+ I915_WRITE(LCPLL_CTL, val);
+ }
+
+ if (val & LCPLL_CD_SOURCE_FCLK) {
+ val = I915_READ(D_COMP);
+ val |= D_COMP_COMP_FORCE;
+ val &= ~D_COMP_COMP_DISABLE;
+ I915_WRITE(D_COMP, val);
+
+ val = I915_READ(LCPLL_CTL);
+ val &= ~LCPLL_PLL_DISABLE;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+ DRM_ERROR("LCPLL not locked yet\n");
+
+ val = I915_READ(LCPLL_CTL);
+ val &= ~LCPLL_CD_SOURCE_FCLK;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+ DRM_ERROR("Switching back to LCPLL failed\n");
+ }
+}
+
+static void hsw_disable_interrupts(struct drm_i915_private *dev_priv)
+{
+ struct i915_c8_saved_registers *c8_regfile = &dev_priv->c8_regfile;
+ unsigned long irqflags;
+ uint32_t val, deier, sdeier, hotplug;
+
+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+ /* TODO: The spec says we need to clear all pending graphics interrupts,
+ * but what's the best way to guarantee this? For now let's just print
+ * some error messages and assume everything will work. We may have to
+ * manually zero all the IIR registers if we discover they're a real
+ * requirement to reach PC8+. */
+ val = I915_READ(DEIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: DEIIR 0x%08x\n", val);
+
+ val = I915_READ(AUDIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: AUDIIR 0x%08x\n", val);
+
+ val = I915_READ(GTIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: GTIIR 0x%08x\n", val);
+
+ val = I915_READ(GEN6_PMIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: PMIIR 0x%08x\n", val);
+
+ val = I915_READ(SRDIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: SRDIIR 0x%08x\n", val);
+
+ val = I915_READ(SDEIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: SDEIIR 0x%08x\n", val);
+
+ val = I915_READ(_FDI_RXA_IIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: FDIRXAIIR 0x%08x\n", val);
+
+ val = I915_READ(PCH_GTCIIR);
+ if (val)
+ DRM_ERROR("Pending interrupt: GTCPCHIIR 0x%08x\n", val);
+
+ c8_regfile->de_imr = I915_READ(DEIMR);
+ c8_regfile->de_ier = I915_READ(DEIER);
+ c8_regfile->aud_imr = I915_READ(AUDIMR);
+ c8_regfile->aud_ier = I915_READ(AUDIER);
+ c8_regfile->gt_imr = I915_READ(GTIMR);
+ c8_regfile->gt_ier = I915_READ(GTIER);
+ c8_regfile->pm_imr = I915_READ(GEN6_PMIMR);
+ c8_regfile->pm_ier = I915_READ(GEN6_PMIER);
+ c8_regfile->srd_imr = I915_READ(SRDIMR);
+ c8_regfile->hotplug_ctl = I915_READ(DIGITAL_PORT_HOTPLUG_CNTRL);
+ c8_regfile->err_int = I915_READ(GEN7_ERR_INT);
+
+ c8_regfile->sde_imr = I915_READ(SDEIMR);
+ c8_regfile->sde_ier = I915_READ(SDEIER);
+ c8_regfile->fdirx_imr = I915_READ(_FDI_RXA_IMR);
+ c8_regfile->gtcpch_imr = I915_READ(PCH_GTCIMR);
+ c8_regfile->shotplug_ctl = I915_READ(PCH_PORT_HOTPLUG);
+ c8_regfile->serr_int = I915_READ(SERR_INT);
+
+ deier = DE_MASTER_IRQ_CONTROL | DE_PCH_EVENT_IVB;
+ I915_WRITE(DEIMR, ~deier);
+ I915_WRITE(DEIER, deier);
+
+ I915_WRITE(AUDIMR, 0xFFFFFFFF);
+ I915_WRITE(AUDIER, 0);
+
+ I915_WRITE(GTIMR, 0xFFFFFFFF);
+ I915_WRITE(GTIER, 0);
+
+ I915_WRITE(GEN6_PMIMR, 0xFFFFFFFF);
+ I915_WRITE(GEN6_PMIER, 0);
+
+ I915_WRITE(SRDIMR, 0xFFFFFFFF);
+
+ I915_WRITE(DIGITAL_PORT_HOTPLUG_CNTRL, 0);
+
+ I915_WRITE(GEN7_ERR_INT, 0xFFFFFFFF);
+
+ sdeier = (SDE_PORTD_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT |
+ SDE_PORTB_HOTPLUG_CPT | SDE_CRT_HOTPLUG_CPT) &
+ c8_regfile->sde_ier;
+ I915_WRITE(SDEIMR, ~sdeier);
+ I915_WRITE(SDEIER, sdeier);
+
+ I915_WRITE(_FDI_RXA_IMR, 0xFFFFFFFF);
+ I915_WRITE(PCH_GTCIMR, 0xFFFFFFFF);
+
+ hotplug = (PORTD_HOTPLUG_ENABLE | PORTC_HOTPLUG_ENABLE |
+ PORTB_HOTPLUG_ENABLE) & c8_regfile->hotplug_ctl;
+ I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
+ I915_WRITE(SERR_INT, 0xFFFFFFFF);
+
+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+static void hsw_restore_interrupts(struct drm_i915_private *dev_priv)
+{
+ struct i915_c8_saved_registers *c8_regfile = &dev_priv->c8_regfile;
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+ I915_WRITE(DEIMR, c8_regfile->de_imr);
+ I915_WRITE(DEIER, c8_regfile->de_ier);
+ I915_WRITE(AUDIMR, c8_regfile->aud_imr);
+ I915_WRITE(AUDIER, c8_regfile->aud_ier);
+ I915_WRITE(GTIMR, c8_regfile->gt_imr);
+ I915_WRITE(GTIER, c8_regfile->gt_ier);
+ I915_WRITE(GEN6_PMIMR, c8_regfile->pm_imr);
+ I915_WRITE(GEN6_PMIER, c8_regfile->pm_ier);
+ I915_WRITE(SRDIMR, c8_regfile->srd_imr);
+ I915_WRITE(DIGITAL_PORT_HOTPLUG_CNTRL, c8_regfile->hotplug_ctl);
+ I915_WRITE(GEN7_ERR_INT, c8_regfile->err_int);
+
+ I915_WRITE(SDEIMR, c8_regfile->sde_imr);
+ I915_WRITE(SDEIER, c8_regfile->sde_ier);
+ I915_WRITE(_FDI_RXA_IMR, c8_regfile->fdirx_imr);
+ I915_WRITE(PCH_GTCIMR, c8_regfile->gtcpch_imr);
+ I915_WRITE(PCH_PORT_HOTPLUG, c8_regfile->shotplug_ctl);
+ I915_WRITE(SERR_INT, c8_regfile->serr_int);
+
+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+static void hsw_allow_package_c8(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ WARN_ON(!mutex_is_locked(&dev_priv->c8_lock));
+ WARN(dev_priv->c8_forbid_refcnt < 1,
+ "c8_forbid_refcnt: %d\n", dev_priv->c8_forbid_refcnt);
+
+ dev_priv->c8_forbid_refcnt--;
+ if (dev_priv->c8_forbid_refcnt != 0)
+ return;
+
+ DRM_DEBUG_KMS("Allowing package C8+\n");
+
+ if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+ val = I915_READ(SOUTH_DSPCLK_GATE_D);
+ val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
+ I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+ }
+
+ lpt_disable_clkout_dp(dev_priv);
+ hsw_disable_interrupts(dev_priv);
+ hsw_disable_lcpll(dev_priv);
+}
+
+static void hsw_disallow_package_c8(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ WARN_ON(!mutex_is_locked(&dev_priv->c8_lock));
+ WARN(dev_priv->c8_forbid_refcnt < 0,
+ "c8_forbid_refcnt: %d\n", dev_priv->c8_forbid_refcnt);
+
+ dev_priv->c8_forbid_refcnt++;
+ if (dev_priv->c8_forbid_refcnt != 1)
+ return;
+
+ DRM_DEBUG_KMS("Disallowing package C8+\n");
+
+ hsw_restore_lcpll(dev_priv);
+ hsw_restore_interrupts(dev_priv);
+ lpt_enable_clkout_dp(dev_priv);
+
+ if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+ val = I915_READ(SOUTH_DSPCLK_GATE_D);
+ val |= PCH_LP_PARTITION_LEVEL_DISABLE;
+ I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+ }
+
+ intel_prepare_ddi(dev_priv->dev);
+ i915_gem_init_swizzling(dev_priv->dev);
+}
+
+static bool hsw_can_allow_package_c8(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv->dev;
+ struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
+ struct intel_crtc *crtc;
+ uint32_t val;
+
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+ if (crtc->base.enabled)
+ return false;
+
+ /* This case is still possible since we have the i915.disable_power_well
+ * parameter and also the KVMr or something else might be requesting the
+ * power well. */
+ val = I915_READ(HSW_PWR_WELL_DRIVER);
+ if (val != 0) {
+ DRM_DEBUG_KMS("Not allowing PC8: power well on\n");
+ return false;
+ }
+
+ if (WARN(plls->spll_refcount, "Not allowing PC8: SPLL enabled\n"))
+ return false;
+
+ if (WARN(plls->wrpll1_refcount, "Not allowing PC8: WRPLL1 enabled\n"))
+ return false;
+
+ if (WARN(plls->wrpll2_refcount, "Not allowing PC8: WRPLL2 enabled\n"))
+ return false;
+
+ val = I915_READ(PCH_PP_STATUS);
+ if (WARN(val & PP_ON, "Not allowing PC8: panel power on\n"))
+ return false;
+
+ val = I915_READ(BLC_PWM_CPU_CTL2);
+ if (WARN(val & BLM_PWM_ENABLE, "Not allowing PC8: CPU PWM1 enabled\n"))
+ return false;
+
+ val = I915_READ(HSW_BLC_PWM2_CTL);
+ if (WARN(val & BLM_PWM_ENABLE, "Not allowing PC8: CPU PWM2 enabled\n"))
+ return false;
+
+ val = I915_READ(BLC_PWM_PCH_CTL1);
+ if (WARN(val & BLM_PCH_PWM_ENABLE,
+ "Not allowing PC8: PCH PWM1 enabled\n"))
+ return false;
+
+ val = I915_READ(UTIL_PIN_CTL);
+ if (WARN(val & UTIL_PIN_ENABLE,
+ "Not allowing PC8: utility pin enabled\n"))
+ return false;
+
+ val = I915_READ(PCH_GTC_CTL);
+ if (WARN(val & PCH_GTC_ENABLE, "Not allowing PC8: PCH GTC enabled\n"))
+ return false;
+
+ return true;
+}
+
+/* Since we're called from modeset_global_resources there's no way to
+ * symmetrically increase and decrease the refcnt, so we use
+ * dev_priv->allowing_package_c8 to track whether we already have the refcnt or
+ * not. */
+static void hsw_set_package_c8(struct drm_i915_private *dev_priv)
+{
+ bool allow = hsw_can_allow_package_c8(dev_priv);
+
+ mutex_lock(&dev_priv->c8_lock);
+
+ if (allow == dev_priv->allowing_package_c8)
+ goto done;
+
+ dev_priv->allowing_package_c8 = allow;
+
+ if (allow)
+ hsw_allow_package_c8(dev_priv);
+ else
+ hsw_disallow_package_c8(dev_priv);
+
+done:
+ mutex_unlock(&dev_priv->c8_lock);
+}
+
+
static void haswell_modeset_global_resources(struct drm_device *dev)
{
- bool enable = false;
+ bool enable_power_well = false;
struct intel_crtc *crtc;
list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
@@ -5855,10 +6263,26 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.size ||
crtc->config.cpu_transcoder != TRANSCODER_EDP)
- enable = true;
+ enable_power_well = true;
}
- intel_set_power_well(dev, enable);
+ intel_set_power_well(dev, enable_power_well);
+
+ hsw_set_package_c8(dev->dev_private);
+}
+
+void intel_aux_display_shutdown_forbid(struct drm_i915_private *dev_priv)
+{
+ mutex_lock(&dev_priv->c8_lock);
+ hsw_disallow_package_c8(dev_priv);
+ mutex_unlock(&dev_priv->c8_lock);
+}
+
+void intel_aux_display_shutdown_allow(struct drm_i915_private *dev_priv)
+{
+ mutex_lock(&dev_priv->c8_lock);
+ hsw_allow_package_c8(dev_priv);
+ mutex_unlock(&dev_priv->c8_lock);
}
static int haswell_crtc_mode_set(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 759a1c5..0de82bb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eae3dbc..50ca1bf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index bc12518..7fd186d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 0/6] Enable PC8+ states
2013-06-05 17:21 [PATCH 0/6] Enable PC8+ states Paulo Zanoni
` (5 preceding siblings ...)
2013-06-05 17:21 ` [PATCH 6/6] drm/i915: implement HSW display sequences for package C8+ Paulo Zanoni
@ 2013-06-12 16:27 ` Daniel Vetter
6 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2013-06-12 16:27 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Wed, Jun 05, 2013 at 02:21:50PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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.
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
^ permalink raw reply [flat|nested] 11+ messages in thread