Re: Oops in i915 intel_init_clock_gating

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

The problem of dev_priv->display.init_clock_gating not getting set is 
still present in 3.0-rc3.  On my system this happens because 
intel_init_display() never gets called in the first place.

AFAICT, the normal calling sequence during driver initialization is:

	i915_driver_load() -> i915_load_modeset_init() ->
		intel_modeset_init() -> intel_init_display().

But in my case the call to i915_load_modeset_init() doesn't occur 
because drm_core_check_feature(dev, DRIVER_MODESET) is False.

I tested with the following patch:


Index: usb-3.0/drivers/gpu/drm/i915/intel_display.c
===================================================================
--- usb-3.0.orig/drivers/gpu/drm/i915/intel_display.c
+++ usb-3.0/drivers/gpu/drm/i915/intel_display.c
@@ -7511,6 +7511,10 @@ void intel_init_clock_gating(struct drm_
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!dev_priv->display.init_clock_gating) {
+		printk(KERN_WARNING "init_clock_gating not set!\n");
+		WARN_ON(1);
+	} else
 	dev_priv->display.init_clock_gating(dev);
 
 	if (dev_priv->display.init_pch_clock_gating)
Index: usb-3.0/drivers/gpu/drm/i915/i915_dma.c
===================================================================
--- usb-3.0.orig/drivers/gpu/drm/i915/i915_dma.c
+++ usb-3.0/drivers/gpu/drm/i915/i915_dma.c
@@ -2078,7 +2078,9 @@ int i915_driver_load(struct drm_device *
 
 	intel_detect_pch(dev);
 
+printk(KERN_INFO "Testing drm_core_check_feature DRIVER_MODESET\n");
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+printk(KERN_INFO "Calling i915_load_modeset_init\n");
 		ret = i915_load_modeset_init(dev);
 		if (ret < 0) {
 			DRM_ERROR("failed to init modeset\n");


Here is the dmesg log showing what happens during "insmod i915.ko":

[  908.129497] pci 0000:00:02.0: setting latency timer to 64
[  908.179865] [drm] Supports vblank timestamp caching Rev 1 (10.10.2010).
[  908.180633] [drm] No driver support for vblank timestamp query.
[  908.180804] Testing drm_core_check_feature DRIVER_MODESET
[  908.181120] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0

The first debugging message is printed but not the second.


Here is what happens during a resume from system suspend:

[  943.013656] init_clock_gating not set!
[  943.013790] ------------[ cut here ]------------
[  943.013954] WARNING: at drivers/gpu/drm/i915/intel_display.c:7516 intel_init_clock_gating+0x30/0x4a [i915]()
[  943.014193] Hardware name: HP dx2000 MT (EE004AA)
[  943.014330] Modules linked in: i915 fbcon font bitblit softcursor drm_kms_helper drm fb fbdev i2c_algo_bit cfbcopyarea i2c_core video backlight cfbimgblt cfbfillrect e100 ohci_hcd mii pcspkr evdev uhci_hcd ehci_hcd fan processor thermal_sys button usbcore [last unloaded: i915]
[  943.015825] Pid: 1678, comm: bash Not tainted 3.0.0-rc3 #2
[  943.015966] Call Trace:
[  943.016179]  [<c1027315>] warn_slowpath_common+0x65/0x7a
[  943.016342]  [<f013e716>] ? intel_init_clock_gating+0x30/0x4a [i915]
[  943.016487]  [<c1027339>] warn_slowpath_null+0xf/0x13
[  943.016644]  [<f013e716>] intel_init_clock_gating+0x30/0x4a [i915]
[  943.016801]  [<f012f1f9>] i915_restore_state+0xf4/0x1bf [i915]
[  943.016989]  [<f0124100>] i915_drm_thaw+0x41/0xc1 [i915]
[  943.017141]  [<f01242c0>] i915_resume+0x38/0x4b [i915]
[  943.017301]  [<f00b0472>] drm_class_resume+0x39/0x3b [drm]
[  943.017447]  [<c116b520>] legacy_resume+0x1e/0x46
[  943.017599]  [<f00b0439>] ? drm_class_suspend+0x3d/0x3d [drm]
[  943.017742]  [<c116b7de>] device_resume+0x83/0xa0
[  943.017881]  [<c116bdd8>] dpm_resume+0xdc/0x156
[  943.018020]  [<c116bf68>] dpm_resume_end+0xb/0x15
[  943.018162]  [<c1053994>] suspend_devices_and_enter+0x165/0x192
[  943.018330]  [<c1053a93>] enter_state+0xd2/0x123
[  943.018471]  [<c105329f>] state_store+0x95/0xa1
[  943.018610]  [<c105320a>] ? pm_async_store+0x33/0x33
[  943.018752]  [<c1103c15>] kobj_attr_store+0x16/0x22
[  943.018894]  [<c10c7e5b>] sysfs_write_file+0xb3/0xec
[  943.019034]  [<c10c7da8>] ? sysfs_open_file+0x1c2/0x1c2
[  943.019176]  [<c108d793>] vfs_write+0x76/0xa2
[  943.019315]  [<c108d8f6>] sys_write+0x3b/0x5d
[  943.019456]  [<c11fb610>] sysenter_do_call+0x12/0x36
[  943.019625] ---[ end trace dc74bd86a8bff7da ]---


What's the right way to fix this?

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Jesse Barnes]

On Wed, 15 Jun 2011 16:08:51 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> The problem of dev_priv->display.init_clock_gating not getting set is 
> still present in 3.0-rc3.  On my system this happens because 
> intel_init_display() never gets called in the first place.
> 
> AFAICT, the normal calling sequence during driver initialization is:
> 
> 	i915_driver_load() -> i915_load_modeset_init() ->
> 		intel_modeset_init() -> intel_init_display().
> 
> But in my case the call to i915_load_modeset_init() doesn't occur 
> because drm_core_check_feature(dev, DRIVER_MODESET) is False.

Ouch, a non-KMS config.  Any reason you can't use KMS?

This patch should help at any rate.

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0defd42..a1a28fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -429,6 +429,9 @@ static int i915_drm_thaw(struct drm_device *dev)
 	/* KMS EnterVT equivalent */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		mutex_lock(&dev->struct_mutex);
+
+		intel_init_clock_gating(dev);
+
 		dev_priv->mm.suspended = 0;
 
 		error = i915_gem_init_ringbuffer(dev);
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 60a94d2..b478d16 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -863,8 +863,6 @@ int i915_restore_state(struct drm_device *dev)
 		I915_WRITE(IMR, dev_priv->saveIMR);
 	}
 
-	intel_init_clock_gating(dev);
-
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_enable_drps(dev);
 		intel_init_emon(dev);

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 15 Jun 2011, Jesse Barnes wrote:

> On Wed, 15 Jun 2011 16:08:51 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > The problem of dev_priv->display.init_clock_gating not getting set is 
> > still present in 3.0-rc3.  On my system this happens because 
> > intel_init_display() never gets called in the first place.
> > 
> > AFAICT, the normal calling sequence during driver initialization is:
> > 
> > 	i915_driver_load() -> i915_load_modeset_init() ->
> > 		intel_modeset_init() -> intel_init_display().
> > 
> > But in my case the call to i915_load_modeset_init() doesn't occur 
> > because drm_core_check_feature(dev, DRIVER_MODESET) is False.
> 
> Ouch, a non-KMS config.  Any reason you can't use KMS?

Normally I do use it.  This was a special testing config I've been 
nursing along for years, since well before KMS existed.  Either I never 
enabled KMS in the config, or else at some point it caused trouble so I 
removed it and never added it back.  Can't remember which -- all the 
testing I do with this config is at a VT, never under X.

> This patch should help at any rate.

I confirm that the patch fixes the problem.  Thanks.

On a different but related note, "rmmod i915" incites a lockdep 
notification:

[   54.316439] INFO: trying to register non-static key.
[   54.316589] the code is fine but needs lockdep annotation.
[   54.316729] turning off the locking correctness validator.
[   54.316871] Pid: 1683, comm: rmmod Not tainted 3.0.0-rc3 #2
[   54.317011] Call Trace:
[   54.317153]  [<c11f582a>] ? printk+0xf/0x11
[   54.317296]  [<c1049b3f>] register_lock_class+0x58/0x2d7
[   54.317438]  [<c102176e>] ? get_parent_ip+0xb/0x31
[   54.317579]  [<c105295a>] ? is_module_text_address+0x37/0x45
[   54.317722]  [<c1038917>] ? __kernel_text_address+0x1c/0x3e
[   54.317864]  [<c1049e61>] __lock_acquire+0xa3/0xc5a
[   54.318005]  [<c1003833>] ? dump_trace+0x7f/0xa5
[   54.318146]  [<c104aa09>] ? __lock_acquire+0xc4b/0xc5a
[   54.318287]  [<c104adf7>] lock_acquire+0x5e/0x75
[   54.318427]  [<c10364f4>] ? work_on_cpu+0x96/0x96
[   54.318567]  [<c1036530>] wait_on_work+0x3c/0x133
[   54.318707]  [<c10364f4>] ? work_on_cpu+0x96/0x96
[   54.318848]  [<c102fe0d>] ? lock_timer_base.clone.23+0x20/0x3e
[   54.318991]  [<c11f7892>] ? _raw_spin_unlock_irqrestore+0x36/0x5b
[   54.319134]  [<c102176e>] ? get_parent_ip+0xb/0x31
[   54.319275]  [<c11f9f99>] ? sub_preempt_count+0x7c/0x89
[   54.319417]  [<c1036cb2>] __cancel_work_timer+0xa0/0xde
[   54.319559]  [<c1036d07>] cancel_work_sync+0xa/0xc
[   54.319714]  [<f0128105>] i915_driver_unload+0x136/0x224 [i915]
[   54.319874]  [<f00af39d>] drm_put_dev+0xa9/0x170 [drm]
[   54.320029]  [<f00b086d>] drm_pci_exit+0x49/0x63 [drm]
[   54.320045]  [<f01508d0>] i915_exit+0x12/0x742 [i915]
[   54.320045]  [<c1050da5>] sys_delete_module+0x175/0x1c1
[   54.320045]  [<c107efb2>] ? remove_vma+0x52/0x58
[   54.320045]  [<c11f7ce0>] ? restore_all+0xf/0xf
[   54.320045]  [<c11fb610>] sysenter_do_call+0x12/0x36
[   54.336786] [drm] Module unloaded

Is this a known problem?

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Keith Packard]

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

On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0defd42..a1a28fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -429,6 +429,9 @@ static int i915_drm_thaw(struct drm_device *dev)
>  	/* KMS EnterVT equivalent */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		mutex_lock(&dev->struct_mutex);
> +
> +		intel_init_clock_gating(dev);
> +
>  		dev_priv->mm.suspended = 0;
>  
>  		error = i915_gem_init_ringbuffer(dev);
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 60a94d2..b478d16 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -863,8 +863,6 @@ int i915_restore_state(struct drm_device *dev)
>  		I915_WRITE(IMR, dev_priv->saveIMR);
>  	}
>  
> -	intel_init_clock_gating(dev);
> -
>  	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_enable_drps(dev);
>  		intel_init_emon(dev);
> 

I haven't seen any comments as to whether this patch needs to be merged
into the kernel. Has anyone tested this?

-- 
keith.packard@intel.com

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

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 22 Jun 2011, Keith Packard wrote:

> On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 0defd42..a1a28fb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -429,6 +429,9 @@ static int i915_drm_thaw(struct drm_device *dev)
> >  	/* KMS EnterVT equivalent */
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		mutex_lock(&dev->struct_mutex);
> > +
> > +		intel_init_clock_gating(dev);
> > +
> >  		dev_priv->mm.suspended = 0;
> >  
> >  		error = i915_gem_init_ringbuffer(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 60a94d2..b478d16 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -863,8 +863,6 @@ int i915_restore_state(struct drm_device *dev)
> >  		I915_WRITE(IMR, dev_priv->saveIMR);
> >  	}
> >  
> > -	intel_init_clock_gating(dev);
> > -
> >  	if (IS_IRONLAKE_M(dev)) {
> >  		ironlake_enable_drps(dev);
> >  		intel_init_emon(dev);
> > 
> 
> I haven't seen any comments as to whether this patch needs to be merged
> into the kernel. Has anyone tested this?

I have tested it, and it worked well on my system:

	http://marc.info/?l=linux-kernel&m=130817292323254&w=2

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Florian Mickler]

On Wed, 22 Jun 2011 15:04:56 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 22 Jun 2011, Keith Packard wrote:
> 
> > On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 
> > I haven't seen any comments as to whether this patch needs to be merged
> > into the kernel. Has anyone tested this?
> 
> I have tested it, and it worked well on my system:
> 
> 	http://marc.info/?l=linux-kernel&m=130817292323254&w=2
> 
> Alan Stern
> 
Some clock-gating patch was also proposed here:
https://bugzilla.kernel.org/show_bug.cgi?id=37252

Regards,
Flo

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Thu, 23 Jun 2011, Florian Mickler wrote:

> On Wed, 22 Jun 2011 15:04:56 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Wed, 22 Jun 2011, Keith Packard wrote:
> > 
> > > On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > 
> > > I haven't seen any comments as to whether this patch needs to be merged
> > > into the kernel. Has anyone tested this?
> > 
> > I have tested it, and it worked well on my system:
> > 
> > 	http://marc.info/?l=linux-kernel&m=130817292323254&w=2
> > 
> > Alan Stern
> > 
> Some clock-gating patch was also proposed here:
> https://bugzilla.kernel.org/show_bug.cgi?id=37252

That was a different fix for a different problem, although the symptoms 
were the same.

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Keith Packard]

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

On Sun, 5 Jun 2011 01:02:35 +0100 (BST), Scott Ashcroft <scott.ashcroft@talk21.com> wrote:
> Looks like the following commit:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6067aaeadb5b3df26f27ac827256b1ef01e674f5
> 
> 
> didn't cope with the pineview case so dev_priv->display.init_clock_gating is 
> NULL causing an Oops.
> 
> The old code didn't seem to do anything for pineview so I wrapped the call with 
> a simple NULL check but I'm not sure if that's the correct fix.

A fix is on its way to master through drm:

95e0ee92d3a605de75a633dd2360700595d5a8ad

    drm/i915: fix regression after clock gating init split
    
    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.

-- 
keith.packard@intel.com

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

Oops in i915 intel_init_clock_gating

[Scott Ashcroft]

Looks like the following commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6067aaeadb5b3df26f27ac827256b1ef01e674f5


didn't cope with the pineview case so dev_priv->display.init_clock_gating is 
NULL causing an Oops.

The old code didn't seem to do anything for pineview so I wrapped the call with 
a simple NULL check but I'm not sure if that's the correct fix.

Cheers,
Scott