[PATCH v3 0/3] improve handling of the driver's internal (default) context

[PATCH v3 0/3] improve handling of the driver's internal (default) context

[Dave Gordon]

During startup, the driver creates a unique "intel_context" that will
provide a home for orphan requests (i.e. those generated by the driver
internally, not associated with user batchbuffers).

However, one of the infelicities of the current code is that the driver
keeps a per-engine pointer to the default context for that engine (this
is probably from a decision made early in execlists implementation,
when it was thought that a context was engine-specific, and not revised
when it was decided that an "intel_context" represents a multiplex of
engine-level contexts). All these per-engine pointers actually end up
pointing to the same unique object.

To compound this, the refcounting is bogus; the driver holds just one
reference to the default context, even though there are multiple pointers
to it (RCS is considered to hold this "on behalf of" the other engines,
but this can lead to problems during unload as it makes the code sensitive
to the order of engine teardown -- RCS should be done last, but it isn't).

Also, some of the execlists batch submission code treats the default
context differently from any other; testing for it by comparing against
the per-engine pointer is quite clumsy, especially in debugfs where
contexts are enumerated from a global list and therefore not automatically
known to be associated with a particular engine.

Therefore, we should replace the per-engine pointers with a single
driver-wide reference, which will make the refcounting sane and simplify
the code that uses this context for non-user-related requests.

THIS patchset does NOT address the execlists code's special handling of
the default context, but it will simplify the planned future cleanup of
that code.

v3: don't flag the context created during startup (and fully instantiated
on all engines) for driver-internal purposes as "is_global_default",
1448630935-27377-1-git-send-email-chris@chris-wilson.co.uk notwithstanding.
[Chris Wilson] (We now call it "the kernel context", and compare against
the device-wide pointer).



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

[PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

There are a number of places where the driver needs a request, but isn't
working on behalf of any specific user or in a specific context. At
present, we associate them with the per-engine default context. A future
patch will abolish those per-engine context pointers; but we can already
eliminate a lot of the references to them, just by making the allocator
allow NULL as a shorthand for "an appropriate context for this ring",
which will mean that the callers don't need to know anything about how
the "appropriate context" is found (e.g. per-ring vs per-device, etc).

So this patch renames the existing i915_gem_request_alloc(), and makes
it local (static inline), and replaces it with a wrapper that provides
a default if the context is NULL, and also has a nicer calling
convention (doesn't require a pointer to an output parameter). Then we
change all callers to use the new convention:
OLD:
	err = i915_gem_request_alloc(ring, user_ctx, &req);
	if (err) ...
NEW:
	req = i915_gem_request_alloc(ring, user_ctx);
	if (IS_ERR(req)) ...
OLD:
	err = i915_gem_request_alloc(ring, ring->default_context, &req);
	if (err) ...
NEW:
	req = i915_gem_request_alloc(ring, NULL);
	if (IS_ERR(req)) ...

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
 drivers/gpu/drm/i915/i915_gem.c            | 55 +++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++---
 drivers/gpu/drm/i915/intel_display.c       |  6 ++--
 drivers/gpu/drm/i915/intel_lrc.c           |  9 +++--
 drivers/gpu/drm/i915/intel_overlay.c       | 24 ++++++-------
 6 files changed, 74 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c6dd4db..c2b000a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {
 
 };
 
-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request * __must_check
+i915_gem_request_alloc(struct intel_engine_cs *engine,
+		       struct intel_context *ctx);
 void i915_gem_request_cancel(struct drm_i915_gem_request *req);
 void i915_gem_request_free(struct kref *req_ref);
 int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6c60e04..c908ed1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2688,9 +2688,10 @@ void i915_gem_request_free(struct kref *req_ref)
 	kmem_cache_free(req->i915->requests, req);
 }
 
-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out)
+static inline int
+__i915_gem_request_alloc(struct intel_engine_cs *ring,
+			 struct intel_context *ctx,
+			 struct drm_i915_gem_request **req_out)
 {
 	struct drm_i915_private *dev_priv = to_i915(ring->dev);
 	struct drm_i915_gem_request *req;
@@ -2753,6 +2754,31 @@ err:
 	return ret;
 }
 
+/**
+ * i915_gem_request_alloc - allocate a request structure
+ *
+ * @engine: engine that we wish to issue the request on.
+ * @ctx: context that the request will be associated with.
+ *       This can be NULL if the request is not directly related to
+ *       any specific user context, in which case this function will
+ *       choose an appropriate context to use.
+ *
+ * Returns a pointer to the allocated request if successful,
+ * or an error code if not.
+ */
+struct drm_i915_gem_request *
+i915_gem_request_alloc(struct intel_engine_cs *engine,
+		       struct intel_context *ctx)
+{
+	struct drm_i915_gem_request *req;
+	int err;
+
+	if (ctx == NULL)
+		ctx = engine->default_context;
+	err = __i915_gem_request_alloc(engine, ctx, &req);
+	return err ? ERR_PTR(err) : req;
+}
+
 void i915_gem_request_cancel(struct drm_i915_gem_request *req)
 {
 	intel_ring_reserved_space_cancel(req->ringbuf);
@@ -3170,9 +3196,13 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			return 0;
 
 		if (*to_req == NULL) {
-			ret = i915_gem_request_alloc(to, to->default_context, to_req);
-			if (ret)
-				return ret;
+			struct drm_i915_gem_request *req;
+
+			req = i915_gem_request_alloc(to, NULL);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
+
+			*to_req = req;
 		}
 
 		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
@@ -3372,9 +3402,9 @@ int i915_gpu_idle(struct drm_device *dev)
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
-			ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-			if (ret)
-				return ret;
+			req = i915_gem_request_alloc(ring, NULL);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
 
 			ret = i915_switch_context(req);
 			if (ret) {
@@ -4869,10 +4899,9 @@ i915_gem_init_hw(struct drm_device *dev)
 	for_each_ring(ring, dev_priv, i) {
 		struct drm_i915_gem_request *req;
 
-		WARN_ON(!ring->default_context);
-
-		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-		if (ret) {
+		req = i915_gem_request_alloc(ring, NULL);
+		if (IS_ERR(req)) {
+			ret = PTR_ERR(req);
 			i915_gem_cleanup_ringbuffer(dev);
 			goto out;
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index dccb517..dc6caeb 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1381,6 +1381,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_i915_gem_exec_object2 *exec)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_request *req = NULL;
 	struct eb_vmas *eb;
 	struct drm_i915_gem_object *batch_obj;
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
@@ -1602,11 +1603,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
 
 	/* Allocate a request for this batch buffer nice and early. */
-	ret = i915_gem_request_alloc(ring, ctx, &params->request);
-	if (ret)
+	req = i915_gem_request_alloc(ring, ctx);
+	if (IS_ERR(req)) {
+		ret = PTR_ERR(req);
 		goto err_batch_unpin;
+	}
 
-	ret = i915_gem_request_add_to_client(params->request, file);
+	ret = i915_gem_request_add_to_client(req, file);
 	if (ret)
 		goto err_batch_unpin;
 
@@ -1622,6 +1625,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	params->dispatch_flags          = dispatch_flags;
 	params->batch_obj               = batch_obj;
 	params->ctx                     = ctx;
+	params->request                 = req;
 
 	ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
 
@@ -1645,8 +1649,8 @@ err:
 	 * must be freed again. If it was submitted then it is being tracked
 	 * on the active request list and no clean up is required here.
 	 */
-	if (ret && params->request)
-		i915_gem_request_cancel(params->request);
+	if (ret && req)
+		i915_gem_request_cancel(req);
 
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed9ab60..1f8f781 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11700,9 +11700,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 					obj->last_write_req);
 	} else {
 		if (!request) {
-			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
-			if (ret)
+			request = i915_gem_request_alloc(ring, NULL);
+			if (IS_ERR(request)) {
+				ret = PTR_ERR(request);
 				goto cleanup_unpin;
+			}
 		}
 
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8096c6a..8b6542d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2510,11 +2510,10 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	if (ctx != ring->default_context && ring->init_context) {
 		struct drm_i915_gem_request *req;
 
-		ret = i915_gem_request_alloc(ring,
-			ctx, &req);
-		if (ret) {
-			DRM_ERROR("ring create req: %d\n",
-				ret);
+		req = i915_gem_request_alloc(ring, ctx);
+		if (IS_ERR(req)) {
+			ret = PTR_ERR(req);
+			DRM_ERROR("ring create req: %d\n", ret);
 			goto error_ringbuf;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 76f1980..9168413 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -240,9 +240,9 @@ static int intel_overlay_on(struct intel_overlay *overlay)
 	WARN_ON(overlay->active);
 	WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc(ring, NULL);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 4);
 	if (ret) {
@@ -283,9 +283,9 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
 	if (tmp & (1 << 17))
 		DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc(ring, NULL);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 2);
 	if (ret) {
@@ -349,9 +349,9 @@ static int intel_overlay_off(struct intel_overlay *overlay)
 	 * of the hw. Do it in both cases */
 	flip_addr |= OFC_UPDATE;
 
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
+	req = i915_gem_request_alloc(ring, NULL);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
 
 	ret = intel_ring_begin(req, 6);
 	if (ret) {
@@ -423,9 +423,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 		/* synchronous slowpath */
 		struct drm_i915_gem_request *req;
 
-		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-		if (ret)
-			return ret;
+		req = i915_gem_request_alloc(ring, NULL);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
 
 		ret = intel_ring_begin(req, 2);
 		if (ret) {
-- 
1.9.1

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

[PATCH v3 2/3] drm/i915: abolish separate per-ring default_context pointers

[Dave Gordon]

Now that we've eliminated a lot of uses of ring->default_context,
we can eliminate the pointer itself.

All the engines share the same default intel_context, so we can just
keep a single reference to it in the dev_priv structure rather than one
in each of the engine[] elements. This make refcounting more sensible
too, as we now have a refcount of one for the one pointer, rather than
a refcount of one but multiple pointers.

From an idea by Chris Wilson.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h            |  2 ++
 drivers/gpu/drm/i915/i915_gem.c            |  6 +++---
 drivers/gpu/drm/i915/i915_gem_context.c    | 22 ++++++++--------------
 drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  6 +++---
 drivers/gpu/drm/i915/intel_lrc.c           | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
 8 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0fc38bb..2613708 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1943,7 +1943,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
 		seq_puts(m, "HW context ");
 		describe_ctx(m, ctx);
 		for_each_ring(ring, dev_priv, i) {
-			if (ring->default_context == ctx)
+			if (dev_priv->kernel_context == ctx)
 				seq_printf(m, "(default context %s) ",
 					   ring->name);
 		}
@@ -2039,7 +2039,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 
 	list_for_each_entry(ctx, &dev_priv->context_list, link) {
 		for_each_ring(ring, dev_priv, i) {
-			if (ring->default_context != ctx)
+			if (dev_priv->kernel_context != ctx)
 				i915_dump_lrc_obj(m, ring,
 						  ctx->engine[i].state);
 		}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c2b000a..aef86a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1940,6 +1940,8 @@ struct drm_i915_private {
 		void (*stop_ring)(struct intel_engine_cs *ring);
 	} gt;
 
+	struct intel_context *kernel_context;
+
 	bool edp_low_vswing;
 
 	/* perform PHY state sanity checks? */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c908ed1..8f101121 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2678,7 +2678,7 @@ void i915_gem_request_free(struct kref *req_ref)
 
 	if (ctx) {
 		if (i915.enable_execlists) {
-			if (ctx != req->ring->default_context)
+			if (ctx != req->i915->kernel_context)
 				intel_lr_context_unpin(req);
 		}
 
@@ -2774,7 +2774,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
 	int err;
 
 	if (ctx == NULL)
-		ctx = engine->default_context;
+		ctx = to_i915(engine->dev)->kernel_context;
 	err = __i915_gem_request_alloc(engine, ctx, &req);
 	return err ? ERR_PTR(err) : req;
 }
@@ -4862,7 +4862,7 @@ i915_gem_init_hw(struct drm_device *dev)
 	 */
 	init_unused_rings(dev);
 
-	BUG_ON(!dev_priv->ring[RCS].default_context);
+	BUG_ON(!dev_priv->kernel_context);
 
 	ret = i915_ppgtt_init_hw(dev);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..e1d767e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -354,11 +354,10 @@ int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
-	int i;
 
 	/* Init should only be called once per module load. Eventually the
 	 * restriction on the context_disabled check can be loosened. */
-	if (WARN_ON(dev_priv->ring[RCS].default_context))
+	if (WARN_ON(dev_priv->kernel_context))
 		return 0;
 
 	if (intel_vgpu_active(dev) && HAS_LOGICAL_RING_CONTEXTS(dev)) {
@@ -388,12 +387,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_engine_cs *ring = &dev_priv->ring[i];
-
-		/* NB: RCS will hold a ref for all rings */
-		ring->default_context = ctx;
-	}
+	dev_priv->kernel_context = ctx;
 
 	DRM_DEBUG_DRIVER("%s context support initialized\n",
 			i915.enable_execlists ? "LR" :
@@ -404,7 +398,7 @@ int i915_gem_context_init(struct drm_device *dev)
 void i915_gem_context_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
+	struct intel_context *dctx = dev_priv->kernel_context;
 	int i;
 
 	if (dctx->legacy_hw_ctx.rcs_state) {
@@ -431,17 +425,17 @@ void i915_gem_context_fini(struct drm_device *dev)
 		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for (i = I915_NUM_RINGS; --i >= 0;) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
-		if (ring->last_context)
+		if (ring->last_context) {
 			i915_gem_context_unreference(ring->last_context);
-
-		ring->default_context = NULL;
-		ring->last_context = NULL;
+			ring->last_context = NULL;
+		}
 	}
 
 	i915_gem_context_unreference(dctx);
+	dev_priv->kernel_context = NULL;
 }
 
 int i915_gem_context_enable(struct drm_i915_gem_request *req)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 06ca408..7eeb244 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1050,7 +1050,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 			if (request)
 				rbuf = request->ctx->engine[ring->id].ringbuf;
 			else
-				rbuf = ring->default_context->engine[ring->id].ringbuf;
+				rbuf = dev_priv->kernel_context->engine[ring->id].ringbuf;
 		} else
 			rbuf = ring->buffer;
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 9c24424..51ae5c1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -964,7 +964,7 @@ int i915_guc_submission_enable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_context *ctx = dev_priv->ring[RCS].default_context;
+	struct intel_context *ctx = dev_priv->kernel_context;
 	struct i915_guc_client *client;
 
 	/* client for execbuf submission */
@@ -1021,7 +1021,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->ring[RCS].default_context;
+	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -1047,7 +1047,7 @@ int intel_guc_resume(struct drm_device *dev)
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->ring[RCS].default_context;
+	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8b6542d..aaaa5a3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -571,7 +571,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
 	struct drm_i915_gem_request *cursor;
 	int num_elements = 0;
 
-	if (request->ctx != ring->default_context)
+	if (request->ctx != request->i915->kernel_context)
 		intel_lr_context_pin(request);
 
 	i915_gem_request_reference(request);
@@ -664,7 +664,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 
 	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
 
-	if (request->ctx != request->ring->default_context) {
+	if (request->ctx != request->i915->kernel_context) {
 		ret = intel_lr_context_pin(request);
 		if (ret)
 			return ret;
@@ -980,7 +980,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
 		struct drm_i915_gem_object *ctx_obj =
 				ctx->engine[ring->id].state;
 
-		if (ctx_obj && (ctx != ring->default_context))
+		if (ctx_obj && (ctx != req->i915->kernel_context))
 			intel_lr_context_unpin(req);
 		list_del(&req->execlist_link);
 		i915_gem_request_unreference(req);
@@ -1487,7 +1487,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
-				ring->default_context->engine[ring->id].state);
+				dev_priv->kernel_context->engine[ring->id].state);
 
 	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
 	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
@@ -1929,6 +1929,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
+	int ring_id = ring->id;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -1949,15 +1952,14 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 	if (ret)
 		goto error;
 
-	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
+	ret = intel_lr_context_deferred_alloc(dctx, ring);
 	if (ret)
 		goto error;
 
 	/* As this is the default context, always pin it */
-	ret = intel_lr_context_do_pin(
-			ring,
-			ring->default_context->engine[ring->id].state,
-			ring->default_context->engine[ring->id].ringbuf);
+	ret = intel_lr_context_do_pin(ring,
+				      dctx->engine[ring_id].state,
+				      dctx->engine[ring_id].ringbuf);
 	if (ret) {
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
@@ -2388,7 +2390,7 @@ void intel_lr_context_free(struct intel_context *ctx)
 					ctx->engine[i].ringbuf;
 			struct intel_engine_cs *ring = ringbuf->ring;
 
-			if (ctx == ring->default_context) {
+			if (ctx == ctx->i915->kernel_context) {
 				intel_unpin_ringbuffer_obj(ringbuf);
 				i915_gem_object_ggtt_unpin(ctx_obj);
 			}
@@ -2507,7 +2509,7 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	ctx->engine[ring->id].ringbuf = ringbuf;
 	ctx->engine[ring->id].state = ctx_obj;
 
-	if (ctx != ring->default_context && ring->init_context) {
+	if (ctx != ctx->i915->kernel_context && ring->init_context) {
 		struct drm_i915_gem_request *req;
 
 		req = i915_gem_request_alloc(ring, ctx);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 49574ff..e950f6c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -305,7 +305,6 @@ struct  intel_engine_cs {
 
 	wait_queue_head_t irq_queue;
 
-	struct intel_context *default_context;
 	struct intel_context *last_context;
 
 	struct intel_ring_hangcheck hangcheck;
-- 
1.9.1

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

[PATCH v3 3/3] drm/i915: tidy up a few leftovers

[Dave Gordon]

There are a few bits of code which the transformations implemented by
the previous patch reveal to be suboptimal, once the notion of a per-
ring default context has gone away. So this tidies up the leftovers.

It could have been squashed into the previous patch, but that would have
made that patch less clearly a simple transformation. In particular, any
change which alters the code block structure or indentation has been
deferred into this separate patch, because such things tend to make diffs
more difficult to read.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++----------
 drivers/gpu/drm/i915/i915_gem.c     |  6 ++----
 drivers/gpu/drm/i915/intel_lrc.c    | 38 +++++++++++++++++--------------------
 3 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2613708..bbb23da 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
 
 		seq_puts(m, "HW context ");
 		describe_ctx(m, ctx);
-		for_each_ring(ring, dev_priv, i) {
-			if (dev_priv->kernel_context == ctx)
-				seq_printf(m, "(default context %s) ",
-					   ring->name);
-		}
+		if (ctx == dev_priv->kernel_context)
+			seq_printf(m, "(kernel context) ");
 
 		if (i915.enable_execlists) {
 			seq_putc(m, '\n');
@@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 	if (ret)
 		return ret;
 
-	list_for_each_entry(ctx, &dev_priv->context_list, link) {
-		for_each_ring(ring, dev_priv, i) {
-			if (dev_priv->kernel_context != ctx)
+	list_for_each_entry(ctx, &dev_priv->context_list, link)
+		if (ctx != dev_priv->kernel_context)
+			for_each_ring(ring, dev_priv, i)
 				i915_dump_lrc_obj(m, ring,
 						  ctx->engine[i].state);
-		}
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8f101121..4f45eb2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref)
 		i915_gem_request_remove_from_client(req);
 
 	if (ctx) {
-		if (i915.enable_execlists) {
-			if (ctx != req->i915->kernel_context)
-				intel_lr_context_unpin(req);
-		}
+		if (i915.enable_execlists && ctx != req->i915->kernel_context)
+			intel_lr_context_unpin(req);
 
 		i915_gem_context_unreference(ctx);
 	}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index aaaa5a3..8c4c9b9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -660,16 +660,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 
 int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
 {
-	int ret;
+	int ret = 0;
 
 	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
 
-	if (request->ctx != request->i915->kernel_context) {
-		ret = intel_lr_context_pin(request);
-		if (ret)
-			return ret;
-	}
-
 	if (i915.enable_guc_submission) {
 		/*
 		 * Check that the GuC has space for the request before
@@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 			return ret;
 	}
 
-	return 0;
+	if (request->ctx != request->i915->kernel_context)
+		ret = intel_lr_context_pin(request);
+
+	return ret;
 }
 
 static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
@@ -2382,22 +2379,21 @@ void intel_lr_context_free(struct intel_context *ctx)
 {
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for (i = I915_NUM_RINGS; --i >= 0; ) {
+		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
 		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
 
-		if (ctx_obj) {
-			struct intel_ringbuffer *ringbuf =
-					ctx->engine[i].ringbuf;
-			struct intel_engine_cs *ring = ringbuf->ring;
+		if (!ctx_obj)
+			continue;
 
-			if (ctx == ctx->i915->kernel_context) {
-				intel_unpin_ringbuffer_obj(ringbuf);
-				i915_gem_object_ggtt_unpin(ctx_obj);
-			}
-			WARN_ON(ctx->engine[ring->id].pin_count);
-			intel_ringbuffer_free(ringbuf);
-			drm_gem_object_unreference(&ctx_obj->base);
+		if (ctx == ctx->i915->kernel_context) {
+			intel_unpin_ringbuffer_obj(ringbuf);
+			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
+
+		WARN_ON(ctx->engine[i].pin_count);
+		intel_ringbuffer_free(ringbuf);
+		drm_gem_object_unreference(&ctx_obj->base);
 	}
 }
 
@@ -2472,7 +2468,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
  */
 
 int intel_lr_context_deferred_alloc(struct intel_context *ctx,
-				     struct intel_engine_cs *ring)
+				    struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_gem_object *ctx_obj;
-- 
1.9.1

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> There are a number of places where the driver needs a request, but isn't
> working on behalf of any specific user or in a specific context. At
> present, we associate them with the per-engine default context. A future
> patch will abolish those per-engine context pointers; but we can already
> eliminate a lot of the references to them, just by making the allocator
> allow NULL as a shorthand for "an appropriate context for this ring",
> which will mean that the callers don't need to know anything about how
> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> 
> So this patch renames the existing i915_gem_request_alloc(), and makes
> it local (static inline), and replaces it with a wrapper that provides
> a default if the context is NULL, and also has a nicer calling
> convention (doesn't require a pointer to an output parameter). Then we
> change all callers to use the new convention:
> OLD:
> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, user_ctx);
> 	if (IS_ERR(req)) ...
> OLD:
> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, NULL);
> 	if (IS_ERR(req)) ...

Nak. You haven't fixed i915_gem_request_alloc() at all.

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
is the patch I have been carrying ever since.
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 07/01/16 11:58, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
>> There are a number of places where the driver needs a request, but isn't
>> working on behalf of any specific user or in a specific context. At
>> present, we associate them with the per-engine default context. A future
>> patch will abolish those per-engine context pointers; but we can already
>> eliminate a lot of the references to them, just by making the allocator
>> allow NULL as a shorthand for "an appropriate context for this ring",
>> which will mean that the callers don't need to know anything about how
>> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>>
>> So this patch renames the existing i915_gem_request_alloc(), and makes
>> it local (static inline), and replaces it with a wrapper that provides
>> a default if the context is NULL, and also has a nicer calling
>> convention (doesn't require a pointer to an output parameter). Then we
>> change all callers to use the new convention:
>> OLD:
>> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
>> 	if (err) ...
>> NEW:
>> 	req = i915_gem_request_alloc(ring, user_ctx);
>> 	if (IS_ERR(req)) ...
>> OLD:
>> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>> 	if (err) ...
>> NEW:
>> 	req = i915_gem_request_alloc(ring, NULL);
>> 	if (IS_ERR(req)) ...
>
> Nak. You haven't fixed i915_gem_request_alloc() at all.
>
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
> is the patch I have been carrying ever since.
> -Chris

I think you'll find that the version of i915_gem_request_alloc() I've 
implemented is equivalent to yours, with the *additional* (and better) 
semantic of not requiring the caller to specify (ring->default_param) as 
the context parameter (which is the main point, as far as I'm concerned; 
making the calling convention nicer was just incidental).

*MINE*:
diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h
index c6dd4db..c2b000a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {

  };

-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request * __must_check
+i915_gem_request_alloc(struct intel_engine_cs *engine,
+		       struct intel_context *ctx);
  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
  void i915_gem_request_free(struct kref *req_ref);
  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
...

*YOURS*:
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h 
b/drivers/gpu/drm/i915/i915_gem_request.h
index 0869505..2da9e0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -118,9 +118,9 @@ struct drm_i915_gem_request {
  	int elsp_submitted;
  };

-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request *
+i915_gem_request_alloc(struct intel_engine_cs *ring,
+		       struct intel_context *ctx);
  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
  				   struct drm_file *file);
...

You seem to have done EXACTLY THE SAME transformation of the function 
signature (except I remembered to add __must_check, and wrote some 
kerneldoc for it), so what's not to like?

I haven't done anything to {__}i915_gem_object_sync(), but that's a 
separate issue that you can fix on top of this.

.Dave.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Jesse Barnes]

On 01/07/2016 03:58 AM, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
>> There are a number of places where the driver needs a request, but isn't
>> working on behalf of any specific user or in a specific context. At
>> present, we associate them with the per-engine default context. A future
>> patch will abolish those per-engine context pointers; but we can already
>> eliminate a lot of the references to them, just by making the allocator
>> allow NULL as a shorthand for "an appropriate context for this ring",
>> which will mean that the callers don't need to know anything about how
>> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>>
>> So this patch renames the existing i915_gem_request_alloc(), and makes
>> it local (static inline), and replaces it with a wrapper that provides
>> a default if the context is NULL, and also has a nicer calling
>> convention (doesn't require a pointer to an output parameter). Then we
>> change all callers to use the new convention:
>> OLD:
>> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
>> 	if (err) ...
>> NEW:
>> 	req = i915_gem_request_alloc(ring, user_ctx);
>> 	if (IS_ERR(req)) ...
>> OLD:
>> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>> 	if (err) ...
>> NEW:
>> 	req = i915_gem_request_alloc(ring, NULL);
>> 	if (IS_ERR(req)) ...
> 
> Nak. You haven't fixed i915_gem_request_alloc() at all.
> 
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
> is the patch I have been carrying ever since.

Can we stop with the "nak"?  This patch wraps the request alloc
differently than yours, but you haven't given details as to why you
think it's incorrect (see Dave's reply).

A patch review should contain one of the following:
  - Acknowledge and accept patch: provide Reviewed-by tag
  - Request for changes or fixes – clear list of actionable items to be
    addressed before Reviewed-by tag is given
  - Reject due to fundamental issue with approach or conflict with
    other work – clear reasons must be provided, in the case of
    conflict, JIRA or BZ of conflicting work should be provided for
    tracking and to ensure requirements are captured

If you really have a fundamental issue here (it doesn't sound like it)
you need to be clear about it.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Thu, Jan 07, 2016 at 08:49:38AM -0800, Jesse Barnes wrote:
> On 01/07/2016 03:58 AM, Chris Wilson wrote:
> > On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> >> There are a number of places where the driver needs a request, but isn't
> >> working on behalf of any specific user or in a specific context. At
> >> present, we associate them with the per-engine default context. A future
> >> patch will abolish those per-engine context pointers; but we can already
> >> eliminate a lot of the references to them, just by making the allocator
> >> allow NULL as a shorthand for "an appropriate context for this ring",
> >> which will mean that the callers don't need to know anything about how
> >> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> >>
> >> So this patch renames the existing i915_gem_request_alloc(), and makes
> >> it local (static inline), and replaces it with a wrapper that provides
> >> a default if the context is NULL, and also has a nicer calling
> >> convention (doesn't require a pointer to an output parameter). Then we
> >> change all callers to use the new convention:
> >> OLD:
> >> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> >> 	if (err) ...
> >> NEW:
> >> 	req = i915_gem_request_alloc(ring, user_ctx);
> >> 	if (IS_ERR(req)) ...
> >> OLD:
> >> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> >> 	if (err) ...
> >> NEW:
> >> 	req = i915_gem_request_alloc(ring, NULL);
> >> 	if (IS_ERR(req)) ...
> > 
> > Nak. You haven't fixed i915_gem_request_alloc() at all.
> > 
> > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
> > is the patch I have been carrying ever since.
> 
> Can we stop with the "nak"?  This patch wraps the request alloc
> differently than yours, but you haven't given details as to why you
> think it's incorrect (see Dave's reply).

I am annoyed that people do not review my patches and are duplicating
work I did last year and the year before, without attempting to fix
real bugs.
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Thu, Jan 07, 2016 at 12:34:39PM +0000, Dave Gordon wrote:
> On 07/01/16 11:58, Chris Wilson wrote:
> >On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> >>There are a number of places where the driver needs a request, but isn't
> >>working on behalf of any specific user or in a specific context. At
> >>present, we associate them with the per-engine default context. A future
> >>patch will abolish those per-engine context pointers; but we can already
> >>eliminate a lot of the references to them, just by making the allocator
> >>allow NULL as a shorthand for "an appropriate context for this ring",
> >>which will mean that the callers don't need to know anything about how
> >>the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> >>
> >>So this patch renames the existing i915_gem_request_alloc(), and makes
> >>it local (static inline), and replaces it with a wrapper that provides
> >>a default if the context is NULL, and also has a nicer calling
> >>convention (doesn't require a pointer to an output parameter). Then we
> >>change all callers to use the new convention:
> >>OLD:
> >>	err = i915_gem_request_alloc(ring, user_ctx, &req);
> >>	if (err) ...
> >>NEW:
> >>	req = i915_gem_request_alloc(ring, user_ctx);
> >>	if (IS_ERR(req)) ...
> >>OLD:
> >>	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> >>	if (err) ...
> >>NEW:
> >>	req = i915_gem_request_alloc(ring, NULL);
> >>	if (IS_ERR(req)) ...
> >
> >Nak. You haven't fixed i915_gem_request_alloc() at all.
> >
> >http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
> >is the patch I have been carrying ever since.
> >-Chris
> 
> I think you'll find that the version of i915_gem_request_alloc()
> I've implemented is equivalent to yours, with the *additional* (and
> better) semantic of not requiring the caller to specify
> (ring->default_param) as the context parameter (which is the main
> point, as far as I'm concerned; making the calling convention nicer
> was just incidental).

No. Specifying the context is crucial to request allocation. The issue
in the current function call chains are the requests appear out of
nowhere rather than being created with explicit context.

Hiding the context is bad.
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 07/01/16 16:56, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 12:34:39PM +0000, Dave Gordon wrote:
>> On 07/01/16 11:58, Chris Wilson wrote:
>>> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
>>>> There are a number of places where the driver needs a request, but isn't
>>>> working on behalf of any specific user or in a specific context. At
>>>> present, we associate them with the per-engine default context. A future
>>>> patch will abolish those per-engine context pointers; but we can already
>>>> eliminate a lot of the references to them, just by making the allocator
>>>> allow NULL as a shorthand for "an appropriate context for this ring",
>>>> which will mean that the callers don't need to know anything about how
>>>> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>>>>
>>>> So this patch renames the existing i915_gem_request_alloc(), and makes
>>>> it local (static inline), and replaces it with a wrapper that provides
>>>> a default if the context is NULL, and also has a nicer calling
>>>> convention (doesn't require a pointer to an output parameter). Then we
>>>> change all callers to use the new convention:
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, user_ctx);
>>>> 	if (IS_ERR(req)) ...
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, NULL);
>>>> 	if (IS_ERR(req)) ...
>>>
>>> Nak. You haven't fixed i915_gem_request_alloc() at all.
>>>
>>> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
>>> is the patch I have been carrying ever since.
>>> -Chris
>>
>> I think you'll find that the version of i915_gem_request_alloc()
>> I've implemented is equivalent to yours, with the *additional* (and
>> better) semantic of not requiring the caller to specify
>> (ring->default_param) as the context parameter (which is the main
>> point, as far as I'm concerned; making the calling convention nicer
>> was just incidental).
>
> No. Specifying the context is crucial to request allocation. The issue
> in the current function call chains are the requests appear out of
> nowhere rather than being created with explicit context.
>
> Hiding the context is bad.
> -Chris

Why would it be better to name dev_priv->kernel_context at every 
callsite rather than once in the called function (especially in my 
version where it's very obvious that the new wrapper provides that 
default before calling the original underlying allocator).

The context is only important to callers who have one in hand. To the 
others, it's an irrelevance; they're just saying, "hey, I want a request 
but it's not connected to any context I know of, so gimme a default". 
The proof of that is that we can switch them from a per-engine default 
to a per-driver default without changing anything else about them. They 
really don't need to know how to find the default context, so let the 
allocator find it for them.

<aside>
Suppose we some day decide that perhaps there should be *multiple* 
/kernel contexts/ (maybe for greater parallelism); my approach will let 
us spread non-user requests across all suitable contexts just by 
implementing that choice in one place (the new wrapper). By contrast, 
naming the default kernel context at every callsite would make this a 
far more invasive change.
</aside>

It is a commonplace of modern software practice that encapsulating (and 
thus hiding) irrelevant detail is GOOD practice; it is the exposing of 
internal implementation detail that is BAD.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 07/01/16 16:53, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 08:49:38AM -0800, Jesse Barnes wrote:
>> On 01/07/2016 03:58 AM, Chris Wilson wrote:
>>> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
>>>> There are a number of places where the driver needs a request, but isn't
>>>> working on behalf of any specific user or in a specific context. At
>>>> present, we associate them with the per-engine default context. A future
>>>> patch will abolish those per-engine context pointers; but we can already
>>>> eliminate a lot of the references to them, just by making the allocator
>>>> allow NULL as a shorthand for "an appropriate context for this ring",
>>>> which will mean that the callers don't need to know anything about how
>>>> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>>>>
>>>> So this patch renames the existing i915_gem_request_alloc(), and makes
>>>> it local (static inline), and replaces it with a wrapper that provides
>>>> a default if the context is NULL, and also has a nicer calling
>>>> convention (doesn't require a pointer to an output parameter). Then we
>>>> change all callers to use the new convention:
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, user_ctx);
>>>> 	if (IS_ERR(req)) ...
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, NULL);
>>>> 	if (IS_ERR(req)) ...
>>>
>>> Nak. You haven't fixed i915_gem_request_alloc() at all.
>>>
>>> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
>>> is the patch I have been carrying ever since.
>>
>> Can we stop with the "nak"?  This patch wraps the request alloc
>> differently than yours, but you haven't given details as to why you
>> think it's incorrect (see Dave's reply).
>
> I am annoyed that people do not review my patches and are duplicating
> work I did last year and the year before, without attempting to fix
> real bugs.
> -Chris

Chris, this patchset is totally directed towards fixing a specific bug, 
one which, moreover, arose a consequence of a patch YOU wrote:
b0366a5 drm/i915: intel_ring_initialized() must be simple and inline
(mea culpa too, obviously, since I was the one who rebased & pushed it).
Nick has a fix for the original bug, which involves reversing the 
teardown order, but can't now use it since b0366a5, so the bug remains. 
Nick's fix can be made to work if we replace the per-engine default 
contexts with the global one, which you've already agreed is a good idea 
(I think it was your idea in the first place!).

We can't take your patch because it doesn't apply to nightly. If you 
provide a standalone version that's not entangled with 100 other patches 
I'll happily review it. Or I might cherry-pick your existing one out of 
the 190-element patchset and try to rebase it onto nightly, which is how 
b0366a5 got in in the first place. I suspect it would look very much 
like mine then ...

Maybe we should just revert b0366a5 instead? Even though it was quite 
nice in itself ...

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Daniel Vetter]

On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> There are a number of places where the driver needs a request, but isn't
> working on behalf of any specific user or in a specific context. At
> present, we associate them with the per-engine default context. A future
> patch will abolish those per-engine context pointers; but we can already
> eliminate a lot of the references to them, just by making the allocator
> allow NULL as a shorthand for "an appropriate context for this ring",
> which will mean that the callers don't need to know anything about how
> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> 
> So this patch renames the existing i915_gem_request_alloc(), and makes
> it local (static inline), and replaces it with a wrapper that provides
> a default if the context is NULL, and also has a nicer calling
> convention (doesn't require a pointer to an output parameter). Then we
> change all callers to use the new convention:
> OLD:
> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, user_ctx);
> 	if (IS_ERR(req)) ...
> OLD:
> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, NULL);
> 	if (IS_ERR(req)) ...
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
>  drivers/gpu/drm/i915/i915_gem.c            | 55 +++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++---
>  drivers/gpu/drm/i915/intel_display.c       |  6 ++--
>  drivers/gpu/drm/i915/intel_lrc.c           |  9 +++--
>  drivers/gpu/drm/i915/intel_overlay.c       | 24 ++++++-------
>  6 files changed, 74 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c6dd4db..c2b000a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {
>  
>  };
>  
> -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> -			   struct intel_context *ctx,
> -			   struct drm_i915_gem_request **req_out);
> +struct drm_i915_gem_request * __must_check
> +i915_gem_request_alloc(struct intel_engine_cs *engine,
> +		       struct intel_context *ctx);
>  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>  void i915_gem_request_free(struct kref *req_ref);
>  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6c60e04..c908ed1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2688,9 +2688,10 @@ void i915_gem_request_free(struct kref *req_ref)
>  	kmem_cache_free(req->i915->requests, req);
>  }
>  
> -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> -			   struct intel_context *ctx,
> -			   struct drm_i915_gem_request **req_out)
> +static inline int
> +__i915_gem_request_alloc(struct intel_engine_cs *ring,
> +			 struct intel_context *ctx,
> +			 struct drm_i915_gem_request **req_out)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(ring->dev);
>  	struct drm_i915_gem_request *req;
> @@ -2753,6 +2754,31 @@ err:
>  	return ret;
>  }
>  
> +/**
> + * i915_gem_request_alloc - allocate a request structure
> + *
> + * @engine: engine that we wish to issue the request on.
> + * @ctx: context that the request will be associated with.
> + *       This can be NULL if the request is not directly related to
> + *       any specific user context, in which case this function will
> + *       choose an appropriate context to use.
> + *
> + * Returns a pointer to the allocated request if successful,
> + * or an error code if not.
> + */
> +struct drm_i915_gem_request *
> +i915_gem_request_alloc(struct intel_engine_cs *engine,
> +		       struct intel_context *ctx)
> +{
> +	struct drm_i915_gem_request *req;
> +	int err;
> +
> +	if (ctx == NULL)
> +		ctx = engine->default_context;

I think NULL vs. explicit dev_priv->kernel_context (or whatever it is) is
semantically equivalent enough that it doesn't matter. And we can easily
sed this (or an entire patch series for easy rebasing) if we change our
minds.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	err = __i915_gem_request_alloc(engine, ctx, &req);
> +	return err ? ERR_PTR(err) : req;
> +}
> +
>  void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>  {
>  	intel_ring_reserved_space_cancel(req->ringbuf);
> @@ -3170,9 +3196,13 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  			return 0;
>  
>  		if (*to_req == NULL) {
> -			ret = i915_gem_request_alloc(to, to->default_context, to_req);
> -			if (ret)
> -				return ret;
> +			struct drm_i915_gem_request *req;
> +
> +			req = i915_gem_request_alloc(to, NULL);
> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
> +
> +			*to_req = req;
>  		}
>  
>  		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
> @@ -3372,9 +3402,9 @@ int i915_gpu_idle(struct drm_device *dev)
>  		if (!i915.enable_execlists) {
>  			struct drm_i915_gem_request *req;
>  
> -			ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -			if (ret)
> -				return ret;
> +			req = i915_gem_request_alloc(ring, NULL);
> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
>  
>  			ret = i915_switch_context(req);
>  			if (ret) {
> @@ -4869,10 +4899,9 @@ i915_gem_init_hw(struct drm_device *dev)
>  	for_each_ring(ring, dev_priv, i) {
>  		struct drm_i915_gem_request *req;
>  
> -		WARN_ON(!ring->default_context);
> -
> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -		if (ret) {
> +		req = i915_gem_request_alloc(ring, NULL);
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
>  			i915_gem_cleanup_ringbuffer(dev);
>  			goto out;
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index dccb517..dc6caeb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1381,6 +1381,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		       struct drm_i915_gem_exec_object2 *exec)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_request *req = NULL;
>  	struct eb_vmas *eb;
>  	struct drm_i915_gem_object *batch_obj;
>  	struct drm_i915_gem_exec_object2 shadow_exec_entry;
> @@ -1602,11 +1603,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>  
>  	/* Allocate a request for this batch buffer nice and early. */
> -	ret = i915_gem_request_alloc(ring, ctx, &params->request);
> -	if (ret)
> +	req = i915_gem_request_alloc(ring, ctx);
> +	if (IS_ERR(req)) {
> +		ret = PTR_ERR(req);
>  		goto err_batch_unpin;
> +	}
>  
> -	ret = i915_gem_request_add_to_client(params->request, file);
> +	ret = i915_gem_request_add_to_client(req, file);
>  	if (ret)
>  		goto err_batch_unpin;
>  
> @@ -1622,6 +1625,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	params->dispatch_flags          = dispatch_flags;
>  	params->batch_obj               = batch_obj;
>  	params->ctx                     = ctx;
> +	params->request                 = req;
>  
>  	ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
>  
> @@ -1645,8 +1649,8 @@ err:
>  	 * must be freed again. If it was submitted then it is being tracked
>  	 * on the active request list and no clean up is required here.
>  	 */
> -	if (ret && params->request)
> -		i915_gem_request_cancel(params->request);
> +	if (ret && req)
> +		i915_gem_request_cancel(req);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed9ab60..1f8f781 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11700,9 +11700,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  					obj->last_write_req);
>  	} else {
>  		if (!request) {
> -			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
> -			if (ret)
> +			request = i915_gem_request_alloc(ring, NULL);
> +			if (IS_ERR(request)) {
> +				ret = PTR_ERR(request);
>  				goto cleanup_unpin;
> +			}
>  		}
>  
>  		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8096c6a..8b6542d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2510,11 +2510,10 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>  	if (ctx != ring->default_context && ring->init_context) {
>  		struct drm_i915_gem_request *req;
>  
> -		ret = i915_gem_request_alloc(ring,
> -			ctx, &req);
> -		if (ret) {
> -			DRM_ERROR("ring create req: %d\n",
> -				ret);
> +		req = i915_gem_request_alloc(ring, ctx);
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
> +			DRM_ERROR("ring create req: %d\n", ret);
>  			goto error_ringbuf;
>  		}
>  
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 76f1980..9168413 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -240,9 +240,9 @@ static int intel_overlay_on(struct intel_overlay *overlay)
>  	WARN_ON(overlay->active);
>  	WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
>  
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>  
>  	ret = intel_ring_begin(req, 4);
>  	if (ret) {
> @@ -283,9 +283,9 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
>  	if (tmp & (1 << 17))
>  		DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
>  
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>  
>  	ret = intel_ring_begin(req, 2);
>  	if (ret) {
> @@ -349,9 +349,9 @@ static int intel_overlay_off(struct intel_overlay *overlay)
>  	 * of the hw. Do it in both cases */
>  	flip_addr |= OFC_UPDATE;
>  
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>  
>  	ret = intel_ring_begin(req, 6);
>  	if (ret) {
> @@ -423,9 +423,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>  		/* synchronous slowpath */
>  		struct drm_i915_gem_request *req;
>  
> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -		if (ret)
> -			return ret;
> +		req = i915_gem_request_alloc(ring, NULL);
> +		if (IS_ERR(req))
> +			return PTR_ERR(req);
>  
>  		ret = intel_ring_begin(req, 2);
>  		if (ret) {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/3] drm/i915: abolish separate per-ring default_context pointers

[Daniel Vetter]

On Thu, Jan 07, 2016 at 10:20:51AM +0000, Dave Gordon wrote:
> Now that we've eliminated a lot of uses of ring->default_context,
> we can eliminate the pointer itself.
> 
> All the engines share the same default intel_context, so we can just
> keep a single reference to it in the dev_priv structure rather than one
> in each of the engine[] elements. This make refcounting more sensible
> too, as we now have a refcount of one for the one pointer, rather than
> a refcount of one but multiple pointers.
> 
> From an idea by Chris Wilson.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h            |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c            |  6 +++---
>  drivers/gpu/drm/i915/i915_gem_context.c    | 22 ++++++++--------------
>  drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c |  6 +++---
>  drivers/gpu/drm/i915/intel_lrc.c           | 24 +++++++++++++-----------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
>  8 files changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0fc38bb..2613708 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1943,7 +1943,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  		seq_puts(m, "HW context ");
>  		describe_ctx(m, ctx);
>  		for_each_ring(ring, dev_priv, i) {
> -			if (ring->default_context == ctx)
> +			if (dev_priv->kernel_context == ctx)
>  				seq_printf(m, "(default context %s) ",
>  					   ring->name);
>  		}
> @@ -2039,7 +2039,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>  
>  	list_for_each_entry(ctx, &dev_priv->context_list, link) {
>  		for_each_ring(ring, dev_priv, i) {
> -			if (ring->default_context != ctx)
> +			if (dev_priv->kernel_context != ctx)
>  				i915_dump_lrc_obj(m, ring,
>  						  ctx->engine[i].state);
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c2b000a..aef86a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1940,6 +1940,8 @@ struct drm_i915_private {
>  		void (*stop_ring)(struct intel_engine_cs *ring);
>  	} gt;
>  
> +	struct intel_context *kernel_context;
> +
>  	bool edp_low_vswing;
>  
>  	/* perform PHY state sanity checks? */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c908ed1..8f101121 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2678,7 +2678,7 @@ void i915_gem_request_free(struct kref *req_ref)
>  
>  	if (ctx) {
>  		if (i915.enable_execlists) {
> -			if (ctx != req->ring->default_context)
> +			if (ctx != req->i915->kernel_context)
>  				intel_lr_context_unpin(req);
>  		}
>  
> @@ -2774,7 +2774,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	int err;
>  
>  	if (ctx == NULL)
> -		ctx = engine->default_context;
> +		ctx = to_i915(engine->dev)->kernel_context;
>  	err = __i915_gem_request_alloc(engine, ctx, &req);
>  	return err ? ERR_PTR(err) : req;
>  }
> @@ -4862,7 +4862,7 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 */
>  	init_unused_rings(dev);
>  
> -	BUG_ON(!dev_priv->ring[RCS].default_context);
> +	BUG_ON(!dev_priv->kernel_context);
>  
>  	ret = i915_ppgtt_init_hw(dev);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 900ffd0..e1d767e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -354,11 +354,10 @@ int i915_gem_context_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_context *ctx;
> -	int i;
>  
>  	/* Init should only be called once per module load. Eventually the
>  	 * restriction on the context_disabled check can be loosened. */
> -	if (WARN_ON(dev_priv->ring[RCS].default_context))
> +	if (WARN_ON(dev_priv->kernel_context))
>  		return 0;
>  
>  	if (intel_vgpu_active(dev) && HAS_LOGICAL_RING_CONTEXTS(dev)) {
> @@ -388,12 +387,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		struct intel_engine_cs *ring = &dev_priv->ring[i];
> -
> -		/* NB: RCS will hold a ref for all rings */
> -		ring->default_context = ctx;
> -	}
> +	dev_priv->kernel_context = ctx;
>  
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
>  			i915.enable_execlists ? "LR" :
> @@ -404,7 +398,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  void i915_gem_context_fini(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
> +	struct intel_context *dctx = dev_priv->kernel_context;
>  	int i;
>  
>  	if (dctx->legacy_hw_ctx.rcs_state) {
> @@ -431,17 +425,17 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
>  	}
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = I915_NUM_RINGS; --i >= 0;) {
>  		struct intel_engine_cs *ring = &dev_priv->ring[i];
>  
> -		if (ring->last_context)
> +		if (ring->last_context) {
>  			i915_gem_context_unreference(ring->last_context);
> -
> -		ring->default_context = NULL;
> -		ring->last_context = NULL;
> +			ring->last_context = NULL;
> +		}
>  	}
>  
>  	i915_gem_context_unreference(dctx);
> +	dev_priv->kernel_context = NULL;
>  }
>  
>  int i915_gem_context_enable(struct drm_i915_gem_request *req)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 06ca408..7eeb244 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1050,7 +1050,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  			if (request)
>  				rbuf = request->ctx->engine[ring->id].ringbuf;
>  			else
> -				rbuf = ring->default_context->engine[ring->id].ringbuf;
> +				rbuf = dev_priv->kernel_context->engine[ring->id].ringbuf;
>  		} else
>  			rbuf = ring->buffer;
>  
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9c24424..51ae5c1 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -964,7 +964,7 @@ int i915_guc_submission_enable(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc *guc = &dev_priv->guc;
> -	struct intel_context *ctx = dev_priv->ring[RCS].default_context;
> +	struct intel_context *ctx = dev_priv->kernel_context;
>  	struct i915_guc_client *client;
>  
>  	/* client for execbuf submission */
> @@ -1021,7 +1021,7 @@ int intel_guc_suspend(struct drm_device *dev)
>  	if (!i915.enable_guc_submission)
>  		return 0;
>  
> -	ctx = dev_priv->ring[RCS].default_context;
> +	ctx = dev_priv->kernel_context;
>  
>  	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> @@ -1047,7 +1047,7 @@ int intel_guc_resume(struct drm_device *dev)
>  	if (!i915.enable_guc_submission)
>  		return 0;
>  
> -	ctx = dev_priv->ring[RCS].default_context;
> +	ctx = dev_priv->kernel_context;
>  
>  	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
>  	data[1] = GUC_POWER_D0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8b6542d..aaaa5a3 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -571,7 +571,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
>  	struct drm_i915_gem_request *cursor;
>  	int num_elements = 0;
>  
> -	if (request->ctx != ring->default_context)
> +	if (request->ctx != request->i915->kernel_context)
>  		intel_lr_context_pin(request);
>  
>  	i915_gem_request_reference(request);
> @@ -664,7 +664,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>  
>  	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
>  
> -	if (request->ctx != request->ring->default_context) {
> +	if (request->ctx != request->i915->kernel_context) {
>  		ret = intel_lr_context_pin(request);
>  		if (ret)
>  			return ret;
> @@ -980,7 +980,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
>  		struct drm_i915_gem_object *ctx_obj =
>  				ctx->engine[ring->id].state;
>  
> -		if (ctx_obj && (ctx != ring->default_context))
> +		if (ctx_obj && (ctx != req->i915->kernel_context))
>  			intel_lr_context_unpin(req);
>  		list_del(&req->execlist_link);
>  		i915_gem_request_unreference(req);
> @@ -1487,7 +1487,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>  	u8 next_context_status_buffer_hw;
>  
>  	lrc_setup_hardware_status_page(ring,
> -				ring->default_context->engine[ring->id].state);
> +				dev_priv->kernel_context->engine[ring->id].state);
>  
>  	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
>  	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
> @@ -1929,6 +1929,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>  
>  static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_context *dctx = dev_priv->kernel_context;
> +	int ring_id = ring->id;
>  	int ret;
>  
>  	/* Intentionally left blank. */
> @@ -1949,15 +1952,14 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>  	if (ret)
>  		goto error;
>  
> -	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
> +	ret = intel_lr_context_deferred_alloc(dctx, ring);
>  	if (ret)
>  		goto error;
>  
>  	/* As this is the default context, always pin it */
> -	ret = intel_lr_context_do_pin(
> -			ring,
> -			ring->default_context->engine[ring->id].state,
> -			ring->default_context->engine[ring->id].ringbuf);
> +	ret = intel_lr_context_do_pin(ring,
> +				      dctx->engine[ring_id].state,
> +				      dctx->engine[ring_id].ringbuf);
>  	if (ret) {
>  		DRM_ERROR(
>  			"Failed to pin and map ringbuffer %s: %d\n",
> @@ -2388,7 +2390,7 @@ void intel_lr_context_free(struct intel_context *ctx)
>  					ctx->engine[i].ringbuf;
>  			struct intel_engine_cs *ring = ringbuf->ring;
>  
> -			if (ctx == ring->default_context) {
> +			if (ctx == ctx->i915->kernel_context) {
>  				intel_unpin_ringbuffer_obj(ringbuf);
>  				i915_gem_object_ggtt_unpin(ctx_obj);
>  			}
> @@ -2507,7 +2509,7 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>  	ctx->engine[ring->id].ringbuf = ringbuf;
>  	ctx->engine[ring->id].state = ctx_obj;
>  
> -	if (ctx != ring->default_context && ring->init_context) {
> +	if (ctx != ctx->i915->kernel_context && ring->init_context) {
>  		struct drm_i915_gem_request *req;
>  
>  		req = i915_gem_request_alloc(ring, ctx);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 49574ff..e950f6c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -305,7 +305,6 @@ struct  intel_engine_cs {
>  
>  	wait_queue_head_t irq_queue;
>  
> -	struct intel_context *default_context;
>  	struct intel_context *last_context;
>  
>  	struct intel_ring_hangcheck hangcheck;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/3] drm/i915: tidy up a few leftovers

[Daniel Vetter]

On Thu, Jan 07, 2016 at 10:20:52AM +0000, Dave Gordon wrote:
> There are a few bits of code which the transformations implemented by
> the previous patch reveal to be suboptimal, once the notion of a per-
> ring default context has gone away. So this tidies up the leftovers.
> 
> It could have been squashed into the previous patch, but that would have
> made that patch less clearly a simple transformation. In particular, any
> change which alters the code block structure or indentation has been
> deferred into this separate patch, because such things tend to make diffs
> more difficult to read.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Yeah I think this was good to be split up, since I'm not convinced it's
a net improvement (from a quick look at least). Still plenty of default
context checks that seem superfluous (e.g. not pinning the default context
when going through execlist submission is imo a needless special case -
besides that we really should use active tracking).

So I'll refrain from ack or nack on this one.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++----------
>  drivers/gpu/drm/i915/i915_gem.c     |  6 ++----
>  drivers/gpu/drm/i915/intel_lrc.c    | 38 +++++++++++++++++--------------------
>  3 files changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2613708..bbb23da 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  
>  		seq_puts(m, "HW context ");
>  		describe_ctx(m, ctx);
> -		for_each_ring(ring, dev_priv, i) {
> -			if (dev_priv->kernel_context == ctx)
> -				seq_printf(m, "(default context %s) ",
> -					   ring->name);
> -		}
> +		if (ctx == dev_priv->kernel_context)
> +			seq_printf(m, "(kernel context) ");
>  
>  		if (i915.enable_execlists) {
>  			seq_putc(m, '\n');
> @@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>  	if (ret)
>  		return ret;
>  
> -	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> -		for_each_ring(ring, dev_priv, i) {
> -			if (dev_priv->kernel_context != ctx)
> +	list_for_each_entry(ctx, &dev_priv->context_list, link)
> +		if (ctx != dev_priv->kernel_context)
> +			for_each_ring(ring, dev_priv, i)
>  				i915_dump_lrc_obj(m, ring,
>  						  ctx->engine[i].state);
> -		}
> -	}
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8f101121..4f45eb2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref)
>  		i915_gem_request_remove_from_client(req);
>  
>  	if (ctx) {
> -		if (i915.enable_execlists) {
> -			if (ctx != req->i915->kernel_context)
> -				intel_lr_context_unpin(req);
> -		}
> +		if (i915.enable_execlists && ctx != req->i915->kernel_context)
> +			intel_lr_context_unpin(req);
>  
>  		i915_gem_context_unreference(ctx);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index aaaa5a3..8c4c9b9 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -660,16 +660,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>  
>  int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
>  
> -	if (request->ctx != request->i915->kernel_context) {
> -		ret = intel_lr_context_pin(request);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	if (i915.enable_guc_submission) {
>  		/*
>  		 * Check that the GuC has space for the request before
> @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>  			return ret;
>  	}
>  
> -	return 0;
> +	if (request->ctx != request->i915->kernel_context)
> +		ret = intel_lr_context_pin(request);
> +
> +	return ret;
>  }
>  
>  static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> @@ -2382,22 +2379,21 @@ void intel_lr_context_free(struct intel_context *ctx)
>  {
>  	int i;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = I915_NUM_RINGS; --i >= 0; ) {
> +		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
>  		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
>  
> -		if (ctx_obj) {
> -			struct intel_ringbuffer *ringbuf =
> -					ctx->engine[i].ringbuf;
> -			struct intel_engine_cs *ring = ringbuf->ring;
> +		if (!ctx_obj)
> +			continue;
>  
> -			if (ctx == ctx->i915->kernel_context) {
> -				intel_unpin_ringbuffer_obj(ringbuf);
> -				i915_gem_object_ggtt_unpin(ctx_obj);
> -			}
> -			WARN_ON(ctx->engine[ring->id].pin_count);
> -			intel_ringbuffer_free(ringbuf);
> -			drm_gem_object_unreference(&ctx_obj->base);
> +		if (ctx == ctx->i915->kernel_context) {
> +			intel_unpin_ringbuffer_obj(ringbuf);
> +			i915_gem_object_ggtt_unpin(ctx_obj);
>  		}
> +
> +		WARN_ON(ctx->engine[i].pin_count);
> +		intel_ringbuffer_free(ringbuf);
> +		drm_gem_object_unreference(&ctx_obj->base);
>  	}
>  }
>  
> @@ -2472,7 +2468,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
>   */
>  
>  int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> -				     struct intel_engine_cs *ring)
> +				    struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_gem_object *ctx_obj;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Tue, Jan 12, 2016 at 02:50:28PM +0100, Daniel Vetter wrote:
> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> > There are a number of places where the driver needs a request, but isn't
> > working on behalf of any specific user or in a specific context. At
> > present, we associate them with the per-engine default context. A future
> > patch will abolish those per-engine context pointers; but we can already
> > eliminate a lot of the references to them, just by making the allocator
> > allow NULL as a shorthand for "an appropriate context for this ring",
> > which will mean that the callers don't need to know anything about how
> > the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> > 
> > So this patch renames the existing i915_gem_request_alloc(), and makes
> > it local (static inline), and replaces it with a wrapper that provides
> > a default if the context is NULL, and also has a nicer calling
> > convention (doesn't require a pointer to an output parameter). Then we
> > change all callers to use the new convention:
> > OLD:
> > 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> > 	if (err) ...
> > NEW:
> > 	req = i915_gem_request_alloc(ring, user_ctx);
> > 	if (IS_ERR(req)) ...
> > OLD:
> > 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> > 	if (err) ...
> > NEW:
> > 	req = i915_gem_request_alloc(ring, NULL);
> > 	if (IS_ERR(req)) ...
> > 
> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
> >  drivers/gpu/drm/i915/i915_gem.c            | 55 +++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++---
> >  drivers/gpu/drm/i915/intel_display.c       |  6 ++--
> >  drivers/gpu/drm/i915/intel_lrc.c           |  9 +++--
> >  drivers/gpu/drm/i915/intel_overlay.c       | 24 ++++++-------
> >  6 files changed, 74 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c6dd4db..c2b000a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {
> >  
> >  };
> >  
> > -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> > -			   struct intel_context *ctx,
> > -			   struct drm_i915_gem_request **req_out);
> > +struct drm_i915_gem_request * __must_check
> > +i915_gem_request_alloc(struct intel_engine_cs *engine,
> > +		       struct intel_context *ctx);
> >  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
> >  void i915_gem_request_free(struct kref *req_ref);
> >  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6c60e04..c908ed1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2688,9 +2688,10 @@ void i915_gem_request_free(struct kref *req_ref)
> >  	kmem_cache_free(req->i915->requests, req);
> >  }
> >  
> > -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> > -			   struct intel_context *ctx,
> > -			   struct drm_i915_gem_request **req_out)
> > +static inline int
> > +__i915_gem_request_alloc(struct intel_engine_cs *ring,
> > +			 struct intel_context *ctx,
> > +			 struct drm_i915_gem_request **req_out)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> >  	struct drm_i915_gem_request *req;
> > @@ -2753,6 +2754,31 @@ err:
> >  	return ret;
> >  }
> >  
> > +/**
> > + * i915_gem_request_alloc - allocate a request structure
> > + *
> > + * @engine: engine that we wish to issue the request on.
> > + * @ctx: context that the request will be associated with.
> > + *       This can be NULL if the request is not directly related to
> > + *       any specific user context, in which case this function will
> > + *       choose an appropriate context to use.
> > + *
> > + * Returns a pointer to the allocated request if successful,
> > + * or an error code if not.
> > + */
> > +struct drm_i915_gem_request *
> > +i915_gem_request_alloc(struct intel_engine_cs *engine,
> > +		       struct intel_context *ctx)
> > +{
> > +	struct drm_i915_gem_request *req;
> > +	int err;
> > +
> > +	if (ctx == NULL)
> > +		ctx = engine->default_context;
> 
> I think NULL vs. explicit dev_priv->kernel_context (or whatever it is) is
> semantically equivalent enough that it doesn't matter. And we can easily
> sed this (or an entire patch series for easy rebasing) if we change our
> minds.

But we were removing the engine->default_context as it complicated the
rest of the code. I strongly prefer keeping the contexts explicit as
context separation should be first and foremost in the driver.
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
> But we were removing the engine->default_context as it complicated the
> rest of the code. I strongly prefer keeping the contexts explicit as
> context separation should be first and foremost in the driver.

$ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);

Changing those *two* callsites to pass NULL seems on the odd side, and
at least for the eviction case discards important information.
-Chris

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

Re: [PATCH v3 3/3] drm/i915: tidy up a few leftovers

[Dave Gordon]

On 12/01/16 13:53, Daniel Vetter wrote:
> On Thu, Jan 07, 2016 at 10:20:52AM +0000, Dave Gordon wrote:
>> There are a few bits of code which the transformations implemented by
>> the previous patch reveal to be suboptimal, once the notion of a per-
>> ring default context has gone away. So this tidies up the leftovers.
>>
>> It could have been squashed into the previous patch, but that would have
>> made that patch less clearly a simple transformation. In particular, any
>> change which alters the code block structure or indentation has been
>> deferred into this separate patch, because such things tend to make diffs
>> more difficult to read.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>
> Yeah I think this was good to be split up, since I'm not convinced it's
> a net improvement (from a quick look at least). Still plenty of default
> context checks that seem superfluous (e.g. not pinning the default context
> when going through execlist submission is imo a needless special case -
> besides that we really should use active tracking).
>
> So I'll refrain from ack or nack on this one.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++----------
>>   drivers/gpu/drm/i915/i915_gem.c     |  6 ++----
>>   drivers/gpu/drm/i915/intel_lrc.c    | 38 +++++++++++++++++--------------------
>>   3 files changed, 24 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 2613708..bbb23da 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
>>
>>   		seq_puts(m, "HW context ");
>>   		describe_ctx(m, ctx);
>> -		for_each_ring(ring, dev_priv, i) {
>> -			if (dev_priv->kernel_context == ctx)
>> -				seq_printf(m, "(default context %s) ",
>> -					   ring->name);
>> -		}
>> +		if (ctx == dev_priv->kernel_context)
>> +			seq_printf(m, "(kernel context) ");

This is replacing the multiple messages in the debugfs output:
"(default context render ring) (default context blitter ring) (default 
context ..."
(which are redundant because the same context is the default for all 
rings) with the single more concise message "(kernel context)".

One part of one of Chris' patches does the same or an equivalent thing, 
so I don't think it's contentious.

>>
>>   		if (i915.enable_execlists) {
>>   			seq_putc(m, '\n');
>> @@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>>   	if (ret)
>>   		return ret;
>>
>> -	list_for_each_entry(ctx, &dev_priv->context_list, link) {
>> -		for_each_ring(ring, dev_priv, i) {
>> -			if (dev_priv->kernel_context != ctx)
>> +	list_for_each_entry(ctx, &dev_priv->context_list, link)
>> +		if (ctx != dev_priv->kernel_context)
>> +			for_each_ring(ring, dev_priv, i)
>>   				i915_dump_lrc_obj(m, ring,
>>   						  ctx->engine[i].state);
>> -		}
>> -	}

Since we know that there is only one kernel context, the if() should be 
outside rather than outside the for_each_ring(). The {} are redundant 
anyway.

>>
>>   	mutex_unlock(&dev->struct_mutex);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 8f101121..4f45eb2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref)
>>   		i915_gem_request_remove_from_client(req);
>>
>>   	if (ctx) {
>> -		if (i915.enable_execlists) {
>> -			if (ctx != req->i915->kernel_context)
>> -				intel_lr_context_unpin(req);
>> -		}
>> +		if (i915.enable_execlists && ctx != req->i915->kernel_context)
>> +			intel_lr_context_unpin(req);

Trivial rewrite of the nested if() to reduce indentation.

Actually removing this test is left for a subsequent rationalisation of 
the execlist code (one of Chris' patches will do it).

>>
>>   		i915_gem_context_unreference(ctx);
>>   	}
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index aaaa5a3..8c4c9b9 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -660,16 +660,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>>
>>   int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>>   {
>> -	int ret;
>> +	int ret = 0;
>>
>>   	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
>>
>> -	if (request->ctx != request->i915->kernel_context) {
>> -		ret = intel_lr_context_pin(request);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>>   	if (i915.enable_guc_submission) {
>>   		/*
>>   		 * Check that the GuC has space for the request before
>> @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>>   			return ret;
>>   	}
>>
>> -	return 0;
>> +	if (request->ctx != request->i915->kernel_context)
>> +		ret = intel_lr_context_pin(request);
>> +
>> +	return ret;
>>   }

Two things here, one is restructuring the if/ret tests, which is 
trivial. More importantly, the pinning is moved to AFTER the GuC space 
pre-check, otherwise the pinning is not undone if this check fails.

I can move this to a separate patch if required, as it's actually a fix 
for a potential bug.

>>
>>   static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
>> @@ -2382,22 +2379,21 @@ void intel_lr_context_free(struct intel_context *ctx)
>>   {
>>   	int i;
>>
>> -	for (i = 0; i < I915_NUM_RINGS; i++) {
>> +	for (i = I915_NUM_RINGS; --i >= 0; ) {
>> +		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
>>   		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
>>
>> -		if (ctx_obj) {
>> -			struct intel_ringbuffer *ringbuf =
>> -					ctx->engine[i].ringbuf;
>> -			struct intel_engine_cs *ring = ringbuf->ring;
>> +		if (!ctx_obj)
>> +			continue;
>>
>> -			if (ctx == ctx->i915->kernel_context) {
>> -				intel_unpin_ringbuffer_obj(ringbuf);
>> -				i915_gem_object_ggtt_unpin(ctx_obj);
>> -			}
>> -			WARN_ON(ctx->engine[ring->id].pin_count);
>> -			intel_ringbuffer_free(ringbuf);
>> -			drm_gem_object_unreference(&ctx_obj->base);
>> +		if (ctx == ctx->i915->kernel_context) {
>> +			intel_unpin_ringbuffer_obj(ringbuf);
>> +			i915_gem_object_ggtt_unpin(ctx_obj);
>>   		}
>> +
>> +		WARN_ON(ctx->engine[i].pin_count);
>> +		intel_ringbuffer_free(ringbuf);
>> +		drm_gem_object_unreference(&ctx_obj->base);
>>   	}
>>   }

Again two things; one is the trivial restructuring of the loop using 
continue to reduce indentation depth.

The other (more interesting) one is to run the teardown loop in reverse. 
This avoided a bug in some *other* destructor-type code that got called 
from here, which assumed that RCS state was still valid while releasing 
resources on the other rings. That specific bug has been eliminated by 
the change from per-engine to a global default context, but nonetheless 
it seems a good policy to release resources in reverse order of 
acquisition; hence teardown loops should in general run backwards just 
in case there are cross-engine dependencies.

>>
>> @@ -2472,7 +2468,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
>>    */
>>
>>   int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>> -				     struct intel_engine_cs *ring)
>> +				    struct intel_engine_cs *ring)

Trivial whitespace fix.

>>   {
>>   	struct drm_device *dev = ring->dev;
>>   	struct drm_i915_gem_object *ctx_obj;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Let me know whether this all looks OK as-is, with the explanations above 
('cos I don't think there's anything problematic there), or whether some 
bits should be posted separately. The only bit that really matters is 
the pinning-vs-GuC-precheck error path fix, and that error won't be 
possible once the scheduler is in, so it's just to avoid leaks if it 
happens in the short term.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 12/01/16 14:27, Chris Wilson wrote:
> On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
>> But we were removing the engine->default_context as it complicated the
>> rest of the code. I strongly prefer keeping the contexts explicit as
>> context separation should be first and foremost in the driver.
>
> $ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
> drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
> drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);
>
> Changing those *two* callsites to pass NULL seems on the odd side, and
> at least for the eviction case discards important information.
> -Chris

Those specific lines won't be touched by my patch, as *they don't 
actually exist in today's drm-intel-nightly* branch. If you want to add 
*new* calls to i915_gem_request_alloc() such as the above then you're 
quite free to pass any context you want, whether it's a real user 
context, the default kernel context explicitly, if you think it's 
important that the reader know that that specific context will be used; 
or NULL if you don't care what context is used.

dev_priv->kernel_context carries exactly the same amount of information 
as NULL; they both mean "I don't have a specific context to use here, so 
(I'm going to) use the one the driver provides for such activities". 
Having the option of using NULL rather than"dev_priv->kernel_context" 
explicitly doesn't prevent you from doing so where the caller cares. But 
I think most callers *don't* care.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Wed, Jan 13, 2016 at 01:27:51PM +0000, Dave Gordon wrote:
> On 12/01/16 14:27, Chris Wilson wrote:
> >On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
> >>But we were removing the engine->default_context as it complicated the
> >>rest of the code. I strongly prefer keeping the contexts explicit as
> >>context separation should be first and foremost in the driver.
> >
> >$ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
> >drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >
> >Changing those *two* callsites to pass NULL seems on the odd side, and
> >at least for the eviction case discards important information.
> >-Chris
> 
> Those specific lines won't be touched by my patch, as *they don't
> actually exist in today's drm-intel-nightly* branch. If you want to
> add *new* calls to i915_gem_request_alloc() such as the above then
> you're quite free to pass any context you want, whether it's a real
> user context, the default kernel context explicitly, if you think
> it's important that the reader know that that specific context will
> be used; or NULL if you don't care what context is used.

They are the same calls as the ones you are patching. They are not new
calls, they are the only users of the kernel_context for emission. Which
is why I am suggesting a different series of steps to take in tidying
this up.
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 13/01/16 13:41, Chris Wilson wrote:
> On Wed, Jan 13, 2016 at 01:27:51PM +0000, Dave Gordon wrote:
>> On 12/01/16 14:27, Chris Wilson wrote:
>>> On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
>>>> But we were removing the engine->default_context as it complicated the
>>>> rest of the code. I strongly prefer keeping the contexts explicit as
>>>> context separation should be first and foremost in the driver.
>>>
>>> $ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
>>> drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
>>> drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);
>>>
>>> Changing those *two* callsites to pass NULL seems on the odd side, and
>>> at least for the eviction case discards important information.
>>> -Chris
>>
>> Those specific lines won't be touched by my patch, as *they don't
>> actually exist in today's drm-intel-nightly* branch. If you want to
>> add *new* calls to i915_gem_request_alloc() such as the above then
>> you're quite free to pass any context you want, whether it's a real
>> user context, the default kernel context explicitly, if you think
>> it's important that the reader know that that specific context will
>> be used; or NULL if you don't care what context is used.
>
> They are the same calls as the ones you are patching. They are not new
> calls, they are the only users of the kernel_context for emission. Which
> is why I am suggesting a different series of steps to take in tidying
> this up.
> -Chris

As of now (i.e. pre-conversion of the default_context pointer), there 
are *eight* calls to i915_gem_request_alloc(), but NONE in i915_gem_evict.c:

drm-intel-nightly$ git grep -c 
'i915_gem_request_alloc(.*default_context' -- drivers/gpu/drm/i915/
drivers/gpu/drm/i915/i915_gem.c:3
drivers/gpu/drm/i915/intel_display.c:1
drivers/gpu/drm/i915/intel_overlay.c:4

So you must be looking in a different tree, presumably one where you've 
already done some other bunch of cleanups in a different order.

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Wed, Jan 13, 2016 at 06:46:08PM +0000, Dave Gordon wrote:
> On 13/01/16 13:41, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 01:27:51PM +0000, Dave Gordon wrote:
> >>On 12/01/16 14:27, Chris Wilson wrote:
> >>>On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
> >>>>But we were removing the engine->default_context as it complicated the
> >>>>rest of the code. I strongly prefer keeping the contexts explicit as
> >>>>context separation should be first and foremost in the driver.
> >>>
> >>>$ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
> >>>drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >>>drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >>>
> >>>Changing those *two* callsites to pass NULL seems on the odd side, and
> >>>at least for the eviction case discards important information.
> >>>-Chris
> >>
> >>Those specific lines won't be touched by my patch, as *they don't
> >>actually exist in today's drm-intel-nightly* branch. If you want to
> >>add *new* calls to i915_gem_request_alloc() such as the above then
> >>you're quite free to pass any context you want, whether it's a real
> >>user context, the default kernel context explicitly, if you think
> >>it's important that the reader know that that specific context will
> >>be used; or NULL if you don't care what context is used.
> >
> >They are the same calls as the ones you are patching. They are not new
> >calls, they are the only users of the kernel_context for emission. Which
> >is why I am suggesting a different series of steps to take in tidying
> >this up.
> >-Chris
> 
> As of now (i.e. pre-conversion of the default_context pointer),
> there are *eight* calls to i915_gem_request_alloc(), but NONE in
> i915_gem_evict.c:
> 
> drm-intel-nightly$ git grep -c
> 'i915_gem_request_alloc(.*default_context' -- drivers/gpu/drm/i915/
> drivers/gpu/drm/i915/i915_gem.c:3
> drivers/gpu/drm/i915/intel_display.c:1
> drivers/gpu/drm/i915/intel_overlay.c:4
> 
> So you must be looking in a different tree, presumably one where
> you've already done some other bunch of cleanups in a different
> order.

Exactly. I disagree with the notion of an implicit context, and have
outlined a course of action where there is only a single active user of
the kernel context (overlay). And if we talk nicely to Ville we could
eliminate the overlay as well. The other use of that kernel context for
request construction is then to provide a means to flush other contexts
from the GPU - we don't use it for anything other than pinned storage.

For execlists we can do away with the kernel context entirely, we don't
even need it for the HWS if we just switch to per-context seqno. (It is
not required for flushing the last context.) I have no idea what the guc
is doing with it though, or whether it is being set up in hardware just
because it is there (even though we never use it).
-Chris

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

Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Nick Hoath]

On 07/01/2016 10:20, Dave Gordon wrote:
> There are a number of places where the driver needs a request, but isn't
> working on behalf of any specific user or in a specific context. At
> present, we associate them with the per-engine default context. A future
> patch will abolish those per-engine context pointers; but we can already
> eliminate a lot of the references to them, just by making the allocator
> allow NULL as a shorthand for "an appropriate context for this ring",
> which will mean that the callers don't need to know anything about how
> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>
> So this patch renames the existing i915_gem_request_alloc(), and makes
> it local (static inline), and replaces it with a wrapper that provides
> a default if the context is NULL, and also has a nicer calling
> convention (doesn't require a pointer to an output parameter). Then we
> change all callers to use the new convention:
> OLD:
> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, user_ctx);
> 	if (IS_ERR(req)) ...
> OLD:
> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> 	if (err) ...
> NEW:
> 	req = i915_gem_request_alloc(ring, NULL);
> 	if (IS_ERR(req)) ...
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
>   drivers/gpu/drm/i915/i915_gem.c            | 55 +++++++++++++++++++++++-------
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++---
>   drivers/gpu/drm/i915/intel_display.c       |  6 ++--
>   drivers/gpu/drm/i915/intel_lrc.c           |  9 +++--
>   drivers/gpu/drm/i915/intel_overlay.c       | 24 ++++++-------
>   6 files changed, 74 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c6dd4db..c2b000a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {
>
>   };
>
> -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> -			   struct intel_context *ctx,
> -			   struct drm_i915_gem_request **req_out);
> +struct drm_i915_gem_request * __must_check
> +i915_gem_request_alloc(struct intel_engine_cs *engine,
> +		       struct intel_context *ctx);
>   void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>   void i915_gem_request_free(struct kref *req_ref);
>   int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6c60e04..c908ed1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2688,9 +2688,10 @@ void i915_gem_request_free(struct kref *req_ref)
>   	kmem_cache_free(req->i915->requests, req);
>   }
>
> -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> -			   struct intel_context *ctx,
> -			   struct drm_i915_gem_request **req_out)
> +static inline int
> +__i915_gem_request_alloc(struct intel_engine_cs *ring,
> +			 struct intel_context *ctx,
> +			 struct drm_i915_gem_request **req_out)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(ring->dev);
>   	struct drm_i915_gem_request *req;
> @@ -2753,6 +2754,31 @@ err:
>   	return ret;
>   }
>
> +/**
> + * i915_gem_request_alloc - allocate a request structure
> + *
> + * @engine: engine that we wish to issue the request on.
> + * @ctx: context that the request will be associated with.
> + *       This can be NULL if the request is not directly related to
> + *       any specific user context, in which case this function will
> + *       choose an appropriate context to use.
> + *
> + * Returns a pointer to the allocated request if successful,
> + * or an error code if not.
> + */
> +struct drm_i915_gem_request *
> +i915_gem_request_alloc(struct intel_engine_cs *engine,
> +		       struct intel_context *ctx)
> +{
> +	struct drm_i915_gem_request *req;
> +	int err;
> +
> +	if (ctx == NULL)
> +		ctx = engine->default_context;
> +	err = __i915_gem_request_alloc(engine, ctx, &req);
> +	return err ? ERR_PTR(err) : req;
> +}
> +
>   void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>   {
>   	intel_ring_reserved_space_cancel(req->ringbuf);
> @@ -3170,9 +3196,13 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   			return 0;
>
>   		if (*to_req == NULL) {
> -			ret = i915_gem_request_alloc(to, to->default_context, to_req);
> -			if (ret)
> -				return ret;
> +			struct drm_i915_gem_request *req;
> +
> +			req = i915_gem_request_alloc(to, NULL);
> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
> +
> +			*to_req = req;
>   		}
>
>   		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
> @@ -3372,9 +3402,9 @@ int i915_gpu_idle(struct drm_device *dev)
>   		if (!i915.enable_execlists) {
>   			struct drm_i915_gem_request *req;
>
> -			ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -			if (ret)
> -				return ret;
> +			req = i915_gem_request_alloc(ring, NULL);
> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
>
>   			ret = i915_switch_context(req);
>   			if (ret) {
> @@ -4869,10 +4899,9 @@ i915_gem_init_hw(struct drm_device *dev)
>   	for_each_ring(ring, dev_priv, i) {
>   		struct drm_i915_gem_request *req;
>
> -		WARN_ON(!ring->default_context);
> -
> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -		if (ret) {
> +		req = i915_gem_request_alloc(ring, NULL);
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
>   			i915_gem_cleanup_ringbuffer(dev);
>   			goto out;
>   		}
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index dccb517..dc6caeb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1381,6 +1381,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   		       struct drm_i915_gem_exec_object2 *exec)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_request *req = NULL;
>   	struct eb_vmas *eb;
>   	struct drm_i915_gem_object *batch_obj;
>   	struct drm_i915_gem_exec_object2 shadow_exec_entry;
> @@ -1602,11 +1603,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>
>   	/* Allocate a request for this batch buffer nice and early. */
> -	ret = i915_gem_request_alloc(ring, ctx, &params->request);
> -	if (ret)
> +	req = i915_gem_request_alloc(ring, ctx);
> +	if (IS_ERR(req)) {
> +		ret = PTR_ERR(req);
>   		goto err_batch_unpin;
> +	}
>
> -	ret = i915_gem_request_add_to_client(params->request, file);
> +	ret = i915_gem_request_add_to_client(req, file);
>   	if (ret)
>   		goto err_batch_unpin;
>
> @@ -1622,6 +1625,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   	params->dispatch_flags          = dispatch_flags;
>   	params->batch_obj               = batch_obj;
>   	params->ctx                     = ctx;
> +	params->request                 = req;
>
>   	ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
>
> @@ -1645,8 +1649,8 @@ err:
>   	 * must be freed again. If it was submitted then it is being tracked
>   	 * on the active request list and no clean up is required here.
>   	 */
> -	if (ret && params->request)
> -		i915_gem_request_cancel(params->request);
> +	if (ret && req)
> +		i915_gem_request_cancel(req);
>
>   	mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed9ab60..1f8f781 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11700,9 +11700,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   					obj->last_write_req);
>   	} else {
>   		if (!request) {
> -			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
> -			if (ret)
> +			request = i915_gem_request_alloc(ring, NULL);
> +			if (IS_ERR(request)) {
> +				ret = PTR_ERR(request);
>   				goto cleanup_unpin;
> +			}
>   		}
>
>   		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8096c6a..8b6542d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2510,11 +2510,10 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>   	if (ctx != ring->default_context && ring->init_context) {
>   		struct drm_i915_gem_request *req;
>
> -		ret = i915_gem_request_alloc(ring,
> -			ctx, &req);
> -		if (ret) {
> -			DRM_ERROR("ring create req: %d\n",
> -				ret);
> +		req = i915_gem_request_alloc(ring, ctx);
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
> +			DRM_ERROR("ring create req: %d\n", ret);
>   			goto error_ringbuf;
>   		}
>
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 76f1980..9168413 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -240,9 +240,9 @@ static int intel_overlay_on(struct intel_overlay *overlay)
>   	WARN_ON(overlay->active);
>   	WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
>
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>
>   	ret = intel_ring_begin(req, 4);
>   	if (ret) {
> @@ -283,9 +283,9 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
>   	if (tmp & (1 << 17))
>   		DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
>
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>
>   	ret = intel_ring_begin(req, 2);
>   	if (ret) {
> @@ -349,9 +349,9 @@ static int intel_overlay_off(struct intel_overlay *overlay)
>   	 * of the hw. Do it in both cases */
>   	flip_addr |= OFC_UPDATE;
>
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -	if (ret)
> -		return ret;
> +	req = i915_gem_request_alloc(ring, NULL);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
>
>   	ret = intel_ring_begin(req, 6);
>   	if (ret) {
> @@ -423,9 +423,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>   		/* synchronous slowpath */
>   		struct drm_i915_gem_request *req;
>
> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -		if (ret)
> -			return ret;
> +		req = i915_gem_request_alloc(ring, NULL);
> +		if (IS_ERR(req))
> +			return PTR_ERR(req);
>
>   		ret = intel_ring_begin(req, 2);
>   		if (ret) {
>

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

Re: [PATCH v3 2/3] drm/i915: abolish separate per-ring default_context pointers

[Nick Hoath]

On 07/01/2016 10:20, Dave Gordon wrote:
> Now that we've eliminated a lot of uses of ring->default_context,
> we can eliminate the pointer itself.
>
> All the engines share the same default intel_context, so we can just
> keep a single reference to it in the dev_priv structure rather than one
> in each of the engine[] elements. This make refcounting more sensible
> too, as we now have a refcount of one for the one pointer, rather than
> a refcount of one but multiple pointers.
>
>  From an idea by Chris Wilson.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++--
>   drivers/gpu/drm/i915/i915_drv.h            |  2 ++
>   drivers/gpu/drm/i915/i915_gem.c            |  6 +++---
>   drivers/gpu/drm/i915/i915_gem_context.c    | 22 ++++++++--------------
>   drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
>   drivers/gpu/drm/i915/i915_guc_submission.c |  6 +++---
>   drivers/gpu/drm/i915/intel_lrc.c           | 24 +++++++++++++-----------
>   drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
>   8 files changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0fc38bb..2613708 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1943,7 +1943,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>   		seq_puts(m, "HW context ");
>   		describe_ctx(m, ctx);
>   		for_each_ring(ring, dev_priv, i) {
> -			if (ring->default_context == ctx)
> +			if (dev_priv->kernel_context == ctx)
>   				seq_printf(m, "(default context %s) ",
>   					   ring->name);
>   		}
> @@ -2039,7 +2039,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>
>   	list_for_each_entry(ctx, &dev_priv->context_list, link) {
>   		for_each_ring(ring, dev_priv, i) {
> -			if (ring->default_context != ctx)
> +			if (dev_priv->kernel_context != ctx)
>   				i915_dump_lrc_obj(m, ring,
>   						  ctx->engine[i].state);
>   		}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c2b000a..aef86a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1940,6 +1940,8 @@ struct drm_i915_private {
>   		void (*stop_ring)(struct intel_engine_cs *ring);
>   	} gt;
>
> +	struct intel_context *kernel_context;
> +
>   	bool edp_low_vswing;
>
>   	/* perform PHY state sanity checks? */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c908ed1..8f101121 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2678,7 +2678,7 @@ void i915_gem_request_free(struct kref *req_ref)
>
>   	if (ctx) {
>   		if (i915.enable_execlists) {
> -			if (ctx != req->ring->default_context)
> +			if (ctx != req->i915->kernel_context)
>   				intel_lr_context_unpin(req);
>   		}
>
> @@ -2774,7 +2774,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>   	int err;
>
>   	if (ctx == NULL)
> -		ctx = engine->default_context;
> +		ctx = to_i915(engine->dev)->kernel_context;
>   	err = __i915_gem_request_alloc(engine, ctx, &req);
>   	return err ? ERR_PTR(err) : req;
>   }
> @@ -4862,7 +4862,7 @@ i915_gem_init_hw(struct drm_device *dev)
>   	 */
>   	init_unused_rings(dev);
>
> -	BUG_ON(!dev_priv->ring[RCS].default_context);
> +	BUG_ON(!dev_priv->kernel_context);
>
>   	ret = i915_ppgtt_init_hw(dev);
>   	if (ret) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 900ffd0..e1d767e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -354,11 +354,10 @@ int i915_gem_context_init(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_context *ctx;
> -	int i;
>
>   	/* Init should only be called once per module load. Eventually the
>   	 * restriction on the context_disabled check can be loosened. */
> -	if (WARN_ON(dev_priv->ring[RCS].default_context))
> +	if (WARN_ON(dev_priv->kernel_context))
>   		return 0;
>
>   	if (intel_vgpu_active(dev) && HAS_LOGICAL_RING_CONTEXTS(dev)) {
> @@ -388,12 +387,7 @@ int i915_gem_context_init(struct drm_device *dev)
>   		return PTR_ERR(ctx);
>   	}
>
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		struct intel_engine_cs *ring = &dev_priv->ring[i];
> -
> -		/* NB: RCS will hold a ref for all rings */
> -		ring->default_context = ctx;
> -	}
> +	dev_priv->kernel_context = ctx;
>
>   	DRM_DEBUG_DRIVER("%s context support initialized\n",
>   			i915.enable_execlists ? "LR" :
> @@ -404,7 +398,7 @@ int i915_gem_context_init(struct drm_device *dev)
>   void i915_gem_context_fini(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
> +	struct intel_context *dctx = dev_priv->kernel_context;
>   	int i;
>
>   	if (dctx->legacy_hw_ctx.rcs_state) {
> @@ -431,17 +425,17 @@ void i915_gem_context_fini(struct drm_device *dev)
>   		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
>   	}
>
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = I915_NUM_RINGS; --i >= 0;) {
>   		struct intel_engine_cs *ring = &dev_priv->ring[i];
>
> -		if (ring->last_context)
> +		if (ring->last_context) {
>   			i915_gem_context_unreference(ring->last_context);
> -
> -		ring->default_context = NULL;
> -		ring->last_context = NULL;
> +			ring->last_context = NULL;
> +		}
>   	}
>
>   	i915_gem_context_unreference(dctx);
> +	dev_priv->kernel_context = NULL;
>   }
>
>   int i915_gem_context_enable(struct drm_i915_gem_request *req)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 06ca408..7eeb244 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1050,7 +1050,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>   			if (request)
>   				rbuf = request->ctx->engine[ring->id].ringbuf;
>   			else
> -				rbuf = ring->default_context->engine[ring->id].ringbuf;
> +				rbuf = dev_priv->kernel_context->engine[ring->id].ringbuf;
>   		} else
>   			rbuf = ring->buffer;
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9c24424..51ae5c1 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -964,7 +964,7 @@ int i915_guc_submission_enable(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc *guc = &dev_priv->guc;
> -	struct intel_context *ctx = dev_priv->ring[RCS].default_context;
> +	struct intel_context *ctx = dev_priv->kernel_context;
>   	struct i915_guc_client *client;
>
>   	/* client for execbuf submission */
> @@ -1021,7 +1021,7 @@ int intel_guc_suspend(struct drm_device *dev)
>   	if (!i915.enable_guc_submission)
>   		return 0;
>
> -	ctx = dev_priv->ring[RCS].default_context;
> +	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
>   	/* any value greater than GUC_POWER_D0 */
> @@ -1047,7 +1047,7 @@ int intel_guc_resume(struct drm_device *dev)
>   	if (!i915.enable_guc_submission)
>   		return 0;
>
> -	ctx = dev_priv->ring[RCS].default_context;
> +	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
>   	data[1] = GUC_POWER_D0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8b6542d..aaaa5a3 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -571,7 +571,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
>   	struct drm_i915_gem_request *cursor;
>   	int num_elements = 0;
>
> -	if (request->ctx != ring->default_context)
> +	if (request->ctx != request->i915->kernel_context)
>   		intel_lr_context_pin(request);
>
>   	i915_gem_request_reference(request);
> @@ -664,7 +664,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>
>   	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
>
> -	if (request->ctx != request->ring->default_context) {
> +	if (request->ctx != request->i915->kernel_context) {
>   		ret = intel_lr_context_pin(request);
>   		if (ret)
>   			return ret;
> @@ -980,7 +980,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
>   		struct drm_i915_gem_object *ctx_obj =
>   				ctx->engine[ring->id].state;
>
> -		if (ctx_obj && (ctx != ring->default_context))
> +		if (ctx_obj && (ctx != req->i915->kernel_context))
>   			intel_lr_context_unpin(req);
>   		list_del(&req->execlist_link);
>   		i915_gem_request_unreference(req);
> @@ -1487,7 +1487,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>   	u8 next_context_status_buffer_hw;
>
>   	lrc_setup_hardware_status_page(ring,
> -				ring->default_context->engine[ring->id].state);
> +				dev_priv->kernel_context->engine[ring->id].state);
>
>   	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
>   	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
> @@ -1929,6 +1929,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>
>   static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_context *dctx = dev_priv->kernel_context;
> +	int ring_id = ring->id;
>   	int ret;
>
>   	/* Intentionally left blank. */
> @@ -1949,15 +1952,14 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>   	if (ret)
>   		goto error;
>
> -	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
> +	ret = intel_lr_context_deferred_alloc(dctx, ring);
>   	if (ret)
>   		goto error;
>
>   	/* As this is the default context, always pin it */
> -	ret = intel_lr_context_do_pin(
> -			ring,
> -			ring->default_context->engine[ring->id].state,
> -			ring->default_context->engine[ring->id].ringbuf);
> +	ret = intel_lr_context_do_pin(ring,
> +				      dctx->engine[ring_id].state,
> +				      dctx->engine[ring_id].ringbuf);
>   	if (ret) {
>   		DRM_ERROR(
>   			"Failed to pin and map ringbuffer %s: %d\n",
> @@ -2388,7 +2390,7 @@ void intel_lr_context_free(struct intel_context *ctx)
>   					ctx->engine[i].ringbuf;
>   			struct intel_engine_cs *ring = ringbuf->ring;
>
> -			if (ctx == ring->default_context) {
> +			if (ctx == ctx->i915->kernel_context) {
>   				intel_unpin_ringbuffer_obj(ringbuf);
>   				i915_gem_object_ggtt_unpin(ctx_obj);
>   			}
> @@ -2507,7 +2509,7 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>   	ctx->engine[ring->id].ringbuf = ringbuf;
>   	ctx->engine[ring->id].state = ctx_obj;
>
> -	if (ctx != ring->default_context && ring->init_context) {
> +	if (ctx != ctx->i915->kernel_context && ring->init_context) {
>   		struct drm_i915_gem_request *req;
>
>   		req = i915_gem_request_alloc(ring, ctx);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 49574ff..e950f6c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -305,7 +305,6 @@ struct  intel_engine_cs {
>
>   	wait_queue_head_t irq_queue;
>
> -	struct intel_context *default_context;
>   	struct intel_context *last_context;
>
>   	struct intel_ring_hangcheck hangcheck;
>

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

Re: [PATCH v3 3/3] drm/i915: tidy up a few leftovers

[Nick Hoath]

On 07/01/2016 10:20, Dave Gordon wrote:
> There are a few bits of code which the transformations implemented by
> the previous patch reveal to be suboptimal, once the notion of a per-
> ring default context has gone away. So this tidies up the leftovers.
>
> It could have been squashed into the previous patch, but that would have
> made that patch less clearly a simple transformation. In particular, any
> change which alters the code block structure or indentation has been
> deferred into this separate patch, because such things tend to make diffs
> more difficult to read.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++----------
>   drivers/gpu/drm/i915/i915_gem.c     |  6 ++----
>   drivers/gpu/drm/i915/intel_lrc.c    | 38 +++++++++++++++++--------------------
>   3 files changed, 24 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2613708..bbb23da 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
>
>   		seq_puts(m, "HW context ");
>   		describe_ctx(m, ctx);
> -		for_each_ring(ring, dev_priv, i) {
> -			if (dev_priv->kernel_context == ctx)
> -				seq_printf(m, "(default context %s) ",
> -					   ring->name);
> -		}
> +		if (ctx == dev_priv->kernel_context)
> +			seq_printf(m, "(kernel context) ");
>
>   		if (i915.enable_execlists) {
>   			seq_putc(m, '\n');
> @@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>   	if (ret)
>   		return ret;
>
> -	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> -		for_each_ring(ring, dev_priv, i) {
> -			if (dev_priv->kernel_context != ctx)
> +	list_for_each_entry(ctx, &dev_priv->context_list, link)
> +		if (ctx != dev_priv->kernel_context)
> +			for_each_ring(ring, dev_priv, i)
>   				i915_dump_lrc_obj(m, ring,
>   						  ctx->engine[i].state);
> -		}
> -	}
>
>   	mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8f101121..4f45eb2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref)
>   		i915_gem_request_remove_from_client(req);
>
>   	if (ctx) {
> -		if (i915.enable_execlists) {
> -			if (ctx != req->i915->kernel_context)
> -				intel_lr_context_unpin(req);
> -		}
> +		if (i915.enable_execlists && ctx != req->i915->kernel_context)
> +			intel_lr_context_unpin(req);
>
>   		i915_gem_context_unreference(ctx);
>   	}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index aaaa5a3..8c4c9b9 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -660,16 +660,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>
>   int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>   {
> -	int ret;
> +	int ret = 0;
>
>   	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
>
> -	if (request->ctx != request->i915->kernel_context) {
> -		ret = intel_lr_context_pin(request);
> -		if (ret)
> -			return ret;
> -	}
> -
>   	if (i915.enable_guc_submission) {
>   		/*
>   		 * Check that the GuC has space for the request before
> @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   			return ret;
>   	}
>
> -	return 0;
> +	if (request->ctx != request->i915->kernel_context)
> +		ret = intel_lr_context_pin(request);
> +
> +	return ret;
>   }
>
>   static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> @@ -2382,22 +2379,21 @@ void intel_lr_context_free(struct intel_context *ctx)
>   {
>   	int i;
>
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = I915_NUM_RINGS; --i >= 0; ) {
> +		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
>   		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
>
> -		if (ctx_obj) {
> -			struct intel_ringbuffer *ringbuf =
> -					ctx->engine[i].ringbuf;
> -			struct intel_engine_cs *ring = ringbuf->ring;
> +		if (!ctx_obj)
> +			continue;
>
> -			if (ctx == ctx->i915->kernel_context) {
> -				intel_unpin_ringbuffer_obj(ringbuf);
> -				i915_gem_object_ggtt_unpin(ctx_obj);
> -			}
> -			WARN_ON(ctx->engine[ring->id].pin_count);
> -			intel_ringbuffer_free(ringbuf);
> -			drm_gem_object_unreference(&ctx_obj->base);
> +		if (ctx == ctx->i915->kernel_context) {
> +			intel_unpin_ringbuffer_obj(ringbuf);
> +			i915_gem_object_ggtt_unpin(ctx_obj);
>   		}
> +
> +		WARN_ON(ctx->engine[i].pin_count);
> +		intel_ringbuffer_free(ringbuf);
> +		drm_gem_object_unreference(&ctx_obj->base);
>   	}
>   }
>
> @@ -2472,7 +2468,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
>    */
>
>   int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> -				     struct intel_engine_cs *ring)
> +				    struct intel_engine_cs *ring)
>   {
>   	struct drm_device *dev = ring->dev;
>   	struct drm_i915_gem_object *ctx_obj;
>

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

Re: [PATCH v3 2/3] drm/i915: abolish separate per-ring default_context pointers

[Daniel Vetter]

On Mon, Jan 18, 2016 at 04:16:39PM +0000, Nick Hoath wrote:
> On 07/01/2016 10:20, Dave Gordon wrote:
> >Now that we've eliminated a lot of uses of ring->default_context,
> >we can eliminate the pointer itself.
> >
> >All the engines share the same default intel_context, so we can just
> >keep a single reference to it in the dev_priv structure rather than one
> >in each of the engine[] elements. This make refcounting more sensible
> >too, as we now have a refcount of one for the one pointer, rather than
> >a refcount of one but multiple pointers.
> >
> > From an idea by Chris Wilson.
> >
> >Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> 
> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

In the interest of hampering my popularity I wanted to to merge the first
two patches in this series, but this one here conflicts with patches
Tvrtko just merged. Also this seems to have fallen into a period where CI
was down, so lacking bat results too. Can you pls resend?

Thanks, Daniel
> 
> >---
> >  drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++--
> >  drivers/gpu/drm/i915/i915_drv.h            |  2 ++
> >  drivers/gpu/drm/i915/i915_gem.c            |  6 +++---
> >  drivers/gpu/drm/i915/i915_gem_context.c    | 22 ++++++++--------------
> >  drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
> >  drivers/gpu/drm/i915/i915_guc_submission.c |  6 +++---
> >  drivers/gpu/drm/i915/intel_lrc.c           | 24 +++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
> >  8 files changed, 32 insertions(+), 35 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 0fc38bb..2613708 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -1943,7 +1943,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
> >  		seq_puts(m, "HW context ");
> >  		describe_ctx(m, ctx);
> >  		for_each_ring(ring, dev_priv, i) {
> >-			if (ring->default_context == ctx)
> >+			if (dev_priv->kernel_context == ctx)
> >  				seq_printf(m, "(default context %s) ",
> >  					   ring->name);
> >  		}
> >@@ -2039,7 +2039,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
> >
> >  	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> >  		for_each_ring(ring, dev_priv, i) {
> >-			if (ring->default_context != ctx)
> >+			if (dev_priv->kernel_context != ctx)
> >  				i915_dump_lrc_obj(m, ring,
> >  						  ctx->engine[i].state);
> >  		}
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c2b000a..aef86a8 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -1940,6 +1940,8 @@ struct drm_i915_private {
> >  		void (*stop_ring)(struct intel_engine_cs *ring);
> >  	} gt;
> >
> >+	struct intel_context *kernel_context;
> >+
> >  	bool edp_low_vswing;
> >
> >  	/* perform PHY state sanity checks? */
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index c908ed1..8f101121 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2678,7 +2678,7 @@ void i915_gem_request_free(struct kref *req_ref)
> >
> >  	if (ctx) {
> >  		if (i915.enable_execlists) {
> >-			if (ctx != req->ring->default_context)
> >+			if (ctx != req->i915->kernel_context)
> >  				intel_lr_context_unpin(req);
> >  		}
> >
> >@@ -2774,7 +2774,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> >  	int err;
> >
> >  	if (ctx == NULL)
> >-		ctx = engine->default_context;
> >+		ctx = to_i915(engine->dev)->kernel_context;
> >  	err = __i915_gem_request_alloc(engine, ctx, &req);
> >  	return err ? ERR_PTR(err) : req;
> >  }
> >@@ -4862,7 +4862,7 @@ i915_gem_init_hw(struct drm_device *dev)
> >  	 */
> >  	init_unused_rings(dev);
> >
> >-	BUG_ON(!dev_priv->ring[RCS].default_context);
> >+	BUG_ON(!dev_priv->kernel_context);
> >
> >  	ret = i915_ppgtt_init_hw(dev);
> >  	if (ret) {
> >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >index 900ffd0..e1d767e 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >@@ -354,11 +354,10 @@ int i915_gem_context_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_context *ctx;
> >-	int i;
> >
> >  	/* Init should only be called once per module load. Eventually the
> >  	 * restriction on the context_disabled check can be loosened. */
> >-	if (WARN_ON(dev_priv->ring[RCS].default_context))
> >+	if (WARN_ON(dev_priv->kernel_context))
> >  		return 0;
> >
> >  	if (intel_vgpu_active(dev) && HAS_LOGICAL_RING_CONTEXTS(dev)) {
> >@@ -388,12 +387,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		return PTR_ERR(ctx);
> >  	}
> >
> >-	for (i = 0; i < I915_NUM_RINGS; i++) {
> >-		struct intel_engine_cs *ring = &dev_priv->ring[i];
> >-
> >-		/* NB: RCS will hold a ref for all rings */
> >-		ring->default_context = ctx;
> >-	}
> >+	dev_priv->kernel_context = ctx;
> >
> >  	DRM_DEBUG_DRIVER("%s context support initialized\n",
> >  			i915.enable_execlists ? "LR" :
> >@@ -404,7 +398,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  void i915_gem_context_fini(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >-	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
> >+	struct intel_context *dctx = dev_priv->kernel_context;
> >  	int i;
> >
> >  	if (dctx->legacy_hw_ctx.rcs_state) {
> >@@ -431,17 +425,17 @@ void i915_gem_context_fini(struct drm_device *dev)
> >  		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> >  	}
> >
> >-	for (i = 0; i < I915_NUM_RINGS; i++) {
> >+	for (i = I915_NUM_RINGS; --i >= 0;) {
> >  		struct intel_engine_cs *ring = &dev_priv->ring[i];
> >
> >-		if (ring->last_context)
> >+		if (ring->last_context) {
> >  			i915_gem_context_unreference(ring->last_context);
> >-
> >-		ring->default_context = NULL;
> >-		ring->last_context = NULL;
> >+			ring->last_context = NULL;
> >+		}
> >  	}
> >
> >  	i915_gem_context_unreference(dctx);
> >+	dev_priv->kernel_context = NULL;
> >  }
> >
> >  int i915_gem_context_enable(struct drm_i915_gem_request *req)
> >diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >index 06ca408..7eeb244 100644
> >--- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >@@ -1050,7 +1050,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
> >  			if (request)
> >  				rbuf = request->ctx->engine[ring->id].ringbuf;
> >  			else
> >-				rbuf = ring->default_context->engine[ring->id].ringbuf;
> >+				rbuf = dev_priv->kernel_context->engine[ring->id].ringbuf;
> >  		} else
> >  			rbuf = ring->buffer;
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index 9c24424..51ae5c1 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -964,7 +964,7 @@ int i915_guc_submission_enable(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_guc *guc = &dev_priv->guc;
> >-	struct intel_context *ctx = dev_priv->ring[RCS].default_context;
> >+	struct intel_context *ctx = dev_priv->kernel_context;
> >  	struct i915_guc_client *client;
> >
> >  	/* client for execbuf submission */
> >@@ -1021,7 +1021,7 @@ int intel_guc_suspend(struct drm_device *dev)
> >  	if (!i915.enable_guc_submission)
> >  		return 0;
> >
> >-	ctx = dev_priv->ring[RCS].default_context;
> >+	ctx = dev_priv->kernel_context;
> >
> >  	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
> >  	/* any value greater than GUC_POWER_D0 */
> >@@ -1047,7 +1047,7 @@ int intel_guc_resume(struct drm_device *dev)
> >  	if (!i915.enable_guc_submission)
> >  		return 0;
> >
> >-	ctx = dev_priv->ring[RCS].default_context;
> >+	ctx = dev_priv->kernel_context;
> >
> >  	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
> >  	data[1] = GUC_POWER_D0;
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 8b6542d..aaaa5a3 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -571,7 +571,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
> >  	struct drm_i915_gem_request *cursor;
> >  	int num_elements = 0;
> >
> >-	if (request->ctx != ring->default_context)
> >+	if (request->ctx != request->i915->kernel_context)
> >  		intel_lr_context_pin(request);
> >
> >  	i915_gem_request_reference(request);
> >@@ -664,7 +664,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> >
> >  	request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
> >
> >-	if (request->ctx != request->ring->default_context) {
> >+	if (request->ctx != request->i915->kernel_context) {
> >  		ret = intel_lr_context_pin(request);
> >  		if (ret)
> >  			return ret;
> >@@ -980,7 +980,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
> >  		struct drm_i915_gem_object *ctx_obj =
> >  				ctx->engine[ring->id].state;
> >
> >-		if (ctx_obj && (ctx != ring->default_context))
> >+		if (ctx_obj && (ctx != req->i915->kernel_context))
> >  			intel_lr_context_unpin(req);
> >  		list_del(&req->execlist_link);
> >  		i915_gem_request_unreference(req);
> >@@ -1487,7 +1487,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
> >  	u8 next_context_status_buffer_hw;
> >
> >  	lrc_setup_hardware_status_page(ring,
> >-				ring->default_context->engine[ring->id].state);
> >+				dev_priv->kernel_context->engine[ring->id].state);
> >
> >  	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
> >  	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
> >@@ -1929,6 +1929,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
> >
> >  static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
> >  {
> >+	struct drm_i915_private *dev_priv = to_i915(dev);
> >+	struct intel_context *dctx = dev_priv->kernel_context;
> >+	int ring_id = ring->id;
> >  	int ret;
> >
> >  	/* Intentionally left blank. */
> >@@ -1949,15 +1952,14 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
> >  	if (ret)
> >  		goto error;
> >
> >-	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
> >+	ret = intel_lr_context_deferred_alloc(dctx, ring);
> >  	if (ret)
> >  		goto error;
> >
> >  	/* As this is the default context, always pin it */
> >-	ret = intel_lr_context_do_pin(
> >-			ring,
> >-			ring->default_context->engine[ring->id].state,
> >-			ring->default_context->engine[ring->id].ringbuf);
> >+	ret = intel_lr_context_do_pin(ring,
> >+				      dctx->engine[ring_id].state,
> >+				      dctx->engine[ring_id].ringbuf);
> >  	if (ret) {
> >  		DRM_ERROR(
> >  			"Failed to pin and map ringbuffer %s: %d\n",
> >@@ -2388,7 +2390,7 @@ void intel_lr_context_free(struct intel_context *ctx)
> >  					ctx->engine[i].ringbuf;
> >  			struct intel_engine_cs *ring = ringbuf->ring;
> >
> >-			if (ctx == ring->default_context) {
> >+			if (ctx == ctx->i915->kernel_context) {
> >  				intel_unpin_ringbuffer_obj(ringbuf);
> >  				i915_gem_object_ggtt_unpin(ctx_obj);
> >  			}
> >@@ -2507,7 +2509,7 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> >  	ctx->engine[ring->id].ringbuf = ringbuf;
> >  	ctx->engine[ring->id].state = ctx_obj;
> >
> >-	if (ctx != ring->default_context && ring->init_context) {
> >+	if (ctx != ctx->i915->kernel_context && ring->init_context) {
> >  		struct drm_i915_gem_request *req;
> >
> >  		req = i915_gem_request_alloc(ring, ctx);
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >index 49574ff..e950f6c 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >@@ -305,7 +305,6 @@ struct  intel_engine_cs {
> >
> >  	wait_queue_head_t irq_queue;
> >
> >-	struct intel_context *default_context;
> >  	struct intel_context *last_context;
> >
> >  	struct intel_ring_hangcheck hangcheck;
> >
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx