(no subject)

(no subject)

[Jesse Barnes]

These are some prep patches I'd like to get feedback on.  I've only
compile tested them so far (the actual hw support code this is for was
tested before the split), so testing would be appreciated as well.

Thanks,
Jesse

[PATCH 1/3] drm/i915: make FDI training a display function

[Jesse Barnes]

Rather than branching in ironlake_pch_enable, add a new train_fdi
function to the display function pointer struct and use it instead.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..b4116ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -203,6 +203,7 @@ struct drm_i915_display_funcs {
 	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);
+	void (*train_fdi)(struct drm_crtc *crtc);
 	/* clock updates for mode set */
 	/* cursor updates */
 	/* render clock increase/decrease */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 432fc04..9055eff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2757,10 +2757,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	u32 reg, temp;
 
 	/* For PCH output, training FDI link */
-	if (IS_GEN6(dev))
-		gen6_fdi_link_train(crtc);
-	else
-		ironlake_fdi_link_train(crtc);
+	dev_priv->display.train_fdi(crtc);
 
 	intel_enable_pch_pll(dev_priv, pipe);
 
@@ -7270,6 +7267,7 @@ static void intel_init_display(struct drm_device *dev)
 					      "Disable CxSR\n");
 				dev_priv->display.update_wm = NULL;
 			}
+			dev_priv->display.train_fdi = ironlake_fdi_link_train;
 		} else if (IS_GEN6(dev)) {
 			if (SNB_READ_WM0_LATENCY()) {
 				dev_priv->display.update_wm = sandybridge_update_wm;
@@ -7278,6 +7276,7 @@ static void intel_init_display(struct drm_device *dev)
 					      "Disable CxSR\n");
 				dev_priv->display.update_wm = NULL;
 			}
+			dev_priv->display.train_fdi = gen6_fdi_link_train;
 		} else
 			dev_priv->display.update_wm = NULL;
 	} else if (IS_PINEVIEW(dev)) {
-- 
1.7.1

[PATCH 2/3] drm/i915: split irq handling into per-chipset functions

[Jesse Barnes]

Set the IRQ handling functions in driver load so they'll just be used
directly, rather than branching over most of the code in the chipset
functions.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c |   12 ++++++++++
 drivers/gpu/drm/i915/i915_drv.h |    6 +++++
 drivers/gpu/drm/i915/i915_irq.c |   45 +++++++++++++++++++++-----------------
 3 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..38e62bd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1253,6 +1253,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_modeset_init(dev);
 
+	if (HAS_PCH_SPLIT(dev)) {
+		dev->driver->irq_handler = ironlake_irq_handler;
+		dev->driver->irq_preinstall = ironlake_irq_preinstall;
+		dev->driver->irq_postinstall = ironlake_irq_postinstall;
+		dev->driver->irq_uninstall = ironlake_irq_uninstall;
+	} else {
+		dev->driver->irq_preinstall = i915_driver_irq_preinstall;
+		dev->driver->irq_postinstall = i915_driver_irq_postinstall;
+		dev->driver->irq_uninstall = i915_driver_irq_uninstall;
+		dev->driver->irq_handler = i915_driver_irq_handler;
+	}
+
 	ret = drm_irq_install(dev);
 	if (ret)
 		goto cleanup_vga_switcheroo;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b4116ae..313202f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1011,6 +1011,12 @@ extern irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS);
 extern void i915_driver_irq_preinstall(struct drm_device * dev);
 extern int i915_driver_irq_postinstall(struct drm_device *dev);
 extern void i915_driver_irq_uninstall(struct drm_device * dev);
+
+extern irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS);
+extern void ironlake_irq_preinstall(struct drm_device *dev);
+extern int ironlake_irq_postinstall(struct drm_device *dev);
+extern void ironlake_irq_uninstall(struct drm_device *dev);
+
 extern int i915_vblank_pipe_set(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
 extern int i915_vblank_pipe_get(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..342dd7a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -448,8 +448,9 @@ static void pch_irq_handler(struct drm_device *dev)
 		DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
 }
 
-static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
+irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 {
+	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int ret = IRQ_NONE;
 	u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
@@ -457,6 +458,8 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
 	struct drm_i915_master_private *master_priv;
 	u32 bsd_usr_interrupt = GT_BSD_USER_INTERRUPT;
 
+	atomic_inc(&dev_priv->irq_received);
+
 	if (IS_GEN6(dev))
 		bsd_usr_interrupt = GT_GEN6_BSD_USER_INTERRUPT;
 
@@ -1103,9 +1106,6 @@ irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
 
 	atomic_inc(&dev_priv->irq_received);
 
-	if (HAS_PCH_SPLIT(dev))
-		return ironlake_irq_handler(dev);
-
 	iir = I915_READ(IIR);
 
 	if (INTEL_INFO(dev)->gen >= 4)
@@ -1562,10 +1562,15 @@ repeat:
 
 /* drm_dma.h hooks
 */
-static void ironlake_irq_preinstall(struct drm_device *dev)
+void ironlake_irq_preinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 
+	atomic_set(&dev_priv->irq_received, 0);
+
+	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
+	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
+
 	I915_WRITE(HWSTAM, 0xeffe);
 
 	/* XXX hotplug from PCH */
@@ -1585,7 +1590,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
-static int ironlake_irq_postinstall(struct drm_device *dev)
+int ironlake_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	/* enable kind of interrupts always enabled */
@@ -1594,6 +1599,13 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 	u32 render_irqs;
 	u32 hotplug_mask;
 
+	DRM_INIT_WAITQUEUE(&dev_priv->ring[RCS].irq_queue);
+	if (HAS_BSD(dev))
+		DRM_INIT_WAITQUEUE(&dev_priv->ring[VCS].irq_queue);
+	if (HAS_BLT(dev))
+		DRM_INIT_WAITQUEUE(&dev_priv->ring[BCS].irq_queue);
+
+	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
 	dev_priv->irq_mask = ~display_mask;
 
 	/* should always can generate irq */
@@ -1660,11 +1672,6 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
 	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
 	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
 
-	if (HAS_PCH_SPLIT(dev)) {
-		ironlake_irq_preinstall(dev);
-		return;
-	}
-
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -1696,9 +1703,6 @@ int i915_driver_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
 
-	if (HAS_PCH_SPLIT(dev))
-		return ironlake_irq_postinstall(dev);
-
 	/* Unmask the interrupts that we always want on. */
 	dev_priv->irq_mask = ~I915_INTERRUPT_ENABLE_FIX;
 
@@ -1767,9 +1771,15 @@ int i915_driver_irq_postinstall(struct drm_device *dev)
 	return 0;
 }
 
-static void ironlake_irq_uninstall(struct drm_device *dev)
+void ironlake_irq_uninstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
+	if (!dev_priv)
+		return;
+
+	dev_priv->vblank_pipe = 0;
+
 	I915_WRITE(HWSTAM, 0xffffffff);
 
 	I915_WRITE(DEIMR, 0xffffffff);
@@ -1791,11 +1801,6 @@ void i915_driver_irq_uninstall(struct drm_device * dev)
 
 	dev_priv->vblank_pipe = 0;
 
-	if (HAS_PCH_SPLIT(dev)) {
-		ironlake_irq_uninstall(dev);
-		return;
-	}
-
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-- 
1.7.1

[PATCH 3/3] drm/i915: split enable/disable vblank code into chipset specific functions

[Jesse Barnes]

This makes the Ironlake+ code trivial and generally simplifies things.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c |    4 +++
 drivers/gpu/drm/i915/i915_drv.h |    2 +
 drivers/gpu/drm/i915/i915_irq.c |   42 ++++++++++++++++++++++++++++----------
 3 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 38e62bd..a369634 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1258,11 +1258,15 @@ static int i915_load_modeset_init(struct drm_device *dev)
 		dev->driver->irq_preinstall = ironlake_irq_preinstall;
 		dev->driver->irq_postinstall = ironlake_irq_postinstall;
 		dev->driver->irq_uninstall = ironlake_irq_uninstall;
+		dev->driver->enable_vblank = ironlake_enable_vblank;
+		dev->driver->disable_vblank = ironlake_disable_vblank;
 	} else {
 		dev->driver->irq_preinstall = i915_driver_irq_preinstall;
 		dev->driver->irq_postinstall = i915_driver_irq_postinstall;
 		dev->driver->irq_uninstall = i915_driver_irq_uninstall;
 		dev->driver->irq_handler = i915_driver_irq_handler;
+		dev->driver->enable_vblank = i915_enable_vblank;
+		dev->driver->disable_vblank = i915_disable_vblank;
 	}
 
 	ret = drm_irq_install(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 313202f..7d4fafc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1023,6 +1023,8 @@ extern int i915_vblank_pipe_get(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
 extern int i915_enable_vblank(struct drm_device *dev, int crtc);
 extern void i915_disable_vblank(struct drm_device *dev, int crtc);
+extern int ironlake_enable_vblank(struct drm_device *dev, int crtc);
+extern void ironlake_disable_vblank(struct drm_device *dev, int crtc);
 extern u32 i915_get_vblank_counter(struct drm_device *dev, int crtc);
 extern u32 gm45_get_vblank_counter(struct drm_device *dev, int crtc);
 extern int i915_vblank_swap(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 342dd7a..97c6f38 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1344,10 +1344,7 @@ int i915_enable_vblank(struct drm_device *dev, int pipe)
 		return -EINVAL;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	if (HAS_PCH_SPLIT(dev))
-		ironlake_enable_display_irq(dev_priv, (pipe == 0) ?
-					    DE_PIPEA_VBLANK: DE_PIPEB_VBLANK);
-	else if (INTEL_INFO(dev)->gen >= 4)
+	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, pipe,
 				     PIPE_START_VBLANK_INTERRUPT_ENABLE);
 	else
@@ -1362,6 +1359,22 @@ int i915_enable_vblank(struct drm_device *dev, int pipe)
 	return 0;
 }
 
+int ironlake_enable_vblank(struct drm_device *dev, int pipe)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	unsigned long irqflags;
+
+	if (!i915_pipe_enabled(dev, pipe))
+		return -EINVAL;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	ironlake_enable_display_irq(dev_priv, (pipe == 0) ?
+				    DE_PIPEA_VBLANK: DE_PIPEB_VBLANK);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	return 0;
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -1375,13 +1388,20 @@ void i915_disable_vblank(struct drm_device *dev, int pipe)
 		I915_WRITE(INSTPM,
 			   INSTPM_AGPBUSY_DIS << 16 | INSTPM_AGPBUSY_DIS);
 
-	if (HAS_PCH_SPLIT(dev))
-		ironlake_disable_display_irq(dev_priv, (pipe == 0) ?
-					     DE_PIPEA_VBLANK: DE_PIPEB_VBLANK);
-	else
-		i915_disable_pipestat(dev_priv, pipe,
-				      PIPE_VBLANK_INTERRUPT_ENABLE |
-				      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+	i915_disable_pipestat(dev_priv, pipe,
+			      PIPE_VBLANK_INTERRUPT_ENABLE |
+			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+void ironlake_disable_vblank(struct drm_device *dev, int pipe)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	ironlake_disable_display_irq(dev_priv, (pipe == 0) ?
+				     DE_PIPEA_VBLANK: DE_PIPEB_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-- 
1.7.1

Re: [PATCH 2/3] drm/i915: split irq handling into per-chipset functions

[Chris Wilson]

On Thu,  7 Apr 2011 14:32:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Set the IRQ handling functions in driver load so they'll just be used
> directly, rather than branching over most of the code in the chipset
> functions.

This is the direction we definitely need to go in. However, it is still a
tangled mess of which functions are called for which chipset.

Is it any clearer to have a display vfunc table for each chipset? It would
still be a mess, but at least there will be an overview of how each chipset
works in a single spot. Invaluable for tracing through the function
pointers later.

One thing we need to be careful is to move the common code into small
helper routines to avoid unnecessarily duplicating it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: split irq handling into per-chipset functions

[Jesse Barnes]

On Thu, 07 Apr 2011 22:50:42 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu,  7 Apr 2011 14:32:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Set the IRQ handling functions in driver load so they'll just be used
> > directly, rather than branching over most of the code in the chipset
> > functions.
> 
> This is the direction we definitely need to go in. However, it is still a
> tangled mess of which functions are called for which chipset.
> 
> Is it any clearer to have a display vfunc table for each chipset? It would
> still be a mess, but at least there will be an overview of how each chipset
> works in a single spot. Invaluable for tracing through the function
> pointers later.

Yeah, initializing it all in one place would help, but the existing
KMS/non-KMS split makes that hard for things like IRQ handling.

> One thing we need to be careful is to move the common code into small
> helper routines to avoid unnecessarily duplicating it.

But not before we're sure about the duplication!  Obviously things like
the workqueue init at IRQ install time could be shared, but I don't
like the idea of sharing hardware code unless it's absolutely
identical, given our history.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: split irq handling into per-chipset functions

[Chris Wilson]

On Thu, 7 Apr 2011 15:04:14 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 07 Apr 2011 22:50:42 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > One thing we need to be careful is to move the common code into small
> > helper routines to avoid unnecessarily duplicating it.
> 
> But not before we're sure about the duplication!  Obviously things like
> the workqueue init at IRQ install time could be shared,

Actually that workqueue init is in the wrong place entirely, but that
was the sort of generic code I was looking at. ;-)

> but I don't
> like the idea of sharing hardware code unless it's absolutely
> identical, given our history.

Agreed, I think the model you've proposed of doing bring-up on a separate
copy of the code and then gradually merge back the duplicate portions
carries the least risk. Also looking at our history, we carry a lot of
bugs that we only notice when looking at the next chipset iteration...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: make FDI training a display function

[Ben Widawsky]

On Thu, Apr 07, 2011 at 02:32:58PM -0700, Jesse Barnes wrote:
> Rather than branching in ironlake_pch_enable, add a new train_fdi
> function to the display function pointer struct and use it instead.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |    7 +++----
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..b4116ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -203,6 +203,7 @@ struct drm_i915_display_funcs {
>  	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);
> +	void (*train_fdi)(struct drm_crtc *crtc);
>  	/* clock updates for mode set */
>  	/* cursor updates */
>  	/* render clock increase/decrease */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 432fc04..9055eff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2757,10 +2757,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  	u32 reg, temp;
>  
>  	/* For PCH output, training FDI link */
> -	if (IS_GEN6(dev))
> -		gen6_fdi_link_train(crtc);
> -	else
> -		ironlake_fdi_link_train(crtc);
> +	dev_priv->display.train_fdi(crtc);
>  
>  	intel_enable_pch_pll(dev_priv, pipe);
>  
> @@ -7270,6 +7267,7 @@ static void intel_init_display(struct drm_device *dev)
>  					      "Disable CxSR\n");
>  				dev_priv->display.update_wm = NULL;
>  			}
> +			dev_priv->display.train_fdi = ironlake_fdi_link_train;
>  		} else if (IS_GEN6(dev)) {
>  			if (SNB_READ_WM0_LATENCY()) {
>  				dev_priv->display.update_wm = sandybridge_update_wm;
> @@ -7278,6 +7276,7 @@ static void intel_init_display(struct drm_device *dev)
>  					      "Disable CxSR\n");
>  				dev_priv->display.update_wm = NULL;
>  			}
> +			dev_priv->display.train_fdi = gen6_fdi_link_train;
>  		} else
>  			dev_priv->display.update_wm = NULL;
>  	} else if (IS_PINEVIEW(dev)) {

I prefer when the function pointer is named similarly to the function.
Makes it easier to read/find code.

(*fdi_link_train)(struct drm_crtc *crtc);
OR
(*link_train)(struct drm_crtc *crtc);

Re: [PATCH 1/3] drm/i915: make FDI training a display function

[Jesse Barnes]

On Wed, 20 Apr 2011 07:45:08 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:
> > @@ -7270,6 +7267,7 @@ static void intel_init_display(struct drm_device *dev)
> >  					      "Disable CxSR\n");
> >  				dev_priv->display.update_wm = NULL;
> >  			}
> > +			dev_priv->display.train_fdi = ironlake_fdi_link_train;
> >  		} else if (IS_GEN6(dev)) {
> >  			if (SNB_READ_WM0_LATENCY()) {
> >  				dev_priv->display.update_wm = sandybridge_update_wm;
> > @@ -7278,6 +7276,7 @@ static void intel_init_display(struct drm_device *dev)
> >  					      "Disable CxSR\n");
> >  				dev_priv->display.update_wm = NULL;
> >  			}
> > +			dev_priv->display.train_fdi = gen6_fdi_link_train;
> >  		} else
> >  			dev_priv->display.update_wm = NULL;
> >  	} else if (IS_PINEVIEW(dev)) {
> 
> I prefer when the function pointer is named similarly to the function.
> Makes it easier to read/find code.
> 
> (*fdi_link_train)(struct drm_crtc *crtc);
> OR
> (*link_train)(struct drm_crtc *crtc);

Yeah, fdi_link_train is probably a better name.  Thanks for checking it
out.

-- 
Jesse Barnes, Intel Open Source Technology Center