[PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Ben Widawsky]

It makes all the code which calls into this function way too confusing.

v2: Fix destroy IOCTL as well

v3: Clarify the other two callers of i915_gem_context_get() to never
check for NULL. (Mika)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c    | 12 +++++++++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ebe0f67..44dddc00 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -526,10 +526,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
+	struct i915_hw_context *ctx;
+
 	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
 		return file_priv->private_default_ctx;
 
-	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	if (!ctx)
+		return ERR_PTR(-ENOENT);
+
+	return ctx;
 }
 
 static inline int
@@ -776,9 +782,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return ret;
 
 	ctx = i915_gem_context_get(file_priv, args->ctx_id);
-	if (!ctx) {
+	if (IS_ERR(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
-		return -ENOENT;
+		return PTR_ERR(ctx);
 	}
 
 	idr_remove(&ctx->file_priv->context_idr, ctx->id);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index bbff8f9..c30a6ee 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -916,7 +916,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
 		return ERR_PTR(-EINVAL);
 
 	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
-	if (IS_ERR_OR_NULL(ctx))
+	if (IS_ERR(ctx))
 		return ctx;
 
 	hs = &ctx->hang_stats;
@@ -1126,7 +1126,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	ctx = i915_gem_validate_context(dev, file, ring, ctx_id);
-	if (IS_ERR_OR_NULL(ctx)) {
+	if (IS_ERR(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = PTR_ERR(ctx);
 		goto pre_mutex_err;
-- 
1.8.5.2

[PATCH 2/3] drm/i915: set ctx->initialized only after RCS

[Ben Widawsky]

The initialized flag is used to specify a context has been initialized
and it's context is safe to load, ie. the 3d state is setup properly.
With full PPGTT, we emit the address space loads during context switch
and this currently marks a context as initialized. With full PPGTT
patches, if a client first emits a batch to !RCS, then later, RCS, the
code will mistake the context as initialized and try to reload an
uninitialized context.

1. context 1 blit // context marked as initialized
2. context 1 render // loads random state

It is really easy to hit this with a planned upcoming patch which makes
default context reuse possible. See the mailing list thread in
references for more details about the issue.

Thanks to Chris for helping me track this down.

References: <1388266309-1178-1-git-send-email-benjamin.widawsky@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 44dddc00..8c7a5b2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -692,10 +692,11 @@ static int do_switch(struct intel_ring_buffer *ring,
 		i915_gem_context_unreference(from);
 	}
 
+	to->is_initialized = true;
+
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
-	to->is_initialized = true;
 	to->last_ring = ring;
 
 	return 0;
-- 
1.8.5.2

[PATCH 3/3] drm/i915: Make file default context persistent

[Ben Widawsky]

With full PPGTT and per file default contexts it no longer makes sense
to inhibit saving and restoring those hw contexts. The /real/ default
context is system wide and still benefits from not saving/restoring.

The upshot of this patch is that with a simple param, userspace will be
able to avoid allocating a new context and simply use the default
context instead.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8c7a5b2..d12d1a0 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -230,7 +230,7 @@ err_out:
 
 static inline bool is_default_context(struct i915_hw_context *ctx)
 {
-	return (ctx->id == DEFAULT_CONTEXT_ID);
+	return (ctx->file_priv == NULL);
 }
 
 /**
@@ -474,8 +474,10 @@ static int context_idr_cleanup(int id, void *p, void *data)
 	struct i915_hw_context *ctx = p;
 
 	/* Ignore the default context because close will handle it */
-	if (is_default_context(ctx))
+	if (ctx->id == DEFAULT_CONTEXT_ID) {
+		BUG_ON(!ctx->file_priv);
 		return 0;
+	}
 
 	i915_gem_context_unreference(ctx);
 	return 0;
-- 
1.8.5.2

[PATCH 1/5] intel: squash unused variable 'bo_gem'

[Ben Widawsky]

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

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 48ff62e..3b1f584 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1337,7 +1337,9 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
 int drm_intel_gem_bo_map_unsynchronized(drm_intel_bo *bo)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+#ifdef HAVE_VALGRIND
 	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+#endif
 	int ret;
 
 	/* If the CPU cache isn't coherent with the GTT, then use a
-- 
1.8.5.2

[PATCH 2/5] intel: Handle malloc fails in context create

[Ben Widawsky]

The previous code would just use the potentially unallocated variable,
which is probably okay most of the time, but not very nice to the user
of the library.

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

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 3b1f584..ad722dd 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3020,15 +3020,19 @@ drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr)
 	drm_intel_context *context = NULL;
 	int ret;
 
+	context = calloc(1, sizeof(*context));
+	if (!context)
+		return NULL;
+
 	VG_CLEAR(create);
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create);
 	if (ret != 0) {
 		DBG("DRM_IOCTL_I915_GEM_CONTEXT_CREATE failed: %s\n",
 		    strerror(errno));
+		free(context);
 		return NULL;
 	}
 
-	context = calloc(1, sizeof(*context));
 	context->ctx_id = create.ctx_id;
 	context->bufmgr = bufmgr;
 
-- 
1.8.5.2

[PATCH 3/5] intel: Merge latest i915_drm.h

[Ben Widawsky]

This was not done as a straight copy because reset_stats IOCTL landed in libdrm
before upstream kernel. (We'll do a similar thing for full PPGTT anyway, so
there isn't really a point in syncing exactly).

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

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index c1914d6..2f4eb8c 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -27,12 +27,36 @@
 #ifndef _I915_DRM_H_
 #define _I915_DRM_H_
 
-#include "drm.h"
+#include <drm.h>
 
 /* Please note that modifications to all structs defined here are
  * subject to backwards-compatibility constraints.
  */
 
+/**
+ * DOC: uevents generated by i915 on it's device node
+ *
+ * I915_L3_PARITY_UEVENT - Generated when the driver receives a parity mismatch
+ *	event from the gpu l3 cache. Additional information supplied is ROW,
+ *	BANK, SUBBANK, SLICE of the affected cacheline. Userspace should keep
+ *	track of these events and if a specific cache-line seems to have a
+ *	persistent error remap it with the l3 remapping tool supplied in
+ *	intel-gpu-tools.  The value supplied with the event is always 1.
+ *
+ * I915_ERROR_UEVENT - Generated upon error detection, currently only via
+ *	hangcheck. The error detection event is a good indicator of when things
+ *	began to go badly. The value supplied with the event is a 1 upon error
+ *	detection, and a 0 upon reset completion, signifying no more error
+ *	exists. NOTE: Disabling hangcheck or reset via module parameter will
+ *	cause the related events to not be seen.
+ *
+ * I915_RESET_UEVENT - Event is generated just before an attempt to reset the
+ *	the GPU. The value supplied with the event is always 1. NOTE: Disable
+ *	reset via module parameter will cause this event to not be seen.
+ */
+#define I915_L3_PARITY_UEVENT		"L3_PARITY_ERROR"
+#define I915_ERROR_UEVENT		"ERROR"
+#define I915_RESET_UEVENT		"RESET"
 
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
@@ -195,8 +219,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_WAIT	0x2c
 #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
 #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
-#define DRM_I915_GEM_SET_CACHEING	0x2f
-#define DRM_I915_GEM_GET_CACHEING	0x30
+#define DRM_I915_GEM_SET_CACHING	0x2f
+#define DRM_I915_GEM_GET_CACHING	0x30
 #define DRM_I915_REG_READ		0x31
 #define DRM_I915_GET_RESET_STATS	0x32
 
@@ -223,8 +247,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_PIN		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_PIN, struct drm_i915_gem_pin)
 #define DRM_IOCTL_I915_GEM_UNPIN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_UNPIN, struct drm_i915_gem_unpin)
 #define DRM_IOCTL_I915_GEM_BUSY		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_BUSY, struct drm_i915_gem_busy)
-#define DRM_IOCTL_I915_GEM_SET_CACHEING		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_SET_CACHEING, struct drm_i915_gem_cacheing)
-#define DRM_IOCTL_I915_GEM_GET_CACHEING		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_GET_CACHEING, struct drm_i915_gem_cacheing)
+#define DRM_IOCTL_I915_GEM_SET_CACHING		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_SET_CACHING, struct drm_i915_gem_caching)
+#define DRM_IOCTL_I915_GEM_GET_CACHING		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_GET_CACHING, struct drm_i915_gem_caching)
 #define DRM_IOCTL_I915_GEM_THROTTLE	DRM_IO ( DRM_COMMAND_BASE + DRM_I915_GEM_THROTTLE)
 #define DRM_IOCTL_I915_GEM_ENTERVT	DRM_IO(DRM_COMMAND_BASE + DRM_I915_GEM_ENTERVT)
 #define DRM_IOCTL_I915_GEM_LEAVEVT	DRM_IO(DRM_COMMAND_BASE + DRM_I915_GEM_LEAVEVT)
@@ -305,7 +329,14 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_LLC     	 	 17
 #define I915_PARAM_HAS_ALIASING_PPGTT	 18
 #define I915_PARAM_HAS_WAIT_TIMEOUT	 19
-#define I915_PARAM_HAS_VEBOX            22
+#define I915_PARAM_HAS_SEMAPHORES	 20
+#define I915_PARAM_HAS_PRIME_VMAP_FLUSH	 21
+#define I915_PARAM_HAS_VEBOX		 22
+#define I915_PARAM_HAS_SECURE_BATCHES	 23
+#define I915_PARAM_HAS_PINNED_BATCHES	 24
+#define I915_PARAM_HAS_EXEC_NO_RELOC	 25
+#define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
+#define I915_PARAM_HAS_WT     	 	 27
 
 typedef struct drm_i915_getparam {
 	int param;
@@ -626,7 +657,11 @@ struct drm_i915_gem_exec_object2 {
 	__u64 offset;
 
 #define EXEC_OBJECT_NEEDS_FENCE (1<<0)
+#define EXEC_OBJECT_NEEDS_GTT	(1<<1)
+#define EXEC_OBJECT_WRITE	(1<<2)
+#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_WRITE<<1)
 	__u64 flags;
+
 	__u64 rsvd1;
 	__u64 rsvd2;
 };
@@ -672,6 +707,34 @@ struct drm_i915_gem_execbuffer2 {
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
 
+/** Request a privileged ("secure") batch buffer. Note only available for
+ * DRM_ROOT_ONLY | DRM_MASTER processes.
+ */
+#define I915_EXEC_SECURE		(1<<9)
+
+/** Inform the kernel that the batch is and will always be pinned. This
+ * negates the requirement for a workaround to be performed to avoid
+ * an incoherent CS (such as can be found on 830/845). If this flag is
+ * not passed, the kernel will endeavour to make sure the batch is
+ * coherent with the CS before execution. If this flag is passed,
+ * userspace assumes the responsibility for ensuring the same.
+ */
+#define I915_EXEC_IS_PINNED		(1<<10)
+
+/** Provide a hint to the kernel that the command stream and auxilliary
+ * state buffers already holds the correct presumed addresses and so the
+ * relocation process may be skipped if no buffers need to be moved in
+ * preparation for the execbuffer.
+ */
+#define I915_EXEC_NO_RELOC		(1<<11)
+
+/** Use the reloc.handle as an index into the exec object array rather
+ * than as the per-file handle.
+ */
+#define I915_EXEC_HANDLE_LUT		(1<<12)
+
+#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
+
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
 	(eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
@@ -708,21 +771,45 @@ struct drm_i915_gem_busy {
 	__u32 busy;
 };
 
-#define I915_CACHEING_NONE		0
-#define I915_CACHEING_CACHED		1
+/**
+ * I915_CACHING_NONE
+ *
+ * GPU access is not coherent with cpu caches. Default for machines without an
+ * LLC.
+ */
+#define I915_CACHING_NONE		0
+/**
+ * I915_CACHING_CACHED
+ *
+ * GPU access is coherent with cpu caches and furthermore the data is cached in
+ * last-level caches shared between cpu cores and the gpu GT. Default on
+ * machines with HAS_LLC.
+ */
+#define I915_CACHING_CACHED		1
+/**
+ * I915_CACHING_DISPLAY
+ *
+ * Special GPU caching mode which is coherent with the scanout engines.
+ * Transparently falls back to I915_CACHING_NONE on platforms where no special
+ * cache mode (like write-through or gfdt flushing) is available. The kernel
+ * automatically sets this mode when using a buffer as a scanout target.
+ * Userspace can manually set this mode to avoid a costly stall and clflush in
+ * the hotpath of drawing the first frame.
+ */
+#define I915_CACHING_DISPLAY		2
 
-struct drm_i915_gem_cacheing {
+struct drm_i915_gem_caching {
 	/**
-	 * Handle of the buffer to set/get the cacheing level of. */
+	 * Handle of the buffer to set/get the caching level of. */
 	__u32 handle;
 
 	/**
 	 * Cacheing level to apply or return value
 	 *
-	 * bits0-15 are for generic cacheing control (i.e. the above defined
+	 * bits0-15 are for generic caching control (i.e. the above defined
 	 * values). bits16-31 are reserved for platform-specific variations
 	 * (e.g. l3$ caching on gen7). */
-	__u32 cacheing;
+	__u32 caching;
 };
 
 #define I915_TILING_NONE	0
@@ -962,4 +1049,4 @@ struct drm_i915_reset_stats {
 	__u32 pad;
 };
 
-#endif				/* _I915_DRM_H_ */
+#endif /* _I915_DRM_H_ */
-- 
1.8.5.2

[PATCH 4/5] intel: Intel full PPGTT param

[Ben Widawsky]

This will allow mesa to determine if it needs to create a context, or
can reuse the default context. Reusing the default context saves memory,
and startup time.

To keep the libdrm interface as dumb as possible, we simply expose a
getter for the default context (which is only a real context when
thereis full PPGTT and per fd contexts). As such, callers can expect
NULL when this is not the case.

See the usage in mesa for details.

Cc: Kenneth Graunke <kenneth.w.graunke@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 include/drm/i915_drm.h    |  1 +
 intel/intel_bufmgr.h      |  1 +
 intel/intel_bufmgr_gem.c  | 30 ++++++++++++++++++++++++++++++
 intel/intel_bufmgr_priv.h |  1 +
 4 files changed, 33 insertions(+)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 2f4eb8c..50b080e 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -337,6 +337,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_EXEC_NO_RELOC	 25
 #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
 #define I915_PARAM_HAS_WT     	 	 27
+#define I915_PARAM_HAS_FULL_PPGTT	 28
 
 typedef struct drm_i915_getparam {
 	int param;
diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 2eb9742..7f08093 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -191,6 +191,7 @@ int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
 int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);
 
 drm_intel_context *drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr);
+drm_intel_context * drm_intel_gem_default_context_get(drm_intel_bufmgr *bufmgr);
 void drm_intel_gem_context_destroy(drm_intel_context *ctx);
 int drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
 				  int used, unsigned int flags);
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index ad722dd..dbd333a 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -126,6 +126,7 @@ typedef struct _drm_intel_bufmgr_gem {
 	unsigned int bo_reuse : 1;
 	unsigned int no_exec : 1;
 	unsigned int has_vebox : 1;
+	unsigned int has_full_ppgtt : 1;
 	bool fenced_relocs;
 
 	char *aub_filename;
@@ -3039,6 +3040,26 @@ drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr)
 	return context;
 }
 
+drm_intel_context *
+drm_intel_gem_default_context_get(drm_intel_bufmgr *bufmgr)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
+	drm_intel_context *context = NULL;
+
+	if (!bufmgr_gem->has_full_ppgtt)
+		return NULL;
+
+	context = calloc(1, sizeof(*context));
+	if (!context)
+		return NULL;
+
+	context->ctx_id = 0;
+	context->bufmgr = bufmgr;
+	context->is_default = 1;
+
+	return context;
+}
+
 void
 drm_intel_gem_context_destroy(drm_intel_context *ctx)
 {
@@ -3049,6 +3070,11 @@ drm_intel_gem_context_destroy(drm_intel_context *ctx)
 	if (ctx == NULL)
 		return;
 
+	if (ctx->is_default) {
+		free(ctx);
+		return;
+	}
+
 	VG_CLEAR(destroy);
 
 	bufmgr_gem = (drm_intel_bufmgr_gem *)ctx->bufmgr;
@@ -3267,6 +3293,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
 	bufmgr_gem->has_vebox = (ret == 0) & (*gp.value > 0);
 
+	gp.param = I915_PARAM_HAS_FULL_PPGTT;
+	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	bufmgr_gem->has_full_ppgtt = (ret == 0) & (*gp.value > 0);
+
 	if (bufmgr_gem->gen < 4) {
 		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
 		gp.value = &bufmgr_gem->available_fences;
diff --git a/intel/intel_bufmgr_priv.h b/intel/intel_bufmgr_priv.h
index 2592d42..e622746 100644
--- a/intel/intel_bufmgr_priv.h
+++ b/intel/intel_bufmgr_priv.h
@@ -283,6 +283,7 @@ struct _drm_intel_bufmgr {
 struct _drm_intel_context {
 	unsigned int ctx_id;
 	struct _drm_intel_bufmgr *bufmgr;
+	int is_default;
 };
 
 #define ALIGN(value, alignment)	((value + alignment - 1) & ~(alignment - 1))
-- 
1.8.5.2

[PATCH 5/5] configure.ac: bump version to 2.4.51 for release

[Ben Widawsky]

Provides the parameter for full PPGTT on Intel.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index c5b8d40..d0d051a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -20,7 +20,7 @@
 
 AC_PREREQ([2.63])
 AC_INIT([libdrm],
-        [2.4.50],
+        [2.4.51],
         [https://bugs.freedesktop.org/enter_bug.cgi?product=DRI],
         [libdrm])
 
-- 
1.8.5.2

[PATCH] i965: Use default contexts when possible.

[Ben Widawsky]

Will full PPGTT support it can be assumed that every file descriptor
gets its own hardware context. As such, there is no need to allocate
anew context in order to use the features provided by hardware contexts.
Eliminating this extra context allocation saves both physical memory
(currently PPGTT pages are pinned forever), GGTT space on IVB, and HSW,
as well as the associated startup cost of allocating the second context
- which can include an eviction in pathological cases.

Unfortunately, this requires a libdrm version bump as it requires a new
interface.

NOTE: I am uncertain if the share context idea is valid. Needs mesa
eyes.

Cc: Kenneth Graunke <kenneth.w.graunke@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 configure.ac                            | 2 +-
 src/mesa/drivers/dri/i965/brw_context.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f75325d..a0ae0b3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -29,7 +29,7 @@ AC_SUBST([OSMESA_VERSION])
 dnl Versions for external dependencies
 LIBDRM_REQUIRED=2.4.24
 LIBDRM_RADEON_REQUIRED=2.4.50
-LIBDRM_INTEL_REQUIRED=2.4.49
+LIBDRM_INTEL_REQUIRED=2.4.51
 LIBDRM_NVVIEUX_REQUIRED=2.4.33
 LIBDRM_NOUVEAU_REQUIRED="2.4.33 libdrm >= 2.4.41"
 LIBDRM_FREEDRENO_REQUIRED=2.4.39
diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
index 78c06fc..f5a66e1 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -674,7 +674,14 @@ brwCreateContext(gl_api api,
 
    intel_fbo_init(brw);
 
-   if (brw->gen >= 6) {
+   /* We assume that the hw_ctx is the first created for the file descriptor if
+    * this is not a share context. If that assumption turns false then we'll
+    * end up having hw_ctx for two contexts using the same actual context.
+    */
+   if (!shareCtx)
+      brw->hw_ctx = drm_intel_gem_default_context_get(brw->bufmgr);
+
+   if (!brw->hw_ctx && brw->gen >= 6) {
       /* Create a new hardware context.  Using a hardware context means that
        * our GPU state will be saved/restored on context switch, allowing us
        * to assume that the GPU is in the same state we left it in.
-- 
1.8.5.2

Re: [PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Mika Kuoppala]

Ben Widawsky <benjamin.widawsky@intel.com> writes:

> It makes all the code which calls into this function way too confusing.
>
> v2: Fix destroy IOCTL as well
>
> v3: Clarify the other two callers of i915_gem_context_get() to never
> check for NULL. (Mika)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  drivers/gpu/drm/i915/i915_gem_context.c    | 12 +++++++++---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 ++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ebe0f67..44dddc00 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -526,10 +526,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  struct i915_hw_context *
>  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  {
> +	struct i915_hw_context *ctx;
> +
>  	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
>  		return file_priv->private_default_ctx;
>  
> -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> +	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> +	if (!ctx)
> +		return ERR_PTR(-ENOENT);
> +
> +	return ctx;
>  }
>  
>  static inline int
> @@ -776,9 +782,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  		return ret;
>  
>  	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> -	if (!ctx) {
> +	if (IS_ERR(ctx)) {
>  		mutex_unlock(&dev->struct_mutex);
> -		return -ENOENT;
> +		return PTR_ERR(ctx);
>  	}
>  
>  	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index bbff8f9..c30a6ee 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -916,7 +916,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>  		return ERR_PTR(-EINVAL);
>  
>  	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
> -	if (IS_ERR_OR_NULL(ctx))
> +	if (IS_ERR(ctx))
>  		return ctx;
>  
>  	hs = &ctx->hang_stats;
> @@ -1126,7 +1126,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	}
>  
>  	ctx = i915_gem_validate_context(dev, file, ring, ctx_id);
> -	if (IS_ERR_OR_NULL(ctx)) {
> +	if (IS_ERR(ctx)) {
>  		mutex_unlock(&dev->struct_mutex);
>  		ret = PTR_ERR(ctx);
>  		goto pre_mutex_err;
> -- 
> 1.8.5.2

Re: [PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Daniel Vetter]

On Fri, Jan 03, 2014 at 02:07:47PM +0200, Mika Kuoppala wrote:
> Ben Widawsky <benjamin.widawsky@intel.com> writes:
> 
> > It makes all the code which calls into this function way too confusing.
> >
> > v2: Fix destroy IOCTL as well
> >
> > v3: Clarify the other two callers of i915_gem_context_get() to never
> > check for NULL. (Mika)
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Neither patch subject nor the commit message mention that this fixes a
bug. Also, the Testcase: line at the bottom is missing. I've fixed this
up.
-Daniel

> 
> >  drivers/gpu/drm/i915/i915_gem_context.c    | 12 +++++++++---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 ++--
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index ebe0f67..44dddc00 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -526,10 +526,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  struct i915_hw_context *
> >  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> >  {
> > +	struct i915_hw_context *ctx;
> > +
> >  	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
> >  		return file_priv->private_default_ctx;
> >  
> > -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> > +	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> > +	if (!ctx)
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	return ctx;
> >  }
> >  
> >  static inline int
> > @@ -776,9 +782,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> >  		return ret;
> >  
> >  	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > -	if (!ctx) {
> > +	if (IS_ERR(ctx)) {
> >  		mutex_unlock(&dev->struct_mutex);
> > -		return -ENOENT;
> > +		return PTR_ERR(ctx);
> >  	}
> >  
> >  	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index bbff8f9..c30a6ee 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -916,7 +916,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
> >  		return ERR_PTR(-EINVAL);
> >  
> >  	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
> > -	if (IS_ERR_OR_NULL(ctx))
> > +	if (IS_ERR(ctx))
> >  		return ctx;
> >  
> >  	hs = &ctx->hang_stats;
> > @@ -1126,7 +1126,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  	}
> >  
> >  	ctx = i915_gem_validate_context(dev, file, ring, ctx_id);
> > -	if (IS_ERR_OR_NULL(ctx)) {
> > +	if (IS_ERR(ctx)) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		ret = PTR_ERR(ctx);
> >  		goto pre_mutex_err;
> > -- 
> > 1.8.5.2
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] intel: Intel full PPGTT param

[Daniel Vetter]

On Thu, Jan 02, 2014 at 07:50:33PM -1000, Ben Widawsky wrote:
> This will allow mesa to determine if it needs to create a context, or
> can reuse the default context. Reusing the default context saves memory,
> and startup time.
> 
> To keep the libdrm interface as dumb as possible, we simply expose a
> getter for the default context (which is only a real context when
> thereis full PPGTT and per fd contexts). As such, callers can expect
> NULL when this is not the case.
> 
> See the usage in mesa for details.
> 
> Cc: Kenneth Graunke <kenneth.w.graunke@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Fyi I've killed this.
-Daniel

> ---
>  include/drm/i915_drm.h    |  1 +
>  intel/intel_bufmgr.h      |  1 +
>  intel/intel_bufmgr_gem.c  | 30 ++++++++++++++++++++++++++++++
>  intel/intel_bufmgr_priv.h |  1 +
>  4 files changed, 33 insertions(+)
> 
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 2f4eb8c..50b080e 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -337,6 +337,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_HAS_EXEC_NO_RELOC	 25
>  #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
>  #define I915_PARAM_HAS_WT     	 	 27
> +#define I915_PARAM_HAS_FULL_PPGTT	 28
>  
>  typedef struct drm_i915_getparam {
>  	int param;
> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> index 2eb9742..7f08093 100644
> --- a/intel/intel_bufmgr.h
> +++ b/intel/intel_bufmgr.h
> @@ -191,6 +191,7 @@ int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
>  int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);
>  
>  drm_intel_context *drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr);
> +drm_intel_context * drm_intel_gem_default_context_get(drm_intel_bufmgr *bufmgr);
>  void drm_intel_gem_context_destroy(drm_intel_context *ctx);
>  int drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
>  				  int used, unsigned int flags);
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index ad722dd..dbd333a 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -126,6 +126,7 @@ typedef struct _drm_intel_bufmgr_gem {
>  	unsigned int bo_reuse : 1;
>  	unsigned int no_exec : 1;
>  	unsigned int has_vebox : 1;
> +	unsigned int has_full_ppgtt : 1;
>  	bool fenced_relocs;
>  
>  	char *aub_filename;
> @@ -3039,6 +3040,26 @@ drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr)
>  	return context;
>  }
>  
> +drm_intel_context *
> +drm_intel_gem_default_context_get(drm_intel_bufmgr *bufmgr)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
> +	drm_intel_context *context = NULL;
> +
> +	if (!bufmgr_gem->has_full_ppgtt)
> +		return NULL;
> +
> +	context = calloc(1, sizeof(*context));
> +	if (!context)
> +		return NULL;
> +
> +	context->ctx_id = 0;
> +	context->bufmgr = bufmgr;
> +	context->is_default = 1;
> +
> +	return context;
> +}
> +
>  void
>  drm_intel_gem_context_destroy(drm_intel_context *ctx)
>  {
> @@ -3049,6 +3070,11 @@ drm_intel_gem_context_destroy(drm_intel_context *ctx)
>  	if (ctx == NULL)
>  		return;
>  
> +	if (ctx->is_default) {
> +		free(ctx);
> +		return;
> +	}
> +
>  	VG_CLEAR(destroy);
>  
>  	bufmgr_gem = (drm_intel_bufmgr_gem *)ctx->bufmgr;
> @@ -3267,6 +3293,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
>  	bufmgr_gem->has_vebox = (ret == 0) & (*gp.value > 0);
>  
> +	gp.param = I915_PARAM_HAS_FULL_PPGTT;
> +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	bufmgr_gem->has_full_ppgtt = (ret == 0) & (*gp.value > 0);
> +
>  	if (bufmgr_gem->gen < 4) {
>  		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
>  		gp.value = &bufmgr_gem->available_fences;
> diff --git a/intel/intel_bufmgr_priv.h b/intel/intel_bufmgr_priv.h
> index 2592d42..e622746 100644
> --- a/intel/intel_bufmgr_priv.h
> +++ b/intel/intel_bufmgr_priv.h
> @@ -283,6 +283,7 @@ struct _drm_intel_bufmgr {
>  struct _drm_intel_context {
>  	unsigned int ctx_id;
>  	struct _drm_intel_bufmgr *bufmgr;
> +	int is_default;
>  };
>  
>  #define ALIGN(value, alignment)	((value + alignment - 1) & ~(alignment - 1))
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] intel: Intel full PPGTT param

[Ben Widawsky]

On Tue, Jan 07, 2014 at 08:53:36AM +0100, Daniel Vetter wrote:
> On Thu, Jan 02, 2014 at 07:50:33PM -1000, Ben Widawsky wrote:
> > This will allow mesa to determine if it needs to create a context, or
> > can reuse the default context. Reusing the default context saves memory,
> > and startup time.
> > 
> > To keep the libdrm interface as dumb as possible, we simply expose a
> > getter for the default context (which is only a real context when
> > thereis full PPGTT and per fd contexts). As such, callers can expect
> > NULL when this is not the case.
> > 
> > See the usage in mesa for details.
> > 
> > Cc: Kenneth Graunke <kenneth.w.graunke@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Fyi I've killed this.
> -Daniel

There was supposed to be a revert in my patch series, but I must have
forgotten to send it out. In either case, it depends on a revert of your
revert.


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Daniel Vetter]

Readding intel-gfx.

On Sat, Jan 11, 2014 at 12:20 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jan 10, 2014 at 8:24 PM, Ben Widawsky
> <benjamin.widawsky@intel.com> wrote:
>> On Mon, Jan 6, 2014 at 11:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> Neither patch subject nor the commit message mention that this fixes a
>>> bug. Also, the Testcase: line at the bottom is missing. I've fixed this
>>> up.
>>> -Daniel
>>
>> I thought "Bugzilla" link implies it fixes a bug.
>
> Forcing people to read through bugzilla to figure out what exactly the
> patch fixes and how is a bit unfriendly. Generally describing the why
> and effects is much more important than than the what.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



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

Re: [Intel-gfx] [PATCH] i965: Use default contexts when possible.

[Ben Widawsky]

On Thu, Jan 02, 2014 at 07:50:35PM -1000, Ben Widawsky wrote:
> Will full PPGTT support it can be assumed that every file descriptor
> gets its own hardware context. As such, there is no need to allocate
> anew context in order to use the features provided by hardware contexts.
> Eliminating this extra context allocation saves both physical memory
> (currently PPGTT pages are pinned forever), GGTT space on IVB, and HSW,
> as well as the associated startup cost of allocating the second context
> - which can include an eviction in pathological cases.
> 
> Unfortunately, this requires a libdrm version bump as it requires a new
> interface.
> 
> NOTE: I am uncertain if the share context idea is valid. Needs mesa
> eyes.
> 
> Cc: Kenneth Graunke <kenneth.w.graunke@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Hi Ken/whomever. Everything but the actual exposing of the full PPGTT
param from this series has been merged. If you can give me a pointer on
what needs doing here in mesa, I'll try again now that full PPGTT is
available in dinq.

Thanks.

> ---
>  configure.ac                            | 2 +-
>  src/mesa/drivers/dri/i965/brw_context.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index f75325d..a0ae0b3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -29,7 +29,7 @@ AC_SUBST([OSMESA_VERSION])
>  dnl Versions for external dependencies
>  LIBDRM_REQUIRED=2.4.24
>  LIBDRM_RADEON_REQUIRED=2.4.50
> -LIBDRM_INTEL_REQUIRED=2.4.49
> +LIBDRM_INTEL_REQUIRED=2.4.51
>  LIBDRM_NVVIEUX_REQUIRED=2.4.33
>  LIBDRM_NOUVEAU_REQUIRED="2.4.33 libdrm >= 2.4.41"
>  LIBDRM_FREEDRENO_REQUIRED=2.4.39
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 78c06fc..f5a66e1 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -674,7 +674,14 @@ brwCreateContext(gl_api api,
>  
>     intel_fbo_init(brw);
>  
> -   if (brw->gen >= 6) {
> +   /* We assume that the hw_ctx is the first created for the file descriptor if
> +    * this is not a share context. If that assumption turns false then we'll
> +    * end up having hw_ctx for two contexts using the same actual context.
> +    */
> +   if (!shareCtx)
> +      brw->hw_ctx = drm_intel_gem_default_context_get(brw->bufmgr);
> +
> +   if (!brw->hw_ctx && brw->gen >= 6) {
>        /* Create a new hardware context.  Using a hardware context means that
>         * our GPU state will be saved/restored on context switch, allowing us
>         * to assume that the GPU is in the same state we left it in.
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center