[PATCH 00/25] i915 HW context support

[PATCH 00/25] i915 HW context support

[Ben Widawsky]

Setting myself up for a late night crying session once again. Most of the
people reading this probably know the history and reasons for the patches. If
not, you can search the intel-gfx mailing list to try to learn more. I won't
recap the whole thing here, and instead let the patches speak for themselves.

Instead a brief review of what's here, and what's happened. Mostly,
these badly need testing and review. I've carried these around for so
long now, and seen so many different failures, I'm quite paranoid they
still aren't perfect. Also, I've spent almost all of the time working on
this in the kernel, so there is bound to be simple errors in the other
stuff.

I've run these on various workloads and saw nothing worth mentioning.


0-16: kernel patches
17-21: libdrm patches (wait render patch is temporary)
22-24: inetl-gpu-tools
25: mesa

kernel
git://people.freedesktop.org/~bwidawsk/drm-intel context_support_rev2

libdrm
git://people.freedesktop.org/~bwidawsk/drm context_support_rev2

i-g-t
git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support



-- 
1.7.10.2

[PATCH 01/25] drm/i915: CXT_SIZE register offsets added

[Ben Widawsky]

From: Ben Widawsky <bwidawsk@gmail.com>

The GPUs can have different default context layouts, and the sizes could
vary based on platform or BIOS. In order to back the context object with
a properly sized BO, we must read this register in order to find out a
sufficient size.

Thankfully (sarcarm!), the register moves and changes meanings
throughout generations.

CTX and CXT differences are intentional as that is how it is in the
documentation (prior to GEN6 it was CXT).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f45a18..66ed7e6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1454,6 +1454,27 @@
  */
 #define CCID			0x2180
 #define   CCID_EN		(1<<0)
+#define CXT_SIZE		0x21a0
+#define GEN6_CXT_POWER_SIZE(cxt_reg)	((cxt_reg >> 24) & 0x3f)
+#define GEN6_CXT_RING_SIZE(cxt_reg)	((cxt_reg >> 18) & 0x3f)
+#define GEN6_CXT_RENDER_SIZE(cxt_reg)	((cxt_reg >> 12) & 0x3f)
+#define GEN6_CXT_EXTENDED_SIZE(cxt_reg)	((cxt_reg >> 6) & 0x3f)
+#define GEN6_CXT_PIPELINE_SIZE(cxt_reg)	((cxt_reg >> 0) & 0x3f)
+#define GEN6_CXT_TOTAL_SIZE(cxt_reg)	(GEN6_CXT_POWER_SIZE(cxt_reg) + \
+					GEN6_CXT_RING_SIZE(cxt_reg) + \
+					GEN6_CXT_RENDER_SIZE(cxt_reg) + \
+					GEN6_CXT_EXTENDED_SIZE(cxt_reg) + \
+					GEN6_CXT_PIPELINE_SIZE(cxt_reg))
+#define GEN7_CTX_SIZE		0x21a8
+#define GEN7_CTX_RENDER_SIZE(ctx_reg)	((ctx_reg >> 16) & 0x3f)
+#define GEN7_CTX_EXTENDED_SIZE(ctx_reg)	((ctx_reg >> 9) & 0x7f)
+#define GEN7_CTX_GT1_SIZE(ctx_reg)	((ctx_reg >> 6) & 0x7)
+#define GEN7_CTX_VFSTATE_SIZE(ctx_reg)	((ctx_reg >> 0) & 0x3f)
+#define GEN7_CTX_TOTAL_SIZE(ctx_reg)	(GEN7_CTX_RENDER_SIZE(ctx_reg) + \
+					 GEN7_CTX_EXTENDED_SIZE(ctx_reg) + \
+					 GEN7_CTX_GT1_SIZE(ctx_reg) + \
+					 GEN7_CTX_VFSTATE_SIZE(ctx_reg))
+
 /*
  * Overlay regs
  */
-- 
1.7.10.2

[PATCH 02/25 v2] drm/i915: preliminary context support

[Ben Widawsky]

Very basic code for context setup/destruction in the driver.

Adds the file i915_gem_context.c This file implements HW context
support. On gen5+ a HW context consists of an opaque GPU object which is
referenced at times of context saves and restores.  With RC6 enabled,
the context is also referenced as the GPU enters and exists from RC6
(GPU has it's own internal power context, except on gen5).  Though
something like a context does exist for the media ring, the code only
supports contexts for the render ring.

In software, there is a distinction between contexts created by the
user, and the default HW context. The default HW context is used by GPU
clients that do not request setup of their own hardware context. The
default context's state is never restored to help prevent programming
errors. This would happen if a client ran and piggy-backed off another
clients GPU state.  The default context only exists to give the GPU some
offset to load as the current to invoke a save of the context we
actually care about. In fact, the code could likely be constructed,
albeit in a more complicated fashion, to never use the default context,
though that limits the driver's ability to swap out, and/or destroy
other contexts.

All other contexts are created as a request by the GPU client. These
contexts store GPU state, and thus allow GPU clients to not re-emit
state (and potentially query certain state) at any time. The kernel
driver makes certain that the appropriate commands are inserted.

There are 4 entry points into the contexts, init, fini, open, close.
The names are self-explanatory except that init can be called during
reset, and also during pm thaw/resume. As we expect our context to be
preserved across these events, we do not reinitialize in this case.

As Adam Jackson pointed out, The cutoff of 1MB where a HW context is
considered too big is arbitrary. The reason for this is even though
context sizes are increasing with every generation, they have yet to
eclipse even 32k. If we somehow read back way more than that, it
probably means BIOS has done something strange, or we're running on a
platform that wasn't designed for this.

v2: rename load/unload to init/fini (daniel)
remove ILK support for get_size() (indirectly daniel)
add HAS_HW_CONTEXTS macro to clarify supported platforms (daniel)
added comments (Ben)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/Makefile           |    1 +
 drivers/gpu/drm/i915/i915_dma.c         |    4 +
 drivers/gpu/drm/i915/i915_drv.c         |    1 +
 drivers/gpu/drm/i915/i915_drv.h         |   10 ++
 drivers/gpu/drm/i915/i915_gem.c         |    5 +
 drivers/gpu/drm/i915/i915_gem_context.c |  175 +++++++++++++++++++++++++++++++
 6 files changed, 196 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_gem_context.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0ca7f76..88a3b50 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -7,6 +7,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  i915_debugfs.o \
           i915_suspend.o \
 	  i915_gem.o \
+	  i915_gem_context.o \
 	  i915_gem_debug.o \
 	  i915_gem_evict.o \
 	  i915_gem_execbuffer.o \
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..3f7c8a9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1630,6 +1630,7 @@ int i915_driver_unload(struct drm_device *dev)
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
 	i915_gem_retire_requests(dev);
+	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Cancel the retire work handler, which should be idle now. */
@@ -1718,6 +1719,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
 	spin_lock_init(&file_priv->mm.lock);
 	INIT_LIST_HEAD(&file_priv->mm.request_list);
 
+	i915_gem_context_open(dev, file);
+
 	return 0;
 }
 
@@ -1750,6 +1753,7 @@ void i915_driver_lastclose(struct drm_device * dev)
 
 void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv)
 {
+	i915_gem_context_close(dev, file_priv);
 	i915_gem_release(dev, file_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d3e1948..9c81505 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -903,6 +903,7 @@ int i915_reset(struct drm_device *dev)
 		for_each_ring(ring, dev_priv, i)
 			ring->init(ring);
 
+		i915_gem_context_init(dev);
 		i915_gem_init_ppgtt(dev);
 
 		mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a17e31e..432b44f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -816,6 +816,9 @@ typedef struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+
+	bool hw_contexts_disabled;
+	uint32_t hw_context_size;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
@@ -1065,6 +1068,7 @@ struct drm_i915_file_private {
 #define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
 #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
 
+#define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >=6)
 
 #define HAS_OVERLAY(dev)		(INTEL_INFO(dev)->has_overlay)
@@ -1344,6 +1348,12 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
 
+/* i915_gem_context.c */
+void i915_gem_context_init(struct drm_device *dev);
+void i915_gem_context_fini(struct drm_device *dev);
+void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
+void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
+
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d2eaa00..28f00dc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3636,6 +3636,11 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	dev_priv->next_seqno = 1;
 
+	/*
+	 * XXX: There was some w/a described somewhere suggesting loading
+	 * contexts before PPGTT.
+	 */
+	i915_gem_context_init(dev);
 	i915_gem_init_ppgtt(dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
new file mode 100644
index 0000000..e39808e
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -0,0 +1,175 @@
+/*
+ * Copyright © 2011-2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+/*
+ * This file implements HW context support. On gen5+ a HW context consists of an
+ * opaque GPU object which is referenced at times of context saves and restores.
+ * With RC6 enabled, the context is also referenced as the GPU enters and exists
+ * from RC6 (GPU has it's own internal power context, except on gen5). Though
+ * something like a context does exist for the media ring, the code only
+ * supports contexts for the render ring.
+ *
+ * In software, there is a distinction between contexts created by the user,
+ * and the default HW context. The default HW context is used by GPU clients
+ * that do not request setup of their own hardware context. The default
+ * context's state is never restored to help prevent programming errors. This
+ * would happen if a client ran and piggy-backed off another clients GPU state.
+ * The default context only exists to give the GPU some offset to load as the
+ * current to invoke a save of the context we actually care about. In fact, the
+ * code could likely be constructed, albeit in a more complicated fashion, to
+ * never use the default context, though that limits the driver's ability to
+ * swap out, and/or destroy other contexts.
+ *
+ * All other contexts are created as a request by the GPU client. These contexts
+ * store GPU state, and thus allow GPU clients to not re-emit state (and
+ * potentially query certain state) at any time. The kernel driver makes
+ * certain that the appropriate commands are inserted.
+ *
+ * The context life cycle is semi-complicated in that context BOs may live
+ * longer than the context itself because of the way the hardware, and object
+ * tracking works. Below is a very crude representation of the state machine
+ * describing the context life.
+ *                                         refcount     pincount     active
+ * S0: initial state                          0            0           0
+ * S1: context created                        1            0           0
+ * S2: context is currently running           2            1           X
+ * S3: GPU referenced, but not current        2            0           1
+ * S4: context is current, but destroyed      1            1           0
+ * S5: like S3, but destroyed                 1            0           1
+ *
+ * The most common (but not all) transitions:
+ * S0->S1: client creates a context
+ * S1->S2: client submits execbuf with context
+ * S2->S3: other clients submits execbuf with context
+ * S3->S1: context object was retired
+ * S3->S2: clients submits another execbuf
+ * S2->S4: context destroy called with current context
+ * S3->S5->S0: destroy path
+ * S4->S5->S0: destroy path on current context
+ *
+ * There are two confusing terms used above:
+ *  The "current context" means the context which is currently running on the
+ *  GPU. The GPU has loaded it's state already and has stored away the gtt
+ *  offset of the BO. The GPU is not actively referencing the data at this
+ *  offset, but it will on the next context switch. The only way to avoid this
+ *  is to do a GPU reset.
+ *
+ *  An "active context' is one which was previously the "current context" and is
+ *  on the active list waiting for the next context switch to occur. Until this
+ *  happens, the object must remain at the same gtt offset. It is therefore
+ *  possible to destroy a context, but it is still active.
+ *
+ */
+
+#include "drmP.h"
+#include "i915_drm.h"
+#include "i915_drv.h"
+
+static int get_context_size(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+	u32 reg;
+
+	switch (INTEL_INFO(dev)->gen) {
+	case 6:
+		reg = I915_READ(CXT_SIZE);
+		ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
+		break;
+	case 7:
+		reg = I915_READ(GEN7_CTX_SIZE);
+		ret = GEN7_CTX_TOTAL_SIZE(reg) * 64;
+		break;
+	default:
+		BUG();
+	}
+
+	return ret;
+}
+
+/**
+ * The default context needs to exist per ring that uses contexts. It stores the
+ * context state of the GPU for applications that don't utilize HW contexts, as
+ * well as an idle case.
+ */
+static int create_default_context(struct drm_i915_private *dev_priv)
+{
+	return 0;
+}
+
+void i915_gem_context_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t ctx_size;
+
+	if (!HAS_HW_CONTEXTS(dev))
+		return;
+
+	/* If called from reset, or thaw... we've been here already */
+	if (dev_priv->hw_contexts_disabled)
+		return;
+
+	ctx_size = get_context_size(dev);
+	dev_priv->hw_context_size = get_context_size(dev);
+	dev_priv->hw_context_size = round_up(dev_priv->hw_context_size, 4096);
+
+	if (ctx_size <= 0 || ctx_size > (1<<20)) {
+		dev_priv->hw_contexts_disabled = true;
+		return;
+	}
+
+	if (create_default_context(dev_priv)) {
+		dev_priv->hw_contexts_disabled = true;
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("HW context support initialized\n");
+}
+
+void i915_gem_context_fini(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
+
+void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
+
+void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
-- 
1.7.10.2

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

[PATCH 03/25 v2] drm/i915: context basic create & destroy

[Ben Widawsky]

Invent an abstraction for a hw context which is passed around through
the core functions. The main bit a hw context holds is the buffer object
which backs the context. The rest of the members are just helper
functions. Specifically the ring member, which could likely go away if
we decide to never implement whatever other hw context support exists.

Of note here is the introduction of the 64k alignment constraint for the
BO. If contexts become heavily used, we should consider tweaking this
down to 4k. Until the contexts are merged and tested a bit though, I
think 64k is a nice start (based on docs).

Since we don't yet switch contexts, there is really not much complexity
here. Creation/destruction works pretty much as one would expect. An idr
is used to generate the context id numbers which are unique per file
descriptor.

v2: add DRM_DEBUG_DRIVERS to distinguish ENOMEM failures (ben)
convert a BUG_ON to WARN_ON, default destruction is still fatal (ben)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |   11 +++
 drivers/gpu/drm/i915/i915_gem_context.c |  142 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +
 3 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 432b44f..f543679 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -308,6 +308,16 @@ struct i915_hw_ppgtt {
 	dma_addr_t scratch_page_dma_addr;
 };
 
+
+/* This must match up with the value previously used for execbuf2.rsvd1. */
+#define DEFAULT_CONTEXT_ID 0
+struct i915_hw_context {
+	int id;
+	struct drm_i915_file_private *file_priv;
+	struct intel_ring_buffer *ring;
+	struct drm_i915_gem_object *obj;
+};
+
 enum no_fbc_reason {
 	FBC_NO_OUTPUT, /* no outputs enabled to compress */
 	FBC_STOLEN_TOO_SMALL, /* not enough space to hold compressed buffers */
@@ -1023,6 +1033,7 @@ struct drm_i915_file_private {
 		struct spinlock lock;
 		struct list_head request_list;
 	} mm;
+	struct idr context_idr;
 };
 
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e39808e..2aca002 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -89,6 +89,15 @@
 #include "i915_drm.h"
 #include "i915_drv.h"
 
+/* This is a HW constraint. The value below is the largest known requirement
+ * I've seen in a spec to date, and that was a workaround for a non-shipping
+ * part. It should be safe to decrease this, but it's more future proof as is.
+ */
+#define CONTEXT_ALIGN (64<<10)
+
+static struct i915_hw_context *
+i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
+
 static int get_context_size(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -111,6 +120,76 @@ static int get_context_size(struct drm_device *dev)
 	return ret;
 }
 
+static void do_destroy(struct i915_hw_context *ctx)
+{
+	struct drm_device *dev = ctx->obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (ctx->file_priv)
+		idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	else
+		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
+
+	drm_gem_object_unreference(&ctx->obj->base);
+	kfree(ctx);
+}
+
+static int
+create_hw_context(struct drm_device *dev,
+		  struct drm_i915_file_private *file_priv,
+		  struct i915_hw_context **ctx_out)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret, id;
+
+	*ctx_out = kzalloc(sizeof(struct drm_i915_file_private), GFP_KERNEL);
+	if (*ctx_out == NULL)
+		return -ENOMEM;
+
+	(*ctx_out)->obj = i915_gem_alloc_object(dev,
+						dev_priv->hw_context_size);
+	if ((*ctx_out)->obj == NULL) {
+		kfree(*ctx_out);
+		DRM_DEBUG_DRIVER("Context object allocated failed\n");
+		return -ENOMEM;
+	}
+
+	/* The ring associated with the context object is handled by the normal
+	 * object tracking code. We give an initial ring value simple to pass an
+	 * assertion in the context switch code.
+	 */
+	(*ctx_out)->ring = &dev_priv->ring[RCS];
+
+	/* Default context will never have a file_priv */
+	if (file_priv == NULL)
+		return 0;
+
+	(*ctx_out)->file_priv = file_priv;
+
+again:
+	if (idr_pre_get(&file_priv->context_idr, GFP_KERNEL) == 0) {
+		ret = -ENOMEM;
+		DRM_DEBUG_DRIVER("idr allocation failed\n");
+		goto err_out;
+	}
+
+	ret = idr_get_new_above(&file_priv->context_idr, *ctx_out,
+				DEFAULT_CONTEXT_ID + 1, &id);
+	if (ret == 0)
+		(*ctx_out)->id = id;
+
+	if (ret == -EAGAIN)
+		goto again;
+	else if (ret)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	do_destroy(*ctx_out);
+	return ret;
+}
+
 /**
  * The default context needs to exist per ring that uses contexts. It stores the
  * context state of the GPU for applications that don't utilize HW contexts, as
@@ -118,7 +197,30 @@ static int get_context_size(struct drm_device *dev)
  */
 static int create_default_context(struct drm_i915_private *dev_priv)
 {
-	return 0;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	ret = create_hw_context(dev_priv->dev, NULL,
+				&dev_priv->ring[RCS].default_context);
+	if (ret)
+		return ret;
+
+	/* We may need to do things with the shrinker which require us to
+	 * immediately switch back to the default context. This can cause a
+	 * problem as pinning the default context also requires GTT space which
+	 * may not be available. To avoid this we always pin the
+	 * default context.
+	 */
+	ctx = dev_priv->ring[RCS].default_context;
+	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
+	if (ret) {
+		do_destroy(ctx);
+		return ret;
+	}
+
+	return ret;
 }
 
 void i915_gem_context_init(struct drm_device *dev)
@@ -130,7 +232,8 @@ void i915_gem_context_init(struct drm_device *dev)
 		return;
 
 	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled)
+	if (dev_priv->hw_contexts_disabled ||
+	    dev_priv->ring[RCS].default_context)
 		return;
 
 	ctx_size = get_context_size(dev);
@@ -156,20 +259,55 @@ void i915_gem_context_fini(struct drm_device *dev)
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
+
+	do_destroy(dev_priv->ring[RCS].default_context);
 }
 
 void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	idr_init(&file_priv->context_idr);
+}
+
+static int context_idr_cleanup(int id, void *p, void *data)
+{
+	struct drm_file *file = (struct drm_file *)data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_hw_context *ctx;
+
+	BUG_ON(id == DEFAULT_CONTEXT_ID);
+	ctx = i915_gem_context_get(file_priv, id);
+	if (WARN_ON(ctx == NULL))
+		return -ENXIO;
+
+	do_destroy(ctx);
+
+	return 0;
 }
 
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	mutex_lock(&dev->struct_mutex);
+	idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
+	idr_destroy(&file_priv->context_idr);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static __used struct i915_hw_context *
+i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
+{
+	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 55d3da2..bb19bec 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -116,6 +116,8 @@ struct  intel_ring_buffer {
 
 	wait_queue_head_t irq_queue;
 
+	struct i915_hw_context *default_context;
+
 	void *private;
 };
 
-- 
1.7.10.2

[PATCH 04/25] drm/i915: add context information to objects

[Ben Widawsky]

Daniel originally shot this patch down as being a layering violation. In
fact, over time I've come to agree with him on this.

However, it is handy for debug information, and after the below commit,
I need some way to be able to not lazily bind objects to the gtt for
contexts.

commit 0ebb98299357e1dbeeea470eec29241263c8f244
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Feb 15 23:50:24 2012 +0100

    drm/i915: enable lazy global-gtt binding

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
 drivers/gpu/drm/i915/i915_gem.c         |    1 +
 drivers/gpu/drm/i915/i915_gem_context.c |    6 +++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f543679..9b309aa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -993,6 +993,11 @@ struct drm_i915_gem_object {
 	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
 	 */
 	atomic_t pending_flip;
+
+	/**
+	 * >= 0 if this object is the object for a context.
+	 */
+	int context_id;
 };
 
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 28f00dc..2142e9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3439,6 +3439,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	obj->madv = I915_MADV_WILLNEED;
 	/* Avoid an unnecessary call to unbind on the first bind. */
 	obj->map_and_fenceable = true;
+	obj->context_id = -1;
 
 	return obj;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2aca002..bf57123 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -162,7 +162,7 @@ create_hw_context(struct drm_device *dev,
 
 	/* Default context will never have a file_priv */
 	if (file_priv == NULL)
-		return 0;
+		goto out;
 
 	(*ctx_out)->file_priv = file_priv;
 
@@ -183,6 +183,8 @@ again:
 	else if (ret)
 		goto err_out;
 
+out:
+	(*ctx_out)->obj->context_id = (*ctx_out)->id;
 	return 0;
 
 err_out:
@@ -214,6 +216,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	 * default context.
 	 */
 	ctx = dev_priv->ring[RCS].default_context;
+	ctx->id = DEFAULT_CONTEXT_ID;
+	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
 	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
 	if (ret) {
 		do_destroy(ctx);
-- 
1.7.10.2

[PATCH 05/25] drm/i915: always bind context objects immediately

[Ben Widawsky]

Context objects must be bound into the global gtt. So we can't get all
fancy with ppgtt.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2142e9c..044dc65 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2720,7 +2720,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		goto search_free;
 	}
 
-	if (!dev_priv->mm.aliasing_ppgtt)
+	if (!dev_priv->mm.aliasing_ppgtt || obj->context_id >= 0)
 		i915_gem_gtt_bind_object(obj, obj->cache_level);
 
 	list_add_tail(&obj->gtt_list, &dev_priv->mm.gtt_list);
-- 
1.7.10.2

[PATCH 06/25 v3] drm/i915: context switch implementation

[Ben Widawsky]

Implement the context switch code as well as the interfaces to do the
context switch. This patch also doesn't match 1:1 with the RFC patches.
The main difference is that from Daniel's responses the last context
object is now stored instead of the last context. This aids in allows us
to free the context data structure, and context object independently.

There is room for optimization: this code will pin the context object
until the next context is active. The optimal way to do it is to
actually pin the object, move it to the active list, do the context
switch, and then unpin it. This allows the eviction code to actually
evict the context object if needed.

The context switch code is missing workarounds, they will be implemented
in future patches.

v2: actually do obj->dirty=1 in switch (daniel)
Modified comment around above
Remove flags to context switch (daniel)
Move mi_set_context code to i915_gem_context.c (daniel)
Remove seqno , use lazy request instead (daniel)

v3: use i915_gem_request_next_seqno instead of
      outstanding_lazy_request (Daniel)
remove id's from trace events (Daniel)
Put the context BO in the instruction domain (Daniel)
Don't unref the BO is context switch fails (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |    3 +
 drivers/gpu/drm/i915/i915_gem_context.c |  146 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
 3 files changed, 149 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b309aa..568d147 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,6 +313,7 @@ struct i915_hw_ppgtt {
 #define DEFAULT_CONTEXT_ID 0
 struct i915_hw_context {
 	int id;
+	bool is_initialized;
 	struct drm_i915_file_private *file_priv;
 	struct intel_ring_buffer *ring;
 	struct drm_i915_gem_object *obj;
@@ -1369,6 +1370,8 @@ void i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
+int i915_switch_context(struct intel_ring_buffer *ring,
+			struct drm_file *file, int to_id);
 
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index bf57123..aabe7d9 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -192,6 +192,11 @@ err_out:
 	return ret;
 }
 
+static inline bool is_default_context(struct i915_hw_context *ctx)
+{
+	return (ctx == ctx->ring->default_context);
+}
+
 /**
  * The default context needs to exist per ring that uses contexts. It stores the
  * context state of the GPU for applications that don't utilize HW contexts, as
@@ -310,8 +315,147 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static __used struct i915_hw_context *
+static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
 	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
 }
+
+static inline int
+mi_set_context(struct intel_ring_buffer *ring,
+	       struct i915_hw_context *new_context,
+	       u32 hw_flags)
+{
+	int ret;
+
+	ret = intel_ring_begin(ring, 4);
+	if (ret)
+		return ret;
+
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_emit(ring, MI_SET_CONTEXT);
+	intel_ring_emit(ring, new_context->obj->gtt_offset |
+			MI_MM_SPACE_GTT |
+			MI_SAVE_EXT_STATE_EN |
+			MI_RESTORE_EXT_STATE_EN |
+			hw_flags);
+	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
+	intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+
+	return ret;
+}
+
+static int do_switch(struct drm_i915_gem_object *from_obj,
+		     struct i915_hw_context *to,
+		     u32 seqno)
+{
+	struct intel_ring_buffer *ring = NULL;
+	u32 hw_flags = 0;
+	int ret;
+
+	BUG_ON(to == NULL);
+	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
+
+	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
+	if (ret)
+		return ret;
+
+	if (!to->is_initialized || is_default_context(to))
+		hw_flags |= MI_RESTORE_INHIBIT;
+	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
+		hw_flags |= MI_FORCE_RESTORE;
+
+	ring = to->ring;
+	ret = mi_set_context(ring, to, hw_flags);
+	if (ret) {
+		i915_gem_object_unpin(to->obj);
+		return ret;
+	}
+
+	/* The backing object for the context is done after switching to the
+	 * *next* context. Therefore we cannot retire the previous context until
+	 * the next context has already started running. In fact, the below code
+	 * is a bit suboptimal because the retiring can occur simply after the
+	 * MI_SET_CONTEXT instead of when the next seqno has completed.
+	 */
+	if (from_obj != NULL) {
+		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		i915_gem_object_move_to_active(from_obj, ring, seqno);
+		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
+		 * whole damn pipeline, we don't need to explicitly mark the
+		 * object dirty. The only exception is that the context must be
+		 * correct in case the object gets swapped out. Ideally we'd be
+		 * able to defer doing this until we know the object would be
+		 * swapped, but there is no way to do that yet.
+		 */
+		from_obj->dirty = 1;
+		BUG_ON(from_obj->ring != to->ring);
+		i915_gem_object_unpin(from_obj);
+	}
+
+	ring->last_context_obj = to->obj;
+	to->is_initialized = true;
+
+	return 0;
+}
+
+/**
+ * i915_switch_context() - perform a GPU context switch.
+ * @ring: ring for which we'll execute the context switch
+ * @file_priv: file_priv associated with the context, may be NULL
+ * @id: context id number
+ * @seqno: sequence number by which the new context will be switched to
+ * @flags:
+ *
+ * The context life cycle is simple. The context refcount is incremented and
+ * decremented by 1 and create and destroy. If the context is in use by the GPU,
+ * it will have a refoucnt > 1. This allows us to destroy the context abstract
+ * object while letting the normal object tracking destroy the backing BO.
+ */
+int i915_switch_context(struct intel_ring_buffer *ring,
+			struct drm_file *file,
+			int to_id)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_file_private *file_priv = NULL;
+	struct i915_hw_context *to;
+	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
+	int ret;
+
+	if (dev_priv->hw_contexts_disabled)
+		return 0;
+
+	if (ring != &dev_priv->ring[RCS])
+		return 0;
+
+	if (file)
+		file_priv = file->driver_priv;
+
+	if (to_id == DEFAULT_CONTEXT_ID) {
+		to = ring->default_context;
+	} else {
+		to = i915_gem_context_get(file_priv, to_id);
+		if (to == NULL)
+			return -EINVAL;
+	}
+
+	if (from_obj == to->obj)
+		return 0;
+
+	ret = do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
+	if (ret)
+		return ret;
+
+	/* Just to make the code a little cleaner we take the object reference
+	 * after the switch was successful. It would be more intuitive to ref
+	 * the 'to' object before the switch but we know the refcount must be >0
+	 * if context_get() succeeded, and we hold struct mutex. So it's safe to
+	 * do this here/now
+	 */
+	drm_gem_object_reference(&to->obj->base);
+	if (from_obj != NULL)
+		drm_gem_object_unreference(&from_obj->base);
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index bb19bec..b7884b9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -117,6 +117,7 @@ struct  intel_ring_buffer {
 	wait_queue_head_t irq_queue;
 
 	struct i915_hw_context *default_context;
+	struct drm_i915_gem_object *last_context_obj;
 
 	void *private;
 };
-- 
1.7.10.2

[PATCH 07/25] drm/i915: context trace events

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |    4 +++
 drivers/gpu/drm/i915/i915_trace.h       |   55 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index aabe7d9..388cf62 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -130,6 +130,8 @@ static void do_destroy(struct i915_hw_context *ctx)
 	else
 		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
 
+	trace_i915_context_destroy(ctx);
+
 	drm_gem_object_unreference(&ctx->obj->base);
 	kfree(ctx);
 }
@@ -185,6 +187,7 @@ again:
 
 out:
 	(*ctx_out)->obj->context_id = (*ctx_out)->id;
+	trace_i915_context_create(*ctx_out);
 	return 0;
 
 err_out:
@@ -398,6 +401,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
 	ring->last_context_obj = to->obj;
 	to->is_initialized = true;
 
+	trace_i915_context_switch(from_obj, to);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index fe90b3a..6d8412c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -434,6 +434,61 @@ TRACE_EVENT(i915_reg_rw,
 		(u32)(__entry->val >> 32))
 );
 
+/* Context tracking */
+TRACE_EVENT(i915_context_create,
+	    TP_PROTO(struct i915_hw_context *ctx),
+	    TP_ARGS(ctx),
+
+	    TP_STRUCT__entry(
+			     __field(struct i915_hw_context *, ctx)
+			     __field(u32, id)
+			     __field(struct drm_i915_gem_object *, obj)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->ctx = ctx;
+			   __entry->id = ctx->id;
+			   __entry->obj = ctx->obj;
+			   ),
+
+	    TP_printk("ctx=%p, id=%u, obj=%p",
+		      __entry->ctx, __entry->id, __entry->obj)
+);
+
+TRACE_EVENT(i915_context_destroy,
+	    TP_PROTO(struct i915_hw_context *ctx),
+	    TP_ARGS(ctx),
+
+	    TP_STRUCT__entry(
+			     __field(struct i915_hw_context *, ctx)
+			     __field(u32, id)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->ctx = ctx;
+			   __entry->id = ctx->id;
+			   ),
+
+	    TP_printk("ctx=%p, id=%u", __entry->ctx, __entry->id)
+);
+
+TRACE_EVENT(i915_context_switch,
+	    TP_PROTO(struct drm_i915_gem_object *from, struct i915_hw_context *to),
+	    TP_ARGS(from, to),
+
+	    TP_STRUCT__entry(
+			     __field(int, from)
+			     __field(int, to)
+			    ),
+
+	    TP_fast_assign(
+			   __entry->from = from->context_id;
+			   __entry->to = to->id;
+			   ),
+
+	    TP_printk("context switch from %d to %d",
+		      __entry->from, __entry->to)
+);
 #endif /* _I915_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
1.7.10.2

[PATCH 08/25] drm/i915: Ivybridge MI_ARB_ON_OFF context w/a

[Ben Widawsky]

The workaround itself applies to gen7 only (according to the docs) and
as Eric Anholt points out shouldn't be required since we don't use HW
scheduling features, and therefore arbitration. Though since it is a
small, and simple addition, and we don't really understand the issue,
just do it.

FWIW, I eventually want to play with some of the arbitration stuff, and
I'd hate to forget about this.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   12 +++++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |    3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 388cf62..06e7264 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -331,10 +331,15 @@ mi_set_context(struct intel_ring_buffer *ring,
 {
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
 
+	if (IS_GEN7(ring->dev))
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+	else
+		intel_ring_emit(ring, MI_NOOP);
+
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
 	intel_ring_emit(ring, new_context->obj->gtt_offset |
@@ -345,6 +350,11 @@ mi_set_context(struct intel_ring_buffer *ring,
 	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
 	intel_ring_emit(ring, MI_NOOP);
 
+	if (IS_GEN7(ring->dev))
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
+	else
+		intel_ring_emit(ring, MI_NOOP);
+
 	intel_ring_advance(ring);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 66ed7e6..700e148 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -210,6 +210,9 @@
 #define MI_DISPLAY_FLIP		MI_INSTR(0x14, 2)
 #define MI_DISPLAY_FLIP_I915	MI_INSTR(0x14, 1)
 #define   MI_DISPLAY_FLIP_PLANE(n) ((n) << 20)
+#define MI_ARB_ON_OFF		MI_INSTR(0x08, 0)
+#define   MI_ARB_ENABLE			(1<<0)
+#define   MI_ARB_DISABLE		(0<<0)
 #define MI_SET_CONTEXT		MI_INSTR(0x18, 0)
 #define   MI_MM_SPACE_GTT		(1<<8)
 #define   MI_MM_SPACE_PHYSICAL		(0<<8)
-- 
1.7.10.2

[PATCH 09/25] drm/i915: PIPE_CONTROL_TLB_INVALIDATE

[Ben Widawsky]

This has showed up in several other patches. It's required for the next
context workaround.

I tested this one on its own and saw no differences in basic tests
(performance or otherwise). This patch is relatively likely to cause
regressions, hence why it's split out.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h         |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 700e148..496bd25 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -294,6 +294,7 @@
 #define   DISPLAY_PLANE_B           (1<<20)
 #define GFX_OP_PIPE_CONTROL(len)	((0x3<<29)|(0x3<<27)|(0x2<<24)|(len-2))
 #define   PIPE_CONTROL_CS_STALL				(1<<20)
+#define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
 #define   PIPE_CONTROL_QW_WRITE				(1<<14)
 #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
 #define   PIPE_CONTROL_WRITE_FLUSH			(1<<12)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1df1694..96265e0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -226,6 +226,7 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	 * impact.
 	 */
 	flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH;
+	flags |= PIPE_CONTROL_TLB_INVALIDATE;
 	flags |= PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE;
 	flags |= PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE;
 	flags |= PIPE_CONTROL_DEPTH_CACHE_FLUSH;
-- 
1.7.10.2

[PATCH 10/25 v2] drm/i915: possibly invalidate TLB before context switch

[Ben Widawsky]

>From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf

[DevSNB] If Flush TLB invalidation Mode is enabled it's the driver's
responsibility to invalidate the TLBs at least once after the previous
context switch after any GTT mappings changed (including new GTT
entries).  This can be done by a pipelined PIPE_CONTROL with TLB inv bit
set immediately before MI_SET_CONTEXT.

On GEN7 the invalidation mode is explicitly set, but this appears to be
lacking for GEN6. Since I don't know the history on this, I've decided
to dynamically read the value at ring init time, and use that value
throughout.

v2: better comment (daniel)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   11 +++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    7 +++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    4 ++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 06e7264..89376d8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -331,6 +331,17 @@ mi_set_context(struct intel_ring_buffer *ring,
 {
 	int ret;
 
+	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
+	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
+	 * explicitly, so we rely on the value at ring init, stored in
+	 * itlb_before_ctx_switch.
+	 */
+	if (IS_GEN6(ring->dev) && ring->itlb_before_ctx_switch) {
+		ret = ring->flush(ring, 0, 0);
+		if (ret)
+			return ret;
+	}
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 96265e0..763f457 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -423,6 +423,13 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 		 */
 		I915_WRITE(CACHE_MODE_0,
 			   _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
+
+		/* This is not explicitly set for GEN6, so read the register.
+		 * see intel_ring_mi_set_context() for why we care.
+		 * TODO: consider explicitly setting the bit for GEN5
+		 */
+		ring->itlb_before_ctx_switch =
+			!!(I915_READ(GFX_MODE) & GFX_TLB_INVALIDATE_ALWAYS);
 	}
 
 	if (INTEL_INFO(dev)->gen >= 6)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b7884b9..594c9c4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -116,6 +116,10 @@ struct  intel_ring_buffer {
 
 	wait_queue_head_t irq_queue;
 
+	/**
+	 * Do an explicit TLB flush before MI_SET_CONTEXT
+	 */
+	bool itlb_before_ctx_switch;
 	struct i915_hw_context *default_context;
 	struct drm_i915_gem_object *last_context_obj;
 
-- 
1.7.10.2

[PATCH 11/25] drm/i915: use the default context

[Ben Widawsky]

With the code to do HW context switches in place have the driver load the
default context for the render ring when the driver loads.

The default context will be an ever present context that is available to
switch to at any time for the given ring.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 89376d8..d824faf 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,6 +97,8 @@
 
 static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
+static int do_switch(struct drm_i915_gem_object *from_obj,
+		     struct i915_hw_context *to, u32 seqno);
 
 static int get_context_size(struct drm_device *dev)
 {
@@ -232,6 +234,14 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 		return ret;
 	}
 
+	ret = do_switch(NULL, ctx, 0);
+	if (ret) {
+		i915_gem_object_unpin(ctx->obj);
+		do_destroy(ctx);
+	} else {
+		DRM_DEBUG_DRIVER("Default HW context loaded\n");
+	}
+
 	return ret;
 }
 
-- 
1.7.10.2

[PATCH 12/25] drm/i915: add ccid to error state

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    1 +
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_irq.c     |    1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index eb2b3c2..3864079 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -713,6 +713,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	seq_printf(m, "EIR: 0x%08x\n", error->eir);
 	seq_printf(m, "IER: 0x%08x\n", error->ier);
 	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
+	seq_printf(m, "CCID: 0x%08x\n", error->ccid);
 
 	for (i = 0; i < dev_priv->num_fence_regs; i++)
 		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 568d147..bf36d96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -176,6 +176,7 @@ struct drm_i915_error_state {
 	u32 eir;
 	u32 pgtbl_er;
 	u32 ier;
+	u32 ccid;
 	bool waiting[I915_NUM_RINGS];
 	u32 pipestat[I915_MAX_PIPES];
 	u32 tail[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5134a62..5515ea6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1096,6 +1096,7 @@ static void i915_capture_error_state(struct drm_device *dev)
 	kref_init(&error->ref);
 	error->eir = I915_READ(EIR);
 	error->pgtbl_er = I915_READ(PGTBL_ER);
+	error->ccid = I915_READ(CCID);
 
 	if (HAS_PCH_SPLIT(dev))
 		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
-- 
1.7.10.2

[PATCH 13/25 v3] drm/i915: switch to default context on idle

[Ben Widawsky]

To keep things as sane as possible, switch to the default context before
idling. This should help free context objects, as well as put things in
a more well defined state before suspending.

v2: remove seqno from context switch call (daniel)
return error on failed context switch instead of WARN+continue (daniel)

v3: move idling to i915_gpu idle (from i915_gem_idle) (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 044dc65..5f57f8a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2281,6 +2281,10 @@ int i915_gpu_idle(struct drm_device *dev)
 		/* Is the device fubar? */
 		if (WARN_ON(!list_empty(&ring->gpu_write_list)))
 			return -EBUSY;
+
+		ret = i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
1.7.10.2

[PATCH 14/25 v3] drm/i915/context: create & destroy ioctls

[Ben Widawsky]

Add the interfaces to allow user space to create and destroy contexts.
Contexts are destroyed automatically if the file descriptor for the dri
device is closed.

Following convention as usual here causes checkpatch warnings.

v2: with is_initialized, no longer need to init at create
drop the context switch on create (daniel)

v3: Use interruptible lock (Chris)
return -ENODEV in !GEM case (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c         |    2 ++
 drivers/gpu/drm/i915/i915_drv.h         |    4 +++
 drivers/gpu/drm/i915/i915_gem_context.c |   55 +++++++++++++++++++++++++++++++
 include/drm/i915_drm.h                  |   15 +++++++++
 4 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 3f7c8a9..b0c50b3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1808,6 +1808,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
+	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),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf36d96..9185154 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1373,6 +1373,10 @@ void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
 			struct drm_file *file, int to_id);
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file);
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index d824faf..13e7395 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -494,3 +494,58 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 		drm_gem_object_unreference(&from_obj->base);
 	return ret;
 }
+
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_context_create *args = data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	if (!(dev->driver->driver_features & DRIVER_GEM))
+		return -ENODEV;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	ret = create_hw_context(dev, file_priv, &ctx);
+	mutex_unlock(&dev->struct_mutex);
+
+	args->ctx_id = ctx->id;
+	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
+
+	return ret;
+}
+
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file)
+{
+	struct drm_i915_gem_context_destroy *args = data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	if (!(dev->driver->driver_features & DRIVER_GEM))
+		return -ENODEV;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	ctx = i915_gem_context_get(file_priv, args->ctx_id);
+	if (!ctx) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	do_destroy(ctx);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
+	return 0;
+}
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index bab1743..6f02451 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -201,6 +201,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GET_SPRITE_COLORKEY	0x2a
 #define DRM_I915_SET_SPRITE_COLORKEY	0x2b
 #define DRM_I915_GEM_WAIT	0x2c
+#define DRM_I915_GEM_CONTEXT_CREATE	0x2d
+#define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
 
 #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)
@@ -245,6 +247,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_SET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
+#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)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -896,4 +900,15 @@ struct drm_i915_gem_wait {
 	__u64 timeout_ns;
 };
 
+struct drm_i915_gem_context_create {
+	/*  output: id of new context*/
+	__u32 ctx_id;
+	__u32 pad;
+};
+
+struct drm_i915_gem_context_destroy {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.10.2

[PATCH 15/25 v2] drm/i915/context: switch contexts with execbuf2

[Ben Widawsky]

Use the rsvd1 field in execbuf2 to specify the context ID associated
with the workload. This will allow the driver to do the proper context
switch when/if needed.

v2: Add checks for context switches on rings not supporting contexts.
Before the code would silently ignore such requests.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   16 ++++++++++++++++
 include/drm/i915_drm.h                     |    8 +++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 974a9f1..f32d024 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1044,6 +1044,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *batch_obj;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
+	u32 ctx_id = i915_execbuffer2_get_context_id(*args);
 	u32 exec_start, exec_len;
 	u32 seqno;
 	u32 mask;
@@ -1065,9 +1066,19 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		break;
 	case I915_EXEC_BSD:
 		ring = &dev_priv->ring[VCS];
+		if (ctx_id != 0) {
+			DRM_DEBUG("Ring %s doesn't support contexts\n",
+				  ring->name);
+			return -EPERM;
+		}
 		break;
 	case I915_EXEC_BLT:
 		ring = &dev_priv->ring[BCS];
+		if (ctx_id != 0) {
+			DRM_DEBUG("Ring %s doesn't support contexts\n",
+				  ring->name);
+			return -EPERM;
+		}
 		break;
 	default:
 		DRM_DEBUG("execbuf with unknown ring: %d\n",
@@ -1261,6 +1272,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
+	ret = i915_switch_context(ring, file, ctx_id);
+	if (ret)
+		goto err;
+
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
 	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
@@ -1367,6 +1382,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 	exec2.num_cliprects = args->num_cliprects;
 	exec2.cliprects_ptr = args->cliprects_ptr;
 	exec2.flags = I915_EXEC_RENDER;
+	i915_execbuffer2_set_context_id(exec2, 0);
 
 	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
 	if (!ret) {
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 6f02451..ed82b7f 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -662,13 +662,19 @@ struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
 	__u64 flags;
-	__u64 rsvd1;
+	__u64 rsvd1; /* now used for context info */
 	__u64 rsvd2;
 };
 
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
 
+#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
+#define i915_execbuffer2_set_context_id(eb2, context) \
+	(eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
+#define i915_execbuffer2_get_context_id(eb2) \
+	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
1.7.10.2

[PATCH 16/25] drm/i915: reset the GPU on context fini

[Ben Widawsky]

It's the only way we know how to make the GPU actually forget about the
default context.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c         |    2 +-
 drivers/gpu/drm/i915/i915_drv.h         |    1 +
 drivers/gpu/drm/i915/i915_gem_context.c |    2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9c81505..d1c9f8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -799,7 +799,7 @@ static int gen6_do_reset(struct drm_device *dev)
 	return ret;
 }
 
-static int intel_gpu_reset(struct drm_device *dev)
+int intel_gpu_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret = -ENODEV;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9185154..225f626 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1180,6 +1180,7 @@ extern long i915_compat_ioctl(struct file *filp, unsigned int cmd,
 extern int i915_emit_box(struct drm_device *dev,
 			 struct drm_clip_rect *box,
 			 int DR1, int DR4);
+extern int intel_gpu_reset(struct drm_device *dev);
 extern int i915_reset(struct drm_device *dev);
 extern unsigned long i915_chipset_val(struct drm_i915_private *dev_priv);
 extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 13e7395..b16ff66 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -285,6 +285,8 @@ void i915_gem_context_fini(struct drm_device *dev)
 	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
 
 	do_destroy(dev_priv->ring[RCS].default_context);
+
+	intel_gpu_reset(dev);
 }
 
 void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
-- 
1.7.10.2

[PATCH 17/25] intel: wait render placeholder

[Ben Widawsky]

The patches have already been sucked in to the kernel. So we need a
placeholder for this IOCTL until the libdrm wait render patches land.
---
 include/drm/i915_drm.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index af3ce17..6d9a9f1 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -191,6 +191,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_EXECBUFFER2	0x29
 #define DRM_I915_GET_SPRITE_COLORKEY	0x2a
 #define DRM_I915_SET_SPRITE_COLORKEY	0x2b
+#define DRM_I915_GEM_WAIT	0x2c
 
 #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)
-- 
1.7.10.2

[PATCH 18/25] intel: Merge updated kernel header

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 include/drm/i915_drm.h |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 6d9a9f1..0ca2d4f 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -192,6 +192,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GET_SPRITE_COLORKEY	0x2a
 #define DRM_I915_SET_SPRITE_COLORKEY	0x2b
 #define DRM_I915_GEM_WAIT	0x2c
+#define DRM_I915_GEM_CONTEXT_CREATE	0x2d
+#define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
 
 #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)
@@ -236,6 +238,9 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_SET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 
+#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)
+
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
  */
@@ -647,13 +652,19 @@ struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
 	__u64 flags;
-	__u64 rsvd1;
+	__u64 rsvd1; /* now used for context info */
 	__u64 rsvd2;
 };
 
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
 
+#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
+#define i915_execbuffer2_set_context_id(eb2, context) \
+	(eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
+#define i915_execbuffer2_get_context_id(eb2) \
+	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
@@ -877,4 +888,15 @@ struct drm_intel_sprite_colorkey {
 	__u32 flags;
 };
 
+struct drm_i915_gem_context_create {
+	/*  output: id of new context*/
+	__u32 ctx_id;
+	__u32 pad;
+};
+
+struct drm_i915_gem_context_destroy {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.10.2

[PATCH 19/25] intel/context: Add drm_intel_context type

[Ben Widawsky]

Add an opaque type representing a HW context.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_bufmgr.h      |    1 +
 intel/intel_bufmgr_priv.h |    5 +++++
 2 files changed, 6 insertions(+)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index c197abc..83a43cb 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -41,6 +41,7 @@
 struct drm_clip_rect;
 
 typedef struct _drm_intel_bufmgr drm_intel_bufmgr;
+typedef struct _drm_intel_context drm_intel_context;
 typedef struct _drm_intel_bo drm_intel_bo;
 
 struct _drm_intel_bo {
diff --git a/intel/intel_bufmgr_priv.h b/intel/intel_bufmgr_priv.h
index 0b62520..2592d42 100644
--- a/intel/intel_bufmgr_priv.h
+++ b/intel/intel_bufmgr_priv.h
@@ -280,6 +280,11 @@ struct _drm_intel_bufmgr {
 	int debug;
 };
 
+struct _drm_intel_context {
+	unsigned int ctx_id;
+	struct _drm_intel_bufmgr *bufmgr;
+};
+
 #define ALIGN(value, alignment)	((value + alignment - 1) & ~(alignment - 1))
 #define ROUND_UP_TO(x, y)	(((x) + (y) - 1) / (y) * (y))
 #define ROUND_UP_TO_MB(x)	ROUND_UP_TO((x), 1024*1024)
-- 
1.7.10.2

[PATCH 20/25] intel/context: new execbuf interface for contexts

[Ben Widawsky]

To support this we extract the common execbuf2 functionality to be
called with, or without contexts.

The context'd execbuf does not support some of the dri1 stuff.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_bufmgr.h     |    5 +++++
 intel/intel_bufmgr_gem.c |   32 +++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 83a43cb..20338c7 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -186,6 +186,11 @@ int drm_intel_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id);
 int drm_intel_get_aperture_sizes(int fd, size_t *mappable, size_t *total);
 int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
 
+drm_intel_context *drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr);
+void drm_intel_gem_context_destroy(drm_intel_context *ctx);
+int drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
+				  int used, unsigned int flags);
+
 /* drm_intel_bufmgr_fake.c */
 drm_intel_bufmgr *drm_intel_bufmgr_fake_init(int fd,
 					     unsigned long low_offset,
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index b776d2f..e74ac0a 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -2134,9 +2134,9 @@ drm_intel_gem_bo_exec(drm_intel_bo *bo, int used,
 }
 
 static int
-drm_intel_gem_bo_mrb_exec2(drm_intel_bo *bo, int used,
-			drm_clip_rect_t *cliprects, int num_cliprects, int DR4,
-			unsigned int flags)
+do_exec2(drm_intel_bo *bo, int used, drm_intel_context *ctx,
+	 drm_clip_rect_t *cliprects, int num_cliprects, int DR4,
+	 unsigned int flags)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr;
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -2178,7 +2178,10 @@ drm_intel_gem_bo_mrb_exec2(drm_intel_bo *bo, int used,
 	execbuf.DR1 = 0;
 	execbuf.DR4 = DR4;
 	execbuf.flags = flags;
-	execbuf.rsvd1 = 0;
+	if (ctx == NULL)
+		i915_execbuffer2_set_context_id(execbuf, 0);
+	else
+		i915_execbuffer2_set_context_id(execbuf, ctx->ctx_id);
 	execbuf.rsvd2 = 0;
 
 	aub_exec(bo, flags, used);
@@ -2226,9 +2229,24 @@ drm_intel_gem_bo_exec2(drm_intel_bo *bo, int used,
 		       drm_clip_rect_t *cliprects, int num_cliprects,
 		       int DR4)
 {
-	return drm_intel_gem_bo_mrb_exec2(bo, used,
-					cliprects, num_cliprects, DR4,
-					I915_EXEC_RENDER);
+	return do_exec2(bo, used, NULL, cliprects, num_cliprects, DR4,
+			I915_EXEC_RENDER);
+}
+
+static int
+drm_intel_gem_bo_mrb_exec2(drm_intel_bo *bo, int used,
+			drm_clip_rect_t *cliprects, int num_cliprects, int DR4,
+			unsigned int flags)
+{
+	return do_exec2(bo, used, NULL, cliprects, num_cliprects, DR4,
+			flags);
+}
+
+int
+drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
+			      int used, unsigned int flags)
+{
+	return do_exec2(bo, used, ctx, NULL, 0, 0, flags);
 }
 
 static int
-- 
1.7.10.2

[PATCH 21/25] intel: add decoding of MI_SET_CONTEXT

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_decode.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index bf23706..5f90a47 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -139,6 +139,22 @@ instr_out(struct drm_intel_decode *ctx, unsigned int index,
 }
 
 static int
+decode_MI_SET_CONTEXT(struct drm_intel_decode *ctx)
+{
+	uint32_t data = ctx->data[1];
+	if (ctx->gen > 7)
+		return 1;
+
+	instr_out(ctx, 0, "MI_SET_CONTEXT\n");
+	instr_out(ctx, 1, "gtt offset = 0x%x%s%s\n",
+		  data & ~0xfff,
+		  data & (1<<1)? ", Force Restore": "",
+		  data & (1<<0)? ", Restore Inhibit": "");
+
+	return 2;
+}
+
+static int
 decode_MI_WAIT_FOR_EVENT(struct drm_intel_decode *ctx)
 {
 	const char *cc_wait;
@@ -233,7 +249,7 @@ decode_mi(struct drm_intel_decode *ctx)
 		{ 0x00, 0, 1, 1, "MI_NOOP" },
 		{ 0x11, 0x3f, 2, 2, "MI_OVERLAY_FLIP" },
 		{ 0x07, 0, 1, 1, "MI_REPORT_HEAD" },
-		{ 0x18, 0x3f, 2, 2, "MI_SET_CONTEXT" },
+		{ 0x18, 0x3f, 2, 2, "MI_SET_CONTEXT", decode_MI_SET_CONTEXT },
 		{ 0x20, 0x3f, 3, 4, "MI_STORE_DATA_IMM" },
 		{ 0x21, 0x3f, 3, 4, "MI_STORE_DATA_INDEX" },
 		{ 0x24, 0x3f, 3, 3, "MI_STORE_REGISTER_MEM" },
-- 
1.7.10.2

[PATCH 22/25] context: libdrm wrappers

[Ben Widawsky]

This wraps libdrm functionality to create, destroy, and exec with
contexts. This patch shouldn't be applied until libdrm for contexts is
updated.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 lib/drmtest.c           |   16 ++++++++++++++++
 lib/drmtest.h           |    2 ++
 lib/intel_batchbuffer.c |   40 ++++++++++++++++++++++++++++++++++++----
 lib/intel_batchbuffer.h |    2 ++
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index ebc396f..eb429b5 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -294,6 +294,22 @@ uint32_t gem_create(int fd, int size)
 	return create.handle;
 }
 
+uint32_t gem_context_create(int fd)
+{
+	struct drm_i915_gem_context_create create;
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create);
+	assert(create.ctx_id);
+
+	return create.ctx_id;
+}
+
+void gem_context_destroy(int fd, uint32_t ctx_id)
+{
+	struct drm_i915_gem_context_destroy destroy;
+	destroy.ctx_id = ctx_id;
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, &destroy);
+}
+
 void *gem_mmap(int fd, uint32_t handle, int size, int prot)
 {
 	struct drm_i915_gem_mmap_gtt mmap_arg;
diff --git a/lib/drmtest.h b/lib/drmtest.h
index f4462a2..170b420 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -50,6 +50,8 @@ void gem_set_domain(int fd, uint32_t handle,
 		    uint32_t read_domains, uint32_t write_domain);
 void gem_sync(int fd, uint32_t handle);
 uint32_t gem_create(int fd, int size);
+uint32_t gem_context_create(int fd);
+void gem_context_destroy(int fd, uint32_t ctx_id);
 void *gem_mmap(int fd, uint32_t handle, int size, int prot);
 uint64_t gem_aperture_size(int fd);
 uint64_t gem_mappable_aperture_size(void);
diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 325066e..d9d49b6 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -75,13 +75,13 @@ intel_batchbuffer_free(struct intel_batchbuffer *batch)
 
 #define CMD_POLY_STIPPLE_OFFSET       0x7906
 
-void
-intel_batchbuffer_flush_on_ring(struct intel_batchbuffer *batch, int ring)
+static unsigned int
+flush_on_ring_common(struct intel_batchbuffer *batch, int ring)
 {
 	unsigned int used = batch->ptr - batch->buffer;
 
 	if (used == 0)
-		return;
+		return 0;
 
 	if (IS_GEN5(batch->devid)) {
 		BEGIN_BATCH(2);
@@ -99,7 +99,17 @@ intel_batchbuffer_flush_on_ring(struct intel_batchbuffer *batch, int ring)
 	/* Mark the end of the buffer. */
 	*(uint32_t *)(batch->ptr) = MI_BATCH_BUFFER_END; /* noop */
 	batch->ptr += 4;
-	used = batch->ptr - batch->buffer;
+	return batch->ptr - batch->buffer;
+}
+
+void
+intel_batchbuffer_flush_on_ring(struct intel_batchbuffer *batch, int ring)
+{
+	int ret;
+	unsigned int used = flush_on_ring_common(batch, ring);
+
+	if (used == 0)
+		return;
 
 	do_or_die(drm_intel_bo_subdata(batch->bo, 0, used, batch->buffer));
 
@@ -111,6 +121,28 @@ intel_batchbuffer_flush_on_ring(struct intel_batchbuffer *batch, int ring)
 }
 
 void
+intel_batchbuffer_flush_with_context(struct intel_batchbuffer *batch,
+				     drm_intel_context *context)
+{
+	int ret;
+	unsigned int used = flush_on_ring_common(batch, I915_EXEC_RENDER);
+
+	if (used == 0)
+		return;
+
+	ret = drm_intel_bo_subdata(batch->bo, 0, used, batch->buffer);
+	assert(ret == 0);
+
+	batch->ptr = NULL;
+
+	ret = drm_intel_gem_bo_context_exec(batch->bo, context, used,
+					    I915_EXEC_RENDER);
+	assert(ret == 0);
+
+	intel_batchbuffer_reset(batch);
+}
+
+void
 intel_batchbuffer_flush(struct intel_batchbuffer *batch)
 {
 	int ring = 0;
diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
index 4dffda7..97224b5 100644
--- a/lib/intel_batchbuffer.h
+++ b/lib/intel_batchbuffer.h
@@ -25,6 +25,8 @@ void intel_batchbuffer_free(struct intel_batchbuffer *batch);
 
 void intel_batchbuffer_flush(struct intel_batchbuffer *batch);
 void intel_batchbuffer_flush_on_ring(struct intel_batchbuffer *batch, int ring);
+void intel_batchbuffer_flush_with_context(struct intel_batchbuffer *batch,
+					  drm_intel_context *context);
 
 void intel_batchbuffer_reset(struct intel_batchbuffer *batch);
 
-- 
1.7.10.2

[PATCH 23/25] context: update for new execbuf2 element

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 lib/drmtest.c                    |    2 +-
 tests/gem_bad_length.c           |    2 +-
 tests/gem_exec_blt.c             |    2 +-
 tests/gem_exec_faulting_reloc.c  |    2 +-
 tests/gem_exec_nop.c             |    2 +-
 tests/gem_linear_blits.c         |    3 ++-
 tests/gen3_mixed_blits.c         |    6 ++++--
 tests/gen3_render_linear_blits.c |    3 ++-
 tests/gen3_render_mixed_blits.c  |    3 ++-
 tests/gen3_render_tiledx_blits.c |    3 ++-
 tests/gen3_render_tiledy_blits.c |    3 ++-
 11 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index eb429b5..69b7c8d 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -102,7 +102,7 @@ void gem_quiescent_gpu(int fd)
 	execbuf.DR1 = 0;
 	execbuf.DR4 = 0;
 	execbuf.flags = 0;
-	execbuf.rsvd1 = 0;
+	i915_execbuffer2_set_context_id(execbuf, 0);
 	execbuf.rsvd2 = 0;
 
 	do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf);
diff --git a/tests/gem_bad_length.c b/tests/gem_bad_length.c
index 512effa..41f44d7 100644
--- a/tests/gem_bad_length.c
+++ b/tests/gem_bad_length.c
@@ -107,7 +107,7 @@ static void exec0(int fd)
 	execbuf.DR1 = 0;
 	execbuf.DR4 = 0;
 	execbuf.flags = 0;
-	execbuf.rsvd1 = 0;
+	i915_execbuffer2_set_context_id(execbuf, 0);
 	execbuf.rsvd2 = 0;
 
 	printf("trying to run an empty batchbuffer\n");
diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
index 472065c..eb5ae66 100644
--- a/tests/gem_exec_blt.c
+++ b/tests/gem_exec_blt.c
@@ -224,7 +224,7 @@ static void run(int object_size)
 	execbuf.DR1 = 0;
 	execbuf.DR4 = 0;
 	execbuf.flags = ring;
-	execbuf.rsvd1 = 0;
+	i915_execbuffer2_set_context_id(execbuf, 0);
 	execbuf.rsvd2 = 0;
 
 	for (count = 1; count <= 1<<17; count <<= 1) {
diff --git a/tests/gem_exec_faulting_reloc.c b/tests/gem_exec_faulting_reloc.c
index e67056d..863a1b0 100644
--- a/tests/gem_exec_faulting_reloc.c
+++ b/tests/gem_exec_faulting_reloc.c
@@ -207,7 +207,7 @@ static void run(int object_size)
 	execbuf.DR1 = 0;
 	execbuf.DR4 = 0;
 	execbuf.flags = ring;
-	execbuf.rsvd1 = 0;
+	i915_execbuffer2_set_context_id(execbuf, 0);
 	execbuf.rsvd2 = 0;
 
 	gem_exec(fd, &execbuf);
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d2b35ed..9dd055c 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -75,7 +75,7 @@ static int exec(int fd, uint32_t handle, int loops)
 	execbuf.DR1 = 0;
 	execbuf.DR4 = 0;
 	execbuf.flags = 0;
-	execbuf.rsvd1 = 0;
+	i915_execbuffer2_set_context_id(execbuf, 0);
 	execbuf.rsvd2 = 0;
 
 	while (loops-- && ret == 0) {
diff --git a/tests/gem_linear_blits.c b/tests/gem_linear_blits.c
index ec2ba26..fe15f1d 100644
--- a/tests/gem_linear_blits.c
+++ b/tests/gem_linear_blits.c
@@ -130,7 +130,8 @@ copy(int fd, uint32_t dst, uint32_t src)
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = HAS_BLT_RING(intel_get_drm_devid(fd)) ? I915_EXEC_BLT : 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
diff --git a/tests/gen3_mixed_blits.c b/tests/gen3_mixed_blits.c
index 0e2fdbc..5bb6d86 100644
--- a/tests/gen3_mixed_blits.c
+++ b/tests/gen3_mixed_blits.c
@@ -312,7 +312,8 @@ render_copy(int fd,
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
@@ -389,7 +390,8 @@ static void blt_copy(int fd, uint32_t dst, uint32_t src)
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
diff --git a/tests/gen3_render_linear_blits.c b/tests/gen3_render_linear_blits.c
index f474789..529e23f 100644
--- a/tests/gen3_render_linear_blits.c
+++ b/tests/gen3_render_linear_blits.c
@@ -281,7 +281,8 @@ copy(int fd, uint32_t dst, uint32_t src)
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
diff --git a/tests/gen3_render_mixed_blits.c b/tests/gen3_render_mixed_blits.c
index 1c3d528..1353b9d 100644
--- a/tests/gen3_render_mixed_blits.c
+++ b/tests/gen3_render_mixed_blits.c
@@ -295,7 +295,8 @@ copy(int fd,
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
diff --git a/tests/gen3_render_tiledx_blits.c b/tests/gen3_render_tiledx_blits.c
index c9e6ff9..0e96e79 100644
--- a/tests/gen3_render_tiledx_blits.c
+++ b/tests/gen3_render_tiledx_blits.c
@@ -282,7 +282,8 @@ copy(int fd, uint32_t dst, uint32_t src)
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
diff --git a/tests/gen3_render_tiledy_blits.c b/tests/gen3_render_tiledy_blits.c
index 922ea1d..90fc7eb 100644
--- a/tests/gen3_render_tiledy_blits.c
+++ b/tests/gen3_render_tiledy_blits.c
@@ -282,7 +282,8 @@ copy(int fd, uint32_t dst, uint32_t src)
 	exec.num_cliprects = 0;
 	exec.cliprects_ptr = 0;
 	exec.flags = 0;
-	exec.rsvd1 = exec.rsvd2 = 0;
+	i915_execbuffer2_set_context_id(exec, 0);
+	exec.rsvd2 = 0;
 
 	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
 	while (ret && errno == EBUSY) {
-- 
1.7.10.2

[PATCH 24/25] contexts: basic test coverage

[Ben Widawsky]

I'm not sure we want to keep some of these. Mostly here to show what
testing I have done already.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/Makefile.am |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9ec07c6..bc33ab0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -61,6 +61,11 @@ TESTS_progs = \
 	drm_vma_limiter_gtt \
 	drm_vma_limiter_cached \
 	sysfs_rc6_residency \
+	gem_ctx_basic \
+	gem_ctx_exec \
+	gem_ctx_bad_destroy \
+	gem_ctx_create \
+	gem_ctx_bad_exec \
 	$(NULL)
 
 # IMPORTANT: The ZZ_ tests need to be run last!
@@ -113,3 +118,6 @@ AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS)
 
 gem_fence_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 gem_fence_thrash_LDADD = $(LDADD) -lpthread
+
+gem_ctx_basic_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
+gem_ctx_basic_LDADD = $(LDADD) -lpthread
-- 
1.7.10.2

[PATCH 25/25] i965: hw context support

[Ben Widawsky]

Based off of a patch from Ken Graunke. I just modified it for a more
modern mesa (also don't allow contexts on blit ring).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 src/mesa/drivers/dri/i965/brw_context.c        |    1 +
 src/mesa/drivers/dri/i965/brw_vtbl.c           |    5 ++++-
 src/mesa/drivers/dri/intel/intel_batchbuffer.c |    9 +++++++--
 src/mesa/drivers/dri/intel/intel_context.c     |    2 ++
 src/mesa/drivers/dri/intel/intel_context.h     |    2 +-
 5 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
index f7073cd..d4159c7 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -298,6 +298,7 @@ brwCreateContext(int api,
 
    brw->prim_restart.in_progress = false;
    brw->prim_restart.enable_cut_index = false;
+   intel->hw_ctx = drm_intel_gem_context_create(intel->bufmgr);
 
    brw_init_state( brw );
 
diff --git a/src/mesa/drivers/dri/i965/brw_vtbl.c b/src/mesa/drivers/dri/i965/brw_vtbl.c
index 5699749..d9fd2cb 100644
--- a/src/mesa/drivers/dri/i965/brw_vtbl.c
+++ b/src/mesa/drivers/dri/i965/brw_vtbl.c
@@ -170,7 +170,10 @@ static void brw_new_batch( struct intel_context *intel )
     * This is probably not as severe as on 915, since almost all of our state
     * is just in referenced buffers.
     */
-   brw->state.dirty.brw |= BRW_NEW_CONTEXT | BRW_NEW_BATCH;
+   if (intel->hw_ctx == NULL)
+      brw->state.dirty.brw |= BRW_NEW_CONTEXT;
+
+   brw->state.dirty.brw |= BRW_NEW_BATCH;
 
    /* Assume that the last command before the start of our batch was a
     * primitive, for safety.
diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
index 76a69f7..7ba141d 100644
--- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
@@ -188,8 +188,13 @@ do_flush_locked(struct intel_context *intel)
       if (ret == 0) {
          if (unlikely(INTEL_DEBUG & DEBUG_AUB) && intel->vtbl.annotate_aub)
             intel->vtbl.annotate_aub(intel);
-	 ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0, 0,
-				     flags);
+	 if (intel->hw_ctx == NULL || batch->is_blit) {
+	    ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0, 0,
+					flags);
+	 } else {
+	    ret = drm_intel_gem_bo_context_exec(batch->bo, intel->hw_ctx,
+					    4 * batch->used, flags);
+	 }
       }
    }
 
diff --git a/src/mesa/drivers/dri/intel/intel_context.c b/src/mesa/drivers/dri/intel/intel_context.c
index 9deb4ca..46c2492 100644
--- a/src/mesa/drivers/dri/intel/intel_context.c
+++ b/src/mesa/drivers/dri/intel/intel_context.c
@@ -593,6 +593,8 @@ intelInitContext(struct intel_context *intel,
    if (intelScreen->bufmgr == NULL)
       return false;
 
+   intel->hw_ctx = NULL;
+
    /* Can't rely on invalidate events, fall back to glViewport hack */
    if (!driContextPriv->driScreenPriv->dri2.useInvalidate) {
       intel->saved_viewport = functions->Viewport;
diff --git a/src/mesa/drivers/dri/intel/intel_context.h b/src/mesa/drivers/dri/intel/intel_context.h
index cc3ee0d..c026fea 100644
--- a/src/mesa/drivers/dri/intel/intel_context.h
+++ b/src/mesa/drivers/dri/intel/intel_context.h
@@ -226,7 +226,7 @@ struct intel_context
    int urb_size;
 
    struct intel_batchbuffer batch;
-
+   drm_intel_context *hw_ctx;
    drm_intel_bo *first_post_swapbuffers_batch;
    bool need_throttle;
    bool no_batch_wrap;
-- 
1.7.10.2

Re: [PATCH 25/25] i965: hw context support

[Paul Berry]

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

On 4 June 2012 14:43, Ben Widawsky <ben@bwidawsk.net> wrote:

> Based off of a patch from Ken Graunke. I just modified it for a more
> modern mesa (also don't allow contexts on blit ring).
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_context.c        |    1 +
>  src/mesa/drivers/dri/i965/brw_vtbl.c           |    5 ++++-
>  src/mesa/drivers/dri/intel/intel_batchbuffer.c |    9 +++++++--
>  src/mesa/drivers/dri/intel/intel_context.c     |    2 ++
>  src/mesa/drivers/dri/intel/intel_context.h     |    2 +-
>  5 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> index f7073cd..d4159c7 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -298,6 +298,7 @@ brwCreateContext(int api,
>
>    brw->prim_restart.in_progress = false;
>    brw->prim_restart.enable_cut_index = false;
> +   intel->hw_ctx = drm_intel_gem_context_create(intel->bufmgr);
>
>    brw_init_state( brw );
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vtbl.c
> b/src/mesa/drivers/dri/i965/brw_vtbl.c
> index 5699749..d9fd2cb 100644
> --- a/src/mesa/drivers/dri/i965/brw_vtbl.c
> +++ b/src/mesa/drivers/dri/i965/brw_vtbl.c
> @@ -170,7 +170,10 @@ static void brw_new_batch( struct intel_context
> *intel )
>     * This is probably not as severe as on 915, since almost all of our
> state
>     * is just in referenced buffers.
>     */
> -   brw->state.dirty.brw |= BRW_NEW_CONTEXT | BRW_NEW_BATCH;
> +   if (intel->hw_ctx == NULL)
> +      brw->state.dirty.brw |= BRW_NEW_CONTEXT;
> +
> +   brw->state.dirty.brw |= BRW_NEW_BATCH;
>

The comment above this change ("Mark all context state as needing to be
re-emitted.") is no longer accurate.  Perhaps change it to something like
this?

"If the kernel supports hardware contexts, then most hardware state is
preserved between batches; we only need to re-emit state that is required
to be in every batch.  Otherwise we need to re-emit all the state that
would otherwise be stored in the context (which for all intents and
purposes means everything)."

Also, I think it would be ok to delete the comment "This is probably not as
severe as on 915 ... referenced buffers"--that comment is mostly just a
rationalization for not having implemented hardware context support
earlier, and not a very convincing one at that :)


>
>    /* Assume that the last command before the start of our batch was a
>     * primitive, for safety.
> diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> index 76a69f7..7ba141d 100644
> --- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> @@ -188,8 +188,13 @@ do_flush_locked(struct intel_context *intel)
>       if (ret == 0) {
>          if (unlikely(INTEL_DEBUG & DEBUG_AUB) && intel->vtbl.annotate_aub)
>             intel->vtbl.annotate_aub(intel);
> -        ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0, 0,
> -                                    flags);
> +        if (intel->hw_ctx == NULL || batch->is_blit) {
> +           ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0,
> 0,
> +                                       flags);
> +        } else {
> +           ret = drm_intel_gem_bo_context_exec(batch->bo, intel->hw_ctx,
> +                                           4 * batch->used, flags);
> +        }
>       }
>    }
>
> diff --git a/src/mesa/drivers/dri/intel/intel_context.c
> b/src/mesa/drivers/dri/intel/intel_context.c
> index 9deb4ca..46c2492 100644
> --- a/src/mesa/drivers/dri/intel/intel_context.c
> +++ b/src/mesa/drivers/dri/intel/intel_context.c
> @@ -593,6 +593,8 @@ intelInitContext(struct intel_context *intel,
>    if (intelScreen->bufmgr == NULL)
>       return false;
>
> +   intel->hw_ctx = NULL;
> +
>    /* Can't rely on invalidate events, fall back to glViewport hack */
>    if (!driContextPriv->driScreenPriv->dri2.useInvalidate) {
>       intel->saved_viewport = functions->Viewport;
> diff --git a/src/mesa/drivers/dri/intel/intel_context.h
> b/src/mesa/drivers/dri/intel/intel_context.h
> index cc3ee0d..c026fea 100644
> --- a/src/mesa/drivers/dri/intel/intel_context.h
> +++ b/src/mesa/drivers/dri/intel/intel_context.h
> @@ -226,7 +226,7 @@ struct intel_context
>    int urb_size;
>
>    struct intel_batchbuffer batch;
> -
> +   drm_intel_context *hw_ctx;
>    drm_intel_bo *first_post_swapbuffers_batch;
>    bool need_throttle;
>    bool no_batch_wrap;
> --
> 1.7.10.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

I'm kind of surprised to see a call to drm_intel_gem_context_create(), but
no call anywhere to a clean-up function that destroys the context.  Was
that an oversight, or is there a reason why it's unnecessary?  If it's the
latter, a comment in brw_destroy_context() would be helpful.

[-- Attachment #1.2: Type: text/html, Size: 6145 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: [Intel-gfx] [PATCH 25/25] i965: hw context support

[Ben Widawsky]

On Mon, 4 Jun 2012 16:01:54 -0700
Paul Berry <stereotype441@gmail.com> wrote:

> On 4 June 2012 14:43, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Based off of a patch from Ken Graunke. I just modified it for a more
> > modern mesa (also don't allow contexts on blit ring).
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.c        |    1 +
> >  src/mesa/drivers/dri/i965/brw_vtbl.c           |    5 ++++-
> >  src/mesa/drivers/dri/intel/intel_batchbuffer.c |    9 +++++++--
> >  src/mesa/drivers/dri/intel/intel_context.c     |    2 ++
> >  src/mesa/drivers/dri/intel/intel_context.h     |    2 +-
> >  5 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > b/src/mesa/drivers/dri/i965/brw_context.c
> > index f7073cd..d4159c7 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -298,6 +298,7 @@ brwCreateContext(int api,
> >
> >    brw->prim_restart.in_progress = false;
> >    brw->prim_restart.enable_cut_index = false;
> > +   intel->hw_ctx = drm_intel_gem_context_create(intel->bufmgr);
> >
> >    brw_init_state( brw );
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vtbl.c
> > b/src/mesa/drivers/dri/i965/brw_vtbl.c
> > index 5699749..d9fd2cb 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vtbl.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vtbl.c
> > @@ -170,7 +170,10 @@ static void brw_new_batch( struct intel_context
> > *intel )
> >     * This is probably not as severe as on 915, since almost all of our
> > state
> >     * is just in referenced buffers.
> >     */
> > -   brw->state.dirty.brw |= BRW_NEW_CONTEXT | BRW_NEW_BATCH;
> > +   if (intel->hw_ctx == NULL)
> > +      brw->state.dirty.brw |= BRW_NEW_CONTEXT;
> > +
> > +   brw->state.dirty.brw |= BRW_NEW_BATCH;
> >
> 
> The comment above this change ("Mark all context state as needing to be
> re-emitted.") is no longer accurate.  Perhaps change it to something like
> this?
> 
> "If the kernel supports hardware contexts, then most hardware state is
> preserved between batches; we only need to re-emit state that is required
> to be in every batch.  Otherwise we need to re-emit all the state that
> would otherwise be stored in the context (which for all intents and
> purposes means everything)."
> 
> Also, I think it would be ok to delete the comment "This is probably not as
> severe as on 915 ... referenced buffers"--that comment is mostly just a
> rationalization for not having implemented hardware context support
> earlier, and not a very convincing one at that :)
> 
> 
> >
> >    /* Assume that the last command before the start of our batch was a
> >     * primitive, for safety.
> > diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > index 76a69f7..7ba141d 100644
> > --- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > @@ -188,8 +188,13 @@ do_flush_locked(struct intel_context *intel)
> >       if (ret == 0) {
> >          if (unlikely(INTEL_DEBUG & DEBUG_AUB) && intel->vtbl.annotate_aub)
> >             intel->vtbl.annotate_aub(intel);
> > -        ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0, 0,
> > -                                    flags);
> > +        if (intel->hw_ctx == NULL || batch->is_blit) {
> > +           ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0,
> > 0,
> > +                                       flags);
> > +        } else {
> > +           ret = drm_intel_gem_bo_context_exec(batch->bo, intel->hw_ctx,
> > +                                           4 * batch->used, flags);
> > +        }
> >       }
> >    }
> >
> > diff --git a/src/mesa/drivers/dri/intel/intel_context.c
> > b/src/mesa/drivers/dri/intel/intel_context.c
> > index 9deb4ca..46c2492 100644
> > --- a/src/mesa/drivers/dri/intel/intel_context.c
> > +++ b/src/mesa/drivers/dri/intel/intel_context.c
> > @@ -593,6 +593,8 @@ intelInitContext(struct intel_context *intel,
> >    if (intelScreen->bufmgr == NULL)
> >       return false;
> >
> > +   intel->hw_ctx = NULL;
> > +
> >    /* Can't rely on invalidate events, fall back to glViewport hack */
> >    if (!driContextPriv->driScreenPriv->dri2.useInvalidate) {
> >       intel->saved_viewport = functions->Viewport;
> > diff --git a/src/mesa/drivers/dri/intel/intel_context.h
> > b/src/mesa/drivers/dri/intel/intel_context.h
> > index cc3ee0d..c026fea 100644
> > --- a/src/mesa/drivers/dri/intel/intel_context.h
> > +++ b/src/mesa/drivers/dri/intel/intel_context.h
> > @@ -226,7 +226,7 @@ struct intel_context
> >    int urb_size;
> >
> >    struct intel_batchbuffer batch;
> > -
> > +   drm_intel_context *hw_ctx;
> >    drm_intel_bo *first_post_swapbuffers_batch;
> >    bool need_throttle;
> >    bool no_batch_wrap;
> > --
> > 1.7.10.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> I'm kind of surprised to see a call to drm_intel_gem_context_create(), but
> no call anywhere to a clean-up function that destroys the context.  Was
> that an oversight, or is there a reason why it's unnecessary?  If it's the
> latter, a comment in brw_destroy_context() would be helpful.

Destroying a context is not required, though it is desirable. The
context will be destroyed when the DRM file handle is closed. I think I
had that info in a comment somewhere, but I forget exactly where it is.
I'd propose mesa people find a good place for destroy as the best
solution :-)


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 00/25] i915 HW context support

[Dave Airlie]

>
> I've run these on various workloads and saw nothing worth mentioning.

Nothing at all? no speedups, slowdowns, etc

why should we merge all this code then :-)

Dave.

Re: [Mesa-dev] [PATCH 00/25] i915 HW context support

[Kenneth Graunke]

On 06/04/2012 10:53 PM, Dave Airlie wrote:
>>
>> I've run these on various workloads and saw nothing worth mentioning.
> 
> Nothing at all? no speedups, slowdowns, etc
> 
> why should we merge all this code then :-)
> 
> Dave.

Preserving hardware state across batches is going to be necessary for:

* Transform feedback in the presence of geometry shaders

  Right now, Mesa counts the number of primitives emitted on the CPU,
  relies on the kernel resetting the register to 0, and offset values
  in software.  With GS, we obviously can't count primitives on the CPU.

* Primitive restart in hardware

and seems to be increasingly necessary for new features as we move
forward.  So we'd like to get it in place; we can cut more state uploads
and tune Mesa further once it's there.

Re: [PATCH 02/25 v2] drm/i915: preliminary context support

[Ville Syrjälä]

On Mon, Jun 04, 2012 at 02:42:42PM -0700, Ben Widawsky wrote:
> Very basic code for context setup/destruction in the driver.
> 
> Adds the file i915_gem_context.c This file implements HW context
> support. On gen5+ a HW context consists of an opaque GPU object which is
> referenced at times of context saves and restores.  With RC6 enabled,
> the context is also referenced as the GPU enters and exists from RC6
> (GPU has it's own internal power context, except on gen5).  Though
> something like a context does exist for the media ring, the code only
> supports contexts for the render ring.
> 
> In software, there is a distinction between contexts created by the
> user, and the default HW context. The default HW context is used by GPU
> clients that do not request setup of their own hardware context. The
> default context's state is never restored to help prevent programming
> errors. This would happen if a client ran and piggy-backed off another
> clients GPU state.  The default context only exists to give the GPU some
> offset to load as the current to invoke a save of the context we
> actually care about. In fact, the code could likely be constructed,
> albeit in a more complicated fashion, to never use the default context,
> though that limits the driver's ability to swap out, and/or destroy
> other contexts.

I'm not actually familiar with the GPU state management in the driver
yet, but I assume that currently much of the state has to be uploaded
every time a client submits rendering commands to the GPU.

So would it make sense to have an implicit context created for each
client? That way each client could just pretend that it's the only user
of the GPU and wouldn't have to worry about someone else corrupting the
GPU state.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 02/25 v2] drm/i915: preliminary context support

[Ben Widawsky]

On Tue, 5 Jun 2012 15:23:26 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Jun 04, 2012 at 02:42:42PM -0700, Ben Widawsky wrote:
> > Very basic code for context setup/destruction in the driver.
> > 
> > Adds the file i915_gem_context.c This file implements HW context
> > support. On gen5+ a HW context consists of an opaque GPU object which is
> > referenced at times of context saves and restores.  With RC6 enabled,
> > the context is also referenced as the GPU enters and exists from RC6
> > (GPU has it's own internal power context, except on gen5).  Though
> > something like a context does exist for the media ring, the code only
> > supports contexts for the render ring.
> > 
> > In software, there is a distinction between contexts created by the
> > user, and the default HW context. The default HW context is used by GPU
> > clients that do not request setup of their own hardware context. The
> > default context's state is never restored to help prevent programming
> > errors. This would happen if a client ran and piggy-backed off another
> > clients GPU state.  The default context only exists to give the GPU some
> > offset to load as the current to invoke a save of the context we
> > actually care about. In fact, the code could likely be constructed,
> > albeit in a more complicated fashion, to never use the default context,
> > though that limits the driver's ability to swap out, and/or destroy
> > other contexts.
> 
> I'm not actually familiar with the GPU state management in the driver
> yet, but I assume that currently much of the state has to be uploaded
> every time a client submits rendering commands to the GPU.

That's correct.

> 
> So would it make sense to have an implicit context created for each
> client? That way each client could just pretend that it's the only user
> of the GPU and wouldn't have to worry about someone else corrupting the
> GPU state.
> 

An earlier implementation of context support had such an idea. And even
Daniel brought it up again recently. It's certainly not a bad idea. The
only reason to do it is what you mention, regarding corruption
prevention. The real way to do that though requires proper PPGTT
support. Until we have that, I think this point isn't so important.

Old userspace isn't ever going to stop emitting all of it's state, and
new clients will still have to be able to fall back to using older
kernels. As such, the idea of being able to remove the "emit everything"
from userspace won't likely go away any time soon.

Another reason not to do it is while context objects are small now, they
do increase in size in coming generations. If we begin creating a lot of
contexts, we potentially need a lot more GTT to handle them.

I think we should discuss this again when real PPGTT support is in
place.


-- 
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: [Mesa-dev] [PATCH 00/25] i915 HW context support

[Ben Widawsky]

On Tue, 05 Jun 2012 00:08:10 -0700
Kenneth Graunke <kenneth@whitecape.org> wrote:

> On 06/04/2012 10:53 PM, Dave Airlie wrote:
> >>
> >> I've run these on various workloads and saw nothing worth mentioning.
> > 
> > Nothing at all? no speedups, slowdowns, etc
> > 
> > why should we merge all this code then :-)
> > 
> > Dave.
> 
> Preserving hardware state across batches is going to be necessary for:
> 
> * Transform feedback in the presence of geometry shaders
> 
>   Right now, Mesa counts the number of primitives emitted on the CPU,
>   relies on the kernel resetting the register to 0, and offset values
>   in software.  With GS, we obviously can't count primitives on the CPU.
> 
> * Primitive restart in hardware
> 
> and seems to be increasingly necessary for new features as we move
> forward.  So we'd like to get it in place; we can cut more state uploads
> and tune Mesa further once it's there.

Also, my testing was far from comprehensive.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Mesa-dev] [PATCH 00/25] i915 HW context support

[Kenneth Graunke]

On 06/04/2012 02:42 PM, Ben Widawsky wrote:
> Setting myself up for a late night crying session once again. Most of the
> people reading this probably know the history and reasons for the patches. If
> not, you can search the intel-gfx mailing list to try to learn more. I won't
> recap the whole thing here, and instead let the patches speak for themselves.
> 
> Instead a brief review of what's here, and what's happened. Mostly,
> these badly need testing and review. I've carried these around for so
> long now, and seen so many different failures, I'm quite paranoid they
> still aren't perfect. Also, I've spent almost all of the time working on
> this in the kernel, so there is bound to be simple errors in the other
> stuff.
> 
> I've run these on various workloads and saw nothing worth mentioning.
> 
> 
> 0-16: kernel patches

> 17-21: libdrm patches (wait render patch is temporary)

Patches 17-21 look fine.
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

> 22-24: intel-gpu-tools
> 25: mesa

Patch 25 looks fine too.  Feel free to apply some combination of:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

I do like Paul's comment updates, so I suggest merging those.

> kernel
> git://people.freedesktop.org/~bwidawsk/drm-intel context_support_rev2
> 
> libdrm
> git://people.freedesktop.org/~bwidawsk/drm context_support_rev2
> 
> i-g-t
> git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support

Re: [PATCH 00/25] i915 HW context support

[Ben Widawsky]

On Tue, 05 Jun 2012 16:37:20 -0700
Kenneth Graunke <kenneth@whitecape.org> wrote:

> On 06/04/2012 02:42 PM, Ben Widawsky wrote:
> > Setting myself up for a late night crying session once again. Most of the
> > people reading this probably know the history and reasons for the patches. If
> > not, you can search the intel-gfx mailing list to try to learn more. I won't
> > recap the whole thing here, and instead let the patches speak for themselves.
> > 
> > Instead a brief review of what's here, and what's happened. Mostly,
> > these badly need testing and review. I've carried these around for so
> > long now, and seen so many different failures, I'm quite paranoid they
> > still aren't perfect. Also, I've spent almost all of the time working on
> > this in the kernel, so there is bound to be simple errors in the other
> > stuff.
> > 
> > I've run these on various workloads and saw nothing worth mentioning.
> > 
> > 
> > 0-16: kernel patches
> 
> > 17-21: libdrm patches (wait render patch is temporary)
> 
> Patches 17-21 look fine.
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
> 
> > 22-24: intel-gpu-tools
> > 25: mesa
> 
> Patch 25 looks fine too.  Feel free to apply some combination of:
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> 
> I do like Paul's comment updates, so I suggest merging those.

Ken, would you do these for me on patch 25? It's really your patch
anyhow, I just made ever so slight modifications :-)

> 
> > kernel
> > git://people.freedesktop.org/~bwidawsk/drm-intel context_support_rev2
> > 
> > libdrm
> > git://people.freedesktop.org/~bwidawsk/drm context_support_rev2
> > 
> > i-g-t
> > git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 04/25] drm/i915: add context information to objects

[Daniel Vetter]

On Mon, Jun 04, 2012 at 02:42:44PM -0700, Ben Widawsky wrote:
> Daniel originally shot this patch down as being a layering violation. In
> fact, over time I've come to agree with him on this.
> 
> However, it is handy for debug information, and after the below commit,
> I need some way to be able to not lazily bind objects to the gtt for
> contexts.
> 
> commit 0ebb98299357e1dbeeea470eec29241263c8f244
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Feb 15 23:50:24 2012 +0100
> 
>     drm/i915: enable lazy global-gtt binding
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

That is imo a really neat layering violation in the following patch. I'll
shot down both patches. Just call i915_gem_gtt_bind_object right before
using the object - object_bind_to_gtt only allocates the gtt area.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
>  drivers/gpu/drm/i915/i915_gem.c         |    1 +
>  drivers/gpu/drm/i915/i915_gem_context.c |    6 +++++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f543679..9b309aa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -993,6 +993,11 @@ struct drm_i915_gem_object {
>  	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
>  	 */
>  	atomic_t pending_flip;
> +
> +	/**
> +	 * >= 0 if this object is the object for a context.
> +	 */
> +	int context_id;
>  };
>  
>  #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 28f00dc..2142e9c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3439,6 +3439,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  	obj->madv = I915_MADV_WILLNEED;
>  	/* Avoid an unnecessary call to unbind on the first bind. */
>  	obj->map_and_fenceable = true;
> +	obj->context_id = -1;
>  
>  	return obj;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2aca002..bf57123 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -162,7 +162,7 @@ create_hw_context(struct drm_device *dev,
>  
>  	/* Default context will never have a file_priv */
>  	if (file_priv == NULL)
> -		return 0;
> +		goto out;
>  
>  	(*ctx_out)->file_priv = file_priv;
>  
> @@ -183,6 +183,8 @@ again:
>  	else if (ret)
>  		goto err_out;
>  
> +out:
> +	(*ctx_out)->obj->context_id = (*ctx_out)->id;
>  	return 0;
>  
>  err_out:
> @@ -214,6 +216,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  	 * default context.
>  	 */
>  	ctx = dev_priv->ring[RCS].default_context;
> +	ctx->id = DEFAULT_CONTEXT_ID;
> +	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
>  	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
>  	if (ret) {
>  		do_destroy(ctx);
> -- 
> 1.7.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 07/25] drm/i915: context trace events

[Daniel Vetter]

On Mon, Jun 04, 2012 at 02:42:47PM -0700, Ben Widawsky wrote:
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I'll punt on that one because I've punted on the obj->context_id patch,
too. Imo we could switch the context_switch tracepoint to object pointers
for both to and from (and maybe keep to->id for convenience).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |    4 +++
>  drivers/gpu/drm/i915/i915_trace.h       |   55 +++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index aabe7d9..388cf62 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -130,6 +130,8 @@ static void do_destroy(struct i915_hw_context *ctx)
>  	else
>  		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
>  
> +	trace_i915_context_destroy(ctx);
> +
>  	drm_gem_object_unreference(&ctx->obj->base);
>  	kfree(ctx);
>  }
> @@ -185,6 +187,7 @@ again:
>  
>  out:
>  	(*ctx_out)->obj->context_id = (*ctx_out)->id;
> +	trace_i915_context_create(*ctx_out);
>  	return 0;
>  
>  err_out:
> @@ -398,6 +401,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  	ring->last_context_obj = to->obj;
>  	to->is_initialized = true;
>  
> +	trace_i915_context_switch(from_obj, to);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index fe90b3a..6d8412c 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -434,6 +434,61 @@ TRACE_EVENT(i915_reg_rw,
>  		(u32)(__entry->val >> 32))
>  );
>  
> +/* Context tracking */
> +TRACE_EVENT(i915_context_create,
> +	    TP_PROTO(struct i915_hw_context *ctx),
> +	    TP_ARGS(ctx),
> +
> +	    TP_STRUCT__entry(
> +			     __field(struct i915_hw_context *, ctx)
> +			     __field(u32, id)
> +			     __field(struct drm_i915_gem_object *, obj)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->ctx = ctx;
> +			   __entry->id = ctx->id;
> +			   __entry->obj = ctx->obj;
> +			   ),
> +
> +	    TP_printk("ctx=%p, id=%u, obj=%p",
> +		      __entry->ctx, __entry->id, __entry->obj)
> +);
> +
> +TRACE_EVENT(i915_context_destroy,
> +	    TP_PROTO(struct i915_hw_context *ctx),
> +	    TP_ARGS(ctx),
> +
> +	    TP_STRUCT__entry(
> +			     __field(struct i915_hw_context *, ctx)
> +			     __field(u32, id)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->ctx = ctx;
> +			   __entry->id = ctx->id;
> +			   ),
> +
> +	    TP_printk("ctx=%p, id=%u", __entry->ctx, __entry->id)
> +);
> +
> +TRACE_EVENT(i915_context_switch,
> +	    TP_PROTO(struct drm_i915_gem_object *from, struct i915_hw_context *to),
> +	    TP_ARGS(from, to),
> +
> +	    TP_STRUCT__entry(
> +			     __field(int, from)
> +			     __field(int, to)
> +			    ),
> +
> +	    TP_fast_assign(
> +			   __entry->from = from->context_id;
> +			   __entry->to = to->id;
> +			   ),
> +
> +	    TP_printk("context switch from %d to %d",
> +		      __entry->from, __entry->to)
> +);
>  #endif /* _I915_TRACE_H_ */
>  
>  /* This part must be outside protection */
> -- 
> 1.7.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 04/25] drm/i915: add context information to objects

[Ben Widawsky]

On Thu, 14 Jun 2012 00:01:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jun 04, 2012 at 02:42:44PM -0700, Ben Widawsky wrote:
> > Daniel originally shot this patch down as being a layering violation. In
> > fact, over time I've come to agree with him on this.
> > 
> > However, it is handy for debug information, and after the below commit,
> > I need some way to be able to not lazily bind objects to the gtt for
> > contexts.
> > 
> > commit 0ebb98299357e1dbeeea470eec29241263c8f244
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Feb 15 23:50:24 2012 +0100
> > 
> >     drm/i915: enable lazy global-gtt binding
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> That is imo a really neat layering violation in the following patch. I'll
> shot down both patches. Just call i915_gem_gtt_bind_object right before
> using the object - object_bind_to_gtt only allocates the gtt area.
> -Daniel
> 

As long as we have a solution to the non-lazy binding of context
objects, we can certainly get rid of this.

> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
> >  drivers/gpu/drm/i915/i915_gem.c         |    1 +
> >  drivers/gpu/drm/i915/i915_gem_context.c |    6 +++++-
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f543679..9b309aa 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -993,6 +993,11 @@ struct drm_i915_gem_object {
> >  	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
> >  	 */
> >  	atomic_t pending_flip;
> > +
> > +	/**
> > +	 * >= 0 if this object is the object for a context.
> > +	 */
> > +	int context_id;
> >  };
> >  
> >  #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 28f00dc..2142e9c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3439,6 +3439,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> >  	obj->madv = I915_MADV_WILLNEED;
> >  	/* Avoid an unnecessary call to unbind on the first bind. */
> >  	obj->map_and_fenceable = true;
> > +	obj->context_id = -1;
> >  
> >  	return obj;
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 2aca002..bf57123 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -162,7 +162,7 @@ create_hw_context(struct drm_device *dev,
> >  
> >  	/* Default context will never have a file_priv */
> >  	if (file_priv == NULL)
> > -		return 0;
> > +		goto out;
> >  
> >  	(*ctx_out)->file_priv = file_priv;
> >  
> > @@ -183,6 +183,8 @@ again:
> >  	else if (ret)
> >  		goto err_out;
> >  
> > +out:
> > +	(*ctx_out)->obj->context_id = (*ctx_out)->id;
> >  	return 0;
> >  
> >  err_out:
> > @@ -214,6 +216,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
> >  	 * default context.
> >  	 */
> >  	ctx = dev_priv->ring[RCS].default_context;
> > +	ctx->id = DEFAULT_CONTEXT_ID;
> > +	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
> >  	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> >  	if (ret) {
> >  		do_destroy(ctx);
> > -- 
> > 1.7.10.2
> > 
> > _______________________________________________
> > 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 07/25] drm/i915: context trace events

[Ben Widawsky]

On Thu, 14 Jun 2012 00:23:12 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jun 04, 2012 at 02:42:47PM -0700, Ben Widawsky wrote:
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I'll punt on that one because I've punted on the obj->context_id patch,
> too. Imo we could switch the context_switch tracepoint to object pointers
> for both to and from (and maybe keep to->id for convenience).
> -Daniel

Reading through traces it's really a lot easier to read id numbers over
pointers. However, if you're going to insist on removing from->id, we
may as well remove to->id also.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c |    4 +++
> >  drivers/gpu/drm/i915/i915_trace.h       |   55 +++++++++++++++++++++++++++++++
> >  2 files changed, 59 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index aabe7d9..388cf62 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -130,6 +130,8 @@ static void do_destroy(struct i915_hw_context *ctx)
> >  	else
> >  		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
> >  
> > +	trace_i915_context_destroy(ctx);
> > +
> >  	drm_gem_object_unreference(&ctx->obj->base);
> >  	kfree(ctx);
> >  }
> > @@ -185,6 +187,7 @@ again:
> >  
> >  out:
> >  	(*ctx_out)->obj->context_id = (*ctx_out)->id;
> > +	trace_i915_context_create(*ctx_out);
> >  	return 0;
> >  
> >  err_out:
> > @@ -398,6 +401,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> >  	ring->last_context_obj = to->obj;
> >  	to->is_initialized = true;
> >  
> > +	trace_i915_context_switch(from_obj, to);
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> > index fe90b3a..6d8412c 100644
> > --- a/drivers/gpu/drm/i915/i915_trace.h
> > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > @@ -434,6 +434,61 @@ TRACE_EVENT(i915_reg_rw,
> >  		(u32)(__entry->val >> 32))
> >  );
> >  
> > +/* Context tracking */
> > +TRACE_EVENT(i915_context_create,
> > +	    TP_PROTO(struct i915_hw_context *ctx),
> > +	    TP_ARGS(ctx),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(struct i915_hw_context *, ctx)
> > +			     __field(u32, id)
> > +			     __field(struct drm_i915_gem_object *, obj)
> > +			     ),
> > +
> > +	    TP_fast_assign(
> > +			   __entry->ctx = ctx;
> > +			   __entry->id = ctx->id;
> > +			   __entry->obj = ctx->obj;
> > +			   ),
> > +
> > +	    TP_printk("ctx=%p, id=%u, obj=%p",
> > +		      __entry->ctx, __entry->id, __entry->obj)
> > +);
> > +
> > +TRACE_EVENT(i915_context_destroy,
> > +	    TP_PROTO(struct i915_hw_context *ctx),
> > +	    TP_ARGS(ctx),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(struct i915_hw_context *, ctx)
> > +			     __field(u32, id)
> > +			     ),
> > +
> > +	    TP_fast_assign(
> > +			   __entry->ctx = ctx;
> > +			   __entry->id = ctx->id;
> > +			   ),
> > +
> > +	    TP_printk("ctx=%p, id=%u", __entry->ctx, __entry->id)
> > +);
> > +
> > +TRACE_EVENT(i915_context_switch,
> > +	    TP_PROTO(struct drm_i915_gem_object *from, struct i915_hw_context *to),
> > +	    TP_ARGS(from, to),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(int, from)
> > +			     __field(int, to)
> > +			    ),
> > +
> > +	    TP_fast_assign(
> > +			   __entry->from = from->context_id;
> > +			   __entry->to = to->id;
> > +			   ),
> > +
> > +	    TP_printk("context switch from %d to %d",
> > +		      __entry->from, __entry->to)
> > +);
> >  #endif /* _I915_TRACE_H_ */
> >  
> >  /* This part must be outside protection */
> > -- 
> > 1.7.10.2
> > 
> > _______________________________________________
> > 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 03/25 v2] drm/i915: context basic create & destroy

[Ben Widawsky]

TODO from irc: change create_hw_context to return the pointer and use
PTR_ERR as appropriate.

On Mon,  4 Jun 2012 14:42:43 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Invent an abstraction for a hw context which is passed around through
> the core functions. The main bit a hw context holds is the buffer object
> which backs the context. The rest of the members are just helper
> functions. Specifically the ring member, which could likely go away if
> we decide to never implement whatever other hw context support exists.
> 
> Of note here is the introduction of the 64k alignment constraint for the
> BO. If contexts become heavily used, we should consider tweaking this
> down to 4k. Until the contexts are merged and tested a bit though, I
> think 64k is a nice start (based on docs).
> 
> Since we don't yet switch contexts, there is really not much complexity
> here. Creation/destruction works pretty much as one would expect. An idr
> is used to generate the context id numbers which are unique per file
> descriptor.
> 
> v2: add DRM_DEBUG_DRIVERS to distinguish ENOMEM failures (ben)
> convert a BUG_ON to WARN_ON, default destruction is still fatal (ben)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |   11 +++
>  drivers/gpu/drm/i915/i915_gem_context.c |  142 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +
>  3 files changed, 153 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 432b44f..f543679 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -308,6 +308,16 @@ struct i915_hw_ppgtt {
>  	dma_addr_t scratch_page_dma_addr;
>  };
>  
> +
> +/* This must match up with the value previously used for execbuf2.rsvd1. */
> +#define DEFAULT_CONTEXT_ID 0
> +struct i915_hw_context {
> +	int id;
> +	struct drm_i915_file_private *file_priv;
> +	struct intel_ring_buffer *ring;
> +	struct drm_i915_gem_object *obj;
> +};
> +
>  enum no_fbc_reason {
>  	FBC_NO_OUTPUT, /* no outputs enabled to compress */
>  	FBC_STOLEN_TOO_SMALL, /* not enough space to hold compressed buffers */
> @@ -1023,6 +1033,7 @@ struct drm_i915_file_private {
>  		struct spinlock lock;
>  		struct list_head request_list;
>  	} mm;
> +	struct idr context_idr;
>  };
>  
>  #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e39808e..2aca002 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -89,6 +89,15 @@
>  #include "i915_drm.h"
>  #include "i915_drv.h"
>  
> +/* This is a HW constraint. The value below is the largest known requirement
> + * I've seen in a spec to date, and that was a workaround for a non-shipping
> + * part. It should be safe to decrease this, but it's more future proof as is.
> + */
> +#define CONTEXT_ALIGN (64<<10)
> +
> +static struct i915_hw_context *
> +i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> +
>  static int get_context_size(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -111,6 +120,76 @@ static int get_context_size(struct drm_device *dev)
>  	return ret;
>  }
>  
> +static void do_destroy(struct i915_hw_context *ctx)
> +{
> +	struct drm_device *dev = ctx->obj->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (ctx->file_priv)
> +		idr_remove(&ctx->file_priv->context_idr, ctx->id);
> +	else
> +		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
> +
> +	drm_gem_object_unreference(&ctx->obj->base);
> +	kfree(ctx);
> +}
> +
> +static int
> +create_hw_context(struct drm_device *dev,
> +		  struct drm_i915_file_private *file_priv,
> +		  struct i915_hw_context **ctx_out)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret, id;
> +
> +	*ctx_out = kzalloc(sizeof(struct drm_i915_file_private), GFP_KERNEL);
> +	if (*ctx_out == NULL)
> +		return -ENOMEM;
> +
> +	(*ctx_out)->obj = i915_gem_alloc_object(dev,
> +						dev_priv->hw_context_size);
> +	if ((*ctx_out)->obj == NULL) {
> +		kfree(*ctx_out);
> +		DRM_DEBUG_DRIVER("Context object allocated failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* The ring associated with the context object is handled by the normal
> +	 * object tracking code. We give an initial ring value simple to pass an
> +	 * assertion in the context switch code.
> +	 */
> +	(*ctx_out)->ring = &dev_priv->ring[RCS];
> +
> +	/* Default context will never have a file_priv */
> +	if (file_priv == NULL)
> +		return 0;
> +
> +	(*ctx_out)->file_priv = file_priv;
> +
> +again:
> +	if (idr_pre_get(&file_priv->context_idr, GFP_KERNEL) == 0) {
> +		ret = -ENOMEM;
> +		DRM_DEBUG_DRIVER("idr allocation failed\n");
> +		goto err_out;
> +	}
> +
> +	ret = idr_get_new_above(&file_priv->context_idr, *ctx_out,
> +				DEFAULT_CONTEXT_ID + 1, &id);
> +	if (ret == 0)
> +		(*ctx_out)->id = id;
> +
> +	if (ret == -EAGAIN)
> +		goto again;
> +	else if (ret)
> +		goto err_out;
> +
> +	return 0;
> +
> +err_out:
> +	do_destroy(*ctx_out);
> +	return ret;
> +}
> +
>  /**
>   * The default context needs to exist per ring that uses contexts. It stores the
>   * context state of the GPU for applications that don't utilize HW contexts, as
> @@ -118,7 +197,30 @@ static int get_context_size(struct drm_device *dev)
>   */
>  static int create_default_context(struct drm_i915_private *dev_priv)
>  {
> -	return 0;
> +	struct i915_hw_context *ctx;
> +	int ret;
> +
> +	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> +
> +	ret = create_hw_context(dev_priv->dev, NULL,
> +				&dev_priv->ring[RCS].default_context);
> +	if (ret)
> +		return ret;
> +
> +	/* We may need to do things with the shrinker which require us to
> +	 * immediately switch back to the default context. This can cause a
> +	 * problem as pinning the default context also requires GTT space which
> +	 * may not be available. To avoid this we always pin the
> +	 * default context.
> +	 */
> +	ctx = dev_priv->ring[RCS].default_context;
> +	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> +	if (ret) {
> +		do_destroy(ctx);
> +		return ret;
> +	}
> +
> +	return ret;
>  }
>  
>  void i915_gem_context_init(struct drm_device *dev)
> @@ -130,7 +232,8 @@ void i915_gem_context_init(struct drm_device *dev)
>  		return;
>  
>  	/* If called from reset, or thaw... we've been here already */
> -	if (dev_priv->hw_contexts_disabled)
> +	if (dev_priv->hw_contexts_disabled ||
> +	    dev_priv->ring[RCS].default_context)
>  		return;
>  
>  	ctx_size = get_context_size(dev);
> @@ -156,20 +259,55 @@ void i915_gem_context_fini(struct drm_device *dev)
>  
>  	if (dev_priv->hw_contexts_disabled)
>  		return;
> +
> +	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
> +
> +	do_destroy(dev_priv->ring[RCS].default_context);
>  }
>  
>  void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
>  
>  	if (dev_priv->hw_contexts_disabled)
>  		return;
> +
> +	idr_init(&file_priv->context_idr);
> +}
> +
> +static int context_idr_cleanup(int id, void *p, void *data)
> +{
> +	struct drm_file *file = (struct drm_file *)data;
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	struct i915_hw_context *ctx;
> +
> +	BUG_ON(id == DEFAULT_CONTEXT_ID);
> +	ctx = i915_gem_context_get(file_priv, id);
> +	if (WARN_ON(ctx == NULL))
> +		return -ENXIO;
> +
> +	do_destroy(ctx);
> +
> +	return 0;
>  }
>  
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
>  
>  	if (dev_priv->hw_contexts_disabled)
>  		return;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
> +	idr_destroy(&file_priv->context_idr);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
> +static __used struct i915_hw_context *
> +i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> +{
> +	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 55d3da2..bb19bec 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -116,6 +116,8 @@ struct  intel_ring_buffer {
>  
>  	wait_queue_head_t irq_queue;
>  
> +	struct i915_hw_context *default_context;
> +
>  	void *private;
>  };
>  



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 24/25] contexts: basic test coverage

[Daniel Vetter]

On Mon, Jun 04, 2012 at 02:43:04PM -0700, Ben Widawsky wrote:
> I'm not sure we want to keep some of these. Mostly here to show what
> testing I have done already.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

git add helps ... ;-)
-Daniel
> ---
>  tests/Makefile.am |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 9ec07c6..bc33ab0 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -61,6 +61,11 @@ TESTS_progs = \
>  	drm_vma_limiter_gtt \
>  	drm_vma_limiter_cached \
>  	sysfs_rc6_residency \
> +	gem_ctx_basic \
> +	gem_ctx_exec \
> +	gem_ctx_bad_destroy \
> +	gem_ctx_create \
> +	gem_ctx_bad_exec \
>  	$(NULL)
>  
>  # IMPORTANT: The ZZ_ tests need to be run last!
> @@ -113,3 +118,6 @@ AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS)
>  
>  gem_fence_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>  gem_fence_thrash_LDADD = $(LDADD) -lpthread
> +
> +gem_ctx_basic_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> +gem_ctx_basic_LDADD = $(LDADD) -lpthread
> -- 
> 1.7.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH] contexts: basic test coverage

[Ben Widawsky]

v2: with the files this time.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/Makefile.am           |    8 +++
 tests/gem_ctx_bad_destroy.c |   85 +++++++++++++++++++++++
 tests/gem_ctx_bad_exec.c    |  103 ++++++++++++++++++++++++++++
 tests/gem_ctx_basic.c       |  158 +++++++++++++++++++++++++++++++++++++++++++
 tests/gem_ctx_create.c      |   53 +++++++++++++++
 tests/gem_ctx_exec.c        |  105 ++++++++++++++++++++++++++++
 6 files changed, 512 insertions(+)
 create mode 100644 tests/gem_ctx_bad_destroy.c
 create mode 100644 tests/gem_ctx_bad_exec.c
 create mode 100644 tests/gem_ctx_basic.c
 create mode 100644 tests/gem_ctx_create.c
 create mode 100644 tests/gem_ctx_exec.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9ec07c6..bc33ab0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -61,6 +61,11 @@ TESTS_progs = \
 	drm_vma_limiter_gtt \
 	drm_vma_limiter_cached \
 	sysfs_rc6_residency \
+	gem_ctx_basic \
+	gem_ctx_exec \
+	gem_ctx_bad_destroy \
+	gem_ctx_create \
+	gem_ctx_bad_exec \
 	$(NULL)
 
 # IMPORTANT: The ZZ_ tests need to be run last!
@@ -113,3 +118,6 @@ AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS)
 
 gem_fence_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 gem_fence_thrash_LDADD = $(LDADD) -lpthread
+
+gem_ctx_basic_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
+gem_ctx_basic_LDADD = $(LDADD) -lpthread
diff --git a/tests/gem_ctx_bad_destroy.c b/tests/gem_ctx_bad_destroy.c
new file mode 100644
index 0000000..07407fc
--- /dev/null
+++ b/tests/gem_ctx_bad_destroy.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+/*
+ * Negative test cases for destroy contexts
+  */
+
+#include <stdio.h>
+#include <string.h>
+#include "i915_drm.h"
+#include "drmtest.h"
+
+static void handle_bad(int ret, int lerrno, int expected)
+{
+	if (ret != 0 && lerrno != expected) {
+		fprintf(stderr, "errno was %d, but should have been %d\n",
+				lerrno, expected);
+		exit(EXIT_FAILURE);
+	} else if (ret == 0) {
+		fprintf(stderr, "Command succeeded, but should have failed\n");
+		exit(EXIT_FAILURE);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct drm_i915_gem_context_create create;
+	struct drm_i915_gem_context_destroy destroy;
+	int ret, fd;
+
+	fd = drm_open_any();
+
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create);
+	if (ret != 0) {
+		fprintf(stderr, "%s\n", strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	destroy.ctx_id = create.ctx_id;
+	/* Make sure a proper destroy works first */
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, &destroy);
+	assert(ret == 0);
+
+	/* try double destroy */
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, &destroy);
+	handle_bad(ret, errno, EINVAL);
+
+	/* destroy something random */
+	destroy.ctx_id = 2;
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, &destroy);
+	handle_bad(ret, errno, EINVAL);
+
+	/* Try to destroy the default context */
+	destroy.ctx_id = 0;
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, &create);
+	handle_bad(ret, errno, EINVAL);
+
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c
new file mode 100644
index 0000000..6e050c0
--- /dev/null
+++ b/tests/gem_ctx_bad_exec.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+/*
+ * Negative test cases:
+ *  test we can't submit contexts to unsupported rings
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+
+/* Copied from gem_exec_nop.c */
+static int exec(int fd, uint32_t handle, int ring, int ctx_id)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 gem_exec;
+	int ret = 0;
+
+	gem_exec.handle = handle;
+	gem_exec.relocation_count = 0;
+	gem_exec.relocs_ptr = 0;
+	gem_exec.alignment = 0;
+	gem_exec.offset = 0;
+	gem_exec.flags = 0;
+	gem_exec.rsvd1 = 0;
+	gem_exec.rsvd2 = 0;
+
+	execbuf.buffers_ptr = (uintptr_t)&gem_exec;
+	execbuf.buffer_count = 1;
+	execbuf.batch_start_offset = 0;
+	execbuf.batch_len = 8;
+	execbuf.cliprects_ptr = 0;
+	execbuf.num_cliprects = 0;
+	execbuf.DR1 = 0;
+	execbuf.DR4 = 0;
+	execbuf.flags = ring;
+	i915_execbuffer2_set_context_id(execbuf, ctx_id);
+	execbuf.rsvd2 = 0;
+
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
+			&execbuf);
+	gem_sync(fd, handle);
+
+	return ret;
+}
+
+#define MI_BATCH_BUFFER_END	(0xA<<23)
+int main(int argc, char *argv[])
+{
+	uint32_t handle;
+	uint32_t batch[2] = {MI_BATCH_BUFFER_END};
+	uint32_t ctx_id;
+	int fd;
+	fd = drm_open_any();
+
+	ctx_id = gem_context_create(fd);
+
+	handle = gem_create(fd, 4096);
+	gem_write(fd, handle, 0, batch, sizeof(batch));
+	assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id) == 0);
+	assert(exec(fd, handle, I915_EXEC_BSD, ctx_id) != 0);
+	assert(exec(fd, handle, I915_EXEC_BLT, ctx_id) != 0);
+
+	exit(EXIT_SUCCESS);
+}
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
new file mode 100644
index 0000000..8492c66
--- /dev/null
+++ b/tests/gem_ctx_basic.c
@@ -0,0 +1,158 @@
+/*
+ * Copyright © 2011 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+/*
+ * This test is useful for finding memory and refcount leaks.
+ */
+
+#include <pthread.h>
+#include "rendercopy.h"
+
+/* options */
+int num_contexts = 10;
+int uncontexted = 0; /* test only context create/destroy */
+int multiple_fds = 1;
+int iter = 10000;
+
+/* globals */
+pthread_t *threads;
+int devid;
+int fd;
+int use_rendercpy = 1;
+
+static void init_buffer(drm_intel_bufmgr *bufmgr,
+			struct scratch_buf *buf,
+			uint32_t size)
+{
+	buf->bo = drm_intel_bo_alloc(bufmgr, "", size, 4096);
+	buf->size = size;
+	assert(buf->bo);
+	buf->tiling = I915_TILING_NONE;
+	buf->stride = 4096;
+}
+
+static void *work(void *arg)
+{
+	struct intel_batchbuffer *batch;
+	drm_intel_context *context;
+	drm_intel_bufmgr *bufmgr;
+	int td_fd;
+	int i;
+
+	if (multiple_fds)
+		td_fd = fd = drm_open_any();
+	else
+		td_fd = fd;
+
+	assert(td_fd >= 0);
+
+	bufmgr = drm_intel_bufmgr_gem_init(td_fd, 4096);
+	batch = intel_batchbuffer_alloc(bufmgr, devid);
+	context = drm_intel_gem_context_create(bufmgr);
+
+	for (i = 0; i < iter; i++) {
+		struct scratch_buf src, dst;
+		uint32_t batch_len;
+
+		init_buffer(bufmgr, &src, 4096);
+		init_buffer(bufmgr, &dst, 4096);
+
+
+		if (uncontexted) {
+			gen6_render_copyfunc(batch, &src, 0, 0, 0, 0, &dst, 0, 0);
+		} else {
+			int ret;
+			ret = drm_intel_bo_subdata(batch->bo, 0, 4096, batch->buffer);
+			assert(ret == 0);
+			intel_batchbuffer_flush_with_context(batch, context);
+		}
+	}
+
+	drm_intel_gem_context_destroy(context);
+	intel_batchbuffer_free(batch);
+	drm_intel_bufmgr_destroy(bufmgr);
+
+	if (multiple_fds)
+		close(td_fd);
+
+	pthread_exit(NULL);
+}
+
+static void parse(int argc, char *argv[])
+{
+	int opt;
+	while ((opt = getopt(argc, argv, "i:c:n:muh?")) != -1) {
+		switch (opt) {
+		case 'i':
+			iter = atoi(optarg);
+			break;
+		case 'c':
+			num_contexts = atoi(optarg);
+			break;
+		case 'm':
+			multiple_fds = 1;
+			break;
+		case 'u':
+			uncontexted = 1;
+			break;
+		case 'h':
+		case '?':
+		default:
+			exit(EXIT_SUCCESS);
+			break;
+		}
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	void *ret;
+	int i;
+
+	fd = drm_open_any();
+	devid = intel_get_drm_devid(fd);
+
+	if (!IS_GEN6(devid))
+		use_rendercpy = 0;
+
+	parse(argc, argv);
+
+	threads = calloc(num_contexts, sizeof(*threads));
+
+	for (i = 0; i < num_contexts; i++)
+		pthread_create(&threads[i], NULL, work, NULL);
+
+	for (i = 0; i < num_contexts; i++) {
+		pthread_join(threads[i], &ret);
+		free(ret);
+	}
+
+	free(threads);
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
new file mode 100644
index 0000000..9d0af0a
--- /dev/null
+++ b/tests/gem_ctx_create.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "i915_drm.h"
+#include "drmtest.h"
+
+int main(int argc, char *argv[])
+{
+	int ret, fd;
+	struct drm_i915_gem_context_create create;
+
+	create.ctx_id = rand();
+	create.pad = rand();
+
+	fd = drm_open_any();
+
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create);
+	if (ret != 0) {
+		fprintf(stderr, "%s\n", strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+	assert(create.ctx_id != 0);
+
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c
new file mode 100644
index 0000000..559d659
--- /dev/null
+++ b/tests/gem_ctx_exec.c
@@ -0,0 +1,105 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+/*
+ * This test covers basic context switch functionality
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+
+/* Copied from gem_exec_nop.c */
+static int exec(int fd, uint32_t handle, int ring, int ctx_id)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 gem_exec;
+	int ret = 0;
+
+	gem_exec.handle = handle;
+	gem_exec.relocation_count = 0;
+	gem_exec.relocs_ptr = 0;
+	gem_exec.alignment = 0;
+	gem_exec.offset = 0;
+	gem_exec.flags = 0;
+	gem_exec.rsvd1 = 0;
+	gem_exec.rsvd2 = 0;
+
+	execbuf.buffers_ptr = (uintptr_t)&gem_exec;
+	execbuf.buffer_count = 1;
+	execbuf.batch_start_offset = 0;
+	execbuf.batch_len = 8;
+	execbuf.cliprects_ptr = 0;
+	execbuf.num_cliprects = 0;
+	execbuf.DR1 = 0;
+	execbuf.DR4 = 0;
+	execbuf.flags = ring;
+	i915_execbuffer2_set_context_id(execbuf, ctx_id);
+	execbuf.rsvd2 = 0;
+
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
+			&execbuf);
+	gem_sync(fd, handle);
+
+	return ret;
+}
+
+#define MI_BATCH_BUFFER_END	(0xA<<23)
+int main(int argc, char *argv[])
+{
+	uint32_t handle;
+	uint32_t batch[2] = {0, MI_BATCH_BUFFER_END};
+	uint32_t ctx_id;
+	int fd;
+	fd = drm_open_any();
+
+	ctx_id = gem_context_create(fd);
+	handle = gem_create(fd, 4096);
+
+	gem_write(fd, handle, 0, batch, sizeof(batch));
+	assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id) == 0);
+	gem_context_destroy(fd, ctx_id);
+
+	ctx_id = gem_context_create(fd);
+	assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id) == 0);
+	gem_context_destroy(fd, ctx_id);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.10.4

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

Re: [PATCH 00/25] i915 HW context support

[Daniel Vetter]

On Mon, Jun 04, 2012 at 02:42:40PM -0700, Ben Widawsky wrote:
> Setting myself up for a late night crying session once again. Most of the
> people reading this probably know the history and reasons for the patches. If
> not, you can search the intel-gfx mailing list to try to learn more. I won't
> recap the whole thing here, and instead let the patches speak for themselves.
> 
> Instead a brief review of what's here, and what's happened. Mostly,
> these badly need testing and review. I've carried these around for so
> long now, and seen so many different failures, I'm quite paranoid they
> still aren't perfect. Also, I've spent almost all of the time working on
> this in the kernel, so there is bound to be simple errors in the other
> stuff.
> 
> I've run these on various workloads and saw nothing worth mentioning.

Ok, my little attempt at in-situ bikeshedding failed, so I've just slurped
in your patches directly and only applied the minimal change to get rid of
object->context_id.

Please commit the i-g-t testcase so that qa can start testing this aspa.

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