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]>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f553ddf..6809339 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
On Sat, 28 May 2011 05:44:11 Keith Packard wrote:
> On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <[email protected]>
wrote:
> > 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
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]>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f553ddf..bb1b59b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
On Sat, 28 May 2011 14:26:28 +1000
Jason Stubbs <[email protected]> wrote:
> On Sat, 28 May 2011 05:44:11 Keith Packard wrote:
> > On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <[email protected]>
> wrote:
> > > 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...
Yeah, the above combos aren't valid, so I don't think we need to handle
them.
Thanks for the Pineview fix!
--
Jesse Barnes, Intel Open Source Technology Center
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]>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f553ddf..bb1b59b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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,
--
Jesse Barnes, Intel Open Source Technology Center