drm: gem_object_free without struct_mutex

drm: gem_object_free without struct_mutex

[Hugh Dickins]

I've changed BUG_ON to WARN_ON in drm_gem.c (patch at bottom) to
get this dmesg when I resume after suspend, instead of crashing.

Perhaps it's a patch that should go in, perhaps not, but obviously
the real problem lies elsewhere.  Happens with 2.6.33-rc1 and -rc2.

No surprise if I'm stupidly misconfigured to get the "pin power context"
error in the first place (.config on demand), but I don't deserve to BUG!

Hugh

[drm:intel_init_clock_gating] *ERROR* failed to pin power context: -16
------------[ cut here ]------------
WARNING: at drivers/gpu/drm/drm_gem.c:438 drm_gem_object_free+0x20/0x5e()
Hardware name: ESPRIMO Mobile V5505           
Modules linked in: snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device
Pid: 3793, comm: s2ram Not tainted 2.6.33-rc2 #4
Call Trace:
 [<7815298e>] warn_slowpath_common+0x59/0x6b
 [<781529b3>] warn_slowpath_null+0x13/0x18
 [<78317c1a>] ? drm_gem_object_free+0x20/0x5e
 [<78317c1a>] drm_gem_object_free+0x20/0x5e
 [<78317bfa>] ? drm_gem_object_free+0x0/0x5e
 [<7829df11>] kref_put+0x38/0x45
 [<7833a5f0>] intel_init_clock_gating+0x232/0x271
 [<78317bfa>] ? drm_gem_object_free+0x0/0x5e
 [<7832c307>] i915_restore_state+0x21a/0x2b3
 [<7832379d>] i915_resume+0x3c/0xbb
 [<78174fe5>] ? trace_hardirqs_on_caller+0xfc/0x123
 [<7831c756>] ? drm_class_resume+0x0/0x3e
 [<7831c78d>] drm_class_resume+0x37/0x3e
 [<78351e0a>] legacy_resume+0x1e/0x51
 [<78351ece>] device_resume+0x91/0xab
 [<7831c756>] ? drm_class_resume+0x0/0x3e
 [<78352226>] dpm_resume+0x58/0x10f
 [<783522fb>] dpm_resume_end+0x1e/0x2c
 [<78180f80>] suspend_devices_and_enter+0x61/0x84
 [<78180ff8>] enter_state+0x55/0x83
 [<7818091c>] state_store+0x94/0xaa
 [<7829d09e>] kobj_attr_store+0x1e/0x23
 [<782098e0>] sysfs_write_file+0x66/0x99
 [<781cd2f0>] vfs_write+0x8a/0x108
 [<781cd408>] sys_write+0x3c/0x63
 [<78125c10>] sysenter_do_call+0x12/0x36
---[ end trace a343537f29950fda ]---

[PATCH] gem: free just WARN_ON(!mutex_is_locked)

drm_gem_object_free() WARN_ON(!mutex_is_locked) instead of BUG_ON:
i915_resume()->i915_restore_state()->intel_init_clock_gating()
is currently causing it to trigger with my config.

Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---

 drivers/gpu/drm/drm_gem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 2.6.33-rc2/drivers/gpu/drm/drm_gem.c	2009-12-03 03:51:21.000000000 +0000
+++ linux/drivers/gpu/drm/drm_gem.c	2009-12-26 18:29:57.000000000 +0000
@@ -435,7 +435,7 @@ drm_gem_object_free(struct kref *kref)
 	struct drm_gem_object *obj = (struct drm_gem_object *) kref;
 	struct drm_device *dev = obj->dev;
 
-	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	if (dev->driver->gem_free_object != NULL)
 		dev->driver->gem_free_object(obj);

Re: drm: gem_object_free without struct_mutex

[Rafael J. Wysocki]

[CC to Jesse.]

On Wednesday 30 December 2009, Hugh Dickins wrote:
> I've changed BUG_ON to WARN_ON in drm_gem.c (patch at bottom) to
> get this dmesg when I resume after suspend, instead of crashing.
> 
> Perhaps it's a patch that should go in, perhaps not, but obviously
> the real problem lies elsewhere.  Happens with 2.6.33-rc1 and -rc2.
> 
> No surprise if I'm stupidly misconfigured to get the "pin power context"
> error in the first place (.config on demand), but I don't deserve to BUG!
> 
> Hugh
> 
> [drm:intel_init_clock_gating] *ERROR* failed to pin power context: -16
> ------------[ cut here ]------------
> WARNING: at drivers/gpu/drm/drm_gem.c:438 drm_gem_object_free+0x20/0x5e()
> Hardware name: ESPRIMO Mobile V5505           
> Modules linked in: snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device
> Pid: 3793, comm: s2ram Not tainted 2.6.33-rc2 #4
> Call Trace:
>  [<7815298e>] warn_slowpath_common+0x59/0x6b
>  [<781529b3>] warn_slowpath_null+0x13/0x18
>  [<78317c1a>] ? drm_gem_object_free+0x20/0x5e
>  [<78317c1a>] drm_gem_object_free+0x20/0x5e
>  [<78317bfa>] ? drm_gem_object_free+0x0/0x5e
>  [<7829df11>] kref_put+0x38/0x45
>  [<7833a5f0>] intel_init_clock_gating+0x232/0x271
>  [<78317bfa>] ? drm_gem_object_free+0x0/0x5e
>  [<7832c307>] i915_restore_state+0x21a/0x2b3
>  [<7832379d>] i915_resume+0x3c/0xbb
>  [<78174fe5>] ? trace_hardirqs_on_caller+0xfc/0x123
>  [<7831c756>] ? drm_class_resume+0x0/0x3e
>  [<7831c78d>] drm_class_resume+0x37/0x3e
>  [<78351e0a>] legacy_resume+0x1e/0x51
>  [<78351ece>] device_resume+0x91/0xab
>  [<7831c756>] ? drm_class_resume+0x0/0x3e
>  [<78352226>] dpm_resume+0x58/0x10f
>  [<783522fb>] dpm_resume_end+0x1e/0x2c
>  [<78180f80>] suspend_devices_and_enter+0x61/0x84
>  [<78180ff8>] enter_state+0x55/0x83
>  [<7818091c>] state_store+0x94/0xaa
>  [<7829d09e>] kobj_attr_store+0x1e/0x23
>  [<782098e0>] sysfs_write_file+0x66/0x99
>  [<781cd2f0>] vfs_write+0x8a/0x108
>  [<781cd408>] sys_write+0x3c/0x63
>  [<78125c10>] sysenter_do_call+0x12/0x36
> ---[ end trace a343537f29950fda ]---
> 
> [PATCH] gem: free just WARN_ON(!mutex_is_locked)
> 
> drm_gem_object_free() WARN_ON(!mutex_is_locked) instead of BUG_ON:
> i915_resume()->i915_restore_state()->intel_init_clock_gating()
> is currently causing it to trigger with my config.
> 
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> ---
> 
>  drivers/gpu/drm/drm_gem.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 2.6.33-rc2/drivers/gpu/drm/drm_gem.c	2009-12-03 03:51:21.000000000 +0000
> +++ linux/drivers/gpu/drm/drm_gem.c	2009-12-26 18:29:57.000000000 +0000
> @@ -435,7 +435,7 @@ drm_gem_object_free(struct kref *kref)
>  	struct drm_gem_object *obj = (struct drm_gem_object *) kref;
>  	struct drm_device *dev = obj->dev;
>  
> -	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  	if (dev->driver->gem_free_object != NULL)
>  		dev->driver->gem_free_object(obj);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: drm: gem_object_free without struct_mutex

[Dan Carpenter]

On Wed, Dec 30, 2009 at 11:17:48PM +0100, Rafael J. Wysocki wrote:
> [CC to Jesse.]
> 
> On Wednesday 30 December 2009, Hugh Dickins wrote:
> > I've changed BUG_ON to WARN_ON in drm_gem.c (patch at bottom) to
> > get this dmesg when I resume after suspend, instead of crashing.
> > 
> > Perhaps it's a patch that should go in, perhaps not, but obviously
> > the real problem lies elsewhere.  Happens with 2.6.33-rc1 and -rc2.
> > 
> > No surprise if I'm stupidly misconfigured to get the "pin power context"
> > error in the first place (.config on demand), but I don't deserve to BUG!
> > 

The problem is dev_priv->mm.suspended which was set in i915_suspend().

In i915_resume()

   107          i915_restore_state(dev);

The i915_restore_state() fails because i915_gem_object_bind_to_gtt()
does this:

        if (dev_priv->mm.suspended)
                return -EBUSY;

   108  
   109          intel_opregion_init(dev, 1);
   110  
   111          /* KMS EnterVT equivalent */
   112          if (drm_core_check_feature(dev, DRIVER_MODESET)) {
   113                  mutex_lock(&dev->struct_mutex);
   114                  dev_priv->mm.suspended = 0;

We do set the dev_priv->mm.suspended here but it's too late.

Not sure the fix though...

regards,
dan carpenter

Re: drm: gem_object_free without struct_mutex

[Jesse Barnes]

On Sat, 2 Jan 2010 16:46:07 +0200
Dan Carpenter <error27@gmail.com> wrote:

> On Wed, Dec 30, 2009 at 11:17:48PM +0100, Rafael J. Wysocki wrote:
> > [CC to Jesse.]
> > 
> > On Wednesday 30 December 2009, Hugh Dickins wrote:
> > > I've changed BUG_ON to WARN_ON in drm_gem.c (patch at bottom) to
> > > get this dmesg when I resume after suspend, instead of crashing.
> > > 
> > > Perhaps it's a patch that should go in, perhaps not, but obviously
> > > the real problem lies elsewhere.  Happens with 2.6.33-rc1 and
> > > -rc2.
> > > 
> > > No surprise if I'm stupidly misconfigured to get the "pin power
> > > context" error in the first place (.config on demand), but I
> > > don't deserve to BUG!
> > > 
> 
> The problem is dev_priv->mm.suspended which was set in i915_suspend().
> 
> In i915_resume()
> 
>    107          i915_restore_state(dev);
> 
> The i915_restore_state() fails because i915_gem_object_bind_to_gtt()
> does this:
> 
>         if (dev_priv->mm.suspended)
>                 return -EBUSY;
> 
>    108  
>    109          intel_opregion_init(dev, 1);
>    110  
>    111          /* KMS EnterVT equivalent */
>    112          if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>    113                  mutex_lock(&dev->struct_mutex);
>    114                  dev_priv->mm.suspended = 0;
> 
> We do set the dev_priv->mm.suspended here but it's too late.
> 
> Not sure the fix though...

I think this one should be fixed by Chris's recent patchset.  Should
land in Eric's tree soon.

-- 
Jesse Barnes, Intel Open Source Technology Center