[RFC] drm/i915: split display functions by chip type

Commit Message

Jesse Barnes Sept. 14, 2009, 9:51 p.m. UTC

This patch is a long time coming IMO, and there are many more cleanups
possible.  It splits out several of the display functions into a
separate display function table to avoid tons of chipset specific
if..else if..else if blocks all over.

Zhenyu, what do you think?  Should help with adding new hardware
support over time and make functionality a bit clearer.

Signed-off-by: Jesse Barnes <[e-mail]>

Comments

Keith Packard Sept. 14, 2009, 10:29 p.m. UTC | #1

On Mon, 2009-09-14 at 14:51 -0700, Jesse Barnes wrote:
> This patch is a long time coming IMO, and there are many more cleanups
> possible.  It splits out several of the display functions into a
> separate display function table to avoid tons of chipset specific
> if..else if..else if blocks all over.
> 
> Zhenyu, what do you think?  Should help with adding new hardware
> support over time and make functionality a bit clearer.

I worry that we'll end up with piles of nearly-duplicated code and
forget to replicate bug fixes across all chips.

Any idea how to balance this?

-keith

Jesse Barnes Sept. 14, 2009, 11:38 p.m. UTC | #2

On Mon, 14 Sep 2009 15:29:46 -0700
Keith Packard <[e-mail]> wrote:

> On Mon, 2009-09-14 at 14:51 -0700, Jesse Barnes wrote:
> > This patch is a long time coming IMO, and there are many more
> > cleanups possible.  It splits out several of the display functions
> > into a separate display function table to avoid tons of chipset
> > specific if..else if..else if blocks all over.
> > 
> > Zhenyu, what do you think?  Should help with adding new hardware
> > support over time and make functionality a bit clearer.
> 
> I worry that we'll end up with piles of nearly-duplicated code and
> forget to replicate bug fixes across all chips.
> 
> Any idea how to balance this?

Yeah, we should avoid having a bunch of duplicated code.  If that's
happening we haven't split things correctly, IMO.

In this set there's a little boilerplate for getting FIFO sizes (just
so we can do I915_READ etc) and the existing DPMS code duplication (not
improved or worsened with this patch).

Boilerplate like the FIFO stuff could be removed by changing the args
to the FIFO routines.

Or were you worried about something in particular?  Maybe with the
suggestions I outlined in the structure definition?

Zhenyu Wang Sept. 15, 2009, 7:22 a.m. UTC | #3

On 2009.09.14 16:38:08 -0700, Jesse Barnes wrote:
> On Mon, 14 Sep 2009 15:29:46 -0700
> Keith Packard <[e-mail]> wrote:
> 
> > On Mon, 2009-09-14 at 14:51 -0700, Jesse Barnes wrote:
> > > This patch is a long time coming IMO, and there are many more
> > > cleanups possible.  It splits out several of the display functions
> > > into a separate display function table to avoid tons of chipset
> > > specific if..else if..else if blocks all over.
> > > 
> > > Zhenyu, what do you think?  Should help with adding new hardware
> > > support over time and make functionality a bit clearer.
> > 
> > I worry that we'll end up with piles of nearly-duplicated code and
> > forget to replicate bug fixes across all chips.
> > 
> > Any idea how to balance this?
> 
> Yeah, we should avoid having a bunch of duplicated code.  If that's
> happening we haven't split things correctly, IMO.
> 
> In this set there's a little boilerplate for getting FIFO sizes (just
> so we can do I915_READ etc) and the existing DPMS code duplication (not
> improved or worsened with this patch).
> 
> Boilerplate like the FIFO stuff could be removed by changing the args
> to the FIFO routines.
> 
> Or were you worried about something in particular?  Maybe with the
> suggestions I outlined in the structure definition?
> 

You did further work than I was thinking. I'd like to see something like
intel_init_display() to initialize all features to some internal mask bits,
and your function assignment work looks also fine with me in consider of
current splitted function hooks.

Jesse Barnes Sept. 15, 2009, 3:46 p.m. UTC | #4

On Tue, 15 Sep 2009 15:22:28 +0800
Zhenyu Wang <[e-mail]> wrote:

> On 2009.09.14 16:38:08 -0700, Jesse Barnes wrote:
> > On Mon, 14 Sep 2009 15:29:46 -0700
> > Keith Packard <[e-mail]> wrote:
> > 
> > > On Mon, 2009-09-14 at 14:51 -0700, Jesse Barnes wrote:
> > > > This patch is a long time coming IMO, and there are many more
> > > > cleanups possible.  It splits out several of the display
> > > > functions into a separate display function table to avoid tons
> > > > of chipset specific if..else if..else if blocks all over.
> > > > 
> > > > Zhenyu, what do you think?  Should help with adding new hardware
> > > > support over time and make functionality a bit clearer.
> > > 
> > > I worry that we'll end up with piles of nearly-duplicated code and
> > > forget to replicate bug fixes across all chips.
> > > 
> > > Any idea how to balance this?
> > 
> > Yeah, we should avoid having a bunch of duplicated code.  If that's
> > happening we haven't split things correctly, IMO.
> > 
> > In this set there's a little boilerplate for getting FIFO sizes
> > (just so we can do I915_READ etc) and the existing DPMS code
> > duplication (not improved or worsened with this patch).
> > 
> > Boilerplate like the FIFO stuff could be removed by changing the
> > args to the FIFO routines.
> > 
> > Or were you worried about something in particular?  Maybe with the
> > suggestions I outlined in the structure definition?
> > 
> 
> You did further work than I was thinking. I'd like to see something
> like intel_init_display() to initialize all features to some internal
> mask bits, and your function assignment work looks also fine with me
> in consider of current splitted function hooks.

Yeah there are probably cases where a simple bitfield of features would
be better than a full set of separate functions.  I'm definitely not
opposed to adding that too...

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 09e9874..f1740f4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -153,6 +153,23 @@  struct drm_i915_error_state {
 	struct timeval time;
 };
 
+struct drm_i915_display_funcs {
+	void (*dpms)(struct drm_crtc *crtc, int mode);
+	bool (*fbc_enabled)(struct drm_crtc *crtc);
+	void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval);
+	void (*disable_fbc)(struct drm_device *dev);
+	int (*get_display_clock_speed)(struct drm_device *dev);
+	int (*get_fifo_size)(struct drm_device *dev, int plane);
+	void (*update_wm)(struct drm_device *dev, int planea_clock,
+			  int planeb_clock, int sr_hdisplay, int pixel_size);
+	/* clock updates for mode set */
+	/* cursor updates */
+	/* render clock increase/decrease */
+	/* display clock increase/decrease */
+	/* pll clock increase/decrease */
+	/* clock gating init */
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 
@@ -246,6 +263,9 @@  typedef struct drm_i915_private {
 	struct work_struct error_work;
 	struct workqueue_struct *wq;
 
+	/* Display functions */
+	struct drm_i915_display_funcs display;
+
 	/* Register state */
 	u8 saveLBB;
 	u32 saveDSPACNTR;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 36a36d7..d149c05 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1064,6 +1064,15 @@  static void intel_update_fbc(struct drm_crtc *crtc,
 	if (!i915_powersave)
 		return;
 
+	if (!dev_priv->display.fbc_enabled ||
+	    !dev_priv->display.enable_fbc ||
+	    !dev_priv->display.disable_fbc)
+		return;
+
+	/* Pre-965 can only compress plane 0, 965+ can do either */
+	if ((!IS_I965G(dev) && plane == 0))
+		return;
+
 	if (!crtc->fb)
 		return;
 
@@ -1101,19 +1110,19 @@  static void intel_update_fbc(struct drm_crtc *crtc,
 		goto out_disable;
 	}
 
-	if (i8xx_fbc_enabled(crtc)) {
+	if (dev_priv->display.fbc_enabled(crtc)) {
 		/* We can re-enable it in this case, but need to update pitch */
 		if (fb->pitch > dev_priv->cfb_pitch)
-			i8xx_disable_fbc(dev);
+			dev_priv->display.disable_fbc(dev);
 		if (obj_priv->fence_reg != dev_priv->cfb_fence)
-			i8xx_disable_fbc(dev);
+			dev_priv->display.disable_fbc(dev);
 		if (plane != dev_priv->cfb_plane)
-			i8xx_disable_fbc(dev);
+			dev_priv->display.disable_fbc(dev);
 	}
 
-	if (!i8xx_fbc_enabled(crtc)) {
+	if (!dev_priv->display.fbc_enabled(crtc)) {
 		/* Now try to turn it back on if possible */
-		i8xx_enable_fbc(crtc, 500);
+		dev_priv->display.enable_fbc(crtc, 500);
 	}
 
 	return;
@@ -1121,8 +1130,8 @@  static void intel_update_fbc(struct drm_crtc *crtc,
 out_disable:
 	DRM_DEBUG("unsupported config, disabling FBC\n");
 	/* Multiple disables should be harmless */
-	if (i8xx_fbc_enabled(crtc))
-		i8xx_disable_fbc(dev);
+	if (dev_priv->display.fbc_enabled(crtc))
+		dev_priv->display.disable_fbc(dev);
 }
 
 static int
@@ -1755,8 +1764,7 @@  static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 		intel_crtc_load_lut(crtc);
 
-		if (I915_HAS_FBC(dev) && (IS_I965G(dev) || plane == 0))
-			intel_update_fbc(crtc, &crtc->mode);
+		intel_update_fbc(crtc, &crtc->mode);
 
 		/* Give the overlay scaler a chance to enable if it's on this pipe */
 		//intel_crtc_dpms_video(crtc, true); TODO
@@ -1767,8 +1775,9 @@  static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
 		/* Give the overlay scaler a chance to disable if it's on this pipe */
 		//intel_crtc_dpms_video(crtc, FALSE); TODO
 
-		if (dev_priv->cfb_plane == plane)
-			i8xx_disable_fbc(dev);
+		if (dev_priv->cfb_plane == plane &&
+		    dev_priv->display.disable_fbc)
+			dev_priv->display.disable_fbc(dev);
 
 		/* Disable the VGA plane that we never use */
 		i915_disable_vga(dev);
@@ -1818,15 +1827,13 @@  static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
 static void intel_crtc_dpms(struct drm_crtc *crtc, int mode)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_master_private *master_priv;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	bool enabled;
 
-	if (IS_IGDNG(dev))
-		igdng_crtc_dpms(crtc, mode);
-	else
-		i9xx_crtc_dpms(crtc, mode);
+	dev_priv->display.dpms(crtc, mode);
 
 	intel_crtc->dpms_mode = mode;
 
@@ -1893,56 +1900,68 @@  static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
+static int i945_get_display_clock_speed(struct drm_device *dev)
+{
+	return 400000;
+}
 
-/** Returns the core display clock speed for i830 - i945 */
-static int intel_get_core_clock_speed(struct drm_device *dev)
+static int i915_get_display_clock_speed(struct drm_device *dev)
 {
+	return 333000;
+}
 
-	/* Core clock values taken from the published datasheets.
-	 * The 830 may go up to 166 Mhz, which we should check.
-	 */
-	if (IS_I945G(dev))
-		return 400000;
-	else if (IS_I915G(dev))
-		return 333000;
-	else if (IS_I945GM(dev) || IS_845G(dev) || IS_IGDGM(dev))
-		return 200000;
-	else if (IS_I915GM(dev)) {
-		u16 gcfgc = 0;
+static int i9xx_misc_get_display_clock_speed(struct drm_device *dev)
+{
+	return 200000;
+}
 
-		pci_read_config_word(dev->pdev, GCFGC, &gcfgc);
+static int i915gm_get_display_clock_speed(struct drm_device *dev)
+{
+	u16 gcfgc = 0;
 
-		if (gcfgc & GC_LOW_FREQUENCY_ENABLE)
-			return 133000;
-		else {
-			switch (gcfgc & GC_DISPLAY_CLOCK_MASK) {
-			case GC_DISPLAY_CLOCK_333_MHZ:
-				return 333000;
-			default:
-			case GC_DISPLAY_CLOCK_190_200_MHZ:
-				return 190000;
-			}
-		}
-	} else if (IS_I865G(dev))
-		return 266000;
-	else if (IS_I855(dev)) {
-		u16 hpllcc = 0;
-		/* Assume that the hardware is in the high speed state.  This
-		 * should be the default.
-		 */
-		switch (hpllcc & GC_CLOCK_CONTROL_MASK) {
-		case GC_CLOCK_133_200:
-		case GC_CLOCK_100_200:
-			return 200000;
-		case GC_CLOCK_166_250:
-			return 250000;
-		case GC_CLOCK_100_133:
-			return 133000;
+	pci_read_config_word(dev->pdev, GCFGC, &gcfgc);
+
+	if (gcfgc & GC_LOW_FREQUENCY_ENABLE)
+		return 133000;
+	else {
+		switch (gcfgc & GC_DISPLAY_CLOCK_MASK) {
+		case GC_DISPLAY_CLOCK_333_MHZ:
+			return 333000;
+		default:
+		case GC_DISPLAY_CLOCK_190_200_MHZ:
+			return 190000;
 		}
-	} else /* 852, 830 */
+	}
+}
+
+static int i865_get_display_clock_speed(struct drm_device *dev)
+{
+	return 266000;
+}
+
+static int i855_get_display_clock_speed(struct drm_device *dev)
+{
+	u16 hpllcc = 0;
+	/* Assume that the hardware is in the high speed state.  This
+	 * should be the default.
+	 */
+	switch (hpllcc & GC_CLOCK_CONTROL_MASK) {
+	case GC_CLOCK_133_200:
+	case GC_CLOCK_100_200:
+		return 200000;
+	case GC_CLOCK_166_250:
+		return 250000;
+	case GC_CLOCK_100_133:
 		return 133000;
+	}
 
-	return 0; /* Silence gcc warning */
+	/* Shouldn't happen */
+	return 0;
+}
+
+static int i830_get_display_clock_speed(struct drm_device *dev)
+{
+	return 133000;
 }
 
 /**
@@ -2268,32 +2287,17 @@  static void igd_enable_cxsr(struct drm_device *dev, unsigned long clock,
  */
 const static int latency_ns = 5000;
 
-static int intel_get_fifo_size(struct drm_device *dev, int plane)
+static int i9xx_get_fifo_size(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t dsparb = I915_READ(DSPARB);
 	int size;
 
-	if (IS_I9XX(dev)) {
-		if (plane == 0)
-			size = dsparb & 0x7f;
-		else
-			size = ((dsparb >> DSPARB_CSTART_SHIFT) & 0x7f) -
-				(dsparb & 0x7f);
-	} else if (IS_I85X(dev)) {
-		if (plane == 0)
-			size = dsparb & 0x1ff;
-		else
-			size = ((dsparb >> DSPARB_BEND_SHIFT) & 0x1ff) -
-				(dsparb & 0x1ff);
-		size >>= 1; /* Convert to cachelines */
-	} else if (IS_845G(dev)) {
-		size = dsparb & 0x7f;
-		size >>= 2; /* Convert to cachelines */
-	} else {
+	if (plane == 0)
 		size = dsparb & 0x7f;
-		size >>= 1; /* Convert to cachelines */
-	}
+	else
+		size = ((dsparb >> DSPARB_CSTART_SHIFT) & 0x7f) -
+			(dsparb & 0x7f);
 
 	DRM_DEBUG("FIFO size - (0x%08x) %s: %d\n", dsparb, plane ? "B" : "A",
 		  size);
@@ -2301,7 +2305,57 @@  static int intel_get_fifo_size(struct drm_device *dev, int plane)
 	return size;
 }
 
-static void g4x_update_wm(struct drm_device *dev)
+static int i85x_get_fifo_size(struct drm_device *dev, int plane)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t dsparb = I915_READ(DSPARB);
+	int size;
+
+	if (plane == 0)
+		size = dsparb & 0x1ff;
+	else
+		size = ((dsparb >> DSPARB_BEND_SHIFT) & 0x1ff) -
+			(dsparb & 0x1ff);
+	size >>= 1; /* Convert to cachelines */
+
+	DRM_DEBUG("FIFO size - (0x%08x) %s: %d\n", dsparb, plane ? "B" : "A",
+		  size);
+
+	return size;
+}
+
+static int i845_get_fifo_size(struct drm_device *dev, int plane)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t dsparb = I915_READ(DSPARB);
+	int size;
+
+	size = dsparb & 0x7f;
+	size >>= 2; /* Convert to cachelines */
+
+	DRM_DEBUG("FIFO size - (0x%08x) %s: %d\n", dsparb, plane ? "B" : "A",
+		  size);
+
+	return size;
+}
+
+static int i830_get_fifo_size(struct drm_device *dev, int plane)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t dsparb = I915_READ(DSPARB);
+	int size;
+
+	size = dsparb & 0x7f;
+	size >>= 1; /* Convert to cachelines */
+
+	DRM_DEBUG("FIFO size - (0x%08x) %s: %d\n", dsparb, plane ? "B" : "A",
+		  size);
+
+	return size;
+}
+
+static void g4x_update_wm(struct drm_device *dev, int unused, int unused2,
+			  int unused3, int unused4)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 fw_blc_self = I915_READ(FW_BLC_SELF);
@@ -2313,7 +2367,8 @@  static void g4x_update_wm(struct drm_device *dev)
 	I915_WRITE(FW_BLC_SELF, fw_blc_self);
 }
 
-static void i965_update_wm(struct drm_device *dev)
+static void i965_update_wm(struct drm_device *dev, int unused, int unused2,
+			   int unused3, int unused4)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2349,8 +2404,8 @@  static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	cacheline_size = planea_params.cacheline_size;
 
 	/* Update per-plane FIFO sizes */
-	planea_params.fifo_size = intel_get_fifo_size(dev, 0);
-	planeb_params.fifo_size = intel_get_fifo_size(dev, 1);
+	planea_params.fifo_size = dev_priv->display.get_fifo_size(dev, 0);
+	planeb_params.fifo_size = dev_priv->display.get_fifo_size(dev, 1);
 
 	planea_wm = intel_calculate_wm(planea_clock, &planea_params,
 				       pixel_size, latency_ns);
@@ -2397,14 +2452,14 @@  static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	I915_WRITE(FW_BLC2, fwater_hi);
 }
 
-static void i830_update_wm(struct drm_device *dev, int planea_clock,
-			   int pixel_size)
+static void i830_update_wm(struct drm_device *dev, int planea_clock, int unused,
+			   int unused2, int pixel_size)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t fwater_lo = I915_READ(FW_BLC) & ~0xfff;
 	int planea_wm;
 
-	i830_wm_info.fifo_size = intel_get_fifo_size(dev, 0);
+	i830_wm_info.fifo_size = dev_priv->display.get_fifo_size(dev, 0);
 
 	planea_wm = intel_calculate_wm(planea_clock, &i830_wm_info,
 				       pixel_size, latency_ns);
@@ -2448,6 +2503,7 @@  static void i830_update_wm(struct drm_device *dev, int planea_clock,
   */
 static void intel_update_watermarks(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
 	int sr_hdisplay = 0;
@@ -2486,15 +2542,8 @@  static void intel_update_watermarks(struct drm_device *dev)
 	else if (IS_IGD(dev))
 		igd_disable_cxsr(dev);
 
-	if (IS_G4X(dev))
-		g4x_update_wm(dev);
-	else if (IS_I965G(dev))
-		i965_update_wm(dev);
-	else if (IS_I9XX(dev) || IS_MOBILE(dev))
-		i9xx_update_wm(dev, planea_clock, planeb_clock, sr_hdisplay,
-			       pixel_size);
-	else
-		i830_update_wm(dev, planea_clock, pixel_size);
+	dev_priv->display.update_wm(dev, planea_clock, planeb_clock,
+				    sr_hdisplay, pixel_size);
 }
 
 static int intel_crtc_mode_set(struct drm_crtc *crtc,
@@ -2765,7 +2814,8 @@  static int intel_crtc_mode_set(struct drm_crtc *crtc,
 		 * XXX: No double-wide on 915GM pipe B. Is that the only reason for the
 		 * pipe == 0 check?
 		 */
-		if (mode->clock > intel_get_core_clock_speed(dev) * 9 / 10)
+		if (mode->clock >
+		    dev_priv->display.get_display_clock_speed(dev) * 9 / 10)
 			pipeconf |= PIPEACONF_DOUBLE_WIDE;
 		else
 			pipeconf &= ~PIPEACONF_DOUBLE_WIDE;
@@ -2922,8 +2972,8 @@  static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	/* Flush the plane changes */
 	ret = intel_pipe_set_base(crtc, x, y, old_fb);
 
-	if (I915_HAS_FBC(dev) && (IS_I965G(dev) || plane == 0))
-		intel_update_fbc(crtc, &crtc->mode);
+	intel_update_fbc(crtc, &crtc->mode);
+
 	intel_update_watermarks(dev);
 
 	drm_vblank_post_modeset(dev, pipe);
@@ -4029,6 +4079,69 @@  void intel_init_clock_gating(struct drm_device *dev)
 	}
 }
 
+/* Set up chip specific display functions */
+static void intel_init_display(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* We always want a DPMS function */
+	if (IS_IGDNG(dev))
+		dev_priv->display.dpms = igdng_crtc_dpms;
+	else
+		dev_priv->display.dpms = i9xx_crtc_dpms;
+
+	/* Only mobile has FBC, leave pointers NULL for other chips */
+	if (IS_MOBILE(dev)) {
+		/* 855GM needs testing */
+		if (IS_I965GM(dev) || IS_I945GM(dev) || IS_I915GM(dev)) {
+			dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
+			dev_priv->display.enable_fbc = i8xx_enable_fbc;
+			dev_priv->display.disable_fbc = i8xx_disable_fbc;
+		}
+	}
+
+	/* Returns the core display clock speed */
+	if (IS_I945G(dev))
+		dev_priv->display.get_display_clock_speed =
+			i945_get_display_clock_speed;
+	else if (IS_I915G(dev))
+		dev_priv->display.get_display_clock_speed =
+			i915_get_display_clock_speed;
+	else if (IS_I945GM(dev) || IS_845G(dev) || IS_IGDGM(dev))
+		dev_priv->display.get_display_clock_speed =
+			i9xx_misc_get_display_clock_speed;
+	else if (IS_I915GM(dev))
+		dev_priv->display.get_display_clock_speed =
+			i915gm_get_display_clock_speed;
+	else if (IS_I865G(dev))
+		dev_priv->display.get_display_clock_speed =
+			i865_get_display_clock_speed;
+	else if (IS_I855(dev))
+		dev_priv->display.get_display_clock_speed =
+			i855_get_display_clock_speed;
+	else /* 852, 830 */
+		dev_priv->display.get_display_clock_speed =
+			i830_get_display_clock_speed;
+
+	/* For FIFO watermark updates */
+	if (IS_G4X(dev))
+		dev_priv->display.update_wm = g4x_update_wm;
+	else if (IS_I965G(dev))
+		dev_priv->display.update_wm = i965_update_wm;
+	else if (IS_I9XX(dev) || IS_MOBILE(dev)) {
+		dev_priv->display.update_wm = i9xx_update_wm;
+		dev_priv->display.get_fifo_size = i9xx_get_fifo_size;
+	} else {
+		if (IS_I85X(dev))
+			dev_priv->display.get_fifo_size = i85x_get_fifo_size;
+		else if (IS_845G(dev))
+			dev_priv->display.get_fifo_size = i845_get_fifo_size;
+		else
+			dev_priv->display.get_fifo_size = i830_get_fifo_size;
+		dev_priv->display.update_wm = i830_update_wm;
+	}
+}
+
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4042,6 +4155,8 @@  void intel_modeset_init(struct drm_device *dev)
 
 	dev->mode_config.funcs = (void *)&intel_mode_funcs;
 
+	intel_init_display(dev);
+
 	if (IS_I965G(dev)) {
 		dev->mode_config.max_width = 8192;
 		dev->mode_config.max_height = 8192;
@@ -4107,7 +4222,9 @@  void intel_modeset_cleanup(struct drm_device *dev)
 
 	mutex_unlock(&dev->struct_mutex);
 
-	i8xx_disable_fbc(dev);
+	if (dev_priv->display.disable_fbc)
+		dev_priv->display.disable_fbc(dev);
+
 	drm_mode_config_cleanup(dev);
 }