[RFC PATCH 00/11] i915 HW context support

[RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

These patches are a heavily revised version of the patches I wrote over
a year ago. These patches have passed basic tests on SNB, and IVB, and
older versions worked on ILK.  In theory, context support should work
all the way back to Gen4, but I haven't tested it. Also since I suspect
ILK may be unstable, so the code has it disabled for now.

HW contexts provide a way for the GPU to save an restore certain state
in between batchbuffer boundaries. Typically, GPU clients must re-emit
the entire state every time they run because the client does not know
what has been destroyed since the last time. With these patches the
driver will emit special instructions to do this on behalf of the client
if it has registered a context, and included that with the batchbuffer.

[... From Ken Graunke ]
This is needed to properly implement Transform Feedback in the presence
of Geometry Shaders, since software wouldn't be able to track the SVBI0
register to restore it to its previous state.

The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
interface may be insufficient for that. The contexts do provide a

I've tested this with an experiemental mesa from Ken Graunke, as well as
some basic i-g-t tests.

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

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

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

I'm getting really tired of looking at these, please help me polish them
up and make them more ready for consumption. I did consider moving the
power context allocation to this set of APIs, but it didn't seem worth
it to me. I can go either way.

Ben Widawsky (11):
  drm/i915: MI_ARB_ON_OFF
  drm/i915: track tlb invalidate GFX_MODE state
  drm/i915: PIPE_CONTROL_TLB_INVALIDATE
  drm/i915/context: CXT_SIZE register offsets added
  drm/i915/context: Preliminary context support
  drm/i915/context: ringbuffer context switch code
  drm/i915/context: implementation details
  drm/i915/context: extend contexts to execbuffer2
  drm/i915/context: add params
  drm/i915/context: track contexts per device
  drm/i915: show contexts in debugfs

 drivers/gpu/drm/i915/Makefile              |    1 +
 drivers/gpu/drm/i915/i915_context.c        |  562 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c        |    6 +
 drivers/gpu/drm/i915/i915_dma.c            |   10 +
 drivers/gpu/drm/i915/i915_drv.h            |   47 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   26 ++-
 drivers/gpu/drm/i915/i915_reg.h            |   17 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  122 ++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +-
 include/drm/i915_drm.h                     |   18 +
 10 files changed, 815 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_context.c

-- 
1.7.9

[PATCH 01/11] drm/i915: MI_ARB_ON_OFF

[Ben Widawsky]

Add instruction definition for upcoming workarounds.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5c62b78..2db6f2f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -197,6 +197,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.9

[PATCH 02/11] drm/i915: track tlb invalidate GFX_MODE state

[Ben Widawsky]

This is needed for an upcoming workaround.

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4956f1b..411a0e5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -412,6 +412,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 			return ret;
 	}
 
+	if (INTEL_INFO(dev)->gen == 6)
+		ring->itlb_before_ctx_switch =
+			!!(I915_READ(GFX_MODE) & GFX_TLB_INVALIDATE_ALWAYS);
+
 	if (INTEL_INFO(dev)->gen >= 6) {
 		I915_WRITE(INSTPM,
 			   INSTPM_FORCE_ORDERING << 16 | INSTPM_FORCE_ORDERING);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c8b9cc0..fad4251 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -110,6 +110,11 @@ struct  intel_ring_buffer {
 	wait_queue_head_t irq_queue;
 	drm_local_map_t map;
 
+	/**
+	 * Do an explicit TLB flush before MI_SET_CONTEXT
+	 */
+	bool itlb_before_ctx_switch;
+
 	void *private;
 };
 
-- 
1.7.9

[PATCH 03/11] drm/i915: PIPE_CONTROL_TLB_INVALIDATE

[Ben Widawsky]

This has showed up in several other patches. It's going to be required
for context workarounds.

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(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2db6f2f..0c785af 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -280,6 +280,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 411a0e5..e71e7fc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -223,6 +223,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.9

[PATCH 04/11] drm/i915/context: 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 |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0c785af..5e61e79 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1361,6 +1361,19 @@
  */
 #define CCID			0x2180
 #define   CCID_EN		(1<<0)
+#define CXT_SIZE		0x21a0
+#define GEN6_CXT_RENDER_SIZE ((I915_READ(CXT_SIZE) >> 12) & 0x3f)
+#define GEN6_CXT_EXTENDED_SIZE ((I915_READ(CXT_SIZE) >> 6) & 0x3f)
+#define GEN6_CXT_PIPELINE_SIZE ((I915_READ(CXT_SIZE) >> 0) & 0x3f)
+#define GEN6_CXT_TOTAL_SIZE (GEN6_CXT_RENDER_SIZE + GEN6_CXT_EXTENDED_SIZE + GEN6_CXT_PIPELINE_SIZE)
+#define GEN7_CTX_SIZE		0x21a8
+#define GEN7_CTX_RENDER_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 16) & 0x3f)
+#define GEN7_CTX_EXTENDED_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 9) & 0x7f)
+#define GEN7_CTX_GT1_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 6) & 0x7)
+#define GEN7_CTX_VFSTATE_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 0) & 0x3f)
+#define GEN7_CTX_TOTAL_SIZE (GEN7_CTX_RENDER_SIZE + GEN7_CTX_EXTENDED_SIZE \
+			    + GEN7_CTX_GT1_SIZE + GEN7_CTX_VFSTATE_SIZE)
+
 /*
  * Overlay regs
  */
-- 
1.7.9

[PATCH 05/11] drm/i915/context: Preliminary context support

[Ben Widawsky]

From: Ben Widawsky <bwidawsk@gmail.com>

Create all the necessary data structures and hooks to support a context
API for userspace. Also plumb in the calls from the i915 core into the
context subsystem. There is a new file i915_context.c which will hold a
majority of the context related code. This file has nulled functions
that get called by the proper part of the i915 driver. This of course
requires a Makefile addition, as well as calls to the new nulled
functions from the other partsd of the driver (i915_dma.c).

With that, there are 2 new ioctls defined , one for creating a context,
and one for destroying a context. These are defined in the necessary
places, and again have null functionality

It's likely that a third ioctl will be required to associate the context
with an execbuf (likely execbuf3). Since that part is mostly trivial,
and will surely be contentious, it's ignored for now.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/Makefile       |    1 +
 drivers/gpu/drm/i915/i915_context.c |   71 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_dma.c     |    8 ++++
 drivers/gpu/drm/i915/i915_drv.h     |   30 +++++++++++++++
 include/drm/i915_drm.h              |   17 ++++++++
 5 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_context.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ce7fc77..a633990 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -6,6 +6,7 @@ ccflags-y := -Iinclude/drm
 i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  i915_debugfs.o \
           i915_suspend.o \
+	  i915_context.o \
 	  i915_gem.o \
 	  i915_gem_debug.o \
 	  i915_gem_evict.o \
diff --git a/drivers/gpu/drm/i915/i915_context.c b/drivers/gpu/drm/i915/i915_context.c
new file mode 100644
index 0000000..9108244
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_context.c
@@ -0,0 +1,71 @@
+/*
+ * 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>
+ */
+
+#include "drmP.h"
+#include "i915_drm.h"
+
+int i915_context_create_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file)
+{
+	struct drm_i915_gem_context_create *args = data;
+	args->ctx_id = 0;
+	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
+	return -EIO;
+}
+
+int i915_context_destroy_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_gem_context_destroy *args = data;
+	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
+	return -EINVAL;
+}
+
+void i915_context_load(struct drm_device *dev)
+{
+
+}
+
+void i915_context_unload(struct drm_device *dev)
+{
+
+}
+
+void i915_context_open(struct drm_device *dev, struct drm_file *file)
+{
+
+}
+
+void i915_context_close(struct drm_device *dev, struct drm_file *file)
+{
+
+}
+
+struct drm_i915_gem_context *i915_get_context(struct drm_file *file,
+					      uint32_t id)
+{
+	return NULL;
+}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 38dfcf9..43acac3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2109,6 +2109,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ips_ping_for_i915_load();
 
+	i915_context_load(dev);
+
 	return 0;
 
 out_gem_unload:
@@ -2142,6 +2144,8 @@ int i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	i915_context_unload(dev);
+
 	spin_lock(&mchdev_lock);
 	i915_mch_dev = NULL;
 	spin_unlock(&mchdev_lock);
@@ -2242,6 +2246,7 @@ 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_context_open(dev, file);
 	return 0;
 }
 
@@ -2274,6 +2279,7 @@ void i915_driver_lastclose(struct drm_device * dev)
 
 void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv)
 {
+	i915_context_close(dev, file_priv);
 	i915_gem_release(dev, file_priv);
 }
 
@@ -2327,6 +2333,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	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_CONTEXT_CREATE, i915_context_create_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_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 000a9ad..34e6f4f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -949,6 +949,20 @@ struct drm_i915_file_private {
 		struct spinlock lock;
 		struct list_head request_list;
 	} mm;
+
+	struct spinlock context_idr_lock;
+	struct idr context_idr;
+};
+
+struct drm_i915_gem_context {
+	struct drm_device *dev;
+	struct drm_file *file;
+
+	uint32_t id;
+	uint8_t ring_enable;
+	struct drm_i915_gem_object *obj;
+	bool is_default;
+	bool is_initialized;
 };
 
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
@@ -1022,6 +1036,9 @@ struct drm_i915_file_private {
 #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
 
+/* Contexts are supported on Gen4+, but untested */
+#define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
+
 #include "i915_trace.h"
 
 extern struct drm_ioctl_desc i915_ioctls[];
@@ -1168,6 +1185,19 @@ int i915_gem_mmap_gtt(struct drm_file *file_priv, struct drm_device *dev,
 		      uint32_t handle, uint64_t *offset);
 int i915_gem_dumb_destroy(struct drm_file *file_priv, struct drm_device *dev,
 			  uint32_t handle);
+
+/* i915_context.c */
+int i915_context_create_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file);
+int i915_context_destroy_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+extern void i915_context_load(struct drm_device *dev);
+extern void i915_context_unload(struct drm_device *dev);
+extern void i915_context_open(struct drm_device *dev, struct drm_file *file);
+extern void i915_context_close(struct drm_device *dev, struct drm_file *file);
+extern struct drm_i915_gem_context *i915_get_context(struct drm_file *file,
+						     uint32_t id);
+
 /**
  * Returns true if seq1 is later than seq2.
  */
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index da929bb..8fa509e 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -200,6 +200,8 @@ 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_CONTEXT_CREATE	0x2c
+#define DRM_I915_GEM_CONTEXT_DESTROY	0x2d
 
 #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)
@@ -243,6 +245,9 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
 #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.
@@ -885,4 +890,16 @@ struct drm_intel_sprite_colorkey {
 	__u32 flags;
 };
 
+struct drm_i915_gem_context_create {
+	/* output: id of new context*/
+	__u32 ctx_id;
+
+	__u16 pad;
+};
+
+struct drm_i915_gem_context_destroy {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.9

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

[PATCH 06/11] drm/i915/context: ringbuffer context switch code

[Ben Widawsky]

This is the HW dependent context switch code.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 34e6f4f..4175929 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -965,6 +965,9 @@ struct drm_i915_gem_context {
 	bool is_initialized;
 };
 
+#define I915_CONTEXT_NORMAL_SWITCH	(1 << 0)
+#define I915_CONTEXT_SAVE_ONLY		(1 << 1)
+#define I915_CONTEXT_FORCED_SWITCH	(1 << 2)
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
 
 #define IS_I830(dev)		((dev)->pci_device == 0x3577)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e71e7fc..dcdc80e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -942,6 +942,122 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 	return 0;
 }
 
+static int do_ring_switch(struct intel_ring_buffer *ring,
+			  struct drm_i915_gem_context *new_context,
+			  u32 hw_flags)
+{
+	struct drm_device *dev = ring->dev;
+	int ret = 0;
+
+	if (!new_context->is_initialized) {
+		ret = ring->flush(ring, 0, 0);
+		if (ret)
+			return ret;
+
+		ret = intel_ring_begin(ring, 2);
+		if (ret)
+			return ret;
+
+		intel_ring_emit(ring, MI_NOOP | (1 << 22) | new_context->id);
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_advance(ring);
+	}
+
+	if (IS_GEN6(dev) && new_context->is_initialized &&
+	    ring->itlb_before_ctx_switch) {
+		/* w/a: If Flush TLB Invalidation Mode is enabled, driver must
+		 * do a TLB invalidation prior to MI_SET_CONTEXT
+		 */
+		gen6_render_ring_flush(ring, 0, 0);
+	}
+
+	ret = intel_ring_begin(ring, 6);
+	if (ret)
+		return ret;
+
+	intel_ring_emit(ring, MI_NOOP);
+
+	switch (INTEL_INFO(dev)->gen) {
+	case 5:
+		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
+		break;
+	case 6:
+		intel_ring_emit(ring, MI_NOOP);
+		break;
+	case 7:
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+		break;
+	case 4:
+	default:
+		BUG();
+	}
+
+	DRM_DEBUG_DRIVER("Context switch %d-%d -> %d-%d\n",
+			 (ring->last_context && ring->last_context->file) ?
+				ring->last_context->file->pid : 0,
+			 ring->last_context ? ring->last_context->id : -1,
+			 new_context->file ? new_context->file->pid : 0,
+			 new_context->id);
+
+
+	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);
+
+	if (IS_GEN5(dev))
+		intel_ring_emit(ring, MI_SUSPEND_FLUSH);
+	else if (IS_GEN7(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;
+
+}
+
+static struct drm_i915_gem_context *
+render_ring_context_switch(struct intel_ring_buffer *ring,
+			   struct drm_i915_gem_context *new_context,
+			   u32 flags)
+{
+	struct drm_device *dev = ring->dev;
+	bool force = (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
+	struct drm_i915_gem_context *last = NULL;
+	uint32_t hw_flags = 0;
+
+	/* last_context is only protected by struct_mutex */
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	BUG_ON(new_context->obj == NULL || new_context->obj->gtt_offset == 0);
+
+	if (!force && (ring->last_context == new_context))
+		return new_context;
+
+	if (flags & I915_CONTEXT_SAVE_ONLY)
+		hw_flags = MI_RESTORE_INHIBIT;
+
+	if (do_ring_switch(ring, new_context, hw_flags))
+		return NULL;
+
+	last = ring->last_context;
+	ring->last_context = new_context;
+
+	/* The first context switch with default context is special */
+	if (last == NULL && new_context->is_default)
+		return new_context;
+
+	return last;
+}
+
 static void cleanup_status_page(struct intel_ring_buffer *ring)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
@@ -1232,6 +1348,7 @@ static const struct intel_ring_buffer render_ring = {
 				   MI_SEMAPHORE_SYNC_RV,
 				   MI_SEMAPHORE_SYNC_RB},
 	.signal_mbox		= {GEN6_VRSYNC, GEN6_BRSYNC},
+	.context_switch		= render_ring_context_switch,
 };
 
 /* ring buffer for bit-stream decoder */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index fad4251..1988728 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -69,10 +69,14 @@ struct  intel_ring_buffer {
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
 					       u32 offset, u32 length);
 	void		(*cleanup)(struct intel_ring_buffer *ring);
+	struct		drm_i915_gem_context *last_context;
+	struct		drm_i915_gem_context *(*context_switch) (
+					     struct intel_ring_buffer *ring,
+					     struct drm_i915_gem_context *ctx,
+					     u32 flags);
 	int		(*sync_to)(struct intel_ring_buffer *ring,
 				   struct intel_ring_buffer *to,
 				   u32 seqno);
-
 	u32		semaphore_register[3]; /*our mbox written by others */
 	u32		signal_mbox[2]; /* mboxes this ring signals to */
 	/**
-- 
1.7.9

[PATCH 07/11] drm/i915/context: implementation details

[Ben Widawsky]

From: Ben Widawsky <bwidawsk@gmail.com>

While loading the context submodule, which is called from the main
i915 driver, we must create and initialize the default context for the
device. The default context is used for clients which do not set up
contexts, and as a known good state for the hardware. Doing this at
initialization allows later clients to determine whether or not context
support exist before actually trying to create or run them (if there was
an error at load, contexts will be disabled).

Unloading the context is much easier. The ref counts, and pin counts are
taken care of by the rest of the driver, so all this code needs to do is
free the kernel memory associated with the default context.

We also implement the previously introduced ioctls.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_context.c |  495 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h     |    9 +
 include/drm/i915_drm.h              |    4 +-
 3 files changed, 502 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_context.c b/drivers/gpu/drm/i915/i915_context.c
index 9108244..0547411 100644
--- a/drivers/gpu/drm/i915/i915_context.c
+++ b/drivers/gpu/drm/i915/i915_context.c
@@ -26,46 +26,533 @@
 
 #include "drmP.h"
 #include "i915_drm.h"
+#include "i915_drv.h"
 
+/* This ID must be 0 if reusing fields from execbuf2. */
+#define DEFAULT_CONTEXT_ID 0
+
+/* HW requirement */
+#define CONTEXT_ALIGN 2048
+
+static int context_idr_cleanup(int id, void *p, void *data);
+
+static int context_generate_id(struct drm_i915_gem_context *ctx)
+{
+	struct drm_i915_file_private *file_priv = ctx->file->driver_priv;
+	int ret, id;
+
+	if (WARN_ON(!mutex_is_locked(&ctx->dev->struct_mutex)))
+		return -ENOENT;
+
+again:
+	if (idr_pre_get(&file_priv->context_idr, GFP_KERNEL) == 0)
+		return -ENOMEM;
+
+	spin_lock(&file_priv->context_idr_lock);
+	ret = idr_get_new_above(&file_priv->context_idr, ctx,
+				DEFAULT_CONTEXT_ID + 1, &id);
+	if (ret == -EAGAIN) {
+		spin_unlock(&file_priv->context_idr_lock);
+		goto again;
+	}
+	spin_unlock(&file_priv->context_idr_lock);
+
+	return id;
+}
+
+static void context_destroy_id(struct drm_i915_gem_context *ctx)
+{
+	struct drm_i915_file_private *file_priv = ctx->file->driver_priv;
+
+	spin_lock(&file_priv->context_idr_lock);
+	idr_remove(&file_priv->context_idr, ctx->id);
+	spin_unlock(&file_priv->context_idr_lock);
+}
+
+/*
+ * Initialize a context for the given ring.
+ */
+static int context_init(struct drm_i915_gem_context *ctx,
+			struct intel_ring_buffer *ring)
+{
+	struct drm_i915_gem_context *last = NULL;
+	int ret;
+
+	if (ring->context_switch == NULL)
+		return -ENOENT;
+
+	if (ctx->is_default) {
+		/*
+		 * NB: default context is always first. The first context needs
+		 * to do an extra save because the first save (according to the
+		 * spec) doesn't actually do anything. So the outcome is
+		 * 1. Save without restore (no context saved)
+		 * 2. Save without restore (context is saved)
+		 * 3. Save with restore (loads the ctx from step 2)
+		 *
+		 * We have to directly use the ring functions because we need
+		 * special semantics.
+		 */
+		last = ring->context_switch(ring, ctx, I915_CONTEXT_SAVE_ONLY);
+		if (last != ctx) {
+			DRM_ERROR("Context switch state invalid");
+			ret = -EIO;
+			goto dctx_err_out;
+		}
+		last = ring->context_switch(ring, ctx,
+					    I915_CONTEXT_SAVE_ONLY |
+					    I915_CONTEXT_FORCED_SWITCH);
+		if (last != ctx) {
+			DRM_ERROR("First context switch fail");
+			ret = -EIO;
+			goto dctx_err_out;
+		}
+		last = ring->context_switch(ring, ctx,
+					    I915_CONTEXT_NORMAL_SWITCH |
+					    I915_CONTEXT_FORCED_SWITCH);
+		if (last != ctx) {
+			DRM_ERROR("Final context switch fail");
+			ret = -EIO;
+			goto dctx_err_out;
+		}
+	} else {
+		/* Initialize the context now so when we later switch to it we
+		 * have a "properly" setup context.
+		 */
+		u32 tmp = 0;
+		ret = i915_switch_context(ring, ctx, &tmp);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+
+dctx_err_out:
+	i915_release_context(ctx);
+	return ret;
+}
+
+/*
+ * Logical context is created, and initialized. The context has a backing buffer
+ * object which is referenced at the end of this function.
+ *
+ * @ctx_out: output context which was created
+ *
+ * In theory a context can exist for multiple rings (currently RCS, and
+ * VCS), but afaik they can't be shared. Try 
+ */
+static int logical_context_alloc(struct drm_device *dev, struct drm_file *file,
+				 struct drm_i915_gem_context **ctx_out)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_context *ctx;
+	int ret;
+
+	if (WARN_ON(!mutex_is_locked(&dev->struct_mutex)))
+		return -EINVAL;
+
+	ctx = kzalloc(sizeof(struct drm_i915_gem_context), GFP_KERNEL);
+	if (ctx == NULL)
+		return -ENOMEM;
+
+	ctx->dev = dev;
+	ctx->file = file;
+
+	if (dev_priv->default_context == *ctx_out) {
+		ctx->is_default = true;
+		ctx->id = DEFAULT_CONTEXT_ID;
+	} else
+		ctx->id = context_generate_id(ctx);
+
+
+	ctx->obj = i915_gem_alloc_object(dev, dev_priv->context_size);
+	if (!ctx->obj) {
+		ret = -ENOMEM;
+		goto id_out;
+	}
+
+	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
+	if (ret) {
+		DRM_ERROR("Failed to pin context: %d\n", ret);
+		goto err_unref;
+	}
+
+	ret = i915_gem_object_set_to_gtt_domain(ctx->obj, false);
+	if (ret) {
+		DRM_ERROR("failed to set domain on context: %d", ret);
+		goto err_unpin;
+	}
+
+	drm_gem_object_reference(&ctx->obj->base);
+
+	if (ctx->is_default)
+		BUG_ON(i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false));
+
+	/* NB: future may want to initialize for multiple rings */
+	ret = context_init(ctx, &dev_priv->ring[RCS]);
+	if (!ret)
+		ctx->ring_enable |= (1 << RCS);
+
+	if (ctx->ring_enable)
+		DRM_DEBUG_DRIVER("Context %d allocated, rings %x\n", ctx->id,
+				 ctx->ring_enable);
+	else
+		ctx = NULL;
+
+	*ctx_out = ctx;
+	return 0;
+
+err_unpin:
+	i915_gem_object_unpin(ctx->obj);
+err_unref:
+	drm_gem_object_unreference(&ctx->obj->base);
+id_out:
+	if (!ctx->is_default)
+		context_destroy_id(ctx);
+	kfree(ctx);
+	return ret;
+}
+
+/* Switch to default context for the specified ring */
+static int switch_to_default(struct intel_ring_buffer *ring,
+			     struct drm_file *file)
+{
+	struct drm_i915_gem_context *dflt;
+	u32 tmp = 0;
+	int ret;
+
+	dflt = i915_get_context(file, DEFAULT_CONTEXT_ID);
+
+	DRM_DEBUG_DRIVER("Switching to default context\n");
+
+	ret = i915_switch_context(ring, dflt, &tmp);
+	if (ret)
+		DRM_ERROR("Couldn't switch back to default context\n");
+
+	return ret;
+}
+
+static void logical_context_fini(struct drm_i915_gem_context *ctx)
+{
+	struct drm_device *dev = ctx->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int i;
+
+	if (WARN_ON(!mutex_is_locked(&dev->struct_mutex)))
+		return;
+
+	/* Since we want to have the default context around for the
+	 * duration of the driver, let DRM clean up our default context
+	 * object when needed */
+	if (ctx == dev_priv->default_context)
+		return;
+
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+		struct intel_ring_buffer *ring = &dev_priv->ring[i];
+		if (!ring->context_switch)
+			continue;
+
+		if (!(ctx->ring_enable & (1 << i)))
+			continue;
+
+		if (ring->last_context != ctx)
+			continue;
+
+		/*
+		 * XXX We can prevent restoring contexts, but not saving them
+		 * so if we're going to take away our backing context object
+		 * of the last context, we have to switch now.
+		 */
+		if (switch_to_default(ring, ctx->file)) {
+			DRM_ERROR("Failed to destroy. A BO may be leaked\n");
+			continue;
+		}
+	}
+}
+
+/*
+ * Free a context and all it's associated memory. By the end of the function the
+ * BO may not actually be freed since we need to do a context switch away from
+ * it first.
+ */
+static int logical_context_free(struct drm_file *file, uint32_t id)
+{
+	struct drm_device *dev;
+	struct drm_i915_private *dev_priv;
+	struct drm_i915_gem_context *ctx;
+
+	if (WARN_ON(id == DEFAULT_CONTEXT_ID))
+		return 0;
+
+	/*  ref and pin the object */
+	ctx = i915_get_context(file, id);
+	if (!ctx) {
+		DRM_ERROR("Couldn't find context %d", id);
+		return -EINVAL;
+	}
+
+	dev = ctx->dev;
+	dev_priv = dev->dev_private;
+	i915_release_context(ctx);
+	logical_context_fini(ctx);
+
+	BUG_ON(ctx->obj->pin_count > 1);
+	drm_gem_object_unreference(&ctx->obj->base);
+
+	context_destroy_id(ctx);
+
+	ctx->file = NULL;
+	ctx->dev = NULL;
+	kfree(ctx);
+
+	return 0;
+}
+
+/**
+ * i915_context_create_ioctl()
+ */
 int i915_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;
-	args->ctx_id = 0;
+	struct drm_i915_gem_context *out_ctx = NULL;
+	int ret = 0;
+
+	mutex_lock(&dev->struct_mutex);
+	if (dev_priv->contexts_disabled) {
+		mutex_unlock(&dev->struct_mutex);
+		return -ENOSPC;
+	}
+	ret = logical_context_alloc(dev, file, &out_ctx);
+	mutex_unlock(&dev->struct_mutex);
+	if (ret == 0) {
+		args->ctx_id = out_ctx->id;
+		if (WARN_ON(args->ctx_id == DEFAULT_CONTEXT_ID))
+			return -ENXIO;
+	}
+
 	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
-	return -EIO;
+	return ret;
 }
 
+/**
+ * i915_context_destroy_ioctl()
+ */
 int i915_context_destroy_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_destroy *args = data;
+	int ret;
+
+	if (args->ctx_id == DEFAULT_CONTEXT_ID)
+		return -EINVAL;
+
+	mutex_lock(&dev->struct_mutex);
+
+	if (dev_priv->contexts_disabled) {
+		mutex_unlock(&dev->struct_mutex);
+		return -ENOSPC;
+	}
+
 	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
-	return -EINVAL;
+	ret = logical_context_free(file, args->ctx_id);
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
 }
 
+static uint32_t
+get_context_size(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 ret = 0;
+
+	switch (INTEL_INFO(dev)->gen) {
+	case 6:
+		ret = GEN6_CXT_TOTAL_SIZE * 64;
+		break;
+	case 7:
+		ret = GEN7_CTX_TOTAL_SIZE * 64;
+		break;
+	}
+
+	return ret;
+}
+
+/**
+ * i915_context_load() - Creates a default context.
+ * @dev: Device for which the context.
+ */
 void i915_context_load(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t size;
+
+	int ret;
+	if (!HAS_HW_CONTEXTS(dev)) {
+		dev_priv->contexts_disabled = true;
+		return;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+	size = get_context_size(dev);
+	if (size > 0x4000) /*  1MB */ {
+		DRM_ERROR("Context bo size seems invalid.");
+		size = 20;
+	}
+	size = roundup(size, 4096);
+	dev_priv->context_size = size;
+	DRM_DEBUG_DRIVER("Logical context size = %d\n", size);
+	ret = logical_context_alloc(dev, NULL, &dev_priv->default_context);
+	if (ret)
+		dev_priv->contexts_disabled = true;
+	else
+		DRM_DEBUG_DRIVER("HW context support initialized\n");
+	mutex_unlock(&dev->struct_mutex);
 
 }
 
 void i915_context_unload(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (dev_priv->contexts_disabled)
+		return;
 
+	kfree(dev_priv->default_context);
 }
 
 void i915_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->contexts_disabled)
+		return;
 
+	idr_init(&file_priv->context_idr);
+	spin_lock_init(&file_priv->context_idr_lock);
 }
 
 void i915_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->contexts_disabled)
+		return;
+
+	idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
+	idr_destroy(&file_priv->context_idr);
+}
+
+static int context_idr_cleanup(int id, void *p, void *data)
+{
+	struct drm_file *file = (struct drm_file *) data;
+	int ret;
 
+	mutex_lock(&file->minor->dev->struct_mutex);
+	ret = logical_context_free(file, id);
+	mutex_unlock(&file->minor->dev->struct_mutex);
+	return ret;
 }
 
+/**
+ * Lookup a context by id, pin, and acquire a reference.
+ */
 struct drm_i915_gem_context *i915_get_context(struct drm_file *file,
 					      uint32_t id)
 {
-	return NULL;
+	struct drm_i915_private *dev_priv = file->minor->dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct drm_i915_gem_context *ctx = NULL;
+	int ret;
+
+	spin_lock(&file_priv->context_idr_lock);
+	if (id == DEFAULT_CONTEXT_ID)
+		ctx = dev_priv->default_context;
+	else
+		ctx = idr_find(&file_priv->context_idr, id);
+	if (ctx) {
+		/* Since we touch the pinned list, we must hold struct_mutex
+		 * when calling this function.
+		 */
+		BUG_ON(!mutex_is_locked(&ctx->dev->struct_mutex));
+		drm_gem_object_reference(&ctx->obj->base);
+		ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
+		if (ret) {
+			drm_gem_object_unreference(&ctx->obj->base);
+			ctx = NULL;
+		}
+	}
+	spin_unlock(&file_priv->context_idr_lock);
+
+	return ctx;
+}
+
+void i915_release_context(struct drm_i915_gem_context *ctx)
+{
+	BUG_ON(!mutex_is_locked(&ctx->dev->struct_mutex));
+	BUG_ON(ctx->is_default && ctx->obj->pin_count == 1);
+
+	i915_gem_object_unpin(ctx->obj);
+	drm_gem_object_unreference(&ctx->obj->base);
+}
+
+/*
+ * Do a context switch on ring to new context. If *seqno is 0, create
+ * a new request when the context can be retired
+ */
+int i915_switch_context(struct intel_ring_buffer *ring,
+			struct drm_i915_gem_context *to,
+			u32 *seqno)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_gem_context *from;
+	u32 type;
+	int ret = 0;
+
+	if (dev_priv->contexts_disabled || !ring->context_switch)
+		return 0;
+
+	BUG_ON(to == NULL);
+	BUG_ON(to->obj->pin_count == 0);
+
+	/*
+	 * The first time a context is loaded, it is unsafe to restore the
+	 * uninitialized context
+	 */
+	if (!to->is_initialized)
+		type = I915_CONTEXT_SAVE_ONLY;
+	else
+		type = I915_CONTEXT_NORMAL_SWITCH;
+
+	from = ring->context_switch(ring, to, type);
+	BUG_ON(from == NULL);
+
+	to->is_initialized = true;
+
+	if (!from->is_default && from != to) {
+		if (*seqno == 0) {
+			struct drm_i915_gem_request *request;
+			request = kzalloc(sizeof(*request), GFP_KERNEL);
+			if (WARN_ON(request == NULL)) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			ret = i915_add_request(ring, NULL, request);
+			if (ret) {
+				kfree(request);
+				goto out;
+			}
+			*seqno = request->seqno;
+		}
+
+		i915_gem_object_move_to_active(from->obj, ring, *seqno);
+	}
+
+out:
+	i915_release_context(from);
+	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4175929..f84d8f3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -759,7 +759,12 @@ typedef struct drm_i915_private {
 	struct backlight_device *backlight;
 
 	struct drm_property *broadcast_rgb_property;
+
 	struct drm_property *force_audio_property;
+
+	uint32_t context_size;
+	struct drm_i915_gem_context *default_context;
+	bool contexts_disabled;
 } drm_i915_private_t;
 
 enum i915_cache_level {
@@ -1200,6 +1205,10 @@ extern void i915_context_open(struct drm_device *dev, struct drm_file *file);
 extern void i915_context_close(struct drm_device *dev, struct drm_file *file);
 extern struct drm_i915_gem_context *i915_get_context(struct drm_file *file,
 						     uint32_t id);
+extern void i915_release_context(struct drm_i915_gem_context *ctx);
+extern int i915_switch_context(struct intel_ring_buffer *ring,
+			       struct drm_i915_gem_context *to,
+			       u32 *seqno);
 
 /**
  * Returns true if seq1 is later than seq2.
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 8fa509e..b8aa665 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -891,10 +891,10 @@ struct drm_intel_sprite_colorkey {
 };
 
 struct drm_i915_gem_context_create {
-	/* output: id of new context*/
+	/* output: id of new context */
 	__u32 ctx_id;
 
-	__u16 pad;
+	__u32 rsvd;
 };
 
 struct drm_i915_gem_context_destroy {
-- 
1.7.9

[PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Ben Widawsky]

Extend the flag parameter to support the context id (from the create
IOCTL) so that userspace can associate a context with the batchbuffer.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h            |    2 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   26 ++++++++++++++++++++++----
 include/drm/i915_drm.h                     |    2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f84d8f3..13e088f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1144,6 +1144,8 @@ int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
+int i915_gem_execbuffer3(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv);
 int i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv);
 int i915_gem_unpin_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 81687af..3bc5b14 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1050,7 +1050,8 @@ static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
 		       struct drm_i915_gem_execbuffer2 *args,
-		       struct drm_i915_gem_exec_object2 *exec)
+		       struct drm_i915_gem_exec_object2 *exec,
+		       u32 ctx_id)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct list_head objects;
@@ -1058,6 +1059,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;
+	struct drm_i915_gem_context *context = NULL;
 	u32 exec_start, exec_len;
 	u32 seqno;
 	u32 mask;
@@ -1270,6 +1272,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
 	exec_len = args->batch_len;
+
+	if (ring->last_context) {
+		context = i915_get_context(file, ctx_id);
+		ret = i915_switch_context(ring, context, &seqno);
+		if (ret)
+			goto err;
+	}
+
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {
 			ret = i915_emit_box(dev, &cliprects[i],
@@ -1373,7 +1383,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 	exec2.cliprects_ptr = args->cliprects_ptr;
 	exec2.flags = I915_EXEC_RENDER;
 
-	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
+	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list, 0);
 	if (!ret) {
 		/* Copy the new buffer offsets back to the user's exec list. */
 		for (i = 0; i < args->buffer_count; i++)
@@ -1397,11 +1407,12 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 }
 
 int
-i915_gem_execbuffer2(struct drm_device *dev, void *data,
+i915_gem_do_execbuffer2(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
 	struct drm_i915_gem_execbuffer2 *args = data;
 	struct drm_i915_gem_exec_object2 *exec2_list = NULL;
+	u32 ctx_id = (args->flags & I915_EXEC_CONTEXT_MASK) >> 9;
 	int ret;
 
 	if (args->buffer_count < 1) {
@@ -1430,7 +1441,7 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EFAULT;
 	}
 
-	ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list);
+	ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list, ctx_id);
 	if (!ret) {
 		/* Copy the new buffer offsets back to the user's exec list. */
 		ret = copy_to_user((struct drm_i915_relocation_entry __user *)
@@ -1448,3 +1459,10 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 	drm_free_large(exec2_list);
 	return ret;
 }
+
+int
+i915_gem_execbuffer2(struct drm_device *dev, void *data,
+		     struct drm_file *file)
+{
+	return i915_gem_do_execbuffer2(dev, data, file);
+}
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index b8aa665..4ffe81e 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -248,7 +248,6 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
 
-
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
  */
@@ -666,6 +665,7 @@ struct drm_i915_gem_execbuffer2 {
 
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
+#define I915_EXEC_CONTEXT_MASK		(((1<<22)-1) << 9)
 
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
-- 
1.7.9

[PATCH 09/11] drm/i915/context: add params

[Ben Widawsky]

From: Ben Widawsky <bwidawsk@gmail.com>

Parameters tell user space if contexts are available.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 ++
 include/drm/i915_drm.h          |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 43acac3..c5aa84c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -780,6 +780,8 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		break;
 	case I915_PARAM_HAS_RELAXED_DELTA:
 		value = 1;
+	case I915_PARAM_HAS_CONTEXTS:
+		value = dev_priv->contexts_disabled ? 0 : 1;
 		break;
 	case I915_PARAM_HAS_GEN7_SOL_RESET:
 		value = 1;
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 4ffe81e..f08714e 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -301,6 +301,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_RELAXED_DELTA	 15
 #define I915_PARAM_HAS_GEN7_SOL_RESET	 16
 #define I915_PARAM_HAS_LLC     	 17
+#define I915_PARAM_HAS_CONTEXTS		 18
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.7.9

[PATCH 10/11] drm/i915/context: track contexts per device

[Ben Widawsky]

Nice to have a list of contexts handy for debug and perhaps other things
in the future.

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

diff --git a/drivers/gpu/drm/i915/i915_context.c b/drivers/gpu/drm/i915/i915_context.c
index 0547411..93a3822 100644
--- a/drivers/gpu/drm/i915/i915_context.c
+++ b/drivers/gpu/drm/i915/i915_context.c
@@ -193,11 +193,13 @@ static int logical_context_alloc(struct drm_device *dev, struct drm_file *file,
 	if (!ret)
 		ctx->ring_enable |= (1 << RCS);
 
-	if (ctx->ring_enable)
+	if (ctx->ring_enable) {
 		DRM_DEBUG_DRIVER("Context %d allocated, rings %x\n", ctx->id,
 				 ctx->ring_enable);
-	else
+		list_add(&ctx->context_link, &dev_priv->context_list);
+	} else {
 		ctx = NULL;
+	}
 
 	*ctx_out = ctx;
 	return 0;
@@ -298,6 +300,7 @@ static int logical_context_free(struct drm_file *file, uint32_t id)
 
 	BUG_ON(ctx->obj->pin_count > 1);
 	drm_gem_object_unreference(&ctx->obj->base);
+	list_del(&ctx->context_link);
 
 	context_destroy_id(ctx);
 
@@ -404,6 +407,7 @@ void i915_context_load(struct drm_device *dev)
 	size = roundup(size, 4096);
 	dev_priv->context_size = size;
 	DRM_DEBUG_DRIVER("Logical context size = %d\n", size);
+	INIT_LIST_HEAD(&dev_priv->context_list);
 	ret = logical_context_alloc(dev, NULL, &dev_priv->default_context);
 	if (ret)
 		dev_priv->contexts_disabled = true;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 13e088f..165a5a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -765,6 +765,7 @@ typedef struct drm_i915_private {
 	uint32_t context_size;
 	struct drm_i915_gem_context *default_context;
 	bool contexts_disabled;
+	struct list_head context_list;
 } drm_i915_private_t;
 
 enum i915_cache_level {
@@ -968,6 +969,8 @@ struct drm_i915_gem_context {
 	struct drm_i915_gem_object *obj;
 	bool is_default;
 	bool is_initialized;
+
+	struct list_head context_link;
 };
 
 #define I915_CONTEXT_NORMAL_SWITCH	(1 << 0)
-- 
1.7.9

[PATCH 11/11] drm/i915: show contexts in debugfs

[Ben Widawsky]

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ae73288..7a0170c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1355,6 +1355,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_i915_gem_context *context;
 	int ret;
 
 	ret = mutex_lock_interruptible(&dev->mode_config.mutex);
@@ -1373,6 +1374,11 @@ static int i915_context_status(struct seq_file *m, void *unused)
 		seq_printf(m, "\n");
 	}
 
+	list_for_each_entry(context, &dev_priv->context_list, context_link) {
+		seq_printf(m, "logical context %d ", context->id);
+		describe_obj(m, context->obj);
+		seq_printf(m, "\n");
+	}
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return 0;
-- 
1.7.9

Re: [PATCH 02/11] drm/i915: track tlb invalidate GFX_MODE state

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> This is needed for an upcoming workaround.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

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

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

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

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

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

Re: [PATCH 05/11] drm/i915/context: Preliminary context support

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> From: Ben Widawsky <bwidawsk@gmail.com>
>
> Create all the necessary data structures and hooks to support a context
> API for userspace. Also plumb in the calls from the i915 core into the
> context subsystem. There is a new file i915_context.c which will hold a
> majority of the context related code. This file has nulled functions
> that get called by the proper part of the i915 driver. This of course
> requires a Makefile addition, as well as calls to the new nulled
> functions from the other partsd of the driver (i915_dma.c).
>
> With that, there are 2 new ioctls defined , one for creating a context,
> and one for destroying a context. These are defined in the necessary
> places, and again have null functionality
>
> It's likely that a third ioctl will be required to associate the context
> with an execbuf (likely execbuf3). Since that part is mostly trivial,
> and will surely be contentious, it's ignored for now.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

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

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

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

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

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

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Eugeni Dodonov]

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

 On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> Extend the flag parameter to support the context id (from the create
> IOCTL) so that userspace can associate a context with the batchbuffer.
>

This seems to be the easiest way to add the support for contexts. So at
least for me, I think this should do the trick.

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

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

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

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

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

Re: [PATCH 09/11] drm/i915/context: add params

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> From: Ben Widawsky <bwidawsk@gmail.com>
>
> Parameters tell user space if contexts are available.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

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

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

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

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

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

Re: [PATCH 10/11] drm/i915/context: track contexts per device

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> Nice to have a list of contexts handy for debug and perhaps other things
> in the future.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

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

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

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

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

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

Re: [PATCH 11/11] drm/i915: show contexts in debugfs

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 19:09, Ben Widawsky <ben@bwidawsk.net> wrote:

> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

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

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

[-- Attachment #1.2: Type: text/html, Size: 616 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: [RFC PATCH 00/11] i915 HW context support

[Adam Jackson]

On 2/14/12 4:09 PM, Ben Widawsky wrote:

> The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
> interface may be insufficient for that. The contexts do provide a
>

A... what?

Series looks reasonable to me.  The one thing that makes me nervous is 
4/11 mentioning the BIOS, which honestly seems like an insane thing to 
let your BIOS have _any_ say in, but then Intel sure does like to give 
the BIOS a chance to make the OS less functional.  The code in 7/11 
effectively limits things to between 4k and 1M; does that actually make 
sense?  I can't imagine wanting to allocate only 4k if it turns out we'd 
always need at least (say) 32k.

I have admittedly not read any of the docs on how this is meant to work, 
apologies if that'd answer this.

- ajax

Re: [RFC PATCH 00/11] i915 HW context support

[Eugeni Dodonov]

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

On Tue, Feb 14, 2012 at 21:17, Adam Jackson <ajax@redhat.com> wrote:

> On 2/14/12 4:09 PM, Ben Widawsky wrote:
>
>  The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
>> interface may be insufficient for that. The contexts do provide a
>>
>>
> A... what?
>
> Series looks reasonable to me.  The one thing that makes me nervous is
> 4/11 mentioning the BIOS, which honestly seems like an insane thing to let
> your BIOS have _any_ say in, but then Intel sure does like to give the BIOS
> a chance to make the OS less functional.  The code in 7/11 effectively
> limits things to between 4k and 1M; does that actually make sense?  I can't
> imagine wanting to allocate only 4k if it turns out we'd always need at
> least (say) 32k.
>
> I have admittedly not read any of the docs on how this is meant to work,
> apologies if that'd answer this.
>

The high-level overview of this feature is that it adds support for having
a per-context set of GPU items the processes have access to. This is mostly
important for GL_ARB_Robustness [1] and GL_EXT_Transform_feedback [2].

So what it does is adding a possibility of having a different context id
for each set of operations. This way, what processes in one context_id do
should not affect the processes in another set. For GL_ARB_Robustness, it
could prevent one WebGL applications from taking down all the other users
of the GPU for example; and for GL_EXT_Transform_feedback it would allow
each process to have a way to store its own feedback data for different
stages of the pipeline. And so on.

As far as I know, it is also needed for full PPGTT support; but I don't
know this part good enough to give more in-depth answer (Daniel?)

(I hope it clarified the things a bit :))

[1] http://www.opengl.org/registry/specs/ARB/robustness.txt
[2] http://www.opengl.org/registry/specs/EXT/transform_feedback.txt

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

[-- Attachment #1.2: Type: text/html, Size: 2641 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: [RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

On Tue, 14 Feb 2012 18:17:57 -0500
Adam Jackson <ajax@redhat.com> wrote:

> On 2/14/12 4:09 PM, Ben Widawsky wrote:
> 
> > The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
> > interface may be insufficient for that. The contexts do provide a
> >
> 
> A... what?

This was left up to the reader as I forgot what I meant to say..

> 
> Series looks reasonable to me.  The one thing that makes me nervous is 
> 4/11 mentioning the BIOS, which honestly seems like an insane thing to 
> let your BIOS have _any_ say in, but then Intel sure does like to give 
> the BIOS a chance to make the OS less functional.  The code in 7/11 
> effectively limits things to between 4k and 1M; does that actually make 
> sense?  I can't imagine wanting to allocate only 4k if it turns out we'd 
> always need at least (say) 32k.
> 

At least up to Gen6, 4k was sufficient. It begins to grow exponentially
afaict after that, and I've not tested on enough platforms to find out
if BIOS does any weird shit... all I know is that it can. I'll see if I
can't make this look more sane.

> I have admittedly not read any of the docs on how this is meant to work, 
> apologies if that'd answer this.
> 
> - ajax

Nah, you're good. Thanks.

Ben

Re: [PATCH 02/11] drm/i915: track tlb invalidate GFX_MODE state

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:09 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> This is needed for an upcoming workaround.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    5 +++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4956f1b..411a0e5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -412,6 +412,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
>  			return ret;
>  	}
>  
> +	if (INTEL_INFO(dev)->gen == 6)
> +		ring->itlb_before_ctx_switch =
> +			!!(I915_READ(GFX_MODE) & GFX_TLB_INVALIDATE_ALWAYS);
> +
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		I915_WRITE(INSTPM,
>  			   INSTPM_FORCE_ORDERING << 16 | INSTPM_FORCE_ORDERING);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c8b9cc0..fad4251 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -110,6 +110,11 @@ struct  intel_ring_buffer {
>  	wait_queue_head_t irq_queue;
>  	drm_local_map_t map;
>  
> +	/**
> +	 * Do an explicit TLB flush before MI_SET_CONTEXT
> +	 */
> +	bool itlb_before_ctx_switch;
> +
>  	void *private;
>  };

I really wanted to see the consumer of this when trying to review this
code.  They weren't logically separate -- this is to implement a quoted
piece of spec in the later commit.

(What I was trying to figure out was whether this was needed for gen7.
It looks like it is not required).

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 04/11] drm/i915/context: CXT_SIZE register offsets added

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:11 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> 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 |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0c785af..5e61e79 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1361,6 +1361,19 @@
>   */
>  #define CCID			0x2180
>  #define   CCID_EN		(1<<0)
> +#define CXT_SIZE		0x21a0
> +#define GEN6_CXT_RENDER_SIZE ((I915_READ(CXT_SIZE) >> 12) & 0x3f)
> +#define GEN6_CXT_EXTENDED_SIZE ((I915_READ(CXT_SIZE) >> 6) & 0x3f)
> +#define GEN6_CXT_PIPELINE_SIZE ((I915_READ(CXT_SIZE) >> 0) & 0x3f)
> +#define GEN6_CXT_TOTAL_SIZE (GEN6_CXT_RENDER_SIZE + GEN6_CXT_EXTENDED_SIZE + GEN6_CXT_PIPELINE_SIZE)
> +#define GEN7_CTX_SIZE		0x21a8
> +#define GEN7_CTX_RENDER_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 16) & 0x3f)
> +#define GEN7_CTX_EXTENDED_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 9) & 0x7f)
> +#define GEN7_CTX_GT1_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 6) & 0x7)
> +#define GEN7_CTX_VFSTATE_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 0) & 0x3f)
> +#define GEN7_CTX_TOTAL_SIZE (GEN7_CTX_RENDER_SIZE + GEN7_CTX_EXTENDED_SIZE \
> +			    + GEN7_CTX_GT1_SIZE + GEN7_CTX_VFSTATE_SIZE)
> +

I was surprised to see register reads in i915_reg.h definitions.  I'd
rather see this folded into the one callsite, and just the reg numbers
in i915_reg.h.

There's a bit of a discrepancy here -- CXT vs CTX.  The docs appear to
actually use CXT, which I will probably never type successfully since
contexts are "ctx" all over my other code.  Still, consistently using
the docs' spelling is probably better than inconsistently using it :)

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 09/11] drm/i915/context: add params

[Chris Wilson]

On Tue, 14 Feb 2012 22:09:16 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> From: Ben Widawsky <bwidawsk@gmail.com>
> 
> Parameters tell user space if contexts are available.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    2 ++
>  include/drm/i915_drm.h          |    1 +
>  2 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 43acac3..c5aa84c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -780,6 +780,8 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		break;
>  	case I915_PARAM_HAS_RELAXED_DELTA:
>  		value = 1;
> +	case I915_PARAM_HAS_CONTEXTS:
> +		value = dev_priv->contexts_disabled ? 0 : 1;
Since I don't think you meant /* fallthrough */, I guess you have a
missing break ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:15 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> Extend the flag parameter to support the context id (from the create
> IOCTL) so that userspace can associate a context with the batchbuffer.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

>  /** Resets the SO write offset registers for transform feedback on gen7. */
>  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
> +#define I915_EXEC_CONTEXT_MASK		(((1<<22)-1) << 9)

Instead of stuffing a context id into a subset of flags, could we just
use a uint32_t context id in half of rsvd1?  Yeah, having over 1 << 22
contexts is crazy, but this just seems to eat up all the rest of the
flags for no reason and then we'll have to carve out of rsvd1 for the
next flag.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Ben Widawsky]

On Wed, 15 Feb 2012 11:55:56 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:15 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Extend the flag parameter to support the context id (from the create
> > IOCTL) so that userspace can associate a context with the batchbuffer.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> >  /** Resets the SO write offset registers for transform feedback on gen7. */
> >  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
> > +#define I915_EXEC_CONTEXT_MASK		(((1<<22)-1) << 9)
> 
> Instead of stuffing a context id into a subset of flags, could we just
> use a uint32_t context id in half of rsvd1?  Yeah, having over 1 << 22
> contexts is crazy, but this just seems to eat up all the rest of the
> flags for no reason and then we'll have to carve out of rsvd1 for the
> next flag.

Sure. I didn't know if rsvd fields were allowed to be used, and I was too lazy
to check libdrm history to see if they'd always been zero'd out.

So yes, I will update this.

Thanks.

Re: [PATCH 09/11] drm/i915/context: add params

[Ben Widawsky]

On Wed, 15 Feb 2012 19:41:49 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, 14 Feb 2012 22:09:16 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > From: Ben Widawsky <bwidawsk@gmail.com>
> > 
> > Parameters tell user space if contexts are available.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c |    2 ++
> >  include/drm/i915_drm.h          |    1 +
> >  2 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 43acac3..c5aa84c 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -780,6 +780,8 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		break;
> >  	case I915_PARAM_HAS_RELAXED_DELTA:
> >  		value = 1;
> > +	case I915_PARAM_HAS_CONTEXTS:
> > +		value = dev_priv->contexts_disabled ? 0 : 1;
> Since I don't think you meant /* fallthrough */, I guess you have a
> missing break ;-)
> -Chris
> 

Thanks, this will be fixed.

Re: [PATCH 04/11] drm/i915/context: CXT_SIZE register offsets added

[Ben Widawsky]

On Wed, 15 Feb 2012 11:39:47 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:11 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > 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 |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0c785af..5e61e79 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1361,6 +1361,19 @@
> >   */
> >  #define CCID			0x2180
> >  #define   CCID_EN		(1<<0)
> > +#define CXT_SIZE		0x21a0
> > +#define GEN6_CXT_RENDER_SIZE ((I915_READ(CXT_SIZE) >> 12) & 0x3f)
> > +#define GEN6_CXT_EXTENDED_SIZE ((I915_READ(CXT_SIZE) >> 6) & 0x3f)
> > +#define GEN6_CXT_PIPELINE_SIZE ((I915_READ(CXT_SIZE) >> 0) & 0x3f)
> > +#define GEN6_CXT_TOTAL_SIZE (GEN6_CXT_RENDER_SIZE + GEN6_CXT_EXTENDED_SIZE + GEN6_CXT_PIPELINE_SIZE)
> > +#define GEN7_CTX_SIZE		0x21a8
> > +#define GEN7_CTX_RENDER_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 16) & 0x3f)
> > +#define GEN7_CTX_EXTENDED_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 9) & 0x7f)
> > +#define GEN7_CTX_GT1_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 6) & 0x7)
> > +#define GEN7_CTX_VFSTATE_SIZE ((I915_READ(GEN7_CTX_SIZE) >> 0) & 0x3f)
> > +#define GEN7_CTX_TOTAL_SIZE (GEN7_CTX_RENDER_SIZE + GEN7_CTX_EXTENDED_SIZE \
> > +			    + GEN7_CTX_GT1_SIZE + GEN7_CTX_VFSTATE_SIZE)
> > +
> 
> I was surprised to see register reads in i915_reg.h definitions.  I'd
> rather see this folded into the one callsite, and just the reg numbers
> in i915_reg.h.

This will be fixed.

> 
> There's a bit of a discrepancy here -- CXT vs CTX.  The docs appear to
> actually use CXT, which I will probably never type successfully since
> contexts are "ctx" all over my other code.  Still, consistently using
> the docs' spelling is probably better than inconsistently using it :)

I think maybe a comment warning the reader of the typo could help.

Re: [PATCH 06/11] drm/i915/context: ringbuffer context switch code

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:13 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> This is the HW dependent context switch code.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    3 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  117 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    6 ++-
>  3 files changed, 125 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 34e6f4f..4175929 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -965,6 +965,9 @@ struct drm_i915_gem_context {
>  	bool is_initialized;
>  };
>  
> +#define I915_CONTEXT_NORMAL_SWITCH	(1 << 0)
> +#define I915_CONTEXT_SAVE_ONLY		(1 << 1)
> +#define I915_CONTEXT_FORCED_SWITCH	(1 << 2)
>  #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
>  
>  #define IS_I830(dev)		((dev)->pci_device == 0x3577)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e71e7fc..dcdc80e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -942,6 +942,122 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
>  	return 0;
>  }
>  
> +static int do_ring_switch(struct intel_ring_buffer *ring,
> +			  struct drm_i915_gem_context *new_context,
> +			  u32 hw_flags)

Can we call this function do_mi_set_context()?  It doesn't look like it
has to do with switching rings.

> +{
> +	struct drm_device *dev = ring->dev;
> +	int ret = 0;
> +
> +	if (!new_context->is_initialized) {
> +		ret = ring->flush(ring, 0, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = intel_ring_begin(ring, 2);
> +		if (ret)
> +			return ret;
> +
> +		intel_ring_emit(ring, MI_NOOP | (1 << 22) | new_context->id);
> +		intel_ring_emit(ring, MI_NOOP);
> +		intel_ring_advance(ring);
> +	}

Not sure what this block is doing, nor have the docs enlightened me.
Comment?

> +	if (IS_GEN6(dev) && new_context->is_initialized &&
> +	    ring->itlb_before_ctx_switch) {
> +		/* w/a: If Flush TLB Invalidation Mode is enabled, driver must
> +		 * do a TLB invalidation prior to MI_SET_CONTEXT
> +		 */
> +		gen6_render_ring_flush(ring, 0, 0);
> +	}
> +
> +	ret = intel_ring_begin(ring, 6);
> +	if (ret)
> +		return ret;
> +
> +	intel_ring_emit(ring, MI_NOOP);
> +
> +	switch (INTEL_INFO(dev)->gen) {
> +	case 5:
> +		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> +		break;
> +	case 6:
> +		intel_ring_emit(ring, MI_NOOP);
> +		break;
> +	case 7:
> +		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> +		break;
> +	case 4:
> +	default:
> +		BUG();
> +	}

I can't see what this MI_ARB_ON_OFF is about.  We don't use run lists,
so preemption can't occur, right?  And if it's needed on gen7, why isn't
it needed on the previous chipsets, where the command apparently exists
as well?

Also, MI_SUSPEND_FLUSH?  (also exists on all chispets) It "Blocks MMIO
sync flush or any flushes related to VT-d while enabled."  We don't use
sync flushes, and presumably if we do VT-d related flushes, we still
want them to work, right?  Why do hardware render contexts change that?

> +	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);


> +static struct drm_i915_gem_context *
> +render_ring_context_switch(struct intel_ring_buffer *ring,
> +			   struct drm_i915_gem_context *new_context,
> +			   u32 flags)
> +{
> +	struct drm_device *dev = ring->dev;
> +	bool force = (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
> +	struct drm_i915_gem_context *last = NULL;
> +	uint32_t hw_flags = 0;
> +
> +	/* last_context is only protected by struct_mutex */
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +
> +	BUG_ON(new_context->obj == NULL || new_context->obj->gtt_offset == 0);
> +
> +	if (!force && (ring->last_context == new_context))
> +		return new_context;
> +
> +	if (flags & I915_CONTEXT_SAVE_ONLY)
> +		hw_flags = MI_RESTORE_INHIBIT;
> +
> +	if (do_ring_switch(ring, new_context, hw_flags))
> +		return NULL;
> +
> +	last = ring->last_context;
> +	ring->last_context = new_context;
> +
> +	/* The first context switch with default context is special */
> +	if (last == NULL && new_context->is_default)
> +		return new_context;

I'm concerned that by returning new_context here, we're about to
release_context() and unpin the context we just switched to, in
i915_switch_context().  I think we don't want to ever return new_context
here, and the caller wants to not do the request stuff in the case that
From == NULL.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:15 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> Extend the flag parameter to support the context id (from the create
> IOCTL) so that userspace can associate a context with the batchbuffer.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

> @@ -1270,6 +1272,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
>  	exec_len = args->batch_len;
> +
> +	if (ring->last_context) {
> +		context = i915_get_context(file, ctx_id);

Add code here to handle context == NULL and erroring out, since
i915_switch_context() just BUG_ONs.

> +		ret = i915_switch_context(ring, context, &seqno);
> +		if (ret)
> +			goto err;
> +	}
> +

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 05/11] drm/i915/context: Preliminary context support

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:12 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> From: Ben Widawsky <bwidawsk@gmail.com>
> 
> Create all the necessary data structures and hooks to support a context
> API for userspace. Also plumb in the calls from the i915 core into the
> context subsystem. There is a new file i915_context.c which will hold a
> majority of the context related code. This file has nulled functions
> that get called by the proper part of the i915 driver. This of course
> requires a Makefile addition, as well as calls to the new nulled
> functions from the other partsd of the driver (i915_dma.c).
> 
> With that, there are 2 new ioctls defined , one for creating a context,
> and one for destroying a context. These are defined in the necessary
> places, and again have null functionality
> 
> It's likely that a third ioctl will be required to associate the context
> with an execbuf (likely execbuf3). Since that part is mostly trivial,
> and will surely be contentious, it's ignored for now.

> +struct drm_i915_gem_context_create {
> +	/* output: id of new context*/
> +	__u32 ctx_id;
> +
> +	__u16 pad;
> +};

The pad should be made u32 at this point instead of fixing it up in a
later commit.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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: [RFC PATCH 00/11] i915 HW context support

[Eric Anholt]

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

On Tue, 14 Feb 2012 22:09:07 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> These patches are a heavily revised version of the patches I wrote over
> a year ago. These patches have passed basic tests on SNB, and IVB, and
> older versions worked on ILK.  In theory, context support should work
> all the way back to Gen4, but I haven't tested it. Also since I suspect
> ILK may be unstable, so the code has it disabled for now.
> 
> HW contexts provide a way for the GPU to save an restore certain state
> in between batchbuffer boundaries. Typically, GPU clients must re-emit
> the entire state every time they run because the client does not know
> what has been destroyed since the last time. With these patches the
> driver will emit special instructions to do this on behalf of the client
> if it has registered a context, and included that with the batchbuffer.

These patches look pretty solid.  In particular, the API
(create/destroy/context id in execbuf) looks like just what we want for
Mesa.  I'll try to get around to testing it out soon (I'm poking at some
performance stuff currently where this might become relevant soon).

The couple of patches without a comment from me are:

Reviewed-by: Eric Anholt <eric@anholt.net>


[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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: [RFC PATCH 00/11] i915 HW context support

[Daniel Vetter]

On Tue, Feb 14, 2012 at 10:09:07PM +0100, Ben Widawsky wrote:
> These patches are a heavily revised version of the patches I wrote over
> a year ago. These patches have passed basic tests on SNB, and IVB, and
> older versions worked on ILK.  In theory, context support should work
> all the way back to Gen4, but I haven't tested it. Also since I suspect
> ILK may be unstable, so the code has it disabled for now.
> 
> HW contexts provide a way for the GPU to save an restore certain state
> in between batchbuffer boundaries. Typically, GPU clients must re-emit
> the entire state every time they run because the client does not know
> what has been destroyed since the last time. With these patches the
> driver will emit special instructions to do this on behalf of the client
> if it has registered a context, and included that with the batchbuffer.
> 
> [... From Ken Graunke ]
> This is needed to properly implement Transform Feedback in the presence
> of Geometry Shaders, since software wouldn't be able to track the SVBI0
> register to restore it to its previous state.
> 
> The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
> interface may be insufficient for that. The contexts do provide a
> 
> I've tested this with an experiemental mesa from Ken Graunke, as well as
> some basic i-g-t tests.
> 
> i-g-t:
> git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support
> 
> libdrm:
> git://people.freedesktop.org/~bwidawsk/drm context_support
> 
> kernel:
> git://people.freedesktop.org/~bwidawsk/drm-intel context_support
> 
> I'm getting really tired of looking at these, please help me polish them
> up and make them more ready for consumption. I did consider moving the
> power context allocation to this set of APIs, but it didn't seem worth
> it to me. I can go either way.
> 
> Ben Widawsky (11):
>   drm/i915: MI_ARB_ON_OFF
>   drm/i915: track tlb invalidate GFX_MODE state
>   drm/i915: PIPE_CONTROL_TLB_INVALIDATE
>   drm/i915/context: CXT_SIZE register offsets added
>   drm/i915/context: Preliminary context support
>   drm/i915/context: ringbuffer context switch code
>   drm/i915/context: implementation details
>   drm/i915/context: extend contexts to execbuffer2
>   drm/i915/context: add params
>   drm/i915/context: track contexts per device
>   drm/i915: show contexts in debugfs
> 
>  drivers/gpu/drm/i915/Makefile              |    1 +
>  drivers/gpu/drm/i915/i915_context.c        |  562 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_debugfs.c        |    6 +
>  drivers/gpu/drm/i915/i915_dma.c            |   10 +
>  drivers/gpu/drm/i915/i915_drv.h            |   47 +++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   26 ++-
>  drivers/gpu/drm/i915/i915_reg.h            |   17 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  122 ++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +-
>  include/drm/i915_drm.h                     |   18 +
>  10 files changed, 815 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_context.c

Eric summed up most of my concerns when I've quickly read through these,
a few are left though. First are bikesheddy in nature, but they hindered
me a bit in digging into the last one, so here we go.

- The patch splitting is a bit strange for me to follow. For new features
  I prefere patches split up into:
  * prep work on existing code, if necessary
  * implement the feature
  * applying any ugly workarounds (if they warrant special attention)
  * enable/use the new feature/wire up the new interfaces
  I don't expect you to rework the entire series, but I think some more
  descriptive commit message would help a lot here (e.g. "implementation
  details" is imo a bit too generic for a patch headline).

- You add a new ringbuffer function pointer but don't actually use it to
  abstract away any generation/chip specific things. Imo such indirection
  should only be added when it's actually used - we then stand a chance to
  get it somewhat right and useful. Furthermore I don't have a high notion
  of the ringbuffer interface, so better not shove more stuff on top of
  it.

Now the real thing is that I expected some decent reference-counting on
the context backing storage bo to get the lifetimes right, but haven't
found it. And indeed you have a comment in the context free function to
the effect that if the switch away form a context fails things will blow
up.

I think when you switch the low-level context handling and switching code
over to just deal with the storage bo you can properly reference count
that and just keep onto that if the ring still uses it, but the userspace
handle gets freed. It should work pretty much exactly like framebuffer
objects and pageflipping (safe for that pageflips run on a different
asynchronous queue than render stuff, so they're a bit more funky and you
can't use the active list to help you in the object tracking).

I think it would even make sense to make the default context just a bo
associated with the render ring and ditch the context struct around it -
but I couldn't follow this idea through the convoluted patch series
structure.

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

Re: [PATCH 06/11] drm/i915/context: ringbuffer context switch code

[Ben Widawsky]

On Wed, 15 Feb 2012 12:11:58 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:13 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > This is the HW dependent context switch code.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |    3 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |  117 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |    6 ++-
> >  3 files changed, 125 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 34e6f4f..4175929 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -965,6 +965,9 @@ struct drm_i915_gem_context {
> >  	bool is_initialized;
> >  };
> >  
> > +#define I915_CONTEXT_NORMAL_SWITCH	(1 << 0)
> > +#define I915_CONTEXT_SAVE_ONLY		(1 << 1)
> > +#define I915_CONTEXT_FORCED_SWITCH	(1 << 2)
> >  #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
> >  
> >  #define IS_I830(dev)		((dev)->pci_device == 0x3577)
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index e71e7fc..dcdc80e 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -942,6 +942,122 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
> >  	return 0;
> >  }
> >  
> > +static int do_ring_switch(struct intel_ring_buffer *ring,
> > +			  struct drm_i915_gem_context *new_context,
> > +			  u32 hw_flags)
> 
> Can we call this function do_mi_set_context()?  It doesn't look like it
> has to do with switching rings.

Sure.

> 
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	int ret = 0;
> > +
> > +	if (!new_context->is_initialized) {
> > +		ret = ring->flush(ring, 0, 0);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = intel_ring_begin(ring, 2);
> > +		if (ret)
> > +			return ret;
> > +
> > +		intel_ring_emit(ring, MI_NOOP | (1 << 22) | new_context->id);
> > +		intel_ring_emit(ring, MI_NOOP);
> > +		intel_ring_advance(ring);
> > +	}
> 
> Not sure what this block is doing, nor have the docs enlightened me.
> Comment?

This incantation came from a document which I can no longer find. I'll
try to remove it and see if anything breaks. It's likely this was from
an old ILK doc if you're curious enough to look (mine doesn't appear old
enough)

> 
> > +	if (IS_GEN6(dev) && new_context->is_initialized &&
> > +	    ring->itlb_before_ctx_switch) {
> > +		/* w/a: If Flush TLB Invalidation Mode is enabled, driver must
> > +		 * do a TLB invalidation prior to MI_SET_CONTEXT
> > +		 */
> > +		gen6_render_ring_flush(ring, 0, 0);
> > +	}
> > +
> > +	ret = intel_ring_begin(ring, 6);
> > +	if (ret)
> > +		return ret;
> > +
> > +	intel_ring_emit(ring, MI_NOOP);
> > +
> > +	switch (INTEL_INFO(dev)->gen) {
> > +	case 5:
> > +		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> > +		break;
> > +	case 6:
> > +		intel_ring_emit(ring, MI_NOOP);
> > +		break;
> > +	case 7:
> > +		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > +		break;
> > +	case 4:
> > +	default:
> > +		BUG();
> > +	}
> 
> I can't see what this MI_ARB_ON_OFF is about.  We don't use run lists,
> so preemption can't occur, right?  And if it's needed on gen7, why isn't
> it needed on the previous chipsets, where the command apparently exists
> as well?

Just following the docs to the letter on the ARB_ON_OFF thing. We may
need the TLB flush on gen7 as well as gen6. I choose the BSPEC
workaround list as the ultimate guide. As best I can tell, the logic is
correct according to that.

> 
> Also, MI_SUSPEND_FLUSH?  (also exists on all chispets) It "Blocks MMIO
> sync flush or any flushes related to VT-d while enabled."  We don't use
> sync flushes, and presumably if we do VT-d related flushes, we still
> want them to work, right?  Why do hardware render contexts change that?

I can't find this one either anymore. This I very clearly recall as a
required workaround for ILK. Since I'm not exposing this currently for
less than GEN6 this is a don't care. My current ILK docs are not the
same as the ones I used when I wrote this. If you look at
ironlake_enable_rc6() you can also see this workaround used.

> 
> > +	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);
> 
> 
> > +static struct drm_i915_gem_context *
> > +render_ring_context_switch(struct intel_ring_buffer *ring,
> > +			   struct drm_i915_gem_context *new_context,
> > +			   u32 flags)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	bool force = (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
> > +	struct drm_i915_gem_context *last = NULL;
> > +	uint32_t hw_flags = 0;
> > +
> > +	/* last_context is only protected by struct_mutex */
> > +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > +
> > +	BUG_ON(new_context->obj == NULL || new_context->obj->gtt_offset == 0);
> > +
> > +	if (!force && (ring->last_context == new_context))
> > +		return new_context;
> > +
> > +	if (flags & I915_CONTEXT_SAVE_ONLY)
> > +		hw_flags = MI_RESTORE_INHIBIT;
> > +
> > +	if (do_ring_switch(ring, new_context, hw_flags))
> > +		return NULL;
> > +
> > +	last = ring->last_context;
> > +	ring->last_context = new_context;
> > +
> > +	/* The first context switch with default context is special */
> > +	if (last == NULL && new_context->is_default)
> > +		return new_context;
> 
> I'm concerned that by returning new_context here, we're about to
> release_context() and unpin the context we just switched to, in
> i915_switch_context().  I think we don't want to ever return new_context
> here, and the caller wants to not do the request stuff in the case that
> From == NULL.

The only case where last is ever null is when setting the default
context. I can add an else BUG() to this if it makes you more
comfortable. I used to have a comment about how this is an ugly hack
where a NULL return means error, and anything else means success. I
should probably add that comment back, not sure what happened to it. But
I only care about this when initializing the default context.

Re: [RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

On Wed, 15 Feb 2012 12:33:38 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:07 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > These patches are a heavily revised version of the patches I wrote over
> > a year ago. These patches have passed basic tests on SNB, and IVB, and
> > older versions worked on ILK.  In theory, context support should work
> > all the way back to Gen4, but I haven't tested it. Also since I suspect
> > ILK may be unstable, so the code has it disabled for now.
> > 
> > HW contexts provide a way for the GPU to save an restore certain state
> > in between batchbuffer boundaries. Typically, GPU clients must re-emit
> > the entire state every time they run because the client does not know
> > what has been destroyed since the last time. With these patches the
> > driver will emit special instructions to do this on behalf of the client
> > if it has registered a context, and included that with the batchbuffer.
> 
> These patches look pretty solid.  In particular, the API
> (create/destroy/context id in execbuf) looks like just what we want for
> Mesa.  I'll try to get around to testing it out soon (I'm poking at some
> performance stuff currently where this might become relevant soon).

I've just started noticing GPU hangs with Ken's test mesa branch on
nexuiz with vsync, full 13x7 and max effects.  It seems to work fine 
with variations like windowed, lower detail, etc. Although it looks
weird on IVB, I cannot reproduce the hangs there. Also, I'd never seen 
the hangs before this morning, and I'm not sure what has changed. So 
FYI, you may want to start out with IVB (unless you want to help me 
figure out what is broken on SNB :-)

I've not tried very hard, but so far it only seems to occur when doing
context switches, however MI_SET_CONTEXT is nowhere in the error state.

> 
> The couple of patches without a comment from me are:
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 

Re: [RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

On Thu, 16 Feb 2012 13:04:14 +0100
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, 15 Feb 2012 12:33:38 -0800
> Eric Anholt <eric@anholt.net> wrote:
> 
> > On Tue, 14 Feb 2012 22:09:07 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > These patches are a heavily revised version of the patches I wrote over
> > > a year ago. These patches have passed basic tests on SNB, and IVB, and
> > > older versions worked on ILK.  In theory, context support should work
> > > all the way back to Gen4, but I haven't tested it. Also since I suspect
> > > ILK may be unstable, so the code has it disabled for now.
> > > 
> > > HW contexts provide a way for the GPU to save an restore certain state
> > > in between batchbuffer boundaries. Typically, GPU clients must re-emit
> > > the entire state every time they run because the client does not know
> > > what has been destroyed since the last time. With these patches the
> > > driver will emit special instructions to do this on behalf of the client
> > > if it has registered a context, and included that with the batchbuffer.
> > 
> > These patches look pretty solid.  In particular, the API
> > (create/destroy/context id in execbuf) looks like just what we want for
> > Mesa.  I'll try to get around to testing it out soon (I'm poking at some
> > performance stuff currently where this might become relevant soon).
> 
> I've just started noticing GPU hangs with Ken's test mesa branch on
> nexuiz with vsync, full 13x7 and max effects.  It seems to work fine 
> with variations like windowed, lower detail, etc. Although it looks
> weird on IVB, I cannot reproduce the hangs there. Also, I'd never seen 
> the hangs before this morning, and I'm not sure what has changed. So 
> FYI, you may want to start out with IVB (unless you want to help me 
> figure out what is broken on SNB :-)
> 
> I've not tried very hard, but so far it only seems to occur when doing
> context switches, however MI_SET_CONTEXT is nowhere in the error state.

Seems like sandybridge + fullscreen nexuiz is the exact fail combo.

> 
> > 
> > The couple of patches without a comment from me are:
> > 
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> > 
> 

Re: [RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

On Thu, 16 Feb 2012 13:21:42 +0100
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Thu, 16 Feb 2012 13:04:14 +0100
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > On Wed, 15 Feb 2012 12:33:38 -0800
> > Eric Anholt <eric@anholt.net> wrote:
> > 
> > > On Tue, 14 Feb 2012 22:09:07 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > > These patches are a heavily revised version of the patches I wrote over
> > > > a year ago. These patches have passed basic tests on SNB, and IVB, and
> > > > older versions worked on ILK.  In theory, context support should work
> > > > all the way back to Gen4, but I haven't tested it. Also since I suspect
> > > > ILK may be unstable, so the code has it disabled for now.
> > > > 
> > > > HW contexts provide a way for the GPU to save an restore certain state
> > > > in between batchbuffer boundaries. Typically, GPU clients must re-emit
> > > > the entire state every time they run because the client does not know
> > > > what has been destroyed since the last time. With these patches the
> > > > driver will emit special instructions to do this on behalf of the client
> > > > if it has registered a context, and included that with the batchbuffer.
> > > 
> > > These patches look pretty solid.  In particular, the API
> > > (create/destroy/context id in execbuf) looks like just what we want for
> > > Mesa.  I'll try to get around to testing it out soon (I'm poking at some
> > > performance stuff currently where this might become relevant soon).
> > 
> > I've just started noticing GPU hangs with Ken's test mesa branch on
> > nexuiz with vsync, full 13x7 and max effects.  It seems to work fine 
> > with variations like windowed, lower detail, etc. Although it looks
> > weird on IVB, I cannot reproduce the hangs there. Also, I'd never seen 
> > the hangs before this morning, and I'm not sure what has changed. So 
> > FYI, you may want to start out with IVB (unless you want to help me 
> > figure out what is broken on SNB :-)
> > 
> > I've not tried very hard, but so far it only seems to occur when doing
> > context switches, however MI_SET_CONTEXT is nowhere in the error state.
> 
> Seems like sandybridge + fullscreen nexuiz is the exact fail combo.

So here was part of Ken's patch
-   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;


If I go back to normal and use contexts, but also set BRW_NEW_CONTEXT, I
don't get hangs. So it sounds like some part of the state isn't being
saved or restored properly. Still trying to figure out what changed to
make it break.

As a side note, IVB has weird artifacts which I thought were just
because I was using an experimental mesa, but now think it's likely the
same cause. Maybe you can figure out what isn't be saved or restored
based on seeing IVB?

Re: [RFC PATCH 00/11] i915 HW context support

[Eric Anholt]

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

On Thu, 16 Feb 2012 14:58:59 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, 16 Feb 2012 13:21:42 +0100
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > On Thu, 16 Feb 2012 13:04:14 +0100
> > Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > > On Wed, 15 Feb 2012 12:33:38 -0800
> > > Eric Anholt <eric@anholt.net> wrote:
> > > 
> > > > On Tue, 14 Feb 2012 22:09:07 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > > > These patches are a heavily revised version of the patches I wrote over
> > > > > a year ago. These patches have passed basic tests on SNB, and IVB, and
> > > > > older versions worked on ILK.  In theory, context support should work
> > > > > all the way back to Gen4, but I haven't tested it. Also since I suspect
> > > > > ILK may be unstable, so the code has it disabled for now.
> > > > > 
> > > > > HW contexts provide a way for the GPU to save an restore certain state
> > > > > in between batchbuffer boundaries. Typically, GPU clients must re-emit
> > > > > the entire state every time they run because the client does not know
> > > > > what has been destroyed since the last time. With these patches the
> > > > > driver will emit special instructions to do this on behalf of the client
> > > > > if it has registered a context, and included that with the batchbuffer.
> > > > 
> > > > These patches look pretty solid.  In particular, the API
> > > > (create/destroy/context id in execbuf) looks like just what we want for
> > > > Mesa.  I'll try to get around to testing it out soon (I'm poking at some
> > > > performance stuff currently where this might become relevant soon).
> > > 
> > > I've just started noticing GPU hangs with Ken's test mesa branch on
> > > nexuiz with vsync, full 13x7 and max effects.  It seems to work fine 
> > > with variations like windowed, lower detail, etc. Although it looks
> > > weird on IVB, I cannot reproduce the hangs there. Also, I'd never seen 
> > > the hangs before this morning, and I'm not sure what has changed. So 
> > > FYI, you may want to start out with IVB (unless you want to help me 
> > > figure out what is broken on SNB :-)
> > > 
> > > I've not tried very hard, but so far it only seems to occur when doing
> > > context switches, however MI_SET_CONTEXT is nowhere in the error state.
> > 
> > Seems like sandybridge + fullscreen nexuiz is the exact fail combo.
> 
> So here was part of Ken's patch
> -   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;
> 
> 
> If I go back to normal and use contexts, but also set BRW_NEW_CONTEXT, I
> don't get hangs. So it sounds like some part of the state isn't being
> saved or restored properly. Still trying to figure out what changed to
> make it break.

Likely.  The *idea* behind CONTEXT vs BATCH was that you could just turn
the CONTEXT flagging off, but since there was no effective difference
it's totally untested until now.

> As a side note, IVB has weird artifacts which I thought were just
> because I was using an experimental mesa, but now think it's likely the
> same cause. Maybe you can figure out what isn't be saved or restored
> based on seeing IVB?

IVB weird rendering in nexuiz appears to be related to some failure in
the FS codegen.  I think.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Jesse Barnes]

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

On Wed, 15 Feb 2012 21:00:16 +0100
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, 15 Feb 2012 11:55:56 -0800
> Eric Anholt <eric@anholt.net> wrote:
> 
> > On Tue, 14 Feb 2012 22:09:15 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > Extend the flag parameter to support the context id (from the create
> > > IOCTL) so that userspace can associate a context with the batchbuffer.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > >  /** Resets the SO write offset registers for transform feedback on gen7. */
> > >  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
> > > +#define I915_EXEC_CONTEXT_MASK		(((1<<22)-1) << 9)
> > 
> > Instead of stuffing a context id into a subset of flags, could we just
> > use a uint32_t context id in half of rsvd1?  Yeah, having over 1 << 22
> > contexts is crazy, but this just seems to eat up all the rest of the
> > flags for no reason and then we'll have to carve out of rsvd1 for the
> > next flag.
> 
> Sure. I didn't know if rsvd fields were allowed to be used, and I was too lazy
> to check libdrm history to see if they'd always been zero'd out.
> 
> So yes, I will update this.

Yeah it's ok to use them.  We added them for just this purpose iirc,
and even tried to make sure we could use them in the future by making
sure old (then current) userspace zeroed them out.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH 08/11] drm/i915/context: extend contexts to execbuffer2

[Ben Widawsky]

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

On Wed, 15 Feb 2012 12:16:20 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:15 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Extend the flag parameter to support the context id (from the create
> > IOCTL) so that userspace can associate a context with the batchbuffer.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> > @@ -1270,6 +1272,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  
> >  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> >  	exec_len = args->batch_len;
> > +
> > +	if (ring->last_context) {
> > +		context = i915_get_context(file, ctx_id);
> 
> Add code here to handle context == NULL and erroring out, since
> i915_switch_context() just BUG_ONs.
> 
> > +		ret = i915_switch_context(ring, context, &seqno);
> > +		if (ret)
> > +			goto err;
> > +	}
> > +

Nice catch. This was the result of a last minute code change (context
was already found prior to this function in the original code). Fixed.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

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

Re: [PATCH 02/11] drm/i915: track tlb invalidate GFX_MODE state

[Ben Widawsky]

On Tue, 14 Feb 2012 20:48:27 -0200
Eugeni Dodonov <eugeni@dodonov.net> wrote:

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

Thanks.

Re: [PATCH 02/11] drm/i915: track tlb invalidate GFX_MODE state

[Ben Widawsky]

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

On Wed, 15 Feb 2012 11:36:10 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:09 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > This is needed for an upcoming workaround.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |    5 +++++
> >  2 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 4956f1b..411a0e5 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -412,6 +412,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
> >  			return ret;
> >  	}
> >  
> > +	if (INTEL_INFO(dev)->gen == 6)
> > +		ring->itlb_before_ctx_switch =
> > +			!!(I915_READ(GFX_MODE) & GFX_TLB_INVALIDATE_ALWAYS);
> > +
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		I915_WRITE(INSTPM,
> >  			   INSTPM_FORCE_ORDERING << 16 | INSTPM_FORCE_ORDERING);
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index c8b9cc0..fad4251 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -110,6 +110,11 @@ struct  intel_ring_buffer {
> >  	wait_queue_head_t irq_queue;
> >  	drm_local_map_t map;
> >  
> > +	/**
> > +	 * Do an explicit TLB flush before MI_SET_CONTEXT
> > +	 */
> > +	bool itlb_before_ctx_switch;
> > +
> >  	void *private;
> >  };
> 
> I really wanted to see the consumer of this when trying to review this
> code.  They weren't logically separate -- this is to implement a quoted
> piece of spec in the later commit.
> 
> (What I was trying to figure out was whether this was needed for gen7.
> It looks like it is not required).

I'm happy to move this around to whatever order/split you'd like. Could
you please recommend how you would like me to break it up? Daniel has a
later email which I'm planning to try to follow.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

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

Re: [PATCH 05/11] drm/i915/context: Preliminary context support

[Ben Widawsky]

On Tue, 14 Feb 2012 20:49:00 -0200
Eugeni Dodonov <eugeni@dodonov.net> wrote:

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

Thanks.

Re: [RFC PATCH 00/11] i915 HW context support

[Ben Widawsky]

On Wed, 15 Feb 2012 23:12:20 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Feb 14, 2012 at 10:09:07PM +0100, Ben Widawsky wrote:
> > These patches are a heavily revised version of the patches I wrote over
> > a year ago. These patches have passed basic tests on SNB, and IVB, and
> > older versions worked on ILK.  In theory, context support should work
> > all the way back to Gen4, but I haven't tested it. Also since I suspect
> > ILK may be unstable, so the code has it disabled for now.
> > 
> > HW contexts provide a way for the GPU to save an restore certain state
> > in between batchbuffer boundaries. Typically, GPU clients must re-emit
> > the entire state every time they run because the client does not know
> > what has been destroyed since the last time. With these patches the
> > driver will emit special instructions to do this on behalf of the client
> > if it has registered a context, and included that with the batchbuffer.
> > 
> > [... From Ken Graunke ]
> > This is needed to properly implement Transform Feedback in the presence
> > of Geometry Shaders, since software wouldn't be able to track the SVBI0
> > register to restore it to its previous state.
> > 
> > The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
> > interface may be insufficient for that. The contexts do provide a
> > 
> > I've tested this with an experiemental mesa from Ken Graunke, as well as
> > some basic i-g-t tests.
> > 
> > i-g-t:
> > git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support
> > 
> > libdrm:
> > git://people.freedesktop.org/~bwidawsk/drm context_support
> > 
> > kernel:
> > git://people.freedesktop.org/~bwidawsk/drm-intel context_support
> > 
> > I'm getting really tired of looking at these, please help me polish them
> > up and make them more ready for consumption. I did consider moving the
> > power context allocation to this set of APIs, but it didn't seem worth
> > it to me. I can go either way.
> > 
> > Ben Widawsky (11):
> >   drm/i915: MI_ARB_ON_OFF
> >   drm/i915: track tlb invalidate GFX_MODE state
> >   drm/i915: PIPE_CONTROL_TLB_INVALIDATE
> >   drm/i915/context: CXT_SIZE register offsets added
> >   drm/i915/context: Preliminary context support
> >   drm/i915/context: ringbuffer context switch code
> >   drm/i915/context: implementation details
> >   drm/i915/context: extend contexts to execbuffer2
> >   drm/i915/context: add params
> >   drm/i915/context: track contexts per device
> >   drm/i915: show contexts in debugfs
> > 
> >  drivers/gpu/drm/i915/Makefile              |    1 +
> >  drivers/gpu/drm/i915/i915_context.c        |  562 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_debugfs.c        |    6 +
> >  drivers/gpu/drm/i915/i915_dma.c            |   10 +
> >  drivers/gpu/drm/i915/i915_drv.h            |   47 +++
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   26 ++-
> >  drivers/gpu/drm/i915/i915_reg.h            |   17 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c    |  122 ++++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +-
> >  include/drm/i915_drm.h                     |   18 +
> >  10 files changed, 815 insertions(+), 5 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/i915_context.c
> 
> Eric summed up most of my concerns when I've quickly read through these,
> a few are left though. First are bikesheddy in nature, but they hindered
> me a bit in digging into the last one, so here we go.
> 
> - The patch splitting is a bit strange for me to follow. For new features
>   I prefere patches split up into:
>   * prep work on existing code, if necessary
>   * implement the feature
>   * applying any ugly workarounds (if they warrant special attention)
>   * enable/use the new feature/wire up the new interfaces
>   I don't expect you to rework the entire series, but I think some more
>   descriptive commit message would help a lot here (e.g. "implementation
>   details" is imo a bit too generic for a patch headline).

I will try to rework the series for better ordering. Since Eric also
stated his distaste for the ordering. I believe the patches are correct
as is, just the ordering needs some tweaks, so shouldn't be too much
work.

> 
> - You add a new ringbuffer function pointer but don't actually use it to
>   abstract away any generation/chip specific things. Imo such indirection
>   should only be added when it's actually used - we then stand a chance to
>   get it somewhat right and useful. Furthermore I don't have a high notion
>   of the ringbuffer interface, so better not shove more stuff on top of
>   it.

I'll sum up our private conversations on this matter... You dislike
intel_ringbuffer.c and are therefore quite partial. I really don't care
much one way or the other. Context switching should work for media ring
iirc, and I do get to take a shortcut to determine if the pointer is
NULL. I don't want to argue whose opinion deserves more merit, so I'll
just change it and move on (and now my shot, it would have been easier
if you responded to the patch that added the function pointer instead of
the intro email).
> 
> Now the real thing is that I expected some decent reference-counting on
> the context backing storage bo to get the lifetimes right, but haven't
> found it. And indeed you have a comment in the context free function to
> the effect that if the switch away form a context fails things will blow
> up.

The reference counting (aside from creation type stuff) should all be
encapsulated in i915_switch_context. There it should add the relevant bo
to the active list as needed, and it should be freed by the existing
infrastructure. The free case is a bit special. I'll look into a proper
fix, but currently the only way it can fail is if the ring buffer is
full, I think - so a few retries should handle all but the absurd.

> 
> I think when you switch the low-level context handling and switching code
> over to just deal with the storage bo you can properly reference count
> that and just keep onto that if the ring still uses it, but the userspace
> handle gets freed. It should work pretty much exactly like framebuffer
> objects and pageflipping (safe for that pageflips run on a different
> asynchronous queue than render stuff, so they're a bit more funky and you
> can't use the active list to help you in the object tracking).
> 
> I think it would even make sense to make the default context just a bo
> associated with the render ring and ditch the context struct around it -
> but I couldn't follow this idea through the convoluted patch series
> structure.

Could you try to explain your first point a bit differently, or maybe we
can discuss it when I see you next week? I don't really understand what
you're asking for.

As for the default context being just a bo associated with the ring, I
really don't see much difference here, if you want it badly, we can do
it, but I think it's nice to keep it mostly not a special case, save for
the context id being special.

> 
> Cheers, Daniel

Thanks for the reviewing.

Re: [PATCH 05/11] drm/i915/context: Preliminary context support

[Ben Widawsky]

On Wed, 15 Feb 2012 12:23:44 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:12 +0100, Ben Widawsky <ben@bwidawsk.net> wrote:
> > From: Ben Widawsky <bwidawsk@gmail.com>
> > 
> > Create all the necessary data structures and hooks to support a context
> > API for userspace. Also plumb in the calls from the i915 core into the
> > context subsystem. There is a new file i915_context.c which will hold a
> > majority of the context related code. This file has nulled functions
> > that get called by the proper part of the i915 driver. This of course
> > requires a Makefile addition, as well as calls to the new nulled
> > functions from the other partsd of the driver (i915_dma.c).
> > 
> > With that, there are 2 new ioctls defined , one for creating a context,
> > and one for destroying a context. These are defined in the necessary
> > places, and again have null functionality
> > 
> > It's likely that a third ioctl will be required to associate the context
> > with an execbuf (likely execbuf3). Since that part is mostly trivial,
> > and will surely be contentious, it's ignored for now.
> 
> > +struct drm_i915_gem_context_create {
> > +	/* output: id of new context*/
> > +	__u32 ctx_id;
> > +
> > +	__u16 pad;
> > +};
> 
> The pad should be made u32 at this point instead of fixing it up in a
> later commit.

Fixed. Thanks.