[PATCH 0/7] Hangcheck and arb robustness

[PATCH 0/7] Hangcheck and arb robustness

[Mika Kuoppala]

Hi,

Assortment of hangcheck related stuff and
reset stats ioctl on top.

Ben noted that his ppgtt series provides 1/7 but
I included it here cause 7/7 depends on it.

-Mika

Chris Wilson (2):
  drm/i915: Fix retrieval of hangcheck stats
  drm/i915: Replace open-coding of DEFAULT_CONTEXT_ID

Mika Kuoppala (5):
  drm/i915: introduce i915_queue_hangcheck
  drm/i915: no hangcheck when reset is in progress
  drm/i915: queue hangcheck on reset
  drm/i915: add i915_reset_count
  drm/i915: add i915_get_reset_stats_ioctl

 drivers/gpu/drm/i915/i915_dma.c            |    1 +
 drivers/gpu/drm/i915/i915_drv.c            |   34 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h            |   14 +++++++++---
 drivers/gpu/drm/i915/i915_gem.c            |    6 ++---
 drivers/gpu/drm/i915/i915_gem_context.c    |   23 +++++++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++---
 drivers/gpu/drm/i915/i915_irq.c            |   28 +++++++++++++++--------
 include/uapi/drm/i915_drm.h                |   17 ++++++++++++++
 8 files changed, 94 insertions(+), 35 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] drm/i915: Fix retrieval of hangcheck stats

[Mika Kuoppala]

From: Chris Wilson <chris@chris-wilson.co.uk>

The default context is always supported (as it contains the global
hangcheck stats) and the contexts for hangcheck are not limited
to any ring.

References: https://bugs.freedesktop.org/show_bug.cgi?id=65845
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |    2 +-
 drivers/gpu/drm/i915/i915_gem_context.c |   23 ++++++++---------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a7c2cfb3..8e73e3b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1840,7 +1840,7 @@ static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
 }
 
 struct i915_ctx_hang_stats * __must_check
-i915_gem_context_get_hang_stats(struct intel_ring_buffer *ring,
+i915_gem_context_get_hang_stats(struct drm_device *dev,
 				struct drm_file *file,
 				u32 id);
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 51b7a21..05ef1c2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -304,31 +304,24 @@ static int context_idr_cleanup(int id, void *p, void *data)
 }
 
 struct i915_ctx_hang_stats *
-i915_gem_context_get_hang_stats(struct intel_ring_buffer *ring,
+i915_gem_context_get_hang_stats(struct drm_device *dev,
 				struct drm_file *file,
 				u32 id)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
-	struct i915_hw_context *to;
-
-	if (dev_priv->hw_contexts_disabled)
-		return ERR_PTR(-ENOENT);
-
-	if (ring->id != RCS)
-		return ERR_PTR(-EINVAL);
-
-	if (file == NULL)
-		return ERR_PTR(-EINVAL);
+	struct i915_hw_context *ctx;
 
 	if (id == DEFAULT_CONTEXT_ID)
 		return &file_priv->hang_stats;
 
-	to = i915_gem_context_get(file->driver_priv, id);
-	if (to == NULL)
+	ctx = NULL;
+	if (!dev_priv->hw_contexts_disabled)
+		ctx = i915_gem_context_get(file->driver_priv, id);
+	if (ctx == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return &to->hang_stats;
+	return &ctx->hang_stats;
 }
 
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
-- 
1.7.9.5

[PATCH 2/7] drm/i915: Replace open-coding of DEFAULT_CONTEXT_ID

[Mika Kuoppala]

From: Chris Wilson <chris@chris-wilson.co.uk>

The intent of the check is made more clear if we use the proper name for
0 here.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 87a3227..b6b47d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -872,7 +872,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		break;
 	case I915_EXEC_BSD:
 		ring = &dev_priv->ring[VCS];
-		if (ctx_id != 0) {
+		if (ctx_id != DEFAULT_CONTEXT_ID) {
 			DRM_DEBUG("Ring %s doesn't support contexts\n",
 				  ring->name);
 			return -EPERM;
@@ -880,7 +880,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		break;
 	case I915_EXEC_BLT:
 		ring = &dev_priv->ring[BCS];
-		if (ctx_id != 0) {
+		if (ctx_id != DEFAULT_CONTEXT_ID) {
 			DRM_DEBUG("Ring %s doesn't support contexts\n",
 				  ring->name);
 			return -EPERM;
@@ -888,7 +888,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		break;
 	case I915_EXEC_VEBOX:
 		ring = &dev_priv->ring[VECS];
-		if (ctx_id != 0) {
+		if (ctx_id != DEFAULT_CONTEXT_ID) {
 			DRM_DEBUG("Ring %s doesn't support contexts\n",
 				  ring->name);
 			return -EPERM;
-- 
1.7.9.5

[PATCH 3/7] drm/i915: introduce i915_queue_hangcheck

[Mika Kuoppala]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

To run hangcheck in near future.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |    6 ++----
 drivers/gpu/drm/i915/i915_irq.c |   21 ++++++++++++---------
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8e73e3b..90cdd49 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1593,6 +1593,7 @@ extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 extern void intel_console_resume(struct work_struct *work);
 
 /* i915_irq.c */
+void i915_queue_hangcheck(struct drm_device *dev);
 void i915_hangcheck_elapsed(unsigned long data);
 void i915_handle_error(struct drm_device *dev, bool wedged);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 769f752..c5c90e3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2082,10 +2082,8 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	ring->outstanding_lazy_request = 0;
 
 	if (!dev_priv->mm.suspended) {
-		if (i915_enable_hangcheck) {
-			mod_timer(&dev_priv->gpu_error.hangcheck_timer,
-				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
-		}
+		i915_queue_hangcheck(ring->dev);
+
 		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
 					   &dev_priv->mm.retire_work,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c1b1e3..dc1b878 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -668,18 +668,13 @@ static void ironlake_handle_rps_change(struct drm_device *dev)
 static void notify_ring(struct drm_device *dev,
 			struct intel_ring_buffer *ring)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	if (ring->obj == NULL)
 		return;
 
 	trace_i915_gem_request_complete(ring, ring->get_seqno(ring, false));
 
 	wake_up_all(&ring->irq_queue);
-	if (i915_enable_hangcheck) {
-		mod_timer(&dev_priv->gpu_error.hangcheck_timer,
-			  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
-	}
+	i915_queue_hangcheck(dev);
 }
 
 static void gen6_pm_rps_work(struct work_struct *work)
@@ -2537,9 +2532,17 @@ void i915_hangcheck_elapsed(unsigned long data)
 	if (busy_count)
 		/* Reset timer case chip hangs without another request
 		 * being added */
-		mod_timer(&dev_priv->gpu_error.hangcheck_timer,
-			  round_jiffies_up(jiffies +
-					   DRM_I915_HANGCHECK_JIFFIES));
+		i915_queue_hangcheck(dev);
+}
+
+void i915_queue_hangcheck(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (!i915_enable_hangcheck)
+		return;
+
+	mod_timer(&dev_priv->gpu_error.hangcheck_timer,
+		  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 }
 
 static void ibx_irq_preinstall(struct drm_device *dev)
-- 
1.7.9.5

[PATCH 4/7] drm/i915: no hangcheck when reset is in progress

[Mika Kuoppala]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

The timer for hangchecking can run again before the previous
reset it has triggered has been handled. This can corrupt
the hangcheck state as reset handling will access and write to
the hangcheck data. To prevent this, avoid running the hangcheck
logic while reset is in progress.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index dc1b878..b0fec7f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2447,6 +2447,9 @@ void i915_hangcheck_elapsed(unsigned long data)
 	if (!i915_enable_hangcheck)
 		return;
 
+	if (i915_reset_in_progress(&dev_priv->gpu_error))
+		return;
+
 	for_each_ring(ring, dev_priv, i) {
 		u32 seqno, acthd;
 		bool busy = true;
-- 
1.7.9.5

[PATCH 5/7] drm/i915: queue hangcheck on reset

[Mika Kuoppala]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Upon resetting the GPU, we begin processing batches once more, so
reset the hangcheck timer.

v2: kicking inside reset instead of hangcheck_elapsed and
    sane commit message by Chris Wilson

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b0fec7f..1b0e903 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1452,6 +1452,8 @@ static void i915_error_work_func(struct work_struct *work)
 
 			kobject_uevent_env(&dev->primary->kdev.kobj,
 					   KOBJ_CHANGE, reset_done_event);
+
+			i915_queue_hangcheck(dev);
 		} else {
 			atomic_set(&error->reset_counter, I915_WEDGED);
 		}
-- 
1.7.9.5

[PATCH 6/7] drm/i915: add i915_reset_count

[Mika Kuoppala]

Use reset_counter to track how many actual resets
have been done. Reset in progress is enough to increment
the counter.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    9 +++++++--
 drivers/gpu/drm/i915/i915_irq.c |    2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 90cdd49..1def049 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -963,7 +963,7 @@ struct i915_gpu_error {
 	 * being true.
 	 */
 #define I915_RESET_IN_PROGRESS_FLAG	1
-#define I915_WEDGED			0xffffffff
+#define I915_WEDGED			(1 << 31)
 
 	/**
 	 * Waitqueue to signal when the reset has completed. Used by clients
@@ -1765,7 +1765,12 @@ static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
 
 static inline bool i915_terminally_wedged(struct i915_gpu_error *error)
 {
-	return atomic_read(&error->reset_counter) == I915_WEDGED;
+	return atomic_read(&error->reset_counter) & I915_WEDGED;
+}
+
+static inline u32 i915_reset_count(struct i915_gpu_error *error)
+{
+	return ((atomic_read(&error->reset_counter) & (~I915_WEDGED)) + 1) / 2;
 }
 
 void i915_gem_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1b0e903..6e13501 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1455,7 +1455,7 @@ static void i915_error_work_func(struct work_struct *work)
 
 			i915_queue_hangcheck(dev);
 		} else {
-			atomic_set(&error->reset_counter, I915_WEDGED);
+			atomic_set_mask(I915_WEDGED, &error->reset_counter);
 		}
 
 		for_each_ring(ring, dev_priv, i)
-- 
1.7.9.5

[PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Mika Kuoppala]

This ioctl returns reset stats for specified context.

The struct returned contains context loss counters.

reset_count:    all resets across all contexts
batch_active:   active batches lost on resets
batch_pending:  pending batches lost on resets

v2: get rid of state tracking completely and deliver only counts. Idea
    from Chris Wilson.

v3: fix commit message

v4: default context handled inside i915_gem_contest_get_hang_stats

v5: reset_count only for priviledged process

v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)

v7: context hang stats never returns NULL

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ian Romanick <idr@freedesktop.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    1 +
 drivers/gpu/drm/i915/i915_drv.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |    2 ++
 include/uapi/drm/i915_drm.h     |   17 +++++++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..d1a006f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 33cb973..0d4e3a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 
 	return 0;
 }
+
+int i915_get_reset_stats_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_reset_stats *args = data;
+	struct i915_ctx_hang_stats *hs;
+	int ret;
+
+	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
+	if (IS_ERR(hs)) {
+		mutex_unlock(&dev->struct_mutex);
+		return PTR_ERR(hs);
+	}
+
+	if (capable(CAP_SYS_ADMIN))
+		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
+	else
+		args->reset_count = 0;
+
+	args->batch_active = hs->batch_active;
+	args->batch_pending = hs->batch_pending;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1def049..0ca98fa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2021,6 +2021,8 @@ extern int intel_enable_rc6(const struct drm_device *dev);
 extern bool i915_semaphore_is_enabled(struct drm_device *dev);
 int i915_reg_read_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
+int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
 
 /* overlay */
 #ifdef CONFIG_DEBUG_FS
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 923ed7f..29b07fd 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -198,6 +198,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_SET_CACHING	0x2f
 #define DRM_I915_GEM_GET_CACHING	0x30
 #define DRM_I915_REG_READ		0x31
+#define DRM_I915_GET_RESET_STATS	0x32
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -247,6 +248,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
 #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
+#define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -981,4 +983,19 @@ struct drm_i915_reg_read {
 	__u64 offset;
 	__u64 val; /* Return value */
 };
+
+struct drm_i915_reset_stats {
+	__u32 ctx_id;
+	__u32 flags;
+
+	/* For all contexts */
+	__u32 reset_count;
+
+	/* For this context */
+	__u32 batch_active;
+	__u32 batch_pending;
+
+	__u32 pad;
+};
+
 #endif /* _UAPI_I915_DRM_H_ */
-- 
1.7.9.5

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Chris Wilson]

On Wed, Jul 03, 2013 at 05:22:12PM +0300, Mika Kuoppala wrote:
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 33cb973..0d4e3a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  
>  	return 0;
>  }
> +
> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> +			       void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reset_stats *args = data;
> +	struct i915_ctx_hang_stats *hs;
> +	int ret;
> +
> +	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;

When per-file default contexts land, there will not be any global
information leak and so we can drop the capability check here.

Ben, will we be informing userspace about the ABI change?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ben Widawsky]

On Wed, Jul 03, 2013 at 04:14:31PM +0100, Chris Wilson wrote:
> On Wed, Jul 03, 2013 at 05:22:12PM +0300, Mika Kuoppala wrote:
> >  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 33cb973..0d4e3a8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  
> >  	return 0;
> >  }
> > +
> > +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> > +			       void *data, struct drm_file *file)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_reset_stats *args = data;
> > +	struct i915_ctx_hang_stats *hs;
> > +	int ret;
> > +
> > +	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> 
> When per-file default contexts land, there will not be any global
> information leak and so we can drop the capability check here.
> 
> Ben, will we be informing userspace about the ABI change?
> -Chris

Hmm. I'm not convinced we ever want to reset the default context stats.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Chris Wilson]

On Wed, Jul 03, 2013 at 02:23:23PM -0700, Ben Widawsky wrote:
> On Wed, Jul 03, 2013 at 04:14:31PM +0100, Chris Wilson wrote:
> > Ben, will we be informing userspace about the ABI change?
> 
> Hmm. I'm not convinced we ever want to reset the default context stats.

I meant: will userspace be able to find out that the kernel now uses
per-fd default contexts. i.e. will I know if the kernel will
automatically allocate me a separate context and switch every time my
batches execute?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ben Widawsky]

On Wed, Jul 03, 2013 at 10:41:11PM +0100, Chris Wilson wrote:
> On Wed, Jul 03, 2013 at 02:23:23PM -0700, Ben Widawsky wrote:
> > On Wed, Jul 03, 2013 at 04:14:31PM +0100, Chris Wilson wrote:
> > > Ben, will we be informing userspace about the ABI change?
> > 
> > Hmm. I'm not convinced we ever want to reset the default context stats.
> 
> I meant: will userspace be able to find out that the kernel now uses
> per-fd default contexts. i.e. will I know if the kernel will
> automatically allocate me a separate context and switch every time my
> batches execute?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Yes. It won't be a requirement for mesa to know, but since I want to
kill aliasing ppgtt as much as possible, I intend to update getparam to
both set 0 to aliasing, and a new one for ppgtt. 



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Daniel Vetter]

On Wed, Jul 3, 2013 at 11:44 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> Yes. It won't be a requirement for mesa to know, but since I want to
> kill aliasing ppgtt as much as possible, I intend to update getparam to
> both set 0 to aliasing, and a new one for ppgtt.

We use that one essentially to decide whether the hw uses ppgtt or
not, aliasing kinda doesn't matter. So to keep api I guess we need to
keep that one enabled. Although only i-g-t really cares I think, but
keeping it working would still be nice. Maybe just add a comment to
the #define explaining what's going on?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ben Widawsky]

On Wed, Jul 03, 2013 at 11:58:31PM +0200, Daniel Vetter wrote:
> On Wed, Jul 3, 2013 at 11:44 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Yes. It won't be a requirement for mesa to know, but since I want to
> > kill aliasing ppgtt as much as possible, I intend to update getparam to
> > both set 0 to aliasing, and a new one for ppgtt.
> 
> We use that one essentially to decide whether the hw uses ppgtt or
> not, aliasing kinda doesn't matter. So to keep api I guess we need to
> keep that one enabled. Although only i-g-t really cares I think, but
> keeping it working would still be nice. Maybe just add a comment to
> the #define explaining what's going on?
> -Daniel

It's a bit off topic for this series, but I don't see any reason to keep
it working. If it doesn't end up being too much trouble, I can - but in
actuality on the kernel would be able to use it. Exposing the param as
true to user space would be a lie.

I was planning to update the IGT tests to support both things FWIW.

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

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ville Syrjälä]

On Wed, Jul 03, 2013 at 03:00:39PM -0700, Ben Widawsky wrote:
> On Wed, Jul 03, 2013 at 11:58:31PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 3, 2013 at 11:44 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > Yes. It won't be a requirement for mesa to know, but since I want to
> > > kill aliasing ppgtt as much as possible, I intend to update getparam to
> > > both set 0 to aliasing, and a new one for ppgtt.
> > 
> > We use that one essentially to decide whether the hw uses ppgtt or
> > not, aliasing kinda doesn't matter. So to keep api I guess we need to
> > keep that one enabled. Although only i-g-t really cares I think, but
> > keeping it working would still be nice. Maybe just add a comment to
> > the #define explaining what's going on?
> > -Daniel
> 
> It's a bit off topic for this series, but I don't see any reason to keep
> it working. If it doesn't end up being too much trouble, I can - but in
> actuality on the kernel would be able to use it. Exposing the param as
> true to user space would be a lie.

But won't old Mesa refuse to work if you tell it aliasing ppgtt is not
enabled?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ben Widawsky]

On Thu, Jul 04, 2013 at 10:54:58AM +0300, Ville Syrjälä wrote:
> On Wed, Jul 03, 2013 at 03:00:39PM -0700, Ben Widawsky wrote:
> > On Wed, Jul 03, 2013 at 11:58:31PM +0200, Daniel Vetter wrote:
> > > On Wed, Jul 3, 2013 at 11:44 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > > Yes. It won't be a requirement for mesa to know, but since I want to
> > > > kill aliasing ppgtt as much as possible, I intend to update getparam to
> > > > both set 0 to aliasing, and a new one for ppgtt.
> > > 
> > > We use that one essentially to decide whether the hw uses ppgtt or
> > > not, aliasing kinda doesn't matter. So to keep api I guess we need to
> > > keep that one enabled. Although only i-g-t really cares I think, but
> > > keeping it working would still be nice. Maybe just add a comment to
> > > the #define explaining what's going on?
> > > -Daniel
> > 
> > It's a bit off topic for this series, but I don't see any reason to keep
> > it working. If it doesn't end up being too much trouble, I can - but in
> > actuality on the kernel would be able to use it. Exposing the param as
> > true to user space would be a lie.
> 
> But won't old Mesa refuse to work if you tell it aliasing ppgtt is not
> enabled?

AFAIK, mesa doesn't check... I can update mesa too if so.

> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: no hangcheck when reset is in progress

[Daniel Vetter]

On Wed, Jul 03, 2013 at 05:22:09PM +0300, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> The timer for hangchecking can run again before the previous
> reset it has triggered has been handled. This can corrupt
> the hangcheck state as reset handling will access and write to
> the hangcheck data. To prevent this, avoid running the hangcheck
> logic while reset is in progress.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index dc1b878..b0fec7f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2447,6 +2447,9 @@ void i915_hangcheck_elapsed(unsigned long data)
>  	if (!i915_enable_hangcheck)
>  		return;
>  
> +	if (i915_reset_in_progress(&dev_priv->gpu_error))
> +		return;

atomic_t are weakly-ordered (I know, we're on x86 here ...) and I think
we're missing the requisite amount of barriers to keep races at bay.

I think wrapping up all access to the hangcheck stats (both from the
hangcheck timer and in the reset code and eventually in the readout code)
with an irq-save spinlock is the simpler and much more obviously correct
(and hence safer) option. Note that you can't optimize away the irq
save/restore stuff in the timer since we call the hangcheck stuff also
from hardirq context.

So I'll punt on this patch here for now, merged all the others leading up
to this one here.

Thanks, Daniel

> +
>  	for_each_ring(ring, dev_priv, i) {
>  		u32 seqno, acthd;
>  		bool busy = true;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/7] drm/i915: queue hangcheck on reset

[Daniel Vetter]

On Wed, Jul 03, 2013 at 05:22:10PM +0300, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> Upon resetting the GPU, we begin processing batches once more, so
> reset the hangcheck timer.
> 
> v2: kicking inside reset instead of hangcheck_elapsed and
>     sane commit message by Chris Wilson
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b0fec7f..1b0e903 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1452,6 +1452,8 @@ static void i915_error_work_func(struct work_struct *work)
>  
>  			kobject_uevent_env(&dev->primary->kdev.kobj,
>  					   KOBJ_CHANGE, reset_done_event);
> +
> +			i915_queue_hangcheck(dev);

Hm, what exactly is this for? After reset we don't have any batches
running right now (since we reset all batches), so I don't understand why
we need this. And the commit message also doesn't give a reason.

Aside a little maintainer bikeshed: The important thing a commit message
should cover isn't really the "what changed" since the code should be
readable enough to not need additional explanation, but the "why". In some
cases the important part of the "why" is "why not a different approach" or
"why does this work at all" (e.g. for bugfixes).

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

Re: [PATCH 5/7] drm/i915: queue hangcheck on reset

[Chris Wilson]

On Tue, Jul 16, 2013 at 10:49:29AM +0200, Daniel Vetter wrote:
> On Wed, Jul 03, 2013 at 05:22:10PM +0300, Mika Kuoppala wrote:
> > From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > 
> > Upon resetting the GPU, we begin processing batches once more, so
> > reset the hangcheck timer.
> > 
> > v2: kicking inside reset instead of hangcheck_elapsed and
> >     sane commit message by Chris Wilson
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index b0fec7f..1b0e903 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1452,6 +1452,8 @@ static void i915_error_work_func(struct work_struct *work)
> >  
> >  			kobject_uevent_env(&dev->primary->kdev.kobj,
> >  					   KOBJ_CHANGE, reset_done_event);
> > +
> > +			i915_queue_hangcheck(dev);
> 
> Hm, what exactly is this for? After reset we don't have any batches
> running right now (since we reset all batches), so I don't understand why
> we need this. And the commit message also doesn't give a reason.

Because our code is a little snafu after reset. We do have batches still
queued, but the rings are incorrectly reset. Instead they should just be
promoted past the failed batch and processing restarted. If you get
extremely fancy, we can no-op out any requests from the hung !default
context to prevent incorrect state leakage. This also likely explains
why we end up with active bo stuck after becoming wedged.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

Since the Mesa merge window is closing soon, I'm finally getting back on
this.  I've pushed a rebase of my old Mesa branch to my fd.o repo

http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3

I have a couple questions...

1. Has any of this landed an a kernel tree anywhere?

2. Has any support code landed in a libdrm tree anywhere?

3. What method should I use to detect that the kernel has support?  In
early discussions, reset notification was only going to be available on
some GPUs, so there was a getparam to detect actual availability.  I
guess now it's just based on kernel version?

It looks like I should just need to update df87cdd and 61dad8e in my
Mesa tree.

On 07/03/2013 07:22 AM, Mika Kuoppala wrote:
> This ioctl returns reset stats for specified context.
> 
> The struct returned contains context loss counters.
> 
> reset_count:    all resets across all contexts
> batch_active:   active batches lost on resets
> batch_pending:  pending batches lost on resets
> 
> v2: get rid of state tracking completely and deliver only counts. Idea
>     from Chris Wilson.
> 
> v3: fix commit message
> 
> v4: default context handled inside i915_gem_contest_get_hang_stats
> 
> v5: reset_count only for priviledged process
> 
> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
> 
> v7: context hang stats never returns NULL
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ian Romanick <idr@freedesktop.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>  drivers/gpu/drm/i915/i915_drv.c |   34 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |    2 ++
>  include/uapi/drm/i915_drm.h     |   17 +++++++++++++++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 0e22142..d1a006f 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 33cb973..0d4e3a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  
>  	return 0;
>  }
> +
> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> +			       void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reset_stats *args = data;
> +	struct i915_ctx_hang_stats *hs;
> +	int ret;
> +
> +	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
> +	if (IS_ERR(hs)) {
> +		mutex_unlock(&dev->struct_mutex);
> +		return PTR_ERR(hs);
> +	}
> +
> +	if (capable(CAP_SYS_ADMIN))
> +		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> +	else
> +		args->reset_count = 0;
> +
> +	args->batch_active = hs->batch_active;
> +	args->batch_pending = hs->batch_pending;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1def049..0ca98fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2021,6 +2021,8 @@ extern int intel_enable_rc6(const struct drm_device *dev);
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
>  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>  			struct drm_file *file);
> +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> +			       struct drm_file *file);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 923ed7f..29b07fd 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -198,6 +198,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_GEM_SET_CACHING	0x2f
>  #define DRM_I915_GEM_GET_CACHING	0x30
>  #define DRM_I915_REG_READ		0x31
> +#define DRM_I915_GET_RESET_STATS	0x32
>  
>  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -247,6 +248,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
>  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
>  #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
> +#define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -981,4 +983,19 @@ struct drm_i915_reg_read {
>  	__u64 offset;
>  	__u64 val; /* Return value */
>  };
> +
> +struct drm_i915_reset_stats {
> +	__u32 ctx_id;
> +	__u32 flags;
> +
> +	/* For all contexts */
> +	__u32 reset_count;
> +
> +	/* For this context */
> +	__u32 batch_active;
> +	__u32 batch_pending;
> +
> +	__u32 pad;
> +};
> +
>  #endif /* _UAPI_I915_DRM_H_ */
> 

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Daniel Vetter]

On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
> Since the Mesa merge window is closing soon, I'm finally getting back on
> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
> 
> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
> 
> I have a couple questions...
> 
> 1. Has any of this landed an a kernel tree anywhere?

Afaik everything but the actual ioctl and i-g-t testcase has landed.

> 2. Has any support code landed in a libdrm tree anywhere?

Dunno whether Mika has libdrm patches. Since mesa is the only expected
user I'd just go with putting the ioctl wrapper (using the drmIoctl
helper) into mesa itself, that get rids of a dep for merging this support.

> 3. What method should I use to detect that the kernel has support?  In
> early discussions, reset notification was only going to be available on
> some GPUs, so there was a getparam to detect actual availability.  I
> guess now it's just based on kernel version?

Usually we add a new feature flag to get get_param ioctl if there's no
natural way otherwise for userspace to figure this out (usually by calling
the new ioctl and disabling the feature if that doesn't work).
-Daniel

> 
> It looks like I should just need to update df87cdd and 61dad8e in my
> Mesa tree.
> 
> On 07/03/2013 07:22 AM, Mika Kuoppala wrote:
> > This ioctl returns reset stats for specified context.
> > 
> > The struct returned contains context loss counters.
> > 
> > reset_count:    all resets across all contexts
> > batch_active:   active batches lost on resets
> > batch_pending:  pending batches lost on resets
> > 
> > v2: get rid of state tracking completely and deliver only counts. Idea
> >     from Chris Wilson.
> > 
> > v3: fix commit message
> > 
> > v4: default context handled inside i915_gem_contest_get_hang_stats
> > 
> > v5: reset_count only for priviledged process
> > 
> > v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
> > 
> > v7: context hang stats never returns NULL
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Ian Romanick <idr@freedesktop.org>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c |    1 +
> >  drivers/gpu/drm/i915/i915_drv.c |   34 ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.h |    2 ++
> >  include/uapi/drm/i915_drm.h     |   17 +++++++++++++++++
> >  4 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 0e22142..d1a006f 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
> >  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
> >  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> >  	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
> > +	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED),
> >  };
> >  
> >  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 33cb973..0d4e3a8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  
> >  	return 0;
> >  }
> > +
> > +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> > +			       void *data, struct drm_file *file)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_reset_stats *args = data;
> > +	struct i915_ctx_hang_stats *hs;
> > +	int ret;
> > +
> > +	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> > +
> > +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> > +	if (ret)
> > +		return ret;
> > +
> > +	hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
> > +	if (IS_ERR(hs)) {
> > +		mutex_unlock(&dev->struct_mutex);
> > +		return PTR_ERR(hs);
> > +	}
> > +
> > +	if (capable(CAP_SYS_ADMIN))
> > +		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> > +	else
> > +		args->reset_count = 0;
> > +
> > +	args->batch_active = hs->batch_active;
> > +	args->batch_pending = hs->batch_pending;
> > +
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1def049..0ca98fa 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2021,6 +2021,8 @@ extern int intel_enable_rc6(const struct drm_device *dev);
> >  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> >  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> >  			struct drm_file *file);
> > +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> > +			       struct drm_file *file);
> >  
> >  /* overlay */
> >  #ifdef CONFIG_DEBUG_FS
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 923ed7f..29b07fd 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -198,6 +198,7 @@ typedef struct _drm_i915_sarea {
> >  #define DRM_I915_GEM_SET_CACHING	0x2f
> >  #define DRM_I915_GEM_GET_CACHING	0x30
> >  #define DRM_I915_REG_READ		0x31
> > +#define DRM_I915_GET_RESET_STATS	0x32
> >  
> >  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
> >  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> > @@ -247,6 +248,7 @@ typedef struct _drm_i915_sarea {
> >  #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
> >  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
> >  #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
> > +#define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
> >  
> >  /* Allow drivers to submit batchbuffers directly to hardware, relying
> >   * on the security mechanisms provided by hardware.
> > @@ -981,4 +983,19 @@ struct drm_i915_reg_read {
> >  	__u64 offset;
> >  	__u64 val; /* Return value */
> >  };
> > +
> > +struct drm_i915_reset_stats {
> > +	__u32 ctx_id;
> > +	__u32 flags;
> > +
> > +	/* For all contexts */
> > +	__u32 reset_count;
> > +
> > +	/* For this context */
> > +	__u32 batch_active;
> > +	__u32 batch_pending;
> > +
> > +	__u32 pad;
> > +};
> > +
> >  #endif /* _UAPI_I915_DRM_H_ */
> > 
> 

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

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 10/27/2013 05:30 AM, Daniel Vetter wrote:
> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>> Since the Mesa merge window is closing soon, I'm finally getting back on
>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>
>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>
>> I have a couple questions...
>>
>> 1. Has any of this landed an a kernel tree anywhere?
> 
> Afaik everything but the actual ioctl and i-g-t testcase has landed.

And that stuff will land once my patches hit the Mesa list or ... ?

>> 2. Has any support code landed in a libdrm tree anywhere?
> 
> Dunno whether Mika has libdrm patches. Since mesa is the only expected
> user I'd just go with putting the ioctl wrapper (using the drmIoctl
> helper) into mesa itself, that get rids of a dep for merging this support.

What's the right way to get the ctx_id out of the drm_intel_context?
That struct is private to libdrm, but the ioctl needs it.

>> 3. What method should I use to detect that the kernel has support?  In
>> early discussions, reset notification was only going to be available on
>> some GPUs, so there was a getparam to detect actual availability.  I
>> guess now it's just based on kernel version?
> 
> Usually we add a new feature flag to get get_param ioctl if there's no
> natural way otherwise for userspace to figure this out (usually by calling
> the new ioctl and disabling the feature if that doesn't work).
> -Daniel
> 
>>
>> It looks like I should just need to update df87cdd and 61dad8e in my
>> Mesa tree.
>>
>> On 07/03/2013 07:22 AM, Mika Kuoppala wrote:
>>> This ioctl returns reset stats for specified context.
>>>
>>> The struct returned contains context loss counters.
>>>
>>> reset_count:    all resets across all contexts
>>> batch_active:   active batches lost on resets
>>> batch_pending:  pending batches lost on resets
>>>
>>> v2: get rid of state tracking completely and deliver only counts. Idea
>>>     from Chris Wilson.
>>>
>>> v3: fix commit message
>>>
>>> v4: default context handled inside i915_gem_contest_get_hang_stats
>>>
>>> v5: reset_count only for priviledged process
>>>
>>> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
>>>
>>> v7: context hang stats never returns NULL
>>>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Ian Romanick <idr@freedesktop.org>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>>>  drivers/gpu/drm/i915/i915_drv.c |   34 ++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/i915_drv.h |    2 ++
>>>  include/uapi/drm/i915_drm.h     |   17 +++++++++++++++++
>>>  4 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>> index 0e22142..d1a006f 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>>>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
>>>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
>>>  	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
>>> +	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED),
>>>  };
>>>  
>>>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index 33cb973..0d4e3a8 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>>>  
>>>  	return 0;
>>>  }
>>> +
>>> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
>>> +			       void *data, struct drm_file *file)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct drm_i915_reset_stats *args = data;
>>> +	struct i915_ctx_hang_stats *hs;
>>> +	int ret;
>>> +
>>> +	if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
>>> +		return -EPERM;
>>> +
>>> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
>>> +	if (IS_ERR(hs)) {
>>> +		mutex_unlock(&dev->struct_mutex);
>>> +		return PTR_ERR(hs);
>>> +	}
>>> +
>>> +	if (capable(CAP_SYS_ADMIN))
>>> +		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
>>> +	else
>>> +		args->reset_count = 0;
>>> +
>>> +	args->batch_active = hs->batch_active;
>>> +	args->batch_pending = hs->batch_pending;
>>> +
>>> +	mutex_unlock(&dev->struct_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 1def049..0ca98fa 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2021,6 +2021,8 @@ extern int intel_enable_rc6(const struct drm_device *dev);
>>>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
>>>  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>>>  			struct drm_file *file);
>>> +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
>>> +			       struct drm_file *file);
>>>  
>>>  /* overlay */
>>>  #ifdef CONFIG_DEBUG_FS
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index 923ed7f..29b07fd 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -198,6 +198,7 @@ typedef struct _drm_i915_sarea {
>>>  #define DRM_I915_GEM_SET_CACHING	0x2f
>>>  #define DRM_I915_GEM_GET_CACHING	0x30
>>>  #define DRM_I915_REG_READ		0x31
>>> +#define DRM_I915_GET_RESET_STATS	0x32
>>>  
>>>  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>>>  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
>>> @@ -247,6 +248,7 @@ typedef struct _drm_i915_sarea {
>>>  #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
>>>  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
>>>  #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
>>> +#define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
>>>  
>>>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>>>   * on the security mechanisms provided by hardware.
>>> @@ -981,4 +983,19 @@ struct drm_i915_reg_read {
>>>  	__u64 offset;
>>>  	__u64 val; /* Return value */
>>>  };
>>> +
>>> +struct drm_i915_reset_stats {
>>> +	__u32 ctx_id;
>>> +	__u32 flags;
>>> +
>>> +	/* For all contexts */
>>> +	__u32 reset_count;
>>> +
>>> +	/* For this context */
>>> +	__u32 batch_active;
>>> +	__u32 batch_pending;
>>> +
>>> +	__u32 pad;
>>> +};
>>> +
>>>  #endif /* _UAPI_I915_DRM_H_ */
>>>

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Daniel Vetter]

On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>
>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>
>>> I have a couple questions...
>>>
>>> 1. Has any of this landed an a kernel tree anywhere?
>>
>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>
> And that stuff will land once my patches hit the Mesa list or ... ?

Yup.

>>> 2. Has any support code landed in a libdrm tree anywhere?
>>
>> Dunno whether Mika has libdrm patches. Since mesa is the only expected
>> user I'd just go with putting the ioctl wrapper (using the drmIoctl
>> helper) into mesa itself, that get rids of a dep for merging this support.
>
> What's the right way to get the ctx_id out of the drm_intel_context?
> That struct is private to libdrm, but the ioctl needs it.

Hm, I guess then we need something in libdrm. I've just figured for
stuff that only mesa really cares about like the hw contexts here
libdrm wrappers are a bit pointless since it adds a 2nd ABI barrier
with no real gain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Dave Airlie]

On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>
>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>
>>>> I have a couple questions...
>>>>
>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>
>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>
>> And that stuff will land once my patches hit the Mesa list or ... ?
>
> Yup.

Hey kernel first, then upstream projects, at the moment libdrm has
ioctls in it that I have no upstream solid kernel commit for,

Either in the next 24 hrs I have this in my tree or the libdrm commits
need to be reverted,

and if someone releases libdrm in that time span then I'm going to be
quite pissed.

Dave.

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Daniel Vetter]

On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>
>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>
>>>>> I have a couple questions...
>>>>>
>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>
>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>
>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>
>> Yup.
>
> Hey kernel first, then upstream projects, at the moment libdrm has
> ioctls in it that I have no upstream solid kernel commit for,
>
> Either in the next 24 hrs I have this in my tree or the libdrm commits
> need to be reverted,
>
> and if someone releases libdrm in that time span then I'm going to be
> quite pissed.

It's kinda too late imo for 3.13 (and there's an open question whether
we need one more flag or not), so I wanted to pull it in into 3.14.
Which also gives us plenty of time to add or not add that optional
flag. So I guess time to revert. Can you do that pls?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 11/07/2013 10:32 PM, Daniel Vetter wrote:
> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie <airlied@gmail.com> wrote:
>> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
>>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>>
>>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>>
>>>>>> I have a couple questions...
>>>>>>
>>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>>
>>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>>
>>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>>
>>> Yup.
>>
>> Hey kernel first, then upstream projects, at the moment libdrm has
>> ioctls in it that I have no upstream solid kernel commit for,
>>
>> Either in the next 24 hrs I have this in my tree or the libdrm commits
>> need to be reverted,
>>
>> and if someone releases libdrm in that time span then I'm going to be
>> quite pissed.
> 
> It's kinda too late imo for 3.13 (and there's an open question whether
> we need one more flag or not), so I wanted to pull it in into 3.14.
> Which also gives us plenty of time to add or not add that optional
> flag. So I guess time to revert. Can you do that pls?

Reverting has completely broken Mesa builds, and was the wrong choice.
Thanks for giving me opportunity to reply before breaking my stuff.

> -Daniel

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Dave Airlie]

On Sat, Nov 9, 2013 at 3:21 AM, Ian Romanick <idr@freedesktop.org> wrote:
> On 11/07/2013 10:32 PM, Daniel Vetter wrote:
>> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie <airlied@gmail.com> wrote:
>>> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
>>>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>>>
>>>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>>>
>>>>>>> I have a couple questions...
>>>>>>>
>>>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>>>
>>>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>>>
>>>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>>>
>>>> Yup.
>>>
>>> Hey kernel first, then upstream projects, at the moment libdrm has
>>> ioctls in it that I have no upstream solid kernel commit for,
>>>
>>> Either in the next 24 hrs I have this in my tree or the libdrm commits
>>> need to be reverted,
>>>
>>> and if someone releases libdrm in that time span then I'm going to be
>>> quite pissed.
>>
>> It's kinda too late imo for 3.13 (and there's an open question whether
>> we need one more flag or not), so I wanted to pull it in into 3.14.
>> Which also gives us plenty of time to add or not add that optional
>> flag. So I guess time to revert. Can you do that pls?
>
> Reverting has completely broken Mesa builds, and was the wrong choice.
> Thanks for giving me opportunity to reply before breaking my stuff.
>

Hey Ian,

stop merging incomplete shit 5 mins before the branch point,

you should know better, Mesa is meant to be moving to 3 mth release
cycle to avoid this kinda merge everything crap,

there was an open thread on the api for this feature, there was
questions of whether a drm cap was needed, you failed to address any
of these and you merged the feature, the rules aren't different than
before, stuff goes in the kernel first and userspace second, we went
down this road before and it always gets screwed up.

Dave.

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Jesse Barnes]

On Fri, 8 Nov 2013 07:32:05 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie <airlied@gmail.com> wrote:
> > On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
> >>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
> >>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
> >>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
> >>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
> >>>>>
> >>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
> >>>>>
> >>>>> I have a couple questions...
> >>>>>
> >>>>> 1. Has any of this landed an a kernel tree anywhere?
> >>>>
> >>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
> >>>
> >>> And that stuff will land once my patches hit the Mesa list or ... ?
> >>
> >> Yup.
> >
> > Hey kernel first, then upstream projects, at the moment libdrm has
> > ioctls in it that I have no upstream solid kernel commit for,
> >
> > Either in the next 24 hrs I have this in my tree or the libdrm commits
> > need to be reverted,
> >
> > and if someone releases libdrm in that time span then I'm going to be
> > quite pissed.
> 
> It's kinda too late imo for 3.13 (and there's an open question whether
> we need one more flag or not), so I wanted to pull it in into 3.14.
> Which also gives us plenty of time to add or not add that optional
> flag. So I guess time to revert. Can you do that pls?

This ioctl is tiny and self-contained.  So it seems like as long as the
Mesa team is good with it (i.e. using it successfully), there shouldn't
be a problem pushing it now.  There's no risk of regression as it's a
new feature and isolated.

Dave?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 11/08/2013 11:00 AM, Dave Airlie wrote:
> On Sat, Nov 9, 2013 at 3:21 AM, Ian Romanick <idr@freedesktop.org> wrote:
>> On 11/07/2013 10:32 PM, Daniel Vetter wrote:
>>> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie <airlied@gmail.com> wrote:
>>>> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick <idr@freedesktop.org> wrote:
>>>>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>>>>
>>>>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>>>>
>>>>>>>> I have a couple questions...
>>>>>>>>
>>>>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>>>>
>>>>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>>>>
>>>>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>>>>
>>>>> Yup.
>>>>
>>>> Hey kernel first, then upstream projects, at the moment libdrm has
>>>> ioctls in it that I have no upstream solid kernel commit for,
>>>>
>>>> Either in the next 24 hrs I have this in my tree or the libdrm commits
>>>> need to be reverted,
>>>>
>>>> and if someone releases libdrm in that time span then I'm going to be
>>>> quite pissed.
>>>
>>> It's kinda too late imo for 3.13 (and there's an open question whether
>>> we need one more flag or not), so I wanted to pull it in into 3.14.
>>> Which also gives us plenty of time to add or not add that optional
>>> flag. So I guess time to revert. Can you do that pls?
>>
>> Reverting has completely broken Mesa builds, and was the wrong choice.
>> Thanks for giving me opportunity to reply before breaking my stuff.
> 
> Hey Ian,
> 
> stop merging incomplete shit 5 mins before the branch point,

Stop calling other people's hard work shit.  It just makes you look like
an asshole.  Seriously.

> you should know better, Mesa is meant to be moving to 3 mth release
> cycle to avoid this kinda merge everything crap,

That's not the reason for the 3 month cycle.  The reason for the 3 month
cycle is because all of the distros, including Fedora, were pulling
random points from master because waiting 6 months for features and
performance improvements was too long.

> there was an open thread on the api for this feature, there was
> questions of whether a drm cap was needed, you failed to address any

I don't know where you're getting "drm cap" nonsense.  That was closed
two weeks ago:

http://lists.freedesktop.org/archives/intel-gfx/2013-October/035016.html

> of these and you merged the feature, the rules aren't different than
> before, stuff goes in the kernel first and userspace second, we went
> down this road before and it always gets screwed up.

I thought Daniel and I had closed that, and I Acked-by the kernel patch.
 He and I talked about this on IRC.  ***IF*** we need an extra flag for
global resets, there's already a place for it in the (currently) pad
field returned by the ioctl.

This isn't some last minute, half-baked interface.  We've been working
on this for nearly a year.  After going through several revisions,
Mika's kernel patch has been on the intel-gfx list since July.  That
people are exploding about this now just boggles my mind.

> Dave.