[PATCH] drm/i915: Disable all GEM timers and work on unload

[PATCH] drm/i915: Disable all GEM timers and work on unload

[Chris Wilson]

We have two once very similar functions, i915_gpu_idle() and
i915_gem_idle(). The former is used as the lower level operation to
flush work on the GPU, whereas the latter is the high level interface to
flush the GEM bookkeeping in addition to flushing the GPU. As such
i915_gem_idle() also clears out the request and activity lists and
cancels the delayed work. This is what we need for unloading the driver,
unfortunately we called i915_gpu_idle() instead.

In the process, make sure that when cancelling the delayed work and
timer, which is synchronous, that we do not hold any locks to prevent a
deadlock if the work item is already waiting upon the mutex. This
requires us to push the mutex down from the caller to i915_gem_idle().

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70334
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c |    8 +------
 drivers/gpu/drm/i915/i915_drv.c |    4 +---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 drivers/gpu/drm/i915/i915_gem.c |   44 +++++++++++++++++----------------------
 4 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e35f29..c274f98 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1699,15 +1699,9 @@ int i915_driver_unload(struct drm_device *dev)
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
 		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
 
-	mutex_lock(&dev->struct_mutex);
-	ret = i915_gpu_idle(dev);
+	ret = i915_gem_idle(dev, true);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
-	i915_gem_retire_requests(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	/* Cancel the retire work handler, which should be idle now. */
-	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 
 	io_mapping_free(dev_priv->gtt.mappable);
 	arch_phys_wc_del(dev_priv->gtt.mtrr);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 59649c0..7cab535 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -479,9 +479,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		int error;
 
-		mutex_lock(&dev->struct_mutex);
-		error = i915_gem_idle(dev);
-		mutex_unlock(&dev->struct_mutex);
+		error = i915_gem_idle(dev, false);
 		if (error) {
 			dev_err(&dev->pdev->dev,
 				"GEM idle failed, resume might fail\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0152e7..10e1236 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2039,7 +2039,7 @@ int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
-int __must_check i915_gem_idle(struct drm_device *dev);
+int __must_check i915_gem_idle(struct drm_device *dev, bool disable);
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
 		       struct drm_i915_gem_object *batch_obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 63dbd3c..0d6c95b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4401,17 +4401,18 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 }
 
 int
-i915_gem_idle(struct drm_device *dev)
+i915_gem_idle(struct drm_device *dev, bool disable)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret;
+	int ret = 0;
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->ums.mm_suspended)
-		return 0;
+		goto err;
 
 	ret = i915_gpu_idle(dev);
 	if (ret)
-		return ret;
+		goto err;
 
 	i915_gem_retire_requests(dev);
 
@@ -4419,16 +4420,25 @@ i915_gem_idle(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_gem_evict_everything(dev);
 
-	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
-
 	i915_kernel_lost_context(dev);
 	i915_gem_cleanup_ringbuffer(dev);
 
-	/* Cancel the retire work handler, which should be idle now. */
+	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
+	 * We need to replace this with a semaphore, or something.
+	 * And not confound ums.mm_suspended!
+	 */
+	dev_priv->ums.mm_suspended = disable;
+	mutex_unlock(&dev->struct_mutex);
+
+	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 	cancel_delayed_work_sync(&dev_priv->mm.idle_work);
 
 	return 0;
+
+err:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
 }
 
 int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice)
@@ -4681,26 +4691,12 @@ int
 i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return 0;
 
 	drm_irq_uninstall(dev);
 
-	mutex_lock(&dev->struct_mutex);
-	ret =  i915_gem_idle(dev);
-
-	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
-	 * We need to replace this with a semaphore, or something.
-	 * And not confound ums.mm_suspended!
-	 */
-	if (ret != 0)
-		dev_priv->ums.mm_suspended = 1;
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
+	return i915_gem_idle(dev, true);
 }
 
 void
@@ -4711,11 +4707,9 @@ i915_gem_lastclose(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return;
 
-	mutex_lock(&dev->struct_mutex);
-	ret = i915_gem_idle(dev);
+	ret = i915_gem_idle(dev, false);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void
-- 
1.7.9.5

Re: [PATCH] drm/i915: Disable all GEM timers and work on unload

[Daniel Vetter]

On Fri, Oct 11, 2013 at 11:14 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
>  void
> @@ -4711,11 +4707,9 @@ i915_gem_lastclose(struct drm_device *dev)
>         if (drm_core_check_feature(dev, DRIVER_MODESET))
>                 return;
>
> -       mutex_lock(&dev->struct_mutex);
> -       ret = i915_gem_idle(dev);
> +       ret = i915_gem_idle(dev, false);

I think this here should also set disable=true, in case X died
prematurely and forgot to do the leavevt dance. At that point I think
we could rip out that disable parameter again and just set it when in
ums mode. Also this change should be in a separate patch, at least I
don't understand why we need to change it here together with the
untangling of the gpu_idle/gem_idle confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Disable all GEM timers and work on unload

[Chris Wilson]

On Fri, Oct 11, 2013 at 06:22:55PM +0200, Daniel Vetter wrote:
> On Fri, Oct 11, 2013 at 11:14 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> >  void
> > @@ -4711,11 +4707,9 @@ i915_gem_lastclose(struct drm_device *dev)
> >         if (drm_core_check_feature(dev, DRIVER_MODESET))
> >                 return;
> >
> > -       mutex_lock(&dev->struct_mutex);
> > -       ret = i915_gem_idle(dev);
> > +       ret = i915_gem_idle(dev, false);
> 
> I think this here should also set disable=true, in case X died
> prematurely and forgot to do the leavevt dance. At that point I think
> we could rip out that disable parameter again and just set it when in
> ums mode. Also this change should be in a separate patch, at least I
> don't understand why we need to change it here together with the
> untangling of the gpu_idle/gem_idle confusion.

Seems reasonable, I just wanted to mimic the existing behaviour closely.
I would also rename i915_gem_idle() then, perhaps i915_gem_freeze(),
_suspend(), _disable(), _plug() or _stop()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Disable all GEM timers and work on unload

[Chris Wilson]

We have two once very similar functions, i915_gpu_idle() and
i915_gem_idle(). The former is used as the lower level operation to
flush work on the GPU, whereas the latter is the high level interface to
flush the GEM bookkeeping in addition to flushing the GPU. As such
i915_gem_idle() also clears out the request and activity lists and
cancels the delayed work. This is what we need for unloading the driver,
unfortunately we called i915_gpu_idle() instead.

In the process, make sure that when cancelling the delayed work and
timer, which is synchronous, that we do not hold any locks to prevent a
deadlock if the work item is already waiting upon the mutex. This
requires us to push the mutex down from the caller to i915_gem_idle().

v2: s/i915_gem_idle/i915_gem_suspend/

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70334
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: xunx.fang@intel.com
---
 drivers/gpu/drm/i915/i915_dma.c |    8 +------
 drivers/gpu/drm/i915/i915_drv.c |    4 +---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 drivers/gpu/drm/i915/i915_gem.c |   44 +++++++++++++++++----------------------
 4 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e35f29..fd6c46b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1699,15 +1699,9 @@ int i915_driver_unload(struct drm_device *dev)
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
 		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
 
-	mutex_lock(&dev->struct_mutex);
-	ret = i915_gpu_idle(dev);
+	ret = i915_gem_suspend(dev);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
-	i915_gem_retire_requests(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	/* Cancel the retire work handler, which should be idle now. */
-	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 
 	io_mapping_free(dev_priv->gtt.mappable);
 	arch_phys_wc_del(dev_priv->gtt.mtrr);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 59649c0..cd05cc7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -479,9 +479,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		int error;
 
-		mutex_lock(&dev->struct_mutex);
-		error = i915_gem_idle(dev);
-		mutex_unlock(&dev->struct_mutex);
+		error = i915_gem_suspend(dev);
 		if (error) {
 			dev_err(&dev->pdev->dev,
 				"GEM idle failed, resume might fail\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0152e7..daefba4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2039,7 +2039,7 @@ int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
-int __must_check i915_gem_idle(struct drm_device *dev);
+int __must_check i915_gem_suspend(struct drm_device *dev);
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
 		       struct drm_i915_gem_object *batch_obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 63dbd3c..2ff35b6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4401,17 +4401,18 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 }
 
 int
-i915_gem_idle(struct drm_device *dev)
+i915_gem_suspend(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret;
+	int ret = 0;
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->ums.mm_suspended)
-		return 0;
+		goto err;
 
 	ret = i915_gpu_idle(dev);
 	if (ret)
-		return ret;
+		goto err;
 
 	i915_gem_retire_requests(dev);
 
@@ -4419,16 +4420,25 @@ i915_gem_idle(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_gem_evict_everything(dev);
 
-	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
-
 	i915_kernel_lost_context(dev);
 	i915_gem_cleanup_ringbuffer(dev);
 
-	/* Cancel the retire work handler, which should be idle now. */
+	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
+	 * We need to replace this with a semaphore, or something.
+	 * And not confound ums.mm_suspended!
+	 */
+	dev_priv->ums.mm_suspended = true;
+	mutex_unlock(&dev->struct_mutex);
+
+	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 	cancel_delayed_work_sync(&dev_priv->mm.idle_work);
 
 	return 0;
+
+err:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
 }
 
 int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice)
@@ -4681,26 +4691,12 @@ int
 i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return 0;
 
 	drm_irq_uninstall(dev);
 
-	mutex_lock(&dev->struct_mutex);
-	ret =  i915_gem_idle(dev);
-
-	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
-	 * We need to replace this with a semaphore, or something.
-	 * And not confound ums.mm_suspended!
-	 */
-	if (ret != 0)
-		dev_priv->ums.mm_suspended = 1;
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
+	return i915_gem_suspend(dev);
 }
 
 void
@@ -4711,11 +4707,9 @@ i915_gem_lastclose(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return;
 
-	mutex_lock(&dev->struct_mutex);
-	ret = i915_gem_idle(dev);
+	ret = i915_gem_suspend(dev);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void
-- 
1.7.9.5

Re: [PATCH] drm/i915: Disable all GEM timers and work on unload

[Daniel Vetter]

On Wed, Oct 16, 2013 at 11:50:01AM +0100, Chris Wilson wrote:
> We have two once very similar functions, i915_gpu_idle() and
> i915_gem_idle(). The former is used as the lower level operation to
> flush work on the GPU, whereas the latter is the high level interface to
> flush the GEM bookkeeping in addition to flushing the GPU. As such
> i915_gem_idle() also clears out the request and activity lists and
> cancels the delayed work. This is what we need for unloading the driver,
> unfortunately we called i915_gpu_idle() instead.
> 
> In the process, make sure that when cancelling the delayed work and
> timer, which is synchronous, that we do not hold any locks to prevent a
> deadlock if the work item is already waiting upon the mutex. This
> requires us to push the mutex down from the caller to i915_gem_idle().
> 
> v2: s/i915_gem_idle/i915_gem_suspend/
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70334
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: xunx.fang@intel.com

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Disable all GEM timers and work on unload

[Chris Wilson]

On Wed, Oct 16, 2013 at 01:50:29PM +0200, Daniel Vetter wrote:
> On Wed, Oct 16, 2013 at 11:50:01AM +0100, Chris Wilson wrote:
> > We have two once very similar functions, i915_gpu_idle() and
> > i915_gem_idle(). The former is used as the lower level operation to
> > flush work on the GPU, whereas the latter is the high level interface to
> > flush the GEM bookkeeping in addition to flushing the GPU. As such
> > i915_gem_idle() also clears out the request and activity lists and
> > cancels the delayed work. This is what we need for unloading the driver,
> > unfortunately we called i915_gpu_idle() instead.
> > 
> > In the process, make sure that when cancelling the delayed work and
> > timer, which is synchronous, that we do not hold any locks to prevent a
> > deadlock if the work item is already waiting upon the mutex. This
> > requires us to push the mutex down from the caller to i915_gem_idle().
> > 
> > v2: s/i915_gem_idle/i915_gem_suspend/
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70334
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Tested-by: xunx.fang@intel.com
> 
> Queued for -next, thanks for the patch.

Oops, spotted a bug in my v2.
ums.mm_suspend should only be set for !kms. Sorry that slipped my mind
when doing the s/_idle/_suspend.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Disable all GEM timers and work on unload

[Daniel Vetter]

On Wed, Oct 16, 2013 at 05:47:26PM +0100, Chris Wilson wrote:
> On Wed, Oct 16, 2013 at 01:50:29PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 16, 2013 at 11:50:01AM +0100, Chris Wilson wrote:
> > > We have two once very similar functions, i915_gpu_idle() and
> > > i915_gem_idle(). The former is used as the lower level operation to
> > > flush work on the GPU, whereas the latter is the high level interface to
> > > flush the GEM bookkeeping in addition to flushing the GPU. As such
> > > i915_gem_idle() also clears out the request and activity lists and
> > > cancels the delayed work. This is what we need for unloading the driver,
> > > unfortunately we called i915_gpu_idle() instead.
> > > 
> > > In the process, make sure that when cancelling the delayed work and
> > > timer, which is synchronous, that we do not hold any locks to prevent a
> > > deadlock if the work item is already waiting upon the mutex. This
> > > requires us to push the mutex down from the caller to i915_gem_idle().
> > > 
> > > v2: s/i915_gem_idle/i915_gem_suspend/
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70334
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Tested-by: xunx.fang@intel.com
> > 
> > Queued for -next, thanks for the patch.
> 
> Oops, spotted a bug in my v2.
> ums.mm_suspend should only be set for !kms. Sorry that slipped my mind
> when doing the s/_idle/_suspend.

Actually I've noticed since it was missing from the changelog, but figured
you've decided against that. I'll rectify it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch