[PATCH 0/7] drm/i915: Pipe A quirk rework

[PATCH 0/7] drm/i915: Pipe A quirk rework

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This series eliminates the problematic load detect abuse for the
pipe A quirk. My main motivations were to isolate these quirks
more from atomic to avoid regressions, and to save a bit of extra
power. I believe I cooked this up a few years ago but never posted
it. In the meantine we had accumulated some more regressions in
the existing code so I tossed in some fixes up front.

Note that I'm entirely removing the few remaining pipe A quirks
for non-830 platforms as they predate KMS and the hardware
really shouldn't need them.

Entire series available here:
git://github.com/vsyrjala/linux.git alm_pipe_quirk_rework_4

Ville Syrjälä (7):
  drm/i915: Fix deadlock witha the pipe A quirk during resume
  drm/i915: Plumb the correct acquire ctx into
    intel_crtc_disable_noatomic()
  drm/i915: Use a loop for the "three times for luck" DPLL procedure
  drm/i915: Add i830 "pipes power well"
  drm/i915: Drop pipe A quirk for Toshiba Protege R205-S209
  drm/i915: Drop pipe A quirk for Thinkapd T60
  drm/i915: Remove pipe A quirk remnants

 drivers/gpu/drm/i915/i915_drv.h         |   2 -
 drivers/gpu/drm/i915/intel_display.c    | 209 +++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h        |   2 +
 drivers/gpu/drm/i915/intel_overlay.c    |   1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c |  64 ++++++++++
 5 files changed, 177 insertions(+), 101 deletions(-)

-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pass down the correct acquire context to the pipe A quirk load detect
hack during display resume. Avoids deadlocking the entire thing.

Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed41b59ee8e3..2f76a63efe8c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
 static void skylake_pfit_enable(struct intel_crtc *crtc);
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
 static void ironlake_pfit_enable(struct intel_crtc *crtc);
-static void intel_modeset_setup_hw_state(struct drm_device *dev);
+static void intel_modeset_setup_hw_state(struct drm_device *dev,
+					 struct drm_modeset_acquire_ctx *ctx);
 static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
 
 struct intel_limit {
@@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int i, ret;
 
-	intel_modeset_setup_hw_state(dev);
+	intel_modeset_setup_hw_state(dev, ctx);
 	i915_redisable_vga(to_i915(dev));
 
 	if (!state)
@@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
 	intel_setup_outputs(dev_priv);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev);
+	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
@@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
 	return 0;
 }
 
-static void intel_enable_pipe_a(struct drm_device *dev)
+static void intel_enable_pipe_a(struct drm_device *dev,
+				struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *crt = NULL;
 	struct intel_load_detect_pipe load_detect_temp;
-	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
 	int ret;
 
 	/* We can't just switch on the pipe A, we need to set things up with a
@@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
 		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
 }
 
-static void intel_sanitize_crtc(struct intel_crtc *crtc)
+static void intel_sanitize_crtc(struct intel_crtc *crtc,
+				struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
 		 * the required bits on. */
-		intel_enable_pipe_a(dev);
+		intel_enable_pipe_a(dev, ctx);
 	}
 
 	/* Adjust the state of the output pipe according to whether we
@@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
  * and sanitizes it to the current state
  */
 static void
-intel_modeset_setup_hw_state(struct drm_device *dev)
+intel_modeset_setup_hw_state(struct drm_device *dev,
+			     struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum pipe pipe;
@@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	for_each_pipe(dev_priv, pipe) {
 		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 
-		intel_sanitize_crtc(crtc);
+		intel_sanitize_crtc(crtc, ctx);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");
 	}
-- 
2.10.2

[PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If intel_crtc_disable_noatomic() were to ever get called during resume
e'd end up deadlocking since resume has its own acqcuire_ctx but
intel_crtc_disable_noatomic() still tries to use the
mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.

Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f76a63efe8c..4e3c64ed4131 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
 		intel_update_watermarks(intel_crtc);
 }
 
-static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
+static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
+					struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		return;
 	}
 
-	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
+	state->acquire_ctx = ctx;
 
 	/* Everything's already locked, -EDEADLK can't happen. */
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		plane = crtc->plane;
 		crtc->base.primary->state->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 		crtc->plane = plane;
 	}
 
@@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	if (crtc->active && !intel_crtc_has_encoders(crtc))
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
 		/*
-- 
2.10.2

[PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If intel_crtc_disable_noatomic() were to ever get called during resume
e'd end up deadlocking since resume has its own acqcuire_ctx but
intel_crtc_disable_noatomic() still tries to use the
mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.

Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f76a63efe8c..4e3c64ed4131 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
 		intel_update_watermarks(intel_crtc);
 }
 
-static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
+static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
+					struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		return;
 	}
 
-	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
+	state->acquire_ctx = ctx;
 
 	/* Everything's already locked, -EDEADLK can't happen. */
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		plane = crtc->plane;
 		crtc->base.primary->state->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 		crtc->plane = plane;
 	}
 
@@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	if (crtc->active && !intel_crtc_has_encoders(crtc))
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
 		/*
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915: Use a loop for the "three times for luck" DPLL procedure

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The magic "enable the  DPLL three times" sequence feels like it
deserves a loop.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e3c64ed4131..2b112229b5b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1550,6 +1550,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	i915_reg_t reg = DPLL(crtc->pipe);
 	u32 dpll = crtc->config->dpll_hw_state.dpll;
+	int i;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 
@@ -1596,15 +1597,11 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
 	}
 
 	/* We do this three times for luck */
-	I915_WRITE(reg, dpll);
-	POSTING_READ(reg);
-	udelay(150); /* wait for warmup */
-	I915_WRITE(reg, dpll);
-	POSTING_READ(reg);
-	udelay(150); /* wait for warmup */
-	I915_WRITE(reg, dpll);
-	POSTING_READ(reg);
-	udelay(150); /* wait for warmup */
+	for (i = 0; i < 3; i++) {
+		I915_WRITE(reg, dpll);
+		POSTING_READ(reg);
+		udelay(150); /* wait for warmup */
+	}
 }
 
 /**
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: Add i830 "pipes power well"

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

830 more or less requires both pipes and DPLLs to remain on as long
as either pipe is needed. However, when neither pipe is actually needed,
we can save a bit of power by turning everything off. To do that we add
a new "power well" that turns both pipes and DPLLs on and off in the
right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010.

This also avoids having to abuse the load detection to force pipe A on
at init time. That was never very robust, and it only worked for one
pipe, whereas 830 really needs both pipes enabled. As a bonus the 830
pipe quirk is now a bit more isolated from the rest of the mode setting
infrastructure, which should mean that it's much less likely someone
will accidentally break it in the future. The extra cost is of course
slight code duplication, but that seems like a worthwile tradeoff here.

v2; s/BIT/BIT_ULL/

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c    | 92 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h        |  2 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 64 +++++++++++++++++++++++
 3 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2b112229b5b2..e1e5926a40a1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5834,6 +5834,10 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
 
 	if (!dev_priv->display.initial_watermarks)
 		intel_update_watermarks(intel_crtc);
+
+	/* clock the pipe down to 640x480@60 to potentially save power */
+	if (IS_I830(dev_priv))
+		i830_enable_pipe(dev_priv, pipe);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
@@ -15130,6 +15134,91 @@ int intel_modeset_init(struct drm_device *dev)
 	return 0;
 }
 
+void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	/* 640x480@60Hz, ~25175 kHz */
+	struct dpll clock = {
+		.m1 = 18,
+		.m2 = 7,
+		.p1 = 13,
+		.p2 = 4,
+		.n = 2,
+	};
+	u32 dpll, fp;
+	int i;
+
+	WARN_ON(i9xx_calc_dpll_params(48000, &clock) != 25154);
+
+	DRM_DEBUG_KMS("enabling pipe %c due to force quirk (vco=%d dot=%d)\n",
+		      pipe_name(pipe), clock.vco, clock.dot);
+
+	fp = i9xx_dpll_compute_fp(&clock);
+	dpll = (I915_READ(DPLL(pipe)) & DPLL_DVO_2X_MODE) |
+		DPLL_VGA_MODE_DIS |
+		((clock.p1 - 2) << DPLL_FPA01_P1_POST_DIV_SHIFT) |
+		PLL_P2_DIVIDE_BY_4 |
+		PLL_REF_INPUT_DREFCLK |
+		DPLL_VCO_ENABLE;
+
+	I915_WRITE(FP0(pipe), fp);
+	I915_WRITE(FP1(pipe), fp);
+
+	I915_WRITE(HTOTAL(pipe), (640 - 1) | ((800 - 1) << 16));
+	I915_WRITE(HBLANK(pipe), (640 - 1) | ((800 - 1) << 16));
+	I915_WRITE(HSYNC(pipe), (656 - 1) | ((752 - 1) << 16));
+	I915_WRITE(VTOTAL(pipe), (480 - 1) | ((525 - 1) << 16));
+	I915_WRITE(VBLANK(pipe), (480 - 1) | ((525 - 1) << 16));
+	I915_WRITE(VSYNC(pipe), (490 - 1) | ((492 - 1) << 16));
+	I915_WRITE(PIPESRC(pipe), ((640 - 1) << 16) | (480 - 1));
+
+	/*
+	 * Apparently we need to have VGA mode enabled prior to changing
+	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
+	 * dividers, even though the register value does change.
+	 */
+	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VGA_MODE_DIS);
+	I915_WRITE(DPLL(pipe), dpll);
+
+	/* Wait for the clocks to stabilize. */
+	POSTING_READ(DPLL(pipe));
+	udelay(150);
+
+	/* The pixel multiplier can only be updated once the
+	 * DPLL is enabled and the clocks are stable.
+	 *
+	 * So write it again.
+	 */
+	I915_WRITE(DPLL(pipe), dpll);
+
+	/* We do this three times for luck */
+	for (i = 0; i < 3 ; i++) {
+		I915_WRITE(DPLL(pipe), dpll);
+		POSTING_READ(DPLL(pipe));
+		udelay(150); /* wait for warmup */
+	}
+
+	I915_WRITE(PIPECONF(pipe), PIPECONF_ENABLE | PIPECONF_PROGRESSIVE);
+	POSTING_READ(PIPECONF(pipe));
+}
+
+void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	DRM_DEBUG_KMS("disabling pipe %c due to force quirk\n",
+		      pipe_name(pipe));
+
+	assert_plane_disabled(dev_priv, PLANE_A);
+	assert_plane_disabled(dev_priv, PLANE_B);
+
+	I915_WRITE(PIPECONF(pipe), 0);
+	POSTING_READ(PIPECONF(pipe));
+
+	if (wait_for(pipe_dsl_stopped(dev_priv, pipe), 100))
+		DRM_ERROR("pipe %c off wait timed out\n", pipe_name(pipe));
+
+	I915_WRITE(DPLL(pipe), DPLL_VGA_MODE_DIS);
+	POSTING_READ(DPLL(pipe));
+}
+
 static void intel_enable_pipe_a(struct drm_device *dev,
 				struct drm_modeset_acquire_ctx *ctx)
 {
@@ -15259,7 +15348,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		crtc->plane = plane;
 	}
 
-	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
+	if (!IS_I830(dev_priv) &&
+	    dev_priv->quirks & QUIRK_PIPEA_FORCE &&
 	    crtc->pipe == PIPE_A && !crtc->active) {
 		/* BIOS forgot to enable pipe A, this mostly happens after
 		 * resume. Force-enable the pipe to fix this, the update_dpms
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 83dd40905821..dd196b645d69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1321,6 +1321,8 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
 		     const struct intel_cdclk_state *cdclk_state);
 
 /* intel_display.c */
+void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
+void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
 enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
 void intel_update_rawclk(struct drm_i915_private *dev_priv);
 int vlv_get_hpll_vco(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f8a375f8dde6..41fd576391e7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -992,6 +992,38 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
 	return true;
 }
 
+static void i830_pipes_power_well_enable(struct drm_i915_private *dev_priv,
+					 struct i915_power_well *power_well)
+{
+	if ((I915_READ(PIPECONF(PIPE_A)) & PIPECONF_ENABLE) == 0)
+		i830_enable_pipe(dev_priv, PIPE_A);
+	if ((I915_READ(PIPECONF(PIPE_B)) & PIPECONF_ENABLE) == 0)
+		i830_enable_pipe(dev_priv, PIPE_B);
+}
+
+static void i830_pipes_power_well_disable(struct drm_i915_private *dev_priv,
+					  struct i915_power_well *power_well)
+{
+	i830_disable_pipe(dev_priv, PIPE_B);
+	i830_disable_pipe(dev_priv, PIPE_A);
+}
+
+static bool i830_pipes_power_well_enabled(struct drm_i915_private *dev_priv,
+					  struct i915_power_well *power_well)
+{
+	return I915_READ(PIPECONF(PIPE_A)) & PIPECONF_ENABLE &&
+		I915_READ(PIPECONF(PIPE_B)) & PIPECONF_ENABLE;
+}
+
+static void i830_pipes_power_well_sync_hw(struct drm_i915_private *dev_priv,
+					  struct i915_power_well *power_well)
+{
+	if (power_well->count > 0)
+		i830_pipes_power_well_enable(dev_priv, power_well);
+	else
+		i830_pipes_power_well_disable(dev_priv, power_well);
+}
+
 static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 			       struct i915_power_well *power_well, bool enable)
 {
@@ -1880,6 +1912,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_AUX_D) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 
+#define I830_PIPES_POWER_DOMAINS (		\
+	BIT_ULL(POWER_DOMAIN_PIPE_A) |		\
+	BIT_ULL(POWER_DOMAIN_PIPE_B) |		\
+	BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |	\
+	BIT_ULL(POWER_DOMAIN_PIPE_B_PANEL_FITTER) |	\
+	BIT_ULL(POWER_DOMAIN_TRANSCODER_A) |	\
+	BIT_ULL(POWER_DOMAIN_TRANSCODER_B) |	\
+	BIT_ULL(POWER_DOMAIN_INIT))
+
 static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
 	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = i9xx_always_on_power_well_noop,
@@ -1910,6 +1951,27 @@ static struct i915_power_well i9xx_always_on_power_well[] = {
 	},
 };
 
+static const struct i915_power_well_ops i830_pipes_power_well_ops = {
+	.sync_hw = i830_pipes_power_well_sync_hw,
+	.enable = i830_pipes_power_well_enable,
+	.disable = i830_pipes_power_well_disable,
+	.is_enabled = i830_pipes_power_well_enabled,
+};
+
+static struct i915_power_well i830_power_wells[] = {
+	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = POWER_DOMAIN_MASK,
+		.ops = &i9xx_always_on_power_well_ops,
+	},
+	{
+		.name = "pipes",
+		.domains = I830_PIPES_POWER_DOMAINS,
+		.ops = &i830_pipes_power_well_ops,
+	},
+};
+
 static const struct i915_power_well_ops hsw_power_well_ops = {
 	.sync_hw = hsw_power_well_sync_hw,
 	.enable = hsw_power_well_enable,
@@ -2377,6 +2439,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 		set_power_wells(power_domains, chv_power_wells);
 	} else if (IS_VALLEYVIEW(dev_priv)) {
 		set_power_wells(power_domains, vlv_power_wells);
+	} else if (IS_I830(dev_priv)) {
+		set_power_wells(power_domains, i830_power_wells);
 	} else {
 		set_power_wells(power_domains, i9xx_always_on_power_well);
 	}
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915: Drop pipe A quirk for Toshiba Protege R205-S209

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The pipe A force quirk shouldn't needed except on 830. So let's nuke it
for the Toshiba Protege R-205/S-209 945 machines. This quirk pre-dates
KMS so it's usefulness is doubtful at best now.

Unfortunately the original bug report [1] isn't very helpful since it
doesn't describe the symptoms. And the commit message in xf86-video-intel
commit ecdb5963ef68 ("Add pipe A force enable quirk for Toshiba Portege R205-S209")
is not much help either.

However, if we assume the problem was the typical "closing the lid
hangs the box" type of thing, we already nuked the quirk for another
945 machine in
commit 736a69ca8c99 ("drm/i915: Drop PIPE-A quirk for 945GSE HP Mini")
and so I hope we can drop this one as well.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=14944

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1e5926a40a1..31e534c893ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14802,9 +14802,6 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = {
 };
 
 static struct intel_quirk intel_quirks[] = {
-	/* Toshiba Protege R-205, S-209 needs pipe A force quirk */
-	{ 0x2592, 0x1179, 0x0001, quirk_pipea_force },
-
 	/* ThinkPad T60 needs pipe A force quirk (bug #16494) */
 	{ 0x2782, 0x17aa, 0x201a, quirk_pipea_force },
 
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915: Drop pipe A quirk for Thinkapd T60

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The pipe A force quirk shouldn't needed except on 830. So let's nuke it
for the IBM Thinkpad T60 945 machines. This quirk pre-dates
KMS so it's usefulness is doubtful at best now.

The original bug report [1] describes the symptoms as "system hang on
closing T60 panel lid", and we already dropped a similar quirk for
another 945 machine in
commit 736a69ca8c99 ("drm/i915: Drop PIPE-A quirk for 945GSE HP Mini")
so I'm hopeful we can drop this one as well.

The quirk was added into xf86-video-intel in
commit 08903abe4dc0 ("Add pipe a force enable quirk for Lenovo T60")

[1] https://bugs.freedesktop.org/show_bug.cgi?id=16494

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 31e534c893ce..935e600ab179 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14802,9 +14802,6 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = {
 };
 
 static struct intel_quirk intel_quirks[] = {
-	/* ThinkPad T60 needs pipe A force quirk (bug #16494) */
-	{ 0x2782, 0x17aa, 0x201a, quirk_pipea_force },
-
 	/* 830 needs to leave pipe A & dpll A up */
 	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915: Remove pipe A quirk remnants

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

With 830 the only thing needing pipe quirks, we can just drop the quirk
defines and replace the checks with IS_I830() checks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 -
 drivers/gpu/drm/i915/intel_display.c | 92 ++++--------------------------------
 drivers/gpu/drm/i915/intel_overlay.c |  1 -
 3 files changed, 10 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bde554eb2257..4e5db51803f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1159,11 +1159,9 @@ enum intel_sbi_destination {
 	SBI_MPHY,
 };
 
-#define QUIRK_PIPEA_FORCE (1<<0)
 #define QUIRK_LVDS_SSC_DISABLE (1<<1)
 #define QUIRK_INVERT_BRIGHTNESS (1<<2)
 #define QUIRK_BACKLIGHT_PRESENT (1<<3)
-#define QUIRK_PIPEB_FORCE (1<<4)
 #define QUIRK_PIN_SWIZZLED_PAGES (1<<5)
 
 struct intel_fbdev;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 935e600ab179..774a6e523980 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1193,9 +1193,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 								      pipe);
 	enum intel_display_power_domain power_domain;
 
-	/* if we need the pipe quirk it must be always on */
-	if ((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-	    (pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
 		state = true;
 
 	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
@@ -1629,8 +1628,7 @@ static void i9xx_disable_pll(struct intel_crtc *crtc)
 	}
 
 	/* Don't disable pipe or pipe PLLs if needed */
-	if ((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-	    (pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+	if (IS_I830(dev_priv))
 		return;
 
 	/* Make sure the pipe isn't still relying on us */
@@ -1913,8 +1911,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
 	reg = PIPECONF(cpu_transcoder);
 	val = I915_READ(reg);
 	if (val & PIPECONF_ENABLE) {
-		WARN_ON(!((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-			  (pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)));
+		/* we keep both pipes enabled on 830 */
+		WARN_ON(!IS_I830(dev_priv));
 		return;
 	}
 
@@ -1974,8 +1972,7 @@ static void intel_disable_pipe(struct intel_crtc *crtc)
 		val &= ~PIPECONF_DOUBLE_WIDE;
 
 	/* Don't disable pipe or pipe PLLs if needed */
-	if (!(pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) &&
-	    !(pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+	if (!IS_I830(dev_priv))
 		val &= ~PIPECONF_ENABLE;
 
 	I915_WRITE(reg, val);
@@ -7049,8 +7046,8 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 
 	pipeconf = 0;
 
-	if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-	    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
 		pipeconf |= I915_READ(PIPECONF(intel_crtc->pipe)) & PIPECONF_ENABLE;
 
 	if (intel_crtc->config->double_wide)
@@ -12220,9 +12217,8 @@ verify_crtc_state(struct drm_crtc *crtc,
 
 	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
 
-	/* hw state is inconsistent with the pipe quirk */
-	if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-	    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
 		active = new_crtc_state->active;
 
 	I915_STATE_WARN(new_crtc_state->active != active,
@@ -14717,27 +14713,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 }
 
 /*
- * Some BIOSes insist on assuming the GPU's pipe A is enabled at suspend,
- * resume, or other times.  This quirk makes sure that's the case for
- * affected systems.
- */
-static void quirk_pipea_force(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-
-	dev_priv->quirks |= QUIRK_PIPEA_FORCE;
-	DRM_INFO("applying pipe a force quirk\n");
-}
-
-static void quirk_pipeb_force(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-
-	dev_priv->quirks |= QUIRK_PIPEB_FORCE;
-	DRM_INFO("applying pipe b force quirk\n");
-}
-
-/*
  * Some machines (Lenovo U160) do not work with SSC on LVDS for some reason
  */
 static void quirk_ssc_force_disable(struct drm_device *dev)
@@ -14802,12 +14777,6 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = {
 };
 
 static struct intel_quirk intel_quirks[] = {
-	/* 830 needs to leave pipe A & dpll A up */
-	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
-
-	/* 830 needs to leave pipe B & dpll B up */
-	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipeb_force },
-
 	/* Lenovo U160 cannot use SSC on LVDS */
 	{ 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable },
 
@@ -15213,37 +15182,6 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
 	POSTING_READ(DPLL(pipe));
 }
 
-static void intel_enable_pipe_a(struct drm_device *dev,
-				struct drm_modeset_acquire_ctx *ctx)
-{
-	struct intel_connector *connector;
-	struct drm_connector_list_iter conn_iter;
-	struct drm_connector *crt = NULL;
-	struct intel_load_detect_pipe load_detect_temp;
-	int ret;
-
-	/* We can't just switch on the pipe A, we need to set things up with a
-	 * proper mode and output configuration. As a gross hack, enable pipe A
-	 * by enabling the load detect pipe once. */
-	drm_connector_list_iter_begin(dev, &conn_iter);
-	for_each_intel_connector_iter(connector, &conn_iter) {
-		if (connector->encoder->type == INTEL_OUTPUT_ANALOG) {
-			crt = &connector->base;
-			break;
-		}
-	}
-	drm_connector_list_iter_end(&conn_iter);
-
-	if (!crt)
-		return;
-
-	ret = intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, ctx);
-	WARN(ret < 0, "All modeset mutexes are locked, but intel_get_load_detect_pipe failed\n");
-
-	if (ret > 0)
-		intel_release_load_detect_pipe(crt, &load_detect_temp, ctx);
-}
-
 static bool
 intel_check_plane_mapping(struct intel_crtc *crtc)
 {
@@ -15342,16 +15280,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		crtc->plane = plane;
 	}
 
-	if (!IS_I830(dev_priv) &&
-	    dev_priv->quirks & QUIRK_PIPEA_FORCE &&
-	    crtc->pipe == PIPE_A && !crtc->active) {
-		/* BIOS forgot to enable pipe A, this mostly happens after
-		 * resume. Force-enable the pipe to fix this, the update_dpms
-		 * call below we restore the pipe to the right state, but leave
-		 * the required bits on. */
-		intel_enable_pipe_a(dev, ctx);
-	}
-
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	if (crtc->active && !intel_crtc_has_encoders(crtc))
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 2e0c56ed22bb..b96aed941b97 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -270,7 +270,6 @@ static int intel_overlay_on(struct intel_overlay *overlay)
 	u32 *cs;
 
 	WARN_ON(overlay->active);
-	WARN_ON(IS_I830(dev_priv) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
 
 	req = alloc_request(overlay);
 	if (IS_ERR(req))
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] drm/i915: Pipe A quirk rework

[Chris Wilson]

On Thu, Jun 01, 2017 at 05:36:12PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Note that I'm entirely removing the few remaining pipe A quirks
> for non-830 platforms as they predate KMS and the hardware
> really shouldn't need them.

>   drm/i915: Drop pipe A quirk for Toshiba Protege R205-S209
>   drm/i915: Drop pipe A quirk for Thinkapd T60
>   drm/i915: Remove pipe A quirk remnants

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

I've wanted to do that for a long time.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Add i830 "pipes power well"

[Chris Wilson]

On Thu, Jun 01, 2017 at 05:36:16PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 830 more or less requires both pipes and DPLLs to remain on as long
> as either pipe is needed. However, when neither pipe is actually needed,
> we can save a bit of power by turning everything off. To do that we add
> a new "power well" that turns both pipes and DPLLs on and off in the
> right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010.
> 
> This also avoids having to abuse the load detection to force pipe A on
> at init time. That was never very robust, and it only worked for one
> pipe, whereas 830 really needs both pipes enabled. As a bonus the 830
> pipe quirk is now a bit more isolated from the rest of the mode setting
> infrastructure, which should mean that it's much less likely someone
> will accidentally break it in the future. The extra cost is of course
> slight code duplication, but that seems like a worthwile tradeoff here.

Defining it as a powerwell is an interesting way to do it, and seems
quite apt. My only nit is a request not to add to intel_display.c if we
can place it elsewhere, intel_gen2_pm.c ? gen2_runtime_pm.c?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Jani Nikula]

On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pass down the correct acquire context to the pipe A quirk load detect
> hack during display resume. Avoids deadlocking the entire thing.

Have we seen this in the wild? References?

BR,
Jani.

>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed41b59ee8e3..2f76a63efe8c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>  static void skylake_pfit_enable(struct intel_crtc *crtc);
>  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>  static void ironlake_pfit_enable(struct intel_crtc *crtc);
> -static void intel_modeset_setup_hw_state(struct drm_device *dev);
> +static void intel_modeset_setup_hw_state(struct drm_device *dev,
> +					 struct drm_modeset_acquire_ctx *ctx);
>  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
>  
>  struct intel_limit {
> @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int i, ret;
>  
> -	intel_modeset_setup_hw_state(dev);
> +	intel_modeset_setup_hw_state(dev, ctx);
>  	i915_redisable_vga(to_i915(dev));
>  
>  	if (!state)
> @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
>  	intel_setup_outputs(dev_priv);
>  
>  	drm_modeset_lock_all(dev);
> -	intel_modeset_setup_hw_state(dev);
> +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static void intel_enable_pipe_a(struct drm_device *dev)
> +static void intel_enable_pipe_a(struct drm_device *dev,
> +				struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct intel_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *crt = NULL;
>  	struct intel_load_detect_pipe load_detect_temp;
> -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>  	int ret;
>  
>  	/* We can't just switch on the pipe A, we need to set things up with a
> @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
>  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
>  }
>  
> -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
>  		 * the required bits on. */
> -		intel_enable_pipe_a(dev);
> +		intel_enable_pipe_a(dev, ctx);
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
>   * and sanitizes it to the current state
>   */
>  static void
> -intel_modeset_setup_hw_state(struct drm_device *dev)
> +intel_modeset_setup_hw_state(struct drm_device *dev,
> +			     struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum pipe pipe;
> @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>  	for_each_pipe(dev_priv, pipe) {
>  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  
> -		intel_sanitize_crtc(crtc);
> +		intel_sanitize_crtc(crtc, ctx);
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[setup_hw_state]");
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Jani Nikula]

On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If intel_crtc_disable_noatomic() were to ever get called during resume
> e'd end up deadlocking since resume has its own acqcuire_ctx but
> intel_crtc_disable_noatomic() still tries to use the
> mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.

Same here, have we seen this, or "just" theoretical?

BR,
Jani.

>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2f76a63efe8c..4e3c64ed4131 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
>  		intel_update_watermarks(intel_crtc);
>  }
>  
> -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> +					struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct intel_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  		return;
>  	}
>  
> -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> +	state->acquire_ctx = ctx;
>  
>  	/* Everything's already locked, -EDEADLK can't happen. */
>  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		plane = crtc->plane;
>  		crtc->base.primary->state->visible = true;
>  		crtc->plane = !plane;
> -		intel_crtc_disable_noatomic(&crtc->base);
> +		intel_crtc_disable_noatomic(&crtc->base, ctx);
>  		crtc->plane = plane;
>  	}
>  
> @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  	/* Adjust the state of the output pipe according to whether we
>  	 * have active connectors/encoders. */
>  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> -		intel_crtc_disable_noatomic(&crtc->base);
> +		intel_crtc_disable_noatomic(&crtc->base, ctx);
>  
>  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
>  		/*

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/7] drm/i915: Use a loop for the "three times for luck" DPLL procedure

[Chris Wilson]

On Thu, Jun 01, 2017 at 05:36:15PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The magic "enable the  DPLL three times" sequence feels like it
> deserves a loop.

Hmm, I thought once upon a time we figured out what the magic was about.
Or was that another loop?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Use a loop for the "three times for luck" DPLL procedure

[Jani Nikula]

On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The magic "enable the  DPLL three times" sequence feels like it
> deserves a loop.

I think the copy-paste unrolled loop is more fun. ;)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4e3c64ed4131..2b112229b5b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1550,6 +1550,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	i915_reg_t reg = DPLL(crtc->pipe);
>  	u32 dpll = crtc->config->dpll_hw_state.dpll;
> +	int i;
>  
>  	assert_pipe_disabled(dev_priv, crtc->pipe);
>  
> @@ -1596,15 +1597,11 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>  	}
>  
>  	/* We do this three times for luck */
> -	I915_WRITE(reg, dpll);
> -	POSTING_READ(reg);
> -	udelay(150); /* wait for warmup */
> -	I915_WRITE(reg, dpll);
> -	POSTING_READ(reg);
> -	udelay(150); /* wait for warmup */
> -	I915_WRITE(reg, dpll);
> -	POSTING_READ(reg);
> -	udelay(150); /* wait for warmup */
> +	for (i = 0; i < 3; i++) {
> +		I915_WRITE(reg, dpll);
> +		POSTING_READ(reg);
> +		udelay(150); /* wait for warmup */
> +	}
>  }
>  
>  /**

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Pipe A quirk rework

[Patchwork]

== Series Details ==

Series: drm/i915: Pipe A quirk rework
URL   : https://patchwork.freedesktop.org/series/25169/
State : success

== Summary ==

Series 25169v1 drm/i915: Pipe A quirk rework
https://patchwork.freedesktop.org/api/1.0/series/25169/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:441s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:434s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:588s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:519s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:490s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:417s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:495s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:462s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:463s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:567s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:454s
fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:432s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:464s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:501s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:411s

2c9abf8ec88cf4b3d3735976bb0ff7a4991946b2 drm-tip: 2017y-06m-01d-13h-29m-05s UTC integration manifest
ff4fbbc drm/i915: Remove pipe A quirk remnants
7b6bcec drm/i915: Drop pipe A quirk for Thinkapd T60
24fa310 drm/i915: Drop pipe A quirk for Toshiba Protege R205-S209
32f154c drm/i915: Add i830 "pipes power well"
132e825 drm/i915: Use a loop for the "three times for luck" DPLL procedure
15ac5e0 drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()
7efca7d drm/i915: Fix deadlock witha the pipe A quirk during resume

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4857/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Pass down the correct acquire context to the pipe A quirk load detect
> > hack during display resume. Avoids deadlocking the entire thing.
> 
> Have we seen this in the wild? References?

My 830.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ed41b59ee8e3..2f76a63efe8c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
> > +					 struct drm_modeset_acquire_ctx *ctx);
> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
> >  
> >  struct intel_limit {
> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	int i, ret;
> >  
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, ctx);
> >  	i915_redisable_vga(to_i915(dev));
> >  
> >  	if (!state)
> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
> >  	intel_setup_outputs(dev_priv);
> >  
> >  	drm_modeset_lock_all(dev);
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
> >  	drm_modeset_unlock_all(dev);
> >  
> >  	for_each_intel_crtc(dev, crtc) {
> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > -static void intel_enable_pipe_a(struct drm_device *dev)
> > +static void intel_enable_pipe_a(struct drm_device *dev,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct drm_connector *crt = NULL;
> >  	struct intel_load_detect_pipe load_detect_temp;
> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
> >  	int ret;
> >  
> >  	/* We can't just switch on the pipe A, we need to set things up with a
> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> >  }
> >  
> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
> >  		 * call below we restore the pipe to the right state, but leave
> >  		 * the required bits on. */
> > -		intel_enable_pipe_a(dev);
> > +		intel_enable_pipe_a(dev, ctx);
> >  	}
> >  
> >  	/* Adjust the state of the output pipe according to whether we
> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> >   * and sanitizes it to the current state
> >   */
> >  static void
> > -intel_modeset_setup_hw_state(struct drm_device *dev)
> > +intel_modeset_setup_hw_state(struct drm_device *dev,
> > +			     struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum pipe pipe;
> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> >  	for_each_pipe(dev_priv, pipe) {
> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >  
> > -		intel_sanitize_crtc(crtc);
> > +		intel_sanitize_crtc(crtc, ctx);
> >  		intel_dump_pipe_config(crtc, crtc->config,
> >  				       "[setup_hw_state]");
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Pass down the correct acquire context to the pipe A quirk load detect
> > hack during display resume. Avoids deadlocking the entire thing.
> 
> Have we seen this in the wild? References?

My 830.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ed41b59ee8e3..2f76a63efe8c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
> > +					 struct drm_modeset_acquire_ctx *ctx);
> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
> >  
> >  struct intel_limit {
> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	int i, ret;
> >  
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, ctx);
> >  	i915_redisable_vga(to_i915(dev));
> >  
> >  	if (!state)
> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
> >  	intel_setup_outputs(dev_priv);
> >  
> >  	drm_modeset_lock_all(dev);
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
> >  	drm_modeset_unlock_all(dev);
> >  
> >  	for_each_intel_crtc(dev, crtc) {
> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > -static void intel_enable_pipe_a(struct drm_device *dev)
> > +static void intel_enable_pipe_a(struct drm_device *dev,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct drm_connector *crt = NULL;
> >  	struct intel_load_detect_pipe load_detect_temp;
> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
> >  	int ret;
> >  
> >  	/* We can't just switch on the pipe A, we need to set things up with a
> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> >  }
> >  
> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
> >  		 * call below we restore the pipe to the right state, but leave
> >  		 * the required bits on. */
> > -		intel_enable_pipe_a(dev);
> > +		intel_enable_pipe_a(dev, ctx);
> >  	}
> >  
> >  	/* Adjust the state of the output pipe according to whether we
> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> >   * and sanitizes it to the current state
> >   */
> >  static void
> > -intel_modeset_setup_hw_state(struct drm_device *dev)
> > +intel_modeset_setup_hw_state(struct drm_device *dev,
> > +			     struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum pipe pipe;
> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> >  	for_each_pipe(dev_priv, pipe) {
> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >  
> > -		intel_sanitize_crtc(crtc);
> > +		intel_sanitize_crtc(crtc, ctx);
> >  		intel_dump_pipe_config(crtc, crtc->config,
> >  				       "[setup_hw_state]");
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > If intel_crtc_disable_noatomic() were to ever get called during resume
> > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > intel_crtc_disable_noatomic() still tries to use the
> > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> 
> Same here, have we seen this, or "just" theoretical?

Happens on my 830 after adding the pipes powerwell. Before that the
machine resumed with both pipes off and the pipe A quirk left
crtc->active as false so intel_crtc_disable_noatomic() didn't get
called.

Hmm. Actually, now that I think about it more, I believe this should
happen for S4 even without the new powerwell since both pipes should
have been enabled by the BIOS when resuming from S4. I'll have to
run a proper to test to verify that.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2f76a63efe8c..4e3c64ed4131 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> >  		intel_update_watermarks(intel_crtc);
> >  }
> >  
> > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > +					struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> >  		return;
> >  	}
> >  
> > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > +	state->acquire_ctx = ctx;
> >  
> >  	/* Everything's already locked, -EDEADLK can't happen. */
> >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  		plane = crtc->plane;
> >  		crtc->base.primary->state->visible = true;
> >  		crtc->plane = !plane;
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  		crtc->plane = plane;
> >  	}
> >  
> > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  	/* Adjust the state of the output pipe according to whether we
> >  	 * have active connectors/encoders. */
> >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  
> >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> >  		/*
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > If intel_crtc_disable_noatomic() were to ever get called during resume
> > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > intel_crtc_disable_noatomic() still tries to use the
> > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> 
> Same here, have we seen this, or "just" theoretical?

Happens on my 830 after adding the pipes powerwell. Before that the
machine resumed with both pipes off and the pipe A quirk left
crtc->active as false so intel_crtc_disable_noatomic() didn't get
called.

Hmm. Actually, now that I think about it more, I believe this should
happen for S4 even without the new powerwell since both pipes should
have been enabled by the BIOS when resuming from S4. I'll have to
run a proper to test to verify that.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2f76a63efe8c..4e3c64ed4131 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> >  		intel_update_watermarks(intel_crtc);
> >  }
> >  
> > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > +					struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> >  		return;
> >  	}
> >  
> > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > +	state->acquire_ctx = ctx;
> >  
> >  	/* Everything's already locked, -EDEADLK can't happen. */
> >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  		plane = crtc->plane;
> >  		crtc->base.primary->state->visible = true;
> >  		crtc->plane = !plane;
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  		crtc->plane = plane;
> >  	}
> >  
> > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  	/* Adjust the state of the output pipe according to whether we
> >  	 * have active connectors/encoders. */
> >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  
> >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> >  		/*
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Use a loop for the "three times for luck" DPLL procedure

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 03:48:51PM +0100, Chris Wilson wrote:
> On Thu, Jun 01, 2017 at 05:36:15PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The magic "enable the  DPLL three times" sequence feels like it
> > deserves a loop.
> 
> Hmm, I thought once upon a time we figured out what the magic was about.
> Or was that another loop?

At least I've never tried to understand this particular loop. The
VGA_MODE_DISABLE thing I did manage to figure out. Whether that might
have been a contributing factor for adding this loop is unclear.

We actually have a couple more DPLL writes just before the loop, so we
might want consider folding those into the loop as well.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Add i830 "pipes power well"

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 03:46:43PM +0100, Chris Wilson wrote:
> On Thu, Jun 01, 2017 at 05:36:16PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 830 more or less requires both pipes and DPLLs to remain on as long
> > as either pipe is needed. However, when neither pipe is actually needed,
> > we can save a bit of power by turning everything off. To do that we add
> > a new "power well" that turns both pipes and DPLLs on and off in the
> > right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010.
> > 
> > This also avoids having to abuse the load detection to force pipe A on
> > at init time. That was never very robust, and it only worked for one
> > pipe, whereas 830 really needs both pipes enabled. As a bonus the 830
> > pipe quirk is now a bit more isolated from the rest of the mode setting
> > infrastructure, which should mean that it's much less likely someone
> > will accidentally break it in the future. The extra cost is of course
> > slight code duplication, but that seems like a worthwile tradeoff here.
> 
> Defining it as a powerwell is an interesting way to do it, and seems
> quite apt. My only nit is a request not to add to intel_display.c if we
> can place it elsewhere, intel_gen2_pm.c ? gen2_runtime_pm.c?

Not sure a new file for just these two functions is warranted. I was
actually thinking of keeping them reasonably close by to the normal
pipe/dpll enable/disable code so that if people make a change to one
they would realize that they should perhaps change the other one as
well. Although I suppose intel_display.c is so big that even having
them in the same file won't really guarantee that.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 07:05:41PM +0300, Ville Syrj�l� wrote:
> On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> > On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > >
> > > If intel_crtc_disable_noatomic() were to ever get called during resume
> > > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > > intel_crtc_disable_noatomic() still tries to use the
> > > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> > 
> > Same here, have we seen this, or "just" theoretical?
> 
> Happens on my 830 after adding the pipes powerwell. Before that the
> machine resumed with both pipes off and the pipe A quirk left
> crtc->active as false so intel_crtc_disable_noatomic() didn't get
> called.
> 
> Hmm. Actually, now that I think about it more, I believe this should
> happen for S4 even without the new powerwell since both pipes should
> have been enabled by the BIOS when resuming from S4. I'll have to
> run a proper to test to verify that.

Indeed. In fact it deadlocks already during the thaw phase between
creating the hibernation image and writing to it disk.

> 
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2f76a63efe8c..4e3c64ed4131 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> > >  		intel_update_watermarks(intel_crtc);
> > >  }
> > >  
> > > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > > +					struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct intel_encoder *encoder;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > >  		return;
> > >  	}
> > >  
> > > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > > +	state->acquire_ctx = ctx;
> > >  
> > >  	/* Everything's already locked, -EDEADLK can't happen. */
> > >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  		plane = crtc->plane;
> > >  		crtc->base.primary->state->visible = true;
> > >  		crtc->plane = !plane;
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  		crtc->plane = plane;
> > >  	}
> > >  
> > > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  	/* Adjust the state of the output pipe according to whether we
> > >  	 * have active connectors/encoders. */
> > >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  
> > >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> > >  		/*
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Ville Syrj�l�
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 07:05:41PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> > On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > If intel_crtc_disable_noatomic() were to ever get called during resume
> > > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > > intel_crtc_disable_noatomic() still tries to use the
> > > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> > 
> > Same here, have we seen this, or "just" theoretical?
> 
> Happens on my 830 after adding the pipes powerwell. Before that the
> machine resumed with both pipes off and the pipe A quirk left
> crtc->active as false so intel_crtc_disable_noatomic() didn't get
> called.
> 
> Hmm. Actually, now that I think about it more, I believe this should
> happen for S4 even without the new powerwell since both pipes should
> have been enabled by the BIOS when resuming from S4. I'll have to
> run a proper to test to verify that.

Indeed. In fact it deadlocks already during the thaw phase between
creating the hibernation image and writing to it disk.

> 
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2f76a63efe8c..4e3c64ed4131 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> > >  		intel_update_watermarks(intel_crtc);
> > >  }
> > >  
> > > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > > +					struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct intel_encoder *encoder;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > >  		return;
> > >  	}
> > >  
> > > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > > +	state->acquire_ctx = ctx;
> > >  
> > >  	/* Everything's already locked, -EDEADLK can't happen. */
> > >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  		plane = crtc->plane;
> > >  		crtc->base.primary->state->visible = true;
> > >  		crtc->plane = !plane;
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  		crtc->plane = plane;
> > >  	}
> > >  
> > > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  	/* Adjust the state of the output pipe according to whether we
> > >  	 * have active connectors/encoders. */
> > >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  
> > >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> > >  		/*
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Jani Nikula]

On Thu, 01 Jun 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
>> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Pass down the correct acquire context to the pipe A quirk load detect
>> > hack during display resume. Avoids deadlocking the entire thing.
>> 
>> Have we seen this in the wild? References?
>
> My 830.

Okay, just checking if the cc: stable was really warranted in this one
and the next patch. Carry on! :)

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Cc: stable@vger.kernel.org
>> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>> >  1 file changed, 12 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index ed41b59ee8e3..2f76a63efe8c 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
>> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
>> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
>> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +					 struct drm_modeset_acquire_ctx *ctx);
>> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
>> >  
>> >  struct intel_limit {
>> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
>> >  	struct drm_crtc *crtc;
>> >  	int i, ret;
>> >  
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, ctx);
>> >  	i915_redisable_vga(to_i915(dev));
>> >  
>> >  	if (!state)
>> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	intel_setup_outputs(dev_priv);
>> >  
>> >  	drm_modeset_lock_all(dev);
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>> >  	drm_modeset_unlock_all(dev);
>> >  
>> >  	for_each_intel_crtc(dev, crtc) {
>> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	return 0;
>> >  }
>> >  
>> > -static void intel_enable_pipe_a(struct drm_device *dev)
>> > +static void intel_enable_pipe_a(struct drm_device *dev,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct intel_connector *connector;
>> >  	struct drm_connector_list_iter conn_iter;
>> >  	struct drm_connector *crt = NULL;
>> >  	struct intel_load_detect_pipe load_detect_temp;
>> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>> >  	int ret;
>> >  
>> >  	/* We can't just switch on the pipe A, we need to set things up with a
>> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
>> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
>> >  }
>> >  
>> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_device *dev = crtc->base.dev;
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
>> >  		 * call below we restore the pipe to the right state, but leave
>> >  		 * the required bits on. */
>> > -		intel_enable_pipe_a(dev);
>> > +		intel_enable_pipe_a(dev, ctx);
>> >  	}
>> >  
>> >  	/* Adjust the state of the output pipe according to whether we
>> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
>> >   * and sanitizes it to the current state
>> >   */
>> >  static void
>> > -intel_modeset_setup_hw_state(struct drm_device *dev)
>> > +intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +			     struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> >  	enum pipe pipe;
>> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>> >  	for_each_pipe(dev_priv, pipe) {
>> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>> >  
>> > -		intel_sanitize_crtc(crtc);
>> > +		intel_sanitize_crtc(crtc, ctx);
>> >  		intel_dump_pipe_config(crtc, crtc->config,
>> >  				       "[setup_hw_state]");
>> >  	}
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Jani Nikula]

On Thu, 01 Jun 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
>> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Pass down the correct acquire context to the pipe A quirk load detect
>> > hack during display resume. Avoids deadlocking the entire thing.
>> 
>> Have we seen this in the wild? References?
>
> My 830.

Okay, just checking if the cc: stable was really warranted in this one
and the next patch. Carry on! :)

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Cc: stable@vger.kernel.org
>> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>> >  1 file changed, 12 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index ed41b59ee8e3..2f76a63efe8c 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
>> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
>> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
>> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +					 struct drm_modeset_acquire_ctx *ctx);
>> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
>> >  
>> >  struct intel_limit {
>> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
>> >  	struct drm_crtc *crtc;
>> >  	int i, ret;
>> >  
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, ctx);
>> >  	i915_redisable_vga(to_i915(dev));
>> >  
>> >  	if (!state)
>> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	intel_setup_outputs(dev_priv);
>> >  
>> >  	drm_modeset_lock_all(dev);
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>> >  	drm_modeset_unlock_all(dev);
>> >  
>> >  	for_each_intel_crtc(dev, crtc) {
>> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	return 0;
>> >  }
>> >  
>> > -static void intel_enable_pipe_a(struct drm_device *dev)
>> > +static void intel_enable_pipe_a(struct drm_device *dev,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct intel_connector *connector;
>> >  	struct drm_connector_list_iter conn_iter;
>> >  	struct drm_connector *crt = NULL;
>> >  	struct intel_load_detect_pipe load_detect_temp;
>> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>> >  	int ret;
>> >  
>> >  	/* We can't just switch on the pipe A, we need to set things up with a
>> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
>> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
>> >  }
>> >  
>> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_device *dev = crtc->base.dev;
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
>> >  		 * call below we restore the pipe to the right state, but leave
>> >  		 * the required bits on. */
>> > -		intel_enable_pipe_a(dev);
>> > +		intel_enable_pipe_a(dev, ctx);
>> >  	}
>> >  
>> >  	/* Adjust the state of the output pipe according to whether we
>> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
>> >   * and sanitizes it to the current state
>> >   */
>> >  static void
>> > -intel_modeset_setup_hw_state(struct drm_device *dev)
>> > +intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +			     struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> >  	enum pipe pipe;
>> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>> >  	for_each_pipe(dev_priv, pipe) {
>> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>> >  
>> > -		intel_sanitize_crtc(crtc);
>> > +		intel_sanitize_crtc(crtc, ctx);
>> >  		intel_dump_pipe_config(crtc, crtc->config,
>> >  				       "[setup_hw_state]");
>> >  	}
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Add i830 "pipes power well"

[Maarten Lankhorst]

Op 01-06-17 om 16:36 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 830 more or less requires both pipes and DPLLs to remain on as long
> as either pipe is needed. However, when neither pipe is actually needed,
> we can save a bit of power by turning everything off. To do that we add
> a new "power well" that turns both pipes and DPLLs on and off in the
> right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010.
>
> This also avoids having to abuse the load detection to force pipe A on
> at init time. That was never very robust, and it only worked for one
> pipe, whereas 830 really needs both pipes enabled. As a bonus the 830
> pipe quirk is now a bit more isolated from the rest of the mode setting
> infrastructure, which should mean that it's much less likely someone
> will accidentally break it in the future. The extra cost is of course
> slight code duplication, but that seems like a worthwile tradeoff here.
>
> v2; s/BIT/BIT_ULL/
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c    | 92 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 64 +++++++++++++++++++++++
>  3 files changed, 157 insertions(+), 1 deletion(-)

For patch 1-3:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

If you replace the remainder of crtc->config with pipe_config in
i9xx_crtc_enable and i9xx_crtc_disable and create a crtc_state for
disabling, won't it become possible to re-use some of the called
functions there instead of hardcoding the writes here?

For the rest of the series,

Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Add i830 "pipes power well"

[Ville Syrjälä]

On Tue, Jun 06, 2017 at 09:04:40AM +0200, Maarten Lankhorst wrote:
> Op 01-06-17 om 16:36 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 830 more or less requires both pipes and DPLLs to remain on as long
> > as either pipe is needed. However, when neither pipe is actually needed,
> > we can save a bit of power by turning everything off. To do that we add
> > a new "power well" that turns both pipes and DPLLs on and off in the
> > right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010.
> >
> > This also avoids having to abuse the load detection to force pipe A on
> > at init time. That was never very robust, and it only worked for one
> > pipe, whereas 830 really needs both pipes enabled. As a bonus the 830
> > pipe quirk is now a bit more isolated from the rest of the mode setting
> > infrastructure, which should mean that it's much less likely someone
> > will accidentally break it in the future. The extra cost is of course
> > slight code duplication, but that seems like a worthwile tradeoff here.
> >
> > v2; s/BIT/BIT_ULL/
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c    | 92 ++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 64 +++++++++++++++++++++++
> >  3 files changed, 157 insertions(+), 1 deletion(-)
> 
> For patch 1-3:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> If you replace the remainder of crtc->config with pipe_config in
> i9xx_crtc_enable and i9xx_crtc_disable and create a crtc_state for
> disabling, won't it become possible to re-use some of the called
> functions there instead of hardcoding the writes here?

I considered that, but it would tie the whole thing more closely
with atomic instead of making it more independent. So I decided
that making it stand on its own should result in less regressions.
If we had some way to test this code on modern platforms then I
think we could go with that approach without having to worry about
regressions so much.

I have a somewhat similar conundrum with intel_crtc_mode_get(). I
have a branch where I changed it to reuse the normal state readout,
but I'm a little worried about people changing the called functions
to require a full top level atomic state. I guess if we changed all
the state readout to use a free standing top level atomic state,
like Daniel has suggested, this issue would perhaps disappear since
I'd just add the top level state to intel_crtc_mode_get() as well.

> 
> For the rest of the series,
> 
> Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] drm/i915: Pipe A quirk rework

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:36:12PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This series eliminates the problematic load detect abuse for the
> pipe A quirk. My main motivations were to isolate these quirks
> more from atomic to avoid regressions, and to save a bit of extra
> power. I believe I cooked this up a few years ago but never posted
> it. In the meantine we had accumulated some more regressions in
> the existing code so I tossed in some fixes up front.
> 
> Note that I'm entirely removing the few remaining pipe A quirks
> for non-830 platforms as they predate KMS and the hardware
> really shouldn't need them.
> 
> Entire series available here:
> git://github.com/vsyrjala/linux.git alm_pipe_quirk_rework_4
> 
> Ville Syrjälä (7):
>   drm/i915: Fix deadlock witha the pipe A quirk during resume
>   drm/i915: Plumb the correct acquire ctx into
>     intel_crtc_disable_noatomic()
>   drm/i915: Use a loop for the "three times for luck" DPLL procedure
>   drm/i915: Add i830 "pipes power well"
>   drm/i915: Drop pipe A quirk for Toshiba Protege R205-S209
>   drm/i915: Drop pipe A quirk for Thinkapd T60
>   drm/i915: Remove pipe A quirk remnants

I pushed the entire lot to dinq as is (was too lazy to shuffle the
code around in the end). Thanks for the reviews.

> 
>  drivers/gpu/drm/i915/i915_drv.h         |   2 -
>  drivers/gpu/drm/i915/intel_display.c    | 209 +++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h        |   2 +
>  drivers/gpu/drm/i915/intel_overlay.c    |   1 -
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  64 ++++++++++
>  5 files changed, 177 insertions(+), 101 deletions(-)
> 
> -- 
> 2.10.2

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx