[PATCH 1/3] drm/i915: refactor debugfs open function

[PATCH 1/3] drm/i915: refactor debugfs open function

[Daniel Vetter]

Only forcewake has an open with special semantics, the other r/w
debugfs only assign the file private pointer.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   26 +++++---------------------
 1 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5ba63ad..fd0e817 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1328,8 +1328,8 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
 }
 
 static int
-i915_wedged_open(struct inode *inode,
-		 struct file *filp)
+i915_debugfs_common_open(struct inode *inode,
+			 struct file *filp)
 {
 	filp->private_data = inode->i_private;
 	return 0;
@@ -1385,20 +1385,12 @@ i915_wedged_write(struct file *filp,
 
 static const struct file_operations i915_wedged_fops = {
 	.owner = THIS_MODULE,
-	.open = i915_wedged_open,
+	.open = i915_debugfs_common_open,
 	.read = i915_wedged_read,
 	.write = i915_wedged_write,
 	.llseek = default_llseek,
 };
 
-static int
-i915_max_freq_open(struct inode *inode,
-		   struct file *filp)
-{
-	filp->private_data = inode->i_private;
-	return 0;
-}
-
 static ssize_t
 i915_max_freq_read(struct file *filp,
 		   char __user *ubuf,
@@ -1455,20 +1447,12 @@ i915_max_freq_write(struct file *filp,
 
 static const struct file_operations i915_max_freq_fops = {
 	.owner = THIS_MODULE,
-	.open = i915_max_freq_open,
+	.open = i915_debugfs_common_open,
 	.read = i915_max_freq_read,
 	.write = i915_max_freq_write,
 	.llseek = default_llseek,
 };
 
-static int
-i915_cache_sharing_open(struct inode *inode,
-		   struct file *filp)
-{
-	filp->private_data = inode->i_private;
-	return 0;
-}
-
 static ssize_t
 i915_cache_sharing_read(struct file *filp,
 		   char __user *ubuf,
@@ -1534,7 +1518,7 @@ i915_cache_sharing_write(struct file *filp,
 
 static const struct file_operations i915_cache_sharing_fops = {
 	.owner = THIS_MODULE,
-	.open = i915_cache_sharing_open,
+	.open = i915_debugfs_common_open,
 	.read = i915_cache_sharing_read,
 	.write = i915_cache_sharing_write,
 	.llseek = default_llseek,
-- 
1.7.6.4

[PATCH 2/3] drm/i915: refactor debugfs create functions

[Daniel Vetter]

All r/w debugfs files are created equal.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   53 ++++++++++------------------------
 1 files changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fd0e817..8476441 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1547,21 +1547,6 @@ drm_add_fake_info_node(struct drm_minor *minor,
 	return 0;
 }
 
-static int i915_wedged_create(struct dentry *root, struct drm_minor *minor)
-{
-	struct drm_device *dev = minor->dev;
-	struct dentry *ent;
-
-	ent = debugfs_create_file("i915_wedged",
-				  S_IRUGO | S_IWUSR,
-				  root, dev,
-				  &i915_wedged_fops);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-
-	return drm_add_fake_info_node(minor, ent, &i915_wedged_fops);
-}
-
 static int i915_forcewake_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *dev = inode->i_private;
@@ -1623,34 +1608,22 @@ 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_max_freq_create(struct dentry *root, struct drm_minor *minor)
-{
-	struct drm_device *dev = minor->dev;
-	struct dentry *ent;
-
-	ent = debugfs_create_file("i915_max_freq",
-				  S_IRUGO | S_IWUSR,
-				  root, dev,
-				  &i915_max_freq_fops);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-
-	return drm_add_fake_info_node(minor, ent, &i915_max_freq_fops);
-}
-
-static int i915_cache_sharing_create(struct dentry *root, struct drm_minor *minor)
+static int i915_debugfs_create(struct dentry *root,
+			       struct drm_minor *minor,
+			       const char *name,
+			       const struct file_operations *fops)
 {
 	struct drm_device *dev = minor->dev;
 	struct dentry *ent;
 
-	ent = debugfs_create_file("i915_cache_sharing",
+	ent = debugfs_create_file(name,
 				  S_IRUGO | S_IWUSR,
 				  root, dev,
-				  &i915_cache_sharing_fops);
+				  fops);
 	if (IS_ERR(ent))
 		return PTR_ERR(ent);
 
-	return drm_add_fake_info_node(minor, ent, &i915_cache_sharing_fops);
+	return drm_add_fake_info_node(minor, ent, fops);
 }
 
 static struct drm_info_list i915_debugfs_list[] = {
@@ -1699,17 +1672,23 @@ int i915_debugfs_init(struct drm_minor *minor)
 {
 	int ret;
 
-	ret = i915_wedged_create(minor->debugfs_root, minor);
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				  "i915_wedged",
+				  &i915_wedged_fops);
 	if (ret)
 		return ret;
 
 	ret = i915_forcewake_create(minor->debugfs_root, minor);
 	if (ret)
 		return ret;
-	ret = i915_max_freq_create(minor->debugfs_root, minor);
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				  "i915_max_freq",
+				  &i915_max_freq_fops);
 	if (ret)
 		return ret;
-	ret = i915_cache_sharing_create(minor->debugfs_root, minor);
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				  "i915_cache_sharing",
+				  &i915_cache_sharing_fops);
 	if (ret)
 		return ret;
 
-- 
1.7.6.4

[PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Daniel Vetter]

GPU reset is a very important piece of our infrastructure.
Unfortunately we only really test it by actually hanging the gpu,
which often has bad side-effects for the entire system. And the gpu
hang handling code is one of the rather complicated pieces of code we
have, consisting of
- hang detection
- error capture
- actual gpu reset
- reset of all the gem bookkeeping
- reinitialition of the entire gpu

This patch adds a debugfs to selectively stopping rings by ceasing to
update the hw tail pointer. This way we can exercise the gpu hang code
under controlled conditions without a dying gpu taking down the entire
systems.

Patch motivated by me forgetting to properly reinitialize ppgtt after
a gpu reset.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |   63 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c         |    3 +
 drivers/gpu/drm/i915/i915_drv.h         |    2 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++
 4 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8476441..9821a3b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1392,6 +1392,64 @@ static const struct file_operations i915_wedged_fops = {
 };
 
 static ssize_t
+i915_ring_stop_read(struct file *filp,
+		    char __user *ubuf,
+		    size_t max,
+		    loff_t *ppos)
+{
+	struct drm_device *dev = filp->private_data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	char buf[80];
+	int len;
+
+	len = snprintf(buf, sizeof(buf),
+		       "%d\n", dev_priv->stop_rings);
+
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
+	return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+}
+
+static ssize_t
+i915_ring_stop_write(struct file *filp,
+		     const char __user *ubuf,
+		     size_t cnt,
+		     loff_t *ppos)
+{
+	struct drm_device *dev = filp->private_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	char buf[20];
+	int val = 0;
+
+	if (cnt > 0) {
+		if (cnt > sizeof(buf) - 1)
+			return -EINVAL;
+
+		if (copy_from_user(buf, ubuf, cnt))
+			return -EFAULT;
+		buf[cnt] = 0;
+
+		val = simple_strtoul(buf, NULL, 0);
+	}
+
+	DRM_DEBUG_DRIVER("Stopping rings %u\n", val);
+
+	mutex_lock(&dev->struct_mutex);
+	dev_priv->stop_rings = val;
+	mutex_unlock(&dev->struct_mutex);
+
+	return cnt;
+}
+
+static const struct file_operations i915_ring_stop_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_debugfs_common_open,
+	.read = i915_ring_stop_read,
+	.write = i915_ring_stop_write,
+	.llseek = default_llseek,
+};
+static ssize_t
 i915_max_freq_read(struct file *filp,
 		   char __user *ubuf,
 		   size_t max,
@@ -1691,6 +1749,11 @@ int i915_debugfs_init(struct drm_minor *minor)
 				  &i915_cache_sharing_fops);
 	if (ret)
 		return ret;
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				  "i915_ring_stop",
+				  &i915_ring_stop_fops);
+	if (ret)
+		return ret;
 
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 548e04b..566cc1e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -611,6 +611,9 @@ int i915_reset(struct drm_device *dev, u8 flags)
 	if (!mutex_trylock(&dev->struct_mutex))
 		return -EBUSY;
 
+	printk("reenabling rings\n");
+	dev_priv->stop_rings = 0;
+
 	i915_gem_reset(dev);
 
 	ret = -ENODEV;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd98fb3..503ae8c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -330,6 +330,8 @@ typedef struct drm_i915_private {
 	uint32_t last_instdone;
 	uint32_t last_instdone1;
 
+	unsigned int stop_rings;
+
 	unsigned long cfb_size;
 	unsigned int cfb_fb;
 	enum plane cfb_plane;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3c30dba..ef7a1ca 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1179,7 +1179,11 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 
 void intel_ring_advance(struct intel_ring_buffer *ring)
 {
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
 	ring->tail &= ring->size - 1;
+	if (dev_priv->stop_rings & intel_ring_flag(ring))
+		return;
 	ring->write_tail(ring, ring->tail);
 }
 
-- 
1.7.6.4

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1029 bytes --]

On Wed, Nov 2, 2011 at 09:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> GPU reset is a very important piece of our infrastructure.
> Unfortunately we only really test it by actually hanging the gpu,
> which often has bad side-effects for the entire system. And the gpu
> hang handling code is one of the rather complicated pieces of code we
> have, consisting of
> - hang detection
> - error capture
> - actual gpu reset
> - reset of all the gem bookkeeping
> - reinitialition of the entire gpu
>
> This patch adds a debugfs to selectively stopping rings by ceasing to
> update the hw tail pointer. This way we can exercise the gpu hang code
> under controlled conditions without a dying gpu taking down the entire
> systems.
>
> Patch motivated by me forgetting to properly reinitialize ppgtt after
> a gpu reset.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Could be handy for debugging, I like it.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1498 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Daniel Vetter]

On Thu, Nov 03, 2011 at 01:56:11PM -0200, Eugeni Dodonov wrote:
> Could be handy for debugging, I like it.

Have you tried actually blowing up your gpu with it? That's where the fun
starts ;-) My gpu-hanger branch contains the latest iteration of this
idea:

http://cgit.freedesktop.org/~danvet/drm/log/?h=gpu-hanger

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

On Thu, Nov 3, 2011 at 14:10, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Nov 03, 2011 at 01:56:11PM -0200, Eugeni Dodonov wrote:
> > Could be handy for debugging, I like it.
>
> Have you tried actually blowing up your gpu with it? That's where the fun
> starts ;-) My gpu-hanger branch contains the latest iteration of this
> idea:
>
> http://cgit.freedesktop.org/~danvet/drm/log/?h=gpu-hanger
>
>
Yep, it works for me. I can disable and enable rings at will by echo [01] >
/sys/kernel/debug/dri/0/i915_ring_stop

I was using it for working further on that i915_error_state heuristics
idea, outside of intel_decode. It is *very* handy to discover specific
patterns in error_state's.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Chris Wilson]

On Fri, 4 Nov 2011 14:23:19 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> Yep, it works for me. I can disable and enable rings at will by echo [01] >
> /sys/kernel/debug/dri/0/i915_ring_stop
> 
> I was using it for working further on that i915_error_state heuristics
> idea, outside of intel_decode. It is *very* handy to discover specific
> patterns in error_state's.

intel_gpu_dump or perchance reading the code ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 973 bytes --]

On Fri, Nov 4, 2011 at 14:31, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 4 Nov 2011 14:23:19 -0200, Eugeni Dodonov <eugeni@dodonov.net>
> wrote:
> > Yep, it works for me. I can disable and enable rings at will by echo
> [01] >
> > /sys/kernel/debug/dri/0/i915_ring_stop
> >
> > I was using it for working further on that i915_error_state heuristics
> > idea, outside of intel_decode. It is *very* handy to discover specific
> > patterns in error_state's.
>
> intel_gpu_dump or perchance reading the code ;-)
>

I mean, I am using it in a combination with the intel_gpu_dump, based on
the original idea which came from
http://comments.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/6408,
only outside of the real intel_decode routine.

But yes, reading the code is the best way, the overall idea here is to at
least indicate which code to read in the first place (kernel or ddx or mesa
or va or....) :).

-- 
Eugeni Dodonov
 <http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1474 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Ben Widawsky]

On Wed,  2 Nov 2011 12:46:36 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> GPU reset is a very important piece of our infrastructure.
> Unfortunately we only really test it by actually hanging the gpu,
> which often has bad side-effects for the entire system. And the gpu
> hang handling code is one of the rather complicated pieces of code we
> have, consisting of
> - hang detection
> - error capture
> - actual gpu reset
> - reset of all the gem bookkeeping
> - reinitialition of the entire gpu
> 
> This patch adds a debugfs to selectively stopping rings by ceasing to
> update the hw tail pointer. This way we can exercise the gpu hang code
> under controlled conditions without a dying gpu taking down the entire
> systems.
> 
> Patch motivated by me forgetting to properly reinitialize ppgtt after
> a gpu reset.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I like the concept here very much. One thing which I thought might be
interesting would to allow hanging rings temporarily instead of
automatically hanging until reset on the write of the debugfs entry. I
think if we could start dramatically altering the time it takes to get
commands into the ring would really test our buffer dependency tracking.
The obvious target here to me is hang the media ring for say 33ms, and
see what happens to the system.

Also, maybe I missed something but could you explain how you use this
differently than i915_wedged?

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |   63 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c         |    3 +
>  drivers/gpu/drm/i915/i915_drv.h         |    2 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++
>  4 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8476441..9821a3b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1392,6 +1392,64 @@ static const struct file_operations i915_wedged_fops = {
>  };
>  
>  static ssize_t
> +i915_ring_stop_read(struct file *filp,
> +		    char __user *ubuf,
> +		    size_t max,
> +		    loff_t *ppos)
> +{
> +	struct drm_device *dev = filp->private_data;
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	char buf[80];
> +	int len;
> +
> +	len = snprintf(buf, sizeof(buf),
> +		       "%d\n", dev_priv->stop_rings);
> +
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
> +	return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> +}

buf[80] seems a little excessive.

> +
> +static ssize_t
> +i915_ring_stop_write(struct file *filp,
> +		     const char __user *ubuf,
> +		     size_t cnt,
> +		     loff_t *ppos)
> +{
> +	struct drm_device *dev = filp->private_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	char buf[20];
> +	int val = 0;
> +
> +	if (cnt > 0) {
> +		if (cnt > sizeof(buf) - 1)
> +			return -EINVAL;
> +
> +		if (copy_from_user(buf, ubuf, cnt))
> +			return -EFAULT;
> +		buf[cnt] = 0;
> +
> +		val = simple_strtoul(buf, NULL, 0);
> +	}
> +
> +	DRM_DEBUG_DRIVER("Stopping rings %u\n", val);
> +
> +	mutex_lock(&dev->struct_mutex);
> +	dev_priv->stop_rings = val;
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return cnt;
> +}
> +

I think an atomic takes away the need for struct_mutex, unless you plan
to do more.

> +static const struct file_operations i915_ring_stop_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_debugfs_common_open,
> +	.read = i915_ring_stop_read,
> +	.write = i915_ring_stop_write,
> +	.llseek = default_llseek,
> +};
> +static ssize_t
>  i915_max_freq_read(struct file *filp,
>  		   char __user *ubuf,
>  		   size_t max,
> @@ -1691,6 +1749,11 @@ int i915_debugfs_init(struct drm_minor *minor)
>  				  &i915_cache_sharing_fops);
>  	if (ret)
>  		return ret;
> +	ret = i915_debugfs_create(minor->debugfs_root, minor,
> +				  "i915_ring_stop",
> +				  &i915_ring_stop_fops);
> +	if (ret)
> +		return ret;
>  
>  	return drm_debugfs_create_files(i915_debugfs_list,
>  					I915_DEBUGFS_ENTRIES,

I think the fact that you're stopping rings is an implementation detail,
and the name doesn't sounds nearly dangerous enough. I'd just call this
i915_hang_gpu, or something like that. Unless of course you plan
something like what I mentioned at the top about being able to restart
the ring.

> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 548e04b..566cc1e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -611,6 +611,9 @@ int i915_reset(struct drm_device *dev, u8 flags)
>  	if (!mutex_trylock(&dev->struct_mutex))
>  		return -EBUSY;
>  
> +	printk("reenabling rings\n");
> +	dev_priv->stop_rings = 0;
> +
>  	i915_gem_reset(dev);
>  
>  	ret = -ENODEV;

Probably shouldn't be using printk, just to keep things consistent with
everything else. DRM_DEBUG_DRIVER

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bd98fb3..503ae8c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -330,6 +330,8 @@ typedef struct drm_i915_private {
>  	uint32_t last_instdone;
>  	uint32_t last_instdone1;
>  
> +	unsigned int stop_rings;
> +
>  	unsigned long cfb_size;
>  	unsigned int cfb_fb;
>  	enum plane cfb_plane;

bool? Also I think you could consider treating this per ring.

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 3c30dba..ef7a1ca 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1179,7 +1179,11 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
>  
>  void intel_ring_advance(struct intel_ring_buffer *ring)
>  {
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +
>  	ring->tail &= ring->size - 1;
> +	if (dev_priv->stop_rings & intel_ring_flag(ring))
> +		return;
>  	ring->write_tail(ring, ring->tail);
>  }
>  

Maybe wrap the if with "unlikely?"

Re: [PATCH 3/3] [RFC] drm/i915: add interface to simulate gpu hangs

[Daniel Vetter]

On Sat, Nov 05, 2011 at 03:07:45PM -0700, Ben Widawsky wrote:
> On Wed,  2 Nov 2011 12:46:36 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > GPU reset is a very important piece of our infrastructure.
> > Unfortunately we only really test it by actually hanging the gpu,
> > which often has bad side-effects for the entire system. And the gpu
> > hang handling code is one of the rather complicated pieces of code we
> > have, consisting of
> > - hang detection
> > - error capture
> > - actual gpu reset
> > - reset of all the gem bookkeeping
> > - reinitialition of the entire gpu
> > 
> > This patch adds a debugfs to selectively stopping rings by ceasing to
> > update the hw tail pointer. This way we can exercise the gpu hang code
> > under controlled conditions without a dying gpu taking down the entire
> > systems.
> > 
> > Patch motivated by me forgetting to properly reinitialize ppgtt after
> > a gpu reset.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I like the concept here very much. One thing which I thought might be
> interesting would to allow hanging rings temporarily instead of
> automatically hanging until reset on the write of the debugfs entry. I
> think if we could start dramatically altering the time it takes to get
> commands into the ring would really test our buffer dependency tracking.
> The obvious target here to me is hang the media ring for say 33ms, and
> see what happens to the system.
> 
> Also, maybe I missed something but could you explain how you use this
> differently than i915_wedged?

I fail at writing decent patch descriptions. The difference wrt
i915_wedged is the essence of the idea ;-) Let's try again:

The idea is to simulate a gpu hang as accurately as possible without
actually hanging the gpu (or touching it much in any way). Because when
the gpu is dead for real it has the tendency to take down the entire
systems. One way to do that is to simply set the wedged state and manually
call the error_handler, like i915_wedged does. This leaves out all the
hangcheck detection code, though. And it runs the error_capture in a
completely different context.

So we need a way to stop the gpu without actually touching it and without
faking anything in our gem code. The idea used in this patch is to simply
cease to update the ringbuffer tail pointer. This way the gpu will simply
stop processing commands, which the hangcheck eventually notices because
the gpu head pointer doesn't advance anymore. Then the entire gpu hang
machinery gets fired off as if the gpu has died for real (but hopefully
with a massively increased chance that we survive the gpu reset).

> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c     |   63 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.c         |    3 +
> >  drivers/gpu/drm/i915/i915_drv.h         |    2 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++
> >  4 files changed, 72 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 8476441..9821a3b 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1392,6 +1392,64 @@ static const struct file_operations i915_wedged_fops = {
> >  };
> >  
> >  static ssize_t
> > +i915_ring_stop_read(struct file *filp,
> > +		    char __user *ubuf,
> > +		    size_t max,
> > +		    loff_t *ppos)
> > +{
> > +	struct drm_device *dev = filp->private_data;
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	char buf[80];
> > +	int len;
> > +
> > +	len = snprintf(buf, sizeof(buf),
> > +		       "%d\n", dev_priv->stop_rings);
> > +
> > +	if (len > sizeof(buf))
> > +		len = sizeof(buf);
> > +
> > +	return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> > +}
> 
> buf[80] seems a little excessive.

Oops, copy&paste. I'll shrink it a bit ...

> 
> > +
> > +static ssize_t
> > +i915_ring_stop_write(struct file *filp,
> > +		     const char __user *ubuf,
> > +		     size_t cnt,
> > +		     loff_t *ppos)
> > +{
> > +	struct drm_device *dev = filp->private_data;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	char buf[20];
> > +	int val = 0;
> > +
> > +	if (cnt > 0) {
> > +		if (cnt > sizeof(buf) - 1)
> > +			return -EINVAL;
> > +
> > +		if (copy_from_user(buf, ubuf, cnt))
> > +			return -EFAULT;
> > +		buf[cnt] = 0;
> > +
> > +		val = simple_strtoul(buf, NULL, 0);
> > +	}
> > +
> > +	DRM_DEBUG_DRIVER("Stopping rings %u\n", val);
> > +
> > +	mutex_lock(&dev->struct_mutex);
> > +	dev_priv->stop_rings = val;
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	return cnt;
> > +}
> > +
> 
> I think an atomic takes away the need for struct_mutex, unless you plan
> to do more.

atomic_t would require atomic ops on the read side. I don't care about
locking on the read side as long as the new value will eventually show up,
so I've opted for the unlocked (and fastest) option for reads.

> 
> > +static const struct file_operations i915_ring_stop_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = i915_debugfs_common_open,
> > +	.read = i915_ring_stop_read,
> > +	.write = i915_ring_stop_write,
> > +	.llseek = default_llseek,
> > +};
> > +static ssize_t
> >  i915_max_freq_read(struct file *filp,
> >  		   char __user *ubuf,
> >  		   size_t max,
> > @@ -1691,6 +1749,11 @@ int i915_debugfs_init(struct drm_minor *minor)
> >  				  &i915_cache_sharing_fops);
> >  	if (ret)
> >  		return ret;
> > +	ret = i915_debugfs_create(minor->debugfs_root, minor,
> > +				  "i915_ring_stop",
> > +				  &i915_ring_stop_fops);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return drm_debugfs_create_files(i915_debugfs_list,
> >  					I915_DEBUGFS_ENTRIES,
> 
> I think the fact that you're stopping rings is an implementation detail,
> and the name doesn't sounds nearly dangerous enough. I'd just call this
> i915_hang_gpu, or something like that. Unless of course you plan
> something like what I mentioned at the top about being able to restart
> the ring.

Because our current hangcheck code only checks the render ring I've wanted
to stop rings individually (so we could check hangcheck extensions to the
other rings). Adding an extension to restart the rings sounds good, it
would just require to redo the last tail writes that got dropped.

> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 548e04b..566cc1e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -611,6 +611,9 @@ int i915_reset(struct drm_device *dev, u8 flags)
> >  	if (!mutex_trylock(&dev->struct_mutex))
> >  		return -EBUSY;
> >  
> > +	printk("reenabling rings\n");
> > +	dev_priv->stop_rings = 0;
> > +
> >  	i915_gem_reset(dev);
> >  
> >  	ret = -ENODEV;
> 
> Probably shouldn't be using printk, just to keep things consistent with
> everything else. DRM_DEBUG_DRIVER

Oops, debug leftover, I'll change this.

> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bd98fb3..503ae8c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -330,6 +330,8 @@ typedef struct drm_i915_private {
> >  	uint32_t last_instdone;
> >  	uint32_t last_instdone1;
> >  
> > +	unsigned int stop_rings;
> > +
> >  	unsigned long cfb_size;
> >  	unsigned int cfb_fb;
> >  	enum plane cfb_plane;
> 
> bool? Also I think you could consider treating this per ring.

It's a flag array, so already per-ring. See below.

> 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 3c30dba..ef7a1ca 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1179,7 +1179,11 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
> >  
> >  void intel_ring_advance(struct intel_ring_buffer *ring)
> >  {
> > +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > +
> >  	ring->tail &= ring->size - 1;
> > +	if (dev_priv->stop_rings & intel_ring_flag(ring))
> > +		return;
> >  	ring->write_tail(ring, ring->tail);
> >  }
> >  
> 
> Maybe wrap the if with "unlikely?"

Yeah.

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48