[PATCH 00/18] i915 HW Context Support

[PATCH 00/18] i915 HW Context Support

[Ben Widawsky]

The patches have changed quite a bit since the RFC, and therefore I
didn't feel comfortable trying to do v2 information. I didn't feel
comfortable taking the few r-bs that I had from the RFC except for the
one patch that I applied wholesale.

Summary:
- Completely redid the patch splitting.
  The number of patches increased, but LOC is about the same, and a
  handful of the new patches are either because of more splitting, or
  completely new bits.
- Reference counted context allows freeing the data structure and
  freeing the BO independently. This is probably the most significant
  change.
- Convert ILK RC6 code to use context code. I'm hopefuly this will make
  things more stable, but have no proof.
- Added trace events for context create/destroy/switch.
- Only support render ring context switch (previous code supported any
  ring, though media ring is the only other ring which *should* work).

Testing summary.
ILK
  RC6, just booted to desktop
SNB
  module load/unload testing
  20 consecutive suspend resume cycles
  nexuiz with experimental mesa
  piglit quick.tests with experimental mesa
    I've seen time-elapsed, and polygonOffset intermittently fail, I
    believe this is caused by the following...
  Missed IRQs now seem to occur once every other piglit run.
    I have some new code to try to fix this... coming soon, I hope.
IVB
  No testing done since RFC

I'll respond to this email with links to what I used to test (code is
currently not in pushable form).
kernel
libdrm
mesa
intel-gpu-tools

Thanks to Chris Wilson, Daniel Vetter, and Eric Anholt for providing
useful feedback in the RFC.

Ben Widawsky (18):
  drm/i915: CXT_SIZE register offsets added
  drm/i915: preliminary context support
  drm/i915: context basic create & destroy
  drm/i915: add context information to objects
  drm/i915: context switch implementation
  drm/i915: trace events for contexts
  drm/i915: Ivybridge MI_ARB_ON_OFF context w/a
  drm/i915: PIPE_CONTROL_TLB_INVALIDATE
  drm/i915: possibly invalidate TLB before context switch
  drm/i915: use the default context
  drm/i915: switch to default context on idle
  drm/i915: try to reset the gpu before unload
  drm/i915/context: create & destroy ioctls
  drm/i915/context: switch contexts with execbuf2
  drm/i915/context: add params
  drm/i915/context: anonymous context interfaces
  drm/i915: Ironlake rc6 can use context interfaces
  drm/i915: try to enable rc6 on Ironlake... again

 drivers/gpu/drm/i915/Makefile              |    1 +
 drivers/gpu/drm/i915/i915_debugfs.c        |    8 +-
 drivers/gpu/drm/i915/i915_dma.c            |    9 +
 drivers/gpu/drm/i915/i915_drv.c            |    1 +
 drivers/gpu/drm/i915/i915_drv.h            |   43 ++-
 drivers/gpu/drm/i915/i915_gem.c            |   11 +
 drivers/gpu/drm/i915/i915_gem_context.c    |  513 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +
 drivers/gpu/drm/i915/i915_reg.h            |   26 ++
 drivers/gpu/drm/i915/i915_trace.h          |   59 ++++
 drivers/gpu/drm/i915/intel_display.c       |   95 +-----
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   49 +++
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +
 include/drm/i915_drm.h                     |   21 +-
 14 files changed, 755 insertions(+), 98 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_gem_context.c

-- 
1.7.9.4

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

[Ben Widawsky]

From: Ben Widawsky <bwidawsk@gmail.com>

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

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

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

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

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

[PATCH 02/18] drm/i915: preliminary context support

[Ben Widawsky]

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

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

Also an important note, as I intend to use contexts for ILK RC6, the
context initialization must always come before RC6 initialization.

As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB where I
decide the HW context is too big. The reason for this is even though
context sizes are increasing with every generation, they are still
measured in pages. If we somehow read back way more than that, it
probably means BIOS has done something strange, or we're running on a
platform that wasn't designed for this.

The 1MB was just a nice round number. I'm open to changing it to
something sensible if someone has a better idea.

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

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ce7fc77..a625d30 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -7,6 +7,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  i915_debugfs.o \
           i915_suspend.o \
 	  i915_gem.o \
+	  i915_gem_context.o \
 	  i915_gem_debug.o \
 	  i915_gem_evict.o \
 	  i915_gem_execbuffer.o \
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9341eb8..4c7c1dc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2155,6 +2155,7 @@ int i915_driver_unload(struct drm_device *dev)
 	ret = i915_gpu_idle(dev, true);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
+	i915_gem_context_unload(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Cancel the retire work handler, which should be idle now. */
@@ -2244,6 +2245,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
 	spin_lock_init(&file_priv->mm.lock);
 	INIT_LIST_HEAD(&file_priv->mm.request_list);
 
+	i915_gem_context_open(dev, file);
+
 	return 0;
 }
 
@@ -2276,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_gem_context_close(dev, file_priv);
 	i915_gem_release(dev, file_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0694e17..b2c56db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -742,6 +742,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
 		if (HAS_BLT(dev))
 		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
 
+		i915_gem_context_load(dev);
 		i915_gem_init_ppgtt(dev);
 
 		mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0f19f5..33c232a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -779,6 +779,9 @@ typedef struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+
+	bool hw_contexts_disabled;
+	uint32_t hw_context_size;
 } drm_i915_private_t;
 
 enum hdmi_force_audio {
@@ -1280,6 +1283,12 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
 
+/* i915_gem_context.c */
+void i915_gem_context_load(struct drm_device *dev);
+void i915_gem_context_unload(struct drm_device *dev);
+void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
+void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
+
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f441f5..6343a82 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3811,6 +3811,7 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	dev_priv->next_seqno = 1;
 
+	i915_gem_context_load(dev);
 	i915_gem_init_ppgtt(dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
new file mode 100644
index 0000000..caa0e06
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -0,0 +1,114 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include "drmP.h"
+#include "i915_drm.h"
+#include "i915_drv.h"
+
+static int get_context_size(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+	u32 reg;
+
+	/* Context size (as of gen7) is determined in number of cache lines */
+	switch (INTEL_INFO(dev)->gen) {
+	case 5: /* ILK & SNB have the same context reg layout */
+	case 6:
+		reg = I915_READ(CXT_SIZE);
+		ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
+		break;
+	case 7:
+		reg = I915_READ(GEN7_CTX_SIZE);
+		ret = GEN7_CTX_TOTAL_SIZE(reg) * 64;
+		break;
+	default:
+		ret = -1;
+	}
+
+	return ret;
+}
+
+/**
+ * The default context needs to exist per ring that uses contexts. It stores the
+ * context state of the GPU for applications that don't utilize HW contexts, as
+ * well as an idle case.
+ */
+static int create_default_context(struct drm_i915_private *dev_priv)
+{
+	return 0;
+}
+
+void i915_gem_context_load(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t ctx_size;
+
+	/* If called from reset, or thaw... we've been here already */
+	if (dev_priv->hw_contexts_disabled)
+		return;
+
+	ctx_size = get_context_size(dev);
+	dev_priv->hw_context_size = get_context_size(dev);
+	dev_priv->hw_context_size = round_up(dev_priv->hw_context_size, 4096);
+
+	if (ctx_size <= 0 || ctx_size > (1<<20)) {
+		dev_priv->hw_contexts_disabled = true;
+		return;
+	}
+
+	if (create_default_context(dev_priv)) {
+		dev_priv->hw_contexts_disabled = true;
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("HW context support initialized\n");
+}
+
+void i915_gem_context_unload(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
+
+void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
+
+void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->hw_contexts_disabled)
+		return;
+}
-- 
1.7.9.4

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

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

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:42PM -0700, Ben Widawsky wrote:
> Very basic code for context setup/destruction in the driver.
> 
> There are 4 entry points into the contexts, load, unload, open, close.
> The names are self-explanatory except that load can be called during
> reset, and also during pm thaw/resume. As we expect our context to be
> preserved across these events, we do not reinitialize in this case.
> 
> Also an important note, as I intend to use contexts for ILK RC6, the
> context initialization must always come before RC6 initialization.
> 
> As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB where I
> decide the HW context is too big. The reason for this is even though
> context sizes are increasing with every generation, they are still
> measured in pages. If we somehow read back way more than that, it
> probably means BIOS has done something strange, or we're running on a
> platform that wasn't designed for this.
> 
> The 1MB was just a nice round number. I'm open to changing it to
> something sensible if someone has a better idea.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

<bikeshed> I see not that much precedence for _load and _unload for
setup/teardown ...

Also this patch is imo way too early in the series - you just add empty
functions so I have no idea what they're doing. And hence can't check
whether you add them at the right place. Whereas if this comes later I
already know what they're doing and can check without applying whether
they're all called at the right place.
</bikeshed>

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/Makefile           |    1 +
>  drivers/gpu/drm/i915/i915_dma.c         |    4 ++
>  drivers/gpu/drm/i915/i915_drv.c         |    1 +
>  drivers/gpu/drm/i915/i915_drv.h         |    9 +++
>  drivers/gpu/drm/i915/i915_gem.c         |    1 +
>  drivers/gpu/drm/i915/i915_gem_context.c |  114 +++++++++++++++++++++++++++++++
>  6 files changed, 130 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_context.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index ce7fc77..a625d30 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -7,6 +7,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  i915_debugfs.o \
>            i915_suspend.o \
>  	  i915_gem.o \
> +	  i915_gem_context.o \
>  	  i915_gem_debug.o \
>  	  i915_gem_evict.o \
>  	  i915_gem_execbuffer.o \
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9341eb8..4c7c1dc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2155,6 +2155,7 @@ int i915_driver_unload(struct drm_device *dev)
>  	ret = i915_gpu_idle(dev, true);
>  	if (ret)
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> +	i915_gem_context_unload(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	/* Cancel the retire work handler, which should be idle now. */
> @@ -2244,6 +2245,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>  	spin_lock_init(&file_priv->mm.lock);
>  	INIT_LIST_HEAD(&file_priv->mm.request_list);
>  
> +	i915_gem_context_open(dev, file);
> +
>  	return 0;
>  }
>  
> @@ -2276,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_gem_context_close(dev, file_priv);
>  	i915_gem_release(dev, file_priv);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0694e17..b2c56db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -742,6 +742,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
>  		if (HAS_BLT(dev))
>  		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
>  
> +		i915_gem_context_load(dev);
>  		i915_gem_init_ppgtt(dev);
>  
>  		mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0f19f5..33c232a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -779,6 +779,9 @@ typedef struct drm_i915_private {
>  
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
> +
> +	bool hw_contexts_disabled;
> +	uint32_t hw_context_size;
>  } drm_i915_private_t;
>  
>  enum hdmi_force_audio {
> @@ -1280,6 +1283,12 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
>  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				    enum i915_cache_level cache_level);
>  
> +/* i915_gem_context.c */
> +void i915_gem_context_load(struct drm_device *dev);
> +void i915_gem_context_unload(struct drm_device *dev);
> +void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> +void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> +
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1f441f5..6343a82 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3811,6 +3811,7 @@ i915_gem_init_hw(struct drm_device *dev)
>  
>  	dev_priv->next_seqno = 1;
>  
> +	i915_gem_context_load(dev);
>  	i915_gem_init_ppgtt(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> new file mode 100644
> index 0000000..caa0e06
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include "drmP.h"
> +#include "i915_drm.h"
> +#include "i915_drv.h"
> +
> +static int get_context_size(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +	u32 reg;
> +
> +	/* Context size (as of gen7) is determined in number of cache lines */
> +	switch (INTEL_INFO(dev)->gen) {
> +	case 5: /* ILK & SNB have the same context reg layout */
> +	case 6:
> +		reg = I915_READ(CXT_SIZE);
> +		ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
> +		break;
> +	case 7:
> +		reg = I915_READ(GEN7_CTX_SIZE);
> +		ret = GEN7_CTX_TOTAL_SIZE(reg) * 64;
> +		break;
> +	default:
> +		ret = -1;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * The default context needs to exist per ring that uses contexts. It stores the
> + * context state of the GPU for applications that don't utilize HW contexts, as
> + * well as an idle case.
> + */
> +static int create_default_context(struct drm_i915_private *dev_priv)
> +{
> +	return 0;
> +}
> +
> +void i915_gem_context_load(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t ctx_size;
> +
> +	/* If called from reset, or thaw... we've been here already */
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +
> +	ctx_size = get_context_size(dev);
> +	dev_priv->hw_context_size = get_context_size(dev);
> +	dev_priv->hw_context_size = round_up(dev_priv->hw_context_size, 4096);
> +
> +	if (ctx_size <= 0 || ctx_size > (1<<20)) {
> +		dev_priv->hw_contexts_disabled = true;
> +		return;
> +	}
> +
> +	if (create_default_context(dev_priv)) {
> +		dev_priv->hw_contexts_disabled = true;
> +		return;
> +	}
> +
> +	DRM_DEBUG_DRIVER("HW context support initialized\n");
> +}
> +
> +void i915_gem_context_unload(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> +
> +void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> +
> +void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ben Widawsky]

On Thu, 29 Mar 2012 00:43:00 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:42PM -0700, Ben Widawsky wrote:
> > Very basic code for context setup/destruction in the driver.
> > 
> > There are 4 entry points into the contexts, load, unload, open,
> > close. The names are self-explanatory except that load can be
> > called during reset, and also during pm thaw/resume. As we expect
> > our context to be preserved across these events, we do not
> > reinitialize in this case.
> > 
> > Also an important note, as I intend to use contexts for ILK RC6, the
> > context initialization must always come before RC6 initialization.
> > 
> > As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB
> > where I decide the HW context is too big. The reason for this is
> > even though context sizes are increasing with every generation,
> > they are still measured in pages. If we somehow read back way more
> > than that, it probably means BIOS has done something strange, or
> > we're running on a platform that wasn't designed for this.
> > 
> > The 1MB was just a nice round number. I'm open to changing it to
> > something sensible if someone has a better idea.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> <bikeshed> I see not that much precedence for _load and _unload for
> setup/teardown ...
> 
> Also this patch is imo way too early in the series - you just add
> empty functions so I have no idea what they're doing. And hence can't
> check whether you add them at the right place. Whereas if this comes
> later I already know what they're doing and can check without
> applying whether they're all called at the right place.
> </bikeshed>

I understand that you get no greater pleasure than bikeshedding my
patches until I want to cry... but seriously with the precedent, it's
in our driver already. So what do you want instead, setup()/teardown()
- init/fini?

Anyway, here is the precedent:
i915_driver_UNLOAD()->i915_gem_context_unload()
i915_driver_LOAD()->i915_gem_LOAD() // which used to call my function
i915_driver_LOAD()->...->i915)gem_context_load()

As for your other comment, I do agree. The point of this patch was to
show where the hooks go to allow people to review them. But without
code, or comments explaining what the code will do - it's pretty
useless. I think this can just be combined with the next patch.

Also, some people may want to review it this way (it is easier for me
to think of this way, for example).

> 
> Cheers, Daniel

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

[Daniel Vetter]

On Wed, Mar 28, 2012 at 03:59:17PM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 00:43:00 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:42PM -0700, Ben Widawsky wrote:
> > > Very basic code for context setup/destruction in the driver.
> > > 
> > > There are 4 entry points into the contexts, load, unload, open,
> > > close. The names are self-explanatory except that load can be
> > > called during reset, and also during pm thaw/resume. As we expect
> > > our context to be preserved across these events, we do not
> > > reinitialize in this case.
> > > 
> > > Also an important note, as I intend to use contexts for ILK RC6, the
> > > context initialization must always come before RC6 initialization.
> > > 
> > > As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB
> > > where I decide the HW context is too big. The reason for this is
> > > even though context sizes are increasing with every generation,
> > > they are still measured in pages. If we somehow read back way more
> > > than that, it probably means BIOS has done something strange, or
> > > we're running on a platform that wasn't designed for this.
> > > 
> > > The 1MB was just a nice round number. I'm open to changing it to
> > > something sensible if someone has a better idea.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > <bikeshed> I see not that much precedence for _load and _unload for
> > setup/teardown ...
> > 
> > Also this patch is imo way too early in the series - you just add
> > empty functions so I have no idea what they're doing. And hence can't
> > check whether you add them at the right place. Whereas if this comes
> > later I already know what they're doing and can check without
> > applying whether they're all called at the right place.
> > </bikeshed>
> 
> I understand that you get no greater pleasure than bikeshedding my
> patches until I want to cry... but seriously with the precedent, it's
> in our driver already. So what do you want instead, setup()/teardown()
> - init/fini?
> 
> Anyway, here is the precedent:
> i915_driver_UNLOAD()->i915_gem_context_unload()
> i915_driver_LOAD()->i915_gem_LOAD() // which used to call my function
> i915_driver_LOAD()->...->i915)gem_context_load()

Well, I've fixed up gem_load, that's now also called init ;-) And
driver_load and _unload are remnants from the stoneage, when these two
functions could essentially only be called a moduel load/unload time. Now
we have hotplug and drm drivers don't use stealth attach any more ...

Anyway, I've seen a few things while reading your patches yesterday that
looked puzzling and strange, but I didn't really know what to do with
them. So I just added some easy bikeshed comments. After a nights worth of
sleep I think I'm seeing clearer, so expect some real feedback soon.

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

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

[Ben Widawsky]

This commit doesn't really match up 1:1 with any of the RFC patches. The
primary difference in the equivalent logic though is the new use of a reference
counter for the context life cycle. This allows us to free the context backing
object, and context kernel memory separately.

For example if a context is destroyed by IOCTL, or by file close, the
context object itself may still be referenced by the GPU. The object
should be properly destroyed by the existing active/inactive list code,
but that occurs sometime in the future. So we can either refcount the
context object and destroy it immediately, or put a hook when we walk
the free list.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 33c232a..e49e2f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -277,6 +277,18 @@ struct i915_hw_ppgtt {
 	dma_addr_t scratch_page_dma_addr;
 };
 
+
+/* This must match up with the value previously used for execbuf2.rsvd1. */
+#define DEFAULT_CONTEXT_ID 0
+struct i915_hw_context {
+	struct drm_i915_file_private *file_priv;
+	struct kref nref;
+
+	int id;
+	struct intel_ring_buffer *ring;
+	struct drm_i915_gem_object *obj;
+};
+
 enum no_fbc_reason {
 	FBC_NO_OUTPUT, /* no outputs enabled to compress */
 	FBC_STOLEN_TOO_SMALL, /* not enough space to hold compressed buffers */
@@ -981,6 +993,8 @@ struct drm_i915_file_private {
 		struct spinlock lock;
 		struct list_head request_list;
 	} mm;
+	struct spinlock context_lock;
+	struct idr context_idr;
 };
 
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index caa0e06..2c9116d 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -29,6 +29,15 @@
 #include "i915_drm.h"
 #include "i915_drv.h"
 
+/* This is a HW constraint. The value below is the largest known requirement
+ * I've seen in a spec to date, and that was a workaround for a non-shipping
+ * part. It should be safe to decrease this, but it's more future proof as is.
+ */
+#define CONTEXT_ALIGN (64<<10)
+
+static struct i915_hw_context *
+i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
+
 static int get_context_size(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -53,6 +62,91 @@ static int get_context_size(struct drm_device *dev)
 	return ret;
 }
 
+static void do_destroy(struct i915_hw_context *ctx)
+{
+	struct drm_device *dev = ctx->obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (ctx->file_priv)
+		idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	else
+		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
+
+	drm_gem_object_unreference(&ctx->obj->base);
+	kfree(ctx);
+}
+
+static int
+create_hw_context(struct drm_device *dev,
+		  struct drm_i915_file_private *file_priv,
+		  struct i915_hw_context **ctx_out)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret, id;
+
+	*ctx_out = kzalloc(sizeof(struct drm_i915_file_private), GFP_KERNEL);
+	if (*ctx_out == NULL)
+		return -ENOMEM;
+
+	(*ctx_out)->obj = i915_gem_alloc_object(dev,
+						dev_priv->hw_context_size);
+	if ((*ctx_out)->obj == NULL) {
+		kfree(*ctx_out);
+		return -ENOMEM;
+	}
+
+	kref_init(&(*ctx_out)->nref);
+
+	/* The ring associated with the context object is handled by the normal
+	 * object tracking code. We give an initial ring value simple to pass an
+	 * assertion in the context switch code.
+	 */
+	(*ctx_out)->ring = &dev_priv->ring[RCS];
+
+	/* Default context will never have a file_priv */
+	if (file_priv == NULL)
+		return 0;
+
+	(*ctx_out)->file_priv = file_priv;
+
+again:
+	if (idr_pre_get(&file_priv->context_idr, GFP_KERNEL) == 0) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	spin_lock(&file_priv->context_lock);
+	ret = idr_get_new_above(&file_priv->context_idr, *ctx_out,
+				DEFAULT_CONTEXT_ID + 1, &id);
+	if (ret == 0)
+		(*ctx_out)->id = id;
+	spin_unlock(&file_priv->context_lock);
+
+	if (ret == -EAGAIN) {
+		goto again;
+	} else if (ret)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	do_destroy(*ctx_out);
+	return ret;
+}
+
+static void destroy_hw_context(struct kref *kref)
+{
+	struct i915_hw_context *ctx = container_of(kref, struct i915_hw_context,
+						   nref);
+
+	/* The backing object for the context may still be in use if the context
+	 * switch away hasn't yet occurred. So the freeing of the object is
+	 * asynchronously (and asymmetrically) handled by the normal i915
+	 * buffer life cycle.
+	 */
+	do_destroy(ctx);
+}
+
 /**
  * The default context needs to exist per ring that uses contexts. It stores the
  * context state of the GPU for applications that don't utilize HW contexts, as
@@ -60,7 +154,28 @@ static int get_context_size(struct drm_device *dev)
  */
 static int create_default_context(struct drm_i915_private *dev_priv)
 {
-	return 0;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	ret = create_hw_context(dev_priv->dev, NULL,
+				&dev_priv->ring[RCS].default_context);
+	if (ret)
+		return ret;
+
+	/* We may need to do things with the shrinker which require us to
+	 * immediately switch back to the default context. This can cause a
+	 * problem as pinning the default context also requires GTT space which
+	 * may not be available. To avoid this we always pin the
+	 * default context.
+	 */
+	ctx = dev_priv->ring[RCS].default_context;
+	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
+	if (ret)
+		do_destroy(ctx);
+
+	return ret;
 }
 
 void i915_gem_context_load(struct drm_device *dev)
@@ -69,7 +184,8 @@ void i915_gem_context_load(struct drm_device *dev)
 	uint32_t ctx_size;
 
 	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled)
+	if (dev_priv->hw_contexts_disabled ||
+	    dev_priv->ring[RCS].default_context)
 		return;
 
 	ctx_size = get_context_size(dev);
@@ -95,20 +211,68 @@ void i915_gem_context_unload(struct drm_device *dev)
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
+
+	BUG_ON(kref_put(&dev_priv->ring[RCS].default_context->nref,
+		        destroy_hw_context) == 0);
 }
 
 void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	idr_init(&file_priv->context_idr);
+	spin_lock_init(&file_priv->context_lock);
+}
+
+static int context_idr_cleanup(int id, void *p, void *data)
+{
+	struct drm_file *file = (struct drm_file *) data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_hw_context *ctx;
+
+	BUG_ON(id == DEFAULT_CONTEXT_ID);
+	ctx = i915_gem_context_get(file_priv, id);
+	BUG_ON(ctx == NULL);
+	kref_put(&ctx->nref, destroy_hw_context);
+	kref_put(&ctx->nref, destroy_hw_context);
+
+	return 0;
 }
 
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
 
 	if (dev_priv->hw_contexts_disabled)
 		return;
+
+	mutex_lock(&dev->struct_mutex);
+	idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
+	idr_destroy(&file_priv->context_idr);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static __used struct i915_hw_context *
+i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
+{
+	struct i915_hw_context *ctx = NULL;
+
+	/* kref_get must be synchronized with destroy, and file close. If not we
+	 * could find the ctx, and before getting the reference it may be
+	 * destroyed.
+	 */
+	spin_lock(&file_priv->context_lock);
+	ctx = idr_find(&file_priv->context_idr, id);
+	BUG_ON(!mutex_is_locked(&ctx->ring->dev->struct_mutex));
+	kref_get(&ctx->nref);
+	spin_unlock(&file_priv->context_lock);
+
+	return ctx;
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index bc0365b..8c9f898 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -120,6 +120,8 @@ struct  intel_ring_buffer {
 	wait_queue_head_t irq_queue;
 	drm_local_map_t map;
 
+	struct i915_hw_context *default_context;
+
 	void *private;
 };
 
-- 
1.7.9.4

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

[Ben Widawsky]

Handy mostly for assertions.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e49e2f7..f458a8f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -953,6 +953,11 @@ struct drm_i915_gem_object {
 	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
 	 */
 	atomic_t pending_flip;
+
+	/**
+	 * >= 0 if this object is the object for a context.
+	 */
+	int context_id;
 };
 
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6343a82..0985aa5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3629,6 +3629,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	obj->madv = I915_MADV_WILLNEED;
 	/* Avoid an unnecessary call to unbind on the first bind. */
 	obj->map_and_fenceable = true;
+	obj->context_id = -1;
 
 	return obj;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2c9116d..321bafd 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -127,6 +127,7 @@ again:
 	} else if (ret)
 		goto err_out;
 
+	(*ctx_out)->obj->context_id = (*ctx_out)->id;
 	return 0;
 
 err_out:
@@ -171,6 +172,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	 * default context.
 	 */
 	ctx = dev_priv->ring[RCS].default_context;
+	ctx->id = DEFAULT_CONTEXT_ID;
+	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
 	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
 	if (ret)
 		do_destroy(ctx);
-- 
1.7.9.4

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

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> Handy mostly for assertions.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I don't see the point of carrying around a obj->context_id - context_id's
are file_priv, so they're not that useful in the tracepoint. I suggest you
just drop the contex_id arg there - the obj pointers are global and imo
good enough for identification. And the few BUG_ON's don't seem really
useful either.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
>  drivers/gpu/drm/i915/i915_gem.c         |    1 +
>  drivers/gpu/drm/i915/i915_gem_context.c |    3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e49e2f7..f458a8f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -953,6 +953,11 @@ struct drm_i915_gem_object {
>  	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
>  	 */
>  	atomic_t pending_flip;
> +
> +	/**
> +	 * >= 0 if this object is the object for a context.
> +	 */
> +	int context_id;
>  };
>  
>  #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6343a82..0985aa5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3629,6 +3629,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  	obj->madv = I915_MADV_WILLNEED;
>  	/* Avoid an unnecessary call to unbind on the first bind. */
>  	obj->map_and_fenceable = true;
> +	obj->context_id = -1;
>  
>  	return obj;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2c9116d..321bafd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -127,6 +127,7 @@ again:
>  	} else if (ret)
>  		goto err_out;
>  
> +	(*ctx_out)->obj->context_id = (*ctx_out)->id;
>  	return 0;
>  
>  err_out:
> @@ -171,6 +172,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  	 * default context.
>  	 */
>  	ctx = dev_priv->ring[RCS].default_context;
> +	ctx->id = DEFAULT_CONTEXT_ID;
> +	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
>  	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
>  	if (ret)
>  		do_destroy(ctx);
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ben Widawsky]

On Thu, 29 Mar 2012 00:36:21 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> > Handy mostly for assertions.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I don't see the point of carrying around a obj->context_id -
> context_id's are file_priv, so they're not that useful in the
> tracepoint. I suggest you just drop the contex_id arg there - the obj
> pointers are global and imo good enough for identification. And the
> few BUG_ON's don't seem really useful either.
> -Daniel

obj->context_id was requested by Chris to clarify the trace events. I
vaguely recall telling him that you won't like it.

I personally dislike using object pointer though I do agree it serves
the same purpose.

The other nice thing about having the context id is it makes it easy in
debug situations to find out if a certain object is a context object.
But no use case for that yet.

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

[Daniel Vetter]

On Wed, Mar 28, 2012 at 05:20:11PM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 00:36:21 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> > > Handy mostly for assertions.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > I don't see the point of carrying around a obj->context_id -
> > context_id's are file_priv, so they're not that useful in the
> > tracepoint. I suggest you just drop the contex_id arg there - the obj
> > pointers are global and imo good enough for identification. And the
> > few BUG_ON's don't seem really useful either.
> > -Daniel
> 
> obj->context_id was requested by Chris to clarify the trace events. I
> vaguely recall telling him that you won't like it.

That's easily shot down on the grounds that:
- we currently don't dump the id/handles of gem objects into our traces
- your tracepoints dump the context_id at create/destroy time, so with a
  bit of python this can be re-added in userspace.

> I personally dislike using object pointer though I do agree it serves
> the same purpose.
> 
> The other nice thing about having the context id is it makes it easy in
> debug situations to find out if a certain object is a context object.
> But no use case for that yet.

My gripes with obj->context_id is that it smells like a layering
violation. We don't have such special stuff for other special things like
rings. The only exception is framebuffer when pageflipping, but I expect
that we'll need to clean this up sooner or later (because we need to be
able to move framebuffers sooner or later anyway, so they need better
integration with the gem eviction code).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

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

[Ben Widawsky]

On Thu, 29 Mar 2012 10:47:56 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Mar 28, 2012 at 05:20:11PM -0700, Ben Widawsky wrote:
> > On Thu, 29 Mar 2012 00:36:21 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> > > > Handy mostly for assertions.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > 
> > > I don't see the point of carrying around a obj->context_id -
> > > context_id's are file_priv, so they're not that useful in the
> > > tracepoint. I suggest you just drop the contex_id arg there - the
> > > obj pointers are global and imo good enough for identification.
> > > And the few BUG_ON's don't seem really useful either.
> > > -Daniel
> > 
> > obj->context_id was requested by Chris to clarify the trace events.
> > I vaguely recall telling him that you won't like it.
> 
> That's easily shot down on the grounds that:
> - we currently don't dump the id/handles of gem objects into our
> traces
> - your tracepoints dump the context_id at create/destroy time, so
> with a bit of python this can be re-added in userspace.

Yes, but gem handles are not specific to i915, whereas context id
numbers are.

Also the tracepoint that is a problem is switch, not create/destroy

> 
> > I personally dislike using object pointer though I do agree it
> > serves the same purpose.
> > 
> > The other nice thing about having the context id is it makes it
> > easy in debug situations to find out if a certain object is a
> > context object. But no use case for that yet.
> 
> My gripes with obj->context_id is that it smells like a layering
> violation. We don't have such special stuff for other special things
> like rings. The only exception is framebuffer when pageflipping, but
> I expect that we'll need to clean this up sooner or later (because we
> need to be able to move framebuffers sooner or later anyway, so they
> need better integration with the gem eviction code).
> -Daniel

I still think the pros (easy debug) outweigh the cons (not really on
exactly what you don't like). I don't see any harm that can come from
this. /me braces for harm

Anyway, as you said in the other email, I'm expecting more serious
feedback. If I end up redoing a bunch of stuff, this can go too if you
really don't like it.

[PATCH 05/18] drm/i915: context switch implementation

[Ben Widawsky]

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f458a8f..c6c2ada 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1303,10 +1303,15 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
 
 /* i915_gem_context.c */
+#define I915_CONTEXT_FORCED_SWITCH (1<<0)
+#define I915_CONTEXT_INITIAL_SWITCH (1<<1)
 void i915_gem_context_load(struct drm_device *dev);
 void i915_gem_context_unload(struct drm_device *dev);
 void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
+int i915_switch_context(struct intel_ring_buffer *ring,
+			struct drm_file *file,
+			int to_id, u32 seqno, u32 flags);
 
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 321bafd..cc508d5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -262,7 +262,7 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static __used struct i915_hw_context *
+static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
 	struct i915_hw_context *ctx = NULL;
@@ -279,3 +279,119 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 
 	return ctx;
 }
+
+static int do_switch(struct drm_i915_gem_object *from_obj,
+		     struct i915_hw_context *to,
+		     u32 seqno, u32 flags)
+{
+	bool initial_switch = (flags & I915_CONTEXT_INITIAL_SWITCH) ? true : false;
+	bool force =  (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
+	struct intel_ring_buffer *ring = NULL;
+	u32 hw_flags = 0;
+	int ret;
+
+	BUG_ON(to == NULL);
+	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
+	BUG_ON((from_obj != NULL && from_obj->context_id < 0) || to->obj->context_id < 0);
+
+	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
+	if (ret)
+		return ret;
+
+	if (initial_switch)
+		hw_flags |= MI_RESTORE_INHIBIT;
+	if (force)
+		hw_flags |= MI_FORCE_RESTORE;
+
+	ring = to->ring;
+	ret = intel_ring_mi_set_context(ring, to, hw_flags);
+	if (ret) {
+		i915_gem_object_unpin(to->obj);
+		return ret;
+	}
+
+	/* The backing object for the context is done after switching to the
+	 * *next* context. Therefore we cannot retire the previous context until
+	 * the next context has already started running. In fact, the below code
+	 * is a bit suboptimal because the retiring can occur simply after the
+	 * MI_SET_CONTEXT instead of when the next seqno has completed.
+	 */
+	if (from_obj != NULL) {
+		i915_gem_object_move_to_active(from_obj, ring, seqno);
+		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
+		 * whole damn pipeline, we don't need to explicitly mark the
+		 * object dirty. It should be safe to evict the object at any
+		 * point after MI_SET_CONTEXT has finished executing... true as
+		 * of GEN7. If not from_obj->dirty=1 would make this safer.
+		 */
+		BUG_ON(from_obj->ring != to->ring);
+	}
+
+	if (from_obj)
+		i915_gem_object_unpin(from_obj);
+
+	ring->last_context_obj = to->obj;
+
+	return 0;
+}
+
+/**
+ * i915_switch_context() - perform a GPU context switch.
+ * @ring: ring for which we'll execute the context switch
+ * @file_priv: file_priv associated with the context, may be NULL
+ * @id: context id number
+ * @seqno: sequence number by which the new context will be switched to
+ * @flags:
+ *
+ * This function will perform the context switch to the given context id on the
+ * specified ring. Unfortunately the context switch code doesn't have an
+ * independent way of knowing when the context switch has occurred, so the
+ * caller must notify of us a time by which the context switch has occurred.
+ */
+int i915_switch_context(struct intel_ring_buffer *ring,
+			struct drm_file *file,
+			int to_id, u32 seqno, u32 flags)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_file_private *file_priv = NULL;
+	struct i915_hw_context *to;
+	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
+	int ret;
+
+	if (dev_priv->hw_contexts_disabled)
+		return 0;
+
+	if (ring != &dev_priv->ring[RCS])
+		return 0;
+
+	if (file)
+		file_priv = file->driver_priv;
+
+	if (to_id == DEFAULT_CONTEXT_ID) {
+		to = ring->default_context;
+		kref_get(&to->nref);
+	} else {
+		to = i915_gem_context_get(file_priv, to_id);
+		if (to == NULL)
+			return -EINVAL;
+	}
+	drm_gem_object_reference(&to->obj->base);
+
+	/* If the object is still on the active list, we can shortcut */
+	if (from_obj == to->obj) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = do_switch(from_obj, to, seqno, flags);
+	if (ret) {
+		drm_gem_object_unreference(&to->obj->base);
+		goto out;
+	}
+
+out:
+	if (from_obj != NULL)
+		drm_gem_object_unreference(&from_obj->base);
+	kref_put(&to->nref, destroy_hw_context);
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ca3972f..cd74f86 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -920,6 +920,32 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 	return 0;
 }
 
+int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
+			      struct i915_hw_context *new_context,
+			      u32 hw_flags)
+{
+	int ret;
+
+	ret = intel_ring_begin(ring, 4);
+	if (ret)
+		return ret;
+
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_emit(ring, MI_SET_CONTEXT);
+	intel_ring_emit(ring, new_context->obj->gtt_offset |
+			MI_MM_SPACE_GTT |
+			MI_SAVE_EXT_STATE_EN |
+			MI_RESTORE_EXT_STATE_EN |
+			hw_flags);
+	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
+	intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+
+	return ret;
+
+}
+
 static void cleanup_status_page(struct intel_ring_buffer *ring)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 8c9f898..0ed98bb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -121,6 +121,7 @@ struct  intel_ring_buffer {
 	drm_local_map_t map;
 
 	struct i915_hw_context *default_context;
+	struct drm_i915_gem_object *last_context_obj;
 
 	void *private;
 };
@@ -216,6 +217,10 @@ static inline void i915_trace_irq_get(struct intel_ring_buffer *ring, u32 seqno)
 		ring->trace_irq_seqno = seqno;
 }
 
+int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
+			      struct i915_hw_context *new_context,
+			      u32 hw_flags);
+
 /* DRI warts */
 int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
 
-- 
1.7.9.4

Re: [PATCH 05/18] drm/i915: context switch implementation

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote:
> Implement the context switch code as well as the interfaces to do the
> context switch. This patch also doesn't match 1:1 with the RFC patches.
> The main difference is that from Daniel's responses the last context
> object is now stored instead of the last context. This aids in allows us
> to free the context data structure, and context object independently.
> 
> There is room for optimization: this code will pin the context object
> until the next context is active. The optimal way to do it is to
> actually pin the object, move it to the active list, do the context
> switch, and then unpin it. This allows the eviction code to actually
> evict the context object if needed.
> 
> The context switch code is missing workarounds, they will be implemented
> in future patches.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Ok, I've looked at the use-sites of context_get and all this refcounting
and noticed that:
- we always hold dev->struct_mutex
- we always drop the acquired reference to the context structure in the
  same function without dropping struct_mutex in between.

So we don't seem to require any reference counting on these (and
additional locking on the idr). Additionally the idr locking seems to give
us a false sense of security because afaics the locking/refcounting would
be broken when we would _not_ hold struct_mutex.

So can we just rip this out or do we need this (in which case it needs
some more work imo)?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 ++
>  drivers/gpu/drm/i915/i915_gem_context.c |  118 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   26 +++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    5 ++
>  4 files changed, 153 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f458a8f..c6c2ada 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1303,10 +1303,15 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				    enum i915_cache_level cache_level);
>  
>  /* i915_gem_context.c */
> +#define I915_CONTEXT_FORCED_SWITCH (1<<0)
> +#define I915_CONTEXT_INITIAL_SWITCH (1<<1)
>  void i915_gem_context_load(struct drm_device *dev);
>  void i915_gem_context_unload(struct drm_device *dev);
>  void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> +int i915_switch_context(struct intel_ring_buffer *ring,
> +			struct drm_file *file,
> +			int to_id, u32 seqno, u32 flags);
>  
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 321bafd..cc508d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -262,7 +262,7 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -static __used struct i915_hw_context *
> +static struct i915_hw_context *
>  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  {
>  	struct i915_hw_context *ctx = NULL;
> @@ -279,3 +279,119 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  
>  	return ctx;
>  }
> +
> +static int do_switch(struct drm_i915_gem_object *from_obj,
> +		     struct i915_hw_context *to,
> +		     u32 seqno, u32 flags)
> +{
> +	bool initial_switch = (flags & I915_CONTEXT_INITIAL_SWITCH) ? true : false;
> +	bool force =  (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
> +	struct intel_ring_buffer *ring = NULL;
> +	u32 hw_flags = 0;
> +	int ret;
> +
> +	BUG_ON(to == NULL);
> +	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
> +	BUG_ON((from_obj != NULL && from_obj->context_id < 0) || to->obj->context_id < 0);
> +
> +	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
> +	if (ret)
> +		return ret;
> +
> +	if (initial_switch)
> +		hw_flags |= MI_RESTORE_INHIBIT;
> +	if (force)
> +		hw_flags |= MI_FORCE_RESTORE;
> +
> +	ring = to->ring;
> +	ret = intel_ring_mi_set_context(ring, to, hw_flags);
> +	if (ret) {
> +		i915_gem_object_unpin(to->obj);
> +		return ret;
> +	}
> +
> +	/* The backing object for the context is done after switching to the
> +	 * *next* context. Therefore we cannot retire the previous context until
> +	 * the next context has already started running. In fact, the below code
> +	 * is a bit suboptimal because the retiring can occur simply after the
> +	 * MI_SET_CONTEXT instead of when the next seqno has completed.
> +	 */
> +	if (from_obj != NULL) {
> +		i915_gem_object_move_to_active(from_obj, ring, seqno);
> +		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> +		 * whole damn pipeline, we don't need to explicitly mark the
> +		 * object dirty. It should be safe to evict the object at any
> +		 * point after MI_SET_CONTEXT has finished executing... true as
> +		 * of GEN7. If not from_obj->dirty=1 would make this safer.
> +		 */
> +		BUG_ON(from_obj->ring != to->ring);
> +	}
> +
> +	if (from_obj)
> +		i915_gem_object_unpin(from_obj);
> +
> +	ring->last_context_obj = to->obj;
> +
> +	return 0;
> +}
> +
> +/**
> + * i915_switch_context() - perform a GPU context switch.
> + * @ring: ring for which we'll execute the context switch
> + * @file_priv: file_priv associated with the context, may be NULL
> + * @id: context id number
> + * @seqno: sequence number by which the new context will be switched to
> + * @flags:
> + *
> + * This function will perform the context switch to the given context id on the
> + * specified ring. Unfortunately the context switch code doesn't have an
> + * independent way of knowing when the context switch has occurred, so the
> + * caller must notify of us a time by which the context switch has occurred.
> + */
> +int i915_switch_context(struct intel_ring_buffer *ring,
> +			struct drm_file *file,
> +			int to_id, u32 seqno, u32 flags)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_file_private *file_priv = NULL;
> +	struct i915_hw_context *to;
> +	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
> +	int ret;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return 0;
> +
> +	if (ring != &dev_priv->ring[RCS])
> +		return 0;
> +
> +	if (file)
> +		file_priv = file->driver_priv;
> +
> +	if (to_id == DEFAULT_CONTEXT_ID) {
> +		to = ring->default_context;
> +		kref_get(&to->nref);
> +	} else {
> +		to = i915_gem_context_get(file_priv, to_id);
> +		if (to == NULL)
> +			return -EINVAL;
> +	}
> +	drm_gem_object_reference(&to->obj->base);
> +
> +	/* If the object is still on the active list, we can shortcut */
> +	if (from_obj == to->obj) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = do_switch(from_obj, to, seqno, flags);
> +	if (ret) {
> +		drm_gem_object_unreference(&to->obj->base);
> +		goto out;
> +	}
> +
> +out:
> +	if (from_obj != NULL)
> +		drm_gem_object_unreference(&from_obj->base);
> +	kref_put(&to->nref, destroy_hw_context);
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ca3972f..cd74f86 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -920,6 +920,32 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
>  	return 0;
>  }
>  
> +int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
> +			      struct i915_hw_context *new_context,
> +			      u32 hw_flags)
> +{
> +	int ret;
> +
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret)
> +		return ret;
> +
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_SET_CONTEXT);
> +	intel_ring_emit(ring, new_context->obj->gtt_offset |
> +			MI_MM_SPACE_GTT |
> +			MI_SAVE_EXT_STATE_EN |
> +			MI_RESTORE_EXT_STATE_EN |
> +			hw_flags);
> +	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
> +	intel_ring_emit(ring, MI_NOOP);
> +
> +	intel_ring_advance(ring);
> +
> +	return ret;
> +
> +}
> +
>  static void cleanup_status_page(struct intel_ring_buffer *ring)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8c9f898..0ed98bb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -121,6 +121,7 @@ struct  intel_ring_buffer {
>  	drm_local_map_t map;
>  
>  	struct i915_hw_context *default_context;
> +	struct drm_i915_gem_object *last_context_obj;
>  
>  	void *private;
>  };
> @@ -216,6 +217,10 @@ static inline void i915_trace_irq_get(struct intel_ring_buffer *ring, u32 seqno)
>  		ring->trace_irq_seqno = seqno;
>  }
>  
> +int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
> +			      struct i915_hw_context *new_context,
> +			      u32 hw_flags);
> +
>  /* DRI warts */
>  int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
>  
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 05/18] drm/i915: context switch implementation

[Ben Widawsky]

On Thu, 29 Mar 2012 20:24:11 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote:
> > Implement the context switch code as well as the interfaces to do the
> > context switch. This patch also doesn't match 1:1 with the RFC patches.
> > The main difference is that from Daniel's responses the last context
> > object is now stored instead of the last context. This aids in allows us
> > to free the context data structure, and context object independently.
> > 
> > There is room for optimization: this code will pin the context object
> > until the next context is active. The optimal way to do it is to
> > actually pin the object, move it to the active list, do the context
> > switch, and then unpin it. This allows the eviction code to actually
> > evict the context object if needed.
> > 
> > The context switch code is missing workarounds, they will be implemented
> > in future patches.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Ok, I've looked at the use-sites of context_get and all this refcounting
> and noticed that:
> - we always hold dev->struct_mutex
> - we always drop the acquired reference to the context structure in the
>   same function without dropping struct_mutex in between.
> 
> So we don't seem to require any reference counting on these (and
> additional locking on the idr). Additionally the idr locking seems to give
> us a false sense of security because afaics the locking/refcounting would
> be broken when we would _not_ hold struct_mutex.
> 
> So can we just rip this out or do we need this (in which case it needs
> some more work imo)?
> -Daniel

I think it can be ripped out. I was on the fence about this before
submitting the patches and left it out of laziness; it doesn't hurt as
there is no lock contention assuming your statement is true with no
bugs in the code, and it follows the canonical use of idrs.

Let me look it over some more to make sure after you finish reviewing
the other stuff. The idea was supposed to be contexts can be created
and looked up without struct mutex, but that isn't actually done in the
code.

Re: [PATCH 05/18] drm/i915: context switch implementation

[Daniel Vetter]

On Thu, Mar 29, 2012 at 11:43:12AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 20:24:11 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote:
> > > Implement the context switch code as well as the interfaces to do the
> > > context switch. This patch also doesn't match 1:1 with the RFC patches.
> > > The main difference is that from Daniel's responses the last context
> > > object is now stored instead of the last context. This aids in allows us
> > > to free the context data structure, and context object independently.
> > > 
> > > There is room for optimization: this code will pin the context object
> > > until the next context is active. The optimal way to do it is to
> > > actually pin the object, move it to the active list, do the context
> > > switch, and then unpin it. This allows the eviction code to actually
> > > evict the context object if needed.
> > > 
> > > The context switch code is missing workarounds, they will be implemented
> > > in future patches.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Ok, I've looked at the use-sites of context_get and all this refcounting
> > and noticed that:
> > - we always hold dev->struct_mutex
> > - we always drop the acquired reference to the context structure in the
> >   same function without dropping struct_mutex in between.
> > 
> > So we don't seem to require any reference counting on these (and
> > additional locking on the idr). Additionally the idr locking seems to give
> > us a false sense of security because afaics the locking/refcounting would
> > be broken when we would _not_ hold struct_mutex.
> > 
> > So can we just rip this out or do we need this (in which case it needs
> > some more work imo)?
> > -Daniel
> 
> I think it can be ripped out. I was on the fence about this before
> submitting the patches and left it out of laziness; it doesn't hurt as
> there is no lock contention assuming your statement is true with no
> bugs in the code, and it follows the canonical use of idrs.
> 
> Let me look it over some more to make sure after you finish reviewing
> the other stuff. The idea was supposed to be contexts can be created
> and looked up without struct mutex, but that isn't actually done in the
> code.

Well, if you want to leave it I have to add some more review comments
about it - atm I think it would be buggy and racy without holding
struct_mutex ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 05/18] drm/i915: context switch implementation

[Ben Widawsky]

On Thu, 29 Mar 2012 20:49:00 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Mar 29, 2012 at 11:43:12AM -0700, Ben Widawsky wrote:
> > On Thu, 29 Mar 2012 20:24:11 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote:
> > > > Implement the context switch code as well as the interfaces to do the
> > > > context switch. This patch also doesn't match 1:1 with the RFC patches.
> > > > The main difference is that from Daniel's responses the last context
> > > > object is now stored instead of the last context. This aids in allows us
> > > > to free the context data structure, and context object independently.
> > > > 
> > > > There is room for optimization: this code will pin the context object
> > > > until the next context is active. The optimal way to do it is to
> > > > actually pin the object, move it to the active list, do the context
> > > > switch, and then unpin it. This allows the eviction code to actually
> > > > evict the context object if needed.
> > > > 
> > > > The context switch code is missing workarounds, they will be implemented
> > > > in future patches.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > 
> > > Ok, I've looked at the use-sites of context_get and all this refcounting
> > > and noticed that:
> > > - we always hold dev->struct_mutex
> > > - we always drop the acquired reference to the context structure in the
> > >   same function without dropping struct_mutex in between.
> > > 
> > > So we don't seem to require any reference counting on these (and
> > > additional locking on the idr). Additionally the idr locking seems to give
> > > us a false sense of security because afaics the locking/refcounting would
> > > be broken when we would _not_ hold struct_mutex.
> > > 
> > > So can we just rip this out or do we need this (in which case it needs
> > > some more work imo)?
> > > -Daniel
> > 
> > I think it can be ripped out. I was on the fence about this before
> > submitting the patches and left it out of laziness; it doesn't hurt as
> > there is no lock contention assuming your statement is true with no
> > bugs in the code, and it follows the canonical use of idrs.
> > 
> > Let me look it over some more to make sure after you finish reviewing
> > the other stuff. The idea was supposed to be contexts can be created
> > and looked up without struct mutex, but that isn't actually done in the
> > code.
> 
> Well, if you want to leave it I have to add some more review comments
> about it - atm I think it would be buggy and racy without holding
> struct_mutex ...
> -Daniel

As I said before, I'll either find a good use for it, or remove it.
Context creation is the only currently viable case for it- but probably
not worth the extra lock.

Re: [PATCH 05/18] drm/i915: context switch implementation

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote:
> Implement the context switch code as well as the interfaces to do the
> context switch. This patch also doesn't match 1:1 with the RFC patches.
> The main difference is that from Daniel's responses the last context
> object is now stored instead of the last context. This aids in allows us
> to free the context data structure, and context object independently.
> 
> There is room for optimization: this code will pin the context object
> until the next context is active. The optimal way to do it is to
> actually pin the object, move it to the active list, do the context
> switch, and then unpin it. This allows the eviction code to actually
> evict the context object if needed.
> 
> The context switch code is missing workarounds, they will be implemented
> in future patches.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Meh, I've forgotten half of the review, some more comments below.

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 ++
>  drivers/gpu/drm/i915/i915_gem_context.c |  118 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   26 +++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    5 ++
>  4 files changed, 153 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f458a8f..c6c2ada 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1303,10 +1303,15 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				    enum i915_cache_level cache_level);
>  
>  /* i915_gem_context.c */
> +#define I915_CONTEXT_FORCED_SWITCH (1<<0)
> +#define I915_CONTEXT_INITIAL_SWITCH (1<<1)
>  void i915_gem_context_load(struct drm_device *dev);
>  void i915_gem_context_unload(struct drm_device *dev);
>  void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> +int i915_switch_context(struct intel_ring_buffer *ring,
> +			struct drm_file *file,
> +			int to_id, u32 seqno, u32 flags);
>  
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 321bafd..cc508d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -262,7 +262,7 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -static __used struct i915_hw_context *
> +static struct i915_hw_context *
>  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  {
>  	struct i915_hw_context *ctx = NULL;
> @@ -279,3 +279,119 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  
>  	return ctx;
>  }
> +
> +static int do_switch(struct drm_i915_gem_object *from_obj,
> +		     struct i915_hw_context *to,
> +		     u32 seqno, u32 flags)
> +{
> +	bool initial_switch = (flags & I915_CONTEXT_INITIAL_SWITCH) ? true : false;
> +	bool force =  (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;

I don't like this flag passing. force seems to be totally unused. And imo
inital_switch would work much better if we track this in
i915_hw_context->intialized (which will be false on context create) and
check this when before doing the context switch.

That way you'd also lose the complexity of having to initialize a context
explicitly after create - it would just magically happen.

> +	struct intel_ring_buffer *ring = NULL;
> +	u32 hw_flags = 0;
> +	int ret;
> +
> +	BUG_ON(to == NULL);
> +	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
> +	BUG_ON((from_obj != NULL && from_obj->context_id < 0) || to->obj->context_id < 0);
> +
> +	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
> +	if (ret)
> +		return ret;
> +
> +	if (initial_switch)
> +		hw_flags |= MI_RESTORE_INHIBIT;
> +	if (force)
> +		hw_flags |= MI_FORCE_RESTORE;

Please move this into the function that actually emits the MI_ command -
it's imo hard to read things if it's distributed all over the place. And
because we don't seem to need force, a simple (and destriptive) bool
inhibit_restore would be sufficient.

> +
> +	ring = to->ring;
> +	ret = intel_ring_mi_set_context(ring, to, hw_flags);
> +	if (ret) {
> +		i915_gem_object_unpin(to->obj);
> +		return ret;
> +	}
> +
> +	/* The backing object for the context is done after switching to the
> +	 * *next* context. Therefore we cannot retire the previous context until
> +	 * the next context has already started running. In fact, the below code
> +	 * is a bit suboptimal because the retiring can occur simply after the
> +	 * MI_SET_CONTEXT instead of when the next seqno has completed.
> +	 */
> +	if (from_obj != NULL) {
> +		i915_gem_object_move_to_active(from_obj, ring, seqno);

You can use outstanding lazy request here to ditch the seqno argument from
do_switch and i915_switch_context.

> +		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> +		 * whole damn pipeline, we don't need to explicitly mark the
> +		 * object dirty. It should be safe to evict the object at any
> +		 * point after MI_SET_CONTEXT has finished executing... true as
> +		 * of GEN7. If not from_obj->dirty=1 would make this safer.
> +		 */

Actually you _need_ to set to->obj->dirty = 1, otherwise we don't tell
shmem that we've dirtied the pages backing the context gem_bo at unbind
time.

> +		BUG_ON(from_obj->ring != to->ring);
> +	}
> +
> +	if (from_obj)
> +		i915_gem_object_unpin(from_obj);
> +
> +	ring->last_context_obj = to->obj;
> +
> +	return 0;
> +}
> +
> +/**
> + * i915_switch_context() - perform a GPU context switch.
> + * @ring: ring for which we'll execute the context switch
> + * @file_priv: file_priv associated with the context, may be NULL
> + * @id: context id number
> + * @seqno: sequence number by which the new context will be switched to
> + * @flags:
> + *
> + * This function will perform the context switch to the given context id on the
> + * specified ring. Unfortunately the context switch code doesn't have an
> + * independent way of knowing when the context switch has occurred, so the
> + * caller must notify of us a time by which the context switch has occurred.
> + */

It took me a while to figure out that you mean the caller has to pass in
the retiring seqno. This can be fixed with i915_gem_next_request_seqno.
(i.e. the outstanding lazy request stuff).

> +int i915_switch_context(struct intel_ring_buffer *ring,
> +			struct drm_file *file,
> +			int to_id, u32 seqno, u32 flags)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_file_private *file_priv = NULL;
> +	struct i915_hw_context *to;
> +	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
> +	int ret;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return 0;
> +
> +	if (ring != &dev_priv->ring[RCS])
> +		return 0;

Because your execbuffer integration seems to pass in the ctx_id unchecked,
this is actually a userspace bug and should be shot down (a lot earlier
than this even, i.e. before we try to run the batch) with -EINVAL. Same
for the one above.

> +
> +	if (file)
> +		file_priv = file->driver_priv;

Some of these checks (and also some of the BUG_ONs you're sprinkling) look
rather strange. This one here doesn't bother to do anything when !file,
which won't end well.

> +
> +	if (to_id == DEFAULT_CONTEXT_ID) {
> +		to = ring->default_context;
> +		kref_get(&to->nref);
> +	} else {
> +		to = i915_gem_context_get(file_priv, to_id);
> +		if (to == NULL)
> +			return -EINVAL;
> +	}
> +	drm_gem_object_reference(&to->obj->base);
> +
> +	/* If the object is still on the active list, we can shortcut */
> +	if (from_obj == to->obj) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = do_switch(from_obj, to, seqno, flags);
> +	if (ret) {
> +		drm_gem_object_unreference(&to->obj->base);
> +		goto out;
> +	}
> +
> +out:
> +	if (from_obj != NULL)
> +		drm_gem_object_unreference(&from_obj->base);
> +	kref_put(&to->nref, destroy_hw_context);
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ca3972f..cd74f86 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -920,6 +920,32 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
>  	return 0;
>  }
>  
> +int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
> +			      struct i915_hw_context *new_context,
> +			      u32 hw_flags)
> +{
> +	int ret;
> +
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret)
> +		return ret;
> +
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_SET_CONTEXT);
> +	intel_ring_emit(ring, new_context->obj->gtt_offset |
> +			MI_MM_SPACE_GTT |
> +			MI_SAVE_EXT_STATE_EN |
> +			MI_RESTORE_EXT_STATE_EN |
> +			hw_flags);
> +	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
> +	intel_ring_emit(ring, MI_NOOP);
> +
> +	intel_ring_advance(ring);
> +
> +	return ret;
> +
> +}

I know that this started out as a ringbuffer-abstracted thing. But I'd
kinda prefer if it lives together with the other context stuff in
i015_gem_context.c ... Like the pageflip MI_ functions live right next to
the generic pageflip code. I won't be too angry though if you ignore me
here ;-P

> +
>  static void cleanup_status_page(struct intel_ring_buffer *ring)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8c9f898..0ed98bb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -121,6 +121,7 @@ struct  intel_ring_buffer {
>  	drm_local_map_t map;
>  
>  	struct i915_hw_context *default_context;
> +	struct drm_i915_gem_object *last_context_obj;
>  
>  	void *private;
>  };
> @@ -216,6 +217,10 @@ static inline void i915_trace_irq_get(struct intel_ring_buffer *ring, u32 seqno)
>  		ring->trace_irq_seqno = seqno;
>  }
>  
> +int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
> +			      struct i915_hw_context *new_context,
> +			      u32 hw_flags);
> +
>  /* DRI warts */
>  int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
>  
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 06/18] drm/i915: trace events for contexts

[Ben Widawsky]

Recommended by Daniel Vetter. The original code used DRM_DEBUG_DRIVER.
With this we track context creation, destruction, and switching.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index cc508d5..f1559285 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -72,6 +72,8 @@ static void do_destroy(struct i915_hw_context *ctx)
 	else
 		BUG_ON(ctx != dev_priv->ring[RCS].default_context);
 
+	trace_i915_context_destroy(ctx);
+
 	drm_gem_object_unreference(&ctx->obj->base);
 	kfree(ctx);
 }
@@ -105,7 +107,7 @@ create_hw_context(struct drm_device *dev,
 
 	/* Default context will never have a file_priv */
 	if (file_priv == NULL)
-		return 0;
+		goto out;
 
 	(*ctx_out)->file_priv = file_priv;
 
@@ -127,7 +129,9 @@ again:
 	} else if (ret)
 		goto err_out;
 
+out:
 	(*ctx_out)->obj->context_id = (*ctx_out)->id;
+	trace_i915_context_create(*ctx_out);
 	return 0;
 
 err_out:
@@ -332,6 +336,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
 
 	ring->last_context_obj = to->obj;
 
+	trace_i915_context_switch(from_obj, to);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index dac7bba..d72a6f1 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -410,6 +410,65 @@ TRACE_EVENT(i915_reg_rw,
 		(u32)(__entry->val >> 32))
 );
 
+/* Context tracking */
+TRACE_EVENT(i915_context_create,
+	    TP_PROTO(struct i915_hw_context *ctx),
+	    TP_ARGS(ctx),
+
+	    TP_STRUCT__entry(
+			     __field(struct i915_hw_context *, ctx)
+			     __field(u32, id)
+			     __field(struct drm_i915_gem_object *, obj)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->ctx = ctx;
+			   __entry->id = ctx->id;
+			   __entry->obj = ctx->obj;
+			   ),
+
+	    TP_printk("ctx=%p, id=%u, obj=%p",
+		      __entry->ctx, __entry->id, __entry->obj)
+);
+
+TRACE_EVENT(i915_context_destroy,
+	    TP_PROTO(struct i915_hw_context *ctx),
+	    TP_ARGS(ctx),
+
+	    TP_STRUCT__entry(
+			     __field(struct i915_hw_context *, ctx)
+			     __field(u32, id)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->ctx = ctx;
+			   __entry->id = ctx->id;
+			   ),
+
+	    TP_printk("ctx=%p, id=%u", __entry->ctx, __entry->id)
+);
+
+TRACE_EVENT(i915_context_switch,
+	    TP_PROTO(struct drm_i915_gem_object *from, struct i915_hw_context *to),
+	    TP_ARGS(from, to),
+
+	    TP_STRUCT__entry(
+			     __field(struct drm_i915_gem_object *, fobj)
+			     __field(struct drm_i915_gem_object *, tobj)
+			     __field(u32, idf)
+			     __field(u32, idt)
+			    ),
+
+	    TP_fast_assign(
+			   __entry->fobj = from;
+			   __entry->idf = (from == NULL) ? -1 : to->obj->context_id;
+			   __entry->tobj = to->obj;
+			   __entry->idt = to->id;
+			   ),
+
+	    TP_printk("context switch from %u (%p) to %u (%p)",
+		      __entry->idf, __entry->fobj, __entry->idt, __entry->tobj)
+);
 #endif /* _I915_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
1.7.9.4

[PATCH 07/18] drm/i915: Ivybridge MI_ARB_ON_OFF context w/a

[Ben Widawsky]

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9e518f4..1a83b4b 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)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index cd74f86..1c1d6a6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -926,10 +926,15 @@ int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
 {
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
 
+	if (IS_GEN7(ring->dev))
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+	else
+		intel_ring_emit(ring, MI_NOOP);
+
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
 	intel_ring_emit(ring, new_context->obj->gtt_offset |
@@ -940,6 +945,11 @@ int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
 	/* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP */
 	intel_ring_emit(ring, MI_NOOP);
 
+	if (IS_GEN7(ring->dev))
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
+	else
+		intel_ring_emit(ring, MI_NOOP);
+
 	intel_ring_advance(ring);
 
 	return ret;
-- 
1.7.9.4

[PATCH 08/18] drm/i915: PIPE_CONTROL_TLB_INVALIDATE

[Ben Widawsky]

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

I tested this one on its own and saw no differences in basic tests
(performance or otherwise).

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1a83b4b..6b6d685 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 1c1d6a6..e892364 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -209,6 +209,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.4

[PATCH 09/18] drm/i915: possibly invalidate TLB before context switch

[Ben Widawsky]

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e892364..392e782 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -399,6 +399,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);
@@ -927,6 +931,14 @@ int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
 {
 	int ret;
 
+	if (IS_GEN6(ring->dev) && 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;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0ed98bb..e404e52 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -120,6 +120,10 @@ 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;
 	struct i915_hw_context *default_context;
 	struct drm_i915_gem_object *last_context_obj;
 
-- 
1.7.9.4

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

Re: [PATCH 09/18] drm/i915: possibly invalidate TLB before context switch

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:49PM -0700, Ben Widawsky wrote:
> From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf
> 
> [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s
> responsibility to invalidate the TLBs at least once after the previous
> context switch after any GTT mappings changed (including new GTT
> entries).  This can be done by a pipelined PIPE_CONTROL with TLB inv bit
> set immediately before MI_SET_CONTEXT.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Hm, I've beend decently confused about the meaning of
GFX_TLB_INVALIDATE_ALWAYS - it actually means that we flush tlbs on every
full flush (i.e. always) when it's reset. And we don't set this so this
workaround is pretty much just informational. I'm hence wondering whether
a big comment wouldn't be better?

-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   12 ++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e892364..392e782 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -399,6 +399,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);
> @@ -927,6 +931,14 @@ int intel_ring_mi_set_context(struct intel_ring_buffer *ring,
>  {
>  	int ret;
>  
> +	if (IS_GEN6(ring->dev) && 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;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 0ed98bb..e404e52 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -120,6 +120,10 @@ 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;
>  	struct i915_hw_context *default_context;
>  	struct drm_i915_gem_object *last_context_obj;
>  
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/18] drm/i915: possibly invalidate TLB before context switch

[Ben Widawsky]

On Thu, 29 Mar 2012 21:25:49 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:49PM -0700, Ben Widawsky wrote:
> > From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf
> > 
> > [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s
> > responsibility to invalidate the TLBs at least once after the previous
> > context switch after any GTT mappings changed (including new GTT
> > entries).  This can be done by a pipelined PIPE_CONTROL with TLB inv bit
> > set immediately before MI_SET_CONTEXT.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Hm, I've beend decently confused about the meaning of
> GFX_TLB_INVALIDATE_ALWAYS - it actually means that we flush tlbs on every
> full flush (i.e. always) when it's reset. And we don't set this so this
> workaround is pretty much just informational. I'm hence wondering whether
> a big comment wouldn't be better?
> 
> -Daniel

It's probably not much difference in terms of LOC with the plus that the
"comment" turns into correctly functioning code if we flip the bit.
Just from a quick glance at code though, it seems we don't explicitly
touch this bit for GEN6. Which was probably why I did it in the first
place.

if (IS_GEN7(dev))
	I915_WRITE(GFX_MODE_GEN7,
		   GFX_MODE_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
		   GFX_MODE_ENABLE(GFX_REPLAY_MODE));


Anyway, I really don't care enough to argue, but I like the way it is,
and it's been tested the way it is; call the ball.

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

Re: [PATCH 09/18] drm/i915: possibly invalidate TLB before context switch

[Daniel Vetter]

On Fri, Mar 30, 2012 at 11:39:53AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:25:49 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:49PM -0700, Ben Widawsky wrote:
> > > From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf
> > > 
> > > [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s
> > > responsibility to invalidate the TLBs at least once after the previous
> > > context switch after any GTT mappings changed (including new GTT
> > > entries).  This can be done by a pipelined PIPE_CONTROL with TLB inv bit
> > > set immediately before MI_SET_CONTEXT.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Hm, I've beend decently confused about the meaning of
> > GFX_TLB_INVALIDATE_ALWAYS - it actually means that we flush tlbs on every
> > full flush (i.e. always) when it's reset. And we don't set this so this
> > workaround is pretty much just informational. I'm hence wondering whether
> > a big comment wouldn't be better?
> > 
> > -Daniel
> 
> It's probably not much difference in terms of LOC with the plus that the
> "comment" turns into correctly functioning code if we flip the bit.
> Just from a quick glance at code though, it seems we don't explicitly
> touch this bit for GEN6. Which was probably why I did it in the first
> place.
> 
> if (IS_GEN7(dev))
> 	I915_WRITE(GFX_MODE_GEN7,
> 		   GFX_MODE_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
> 		   GFX_MODE_ENABLE(GFX_REPLAY_MODE));
> 
> 
> Anyway, I really don't care enough to argue, but I like the way it is,
> and it's been tested the way it is; call the ball.

Add a quick comment where you insert the wa flush that in our current
config this isn't used and keep it. I don't care too much.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/18] drm/i915: use the default context

[Ben Widawsky]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f1559285..d9a08f2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -37,6 +37,9 @@
 
 static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
+static int do_switch(struct drm_i915_gem_object *from_obj,
+		     struct i915_hw_context *to, u32 seqno,
+		     u32 flags);
 
 static int get_context_size(struct drm_device *dev)
 {
@@ -182,6 +185,14 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		do_destroy(ctx);
 
+	ret = do_switch(NULL, ctx, 0, I915_CONTEXT_INITIAL_SWITCH);
+	if (ret) {
+		i915_gem_object_unpin(ctx->obj);
+		BUG_ON(kref_put(&dev_priv->ring[RCS].default_context->nref,
+			        destroy_hw_context) == 0);
+	} else
+		DRM_DEBUG_DRIVER("Default HW context loaded\n");
+
 	return ret;
 }
 
-- 
1.7.9.4

[PATCH 11/18] drm/i915: switch to default context on idle

[Ben Widawsky]

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

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0985aa5..c1aab45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3685,6 +3685,8 @@ int
 i915_gem_idle(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+	u32 seqno;
 	int ret;
 
 	mutex_lock(&dev->struct_mutex);
@@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
 		return 0;
 	}
 
+	seqno = i915_gem_next_request_seqno(ring);
+	i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
+	WARN_ON(i915_wait_request(ring, seqno, false));
+
 	ret = i915_gpu_idle(dev, true);
 	if (ret) {
 		mutex_unlock(&dev->struct_mutex);
-- 
1.7.9.4

Re: [PATCH 11/18] drm/i915: switch to default context on idle

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
> To keep things as sane as possible, switch to the default context before
> idling. This should help free context objects, as well as put things in
> a more well defined state before suspending.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Context are yet another thing that will nicely fragment our global gtt
space. You don't pin them to mappable, which is a start, but I wonder
whether we should integrate this into our evict_something code? Although
that project is more involved with the current evict code, so maybe just
add a fixme.

> ---
>  drivers/gpu/drm/i915/i915_gem.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0985aa5..c1aab45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3685,6 +3685,8 @@ int
>  i915_gem_idle(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> +	u32 seqno;
>  	int ret;
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
>  		return 0;
>  	}
>  
> +	seqno = i915_gem_next_request_seqno(ring);
> +	i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
> +	WARN_ON(i915_wait_request(ring, seqno, false));

This can block and potentially return early due to a signal, i.e. you need
to properly handle this (like the i915_gpu_idle below).

> +
>  	ret = i915_gpu_idle(dev, true);
>  	if (ret) {
>  		mutex_unlock(&dev->struct_mutex);
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 11/18] drm/i915: switch to default context on idle

[Chris Wilson]

On Thu, 29 Mar 2012 21:29:05 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
> > To keep things as sane as possible, switch to the default context before
> > idling. This should help free context objects, as well as put things in
> > a more well defined state before suspending.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Context are yet another thing that will nicely fragment our global gtt
> space. You don't pin them to mappable, which is a start, but I wonder
> whether we should integrate this into our evict_something code? Although
> that project is more involved with the current evict code, so maybe just
> add a fixme.

My suggestion for when we want to step beyond having the context pinned
all the time was for a hook into unbind that checked to see if this was
a "pinned" object and to fixup any registers to point back to the
default. In the case of contexts, it would check a flag to see if it was
indeed the active context and then schedule a change back to the default
context prior to unbinding. From another viewpoint, this sounds alot
like fences, and maybe we should add the tracking to the CXTID register
in a similar manner.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 11/18] drm/i915: switch to default context on idle

[Ben Widawsky]

On Thu, 29 Mar 2012 21:29:05 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
> > To keep things as sane as possible, switch to the default context before
> > idling. This should help free context objects, as well as put things in
> > a more well defined state before suspending.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Context are yet another thing that will nicely fragment our global gtt
> space. You don't pin them to mappable, which is a start, but I wonder
> whether we should integrate this into our evict_something code? Although
> that project is more involved with the current evict code, so maybe just
> add a fixme.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0985aa5..c1aab45 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3685,6 +3685,8 @@ int
> >  i915_gem_idle(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> > +	u32 seqno;
> >  	int ret;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > @@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
> >  		return 0;
> >  	}
> >  
> > +	seqno = i915_gem_next_request_seqno(ring);
> > +	i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
> > +	WARN_ON(i915_wait_request(ring, seqno, false));
> 
> This can block and potentially return early due to a signal, i.e. you need
> to properly handle this (like the i915_gpu_idle below).

Can you elaborate? Whether or not the context switch fails, I want to
try to idle the GPU. So I *thought* this is doing just that.

> 
> > +
> >  	ret = i915_gpu_idle(dev, true);
> >  	if (ret) {
> >  		mutex_unlock(&dev->struct_mutex);
> > -- 
> > 1.7.9.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 11/18] drm/i915: switch to default context on idle

[Daniel Vetter]

On Fri, Mar 30, 2012 at 02:17:20PM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:29:05 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
> > > To keep things as sane as possible, switch to the default context before
> > > idling. This should help free context objects, as well as put things in
> > > a more well defined state before suspending.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Context are yet another thing that will nicely fragment our global gtt
> > space. You don't pin them to mappable, which is a start, but I wonder
> > whether we should integrate this into our evict_something code? Although
> > that project is more involved with the current evict code, so maybe just
> > add a fixme.
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 0985aa5..c1aab45 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3685,6 +3685,8 @@ int
> > >  i915_gem_idle(struct drm_device *dev)
> > >  {
> > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > > +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> > > +	u32 seqno;
> > >  	int ret;
> > >  
> > >  	mutex_lock(&dev->struct_mutex);
> > > @@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
> > >  		return 0;
> > >  	}
> > >  
> > > +	seqno = i915_gem_next_request_seqno(ring);
> > > +	i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
> > > +	WARN_ON(i915_wait_request(ring, seqno, false));
> > 
> > This can block and potentially return early due to a signal, i.e. you need
> > to properly handle this (like the i915_gpu_idle below).
> 
> Can you elaborate? Whether or not the context switch fails, I want to
> try to idle the GPU. So I *thought* this is doing just that.

wait_request can return error codes, you need to handle them. E.g. gpu
hang, interrupt, out of memory, whatnotelse.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 12/18] drm/i915: try to reset the gpu before unload

[Ben Widawsky]

paranoia

For context support the HW expects the default context to always be
available as there is no way to shut off HW contexts once turned on
(afaics). This is problematic when unloading the driver as we have no
way to prevent the GPU from expecting the BO to still be present once
unloaded.

The best we can do to remedy the situation is to attempt a GPU reset
when doing the unload.

NOTE: this patch isn't *really* required to go with the rest of the
context serious.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c1aab45..848cc45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev)
 	ret = i915_gem_idle(dev);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
+	ret = i915_reset(dev, GRDOM_FULL);
+	if (ret)
+		DRM_ERROR("failed to reset gpu: %d\n", ret);
 }
 
 static void
-- 
1.7.9.4

Re: [PATCH 12/18] drm/i915: try to reset the gpu before unload

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:52PM -0700, Ben Widawsky wrote:
> paranoia
> 
> For context support the HW expects the default context to always be
> available as there is no way to shut off HW contexts once turned on
> (afaics). This is problematic when unloading the driver as we have no
> way to prevent the GPU from expecting the BO to still be present once
> unloaded.
> 
> The best we can do to remedy the situation is to attempt a GPU reset
> when doing the unload.
> 
> NOTE: this patch isn't *really* required to go with the rest of the
> context serious.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I think the paranoia here is justified (albeit it would benefit from some
commit-message love imo). But we do not support i915_reset on all gens, so
I think you need to add a gen >= 5 check here.

> ---
>  drivers/gpu/drm/i915/i915_gem.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c1aab45..848cc45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev)
>  	ret = i915_gem_idle(dev);
>  	if (ret)
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> +	ret = i915_reset(dev, GRDOM_FULL);
> +	if (ret)
> +		DRM_ERROR("failed to reset gpu: %d\n", ret);
>  }
>  
>  static void
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 12/18] drm/i915: try to reset the gpu before unload

[Ben Widawsky]

On Thu, 29 Mar 2012 21:31:21 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:52PM -0700, Ben Widawsky wrote:
> > paranoia
> > 
> > For context support the HW expects the default context to always be
> > available as there is no way to shut off HW contexts once turned on
> > (afaics). This is problematic when unloading the driver as we have no
> > way to prevent the GPU from expecting the BO to still be present once
> > unloaded.
> > 
> > The best we can do to remedy the situation is to attempt a GPU reset
> > when doing the unload.
> > 
> > NOTE: this patch isn't *really* required to go with the rest of the
> > context serious.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I think the paranoia here is justified (albeit it would benefit from some
> commit-message love imo). But we do not support i915_reset on all gens, so
> I think you need to add a gen >= 5 check here.

I think i915_reset does the right thing, but I'm not sure. It has a big
gen switch statement in it.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c1aab45..848cc45 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev)
> >  	ret = i915_gem_idle(dev);
> >  	if (ret)
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> > +	ret = i915_reset(dev, GRDOM_FULL);
> > +	if (ret)
> > +		DRM_ERROR("failed to reset gpu: %d\n", ret);
> >  }
> >  
> >  static void
> > -- 
> > 1.7.9.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 12/18] drm/i915: try to reset the gpu before unload

[Daniel Vetter]

On Fri, Mar 30, 2012 at 11:50:42AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:31:21 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:52PM -0700, Ben Widawsky wrote:
> > > paranoia
> > > 
> > > For context support the HW expects the default context to always be
> > > available as there is no way to shut off HW contexts once turned on
> > > (afaics). This is problematic when unloading the driver as we have no
> > > way to prevent the GPU from expecting the BO to still be present once
> > > unloaded.
> > > 
> > > The best we can do to remedy the situation is to attempt a GPU reset
> > > when doing the unload.
> > > 
> > > NOTE: this patch isn't *really* required to go with the rest of the
> > > context serious.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > I think the paranoia here is justified (albeit it would benefit from some
> > commit-message love imo). But we do not support i915_reset on all gens, so
> > I think you need to add a gen >= 5 check here.
> 
> I think i915_reset does the right thing, but I'm not sure. It has a big
> gen switch statement in it.

Yeah, it just returns. But on reading i915_reset I think you don't want
all it does - it resets the entire modeset and gem state, too. I think it
would be better to just do the hw reset and not bother with everything
else - the only chance is that it accidentally breaks module unload.

What about extracting the actual hw reset code (i.e. the switch(gen)
stuff) into i915_do_reset and only calling that? If you want, add the
is_gen5 check, but given that it's just module unload I don't care about
fried hw due to a not-so-well-working gpu reset ;-)
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 12/18] drm/i915: try to reset the gpu before unload

[Jesse Barnes]

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

On Sun, 18 Mar 2012 13:39:52 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> paranoia
> 
> For context support the HW expects the default context to always be
> available as there is no way to shut off HW contexts once turned on
> (afaics). This is problematic when unloading the driver as we have no
> way to prevent the GPU from expecting the BO to still be present once
> unloaded.
> 
> The best we can do to remedy the situation is to attempt a GPU reset
> when doing the unload.
> 
> NOTE: this patch isn't *really* required to go with the rest of the
> context serious.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c1aab45..848cc45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev)
>  	ret = i915_gem_idle(dev);
>  	if (ret)
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> +	ret = i915_reset(dev, GRDOM_FULL);
> +	if (ret)
> +		DRM_ERROR("failed to reset gpu: %d\n", ret);
>  }
>  
>  static void

This reminds me that we should make our reset finer grained.  We
generally only need to reset the render engine when we detect an error,
but today we reset display and media unconditionally too.  Not
resetting the display would make reset more invisible like it ought to
be...

I haven't tested it, but it's yet another thing for our TODO list.

-- 
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 12/18] drm/i915: try to reset the gpu before unload

[Daniel Vetter]

On Fri, Mar 30, 2012 at 09:54:31AM -0700, Jesse Barnes wrote:
> On Sun, 18 Mar 2012 13:39:52 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > paranoia
> > 
> > For context support the HW expects the default context to always be
> > available as there is no way to shut off HW contexts once turned on
> > (afaics). This is problematic when unloading the driver as we have no
> > way to prevent the GPU from expecting the BO to still be present once
> > unloaded.
> > 
> > The best we can do to remedy the situation is to attempt a GPU reset
> > when doing the unload.
> > 
> > NOTE: this patch isn't *really* required to go with the rest of the
> > context serious.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c1aab45..848cc45 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev)
> >  	ret = i915_gem_idle(dev);
> >  	if (ret)
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> > +	ret = i915_reset(dev, GRDOM_FULL);
> > +	if (ret)
> > +		DRM_ERROR("failed to reset gpu: %d\n", ret);
> >  }
> >  
> >  static void
> 
> This reminds me that we should make our reset finer grained.  We
> generally only need to reset the render engine when we detect an error,
> but today we reset display and media unconditionally too.  Not
> resetting the display would make reset more invisible like it ought to
> be...
> 
> I haven't tested it, but it's yet another thing for our TODO list.

I think with the pch split stuff we don't already reset display. And in
any cases the global gtt seems to just keep going. The real problem is
that you will notice the reset anyway, because we tend to wait a few
seconds before we declare the gpu dead.

For that we also need some improvements (and better culprit detection so
that we can start to reset earlier). That one is on my todo ;-)
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 13/18] drm/i915/context: create & destroy ioctls

[Ben Widawsky]

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

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

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4c7c1dc..fb3fccb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2333,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_gem_context_create_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c6c2ada..d49615e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
 			struct drm_file *file,
 			int to_id, u32 seqno, u32 flags);
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file);
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index d9a08f2..accb3de 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -411,3 +411,67 @@ out:
 	kref_put(&to->nref, destroy_hw_context);
 	return ret;
 }
+
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_context_create *args = data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+	struct i915_hw_context *ctx;
+	int ret;
+
+	if (dev_priv->hw_contexts_disabled)
+		return -EPERM;
+
+	ret = create_hw_context(dev, file_priv, &ctx);
+	if (ret)
+		return ret;
+
+	/* We need to do a special switch (save-only) either now, or before the
+	 * context is actually used in order to keep the context switch request
+	 * in execbuffer fairly simple.
+	 */
+	mutex_lock(&dev->struct_mutex);
+	ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring),
+				  I915_CONTEXT_INITIAL_SWITCH);
+	mutex_unlock(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	args->ctx_id = ctx->id;
+	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
+	return ret;
+}
+
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file)
+{
+	struct drm_i915_gem_context_destroy *args = data;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_hw_context *ctx;
+
+	if (dev_priv->hw_contexts_disabled)
+		return -EPERM;
+
+	mutex_lock(&dev->struct_mutex);
+	ctx = i915_gem_context_get(file_priv, args->ctx_id);
+	if (!ctx) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	/* The first put is for the context ref we got just up above. The second
+	 * put should unref the original ref (and will therefore destroy the context
+	 * unless someone else holds a reference
+	 */
+	kref_put(&ctx->nref, destroy_hw_context);
+	kref_put(&ctx->nref, destroy_hw_context);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
+	return 0;
+}
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index da929bb..bead13e 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,15 @@ struct drm_intel_sprite_colorkey {
 	__u32 flags;
 };
 
+struct drm_i915_gem_context_create {
+	/* output: id of new context*/
+	__u32 ctx_id;
+	__u32 pad;
+};
+
+struct drm_i915_gem_context_destroy {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.9.4

Re: [PATCH 13/18] drm/i915/context: create & destroy ioctls

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
> Add the interfaces to allow user space to create and destroy contexts.
> Contexts are destroyed automatically if the file descriptor for the dri
> device is closed.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         |    2 +
>  drivers/gpu/drm/i915/i915_drv.h         |    4 ++
>  drivers/gpu/drm/i915/i915_gem_context.c |   64 +++++++++++++++++++++++++++++++
>  include/drm/i915_drm.h                  |   16 ++++++++
>  4 files changed, 86 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 4c7c1dc..fb3fccb 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2333,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_gem_context_create_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c6c2ada..d49615e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
>  			struct drm_file *file,
>  			int to_id, u32 seqno, u32 flags);
> +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> +				  struct drm_file *file);
> +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file);
>  
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index d9a08f2..accb3de 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -411,3 +411,67 @@ out:
>  	kref_put(&to->nref, destroy_hw_context);
>  	return ret;
>  }
> +
> +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> +				  struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_context_create *args = data;
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> +	struct i915_hw_context *ctx;
> +	int ret;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return -EPERM;
> +
> +	ret = create_hw_context(dev, file_priv, &ctx);

Note that the gem_alloc_object call in here is rather unsafe, due to the
unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
goof-up, but can I maybe volunteer you to fix this? I think we need an
mm stats spinlock because these counters could be rather big (and atomic_t
is only 32 bits).

> +	if (ret)
> +		return ret;
> +
> +	/* We need to do a special switch (save-only) either now, or before the
> +	 * context is actually used in order to keep the context switch request
> +	 * in execbuffer fairly simple.
> +	 */
> +	mutex_lock(&dev->struct_mutex);

> +	ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring),
> +				  I915_CONTEXT_INITIAL_SWITCH);

With the hw_context->is_intialized change you could ditch this (imo).

> +	mutex_unlock(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	args->ctx_id = ctx->id;
> +	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
> +	return ret;
> +}
> +
> +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file)
> +{
> +	struct drm_i915_gem_context_destroy *args = data;
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_hw_context *ctx;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return -EPERM;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> +	if (!ctx) {
> +		mutex_unlock(&dev->struct_mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* The first put is for the context ref we got just up above. The second
> +	 * put should unref the original ref (and will therefore destroy the context
> +	 * unless someone else holds a reference
> +	 */
> +	kref_put(&ctx->nref, destroy_hw_context);
> +	kref_put(&ctx->nref, destroy_hw_context);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
> +	return 0;
> +}
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index da929bb..bead13e 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,15 @@ struct drm_intel_sprite_colorkey {
>  	__u32 flags;
>  };
>  
> +struct drm_i915_gem_context_create {
> +	/* output: id of new context*/
> +	__u32 ctx_id;
> +	__u32 pad;
> +};
> +
> +struct drm_i915_gem_context_destroy {
> +	__u32 ctx_id;
> +	__u32 pad;
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 13/18] drm/i915/context: create & destroy ioctls

[Ben Widawsky]

On Thu, 29 Mar 2012 21:35:35 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
> > Add the interfaces to allow user space to create and destroy contexts.
> > Contexts are destroyed automatically if the file descriptor for the dri
> > device is closed.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c         |    2 +
> >  drivers/gpu/drm/i915/i915_drv.h         |    4 ++
> >  drivers/gpu/drm/i915/i915_gem_context.c |   64 +++++++++++++++++++++++++++++++
> >  include/drm/i915_drm.h                  |   16 ++++++++
> >  4 files changed, 86 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 4c7c1dc..fb3fccb 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -2333,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_gem_context_create_ioctl, DRM_UNLOCKED),
> > +	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> >  };
> >  
> >  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c6c2ada..d49615e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> >  int i915_switch_context(struct intel_ring_buffer *ring,
> >  			struct drm_file *file,
> >  			int to_id, u32 seqno, u32 flags);
> > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > +				  struct drm_file *file);
> > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > +				   struct drm_file *file);
> >  
> >  /* i915_gem_gtt.c */
> >  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index d9a08f2..accb3de 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -411,3 +411,67 @@ out:
> >  	kref_put(&to->nref, destroy_hw_context);
> >  	return ret;
> >  }
> > +
> > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > +				  struct drm_file *file)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_gem_context_create *args = data;
> > +	struct drm_i915_file_private *file_priv = file->driver_priv;
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> > +	struct i915_hw_context *ctx;
> > +	int ret;
> > +
> > +	if (dev_priv->hw_contexts_disabled)
> > +		return -EPERM;
> > +
> > +	ret = create_hw_context(dev, file_priv, &ctx);
> 
> Note that the gem_alloc_object call in here is rather unsafe, due to the
> unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
> goof-up, but can I maybe volunteer you to fix this? I think we need an
> mm stats spinlock because these counters could be rather big (and atomic_t
> is only 32 bits).

I can do it. In this series?

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* We need to do a special switch (save-only) either now, or before the
> > +	 * context is actually used in order to keep the context switch request
> > +	 * in execbuffer fairly simple.
> > +	 */
> > +	mutex_lock(&dev->struct_mutex);
> 
> > +	ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring),
> > +				  I915_CONTEXT_INITIAL_SWITCH);
> 
> With the hw_context->is_intialized change you could ditch this (imo).

You could ditch it, but I can't make an argument that one way is better
than another. As you stated earlier, you want me to ditch the spinlock
for the context id creation, so I'll still need to acquire struct mutex
anyway. At which point, I think it doesn't hurt to switch now too.

I'm willing to change this is you can describe a benefit for it.

> 
> > +	mutex_unlock(&dev->struct_mutex);
> > +	if (ret)
> > +		return ret;
> > +
> > +	args->ctx_id = ctx->id;
> > +	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
> > +	return ret;
> > +}
> > +
> > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > +				   struct drm_file *file)
> > +{
> > +	struct drm_i915_gem_context_destroy *args = data;
> > +	struct drm_i915_file_private *file_priv = file->driver_priv;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct i915_hw_context *ctx;
> > +
> > +	if (dev_priv->hw_contexts_disabled)
> > +		return -EPERM;
> > +
> > +	mutex_lock(&dev->struct_mutex);
> > +	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > +	if (!ctx) {
> > +		mutex_unlock(&dev->struct_mutex);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* The first put is for the context ref we got just up above. The second
> > +	 * put should unref the original ref (and will therefore destroy the context
> > +	 * unless someone else holds a reference
> > +	 */
> > +	kref_put(&ctx->nref, destroy_hw_context);
> > +	kref_put(&ctx->nref, destroy_hw_context);
> > +
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
> > +	return 0;
> > +}
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index da929bb..bead13e 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,15 @@ struct drm_intel_sprite_colorkey {
> >  	__u32 flags;
> >  };
> >  
> > +struct drm_i915_gem_context_create {
> > +	/* output: id of new context*/
> > +	__u32 ctx_id;
> > +	__u32 pad;
> > +};
> > +
> > +struct drm_i915_gem_context_destroy {
> > +	__u32 ctx_id;
> > +	__u32 pad;
> > +};
> > +
> >  #endif				/* _I915_DRM_H_ */
> > -- 
> > 1.7.9.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 13/18] drm/i915/context: create & destroy ioctls

[Daniel Vetter]

On Fri, Mar 30, 2012 at 11:55:14AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:35:35 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
> > > Add the interfaces to allow user space to create and destroy contexts.
> > > Contexts are destroyed automatically if the file descriptor for the dri
> > > device is closed.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c         |    2 +
> > >  drivers/gpu/drm/i915/i915_drv.h         |    4 ++
> > >  drivers/gpu/drm/i915/i915_gem_context.c |   64 +++++++++++++++++++++++++++++++
> > >  include/drm/i915_drm.h                  |   16 ++++++++
> > >  4 files changed, 86 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 4c7c1dc..fb3fccb 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -2333,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_gem_context_create_ioctl, DRM_UNLOCKED),
> > > +	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> > >  };
> > >  
> > >  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index c6c2ada..d49615e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> > >  int i915_switch_context(struct intel_ring_buffer *ring,
> > >  			struct drm_file *file,
> > >  			int to_id, u32 seqno, u32 flags);
> > > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > > +				  struct drm_file *file);
> > > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > > +				   struct drm_file *file);
> > >  
> > >  /* i915_gem_gtt.c */
> > >  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index d9a08f2..accb3de 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -411,3 +411,67 @@ out:
> > >  	kref_put(&to->nref, destroy_hw_context);
> > >  	return ret;
> > >  }
> > > +
> > > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > > +				  struct drm_file *file)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct drm_i915_gem_context_create *args = data;
> > > +	struct drm_i915_file_private *file_priv = file->driver_priv;
> > > +	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> > > +	struct i915_hw_context *ctx;
> > > +	int ret;
> > > +
> > > +	if (dev_priv->hw_contexts_disabled)
> > > +		return -EPERM;
> > > +
> > > +	ret = create_hw_context(dev, file_priv, &ctx);
> > 
> > Note that the gem_alloc_object call in here is rather unsafe, due to the
> > unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
> > goof-up, but can I maybe volunteer you to fix this? I think we need an
> > mm stats spinlock because these counters could be rather big (and atomic_t
> > is only 32 bits).
> 
> I can do it. In this series?

That bug is pre-existing since gem was merged afaik. So just when you're bored, in a
separate series.

> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* We need to do a special switch (save-only) either now, or before the
> > > +	 * context is actually used in order to keep the context switch request
> > > +	 * in execbuffer fairly simple.
> > > +	 */
> > > +	mutex_lock(&dev->struct_mutex);
> > 
> > > +	ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring),
> > > +				  I915_CONTEXT_INITIAL_SWITCH);
> > 
> > With the hw_context->is_intialized change you could ditch this (imo).
> 
> You could ditch it, but I can't make an argument that one way is better
> than another. As you stated earlier, you want me to ditch the spinlock
> for the context id creation, so I'll still need to acquire struct mutex
> anyway. At which point, I think it doesn't hurt to switch now too.
> 
> I'm willing to change this is you can describe a benefit for it.

Imo the benefit is that we have one (and just one) clear place where we
switch context. Doing a funky switch at create time just feels wonky. Just
now I've also noticed that you call ring->get_seqno(ring) - that reads the
current seqno from the gpu hws and is absolutely not what you want. So
you'd have to add another request (and attach it to the file_priv).

I also like that with the is_initialized stored in the context, we nicely
abstract away this little detail of how we need to frob the switch command
on first use of a context.

I don't feel strongly about this, but if you want to stick with explicit
initialization, please wrap it all up in a little helper function to make
it clear that we don't care about switching and only about initializing.
Imo you should then still rip away the flags parameter from
i915_switch_context - execbuffer really doesn't (and shouldn't) care about
that detail.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 14/18] drm/i915/context: switch contexts with execbuf2

[Ben Widawsky]

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

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 81687af..c365e12 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *batch_obj;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
+	u32 ctx_id = args->context_info & I915_EXEC_CONTEXT_ID_MASK;
 	u32 exec_start, exec_len;
 	u32 seqno;
 	u32 mask;
@@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
+	ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
+	if (ret)
+		goto err;
+
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
 	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
@@ -1372,6 +1377,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 	exec2.num_cliprects = args->num_cliprects;
 	exec2.cliprects_ptr = args->cliprects_ptr;
 	exec2.flags = I915_EXEC_RENDER;
+	exec2.context_info = 0;
 
 	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
 	if (!ret) {
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index bead13e..03d159f 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -660,13 +660,15 @@ struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
 	__u64 flags;
-	__u64 rsvd1;
+	__u64 context_info;
 	__u64 rsvd2;
 };
 
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
 
+#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
1.7.9.4

Re: [PATCH 14/18] drm/i915/context: switch contexts with execbuf2

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote:
> Use the rsvd1 field in execbuf2 to specify the context ID associated
> with the workload. This will allow the driver to do the proper context
> switch when/if needed.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++++
>  include/drm/i915_drm.h                     |    4 +++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 81687af..c365e12 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	struct drm_i915_gem_object *batch_obj;
>  	struct drm_clip_rect *cliprects = NULL;
>  	struct intel_ring_buffer *ring;
> +	u32 ctx_id = args->context_info & I915_EXEC_CONTEXT_ID_MASK;
>  	u32 exec_start, exec_len;
>  	u32 seqno;
>  	u32 mask;
> @@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> +	ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
> +	if (ret)
> +		goto err;
> +

Already complained a bit about these:
- Imo the seqno and hw_flags param can/should go away.
- You need to add some sanity checks somewhere so that we correctly bail
  out of the do_execbuffer stuff without launching the batch. Obviously
  also a prime candidate for an i-g-t tests (because it'll nicely exercise
  a piece of code which usually is rather hard to tests).

Aside: I haven't yet looked at your testcases, so maybe I'll come up with
a crazy idea for another testcase ;-)

>  	trace_i915_gem_ring_dispatch(ring, seqno);
>  
>  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> @@ -1372,6 +1377,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
>  	exec2.num_cliprects = args->num_cliprects;
>  	exec2.cliprects_ptr = args->cliprects_ptr;
>  	exec2.flags = I915_EXEC_RENDER;
> +	exec2.context_info = 0;
>  
>  	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
>  	if (!ret) {
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index bead13e..03d159f 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -660,13 +660,15 @@ struct drm_i915_gem_execbuffer2 {
>  #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
>  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
>  	__u64 flags;
> -	__u64 rsvd1;
> +	__u64 context_info;
>  	__u64 rsvd2;
>  };
>  
>  /** Resets the SO write offset registers for transform feedback on gen7. */
>  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
>  
> +#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> +
>  struct drm_i915_gem_pin {
>  	/** Handle of the buffer to be pinned. */
>  	__u32 handle;
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 14/18] drm/i915/context: switch contexts with execbuf2

[Ben Widawsky]

On Thu, 29 Mar 2012 21:38:20 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote:
> > Use the rsvd1 field in execbuf2 to specify the context ID associated
> > with the workload. This will allow the driver to do the proper context
> > switch when/if needed.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++++
> >  include/drm/i915_drm.h                     |    4 +++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 81687af..c365e12 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  	struct drm_i915_gem_object *batch_obj;
> >  	struct drm_clip_rect *cliprects = NULL;
> >  	struct intel_ring_buffer *ring;
> > +	u32 ctx_id = args->context_info & I915_EXEC_CONTEXT_ID_MASK;
> >  	u32 exec_start, exec_len;
> >  	u32 seqno;
> >  	u32 mask;
> > @@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  			goto err;
> >  	}
> >  
> > +	ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
> > +	if (ret)
> > +		goto err;
> > +
> 
> Already complained a bit about these:
> - Imo the seqno and hw_flags param can/should go away.

Responding to the other checks for this.

> - You need to add some sanity checks somewhere so that we correctly bail
>   out of the do_execbuffer stuff without launching the batch. Obviously
>   also a prime candidate for an i-g-t tests (because it'll nicely exercise
>   a piece of code which usually is rather hard to tests).

I'm not sure what you're asking for, aside from - this is hairy code; be
careful. What was your idea?

> 
> Aside: I haven't yet looked at your testcases, so maybe I'll come up with
> a crazy idea for another testcase ;-)

Testcases are pretty sparse in i-g-t. Most of it is done implicitly with
mesa. If you have ideas, I'll be happy to write them

> 
> >  	trace_i915_gem_ring_dispatch(ring, seqno);
> >  
> >  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> > @@ -1372,6 +1377,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> >  	exec2.num_cliprects = args->num_cliprects;
> >  	exec2.cliprects_ptr = args->cliprects_ptr;
> >  	exec2.flags = I915_EXEC_RENDER;
> > +	exec2.context_info = 0;
> >  
> >  	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
> >  	if (!ret) {
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index bead13e..03d159f 100644
> > --- a/include/drm/i915_drm.h
> > +++ b/include/drm/i915_drm.h
> > @@ -660,13 +660,15 @@ struct drm_i915_gem_execbuffer2 {
> >  #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
> >  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
> >  	__u64 flags;
> > -	__u64 rsvd1;
> > +	__u64 context_info;
> >  	__u64 rsvd2;
> >  };
> >  
> >  /** Resets the SO write offset registers for transform feedback on gen7. */
> >  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
> >  
> > +#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> > +
> >  struct drm_i915_gem_pin {
> >  	/** Handle of the buffer to be pinned. */
> >  	__u32 handle;
> > -- 
> > 1.7.9.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 14/18] drm/i915/context: switch contexts with execbuf2

[Daniel Vetter]

On Fri, Mar 30, 2012 at 11:58:20AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:38:20 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote:
> > > Use the rsvd1 field in execbuf2 to specify the context ID associated
> > > with the workload. This will allow the driver to do the proper context
> > > switch when/if needed.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++++
> > >  include/drm/i915_drm.h                     |    4 +++-
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 81687af..c365e12 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  	struct drm_i915_gem_object *batch_obj;
> > >  	struct drm_clip_rect *cliprects = NULL;
> > >  	struct intel_ring_buffer *ring;
> > > +	u32 ctx_id = args->context_info & I915_EXEC_CONTEXT_ID_MASK;
> > >  	u32 exec_start, exec_len;
> > >  	u32 seqno;
> > >  	u32 mask;
> > > @@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  			goto err;
> > >  	}
> > >  
> > > +	ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > 
> > Already complained a bit about these:
> > - Imo the seqno and hw_flags param can/should go away.
> 
> Responding to the other checks for this.
> 
> > - You need to add some sanity checks somewhere so that we correctly bail
> >   out of the do_execbuffer stuff without launching the batch. Obviously
> >   also a prime candidate for an i-g-t tests (because it'll nicely exercise
> >   a piece of code which usually is rather hard to tests).
> 
> I'm not sure what you're asking for, aside from - this is hairy code; be
> careful. What was your idea?

If userspace tries to run an execbuffer with
- an nonexisting context on render
- non-default context on !render_ring
it needs to get a -EINVAL and the kernel must not execute the batch. Your
code fails at that.

We also need a testcase for this case in i-g-t. Another testcase is trying
to destroy a non-existing context. I haven't rechecked whether your
current code handles that correctly.

> > Aside: I haven't yet looked at your testcases, so maybe I'll come up with
> > a crazy idea for another testcase ;-)
> 
> Testcases are pretty sparse in i-g-t. Most of it is done implicitly with
> mesa. If you have ideas, I'll be happy to write them

See above ;-) I'll look at your current set of testcase later and write
another mail with the things I might find missing.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 15/18] 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>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_dma.c |    3 +++
 include/drm/i915_drm.h          |    1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fb3fccb..07d4b96 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -781,6 +781,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_RELAXED_DELTA:
 		value = 1;
 		break;
+	case I915_PARAM_HAS_CONTEXTS:
+		value = dev_priv->hw_contexts_disabled ? 0 : 1;
+		break;
 	case I915_PARAM_HAS_GEN7_SOL_RESET:
 		value = 1;
 		break;
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 03d159f..dd985e1 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -302,6 +302,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.4

[PATCH 16/18] drm/i915/context: anonymous context interfaces

[Ben Widawsky]

Ironlake RC6 needs to allocate a power context object which the hardware
can automatically switch to. Since the new context code nicely handles
contexts, create some interfaces so we can hook up the existing code to
the new code.

The right way to do this is to move a bunch of code out of
intel_display.c but I don't feel like doing it at this time. This patch
is a step in the right direction though.

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |    3 +++
 drivers/gpu/drm/i915/i915_gem_context.c |   40 +++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d49615e..003b62e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -280,6 +280,7 @@ struct i915_hw_ppgtt {
 
 /* This must match up with the value previously used for execbuf2.rsvd1. */
 #define DEFAULT_CONTEXT_ID 0
+#define ANONYMOUS_CONTEXT_ID 1
 struct i915_hw_context {
 	struct drm_i915_file_private *file_priv;
 	struct kref nref;
@@ -1316,6 +1317,8 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file);
 int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
+struct i915_hw_context *i915_context_alloc_anonymous(struct drm_device *dev);
+void i915_context_destroy_anonymous(struct i915_hw_context *ctx);
 
 /* i915_gem_gtt.c */
 int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index accb3de..de1f3ce 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -122,7 +122,7 @@ again:
 
 	spin_lock(&file_priv->context_lock);
 	ret = idr_get_new_above(&file_priv->context_idr, *ctx_out,
-				DEFAULT_CONTEXT_ID + 1, &id);
+				ANONYMOUS_CONTEXT_ID + 1, &id);
 	if (ret == 0)
 		(*ctx_out)->id = id;
 	spin_unlock(&file_priv->context_lock);
@@ -254,7 +254,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
 
-	BUG_ON(id == DEFAULT_CONTEXT_ID);
+	BUG_ON(id == DEFAULT_CONTEXT_ID || id == ANONYMOUS_CONTEXT_ID);
 	ctx = i915_gem_context_get(file_priv, id);
 	BUG_ON(ctx == NULL);
 	kref_put(&ctx->nref, destroy_hw_context);
@@ -380,6 +380,8 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	if (ring != &dev_priv->ring[RCS])
 		return 0;
 
+	BUG_ON(to_id == ANONYMOUS_CONTEXT_ID);
+
 	if (file)
 		file_priv = file->driver_priv;
 
@@ -475,3 +477,37 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
 	return 0;
 }
+
+struct i915_hw_context *
+i915_context_alloc_anonymous(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_hw_context *ctx;
+	int ret;
+
+	if (dev_priv->hw_contexts_disabled)
+		return NULL;
+
+	ret = create_hw_context(dev, NULL, &ctx);
+	if (ret)
+		return NULL;
+
+	ctx->id = ANONYMOUS_CONTEXT_ID;
+	ctx->obj->context_id = ANONYMOUS_CONTEXT_ID;
+
+	/* Anonymous contexts are assumed to be always pinned */
+	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
+	if (ret)
+		do_destroy(ctx);
+
+	return ctx;
+}
+
+void i915_context_destroy_anonymous(struct i915_hw_context *ctx)
+{
+	BUG_ON(ctx->id != ANONYMOUS_CONTEXT_ID);
+	BUG_ON(ctx->obj->context_id != ANONYMOUS_CONTEXT_ID);
+
+	i915_gem_object_unpin(ctx->obj);
+	kref_put(&ctx->nref, destroy_hw_context);
+}
-- 
1.7.9.4

Re: [PATCH 16/18] drm/i915/context: anonymous context interfaces

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:56PM -0700, Ben Widawsky wrote:
> Ironlake RC6 needs to allocate a power context object which the hardware
> can automatically switch to. Since the new context code nicely handles
> contexts, create some interfaces so we can hook up the existing code to
> the new code.
> 
> The right way to do this is to move a bunch of code out of
> intel_display.c but I don't feel like doing it at this time. This patch
> is a step in the right direction though.
> 
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I've looked through this and assuming I understand things correctly, this
is just to replace the power context on ilk. For the render context stuff
I expect that we can just directly re-use the default context.

Adding up the diffs I don't think the added indirection of these anonymous
contexts over simply using a plain gem object for the powercontext is
worth it. So I suggest we just rip this out.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    3 +++
>  drivers/gpu/drm/i915/i915_gem_context.c |   40 +++++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d49615e..003b62e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -280,6 +280,7 @@ struct i915_hw_ppgtt {
>  
>  /* This must match up with the value previously used for execbuf2.rsvd1. */
>  #define DEFAULT_CONTEXT_ID 0
> +#define ANONYMOUS_CONTEXT_ID 1
>  struct i915_hw_context {
>  	struct drm_i915_file_private *file_priv;
>  	struct kref nref;
> @@ -1316,6 +1317,8 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file);
>  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  				   struct drm_file *file);
> +struct i915_hw_context *i915_context_alloc_anonymous(struct drm_device *dev);
> +void i915_context_destroy_anonymous(struct i915_hw_context *ctx);
>  
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index accb3de..de1f3ce 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -122,7 +122,7 @@ again:
>  
>  	spin_lock(&file_priv->context_lock);
>  	ret = idr_get_new_above(&file_priv->context_idr, *ctx_out,
> -				DEFAULT_CONTEXT_ID + 1, &id);
> +				ANONYMOUS_CONTEXT_ID + 1, &id);
>  	if (ret == 0)
>  		(*ctx_out)->id = id;
>  	spin_unlock(&file_priv->context_lock);
> @@ -254,7 +254,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
>  
> -	BUG_ON(id == DEFAULT_CONTEXT_ID);
> +	BUG_ON(id == DEFAULT_CONTEXT_ID || id == ANONYMOUS_CONTEXT_ID);
>  	ctx = i915_gem_context_get(file_priv, id);
>  	BUG_ON(ctx == NULL);
>  	kref_put(&ctx->nref, destroy_hw_context);
> @@ -380,6 +380,8 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	if (ring != &dev_priv->ring[RCS])
>  		return 0;
>  
> +	BUG_ON(to_id == ANONYMOUS_CONTEXT_ID);
> +
>  	if (file)
>  		file_priv = file->driver_priv;
>  
> @@ -475,3 +477,37 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  	DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
>  	return 0;
>  }
> +
> +struct i915_hw_context *
> +i915_context_alloc_anonymous(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_hw_context *ctx;
> +	int ret;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return NULL;
> +
> +	ret = create_hw_context(dev, NULL, &ctx);
> +	if (ret)
> +		return NULL;
> +
> +	ctx->id = ANONYMOUS_CONTEXT_ID;
> +	ctx->obj->context_id = ANONYMOUS_CONTEXT_ID;
> +
> +	/* Anonymous contexts are assumed to be always pinned */
> +	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> +	if (ret)
> +		do_destroy(ctx);
> +
> +	return ctx;
> +}
> +
> +void i915_context_destroy_anonymous(struct i915_hw_context *ctx)
> +{
> +	BUG_ON(ctx->id != ANONYMOUS_CONTEXT_ID);
> +	BUG_ON(ctx->obj->context_id != ANONYMOUS_CONTEXT_ID);
> +
> +	i915_gem_object_unpin(ctx->obj);
> +	kref_put(&ctx->nref, destroy_hw_context);
> +}
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 17/18] drm/i915: Ironlake rc6 can use context interfaces

[Ben Widawsky]

Use the context interfaces to create the power context.  Assuming we
have a default context, there should be no need to switch to
the render context anymore as the default context should already serve
this purpose.

As a double cautionary measure we check the CCID to make sure everything
looks kosher, and don't enable RC6 if it doesn't.

There is an important difference in logic when switching to the context
interface. The old code use MI_SUSPEND_FLUSH before MI_SET_CONTEXT which was an
ILK specific workaround I remember seeing in old docs but can no longer find.
That workaround is not implemented in the standard context code.

PS. I think there is a double mutex_unlock in the existing error path

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    8 +--
 drivers/gpu/drm/i915/i915_drv.h      |    3 +-
 drivers/gpu/drm/i915/i915_reg.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   89 +++-------------------------------
 4 files changed, 10 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fdb7cce..6c98d18 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1372,13 +1372,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
 
 	if (dev_priv->pwrctx) {
 		seq_printf(m, "power context ");
-		describe_obj(m, dev_priv->pwrctx);
-		seq_printf(m, "\n");
-	}
-
-	if (dev_priv->renderctx) {
-		seq_printf(m, "render context ");
-		describe_obj(m, dev_priv->renderctx);
+		describe_obj(m, dev_priv->pwrctx->obj);
 		seq_printf(m, "\n");
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 003b62e..1329b1f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -352,8 +352,7 @@ typedef struct drm_i915_private {
 	drm_dma_handle_t *status_page_dmah;
 	uint32_t counter;
 	drm_local_map_t hws_map;
-	struct drm_i915_gem_object *pwrctx;
-	struct drm_i915_gem_object *renderctx;
+	struct i915_hw_context *pwrctx;
 
 	struct resource mch_res;
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6b6d685..4965638 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1360,6 +1360,7 @@
  * Logical Context regs
  */
 #define CCID			0x2180
+#define CCID_MASK		0xfffff000
 #define   CCID_EN		(1<<0)
 #define CXT_SIZE		0x21a0
 #define GEN6_CXT_POWER_SIZE(cxt_reg)	((cxt_reg >> 24) & 0x3f)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index de1ba19..4ef968a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7948,42 +7948,6 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
 	.output_poll_changed = intel_fb_output_poll_changed,
 };
 
-static struct drm_i915_gem_object *
-intel_alloc_context_page(struct drm_device *dev)
-{
-	struct drm_i915_gem_object *ctx;
-	int ret;
-
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
-	ctx = i915_gem_alloc_object(dev, 4096);
-	if (!ctx) {
-		DRM_DEBUG("failed to alloc power context, RC6 disabled\n");
-		return NULL;
-	}
-
-	ret = i915_gem_object_pin(ctx, 4096, true);
-	if (ret) {
-		DRM_ERROR("failed to pin power context: %d\n", ret);
-		goto err_unref;
-	}
-
-	ret = i915_gem_object_set_to_gtt_domain(ctx, 1);
-	if (ret) {
-		DRM_ERROR("failed to set-domain on power context: %d\n", ret);
-		goto err_unpin;
-	}
-
-	return ctx;
-
-err_unpin:
-	i915_gem_object_unpin(ctx);
-err_unref:
-	drm_gem_object_unreference(&ctx->base);
-	mutex_unlock(&dev->struct_mutex);
-	return NULL;
-}
-
 bool ironlake_set_drps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -8667,15 +8631,8 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->renderctx) {
-		i915_gem_object_unpin(dev_priv->renderctx);
-		drm_gem_object_unreference(&dev_priv->renderctx->base);
-		dev_priv->renderctx = NULL;
-	}
-
 	if (dev_priv->pwrctx) {
-		i915_gem_object_unpin(dev_priv->pwrctx);
-		drm_gem_object_unreference(&dev_priv->pwrctx->base);
+		i915_context_destroy_anonymous(dev_priv->pwrctx);
 		dev_priv->pwrctx = NULL;
 	}
 }
@@ -8704,13 +8661,11 @@ static int ironlake_setup_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->renderctx == NULL)
-		dev_priv->renderctx = intel_alloc_context_page(dev);
-	if (!dev_priv->renderctx)
-		return -ENOMEM;
+	if (dev_priv->ring[RCS].default_context == NULL)
+		return -EIO;
 
 	if (dev_priv->pwrctx == NULL)
-		dev_priv->pwrctx = intel_alloc_context_page(dev);
+		dev_priv->pwrctx = i915_context_alloc_anonymous(dev);
 	if (!dev_priv->pwrctx) {
 		ironlake_teardown_rc6(dev);
 		return -ENOMEM;
@@ -8737,43 +8692,13 @@ void ironlake_enable_rc6(struct drm_device *dev)
 		return;
 	}
 
-	/*
-	 * GPU can automatically power down the render unit if given a page
-	 * to save state.
-	 */
-	ret = BEGIN_LP_RING(6);
-	if (ret) {
-		ironlake_teardown_rc6(dev);
-		mutex_unlock(&dev->struct_mutex);
-		return;
-	}
-
-	OUT_RING(MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
-	OUT_RING(MI_SET_CONTEXT);
-	OUT_RING(dev_priv->renderctx->gtt_offset |
-		 MI_MM_SPACE_GTT |
-		 MI_SAVE_EXT_STATE_EN |
-		 MI_RESTORE_EXT_STATE_EN |
-		 MI_RESTORE_INHIBIT);
-	OUT_RING(MI_SUSPEND_FLUSH);
-	OUT_RING(MI_NOOP);
-	OUT_RING(MI_FLUSH);
-	ADVANCE_LP_RING();
-
-	/*
-	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
-	 * does an implicit flush, combined with MI_FLUSH above, it should be
-	 * safe to assume that renderctx is valid
-	 */
-	ret = intel_wait_ring_idle(LP_RING(dev_priv));
-	if (ret) {
-		DRM_ERROR("failed to enable ironlake power power savings\n");
-		ironlake_teardown_rc6(dev);
+	if ((I915_READ(CCID) & CCID_MASK) == 0) {
+		DRM_ERROR("RC6 could not be enabled\n");
 		mutex_unlock(&dev->struct_mutex);
 		return;
 	}
 
-	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
+	I915_WRITE(PWRCTXA, dev_priv->pwrctx->obj->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 	mutex_unlock(&dev->struct_mutex);
 }
-- 
1.7.9.4

Re: [PATCH 17/18] drm/i915: Ironlake rc6 can use context interfaces

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:57PM -0700, Ben Widawsky wrote:
> Use the context interfaces to create the power context.  Assuming we
> have a default context, there should be no need to switch to
> the render context anymore as the default context should already serve
> this purpose.
> 
> As a double cautionary measure we check the CCID to make sure everything
> looks kosher, and don't enable RC6 if it doesn't.
> 
> There is an important difference in logic when switching to the context
> interface. The old code use MI_SUSPEND_FLUSH before MI_SET_CONTEXT which was an
> ILK specific workaround I remember seeing in old docs but can no longer find.
> That workaround is not implemented in the standard context code.
> 
> PS. I think there is a double mutex_unlock in the existing error path
> 
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

As I've said in the previous patch review, I don't think it makes much
sense to convert the powercontext to your context code. Afaics the word
context is the only common thing here.

The reaping of the add-hoc renderctx in here is rather nice though. But I
expected this to go hand-in-hand with enabling your context code for gen5
(so that this gets replaced with the default context).

I've been pretty confused when trying to figure out how your code decides
whether it should enable hw context, until I've noticed that you return -1
in get_context_size. Can we make this slightly more explicit, with e.g.
not even attempting to init hw contexts for gen < 5/6? HAS_HW_CONTEXT
macro that checks the gen would be nice ...
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |    8 +--
>  drivers/gpu/drm/i915/i915_drv.h      |    3 +-
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   89 +++-------------------------------
>  4 files changed, 10 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fdb7cce..6c98d18 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1372,13 +1372,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  
>  	if (dev_priv->pwrctx) {
>  		seq_printf(m, "power context ");
> -		describe_obj(m, dev_priv->pwrctx);
> -		seq_printf(m, "\n");
> -	}
> -
> -	if (dev_priv->renderctx) {
> -		seq_printf(m, "render context ");
> -		describe_obj(m, dev_priv->renderctx);
> +		describe_obj(m, dev_priv->pwrctx->obj);
>  		seq_printf(m, "\n");
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 003b62e..1329b1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -352,8 +352,7 @@ typedef struct drm_i915_private {
>  	drm_dma_handle_t *status_page_dmah;
>  	uint32_t counter;
>  	drm_local_map_t hws_map;
> -	struct drm_i915_gem_object *pwrctx;
> -	struct drm_i915_gem_object *renderctx;
> +	struct i915_hw_context *pwrctx;
>  
>  	struct resource mch_res;
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6b6d685..4965638 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1360,6 +1360,7 @@
>   * Logical Context regs
>   */
>  #define CCID			0x2180
> +#define CCID_MASK		0xfffff000
>  #define   CCID_EN		(1<<0)
>  #define CXT_SIZE		0x21a0
>  #define GEN6_CXT_POWER_SIZE(cxt_reg)	((cxt_reg >> 24) & 0x3f)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index de1ba19..4ef968a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7948,42 +7948,6 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
>  	.output_poll_changed = intel_fb_output_poll_changed,
>  };
>  
> -static struct drm_i915_gem_object *
> -intel_alloc_context_page(struct drm_device *dev)
> -{
> -	struct drm_i915_gem_object *ctx;
> -	int ret;
> -
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> -	ctx = i915_gem_alloc_object(dev, 4096);
> -	if (!ctx) {
> -		DRM_DEBUG("failed to alloc power context, RC6 disabled\n");
> -		return NULL;
> -	}
> -
> -	ret = i915_gem_object_pin(ctx, 4096, true);
> -	if (ret) {
> -		DRM_ERROR("failed to pin power context: %d\n", ret);
> -		goto err_unref;
> -	}
> -
> -	ret = i915_gem_object_set_to_gtt_domain(ctx, 1);
> -	if (ret) {
> -		DRM_ERROR("failed to set-domain on power context: %d\n", ret);
> -		goto err_unpin;
> -	}
> -
> -	return ctx;
> -
> -err_unpin:
> -	i915_gem_object_unpin(ctx);
> -err_unref:
> -	drm_gem_object_unreference(&ctx->base);
> -	mutex_unlock(&dev->struct_mutex);
> -	return NULL;
> -}
> -
>  bool ironlake_set_drps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -8667,15 +8631,8 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (dev_priv->renderctx) {
> -		i915_gem_object_unpin(dev_priv->renderctx);
> -		drm_gem_object_unreference(&dev_priv->renderctx->base);
> -		dev_priv->renderctx = NULL;
> -	}
> -
>  	if (dev_priv->pwrctx) {
> -		i915_gem_object_unpin(dev_priv->pwrctx);
> -		drm_gem_object_unreference(&dev_priv->pwrctx->base);
> +		i915_context_destroy_anonymous(dev_priv->pwrctx);
>  		dev_priv->pwrctx = NULL;
>  	}
>  }
> @@ -8704,13 +8661,11 @@ static int ironlake_setup_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (dev_priv->renderctx == NULL)
> -		dev_priv->renderctx = intel_alloc_context_page(dev);
> -	if (!dev_priv->renderctx)
> -		return -ENOMEM;
> +	if (dev_priv->ring[RCS].default_context == NULL)
> +		return -EIO;
>  
>  	if (dev_priv->pwrctx == NULL)
> -		dev_priv->pwrctx = intel_alloc_context_page(dev);
> +		dev_priv->pwrctx = i915_context_alloc_anonymous(dev);
>  	if (!dev_priv->pwrctx) {
>  		ironlake_teardown_rc6(dev);
>  		return -ENOMEM;
> @@ -8737,43 +8692,13 @@ void ironlake_enable_rc6(struct drm_device *dev)
>  		return;
>  	}
>  
> -	/*
> -	 * GPU can automatically power down the render unit if given a page
> -	 * to save state.
> -	 */
> -	ret = BEGIN_LP_RING(6);
> -	if (ret) {
> -		ironlake_teardown_rc6(dev);
> -		mutex_unlock(&dev->struct_mutex);
> -		return;
> -	}
> -
> -	OUT_RING(MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> -	OUT_RING(MI_SET_CONTEXT);
> -	OUT_RING(dev_priv->renderctx->gtt_offset |
> -		 MI_MM_SPACE_GTT |
> -		 MI_SAVE_EXT_STATE_EN |
> -		 MI_RESTORE_EXT_STATE_EN |
> -		 MI_RESTORE_INHIBIT);
> -	OUT_RING(MI_SUSPEND_FLUSH);
> -	OUT_RING(MI_NOOP);
> -	OUT_RING(MI_FLUSH);
> -	ADVANCE_LP_RING();
> -
> -	/*
> -	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
> -	 * does an implicit flush, combined with MI_FLUSH above, it should be
> -	 * safe to assume that renderctx is valid
> -	 */
> -	ret = intel_wait_ring_idle(LP_RING(dev_priv));
> -	if (ret) {
> -		DRM_ERROR("failed to enable ironlake power power savings\n");
> -		ironlake_teardown_rc6(dev);
> +	if ((I915_READ(CCID) & CCID_MASK) == 0) {
> +		DRM_ERROR("RC6 could not be enabled\n");
>  		mutex_unlock(&dev->struct_mutex);
>  		return;
>  	}
>  
> -	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
> +	I915_WRITE(PWRCTXA, dev_priv->pwrctx->obj->gtt_offset | PWRCTX_EN);
>  	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
>  	mutex_unlock(&dev->struct_mutex);
>  }
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 18/18] drm/i915: try to enable rc6 on Ironlake... again

[Ben Widawsky]

In some off chance the HW context code and new error check fixes things
magically..

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4ef968a..52959bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8178,12 +8178,6 @@ static bool intel_enable_rc6(struct drm_device *dev)
 		return i915_enable_rc6;
 
 	/*
-	 * Disable RC6 on Ironlake
-	 */
-	if (INTEL_INFO(dev)->gen == 5)
-		return 0;
-
-	/*
 	 * Disable rc6 on Sandybridge
 	 */
 	if (INTEL_INFO(dev)->gen == 6) {
-- 
1.7.9.4

Re: [PATCH 00/18] i915 HW Context Support

[Ben Widawsky]

On Sun, Mar 18, 2012 at 01:39:40PM -0700, Ben Widawsky wrote:
> The patches have changed quite a bit since the RFC, and therefore I
> didn't feel comfortable trying to do v2 information. I didn't feel
> comfortable taking the few r-bs that I had from the RFC except for the
> one patch that I applied wholesale.
> 
> Summary:
> - Completely redid the patch splitting.
>   The number of patches increased, but LOC is about the same, and a
>   handful of the new patches are either because of more splitting, or
>   completely new bits.
> - Reference counted context allows freeing the data structure and
>   freeing the BO independently. This is probably the most significant
>   change.
> - Convert ILK RC6 code to use context code. I'm hopefuly this will make
>   things more stable, but have no proof.
> - Added trace events for context create/destroy/switch.
> - Only support render ring context switch (previous code supported any
>   ring, though media ring is the only other ring which *should* work).
> 
> Testing summary.
> ILK
>   RC6, just booted to desktop
> SNB
>   module load/unload testing
>   20 consecutive suspend resume cycles
>   nexuiz with experimental mesa
>   piglit quick.tests with experimental mesa
>     I've seen time-elapsed, and polygonOffset intermittently fail, I
>     believe this is caused by the following...
>   Missed IRQs now seem to occur once every other piglit run.
>     I have some new code to try to fix this... coming soon, I hope.
> IVB
>   No testing done since RFC
> 
> I'll respond to this email with links to what I used to test (code is
> currently not in pushable form).
> kernel
> libdrm
> mesa
> intel-gpu-tools

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

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

mesa:
git://people.freedesktop.org/~bwidawsk/mesa context_support

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

Re: [PATCH 00/18] i915 HW Context Support

[Daniel Vetter]

On Sun, Mar 18, 2012 at 01:39:40PM -0700, Ben Widawsky wrote:
> The patches have changed quite a bit since the RFC, and therefore I
> didn't feel comfortable trying to do v2 information. I didn't feel
> comfortable taking the few r-bs that I had from the RFC except for the
> one patch that I applied wholesale.
> 
> Summary:
> - Completely redid the patch splitting.

I've only done a quick and cursory reading, but I like the new splitting
_much_ more. The storyline behind these patches is now much clearer. I'll
try to do a more in-depth review later this week.

Cheers, Daniel

>   The number of patches increased, but LOC is about the same, and a
>   handful of the new patches are either because of more splitting, or
>   completely new bits.
> - Reference counted context allows freeing the data structure and
>   freeing the BO independently. This is probably the most significant
>   change.
> - Convert ILK RC6 code to use context code. I'm hopefuly this will make
>   things more stable, but have no proof.
> - Added trace events for context create/destroy/switch.
> - Only support render ring context switch (previous code supported any
>   ring, though media ring is the only other ring which *should* work).
> 
> Testing summary.
> ILK
>   RC6, just booted to desktop
> SNB
>   module load/unload testing
>   20 consecutive suspend resume cycles
>   nexuiz with experimental mesa
>   piglit quick.tests with experimental mesa
>     I've seen time-elapsed, and polygonOffset intermittently fail, I
>     believe this is caused by the following...
>   Missed IRQs now seem to occur once every other piglit run.
>     I have some new code to try to fix this... coming soon, I hope.
> IVB
>   No testing done since RFC
> 
> I'll respond to this email with links to what I used to test (code is
> currently not in pushable form).
> kernel
> libdrm
> mesa
> intel-gpu-tools
> 
> Thanks to Chris Wilson, Daniel Vetter, and Eric Anholt for providing
> useful feedback in the RFC.
> 
> Ben Widawsky (18):
>   drm/i915: CXT_SIZE register offsets added
>   drm/i915: preliminary context support
>   drm/i915: context basic create & destroy
>   drm/i915: add context information to objects
>   drm/i915: context switch implementation
>   drm/i915: trace events for contexts
>   drm/i915: Ivybridge MI_ARB_ON_OFF context w/a
>   drm/i915: PIPE_CONTROL_TLB_INVALIDATE
>   drm/i915: possibly invalidate TLB before context switch
>   drm/i915: use the default context
>   drm/i915: switch to default context on idle
>   drm/i915: try to reset the gpu before unload
>   drm/i915/context: create & destroy ioctls
>   drm/i915/context: switch contexts with execbuf2
>   drm/i915/context: add params
>   drm/i915/context: anonymous context interfaces
>   drm/i915: Ironlake rc6 can use context interfaces
>   drm/i915: try to enable rc6 on Ironlake... again
> 
>  drivers/gpu/drm/i915/Makefile              |    1 +
>  drivers/gpu/drm/i915/i915_debugfs.c        |    8 +-
>  drivers/gpu/drm/i915/i915_dma.c            |    9 +
>  drivers/gpu/drm/i915/i915_drv.c            |    1 +
>  drivers/gpu/drm/i915/i915_drv.h            |   43 ++-
>  drivers/gpu/drm/i915/i915_gem.c            |   11 +
>  drivers/gpu/drm/i915/i915_gem_context.c    |  513 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +
>  drivers/gpu/drm/i915/i915_reg.h            |   26 ++
>  drivers/gpu/drm/i915/i915_trace.h          |   59 ++++
>  drivers/gpu/drm/i915/intel_display.c       |   95 +-----
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |   49 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +
>  include/drm/i915_drm.h                     |   21 +-
>  14 files changed, 755 insertions(+), 98 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_context.c
> 
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 00/18] i915 HW Context Support

[Daniel Vetter]

On Mon, Mar 19, 2012 at 11:14:32AM +0100, Daniel Vetter wrote:
> On Sun, Mar 18, 2012 at 01:39:40PM -0700, Ben Widawsky wrote:
> > The patches have changed quite a bit since the RFC, and therefore I
> > didn't feel comfortable trying to do v2 information. I didn't feel
> > comfortable taking the few r-bs that I had from the RFC except for the
> > one patch that I applied wholesale.
> > 
> > Summary:
> > - Completely redid the patch splitting.
> 
> I've only done a quick and cursory reading, but I like the new splitting
> _much_ more. The storyline behind these patches is now much clearer. I'll
> try to do a more in-depth review later this week.

Ok, I've gone through it and noticed a few things - mostly stuff that are
imo more complicated than necessary and that could be cut out.

Safe for the tlb flush wa I haven't cross-checked anything with Bspec, but
I don't expect any surprises there. I also haven't checked how good the
test coverage is (safe for suggesting that one test for execbuf failure
handling). But again, that's something which can be easily fixed.

The last thing I'm wondering is: How ready is mesa for this? I'd like to
merge this only when the mesa patches are ready to put it to good use.
Otherwise we run the decent risk of shipping broken code, which could end
up in a decent pain for userspace (worst case we have to add a new flag to
claim 'fixed context support').

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