[PATCH] drm/i915: hangcheck parameter

[PATCH] drm/i915: hangcheck parameter

[Ben Widawsky]

The hangcheck is undesirable when doing shader debugging. The debugger
interacts with the EU, and these may cause the hangcheck to fire at most
unfortunate times.

This provides a way to let the user disable the hangcheck when they want
to do shader debugging.

Not Signed-off-by, for review only
I will be resubmitting this with full series is ready
---
 drivers/gpu/drm/i915/i915_dma.c |   13 +++++++++----
 drivers/gpu/drm/i915/i915_drv.c |    3 +++
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |   10 +++++++---
 drivers/gpu/drm/i915/i915_irq.c |   16 +++++++++++-----
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0239e99..b10249a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -43,6 +43,8 @@
 #include <linux/slab.h>
 #include <acpi/video.h>
 
+extern unsigned int i915_enable_hangcheck;
+
 static void i915_write_hws_pga(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -2089,9 +2091,11 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	/* Must be done after probing outputs */
 	intel_opregion_init(dev);
 	acpi_video_register();
-
-	setup_timer(&dev_priv->hangcheck_timer, i915_hangcheck_elapsed,
-		    (unsigned long) dev);
+	if (i915_enable_hangcheck) {
+		dev_priv->enable_hangcheck = true;
+		setup_timer(&dev_priv->hangcheck_timer, i915_hangcheck_elapsed,
+			    (unsigned long) dev);
+	}
 
 	spin_lock(&mchdev_lock);
 	i915_mch_dev = dev_priv;
@@ -2169,7 +2173,8 @@ int i915_driver_unload(struct drm_device *dev)
 	}
 
 	/* Free error state after interrupts are fully disabled. */
-	del_timer_sync(&dev_priv->hangcheck_timer);
+	if (dev_priv->enable_hangcheck)
+		del_timer_sync(&dev_priv->hangcheck_timer);
 	cancel_work_sync(&dev_priv->error_work);
 	i915_destroy_error_state(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0defd42..a38ce4f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -70,6 +70,9 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
 static bool i915_try_reset = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
 
+unsigned int i915_enable_hangcheck = 1;
+module_param_named(enable_hangcheck, i915_enable_hangcheck, int, 0600);
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a9fd91..bfced70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -320,6 +320,7 @@ typedef struct drm_i915_private {
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
+	bool enable_hangcheck;
 	uint32_t last_acthd;
 	uint32_t last_instdone;
 	uint32_t last_instdone1;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb1f61d..71377b1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1777,8 +1777,11 @@ i915_add_request(struct intel_ring_buffer *ring,
 	ring->outstanding_lazy_request = false;
 
 	if (!dev_priv->mm.suspended) {
-		mod_timer(&dev_priv->hangcheck_timer,
-			  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		if (dev_priv->enable_hangcheck) {
+			mod_timer(&dev_priv->hangcheck_timer,
+				  jiffies +
+				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		}
 		if (was_empty)
 			queue_delayed_work(dev_priv->wq,
 					   &dev_priv->mm.retire_work, HZ);
@@ -3813,7 +3816,8 @@ i915_gem_idle(struct drm_device *dev)
 	 * And not confound mm.suspended!
 	 */
 	dev_priv->mm.suspended = 1;
-	del_timer_sync(&dev_priv->hangcheck_timer);
+	if (dev_priv->enable_hangcheck)
+		del_timer_sync(&dev_priv->hangcheck_timer);
 
 	i915_kernel_lost_context(dev);
 	i915_gem_cleanup_ringbuffer(dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b9fafe3..73b7071 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -362,9 +362,12 @@ static void notify_ring(struct drm_device *dev,
 	ring->irq_seqno = seqno;
 	wake_up_all(&ring->irq_queue);
 
-	dev_priv->hangcheck_count = 0;
-	mod_timer(&dev_priv->hangcheck_timer,
-		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	if (dev_priv->enable_hangcheck) {
+		dev_priv->hangcheck_count = 0;
+		mod_timer(&dev_priv->hangcheck_timer,
+			  jiffies +
+			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	}
 }
 
 static void gen6_pm_rps_work(struct work_struct *work)
@@ -1722,8 +1725,11 @@ void i915_hangcheck_elapsed(unsigned long data)
 
 repeat:
 	/* Reset timer case chip hangs without another request being added */
-	mod_timer(&dev_priv->hangcheck_timer,
-		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	if (dev_priv->enable_hangcheck) {
+		mod_timer(&dev_priv->hangcheck_timer,
+			  jiffies +
+			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	}
 }
 
 /* drm_dma.h hooks
-- 
1.7.5.2

Re: [PATCH] drm/i915: hangcheck parameter

[Chris Wilson]

On Wed, 22 Jun 2011 10:32:50 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The hangcheck is undesirable when doing shader debugging. The debugger
> interacts with the EU, and these may cause the hangcheck to fire at most
> unfortunate times.
> 
> This provides a way to let the user disable the hangcheck when they want
> to do shader debugging.

Looks like a reasonable idea, I'm tempted to suggest to make it the period
in ms rather a boolean. But for actual use, either a new ioctl for
enable/disable or another wakelock? Just in case we have more than one
program trying to debug at the same time. (Not so far fetched, as the user
may be snooping on the results whilst debugging in another process?)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: hangcheck parameter

[Ben Widawsky]

On Wed, Jun 22, 2011 at 06:45:56PM +0100, Chris Wilson wrote:
> On Wed, 22 Jun 2011 10:32:50 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > The hangcheck is undesirable when doing shader debugging. The debugger
> > interacts with the EU, and these may cause the hangcheck to fire at most
> > unfortunate times.
> > 
> > This provides a way to let the user disable the hangcheck when they want
> > to do shader debugging.
> 
> Looks like a reasonable idea, I'm tempted to suggest to make it the period
> in ms rather a boolean. But for actual use, either a new ioctl for
> enable/disable or another wakelock? Just in case we have more than one
> program trying to debug at the same time. (Not so far fetched, as the user
> may be snooping on the results whilst debugging in another process?)
> 
> -Chris

Ah, Chris! You reminded me what I had planned to do a month ago, but it
got lost in all the other stuff floating around in my brain.

Yes, actually I will make a debugfs file, which notifies the kernel
we're debugging from userspace, and it should disable the hangcheck
timer. This will allow us to read/write potentially valuable information
in the future, and like you said acts as a lock to prevent concurrent
debuggers.

Thanks! Very helpful response/reminder.

I was planning a debugfs entry for it. Do you see any advantages to
using an IOCTL?

Ben

Re: [PATCH] drm/i915: hangcheck parameter

[Chris Wilson]

On Wed, 22 Jun 2011 10:55:33 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I was planning a debugfs entry for it. Do you see any advantages to
> using an IOCTL?

The only danger is that we developing a useful api and hiding it debugfs.
We may be caught and forced to maintain such even if we do declare it as a
work-in-progress...

I'm not that worried about such just yet, we are a long way from having a
mature enough hw and gfx stack to support a ptrace-like interface for
generic GPU debugging.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915: hangcheck disable flag

[Ben Widawsky]

Provide a flag to disable the hangcheck timer
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |    6 ++++--
 drivers/gpu/drm/i915/i915_irq.c |    9 ++++++---
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a9fd91..05f82a7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -320,6 +320,7 @@ typedef struct drm_i915_private {
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
+	bool hangcheck_disabled;
 	uint32_t last_acthd;
 	uint32_t last_instdone;
 	uint32_t last_instdone1;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb1f61d..d50546c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1777,8 +1777,10 @@ i915_add_request(struct intel_ring_buffer *ring,
 	ring->outstanding_lazy_request = false;
 
 	if (!dev_priv->mm.suspended) {
-		mod_timer(&dev_priv->hangcheck_timer,
-			  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		if (!dev_priv->hangcheck_disabled) {
+			mod_timer(&dev_priv->hangcheck_timer, jiffies +
+				msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		}
 		if (was_empty)
 			queue_delayed_work(dev_priv->wq,
 					   &dev_priv->mm.retire_work, HZ);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b9fafe3..af72412 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -362,9 +362,11 @@ static void notify_ring(struct drm_device *dev,
 	ring->irq_seqno = seqno;
 	wake_up_all(&ring->irq_queue);
 
-	dev_priv->hangcheck_count = 0;
-	mod_timer(&dev_priv->hangcheck_timer,
-		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	if (!dev_priv->hangcheck_disabled) {
+		dev_priv->hangcheck_count = 0;
+		mod_timer(&dev_priv->hangcheck_timer, jiffies +
+			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+	}
 }
 
 static void gen6_pm_rps_work(struct work_struct *work)
@@ -1721,6 +1723,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 	}
 
 repeat:
+	BUG_ON(dev_priv->hangcheck_disabled);
 	/* Reset timer case chip hangs without another request being added */
 	mod_timer(&dev_priv->hangcheck_timer,
 		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
-- 
1.7.5.2

[PATCH 2/2] drm/i915: debugger debugfs entry

[Ben Widawsky]

Provide a way for userspace shader debugger to notify the kernel that it
will be debugging. This interface does two things, provides a way for
the kernel to prepare for debugging, and act as a lock between
concurrent debugging (of course none of this is enforced).
---
 drivers/gpu/drm/i915/i915_debugfs.c |   73 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |    6 +++
 drivers/gpu/drm/i915/i915_irq.c     |    2 +-
 3 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4d46441..ab6e2a2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1395,6 +1395,74 @@ static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
 }
 
+static int i915_debugger_open(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (atomic_add_return(1, &dev_priv->debug.debugging) != 1) {
+		atomic_dec(&dev_priv->debug.debugging);
+		return -EBUSY;
+	}
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret) {
+		atomic_dec(&dev_priv->debug.debugging);
+		return ret;
+	}
+
+	del_timer_sync(&dev_priv->hangcheck_timer);
+	dev_priv->hangcheck_disabled = true;
+
+	dev_priv->debug.debugger = current;
+
+	mutex_unlock(&dev->struct_mutex);
+	return 0;
+}
+
+static int i915_debugger_release(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev->struct_mutex);
+
+	if (WARN_ON(dev_priv->debug.debugger != current))
+		return -ENXIO;
+
+	WARN_ON(atomic_dec_and_test(&dev_priv->debug.debugging) == 0);
+
+	dev_priv->debug.debugger = NULL;
+	dev_priv->hangcheck_disabled = false;
+	mutex_unlock(&dev->struct_mutex);
+	return 0;
+}
+
+static const struct file_operations i915_debugger_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_debugger_open,
+	.release = i915_debugger_release,
+};
+
+static int i915_debugger_create(struct dentry *root, struct drm_minor *minor)
+{
+	struct drm_device *dev = minor->dev;
+	struct dentry *ent;
+
+	ent = debugfs_create_file("i915_debugger",
+				  S_IRUSR,
+				  root, dev,
+				  &i915_debugger_fops);
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);
+
+	return drm_add_fake_info_node(minor, ent, &i915_debugger_fops);
+
+	return 0;
+}
+
+
 static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -1448,6 +1516,9 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	ret = i915_debugger_create(minor->debugfs_root, minor);
+	if (ret)
+		return ret;
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
@@ -1457,6 +1528,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 {
 	drm_debugfs_remove_files(i915_debugfs_list,
 				 I915_DEBUGFS_ENTRIES, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_debugger_fops,
+				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
 				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_wedged_fops,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05f82a7..a4418eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -720,6 +720,12 @@ typedef struct drm_i915_private {
 	struct drm_property *force_audio_property;
 
 	atomic_t forcewake_count;
+
+	struct {
+		atomic_t debugging;
+		struct task_struct *debugger;
+	} debug;
+
 } drm_i915_private_t;
 
 enum i915_cache_level {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index af72412..ca8ec0f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1723,7 +1723,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 	}
 
 repeat:
-	BUG_ON(dev_priv->hangcheck_disabled);
+	WARN_ON(dev_priv->hangcheck_disabled);
 	/* Reset timer case chip hangs without another request being added */
 	mod_timer(&dev_priv->hangcheck_timer,
 		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
-- 
1.7.5.2