drm/i915: fix regression after clock gating init split

PATCH In review Jason Stubbs 3 participants 6 messages May 27, 2011 – May 31, 2011

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Jason Stubbs

author

2011-05-27 13:47 UTC #0 lore raw

PATCH 1/1 drm/i915: fix regression after clock gating init split

From: Jason Stubbs <[email protected]>

In revision 6067aaeadb5b3df26f27ac827256b1ef01e674f5, the function intel_enable_clock_gating is split up by device. drm_i915_display_funcs then gained a function pointer called init_clock_gating that intel_init_display sets to the appropriate function. However, there are some code paths, notably IS_PINEVIEW(dev), where init_clock_gating is not set and not needed. Calling it then fails. This patch fixes it by simply adding a null-pointer check.

Signed-off-by Jason Stubbs <[email protected]>

---

drivers/gpu/drm/i915/intel_display.c +2 -1

index f553ddf..6809339 100644
@@ -7559,7 +7559,8 @@ void intel_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	dev_priv->display.init_clock_gating(dev);
+	if (dev_priv->display.init_clock_gating)
+		dev_priv->display.init_clock_gating(dev);
 
 	if (dev_priv->display.init_pch_clock_gating)
 		dev_priv->display.init_pch_clock_gating(dev);

Keith Packard

2011-05-27 19:44 UTC #1 lore raw

Quoted 9 lines · Jason Stubbs

From: Jason Stubbs <[email protected]>
In revision 6067aaeadb5b3df26f27ac827256b1ef01e674f5, the function intel_enable_clock_gating is split up by device. drm_i915_display_funcs then gained a function pointer called init_clock_gating that intel_init_display sets to the appropriate function. However, there are some code paths, notably IS_PINEVIEW(dev), where init_clock_gating is not set and not needed. Calling it then fails. This patch fixes it by simply adding a null-pointer check.

Looks like Pineview should be using the gen3_init_clock_gating function.

Signature · 1 line

[email protected]

Jason Stubbs

author

2011-05-28 04:29 UTC #2 lore raw

Quoted 1 line · On Sat, 28 May 2011 05:44:11 Keith Packard wrote:

On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <[email protected]>

Quoted 6 lines · wrote:

Quoted 4 lines
From: Jason Stubbs <[email protected]>
However, there are some code paths, notably IS_PINEVIEW(dev), where init_clock_gating is not set and not needed.
Looks like Pineview should be using the gen3_init_clock_gating function.

Yep, you are right. Adding printk()s to the original implementation confirmed that - at least on my hardware. I should have known to check that...

There are two other code paths where an init_clock_gating function isn't specified though. The specific code paths are:

HAS_PCH_SPLIT() && !IS_GEN5() && !IS_GEN6() && !IS_IVYBRIDGE()

Looking at the original intel_enable_clock_gating() function, there would have been some intialization done for this case.

IS_GEN4() && !IS_CRESTLINE() && !IS_BROADWATER()

It looks like this would have gone into the final "no gating match" if/else branch.

If both of the above don't happen in the real world, then I guess it's fine as is. If either are possible, then they need to be fixed too.

I'll just (re)send a patch for the IS_PINEVIEW() case and leave the above to somebody who knows better than I...

Regards,
Jason Stubbs

Jesse Barnes

2011-05-31 16:57 UTC #3 lore raw

Quoted 23 lines · On Sat, 28 May 2011 14:26:28 +1000 Jason Stubbs <[email protected]> wrote:

Quoted 1 line · On Sat, 28 May 2011 05:44:11 Keith Packard wrote:
On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <[email protected]>
Quoted 6 lines · wrote:
Quoted 4 lines
From: Jason Stubbs <[email protected]>
However, there are some code paths, notably IS_PINEVIEW(dev), where init_clock_gating is not set and not needed.
Looks like Pineview should be using the gen3_init_clock_gating function.
Yep, you are right. Adding printk()s to the original implementation confirmed that - at least on my hardware. I should have known to check that...
There are two other code paths where an init_clock_gating function isn't specified though. The specific code paths are:
HAS_PCH_SPLIT() && !IS_GEN5() && !IS_GEN6() && !IS_IVYBRIDGE()
Looking at the original intel_enable_clock_gating() function, there would have been some intialization done for this case.
IS_GEN4() && !IS_CRESTLINE() && !IS_BROADWATER()

Quoted 8 lines

It looks like this would have gone into the final "no gating match" if/else branch.
If both of the above don't happen in the real world, then I guess it's fine as is. If either are possible, then they need to be fixed too.
I'll just (re)send a patch for the IS_PINEVIEW() case and leave the above to somebody who knows better than I...

Yeah, the above combos aren't valid, so I don't think we need to handle them.

Thanks for the Pineview fix!

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Jason Stubbs

author

2011-05-28 04:29 UTC #4 lore raw

PATCH 1/1 drm/i915: fix regression after clock gating init split

In reply to Keith Packard

From: Jason Stubbs <[email protected]>

During the refactoring in revision 6067aaeadb5b3df26f27ac827256b1ef01e674f5, the intel_enable_clock_gating was split up into several functions that are then called indirectly. However, which function to call was not specified for the IS_PINEVIEW() case. This patch specifies the correct gating function.

Signed-off-by Jason Stubbs <[email protected]>

---

drivers/gpu/drm/i915/intel_display.c +1 -0

index f553ddf..bb1b59b 100644
@@ -7675,6 +7675,7 @@ static void intel_init_display(struct drm_device *dev)
 			dev_priv->display.update_wm = NULL;
 		} else
 			dev_priv->display.update_wm = pineview_update_wm;
+		dev_priv->display.init_clock_gating = gen3_init_clock_gating;
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.update_wm = g4x_update_wm;
 		dev_priv->display.init_clock_gating = g4x_init_clock_gating;

Jesse Barnes

Reviewed-by on v1 1/1

2011-05-31 16:58 UTC #5 lore raw

Quoted 15 lines · On Sat, 28 May 2011 14:26:48 +1000 Jason Stubbs <[email protected]> wrote:

From: Jason Stubbs <[email protected]>
During the refactoring in revision 6067aaeadb5b3df26f27ac827256b1ef01e674f5, the intel_enable_clock_gating was split up into several functions that are then called indirectly. However, which function to call was not specified for the IS_PINEVIEW() case. This patch specifies the correct gating function.
Signed-off-by Jason Stubbs <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c +0 -0
index f553ddf..bb1b59b 100644

drivers/gpu/drm/i915/intel_display.c static void intel_init_display(struct drm_device *dev) · line 7681

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -7675,6 +7675,7 @@ static void intel_init_display(struct drm_device *dev)
 			dev_priv->display.update_wm = NULL;
 		} else
 			dev_priv->display.update_wm = pineview_update_wm;
+		dev_priv->display.init_clock_gating = gen3_init_clock_gating;
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.update_wm = g4x_update_wm;
 		dev_priv->display.init_clock_gating = g4x_init_clock_gating;

Reviewed-by Jesse Barnes <[email protected]>

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center