[PATCH 0/7] modeset rework prep patches

[PATCH 0/7] modeset rework prep patches

[Daniel Vetter]

Hi all,

I've noticed that a few prep patches of the modeset rework series haven't been
merged nor reviewed yet, so I've split them out in this resend. Mostly concern
really old gen2 stuff (dvo + pipe A quirk), but little patches in other areas.

Comments&review highly welcome.

Thanks, Daniel

Daniel Vetter (7):
  drm/i915: add missing gen2 pipe A quirk entries
  drm/i915/ns2501: kill pll A enabling hack
  drm/i915: rip out the overlay pipe A workaround
  drm/i915: prepare load-detect pipe code for dpms changes
  drm/i915: simplify dvo dpms interface
  drm/i915: kill a few unused things in dev_priv
  drm/i915: extract ironlake_fdi_pll_disable

 drivers/gpu/drm/i915/dvo.h           |   9 ++-
 drivers/gpu/drm/i915/dvo_ch7017.c    |   8 +--
 drivers/gpu/drm/i915/dvo_ch7xxx.c    |   4 +-
 drivers/gpu/drm/i915/dvo_ivch.c      |   8 +--
 drivers/gpu/drm/i915/dvo_ns2501.c    |  21 ++-----
 drivers/gpu/drm/i915/dvo_sil164.c    |   4 +-
 drivers/gpu/drm/i915/dvo_tfp410.c    |   4 +-
 drivers/gpu/drm/i915/i915_dma.c      |  22 ++++----
 drivers/gpu/drm/i915/i915_drv.h      |  13 ++---
 drivers/gpu/drm/i915/intel_display.c | 105 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_dvo.c     |   4 +-
 drivers/gpu/drm/i915/intel_overlay.c |  58 +------------------
 12 files changed, 94 insertions(+), 166 deletions(-)

-- 
1.7.11.2

[PATCH 1/7] drm/i915: add missing gen2 pipe A quirk entries

[Daniel Vetter]

For some odd reason we've missed i830 and a i855 variant. Also
kill the two now redundant i830 entries.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e3afe96..c10d50b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7027,21 +7027,17 @@ static struct intel_quirk intel_quirks[] = {
 	/* HP Mini needs pipe A force quirk (LP: #322104) */
 	{ 0x27ae, 0x103c, 0x361a, quirk_pipea_force },
 
-	/* Thinkpad R31 needs pipe A force quirk */
-	{ 0x3577, 0x1014, 0x0505, quirk_pipea_force },
 	/* Toshiba Protege R-205, S-209 needs pipe A force quirk */
 	{ 0x2592, 0x1179, 0x0001, quirk_pipea_force },
 
-	/* ThinkPad X30 needs pipe A force quirk (LP: #304614) */
-	{ 0x3577,  0x1014, 0x0513, quirk_pipea_force },
-	/* ThinkPad X40 needs pipe A force quirk */
-
 	/* ThinkPad T60 needs pipe A force quirk (bug #16494) */
 	{ 0x2782, 0x17aa, 0x201a, quirk_pipea_force },
 
 	/* 855 & before need to leave pipe A & dpll A up */
 	{ 0x3582, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 	{ 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
+	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
+	{ 0x358e, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 
 	/* Lenovo U160 cannot use SSC on LVDS */
 	{ 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable },
-- 
1.7.11.2

[PATCH] drm/i915: add missing gen2 pipe A quirk entries

[Daniel Vetter]

For some odd reason we've missed i830 and a i855 variant. Also
kill the two now redundant i830 entries.

v2: Don't add the missing 855 id to the pipe A quirk list, we seem to
lack justification for it.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e3afe96..db0793e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7027,21 +7027,16 @@ static struct intel_quirk intel_quirks[] = {
 	/* HP Mini needs pipe A force quirk (LP: #322104) */
 	{ 0x27ae, 0x103c, 0x361a, quirk_pipea_force },
 
-	/* Thinkpad R31 needs pipe A force quirk */
-	{ 0x3577, 0x1014, 0x0505, quirk_pipea_force },
 	/* Toshiba Protege R-205, S-209 needs pipe A force quirk */
 	{ 0x2592, 0x1179, 0x0001, quirk_pipea_force },
 
-	/* ThinkPad X30 needs pipe A force quirk (LP: #304614) */
-	{ 0x3577,  0x1014, 0x0513, quirk_pipea_force },
-	/* ThinkPad X40 needs pipe A force quirk */
-
 	/* ThinkPad T60 needs pipe A force quirk (bug #16494) */
 	{ 0x2782, 0x17aa, 0x201a, quirk_pipea_force },
 
 	/* 855 & before need to leave pipe A & dpll A up */
 	{ 0x3582, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 	{ 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
+	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 
 	/* Lenovo U160 cannot use SSC on LVDS */
 	{ 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable },
-- 
1.7.11.2

[PATCH 2/7] drm/i915/ns2501: kill pll A enabling hack

[Daniel Vetter]

With the pipe A quirk properly fixed up for i830M, this shouldn't be
required any longer.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/dvo_ns2501.c    | 7 -------
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c b/drivers/gpu/drm/i915/dvo_ns2501.c
index 1a0bad9..6bd383d 100644
--- a/drivers/gpu/drm/i915/dvo_ns2501.c
+++ b/drivers/gpu/drm/i915/dvo_ns2501.c
@@ -75,11 +75,6 @@ struct ns2501_priv {
 #define NSPTR(d) ((NS2501Ptr)(d->DriverPrivate.ptr))
 
 /*
- * Include the PLL launcher prototype
- */
-extern void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
-
-/*
  * For reasons unclear to me, the ns2501 at least on the Fujitsu/Siemens
  * laptops does not react on the i2c bus unless
  * both the PLL is running and the display is configured in its native
@@ -113,8 +108,6 @@ static void enable_dvo(struct intel_dvo_device *dvo)
 	I915_WRITE(DVOC_SRCDIM, 0x400300);	// 1024x768
 	I915_WRITE(FW_BLC, 0x1080304);
 
-	intel_enable_pll(dev_priv, 0);
-
 	I915_WRITE(DVOC, 0x90004084);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c10d50b..13e444c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1431,7 +1431,7 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
  *
  * Unfortunately needed by dvo_ns2501 since the dvo depends on it running.
  */
-void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+static void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
 	int reg;
 	u32 val;
-- 
1.7.11.2

[PATCH 3/7] drm/i915: rip out the overlay pipe A workaround

[Daniel Vetter]

Now that all affected i830M systems have the pipe A quirk set,
we don't need to do any special dances in the overlay code any
longer. And reading through the code I'm rather dubios that it
actually does what it claims to do ...

As a nice benefit this rips out a users of the crtc helper dpms
callback.

v2: As suggested by Chris Wilson, replace the code by an appropriate
WARN to ensure that the pipe A is indeed running.

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

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 830d0dd..c0f4858 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -235,54 +235,6 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	return 0;
 }
 
-/* Workaround for i830 bug where pipe a must be enable to change control regs */
-static int
-i830_activate_pipe_a(struct drm_device *dev)
-{
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc;
-	struct drm_crtc_helper_funcs *crtc_funcs;
-	struct drm_display_mode vesa_640x480 = {
-		DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175, 640, 656,
-			 752, 800, 0, 480, 489, 492, 525, 0,
-			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
-	}, *mode;
-
-	crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[0]);
-	if (crtc->dpms_mode == DRM_MODE_DPMS_ON)
-		return 0;
-
-	/* most i8xx have pipe a forced on, so don't trust dpms mode */
-	if (I915_READ(_PIPEACONF) & PIPECONF_ENABLE)
-		return 0;
-
-	crtc_funcs = crtc->base.helper_private;
-	if (crtc_funcs->dpms == NULL)
-		return 0;
-
-	DRM_DEBUG_DRIVER("Enabling pipe A in order to enable overlay\n");
-
-	mode = drm_mode_duplicate(dev, &vesa_640x480);
-
-	if (!drm_crtc_helper_set_mode(&crtc->base, mode,
-				       crtc->base.x, crtc->base.y,
-				       crtc->base.fb))
-		return 0;
-
-	crtc_funcs->dpms(&crtc->base, DRM_MODE_DPMS_ON);
-	return 1;
-}
-
-static void
-i830_deactivate_pipe_a(struct drm_device *dev)
-{
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[0];
-	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
-
-	crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
-}
-
 /* overlay needs to be disable in OCMD reg */
 static int intel_overlay_on(struct intel_overlay *overlay)
 {
@@ -290,17 +242,12 @@ static int intel_overlay_on(struct intel_overlay *overlay)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	struct drm_i915_gem_request *request;
-	int pipe_a_quirk = 0;
 	int ret;
 
 	BUG_ON(overlay->active);
 	overlay->active = 1;
 
-	if (IS_I830(dev)) {
-		pipe_a_quirk = i830_activate_pipe_a(dev);
-		if (pipe_a_quirk < 0)
-			return pipe_a_quirk;
-	}
+	WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
 
 	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL) {
@@ -322,9 +269,6 @@ static int intel_overlay_on(struct intel_overlay *overlay)
 
 	ret = intel_overlay_do_wait_request(overlay, request, NULL);
 out:
-	if (pipe_a_quirk)
-		i830_deactivate_pipe_a(dev);
-
 	return ret;
 }
 
-- 
1.7.11.2

[PATCH 4/7] drm/i915: prepare load-detect pipe code for dpms changes

[Daniel Vetter]

A few things need adjustement:
- Change the dpms state by calling the dpms connector function and
  not some crtc helper internal callbacks. Otherwise this will break
  once we switch to our own dpms handling.
- Instead of tracking and restoring intel_crtc->dpms_mode use the
  connector's dpms variable - the former relies on the dpms compuation
  rules used by the crtc helper. And it would break when the encoder
  is cloned and the other output has a different dpms state. But luckily
  no one is crazy enough for that.
- Properly clear the connector -> encoder -> crtc linking, even when
  failing (note that the crtc helper removes the encoder -> crtc link
  in disabled_unused_functions for us).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13e444c..082a8a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5625,21 +5625,12 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (encoder->crtc) {
 		crtc = encoder->crtc;
 
-		intel_crtc = to_intel_crtc(crtc);
-		old->dpms_mode = intel_crtc->dpms_mode;
+		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
-			struct drm_encoder_helper_funcs *encoder_funcs;
-			struct drm_crtc_helper_funcs *crtc_funcs;
-
-			crtc_funcs = crtc->helper_private;
-			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
-
-			encoder_funcs = encoder->helper_private;
-			encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
-		}
+		if (connector->dpms != DRM_MODE_DPMS_ON)
+			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
 
 		return true;
 	}
@@ -5667,7 +5658,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	connector->encoder = encoder;
 
 	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = intel_crtc->dpms_mode;
+	old->dpms_mode = connector->dpms;
 	old->load_detect_temp = true;
 	old->release_fb = NULL;
 
@@ -5692,22 +5683,25 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(crtc->fb)) {
 		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
-		crtc->fb = old_fb;
-		return false;
+		goto fail;
 	}
 
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
-		crtc->fb = old_fb;
-		return false;
+		goto fail;
 	}
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	return true;
+fail:
+	connector->encoder = NULL;
+	encoder->crtc = NULL;
+	crtc->fb = old_fb;
+	return false;
 }
 
 void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
@@ -5716,9 +5710,6 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_device *dev = encoder->dev;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
-	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, drm_get_connector_name(connector),
@@ -5726,6 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 
 	if (old->load_detect_temp) {
 		connector->encoder = NULL;
+		encoder->crtc = NULL;
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
@@ -5735,10 +5727,8 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 	}
 
 	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON) {
-		encoder_funcs->dpms(encoder, old->dpms_mode);
-		crtc_funcs->dpms(crtc, old->dpms_mode);
-	}
+	if (old->dpms_mode != DRM_MODE_DPMS_ON)
+		connector->funcs->dpms(connector, old->dpms_mode);
 }
 
 /* Returns the clock of the currently programmed mode of the given pipe. */
-- 
1.7.11.2

[PATCH] drm/i915: drop intel_encoder argument to load_detect_pipe functions

[Daniel Vetter]

Since it's redundant - we can get the attached encoder in the
functions themselves.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c     |  6 ++----
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++----
 drivers/gpu/drm/i915/intel_tv.c      |  7 ++-----
 4 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d19f72c..85645d3 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -582,14 +582,12 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		return connector->status;
 
 	/* for pre-945g platforms use load detect */
-	if (intel_get_load_detect_pipe(&crt->base, connector, NULL,
-				       &tmp)) {
+	if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {
 		if (intel_crt_detect_ddc(connector))
 			status = connector_status_connected;
 		else
 			status = intel_crt_load_detect(crt);
-		intel_release_load_detect_pipe(&crt->base, connector,
-					       &tmp);
+		intel_release_load_detect_pipe(connector, &tmp);
 	} else
 		status = connector_status_unknown;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0542b37..0d22105 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5633,12 +5633,13 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	return fb;
 }
 
-bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-				struct drm_connector *connector,
+bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct drm_display_mode *mode,
 				struct intel_load_detect_pipe *old)
 {
 	struct intel_crtc *intel_crtc;
+	struct intel_encoder *intel_encoder =
+		intel_attached_encoder(connector);
 	struct drm_crtc *possible_crtc;
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
@@ -5740,10 +5741,11 @@ fail:
 	return false;
 }
 
-void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
-				    struct drm_connector *connector,
+void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old)
 {
+	struct intel_encoder *intel_encoder =
+		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
@@ -7889,10 +7891,8 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	if (!crt)
 		return;
 
-	if (intel_get_load_detect_pipe(intel_attached_encoder(crt),
-				       crt, NULL, &load_detect_temp))
-		intel_release_load_detect_pipe(intel_attached_encoder(crt),
-					       crt, &load_detect_temp);
+	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp))
+		intel_release_load_detect_pipe(crt, &load_detect_temp);
 
 
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f780ee..4a9cdd1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -484,12 +484,10 @@ struct intel_load_detect_pipe {
 	bool load_detect_temp;
 	int dpms_mode;
 };
-extern bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-				       struct drm_connector *connector,
+extern bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				       struct drm_display_mode *mode,
 				       struct intel_load_detect_pipe *old);
-extern void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
-					   struct drm_connector *connector,
+extern void intel_release_load_detect_pipe(struct drm_connector *connector,
 					   struct intel_load_detect_pipe *old);
 
 extern void intelfb_restore(void);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index ede0e44..c9b2fb0 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1315,12 +1315,9 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	if (force) {
 		struct intel_load_detect_pipe tmp;
 
-		if (intel_get_load_detect_pipe(&intel_tv->base, connector,
-					       &mode, &tmp)) {
+		if (intel_get_load_detect_pipe(connector, &mode, &tmp)) {
 			type = intel_tv_detect_type(intel_tv, connector);
-			intel_release_load_detect_pipe(&intel_tv->base,
-						       connector,
-						       &tmp);
+			intel_release_load_detect_pipe(connector, &tmp);
 		} else
 			return connector_status_unknown;
 	} else
-- 
1.7.11.2

[PATCH 5/7] drm/i915: simplify dvo dpms interface

[Daniel Vetter]

All dvo drivers only support 2 dpms states, and our dvo driver
even switches of the dvo port for anything else than DPMS_ON. Hence
ditch this complexity and simply use bool enable.

While reading through this code I've noticed that the mode_set
function of ch7017 is a bit peculiar - it disable the lvds again, even
though the crtc helper code should have done that ... This might be to
work around an issue at driver load, we pretty much ignore the hw
state when taking over.

v2: Also do the conversion for the new ns2501 driver.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/dvo.h        |  9 ++++-----
 drivers/gpu/drm/i915/dvo_ch7017.c |  8 ++++----
 drivers/gpu/drm/i915/dvo_ch7xxx.c |  4 ++--
 drivers/gpu/drm/i915/dvo_ivch.c   |  8 ++++----
 drivers/gpu/drm/i915/dvo_ns2501.c | 14 ++++++--------
 drivers/gpu/drm/i915/dvo_sil164.c |  4 ++--
 drivers/gpu/drm/i915/dvo_tfp410.c |  4 ++--
 drivers/gpu/drm/i915/intel_dvo.c  |  4 ++--
 8 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/dvo.h b/drivers/gpu/drm/i915/dvo.h
index 0c8ac4d..0fa839e 100644
--- a/drivers/gpu/drm/i915/dvo.h
+++ b/drivers/gpu/drm/i915/dvo.h
@@ -58,13 +58,12 @@ struct intel_dvo_dev_ops {
 	void (*create_resources)(struct intel_dvo_device *dvo);
 
 	/*
-	 * Turn on/off output or set intermediate power levels if available.
+	 * Turn on/off output.
 	 *
-	 * Unsupported intermediate modes drop to the lower power setting.
-	 * If the  mode is DPMSModeOff, the output must be disabled,
-	 * as the DPLL may be disabled afterwards.
+	 * Because none of our dvo drivers support an intermediate power levels,
+	 * we don't expose this in the interfac.
 	 */
-	void (*dpms)(struct intel_dvo_device *dvo, int mode);
+	void (*dpms)(struct intel_dvo_device *dvo, bool enable);
 
 	/*
 	 * Callback for testing a video mode for a given output.
diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
index 1ca799a..71e7650 100644
--- a/drivers/gpu/drm/i915/dvo_ch7017.c
+++ b/drivers/gpu/drm/i915/dvo_ch7017.c
@@ -163,7 +163,7 @@ struct ch7017_priv {
 };
 
 static void ch7017_dump_regs(struct intel_dvo_device *dvo);
-static void ch7017_dpms(struct intel_dvo_device *dvo, int mode);
+static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable);
 
 static bool ch7017_read(struct intel_dvo_device *dvo, u8 addr, u8 *val)
 {
@@ -309,7 +309,7 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
 	lvds_power_down = CH7017_LVDS_POWER_DOWN_DEFAULT_RESERVED |
 			  (mode->hdisplay & 0x0700) >> 8;
 
-	ch7017_dpms(dvo, DRM_MODE_DPMS_OFF);
+	ch7017_dpms(dvo, false);
 	ch7017_write(dvo, CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT,
 			horizontal_active_pixel_input);
 	ch7017_write(dvo, CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT,
@@ -331,7 +331,7 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
 }
 
 /* set the CH7017 power state */
-static void ch7017_dpms(struct intel_dvo_device *dvo, int mode)
+static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable)
 {
 	uint8_t val;
 
@@ -345,7 +345,7 @@ static void ch7017_dpms(struct intel_dvo_device *dvo, int mode)
 			CH7017_DAC3_POWER_DOWN |
 			CH7017_TV_POWER_DOWN_EN);
 
-	if (mode == DRM_MODE_DPMS_ON) {
+	if (enable) {
 		/* Turn on the LVDS */
 		ch7017_write(dvo, CH7017_LVDS_POWER_DOWN,
 			     val & ~CH7017_LVDS_POWER_DOWN_EN);
diff --git a/drivers/gpu/drm/i915/dvo_ch7xxx.c b/drivers/gpu/drm/i915/dvo_ch7xxx.c
index 4a03660..c1dea5b 100644
--- a/drivers/gpu/drm/i915/dvo_ch7xxx.c
+++ b/drivers/gpu/drm/i915/dvo_ch7xxx.c
@@ -289,9 +289,9 @@ static void ch7xxx_mode_set(struct intel_dvo_device *dvo,
 }
 
 /* set the CH7xxx power state */
-static void ch7xxx_dpms(struct intel_dvo_device *dvo, int mode)
+static void ch7xxx_dpms(struct intel_dvo_device *dvo, bool enable)
 {
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		ch7xxx_writeb(dvo, CH7xxx_PM, CH7xxx_PM_DVIL | CH7xxx_PM_DVIP);
 	else
 		ch7xxx_writeb(dvo, CH7xxx_PM, CH7xxx_PM_FPD);
diff --git a/drivers/gpu/drm/i915/dvo_ivch.c b/drivers/gpu/drm/i915/dvo_ivch.c
index 04f2893..fa8ff6b 100644
--- a/drivers/gpu/drm/i915/dvo_ivch.c
+++ b/drivers/gpu/drm/i915/dvo_ivch.c
@@ -288,7 +288,7 @@ static enum drm_mode_status ivch_mode_valid(struct intel_dvo_device *dvo,
 }
 
 /** Sets the power state of the panel connected to the ivch */
-static void ivch_dpms(struct intel_dvo_device *dvo, int mode)
+static void ivch_dpms(struct intel_dvo_device *dvo, bool enable)
 {
 	int i;
 	uint16_t vr01, vr30, backlight;
@@ -297,13 +297,13 @@ static void ivch_dpms(struct intel_dvo_device *dvo, int mode)
 	if (!ivch_read(dvo, VR01, &vr01))
 		return;
 
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		backlight = 1;
 	else
 		backlight = 0;
 	ivch_write(dvo, VR80, backlight);
 
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		vr01 |= VR01_LCD_ENABLE | VR01_DVO_ENABLE;
 	else
 		vr01 &= ~(VR01_LCD_ENABLE | VR01_DVO_ENABLE);
@@ -315,7 +315,7 @@ static void ivch_dpms(struct intel_dvo_device *dvo, int mode)
 		if (!ivch_read(dvo, VR30, &vr30))
 			break;
 
-		if (((vr30 & VR30_PANEL_ON) != 0) == (mode == DRM_MODE_DPMS_ON))
+		if (((vr30 & VR30_PANEL_ON) != 0) == enable)
 			break;
 		udelay(1000);
 	}
diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c b/drivers/gpu/drm/i915/dvo_ns2501.c
index 6bd383d..c4d9f2f 100644
--- a/drivers/gpu/drm/i915/dvo_ns2501.c
+++ b/drivers/gpu/drm/i915/dvo_ns2501.c
@@ -493,19 +493,19 @@ static void ns2501_mode_set(struct intel_dvo_device *dvo,
 }
 
 /* set the NS2501 power state */
-static void ns2501_dpms(struct intel_dvo_device *dvo, int mode)
+static void ns2501_dpms(struct intel_dvo_device *dvo, bool enable)
 {
 	bool ok;
 	bool restore = false;
 	struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
 	unsigned char ch;
 
-	DRM_DEBUG_KMS("%s: Trying set the dpms of the DVO to %d\n",
-		      __FUNCTION__, mode);
+	DRM_DEBUG_KMS("%s: Trying set the dpms of the DVO to %i\n",
+		      __FUNCTION__, enable);
 
 	ch = ns->reg_8_shadow;
 
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		ch |= NS2501_8_PD;
 	else
 		ch &= ~NS2501_8_PD;
@@ -519,12 +519,10 @@ static void ns2501_dpms(struct intel_dvo_device *dvo, int mode)
 			ok &= ns2501_writeb(dvo, NS2501_REG8, ch);
 			ok &=
 			    ns2501_writeb(dvo, 0x34,
-					  (mode ==
-					   DRM_MODE_DPMS_ON) ? (0x03) : (0x00));
+					  enable ? 0x03 : 0x00);
 			ok &=
 			    ns2501_writeb(dvo, 0x35,
-					  (mode ==
-					   DRM_MODE_DPMS_ON) ? (0xff) : (0x00));
+					  enable ? 0xff : 0x00);
 			if (!ok) {
 				if (restore)
 					restore_dvo(dvo);
diff --git a/drivers/gpu/drm/i915/dvo_sil164.c b/drivers/gpu/drm/i915/dvo_sil164.c
index a0b13a6..cc24c1c 100644
--- a/drivers/gpu/drm/i915/dvo_sil164.c
+++ b/drivers/gpu/drm/i915/dvo_sil164.c
@@ -208,7 +208,7 @@ static void sil164_mode_set(struct intel_dvo_device *dvo,
 }
 
 /* set the SIL164 power state */
-static void sil164_dpms(struct intel_dvo_device *dvo, int mode)
+static void sil164_dpms(struct intel_dvo_device *dvo, bool enable)
 {
 	int ret;
 	unsigned char ch;
@@ -217,7 +217,7 @@ static void sil164_dpms(struct intel_dvo_device *dvo, int mode)
 	if (ret == false)
 		return;
 
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		ch |= SIL164_8_PD;
 	else
 		ch &= ~SIL164_8_PD;
diff --git a/drivers/gpu/drm/i915/dvo_tfp410.c b/drivers/gpu/drm/i915/dvo_tfp410.c
index aa2cd3e..097b3e8 100644
--- a/drivers/gpu/drm/i915/dvo_tfp410.c
+++ b/drivers/gpu/drm/i915/dvo_tfp410.c
@@ -234,14 +234,14 @@ static void tfp410_mode_set(struct intel_dvo_device *dvo,
 }
 
 /* set the tfp410 power state */
-static void tfp410_dpms(struct intel_dvo_device *dvo, int mode)
+static void tfp410_dpms(struct intel_dvo_device *dvo, bool enable)
 {
 	uint8_t ctl1;
 
 	if (!tfp410_readb(dvo, TFP410_CTL_1, &ctl1))
 		return;
 
-	if (mode == DRM_MODE_DPMS_ON)
+	if (enable)
 		ctl1 |= TFP410_CTL_1_PD;
 	else
 		ctl1 &= ~TFP410_CTL_1_PD;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 03dfdff..227551f 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -115,9 +115,9 @@ static void intel_dvo_dpms(struct drm_encoder *encoder, int mode)
 	if (mode == DRM_MODE_DPMS_ON) {
 		I915_WRITE(dvo_reg, temp | DVO_ENABLE);
 		I915_READ(dvo_reg);
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, mode);
+		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
 	} else {
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, mode);
+		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false);
 		I915_WRITE(dvo_reg, temp & ~DVO_ENABLE);
 		I915_READ(dvo_reg);
 	}
-- 
1.7.11.2

[PATCH 6/7] drm/i915: kill a few unused things in dev_priv

[Daniel Vetter]

... and move a few others only used by i915_dma.c into the dri1
dungeon.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c | 22 +++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h | 13 ++++++-------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0a1b64f..2285ae3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -235,10 +235,10 @@ static int i915_initialize(struct drm_device * dev, drm_i915_init_t * init)
 		}
 	}
 
-	dev_priv->cpp = init->cpp;
-	dev_priv->back_offset = init->back_offset;
-	dev_priv->front_offset = init->front_offset;
-	dev_priv->current_page = 0;
+	dev_priv->dri1.cpp = init->cpp;
+	dev_priv->dri1.back_offset = init->back_offset;
+	dev_priv->dri1.front_offset = init->front_offset;
+	dev_priv->dri1.current_page = 0;
 	if (master_priv->sarea_priv)
 		master_priv->sarea_priv->pf_current_page = 0;
 
@@ -575,7 +575,7 @@ static int i915_dispatch_flip(struct drm_device * dev)
 
 	DRM_DEBUG_DRIVER("%s: page=%d pfCurrentPage=%d\n",
 			  __func__,
-			 dev_priv->current_page,
+			 dev_priv->dri1.current_page,
 			 master_priv->sarea_priv->pf_current_page);
 
 	i915_kernel_lost_context(dev);
@@ -589,12 +589,12 @@ static int i915_dispatch_flip(struct drm_device * dev)
 
 	OUT_RING(CMD_OP_DISPLAYBUFFER_INFO | ASYNC_FLIP);
 	OUT_RING(0);
-	if (dev_priv->current_page == 0) {
-		OUT_RING(dev_priv->back_offset);
-		dev_priv->current_page = 1;
+	if (dev_priv->dri1.current_page == 0) {
+		OUT_RING(dev_priv->dri1.back_offset);
+		dev_priv->dri1.current_page = 1;
 	} else {
-		OUT_RING(dev_priv->front_offset);
-		dev_priv->current_page = 0;
+		OUT_RING(dev_priv->dri1.front_offset);
+		dev_priv->dri1.current_page = 0;
 	}
 	OUT_RING(0);
 
@@ -613,7 +613,7 @@ static int i915_dispatch_flip(struct drm_device * dev)
 		ADVANCE_LP_RING();
 	}
 
-	master_priv->sarea_priv->pf_current_page = dev_priv->current_page;
+	master_priv->sarea_priv->pf_current_page = dev_priv->dri1.current_page;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 261fe21..ed3ba70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -428,12 +428,6 @@ typedef struct drm_i915_private {
 
 	struct resource mch_res;
 
-	unsigned int cpp;
-	int back_offset;
-	int front_offset;
-	int current_page;
-	int page_flipping;
-
 	atomic_t irq_received;
 
 	/* protects the irq masks */
@@ -451,7 +445,6 @@ typedef struct drm_i915_private {
 	u32 hotplug_supported_mask;
 	struct work_struct hotplug_work;
 
-	unsigned int sr01, adpa, ppcr, dvob, dvoc, lvds;
 	int num_pipe;
 	int num_pch_pll;
 
@@ -790,6 +783,12 @@ typedef struct drm_i915_private {
 	struct {
 		unsigned allow_batchbuffer : 1;
 		u32 __iomem *gfx_hws_cpu_addr;
+
+		unsigned int cpp;
+		int back_offset;
+		int front_offset;
+		int current_page;
+		int page_flipping;
 	} dri1;
 
 	/* Kernel Modesetting */
-- 
1.7.11.2

[PATCH 7/7] drm/i915: extract ironlake_fdi_pll_disable

[Daniel Vetter]

Simply to make the ilk+ crtc disable path clearer and more symmetric
with the enable function.

Also switch to intel_crtc for the enable function.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 082a8a7..72e01ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2708,11 +2708,10 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
 	DRM_DEBUG_KMS("FDI train done.\n");
 }
 
-static void ironlake_fdi_pll_enable(struct drm_crtc *crtc)
+static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
 {
-	struct drm_device *dev = crtc->dev;
+	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
@@ -2753,6 +2752,35 @@ static void ironlake_fdi_pll_enable(struct drm_crtc *crtc)
 	}
 }
 
+static void ironlake_fdi_pll_disable(struct intel_crtc *intel_crtc)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = intel_crtc->pipe;
+	u32 reg, temp;
+
+	/* Switch from PCDclk to Rawclk */
+	reg = FDI_RX_CTL(pipe);
+	temp = I915_READ(reg);
+	I915_WRITE(reg, temp & ~FDI_PCDCLK);
+
+	/* Disable CPU FDI TX PLL */
+	reg = FDI_TX_CTL(pipe);
+	temp = I915_READ(reg);
+	I915_WRITE(reg, temp & ~FDI_TX_PLL_ENABLE);
+
+	POSTING_READ(reg);
+	udelay(100);
+
+	reg = FDI_RX_CTL(pipe);
+	temp = I915_READ(reg);
+	I915_WRITE(reg, temp & ~FDI_RX_PLL_ENABLE);
+
+	/* Wait for the clocks to turn off. */
+	POSTING_READ(reg);
+	udelay(100);
+}
+
 static void cpt_phase_pointer_disable(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3200,7 +3228,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	is_pch_port = intel_crtc_driving_pch(crtc);
 
 	if (is_pch_port)
-		ironlake_fdi_pll_enable(crtc);
+		ironlake_fdi_pll_enable(intel_crtc);
 	else
 		ironlake_fdi_disable(crtc);
 
@@ -3303,26 +3331,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	/* disable PCH DPLL */
 	intel_disable_pch_pll(intel_crtc);
 
-	/* Switch from PCDclk to Rawclk */
-	reg = FDI_RX_CTL(pipe);
-	temp = I915_READ(reg);
-	I915_WRITE(reg, temp & ~FDI_PCDCLK);
-
-	/* Disable CPU FDI TX PLL */
-	reg = FDI_TX_CTL(pipe);
-	temp = I915_READ(reg);
-	I915_WRITE(reg, temp & ~FDI_TX_PLL_ENABLE);
-
-	POSTING_READ(reg);
-	udelay(100);
-
-	reg = FDI_RX_CTL(pipe);
-	temp = I915_READ(reg);
-	I915_WRITE(reg, temp & ~FDI_RX_PLL_ENABLE);
-
-	/* Wait for the clocks to turn off. */
-	POSTING_READ(reg);
-	udelay(100);
+	ironlake_fdi_pll_disable(intel_crtc);
 
 	intel_crtc->active = false;
 	intel_update_watermarks(dev);
-- 
1.7.11.2

Re: [PATCH 0/7] modeset rework prep patches

[Chris Wilson]

On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
> 
> I've noticed that a few prep patches of the modeset rework series haven't been
> merged nor reviewed yet, so I've split them out in this resend. Mostly concern
> really old gen2 stuff (dvo + pipe A quirk), but little patches in other areas.
> 
> Comments&review highly welcome.
> 
> Thanks, Daniel
> 
> Daniel Vetter (7):
>   drm/i915: add missing gen2 pipe A quirk entries

I remain dubious whether the 855gm entry is genuine.

>   drm/i915/ns2501: kill pll A enabling hack
>   drm/i915: rip out the overlay pipe A workaround

Look fine and a welcome reduction in code + confusion.

>   drm/i915: prepare load-detect pipe code for dpms changes

It is not immediately obvious from the function that there is a
relationship between the connector and intel_encoder.  If we derived the
encoder from the connector in that function, the reviewer's life gets a
little easier. As it stands the code looks correct and rightly removes
some internal details.

>   drm/i915: simplify dvo dpms interface

This just looks like churn for churn's sake? The changes look correct.

>   drm/i915: kill a few unused things in dev_priv

+1

>   drm/i915: extract ironlake_fdi_pll_disable

Smaller more descriptive functions, what is not to like.

2-7: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
You can have an r-b for 1 if you drop the 855gm chunk. Then get someone
else to a-b the 855gm entry :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/7] modeset rework prep patches

[Daniel Vetter]

On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote:
> On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hi all,
> > 
> > I've noticed that a few prep patches of the modeset rework series haven't been
> > merged nor reviewed yet, so I've split them out in this resend. Mostly concern
> > really old gen2 stuff (dvo + pipe A quirk), but little patches in other areas.
> > 
> > Comments&review highly welcome.
> > 
> > Thanks, Daniel
> > 
> > Daniel Vetter (7):
> >   drm/i915: add missing gen2 pipe A quirk entries
> 
> I remain dubious whether the 855gm entry is genuine.

Ok, since we don't have a bug to support this, I'll drop it again.

> >   drm/i915/ns2501: kill pll A enabling hack
> >   drm/i915: rip out the overlay pipe A workaround
> 
> Look fine and a welcome reduction in code + confusion.
> 
> >   drm/i915: prepare load-detect pipe code for dpms changes
> 
> It is not immediately obvious from the function that there is a
> relationship between the connector and intel_encoder.  If we derived the
> encoder from the connector in that function, the reviewer's life gets a
> little easier. As it stands the code looks correct and rightly removes
> some internal details.

Hm, good point. I'll add a patch on top that drops the intel_encoder
argument (since it's redudant, all callers get it with
intel_attached_encoder). Or better if I squash it together with this one?

> >   drm/i915: simplify dvo dpms interface
> 
> This just looks like churn for churn's sake? The changes look correct.

We don't bother with anything else than dpms on/off states in most of the
modeset code (even for crt newer hw drops the intermediate states). Hence
the new interfaces have only enable/disable functions at the encoder/crtc
level. I've figured it looks odd if we keep the full dpms interface for
dvo. But since it's rather independant churn I've moved it into this
odds bits series.

> >   drm/i915: kill a few unused things in dev_priv
> 
> +1
> 
> >   drm/i915: extract ironlake_fdi_pll_disable
> 
> Smaller more descriptive functions, what is not to like.
> 
> 2-7: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> You can have an r-b for 1 if you drop the 855gm chunk. Then get someone
> else to a-b the 855gm entry :)

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/7] modeset rework prep patches

[Chris Wilson]

On Sun, 12 Aug 2012 22:01:02 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote:
> > On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >   drm/i915: prepare load-detect pipe code for dpms changes
> > 
> > It is not immediately obvious from the function that there is a
> > relationship between the connector and intel_encoder.  If we derived the
> > encoder from the connector in that function, the reviewer's life gets a
> > little easier. As it stands the code looks correct and rightly removes
> > some internal details.
> 
> Hm, good point. I'll add a patch on top that drops the intel_encoder
> argument (since it's redudant, all callers get it with
> intel_attached_encoder). Or better if I squash it together with this one?

I'll buy it either way, with a slight preference to having it as two
steps.

> > >   drm/i915: simplify dvo dpms interface
> > 
> > This just looks like churn for churn's sake? The changes look correct.
> 
> We don't bother with anything else than dpms on/off states in most of the
> modeset code (even for crt newer hw drops the intermediate states). Hence
> the new interfaces have only enable/disable functions at the encoder/crtc
> level. I've figured it looks odd if we keep the full dpms interface for
> dvo. But since it's rather independant churn I've moved it into this
> odds bits series.

The full fledged dpms mode isn't going to completely disappear thanks
to the CRT dinosaur. I just wonder if we can achieve the same
simplification by recognising that all non-zero dpms modes are off, e.g.
s/dpms_mode/powersave/

if (powersave)
  switch_off()
else
  switch_on()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/7] modeset rework prep patches

[Daniel Vetter]

On Sun, Aug 12, 2012 at 09:12:14PM +0100, Chris Wilson wrote:
> On Sun, 12 Aug 2012 22:01:02 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote:
> > > >   drm/i915: simplify dvo dpms interface
> > > 
> > > This just looks like churn for churn's sake? The changes look correct.
> > 
> > We don't bother with anything else than dpms on/off states in most of the
> > modeset code (even for crt newer hw drops the intermediate states). Hence
> > the new interfaces have only enable/disable functions at the encoder/crtc
> > level. I've figured it looks odd if we keep the full dpms interface for
> > dvo. But since it's rather independant churn I've moved it into this
> > odds bits series.
> 
> The full fledged dpms mode isn't going to completely disappear thanks
> to the CRT dinosaur. I just wonder if we can achieve the same
> simplification by recognising that all non-zero dpms modes are off, e.g.
> s/dpms_mode/powersave/
> 
> if (powersave)
>   switch_off()
> else
>   switch_on()

Yeah, I'm still handling the crt dinosaur (on hw that supports it) in the
modeset rework code. I've just noticed while planning the new modeset code
that we handle these dpms codes rather inconsistently. Hence the code has
grown a simple bool at the encoder level to track the desired state, which
then controls the on/off state of the entire output pipeline (the
additional dpms states of the dinosaur are just a nuisance on top of that
- every state that requires an active pipes force the entire pipe to be
on). This has the advantage that we don't force the pipe on for dpms ==
standby/suspend just in case we have a crt on it - the new code actually
shuts it off if possible.

So with the new code the only thing still even dealing with dpms codes is
at the connector level. And even there it's just crt (and sdvo, if anyone
would fancy writing the patch to enable that ...).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/7] modeset rework prep patches

[Daniel Vetter]

On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote:
> On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hi all,
> > 
> > I've noticed that a few prep patches of the modeset rework series haven't been
> > merged nor reviewed yet, so I've split them out in this resend. Mostly concern
> > really old gen2 stuff (dvo + pipe A quirk), but little patches in other areas.
> > 
> > Comments&review highly welcome.
> > 
> > Thanks, Daniel
> > 
> > Daniel Vetter (7):
> >   drm/i915: add missing gen2 pipe A quirk entries
> 
> I remain dubious whether the 855gm entry is genuine.
> 
> >   drm/i915/ns2501: kill pll A enabling hack
> >   drm/i915: rip out the overlay pipe A workaround
> 
> Look fine and a welcome reduction in code + confusion.
> 
> >   drm/i915: prepare load-detect pipe code for dpms changes
> 
> It is not immediately obvious from the function that there is a
> relationship between the connector and intel_encoder.  If we derived the
> encoder from the connector in that function, the reviewer's life gets a
> little easier. As it stands the code looks correct and rightly removes
> some internal details.
> 
> >   drm/i915: simplify dvo dpms interface
> 
> This just looks like churn for churn's sake? The changes look correct.
> 
> >   drm/i915: kill a few unused things in dev_priv
> 
> +1
> 
> >   drm/i915: extract ironlake_fdi_pll_disable
> 
> Smaller more descriptive functions, what is not to like.
> 
> 2-7: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> You can have an r-b for 1 if you drop the 855gm chunk. Then get someone
> else to a-b the 855gm entry :)

I've slurped in the revised series, thanks a lot for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48