[PATCH 0/3] some locking clarifications

[PATCH 0/3] some locking clarifications

[Daniel Vetter]

Hi all,

Three patches for little things I've recently noticed. The long-term aim behind
the last one is to slowly reduce the scope of struct_mutex and clarify what's
actually protected by it.

Comments highly welcome.

Cheers, Daniel

Daniel Vetter (3):
  drm/i915: remove bogus mutex_unlock from error-path
  drm/i915: cancel console_resume_work before destroying the fbdev
  drm/i915: push struct_mutex locking down to ironlake_enable_rc6

 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    2 --
 drivers/gpu/drm/i915/intel_pm.c      |   14 +++++++++-----
 3 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] drm/i915: remove bogus mutex_unlock from error-path

[Daniel Vetter]

This has been lost in the locking rework for intel_alloc_context_page:

commit 2c34b850ee1e9f86b41706149d0954eee58757a3
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Sat Mar 19 18:14:26 2011 -0700

    drm/i915: fix ilk rc6 teardown locking

Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3280cff..7658c39 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2286,7 +2286,6 @@ err_unpin:
 	i915_gem_object_unpin(ctx);
 err_unref:
 	drm_gem_object_unreference(&ctx->base);
-	mutex_unlock(&dev->struct_mutex);
 	return NULL;
 }
 
-- 
1.7.10.4

[PATCH 2/3] drm/i915: cancel console_resume_work before destroying the fbdev

[Daniel Vetter]

Could lead to tears if the work indeed runs after we've destroyed the
fbdev ... Totally unlikely to happen though in reality.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 99daa89..363d2d0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1684,9 +1684,9 @@ int i915_driver_unload(struct drm_device *dev)
 	acpi_video_unregister();
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		cancel_work_sync(&dev_priv->console_resume_work);
 		intel_fbdev_fini(dev);
 		intel_modeset_cleanup(dev);
-		cancel_work_sync(&dev_priv->console_resume_work);
 
 		/*
 		 * free the memory space allocated for the child device
-- 
1.7.10.4

[PATCH 3/3] drm/i915: push struct_mutex locking down to ironlake_enable_rc6

[Daniel Vetter]

Originally dev->struct_mutex protected all kinds of things for rps/ips
(debugfs access, driver state, ...). Nowadays we have
dev_priv->rps.lock for this, so only the ilk ips code needs it really.

Push the locking down. I haven't yet changed the cleanup side since
there we lock large parts of the code with struct_mutex.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    2 --
 drivers/gpu/drm/i915/intel_pm.c      |   13 +++++++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index da1ad9c..91e99a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8945,9 +8945,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
 
 	intel_init_clock_gating(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_enable_gt_powersave(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 void intel_modeset_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7658c39..3ed7114 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2715,6 +2715,8 @@ void ironlake_teardown_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (dev_priv->ips.renderctx) {
 		i915_gem_object_unpin(dev_priv->ips.renderctx);
 		drm_gem_object_unreference(&dev_priv->ips.renderctx->base);
@@ -2778,11 +2780,11 @@ static void ironlake_enable_rc6(struct drm_device *dev)
 	if (!intel_enable_rc6(dev))
 		return;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	mutex_lock(&dev->struct_mutex);
 
 	ret = ironlake_setup_rc6(dev);
 	if (ret)
-		return;
+		goto unlock;
 
 	was_interruptible = dev_priv->mm.interruptible;
 	dev_priv->mm.interruptible = false;
@@ -2795,7 +2797,7 @@ static void ironlake_enable_rc6(struct drm_device *dev)
 	if (ret) {
 		ironlake_teardown_rc6(dev);
 		dev_priv->mm.interruptible = was_interruptible;
-		return;
+		goto unlock;
 	}
 
 	intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
@@ -2820,11 +2822,14 @@ static void ironlake_enable_rc6(struct drm_device *dev)
 	if (ret) {
 		DRM_ERROR("failed to enable ironlake power power savings\n");
 		ironlake_teardown_rc6(dev);
-		return;
+		goto unlock;
 	}
 
 	I915_WRITE(PWRCTXA, dev_priv->ips.pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
+
+unlock:
+	mutex_lock(&dev->struct_mutex);
 }
 
 static unsigned long intel_pxfreq(u32 vidfreq)
-- 
1.7.10.4

Re: [PATCH 1/3] drm/i915: remove bogus mutex_unlock from error-path

[Ben Widawsky]

On Tue, Feb 12, 2013 at 03:36:03PM +0100, Daniel Vetter wrote:
> This has been lost in the locking rework for intel_alloc_context_page:
> 
> commit 2c34b850ee1e9f86b41706149d0954eee58757a3
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Sat Mar 19 18:14:26 2011 -0700
> 
>     drm/i915: fix ilk rc6 teardown locking
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'm confused about how it got lost.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/intel_pm.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3280cff..7658c39 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2286,7 +2286,6 @@ err_unpin:
>  	i915_gem_object_unpin(ctx);
>  err_unref:
>  	drm_gem_object_unreference(&ctx->base);
> -	mutex_unlock(&dev->struct_mutex);
>  	return NULL;
>  }
>  
> -- 
> 1.7.10.4
> 

-- 
Ben Widawsky, Intel Open Source Technology Center