[V2] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

Commit Message

Wolfram Sang June 20, 2011, 5:36 p.m. UTC

Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) introduces an init_clock_gating-pointer. There is one case,
however, where it does not get set, so that caused an OOPS. Change the
code to return -ENODEV in this case and propagate it to the upper
layers.

Signed-off-by: Wolfram Sang <[e-mail]>
Cc: Jesse Barnes <[e-mail]>
Cc: Chris Wilson <[e-mail]>
---

Change since V1: changed error-message per Chris' suggestion.

Note: used %04x instead of %lx to fix compile warning.

 drivers/gpu/drm/i915/i915_dma.c      |    4 +++-
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++------
 3 files changed, 18 insertions(+), 8 deletions(-)

Comments

Jesse Barnes June 20, 2011, 5:38 p.m. UTC | #1

On Mon, 20 Jun 2011 19:36:11 +0200
Wolfram Sang <[e-mail]> wrote:

> Commit 6067aa (drm/i915: split clock gating init into per-chipset
> functions) introduces an init_clock_gating-pointer. There is one case,
> however, where it does not get set, so that caused an OOPS. Change the
> code to return -ENODEV in this case and propagate it to the upper
> layers.
> 
> Signed-off-by: Wolfram Sang <[e-mail]>
> Cc: Jesse Barnes <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> ---

Looks good, thanks Wolfram.

Reviewed-by: Jesse Barnes <[e-mail]>

Wolfram Sang July 2, 2011, 3:55 p.m. UTC | #2

On Mon, Jun 20, 2011 at 10:38:54AM -0700, Jesse Barnes wrote:
> On Mon, 20 Jun 2011 19:36:11 +0200
> Wolfram Sang <[e-mail]> wrote:
> 
> > Commit 6067aa (drm/i915: split clock gating init into per-chipset
> > functions) introduces an init_clock_gating-pointer. There is one case,
> > however, where it does not get set, so that caused an OOPS. Change the
> > code to return -ENODEV in this case and propagate it to the upper
> > layers.
> > 
> > Signed-off-by: Wolfram Sang <[e-mail]>
> > Cc: Jesse Barnes <[e-mail]>
> > Cc: Chris Wilson <[e-mail]>
> > ---
> 
> Looks good, thanks Wolfram.
> 
> Reviewed-by: Jesse Barnes <[e-mail]>

Just wondering: I don't see it in linux-next or the i915-branches I am aware
of. Forgotten or intentional?

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0239e99..ace1df2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1258,7 +1258,9 @@  static int i915_load_modeset_init(struct drm_device *dev)
 	if (IS_GEN3(dev) && (I915_READ(ECOSKPD) & ECO_FLIP_DONE))
 		dev_priv->flip_pending_is_done = true;
 
-	intel_modeset_init(dev);
+	ret = intel_modeset_init(dev);
+	if (ret)
+		goto cleanup_vga_switcheroo;
 
 	ret = i915_load_gem_init(dev);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f63ee16..90ce405 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1303,7 +1303,7 @@  static inline void intel_unregister_dsm_handler(void) { return; }
 #endif /* CONFIG_ACPI */
 
 /* modesetting */
-extern void intel_modeset_init(struct drm_device *dev);
+extern int intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..57a7ba7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7518,7 +7518,7 @@  void intel_init_clock_gating(struct drm_device *dev)
 }
 
 /* Set up chip specific display functions */
-static void intel_init_display(struct drm_device *dev)
+static int intel_init_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -7610,8 +7610,10 @@  static void intel_init_display(struct drm_device *dev)
 			}
 			dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
 
-		} else
-			dev_priv->display.update_wm = NULL;
+		} else {
+			DRM_ERROR("Unknown chipset: %04x\n", dev->pci_device);
+			return -ENODEV;
+		}
 	} else if (IS_PINEVIEW(dev)) {
 		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
 					    dev_priv->is_ddr3,
@@ -7657,6 +7659,8 @@  static void intel_init_display(struct drm_device *dev)
 		else
 			dev_priv->display.get_fifo_size = i830_get_fifo_size;
 	}
+
+	return 0;
 }
 
 /*
@@ -7742,10 +7746,10 @@  static void i915_disable_vga(struct drm_device *dev)
 	POSTING_READ(vga_reg);
 }
 
-void intel_modeset_init(struct drm_device *dev)
+int intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int i;
+	int i, ret;
 
 	drm_mode_config_init(dev);
 
@@ -7756,7 +7760,9 @@  void intel_modeset_init(struct drm_device *dev)
 
 	intel_init_quirks(dev);
 
-	intel_init_display(dev);
+	ret = intel_init_display(dev);
+	if (ret)
+		return ret;
 
 	if (IS_GEN2(dev)) {
 		dev->mode_config.max_width = 2048;
@@ -7794,6 +7800,8 @@  void intel_modeset_init(struct drm_device *dev)
 	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
 	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
 		    (unsigned long)dev);
+
+	return 0;
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)