[PATCH] drm/i915: don't frob mm.suspended when not using ums

[PATCH] drm/i915: don't frob mm.suspended when not using ums

[Daniel Vetter]

In kernel modeset driver mode we're in full control of the chip,
always. So there's no need at all to set mm.suspended in
i915_gem_idle. Hence move that out into the leavevt ioctl. Since
i915_gem_idle doesn't suspend gem any more we can also drop the
re-enabling for KMS in the thaw function.

Also clean up the handling of mm.suspend at driver load by coalescing
all the assignments.

Stumbled over while reading through our resume code for unrelated
reasons.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c | 16 +++++++---------
 drivers/gpu/drm/i915/i915_drv.c |  1 -
 drivers/gpu/drm/i915/i915_gem.c | 21 ++++++++++++++-------
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index adb319b..55cfb4e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
-	if (INTEL_INFO(dev)->num_pipes == 0) {
-		dev_priv->mm.suspended = 0;
+	if (INTEL_INFO(dev)->num_pipes == 0)
 		return 0;
-	}
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
@@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	drm_kms_helper_poll_init(dev);
 
-	/* We're off and running w/KMS */
-	dev_priv->mm.suspended = 0;
-
 	return 0;
 
 cleanup_gem:
@@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	/* Start out suspended */
-	dev_priv->mm.suspended = 1;
-
 	if (HAS_POWER_WELL(dev))
 		i915_init_power_well(dev);
 
@@ -1641,6 +1633,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			DRM_ERROR("failed to init modeset\n");
 			goto out_gem_unload;
 		}
+
+		/* We're off and running w/KMS */
+		dev_priv->mm.suspended = 0;
+	} else {
+		/* Start out suspended in ums mode. */
+		dev_priv->mm.suspended = 1;
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f4af1ca..cff370e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -656,7 +656,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		intel_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
-		dev_priv->mm.suspended = 0;
 
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7f368d7..5f60d92 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4022,11 +4022,6 @@ i915_gem_idle(struct drm_device *dev)
 
 	i915_gem_reset_fences(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 mm.suspended!
-	 */
-	dev_priv->mm.suspended = 1;
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 
 	i915_kernel_lost_context(dev);
@@ -4243,7 +4238,7 @@ int
 i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -4285,11 +4280,23 @@ 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);
-	return i915_gem_idle(dev);
+	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 mm.suspended!
+	 */
+	if (ret != 0)
+		dev_priv->mm.suspended = 1;
+
+	return ret;
 }
 
 void
-- 
1.8.3.2

Re: [PATCH] drm/i915: don't frob mm.suspended when not using ums

[Chris Wilson]

On Tue, Jul 09, 2013 at 10:44:39AM +0200, Daniel Vetter wrote:
> In kernel modeset driver mode we're in full control of the chip,
> always. So there's no need at all to set mm.suspended in
> i915_gem_idle. Hence move that out into the leavevt ioctl. Since
> i915_gem_idle doesn't suspend gem any more we can also drop the
> re-enabling for KMS in the thaw function.
> 
> Also clean up the handling of mm.suspend at driver load by coalescing
> all the assignments.

Then move it into to the dri1/ums dungeon?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: don't frob mm.suspended when not using ums

[Daniel Vetter]

In kernel modeset driver mode we're in full control of the chip,
always. So there's no need at all to set mm.suspended in
i915_gem_idle. Hence move that out into the leavevt ioctl. Since
i915_gem_idle doesn't suspend gem any more we can also drop the
re-enabling for KMS in the thaw function.

Also clean up the handling of mm.suspend at driver load by coalescing
all the assignments.

Stumbled over while reading through our resume code for unrelated
reasons.

v2: Shovel mm.suspended into the (newly created) ums dungeon as
suggested by Chris Wilson. The plan is that once we've completely
stopped relying on the register save/restore code we could shovel even
that in there.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c            | 16 +++++++--------
 drivers/gpu/drm/i915/i915_drv.c            |  5 ++---
 drivers/gpu/drm/i915/i915_drv.h            | 24 +++++++++++++----------
 drivers/gpu/drm/i915/i915_gem.c            | 31 ++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 5 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..beca2d0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
-	if (INTEL_INFO(dev)->num_pipes == 0) {
-		dev_priv->mm.suspended = 0;
+	if (INTEL_INFO(dev)->num_pipes == 0)
 		return 0;
-	}
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
@@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	drm_kms_helper_poll_init(dev);
 
-	/* We're off and running w/KMS */
-	dev_priv->mm.suspended = 0;
-
 	return 0;
 
 cleanup_gem:
@@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	/* Start out suspended */
-	dev_priv->mm.suspended = 1;
-
 	if (HAS_POWER_WELL(dev))
 		i915_init_power_well(dev);
 
@@ -1641,6 +1633,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			DRM_ERROR("failed to init modeset\n");
 			goto out_gem_unload;
 		}
+
+		/* We're off and running w/KMS */
+		dev_priv->ums.mm_suspended = 0;
+	} else {
+		/* Start out suspended in ums mode. */
+		dev_priv->ums.mm_suspended = 1;
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e6dc81c..50a76f59 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -661,7 +661,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		intel_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
-		dev_priv->mm.suspended = 0;
 
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
@@ -960,11 +959,11 @@ int i915_reset(struct drm_device *dev)
 	 * switched away).
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
-			!dev_priv->mm.suspended) {
+			!dev_priv->ums.mm_suspended) {
 		struct intel_ring_buffer *ring;
 		int i;
 
-		dev_priv->mm.suspended = 0;
+		dev_priv->ums.mm_suspended = 0;
 
 		i915_gem_init_swizzling(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8d6104..be2fcd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -814,6 +814,18 @@ struct i915_dri1_state {
 	uint32_t counter;
 };
 
+struct i915_ums_state {
+	/**
+	 * Flag if the X Server, and thus DRM, is not currently in
+	 * control of the device.
+	 *
+	 * This is set between LeaveVT and EnterVT.  It needs to be
+	 * replaced with a semaphore.  It also needs to be
+	 * transitioned away from for kernel modesetting.
+	 */
+	int mm_suspended;
+};
+
 struct intel_l3_parity {
 	u32 *remap_info;
 	struct work_struct error_work;
@@ -884,16 +896,6 @@ struct i915_gem_mm {
 	 */
 	bool interruptible;
 
-	/**
-	 * Flag if the X Server, and thus DRM, is not currently in
-	 * control of the device.
-	 *
-	 * This is set between LeaveVT and EnterVT.  It needs to be
-	 * replaced with a semaphore.  It also needs to be
-	 * transitioned away from for kernel modesetting.
-	 */
-	int suspended;
-
 	/** Bit 6 swizzling required for X tiling */
 	uint32_t bit_6_swizzle_x;
 	/** Bit 6 swizzling required for Y tiling */
@@ -1187,6 +1189,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+	/* Old ums support infrastructure, same warning applies. */
+	struct i915_ums_state ums;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index af61be8..5ad9f35 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2082,7 +2082,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	trace_i915_gem_request_add(ring, request->seqno);
 	ring->outstanding_lazy_request = 0;
 
-	if (!dev_priv->mm.suspended) {
+	if (!dev_priv->ums.mm_suspended) {
 		if (i915_enable_hangcheck) {
 			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
 				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
@@ -2387,7 +2387,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 		idle &= list_empty(&ring->request_list);
 	}
 
-	if (!dev_priv->mm.suspended && !idle)
+	if (!dev_priv->ums.mm_suspended && !idle)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
 				   round_jiffies_up_relative(HZ));
 	if (idle)
@@ -3983,7 +3983,7 @@ i915_gem_idle(struct drm_device *dev)
 
 	mutex_lock(&dev->struct_mutex);
 
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		return 0;
 	}
@@ -3999,11 +3999,6 @@ i915_gem_idle(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_gem_evict_everything(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 mm.suspended!
-	 */
-	dev_priv->mm.suspended = 1;
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 
 	i915_kernel_lost_context(dev);
@@ -4220,7 +4215,7 @@ int
 i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -4232,7 +4227,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	dev_priv->mm.suspended = 0;
+	dev_priv->ums.mm_suspended = 0;
 
 	ret = i915_gem_init_hw(dev);
 	if (ret != 0) {
@@ -4252,7 +4247,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 cleanup_ringbuffer:
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_cleanup_ringbuffer(dev);
-	dev_priv->mm.suspended = 1;
+	dev_priv->ums.mm_suspended = 1;
 	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
@@ -4262,11 +4257,23 @@ 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);
-	return i915_gem_idle(dev);
+	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;
+
+	return ret;
 }
 
 void
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5aeb447..64eda44 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -973,7 +973,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = -EBUSY;
 		goto pre_mutex_err;
-- 
1.7.11.7

Re: [PATCH] drm/i915: don't frob mm.suspended when not using ums

[Chris Wilson]

On Tue, Jul 09, 2013 at 11:45:04AM +0200, Daniel Vetter wrote:
> In kernel modeset driver mode we're in full control of the chip,
> always. So there's no need at all to set mm.suspended in
> i915_gem_idle. Hence move that out into the leavevt ioctl. Since
> i915_gem_idle doesn't suspend gem any more we can also drop the
> re-enabling for KMS in the thaw function.
> 
> Also clean up the handling of mm.suspend at driver load by coalescing
> all the assignments.
> 
> Stumbled over while reading through our resume code for unrelated
> reasons.
> 
> v2: Shovel mm.suspended into the (newly created) ums dungeon as
> suggested by Chris Wilson. The plan is that once we've completely
> stopped relying on the register save/restore code we could shovel even
> that in there.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Minor comments inline. Doing the rename in the same patch made it much
easier to check all the users,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_dma.c            | 16 +++++++--------
>  drivers/gpu/drm/i915/i915_drv.c            |  5 ++---
>  drivers/gpu/drm/i915/i915_drv.h            | 24 +++++++++++++----------
>  drivers/gpu/drm/i915/i915_gem.c            | 31 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
>  5 files changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 0e22142..beca2d0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	/* Always safe in the mode setting case. */
>  	/* FIXME: do pre/post-mode set stuff in core KMS code */
>  	dev->vblank_disable_allowed = 1;
> -	if (INTEL_INFO(dev)->num_pipes == 0) {
> -		dev_priv->mm.suspended = 0;
> +	if (INTEL_INFO(dev)->num_pipes == 0)
>  		return 0;
> -	}
>  
>  	ret = intel_fbdev_init(dev);
>  	if (ret)
> @@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	drm_kms_helper_poll_init(dev);
>  
> -	/* We're off and running w/KMS */
> -	dev_priv->mm.suspended = 0;
> -
>  	return 0;
>  
>  cleanup_gem:
> @@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  			goto out_gem_unload;
>  	}
>  
> -	/* Start out suspended */
> -	dev_priv->mm.suspended = 1;
> -
>  	if (HAS_POWER_WELL(dev))
>  		i915_init_power_well(dev);
>  
> @@ -1641,6 +1633,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  			DRM_ERROR("failed to init modeset\n");
>  			goto out_gem_unload;
>  		}
> +
> +		/* We're off and running w/KMS */
> +		dev_priv->ums.mm_suspended = 0;

Just don't bother touching it here (we kzalloc our driver private).

> +	} else {
> +		/* Start out suspended in ums mode. */
> +		dev_priv->ums.mm_suspended = 1;
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e6dc81c..50a76f59 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -661,7 +661,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  		intel_init_pch_refclk(dev);
>  
>  		mutex_lock(&dev->struct_mutex);
> -		dev_priv->mm.suspended = 0;
>  
>  		error = i915_gem_init_hw(dev);
>  		mutex_unlock(&dev->struct_mutex);
> @@ -960,11 +959,11 @@ int i915_reset(struct drm_device *dev)
>  	 * switched away).
>  	 */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> -			!dev_priv->mm.suspended) {
> +			!dev_priv->ums.mm_suspended) {
>  		struct intel_ring_buffer *ring;
>  		int i;
>  
> -		dev_priv->mm.suspended = 0;
> +		dev_priv->ums.mm_suspended = 0;
>  
>  		i915_gem_init_swizzling(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c8d6104..be2fcd3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -814,6 +814,18 @@ struct i915_dri1_state {
>  	uint32_t counter;
>  };
>  
> +struct i915_ums_state {
> +	/**
> +	 * Flag if the X Server, and thus DRM, is not currently in
> +	 * control of the device.
> +	 *
> +	 * This is set between LeaveVT and EnterVT.  It needs to be
> +	 * replaced with a semaphore.  It also needs to be
> +	 * transitioned away from for kernel modesetting.
> +	 */
> +	int mm_suspended;
> +};
> +
>  struct intel_l3_parity {
>  	u32 *remap_info;
>  	struct work_struct error_work;
> @@ -884,16 +896,6 @@ struct i915_gem_mm {
>  	 */
>  	bool interruptible;
>  
> -	/**
> -	 * Flag if the X Server, and thus DRM, is not currently in
> -	 * control of the device.
> -	 *
> -	 * This is set between LeaveVT and EnterVT.  It needs to be
> -	 * replaced with a semaphore.  It also needs to be
> -	 * transitioned away from for kernel modesetting.
> -	 */
> -	int suspended;
> -
>  	/** Bit 6 swizzling required for X tiling */
>  	uint32_t bit_6_swizzle_x;
>  	/** Bit 6 swizzling required for Y tiling */
> @@ -1187,6 +1189,8 @@ typedef struct drm_i915_private {
>  	/* Old dri1 support infrastructure, beware the dragons ya fools entering
>  	 * here! */
>  	struct i915_dri1_state dri1;
> +	/* Old ums support infrastructure, same warning applies. */
> +	struct i915_ums_state ums;
>  } drm_i915_private_t;
>  
>  /* Iterate over initialised rings */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index af61be8..5ad9f35 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2082,7 +2082,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>  	trace_i915_gem_request_add(ring, request->seqno);
>  	ring->outstanding_lazy_request = 0;
>  
> -	if (!dev_priv->mm.suspended) {
> +	if (!dev_priv->ums.mm_suspended) {
>  		if (i915_enable_hangcheck) {
>  			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
>  				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
> @@ -2387,7 +2387,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  		idle &= list_empty(&ring->request_list);
>  	}
>  
> -	if (!dev_priv->mm.suspended && !idle)
> +	if (!dev_priv->ums.mm_suspended && !idle)
>  		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
>  				   round_jiffies_up_relative(HZ));
>  	if (idle)
> @@ -3983,7 +3983,7 @@ i915_gem_idle(struct drm_device *dev)
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	if (dev_priv->mm.suspended) {
> +	if (dev_priv->ums.mm_suspended) {
>  		mutex_unlock(&dev->struct_mutex);
>  		return 0;
>  	}
> @@ -3999,11 +3999,6 @@ i915_gem_idle(struct drm_device *dev)
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		i915_gem_evict_everything(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 mm.suspended!
> -	 */
> -	dev_priv->mm.suspended = 1;
>  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>  
>  	i915_kernel_lost_context(dev);
> @@ -4220,7 +4215,7 @@ int
>  i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
>  		       struct drm_file *file_priv)
>  {
> -	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
> @@ -4232,7 +4227,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	mutex_lock(&dev->struct_mutex);
> -	dev_priv->mm.suspended = 0;
> +	dev_priv->ums.mm_suspended = 0;
>  
>  	ret = i915_gem_init_hw(dev);
>  	if (ret != 0) {
> @@ -4252,7 +4247,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
>  cleanup_ringbuffer:
>  	mutex_lock(&dev->struct_mutex);
>  	i915_gem_cleanup_ringbuffer(dev);
> -	dev_priv->mm.suspended = 1;
> +	dev_priv->ums.mm_suspended = 1;
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	return ret;
> @@ -4262,11 +4257,23 @@ 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);
> -	return i915_gem_idle(dev);
> +	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;

ARGH. The locking, it hurts. Please never show this code in public again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: don't frob mm.suspended when not using ums

[Daniel Vetter]

In kernel modeset driver mode we're in full control of the chip,
always. So there's no need at all to set mm.suspended in
i915_gem_idle. Hence move that out into the leavevt ioctl. Since
i915_gem_idle doesn't suspend gem any more we can also drop the
re-enabling for KMS in the thaw function.

Also clean up the handling of mm.suspend at driver load by coalescing
all the assignments.

Stumbled over while reading through our resume code for unrelated
reasons.

v2: Shovel mm.suspended into the (newly created) ums dungeon as
suggested by Chris Wilson. The plan is that once we've completely
stopped relying on the register save/restore code we could shovel even
that in there.

v3: Improve the locking for the entervt/leavevt ioctls a bit by moving
the dev->struct_mutex locking outside of i915_gem_idle. Also don't
clear dev_priv->ums.mm_suspended for the kms case, we allocate it with
kzalloc. Both suggested by Chris Wilson.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c            | 13 +++-------
 drivers/gpu/drm/i915/i915_drv.c            | 11 +++++---
 drivers/gpu/drm/i915/i915_drv.h            | 24 ++++++++++--------
 drivers/gpu/drm/i915/i915_gem.c            | 40 ++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 5 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..bece997 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
-	if (INTEL_INFO(dev)->num_pipes == 0) {
-		dev_priv->mm.suspended = 0;
+	if (INTEL_INFO(dev)->num_pipes == 0)
 		return 0;
-	}
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
@@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	drm_kms_helper_poll_init(dev);
 
-	/* We're off and running w/KMS */
-	dev_priv->mm.suspended = 0;
-
 	return 0;
 
 cleanup_gem:
@@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	/* Start out suspended */
-	dev_priv->mm.suspended = 1;
-
 	if (HAS_POWER_WELL(dev))
 		i915_init_power_well(dev);
 
@@ -1641,6 +1633,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			DRM_ERROR("failed to init modeset\n");
 			goto out_gem_unload;
 		}
+	} else {
+		/* Start out suspended in ums mode. */
+		dev_priv->ums.mm_suspended = 1;
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e6dc81c..9580e6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -556,7 +556,11 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	/* If KMS is active, we do the leavevt stuff here */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		int error = i915_gem_idle(dev);
+		int error;
+
+		mutex_lock(&dev->struct_mutex);
+		error = i915_gem_idle(dev);
+		mutex_unlock(&dev->struct_mutex);
 		if (error) {
 			dev_err(&dev->pdev->dev,
 				"GEM idle failed, resume might fail\n");
@@ -661,7 +665,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		intel_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
-		dev_priv->mm.suspended = 0;
 
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
@@ -960,11 +963,11 @@ int i915_reset(struct drm_device *dev)
 	 * switched away).
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
-			!dev_priv->mm.suspended) {
+			!dev_priv->ums.mm_suspended) {
 		struct intel_ring_buffer *ring;
 		int i;
 
-		dev_priv->mm.suspended = 0;
+		dev_priv->ums.mm_suspended = 0;
 
 		i915_gem_init_swizzling(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8d6104..be2fcd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -814,6 +814,18 @@ struct i915_dri1_state {
 	uint32_t counter;
 };
 
+struct i915_ums_state {
+	/**
+	 * Flag if the X Server, and thus DRM, is not currently in
+	 * control of the device.
+	 *
+	 * This is set between LeaveVT and EnterVT.  It needs to be
+	 * replaced with a semaphore.  It also needs to be
+	 * transitioned away from for kernel modesetting.
+	 */
+	int mm_suspended;
+};
+
 struct intel_l3_parity {
 	u32 *remap_info;
 	struct work_struct error_work;
@@ -884,16 +896,6 @@ struct i915_gem_mm {
 	 */
 	bool interruptible;
 
-	/**
-	 * Flag if the X Server, and thus DRM, is not currently in
-	 * control of the device.
-	 *
-	 * This is set between LeaveVT and EnterVT.  It needs to be
-	 * replaced with a semaphore.  It also needs to be
-	 * transitioned away from for kernel modesetting.
-	 */
-	int suspended;
-
 	/** Bit 6 swizzling required for X tiling */
 	uint32_t bit_6_swizzle_x;
 	/** Bit 6 swizzling required for Y tiling */
@@ -1187,6 +1189,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+	/* Old ums support infrastructure, same warning applies. */
+	struct i915_ums_state ums;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index af61be8..cb400b8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2082,7 +2082,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	trace_i915_gem_request_add(ring, request->seqno);
 	ring->outstanding_lazy_request = 0;
 
-	if (!dev_priv->mm.suspended) {
+	if (!dev_priv->ums.mm_suspended) {
 		if (i915_enable_hangcheck) {
 			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
 				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
@@ -2387,7 +2387,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 		idle &= list_empty(&ring->request_list);
 	}
 
-	if (!dev_priv->mm.suspended && !idle)
+	if (!dev_priv->ums.mm_suspended && !idle)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
 				   round_jiffies_up_relative(HZ));
 	if (idle)
@@ -3981,9 +3981,7 @@ i915_gem_idle(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
-
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		return 0;
 	}
@@ -3999,18 +3997,11 @@ i915_gem_idle(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_gem_evict_everything(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 mm.suspended!
-	 */
-	dev_priv->mm.suspended = 1;
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 
 	i915_kernel_lost_context(dev);
 	i915_gem_cleanup_ringbuffer(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);
 
@@ -4220,7 +4211,7 @@ int
 i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -4232,7 +4223,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	dev_priv->mm.suspended = 0;
+	dev_priv->ums.mm_suspended = 0;
 
 	ret = i915_gem_init_hw(dev);
 	if (ret != 0) {
@@ -4252,7 +4243,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 cleanup_ringbuffer:
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_cleanup_ringbuffer(dev);
-	dev_priv->mm.suspended = 1;
+	dev_priv->ums.mm_suspended = 1;
 	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
@@ -4262,11 +4253,26 @@ 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);
-	return i915_gem_idle(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;
 }
 
 void
@@ -4277,9 +4283,11 @@ 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);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5aeb447..64eda44 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -973,7 +973,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = -EBUSY;
 		goto pre_mutex_err;
-- 
1.7.11.7

Re: [PATCH] drm/i915: don't frob mm.suspended when not using ums

[Daniel Vetter]

On Tue, Jul 09, 2013 at 02:00:51PM +0100, Chris Wilson wrote:
> On Tue, Jul 09, 2013 at 11:45:04AM +0200, Daniel Vetter wrote:
> > In kernel modeset driver mode we're in full control of the chip,
> > always. So there's no need at all to set mm.suspended in
> > i915_gem_idle. Hence move that out into the leavevt ioctl. Since
> > i915_gem_idle doesn't suspend gem any more we can also drop the
> > re-enabling for KMS in the thaw function.
> > 
> > Also clean up the handling of mm.suspend at driver load by coalescing
> > all the assignments.
> > 
> > Stumbled over while reading through our resume code for unrelated
> > reasons.
> > 
> > v2: Shovel mm.suspended into the (newly created) ums dungeon as
> > suggested by Chris Wilson. The plan is that once we've completely
> > stopped relying on the register save/restore code we could shovel even
> > that in there.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Minor comments inline. Doing the rename in the same patch made it much
> easier to check all the users,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I've merged the updated version with the nitpicks fixed, thanks for your
review.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c            | 16 +++++++--------
> >  drivers/gpu/drm/i915/i915_drv.c            |  5 ++---
> >  drivers/gpu/drm/i915/i915_drv.h            | 24 +++++++++++++----------
> >  drivers/gpu/drm/i915/i915_gem.c            | 31 ++++++++++++++++++------------
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
> >  5 files changed, 43 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 0e22142..beca2d0 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	/* Always safe in the mode setting case. */
> >  	/* FIXME: do pre/post-mode set stuff in core KMS code */
> >  	dev->vblank_disable_allowed = 1;
> > -	if (INTEL_INFO(dev)->num_pipes == 0) {
> > -		dev_priv->mm.suspended = 0;
> > +	if (INTEL_INFO(dev)->num_pipes == 0)
> >  		return 0;
> > -	}
> >  
> >  	ret = intel_fbdev_init(dev);
> >  	if (ret)
> > @@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  
> >  	drm_kms_helper_poll_init(dev);
> >  
> > -	/* We're off and running w/KMS */
> > -	dev_priv->mm.suspended = 0;
> > -
> >  	return 0;
> >  
> >  cleanup_gem:
> > @@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  			goto out_gem_unload;
> >  	}
> >  
> > -	/* Start out suspended */
> > -	dev_priv->mm.suspended = 1;
> > -
> >  	if (HAS_POWER_WELL(dev))
> >  		i915_init_power_well(dev);
> >  
> > @@ -1641,6 +1633,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  			DRM_ERROR("failed to init modeset\n");
> >  			goto out_gem_unload;
> >  		}
> > +
> > +		/* We're off and running w/KMS */
> > +		dev_priv->ums.mm_suspended = 0;
> 
> Just don't bother touching it here (we kzalloc our driver private).
> 
> > +	} else {
> > +		/* Start out suspended in ums mode. */
> > +		dev_priv->ums.mm_suspended = 1;
> >  	}
> >  
> >  	i915_setup_sysfs(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e6dc81c..50a76f59 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -661,7 +661,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >  		intel_init_pch_refclk(dev);
> >  
> >  		mutex_lock(&dev->struct_mutex);
> > -		dev_priv->mm.suspended = 0;
> >  
> >  		error = i915_gem_init_hw(dev);
> >  		mutex_unlock(&dev->struct_mutex);
> > @@ -960,11 +959,11 @@ int i915_reset(struct drm_device *dev)
> >  	 * switched away).
> >  	 */
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> > -			!dev_priv->mm.suspended) {
> > +			!dev_priv->ums.mm_suspended) {
> >  		struct intel_ring_buffer *ring;
> >  		int i;
> >  
> > -		dev_priv->mm.suspended = 0;
> > +		dev_priv->ums.mm_suspended = 0;
> >  
> >  		i915_gem_init_swizzling(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c8d6104..be2fcd3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -814,6 +814,18 @@ struct i915_dri1_state {
> >  	uint32_t counter;
> >  };
> >  
> > +struct i915_ums_state {
> > +	/**
> > +	 * Flag if the X Server, and thus DRM, is not currently in
> > +	 * control of the device.
> > +	 *
> > +	 * This is set between LeaveVT and EnterVT.  It needs to be
> > +	 * replaced with a semaphore.  It also needs to be
> > +	 * transitioned away from for kernel modesetting.
> > +	 */
> > +	int mm_suspended;
> > +};
> > +
> >  struct intel_l3_parity {
> >  	u32 *remap_info;
> >  	struct work_struct error_work;
> > @@ -884,16 +896,6 @@ struct i915_gem_mm {
> >  	 */
> >  	bool interruptible;
> >  
> > -	/**
> > -	 * Flag if the X Server, and thus DRM, is not currently in
> > -	 * control of the device.
> > -	 *
> > -	 * This is set between LeaveVT and EnterVT.  It needs to be
> > -	 * replaced with a semaphore.  It also needs to be
> > -	 * transitioned away from for kernel modesetting.
> > -	 */
> > -	int suspended;
> > -
> >  	/** Bit 6 swizzling required for X tiling */
> >  	uint32_t bit_6_swizzle_x;
> >  	/** Bit 6 swizzling required for Y tiling */
> > @@ -1187,6 +1189,8 @@ typedef struct drm_i915_private {
> >  	/* Old dri1 support infrastructure, beware the dragons ya fools entering
> >  	 * here! */
> >  	struct i915_dri1_state dri1;
> > +	/* Old ums support infrastructure, same warning applies. */
> > +	struct i915_ums_state ums;
> >  } drm_i915_private_t;
> >  
> >  /* Iterate over initialised rings */
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index af61be8..5ad9f35 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2082,7 +2082,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
> >  	trace_i915_gem_request_add(ring, request->seqno);
> >  	ring->outstanding_lazy_request = 0;
> >  
> > -	if (!dev_priv->mm.suspended) {
> > +	if (!dev_priv->ums.mm_suspended) {
> >  		if (i915_enable_hangcheck) {
> >  			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
> >  				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
> > @@ -2387,7 +2387,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
> >  		idle &= list_empty(&ring->request_list);
> >  	}
> >  
> > -	if (!dev_priv->mm.suspended && !idle)
> > +	if (!dev_priv->ums.mm_suspended && !idle)
> >  		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
> >  				   round_jiffies_up_relative(HZ));
> >  	if (idle)
> > @@ -3983,7 +3983,7 @@ i915_gem_idle(struct drm_device *dev)
> >  
> >  	mutex_lock(&dev->struct_mutex);
> >  
> > -	if (dev_priv->mm.suspended) {
> > +	if (dev_priv->ums.mm_suspended) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		return 0;
> >  	}
> > @@ -3999,11 +3999,6 @@ i915_gem_idle(struct drm_device *dev)
> >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  		i915_gem_evict_everything(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 mm.suspended!
> > -	 */
> > -	dev_priv->mm.suspended = 1;
> >  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> >  
> >  	i915_kernel_lost_context(dev);
> > @@ -4220,7 +4215,7 @@ int
> >  i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  		       struct drm_file *file_priv)
> >  {
> > -	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET))
> > @@ -4232,7 +4227,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  	}
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > -	dev_priv->mm.suspended = 0;
> > +	dev_priv->ums.mm_suspended = 0;
> >  
> >  	ret = i915_gem_init_hw(dev);
> >  	if (ret != 0) {
> > @@ -4252,7 +4247,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  cleanup_ringbuffer:
> >  	mutex_lock(&dev->struct_mutex);
> >  	i915_gem_cleanup_ringbuffer(dev);
> > -	dev_priv->mm.suspended = 1;
> > +	dev_priv->ums.mm_suspended = 1;
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	return ret;
> > @@ -4262,11 +4257,23 @@ 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);
> > -	return i915_gem_idle(dev);
> > +	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;
> 
> ARGH. The locking, it hurts. Please never show this code in public again.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

[PATCH] drm/i915: don't frob mm.suspended when not using ums

[Daniel Vetter]

In kernel modeset driver mode we're in full control of the chip,
always. So there's no need at all to set mm.suspended in
i915_gem_idle. Hence move that out into the leavevt ioctl. Since
i915_gem_idle doesn't suspend gem any more we can also drop the
re-enabling for KMS in the thaw function.

Also clean up the handling of mm.suspend at driver load by coalescing
all the assignments.

Stumbled over while reading through our resume code for unrelated
reasons.

v2: Shovel mm.suspended into the (newly created) ums dungeon as
suggested by Chris Wilson. The plan is that once we've completely
stopped relying on the register save/restore code we could shovel even
that in there.

v3: Improve the locking for the entervt/leavevt ioctls a bit by moving
the dev->struct_mutex locking outside of i915_gem_idle. Also don't
clear dev_priv->ums.mm_suspended for the kms case, we allocate it with
kzalloc. Both suggested by Chris Wilson.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c            | 13 +++-------
 drivers/gpu/drm/i915/i915_drv.c            | 11 +++++---
 drivers/gpu/drm/i915/i915_drv.h            | 24 ++++++++++--------
 drivers/gpu/drm/i915/i915_gem.c            | 40 ++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 5 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..bece997 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1323,10 +1323,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
-	if (INTEL_INFO(dev)->num_pipes == 0) {
-		dev_priv->mm.suspended = 0;
+	if (INTEL_INFO(dev)->num_pipes == 0)
 		return 0;
-	}
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
@@ -1352,9 +1350,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	drm_kms_helper_poll_init(dev);
 
-	/* We're off and running w/KMS */
-	dev_priv->mm.suspended = 0;
-
 	return 0;
 
 cleanup_gem:
@@ -1629,9 +1624,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	/* Start out suspended */
-	dev_priv->mm.suspended = 1;
-
 	if (HAS_POWER_WELL(dev))
 		i915_init_power_well(dev);
 
@@ -1641,6 +1633,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			DRM_ERROR("failed to init modeset\n");
 			goto out_gem_unload;
 		}
+	} else {
+		/* Start out suspended in ums mode. */
+		dev_priv->ums.mm_suspended = 1;
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e6dc81c..9580e6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -556,7 +556,11 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	/* If KMS is active, we do the leavevt stuff here */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		int error = i915_gem_idle(dev);
+		int error;
+
+		mutex_lock(&dev->struct_mutex);
+		error = i915_gem_idle(dev);
+		mutex_unlock(&dev->struct_mutex);
 		if (error) {
 			dev_err(&dev->pdev->dev,
 				"GEM idle failed, resume might fail\n");
@@ -661,7 +665,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		intel_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
-		dev_priv->mm.suspended = 0;
 
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
@@ -960,11 +963,11 @@ int i915_reset(struct drm_device *dev)
 	 * switched away).
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
-			!dev_priv->mm.suspended) {
+			!dev_priv->ums.mm_suspended) {
 		struct intel_ring_buffer *ring;
 		int i;
 
-		dev_priv->mm.suspended = 0;
+		dev_priv->ums.mm_suspended = 0;
 
 		i915_gem_init_swizzling(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8d6104..be2fcd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -814,6 +814,18 @@ struct i915_dri1_state {
 	uint32_t counter;
 };
 
+struct i915_ums_state {
+	/**
+	 * Flag if the X Server, and thus DRM, is not currently in
+	 * control of the device.
+	 *
+	 * This is set between LeaveVT and EnterVT.  It needs to be
+	 * replaced with a semaphore.  It also needs to be
+	 * transitioned away from for kernel modesetting.
+	 */
+	int mm_suspended;
+};
+
 struct intel_l3_parity {
 	u32 *remap_info;
 	struct work_struct error_work;
@@ -884,16 +896,6 @@ struct i915_gem_mm {
 	 */
 	bool interruptible;
 
-	/**
-	 * Flag if the X Server, and thus DRM, is not currently in
-	 * control of the device.
-	 *
-	 * This is set between LeaveVT and EnterVT.  It needs to be
-	 * replaced with a semaphore.  It also needs to be
-	 * transitioned away from for kernel modesetting.
-	 */
-	int suspended;
-
 	/** Bit 6 swizzling required for X tiling */
 	uint32_t bit_6_swizzle_x;
 	/** Bit 6 swizzling required for Y tiling */
@@ -1187,6 +1189,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+	/* Old ums support infrastructure, same warning applies. */
+	struct i915_ums_state ums;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index af61be8..cb400b8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2082,7 +2082,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	trace_i915_gem_request_add(ring, request->seqno);
 	ring->outstanding_lazy_request = 0;
 
-	if (!dev_priv->mm.suspended) {
+	if (!dev_priv->ums.mm_suspended) {
 		if (i915_enable_hangcheck) {
 			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
 				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
@@ -2387,7 +2387,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 		idle &= list_empty(&ring->request_list);
 	}
 
-	if (!dev_priv->mm.suspended && !idle)
+	if (!dev_priv->ums.mm_suspended && !idle)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
 				   round_jiffies_up_relative(HZ));
 	if (idle)
@@ -3981,9 +3981,7 @@ i915_gem_idle(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
-
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		return 0;
 	}
@@ -3999,18 +3997,11 @@ i915_gem_idle(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_gem_evict_everything(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 mm.suspended!
-	 */
-	dev_priv->mm.suspended = 1;
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 
 	i915_kernel_lost_context(dev);
 	i915_gem_cleanup_ringbuffer(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);
 
@@ -4220,7 +4211,7 @@ int
 i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -4232,7 +4223,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	dev_priv->mm.suspended = 0;
+	dev_priv->ums.mm_suspended = 0;
 
 	ret = i915_gem_init_hw(dev);
 	if (ret != 0) {
@@ -4252,7 +4243,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 cleanup_ringbuffer:
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_cleanup_ringbuffer(dev);
-	dev_priv->mm.suspended = 1;
+	dev_priv->ums.mm_suspended = 1;
 	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
@@ -4262,11 +4253,26 @@ 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);
-	return i915_gem_idle(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;
 }
 
 void
@@ -4277,9 +4283,11 @@ 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);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5aeb447..64eda44 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -973,7 +973,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
-	if (dev_priv->mm.suspended) {
+	if (dev_priv->ums.mm_suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = -EBUSY;
 		goto pre_mutex_err;
-- 
1.7.11.7