improve handling of the driver's internal default context

improve handling of the driver's internal default context

[Dave Gordon]

A collection of patches to simplify the creation, use, and destruction
of the driver's global default context.

    The first two simplify the many places where the code treats the
    global default context differently from any other context:

	[1/6] drm/i915: mark the global default (intel_)context as such
	[2/6] drm/i915: simplify testing for the global default context

    Then we hide the use of the global-default-context from callers who
    don't need to know how non-batch-related requests are implemented
    internally:

	[3/6] drm/i915: simplify allocation of driver-internal requests

    And get rid of the multiple references to the default context, one
    from each engine (but all pointing to the same structure). This
    allows the refcount and the number of references to match!

	[4/6] drm/i915: abolish separate per-engine default_context

    Finally, fix some incorrect failure paths and tidy up the
    corresponding teardown code:

	[5/6] drm/i915: tidy up initialisation failure paths (legacy)
	[6/6] drm/i915: tidy up initialisation failure paths (GEM &

These should make subsequent reorganisation of other startup/teardown
code easier and safer.

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

[PATCH 1/6] drm/i915: mark the global default (intel_)context as such

[Dave Gordon]

Some of the LRC-specific context-destruction code has to special-case
the global default context, bacause the HWSP is part of that context. At
present it deduces it indirectly by checking for the backpointer from
the engine to the context, but that's an unsafe assumption if the setup
and teardown code is reorganised. (It could also test ctx->file_priv ==
NULL, but again that's a detail that might be subject to change).

So here we explicitly flag the default context at the point of creation,
and then reorganise the code in intel_lr_context_free() not to rely on
the ring->default_pointer (still) being set up; to iterate over engines
in reverse, as this is teardown code; and to reduce the nesting
level so it's easier to read.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 ++
 drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++-----
 drivers/gpu/drm/i915/intel_lrc.c        | 25 ++++++++++++-------------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b82c45..666d07c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -854,6 +854,7 @@ struct i915_ctx_hang_stats {
  * @ref: reference count.
  * @user_handle: userspace tracking identity for this context.
  * @remap_slice: l3 row remapping information.
+ * @is_default: true iff this is the global default context
  * @flags: context specific flags:
  *         CONTEXT_NO_ZEROMAP: do not allow mapping things to page 0.
  * @file_priv: filp associated with this context (NULL for global default
@@ -872,6 +873,7 @@ struct intel_context {
 	struct kref ref;
 	int user_handle;
 	uint8_t remap_slice;
+	uint8_t is_global_default;
 	struct drm_i915_private *i915;
 	int flags;
 	struct drm_i915_file_private *file_priv;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..5d3e287 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -241,8 +241,10 @@ __create_hw_context(struct drm_device *dev,
 				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
 		if (ret < 0)
 			goto err_out;
-	} else
+	} else {
+		ctx->is_global_default = true;
 		ret = DEFAULT_CONTEXT_HANDLE;
+	}
 
 	ctx->file_priv = file_priv;
 	ctx->user_handle = ret;
@@ -269,7 +271,6 @@ static struct intel_context *
 i915_gem_create_context(struct drm_device *dev,
 			struct drm_i915_file_private *file_priv)
 {
-	const bool is_global_default_ctx = file_priv == NULL;
 	struct intel_context *ctx;
 	int ret = 0;
 
@@ -279,8 +280,9 @@ i915_gem_create_context(struct drm_device *dev,
 	if (IS_ERR(ctx))
 		return ctx;
 
-	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) {
-		/* We may need to do things with the shrinker which
+	if (ctx->is_global_default && ctx->legacy_hw_ctx.rcs_state) {
+		/*
+		 * We may need to do things with the shrinker which
 		 * require us to immediately switch back to the default
 		 * context. This can cause a problem as pinning the
 		 * default context also requires GTT space which may not
@@ -313,7 +315,7 @@ i915_gem_create_context(struct drm_device *dev,
 	return ctx;
 
 err_unpin:
-	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state)
+	if (ctx->is_global_default && ctx->legacy_hw_ctx.rcs_state)
 		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
 err_destroy:
 	idr_remove(&file_priv->context_idr, ctx->user_handle);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3aa6147..23f90b2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2367,22 +2367,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 == ring->default_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->is_global_default) {
+			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);
 	}
 }
 
@@ -2443,7 +2442,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 1/6] drm/i915: mark the global default (intel_)context as such

[Chris Wilson]

On Mon, Dec 21, 2015 at 04:04:40PM +0000, Dave Gordon wrote:
> Some of the LRC-specific context-destruction code has to special-case
> the global default context, bacause the HWSP is part of that context. At
> present it deduces it indirectly by checking for the backpointer from
> the engine to the context, but that's an unsafe assumption if the setup
> and teardown code is reorganised. (It could also test ctx->file_priv ==
> NULL, but again that's a detail that might be subject to change).
> 
> So here we explicitly flag the default context at the point of creation,
> and then reorganise the code in intel_lr_context_free() not to rely on
> the ring->default_pointer (still) being set up; to iterate over engines
> in reverse, as this is teardown code; and to reduce the nesting
> level so it's easier to read.

I still strongly disagree with the confusion inherent here.
-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 1/6] drm/i915: mark the global default (intel_)context as such

[Dave Gordon]

On 22/12/15 09:08, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:40PM +0000, Dave Gordon wrote:
>> Some of the LRC-specific context-destruction code has to special-case
>> the global default context, bacause the HWSP is part of that context. At
>> present it deduces it indirectly by checking for the backpointer from
>> the engine to the context, but that's an unsafe assumption if the setup
>> and teardown code is reorganised. (It could also test ctx->file_priv ==
>> NULL, but again that's a detail that might be subject to change).
>>
>> So here we explicitly flag the default context at the point of creation,
>> and then reorganise the code in intel_lr_context_free() not to rely on
>> the ring->default_pointer (still) being set up; to iterate over engines
>> in reverse, as this is teardown code; and to reduce the nesting
>> level so it's easier to read.
>
> I still strongly disagree with the confusion inherent here.
> -Chris

I'm not sure that anything could be *less* confusing than indicating the 
global default context with a flag called "is_global_default".

There may indeed be all sorts of confusion in the LRC code, but I'm not 
trying to fix all of that in one go. This is just an enabler so that one 
small piece of that confusion can be replaced by something clear and 
explicit. Once that's done, we'll move on and see what else can also be 
clarified.

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

[PATCH 2/6] drm/i915: simplify testing for the global default context

[Dave Gordon]

There are quite a number of places where the driver tests whether a
given context is or is not the global default context, usually by
checking whether an engine's default_pointer points to the context. Now
that we have a 'is_global_default' flag in the context itself, these can
be rewritten to use it. This makes the logic more obvious, and usually
saves at least one memory reference.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 +++-----
 drivers/gpu/drm/i915/i915_gem.c     |  8 ++------
 drivers/gpu/drm/i915/intel_lrc.c    | 26 ++++++++++++--------------
 3 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0fc38bb..13261fc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2037,13 +2037,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 (ring->default_context != ctx)
+	list_for_each_entry(ctx, &dev_priv->context_list, link)
+		if (!ctx->is_global_default)
+			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 6c60e04..be1f984 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->ring->default_context)
-				intel_lr_context_unpin(req);
-		}
+		if (i915.enable_execlists && !ctx->is_global_default)
+			intel_lr_context_unpin(req);
 
 		i915_gem_context_unreference(ctx);
 	}
@@ -4869,8 +4867,6 @@ 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) {
 			i915_gem_cleanup_ringbuffer(dev);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 23f90b2..c44bd86 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->is_global_default)
 		intel_lr_context_pin(request);
 
 	i915_gem_request_reference(request);
@@ -660,17 +660,14 @@ 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->ring->default_context) {
+	if (!request->ctx->is_global_default)
 		ret = intel_lr_context_pin(request);
-		if (ret)
-			return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
@@ -967,7 +964,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->is_global_default)
 			intel_lr_context_unpin(req);
 		list_del(&req->execlist_link);
 		i915_gem_request_unreference(req);
@@ -1916,6 +1913,8 @@ 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 intel_context *dctx = ring->default_context;
+	int ring_id = ring->id;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -1936,15 +1935,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",
@@ -2479,7 +2477,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->is_global_default && ring->init_context) {
 		struct drm_i915_gem_request *req;
 
 		ret = i915_gem_request_alloc(ring,
-- 
1.9.1

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

Re: [PATCH 2/6] drm/i915: simplify testing for the global default context

[Chris Wilson]

On Mon, Dec 21, 2015 at 04:04:41PM +0000, Dave Gordon wrote:
> There are quite a number of places where the driver tests whether a
> given context is or is not the global default context, usually by
> checking whether an engine's default_pointer points to the context. Now
> that we have a 'is_global_default' flag in the context itself, these can
> be rewritten to use it. This makes the logic more obvious, and usually
> saves at least one memory reference.

All these places do not need to exist. Please just fix execlists.
-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 2/6] drm/i915: simplify testing for the global default context

[Dave Gordon]

On 22/12/15 09:05, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:41PM +0000, Dave Gordon wrote:
>> There are quite a number of places where the driver tests whether a
>> given context is or is not the global default context, usually by
>> checking whether an engine's default_pointer points to the context. Now
>> that we have a 'is_global_default' flag in the context itself, these can
>> be rewritten to use it. This makes the logic more obvious, and usually
>> saves at least one memory reference.
>
> All these places do not need to exist. Please just fix execlists.
> -Chris

The patchset "to fix execlists" in one go would be too large to be 
accepted here and would take too long to develop, given the nature of 
the moving target. Ergo, we can fix execlists only by taking every 
opportunity to move towards a clearer design, even though each step 
fails to "fix execlists" on its own.

We therefore have to judge each patch on the basis of "does it make 
things better or worse", not "does it fix all known problems". IMHO, 
this patch (and the rest of the set) are small steps towards a better 
design, and you should therefore support their adoption, unless of 
course you think it actually makes things worse - in which case, point 
out what's worse and I'll change it.

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

[PATCH 3/6] 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. For
such requests, we associate them with the default context for the engine
that the request will be submitted to.

This patch provides a shorthand for doing such request allocations and
changes all such calls to use the new function.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_gem.c      | 36 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c |  6 ++++--
 drivers/gpu/drm/i915/intel_overlay.c | 24 ++++++++++++------------
 4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 666d07c..4955db9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2270,6 +2270,8 @@ 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_anon(struct intel_engine_cs *ring);
 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 be1f984..9f9c0c0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2751,6 +2751,21 @@ err:
 	return ret;
 }
 
+/*
+ * Allocate a request associated with the default context for the given
+ * ring. This can be used where the driver needs a request for internal
+ * purposes not directly related to a user batch submission.
+ */
+struct drm_i915_gem_request *
+i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
+{
+	struct drm_i915_gem_request *req;
+	int err;
+
+	err = i915_gem_request_alloc(ring, ring->default_context, &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);
@@ -3168,9 +3183,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_anon(to);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
+
+			*to_req = req;
 		}
 
 		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
@@ -3370,9 +3389,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_anon(ring);
+			if (IS_ERR(req))
+				return PTR_ERR(req);
 
 			ret = i915_switch_context(req);
 			if (ret) {
@@ -4867,8 +4886,9 @@ i915_gem_init_hw(struct drm_device *dev)
 	for_each_ring(ring, dev_priv, i) {
 		struct drm_i915_gem_request *req;
 
-		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-		if (ret) {
+		req = i915_gem_request_alloc_anon(ring);
+		if (IS_ERR(req)) {
+			ret = PTR_ERR(req);
 			i915_gem_cleanup_ringbuffer(dev);
 			goto out;
 		}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index abd2d29..5716f4a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11662,9 +11662,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_anon(ring);
+			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_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 76f1980..57cd503 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_anon(ring);
+	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_anon(ring);
+	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_anon(ring);
+	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_anon(ring);
+		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

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

[Chris Wilson]

On Mon, Dec 21, 2015 at 04:04:42PM +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. For
> such requests, we associate them with the default context for the engine
> that the request will be submitted to.
> 
> This patch provides a shorthand for doing such request allocations and
> changes all such calls to use the new function.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c      | 36 ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_display.c |  6 ++++--
>  drivers/gpu/drm/i915/intel_overlay.c | 24 ++++++++++++------------
>  4 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 666d07c..4955db9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2270,6 +2270,8 @@ 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_anon(struct intel_engine_cs *ring);
>  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 be1f984..9f9c0c0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2751,6 +2751,21 @@ err:
>  	return ret;
>  }
>  
> +/*
> + * Allocate a request associated with the default context for the given
> + * ring. This can be used where the driver needs a request for internal
> + * purposes not directly related to a user batch submission.
> + */
> +struct drm_i915_gem_request *
> +i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
> +{

As demonstrated, no. Contexts need to be considered properly first.

> +	struct drm_i915_gem_request *req;
> +	int err;
> +
> +	err = i915_gem_request_alloc(ring, ring->default_context, &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);
> @@ -3168,9 +3183,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_anon(to);

Wrong context. Please see patches to fix this mess.

> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
> +
> +			*to_req = req;
>  		}
>  
>  		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
> @@ -3370,9 +3389,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_anon(ring);
> +			if (IS_ERR(req))
> +				return PTR_ERR(req);
>  
>  			ret = i915_switch_context(req);
>  			if (ret) {
> @@ -4867,8 +4886,9 @@ i915_gem_init_hw(struct drm_device *dev)
>  	for_each_ring(ring, dev_priv, i) {
>  		struct drm_i915_gem_request *req;
>  
> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> -		if (ret) {
> +		req = i915_gem_request_alloc_anon(ring);
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
>  			i915_gem_cleanup_ringbuffer(dev);
>  			goto out;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index abd2d29..5716f4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11662,9 +11662,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)

Wrong context.
-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 3/6] drm/i915: simplify allocation of driver-internal requests

[Dave Gordon]

On 22/12/15 09:08, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:42PM +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. For
>> such requests, we associate them with the default context for the engine
>> that the request will be submitted to.
>>
>> This patch provides a shorthand for doing such request allocations and
>> changes all such calls to use the new function.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>   drivers/gpu/drm/i915/i915_gem.c      | 36 ++++++++++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_display.c |  6 ++++--
>>   drivers/gpu/drm/i915/intel_overlay.c | 24 ++++++++++++------------
>>   4 files changed, 46 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 666d07c..4955db9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2270,6 +2270,8 @@ 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_anon(struct intel_engine_cs *ring);
>>   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 be1f984..9f9c0c0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2751,6 +2751,21 @@ err:
>>   	return ret;
>>   }
>>
>> +/*
>> + * Allocate a request associated with the default context for the given
>> + * ring. This can be used where the driver needs a request for internal
>> + * purposes not directly related to a user batch submission.
>> + */
>> +struct drm_i915_gem_request *
>> +i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
>> +{
>
> As demonstrated, no. Contexts need to be considered properly first.
>
>> +	struct drm_i915_gem_request *req;
>> +	int err;
>> +
>> +	err = i915_gem_request_alloc(ring, ring->default_context, &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);
>> @@ -3168,9 +3183,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_anon(to);
>
> Wrong context. Please see patches to fix this mess.
>
>> +			if (IS_ERR(req))
>> +				return PTR_ERR(req);
>> +
>> +			*to_req = req;
>>   		}
>>
>>   		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
>> @@ -3370,9 +3389,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_anon(ring);
>> +			if (IS_ERR(req))
>> +				return PTR_ERR(req);
>>
>>   			ret = i915_switch_context(req);
>>   			if (ret) {
>> @@ -4867,8 +4886,9 @@ i915_gem_init_hw(struct drm_device *dev)
>>   	for_each_ring(ring, dev_priv, i) {
>>   		struct drm_i915_gem_request *req;
>>
>> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
>> -		if (ret) {
>> +		req = i915_gem_request_alloc_anon(ring);
>> +		if (IS_ERR(req)) {
>> +			ret = PTR_ERR(req);
>>   			i915_gem_cleanup_ringbuffer(dev);
>>   			goto out;
>>   		}
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index abd2d29..5716f4a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11662,9 +11662,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)
>
> Wrong context.
> -Chris

What do you mean, "wrong context". The line above is the way it 
*currently* works, which I'm replacing with one that *doesn't* refer to 
the default context.

This patch doesn't change any functionality, just simplifies it by 
reducing the number of instances of "default_context" so that patch 4/6 
can actually get rid of ring->default_context entirely. I thought that 
was what you were aiming for?

.Dave.

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

[PATCH 4/6] drm/i915: abolish separate per-engine default_context pointers

[Dave Gordon]

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 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>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
 drivers/gpu/drm/i915/i915_drv.h            |  2 ++
 drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
 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           |  5 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
 8 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 13261fc..722f302 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);
 		}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4955db9..3c371ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1947,6 +1947,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 9f9c0c0..01b1eea 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2759,10 +2759,11 @@ err:
 struct drm_i915_gem_request *
 i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
 {
+	struct drm_i915_private *dev_priv = to_i915(ring->dev);
 	struct drm_i915_gem_request *req;
 	int err;
 
-	err = i915_gem_request_alloc(ring, ring->default_context, &req);
+	err = i915_gem_request_alloc(ring, dev_priv->kernel_context, &req);
 	return err ? ERR_PTR(err) : req;
 }
 
@@ -4849,7 +4850,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 5d3e287..b655a50 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -356,11 +356,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)) {
@@ -390,12 +389,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" :
@@ -406,7 +400,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) {
@@ -433,17 +427,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 9cc3b84..70d0da6 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -863,7 +863,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 */
@@ -917,7 +917,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 */
@@ -943,7 +943,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 c44bd86..743ac81 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1471,7 +1471,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);
@@ -1913,7 +1913,8 @@ 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 intel_context *dctx = ring->default_context;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
 	int ring_id = ring->id;
 	int ret;
 
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 5/6] drm/i915: tidy up initialisation failure paths (legacy)

[Dave Gordon]

1. Fix intel_cleanup_ring_buffer() to handle the error cleanup
   case where the ringbuffer has been allocated but map-and-pin
   failed. Unpin it iff it's previously been mapped-and-pinned.

2. Fix the error path in intel_init_ring_buffer(), which already
   called intel_destroy_ringbuffer_obj(), but failed to free the
   actual ringbuffer structure. Calling intel_ringbuffer_free()
   instead does both in one go.

3. With the above change, intel_destroy_ringbuffer_obj() is only
   called in one place (intel_ringbuffer_free()), so flatten it
   into that function.

4. move low-level register accesses from intel_cleanup_ring_buffer()
   (which calls intel_stop_ring_buffer(ring) which calls stop_ring())
   down into stop_ring() itself), which is already doing low-level
   register accesses. Then, intel_cleanup_ring_buffer() no longer
   needs 'dev_priv'.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 339701d..cafbcd5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -549,6 +549,8 @@ static bool stop_ring(struct intel_engine_cs *ring)
 		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
 	}
 
+	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
+
 	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
 }
 
@@ -2057,12 +2059,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 	return 0;
 }
 
-static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
-{
-	drm_gem_object_unreference(&ringbuf->obj->base);
-	ringbuf->obj = NULL;
-}
-
 static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 				      struct intel_ringbuffer *ringbuf)
 {
@@ -2125,11 +2121,14 @@ intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int size)
 }
 
 void
-intel_ringbuffer_free(struct intel_ringbuffer *ring)
+intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)
 {
-	intel_destroy_ringbuffer_obj(ring);
-	list_del(&ring->link);
-	kfree(ring);
+	if (ringbuf->obj) {
+		drm_gem_object_unreference(&ringbuf->obj->base);
+		ringbuf->obj = NULL;
+	}
+	list_del(&ringbuf->link);
+	kfree(ringbuf);
 }
 
 static int intel_init_ring_buffer(struct drm_device *dev,
@@ -2157,6 +2156,13 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	}
 	ring->buffer = ringbuf;
 
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
+	if (ret) {
+		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
+				ring->name, ret);
+		goto error;
+	}
+
 	if (I915_NEED_GFX_HWS(dev)) {
 		ret = init_status_page(ring);
 		if (ret)
@@ -2168,14 +2174,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 			goto error;
 	}
 
-	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
-	if (ret) {
-		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
-				ring->name, ret);
-		intel_destroy_ringbuffer_obj(ringbuf);
-		goto error;
-	}
-
 	ret = i915_cmd_parser_init_ring(ring);
 	if (ret)
 		goto error;
@@ -2189,19 +2187,18 @@ error:
 
 void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv;
+	struct intel_ringbuffer *ringbuf;
 
 	if (!intel_ring_initialized(ring))
 		return;
 
-	dev_priv = to_i915(ring->dev);
-
-	if (ring->buffer) {
+	ringbuf = ring->buffer;
+	if (ringbuf) {
 		intel_stop_ring_buffer(ring);
-		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-		intel_unpin_ringbuffer_obj(ring->buffer);
-		intel_ringbuffer_free(ring->buffer);
+		if (ringbuf->virtual_start)
+			intel_unpin_ringbuffer_obj(ringbuf);
+		intel_ringbuffer_free(ringbuf);
 		ring->buffer = NULL;
 	}
 
-- 
1.9.1

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

[PATCH 6/6] drm/i915: tidy up initialisation failure paths (GEM & LRC)

[Dave Gordon]

1. add call to i915_gem_context_fini() to deallocate the default
   context(s) if the call to init_rings() fails, so that we don't
   leak the context in that situation.

2. remove useless code in intel_logical_ring_cleanup(), presumably
   copypasted from legacy ringbuffer version at creation.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  5 ++++-
 drivers/gpu/drm/i915/intel_lrc.c | 10 ++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 01b1eea..20777e4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4964,8 +4964,11 @@ int i915_gem_init(struct drm_device *dev)
 		goto out_unlock;
 
 	ret = dev_priv->gt.init_rings(dev);
-	if (ret)
+	if (ret) {
+		i915_gem_context_fini(dev);
+		/* XXX: anything else to be undone here? */
 		goto out_unlock;
+	}
 
 	ret = i915_gem_init_hw(dev);
 	if (ret == -EIO) {
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 743ac81..b731e16 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1884,17 +1884,11 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
  */
 void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv;
-
 	if (!intel_ring_initialized(ring))
 		return;
 
-	dev_priv = ring->dev->dev_private;
-
-	if (ring->buffer) {
-		intel_logical_ring_stop(ring);
-		WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
-	}
+	/* should not be set in LRC mode */
+	WARN_ON(ring->buffer);
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
-- 
1.9.1

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

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 78deeec98b10627fe2050ce8ebfa2ea2d5b9e6c7 drm-intel-nightly: 2015y-12m-21d-16h-03m-57s UTC integration manifest

Test gem_mmap_gtt:
        Subgroup basic-small-bo:
                dmesg-warn -> PASS       (bdw-nuci7)
Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (bdw-nuci7)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (hsw-xps12)
                dmesg-warn -> PASS       (hsw-brixbox)
                skip       -> DMESG-WARN (bdw-nuci7)
        Subgroup basic-plain-flip:
                skip       -> PASS       (bdw-nuci7)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
                skip       -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (snb-x220t)
                skip       -> PASS       (bdw-nuci7)
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (hsw-xps12)
                pass       -> DMESG-WARN (snb-dellxps)
                skip       -> PASS       (bdw-nuci7)
        Subgroup read-crc-pipe-b-frame-sequence:
                skip       -> PASS       (bdw-nuci7)
        Subgroup read-crc-pipe-c:
                skip       -> PASS       (bdw-nuci7)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                skip       -> PASS       (bdw-nuci7)
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup basic-rte:
                skip       -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (bdw-ultra)

bdw-nuci7        total:132  pass:120  dwarn:3   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:99   dwarn:1   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:121  dwarn:6   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:120  dwarn:2   dfail:0   fail:2   skip:11 

HANGED hsw-gt2 in igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b

Results at /archive/results/CI_IGT_test/Patchwork_783/

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