[Intel-gfx] [PATCH 1/4] drm/i915: Hold struct mutex whilst pinning power context bo.

[Jesse Barnes]

On Mon,  4 Jan 2010 18:57:56 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> Hugh found an error path where we were attempting to unref a bo
> without holding the struct mutex:
> 
>   [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 ]---
> 
> It is in fact slightly more insiduous that first appears since we are
> attempting to not just free the object without the lock, but are
> trying to do the whole bo manipulation without holding the lock.
> 
> Reported-by: Hugh Dickins <hugh.dickins at tiscali.co.uk>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: stable at kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |   73
> ++++++++++++++++++++++------------ 1 files changed, 47 insertions(+),
> 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index 9187a17..e9f0c72 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4414,6 +4414,42 @@ static const struct drm_mode_config_funcs
> intel_mode_funcs = { .fb_changed = intelfb_probe,
>  };
>  
> +static struct drm_gem_object *
> +intel_alloc_power_context(struct drm_device *dev)
> +{
> +	struct drm_gem_object *pwrctx;
> +	int ret;
> +
> +	pwrctx = drm_gem_object_alloc(dev, 4096);
> +	if (!pwrctx) {
> +		DRM_DEBUG("failed to alloc power context, RC6
> disabled\n");
> +		return NULL;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ret = i915_gem_object_pin(pwrctx, 4096);
> +	if (ret) {
> +		DRM_ERROR("failed to pin power context: %d\n", ret);
> +		goto err_unref;
> +	}
> +
> +	ret = i915_gem_object_set_to_gtt_domain(pwrctx, 1);
> +	if (ret) {
> +		DRM_ERROR("failed to set-domain on power context:
> %d\n", ret);
> +		goto err_unpin;
> +	}
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return pwrctx;
> +
> +err_unpin:
> +	i915_gem_object_unpin(pwrctx);
> +err_unref:
> +	drm_gem_object_unreference(pwrctx);
> +	mutex_unlock(&dev->struct_mutex);
> +	return NULL;
> +}
> +
>  void intel_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4467,41 +4503,26 @@ void intel_init_clock_gating(struct
> drm_device *dev)
>  	 * to save state.
>  	 */
>  	if (I915_HAS_RC6(dev) && drm_core_check_feature(dev,
> DRIVER_MODESET)) {
> -		struct drm_gem_object *pwrctx;
> -		struct drm_i915_gem_object *obj_priv;
> -		int ret;
> +		struct drm_i915_gem_object *obj_priv = NULL;
>  
>  		if (dev_priv->pwrctx) {
>  			obj_priv = dev_priv->pwrctx->driver_private;
>  		} else {
> -			pwrctx = drm_gem_object_alloc(dev, 4096);
> -			if (!pwrctx) {
> -				DRM_DEBUG("failed to alloc power
> context, "
> -					  "RC6 disabled\n");
> -				goto out;
> -			}
> +			struct drm_gem_object *pwrctx;
>  
> -			ret = i915_gem_object_pin(pwrctx, 4096);
> -			if (ret) {
> -				DRM_ERROR("failed to pin power
> context: %d\n",
> -					  ret);
> -				drm_gem_object_unreference(pwrctx);
> -				goto out;
> +			pwrctx = intel_alloc_power_context(dev);
> +			if (pwrctx) {
> +				dev_priv->pwrctx = pwrctx;
> +				obj_priv = pwrctx->driver_private;
>  			}
> -
> -			i915_gem_object_set_to_gtt_domain(pwrctx, 1);
> -
> -			dev_priv->pwrctx = pwrctx;
> -			obj_priv = pwrctx->driver_private;
>  		}
>  
> -		I915_WRITE(PWRCTXA, obj_priv->gtt_offset |
> PWRCTX_EN);
> -		I915_WRITE(MCHBAR_RENDER_STANDBY,
> -			   I915_READ(MCHBAR_RENDER_STANDBY) &
> ~RCX_SW_EXIT);
> +		if (obj_priv) {
> +			I915_WRITE(PWRCTXA, obj_priv->gtt_offset |
> PWRCTX_EN);
> +			I915_WRITE(MCHBAR_RENDER_STANDBY,
> +				   I915_READ(MCHBAR_RENDER_STANDBY)
> & ~RCX_SW_EXIT);
> +		}
>  	}
> -
> -out:
> -	return;
>  }
>  
>  /* Set up chip specific display functions */

Looks good to me Chris, thanks.

Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center