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

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

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

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 <jasonbstubbs@gmail.com>
---
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);

Re: [PATCH] drm/i915: fix regression after clock gating init split

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <jasonbstubbs@gmail.com> wrote:
> From: Jason Stubbs <jasonbstubbs@gmail.com>
> 
> 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.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] drm/i915: fix regression after clock gating init split

[Jason Stubbs]

On Sat, 28 May 2011 05:44:11 Keith Packard wrote:
> On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <jasonbstubbs@gmail.com> 
wrote:
> > From: Jason Stubbs <jasonbstubbs@gmail.com>
> > 
> > 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

Re: [PATCH] drm/i915: fix regression after clock gating init split

[Jesse Barnes]

On Sat, 28 May 2011 14:26:28 +1000
Jason Stubbs <jasonbstubbs@gmail.com> wrote:

> On Sat, 28 May 2011 05:44:11 Keith Packard wrote:
> > On Fri, 27 May 2011 23:44:19 +1000, Jason Stubbs <jasonbstubbs@gmail.com> 
> wrote:
> > > From: Jason Stubbs <jasonbstubbs@gmail.com>
> > > 
> > > 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

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

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

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 <jasonbstubbs@gmail.com>
---
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;

Re: [PATCH] drm/i915: fix regression after clock gating init split

[Jesse Barnes]

On Sat, 28 May 2011 14:26:48 +1000
Jason Stubbs <jasonbstubbs@gmail.com> wrote:

> From: Jason Stubbs <jasonbstubbs@gmail.com>
> 
> 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 <jasonbstubbs@gmail.com>
> ---
> 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 <jbarnes@virtuousgeek.org>

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center