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

[PATCH v4 0/3] improve handling of the driver's default (kernel) 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.
(We now call it "the kernel context", and compare against the device-wide
pointer).  [Chris Wilson] 

v4: Rebased

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

[PATCH v4 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)) ...

v4:	Rebased

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 af30148..dfebf9f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2279,9 +2279,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 6b0102d..8e716b6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2690,9 +2690,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;
@@ -2755,6 +2756,31 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
 	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);
@@ -3172,9 +3198,13 @@ void i915_gem_reset(struct drm_device *dev)
 			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);
@@ -3374,9 +3404,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) {
@@ -4871,10 +4901,9 @@ int i915_gem_init_rings(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 4edf1c0..dc32018 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1381,6 +1381,7 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
 		       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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
 		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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
 	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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
 	 * 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 a851cb7..991fda8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11709,9 +11709,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 faaf490..ec2482da 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2520,11 +2520,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

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

[Tvrtko Ursulin]

Hi,

On 19/01/16 19:02, 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)) ...
>
> v4:	Rebased

Wasn't following the discussion in detail but there was always big 
resistance towards API which takes NULLs inferring some default state. 
At least in some of my past work that was an repeated objection. Maybe a 
way around it would be to have two functions, like:

i915_gem_request_alloc(ring); // default context
i915_gem_request_alloc_ctx(ring, ctx); // user context

?

Regards,

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

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

[Chris Wilson]

On Wed, Jan 20, 2016 at 09:56:10AM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 19/01/16 19:02, 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)) ...
> >
> >v4:	Rebased
> 
> Wasn't following the discussion in detail but there was always big
> resistance towards API which takes NULLs inferring some default
> state. At least in some of my past work that was an repeated
> objection. Maybe a way around it would be to have two functions,
> like:
> 
> i915_gem_request_alloc(ring); // default context
> i915_gem_request_alloc_ctx(ring, ctx); // user context

There is only one piece of code where we even want to use a default
kernel context, so it makes no sense from that point of view.
Imho, knowing the context is paramount to achieving context separation
in GEM - which we are far from achieving yet.
-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 v4 1/3] drm/i915: simplify allocation of driver-internal requests

[Tvrtko Ursulin]

On 19/01/16 19:02, 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)) ...
> 
> v4:	Rebased
> 
> 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 af30148..dfebf9f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2279,9 +2279,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 6b0102d..8e716b6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2690,9 +2690,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;
> @@ -2755,6 +2756,31 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>   	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);
> @@ -3172,9 +3198,13 @@ void i915_gem_reset(struct drm_device *dev)
>   			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);
> @@ -3374,9 +3404,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) {
> @@ -4871,10 +4901,9 @@ int i915_gem_init_rings(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 4edf1c0..dc32018 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1381,6 +1381,7 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>   		       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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>   		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;
> +	}

Jump down..

>   
> -	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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>   	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 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>   	 * 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);

.. to here, where req is an ERR_PTR and you call i915_gem_request_cancel
on it and end up with:

[   33.009942] BUG: unable to handle kernel paging request at ffffffffffffffca
[   33.018003] IP: [<ffffffffa01d9428>] i915_gem_request_cancel+0x8/0x60 [i915]
[   33.026191] PGD 2a0b067 PUD 2a0d067 PMD 0 
[   33.031017] Oops: 0000 [#1] PREEMPT SMP 
[   33.035666] Modules linked in: hid_generic usbhid i915 i2c_algo_bit drm_kms_helper asix usbnet libphy cfbfillrect syscopyarea mii cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm coretemp i2c_hid hid pinctrl_sunrisepoint pinctrl_intel video acpi_pad nls_iso8859_1 e1000e ptp psmouse pps_core ahci libahci
[   33.068007] CPU: 0 PID: 1891 Comm: gem_close_race Tainted: G     U          4.4.0-160121+ #123
[   33.078000] Hardware name: Intel Corporation Skylake Client platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551 09/22/2015
[   33.092745] task: ffff88016b750000 ti: ffff88016c980000 task.ti: ffff88016c980000
[   33.101497] RIP: 0010:[<ffffffffa01d9428>]  [<ffffffffa01d9428>] i915_gem_request_cancel+0x8/0x60 [i915]
[   33.112549] RSP: 0018:ffff88016c983c10  EFLAGS: 00010202
[   33.118858] RAX: 0000000000000001 RBX: ffffffffffffff92 RCX: 000000018040003e
[   33.127246] RDX: 000000018040003f RSI: ffffea0001f10080 RDI: ffffffffffffff92
[   33.135650] RBP: ffff88016c983c18 R08: ffff88007c402f00 R09: 000000007c402001
[   33.144051] R10: ffff880172c170c0 R11: ffffea0001f10080 R12: ffff88007c402f00
[   33.152474] R13: 00000000ffffff92 R14: ffffffffffffff92 R15: ffff88016b9143d0
[   33.160888] FS:  00007fe0c4b09700(0000) GS:ffff880172c00000(0000) knlGS:0000000000000000
[   33.170398] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   33.177254] CR2: ffffffffffffffca CR3: 0000000086f41000 CR4: 00000000003406f0
[   33.185704] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   33.194155] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   33.202603] Stack:
[   33.205244]  ffff88007c402f00 ffff88016c983d68 ffffffffa01ce10b ffffea0005980cc0
[   33.214067]  ffff88016c983cb8 ffffffffa01d7785 ffff88016c983c88 ffff88008705e218
[   33.222892]  ffff88016c983e10 ffff88007d808000 ffff88016c983df0 ffff88007ff75740
[   33.231717] Call Trace:
[   33.234889]  [<ffffffffa01ce10b>] i915_gem_do_execbuffer.isra.25+0x10cb/0x12a0 [i915]
[   33.244184]  [<ffffffffa01d7785>] ? i915_gem_object_get_pages_gtt+0x225/0x3c0 [i915]
[   33.253389]  [<ffffffffa01ddfb6>] ? i915_gem_pwrite_ioctl+0xd6/0x9f0 [i915]
[   33.261704]  [<ffffffffa01cee68>] i915_gem_execbuffer2+0xa8/0x250 [i915]
[   33.269738]  [<ffffffffa00f65d8>] drm_ioctl+0x258/0x4f0 [drm]
[   33.276691]  [<ffffffff810d3a97>] ? shmem_destroy_inode+0x17/0x20
[   33.284047]  [<ffffffffa01cedc0>] ? i915_gem_execbuffer+0x340/0x340 [i915]
[   33.292293]  [<ffffffff8111921c>] ? __dentry_kill+0x14c/0x1d0
[   33.299275]  [<ffffffff81121587>] ? mntput_no_expire+0x27/0x1a0
[   33.306459]  [<ffffffff8111590d>] do_vfs_ioctl+0x2cd/0x4a0
[   33.313157]  [<ffffffff8111eac2>] ? __fget+0x72/0xb0
[   33.319274]  [<ffffffff81115b1c>] SyS_ioctl+0x3c/0x70
[   33.325496]  [<ffffffff814effd7>] entry_SYSCALL_64_fastpath+0x12/0x6a
[   33.333289] Code: 00 48 c7 c7 d8 69 27 a0 31 c0 e8 d4 08 e7 e0 e9 b8 fe ff ff 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89 fb <48> 8b 7f 38 e8 5f 2a 01 00 48 8b 43 10 48 8b 40 10 8b 40 60 83 
[   33.356041] RIP  [<ffffffffa01d9428>] i915_gem_request_cancel+0x8/0x60 [i915]
[   33.364700]  RSP <ffff88016c983c10>
[   33.369208] CR2: ffffffffffffffca
[   33.373527] ---[ end trace d38fe9382064e353 ]---

Regards,

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

[PATCH] Fix pointer tests in error-handling paths

[Dave Gordon]

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

  2682708 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2dc08ce..a7bd555 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	 * 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 && req)
+	if (ret && !IS_ERR_OR_NULL(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 8104511..b88cdac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 cleanup_unpin:
 	intel_unpin_fb_obj(fb, crtc->primary->state);
 cleanup_pending:
-	if (request)
+	if (!IS_ERR_OR_NULL(request))
 		i915_gem_request_cancel(request);
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.9.1

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Chris Wilson]

On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
> 
>   2682708 drm/i915: simplify allocation of driver-internal requests
> 
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Quick quiz, name at least testcase that exercised this code?
-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] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 22/01/16 13:01, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
>> In the error-handling paths of i915_gem_do_execbuffer() and
>> intel_crtc_page_flip(), the local pointer-to-request variables
>> were expected to be either valid pointers or NULL. Since
>>
>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>
>> they could also be ERR_PTR() values, so the tests need to be
>> updated to accommodate this case.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Quick quiz, name at least testcase that exercised this code?

gem_close_race did it for me, but can you explain the weird ERR_PTR of 
*ffca ? :)

Regards,

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Chris Wilson]

On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote:
> 
> On 22/01/16 13:01, Chris Wilson wrote:
> >On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
> >>In the error-handling paths of i915_gem_do_execbuffer() and
> >>intel_crtc_page_flip(), the local pointer-to-request variables
> >>were expected to be either valid pointers or NULL. Since
> >>
> >>   2682708 drm/i915: simplify allocation of driver-internal requests
> >>
> >>they could also be ERR_PTR() values, so the tests need to be
> >>updated to accommodate this case.
> >>
> >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >Quick quiz, name at least testcase that exercised this code?
> 
> gem_close_race did it for me, but can you explain the weird ERR_PTR
> of *ffca ? :)

-54

That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc.
-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] Fix pointer tests in error-handling paths

[Dave Gordon]

On 22/01/16 13:34, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote:
>>
>> On 22/01/16 13:01, Chris Wilson wrote:
>>> On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
>>>> In the error-handling paths of i915_gem_do_execbuffer() and
>>>> intel_crtc_page_flip(), the local pointer-to-request variables
>>>> were expected to be either valid pointers or NULL. Since
>>>>
>>>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>>>
>>>> they could also be ERR_PTR() values, so the tests need to be
>>>> updated to accommodate this case.
>>>>
>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Quick quiz, name at least testcase that exercised this code?
>>
>> gem_close_race did it for me, but can you explain the weird ERR_PTR
>> of *ffca ? :)
>
> -54
>
> That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc.
> -Chris

Assuming that's the fault address, it will actually be the sum of the 
errno mistakenly passed to i915_gem_request_cancel(), plus the offset of 
'ringbuf' in the drm_i915_gem_request structure:

void i915_gem_request_cancel(struct drm_i915_gem_request *req)
{
         intel_ring_reserved_space_cancel(req->ringbuf);

         i915_gem_request_unreference(req);
}

As it happens, 'ringbuf' is at offset 0x38 (56), so the errno was -2,
which is the expected error -ENOENT :)

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 22/01/16 12:19, Dave Gordon wrote:
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
>
>    2682708 drm/i915: simplify allocation of driver-internal requests
>
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Not sure if CI will pick up a new patch in an old series.

Anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2dc08ce..a7bd555 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
>   	 * 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 && req)
> +	if (ret && !IS_ERR_OR_NULL(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 8104511..b88cdac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   cleanup_unpin:
>   	intel_unpin_fb_obj(fb, crtc->primary->state);
>   cleanup_pending:
> -	if (request)
> +	if (!IS_ERR_OR_NULL(request))
>   		i915_gem_request_cancel(request);
>   	atomic_dec(&intel_crtc->unpin_work_count);
>   	mutex_unlock(&dev->struct_mutex);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Fix pointer tests in error-handling paths

[Daniel Vetter]

On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote:
> 
> On 22/01/16 12:19, Dave Gordon wrote:
> >In the error-handling paths of i915_gem_do_execbuffer() and
> >intel_crtc_page_flip(), the local pointer-to-request variables
> >were expected to be either valid pointers or NULL. Since
> >
> >   2682708 drm/i915: simplify allocation of driver-internal requests
> >
> >they could also be ERR_PTR() values, so the tests need to be
> >updated to accommodate this case.
> >
> >Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> >  drivers/gpu/drm/i915/intel_display.c       | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Not sure if CI will pick up a new patch in an old series.

I think it'll treat this one as a replacement for patch 1/4 and then ofc
totally fall over. So would need a resend of the entire pile.
-Daniel

> 
> Anyway:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
> 
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >index 2dc08ce..a7bd555 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
> >  	 * 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 && req)
> >+	if (ret && !IS_ERR_OR_NULL(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 8104511..b88cdac 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  cleanup_unpin:
> >  	intel_unpin_fb_obj(fb, crtc->primary->state);
> >  cleanup_pending:
> >-	if (request)
> >+	if (!IS_ERR_OR_NULL(request))
> >  		i915_gem_request_cancel(request);
> >  	atomic_dec(&intel_crtc->unpin_work_count);
> >  	mutex_unlock(&dev->struct_mutex);
> >
> _______________________________________________
> 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] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 25/01/16 16:28, Daniel Vetter wrote:
> On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote:
>>
>> On 22/01/16 12:19, Dave Gordon wrote:
>>> In the error-handling paths of i915_gem_do_execbuffer() and
>>> intel_crtc_page_flip(), the local pointer-to-request variables
>>> were expected to be either valid pointers or NULL. Since
>>>
>>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>>
>>> they could also be ERR_PTR() values, so the tests need to be
>>> updated to accommodate this case.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>>>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Not sure if CI will pick up a new patch in an old series.
>
> I think it'll treat this one as a replacement for patch 1/4 and then ofc
> totally fall over. So would need a resend of the entire pile.

The rest of the pile has been merged already so I think just this patch 
on its own then (not as in-reply-to anything).

Regards,

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

[PATCH v2] Fix pointer tests in error-handling paths

[Dave Gordon]

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

   2682708 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.

v2:	Added testcase (Chris Wilson)

Testcase: igt/gem_close_race
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
  drivers/gpu/drm/i915/intel_display.c       | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2dc08ce..a7bd555 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int 
flags)
  	 * 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 && req)
+	if (ret && !IS_ERR_OR_NULL(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 8104511..b88cdac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc 
*crtc,
  cleanup_unpin:
  	intel_unpin_fb_obj(fb, crtc->primary->state);
  cleanup_pending:
-	if (request)
+	if (!IS_ERR_OR_NULL(request))
  		i915_gem_request_cancel(request);
  	atomic_dec(&intel_crtc->unpin_work_count);
  	mutex_unlock(&dev->struct_mutex);
-- 
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] Fix pointer tests in error-handling paths

[Maarten Lankhorst]

Hey,

Op 25-01-16 om 18:57 schreef Dave Gordon:
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
>
>   2682708 drm/i915: simplify allocation of driver-internal requests
>
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
>
> v2:    Added testcase (Chris Wilson)
>
> Testcase: igt/gem_close_race
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
The testcase you mentioned doesn't work, it crashes on my machine instead of running to completion or triggering the bug.

gem_ringfill.default-interruptible seems to trigger it.

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

Re: [PATCH v2] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 27/01/16 12:33, Maarten Lankhorst wrote:
> Hey,
>
> Op 25-01-16 om 18:57 schreef Dave Gordon:
>> In the error-handling paths of i915_gem_do_execbuffer() and
>> intel_crtc_page_flip(), the local pointer-to-request variables
>> were expected to be either valid pointers or NULL. Since
>>
>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>
>> they could also be ERR_PTR() values, so the tests need to be
>> updated to accommodate this case.
>>
>> v2:    Added testcase (Chris Wilson)
>>
>> Testcase: igt/gem_close_race
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> The testcase you mentioned doesn't work, it crashes on my machine instead of running to completion or triggering the bug.

It worked for me but I think I was running with GuC enabled at the time 
so that might be the differenct.

gem_close_race in general worked for me just yesterday.

Regards,

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

Re: [PATCH v2] Fix pointer tests in error-handling paths

[Maarten Lankhorst]

Hey,

Op 27-01-16 om 13:41 schreef Tvrtko Ursulin:
>
> On 27/01/16 12:33, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 25-01-16 om 18:57 schreef Dave Gordon:
>>> In the error-handling paths of i915_gem_do_execbuffer() and
>>> intel_crtc_page_flip(), the local pointer-to-request variables
>>> were expected to be either valid pointers or NULL. Since
>>>
>>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>>
>>> they could also be ERR_PTR() values, so the tests need to be
>>> updated to accommodate this case.
>>>
>>> v2:    Added testcase (Chris Wilson)
>>>
>>> Testcase: igt/gem_close_race
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> The testcase you mentioned doesn't work, it crashes on my machine instead of running to completion or triggering the bug.
>
> It worked for me but I think I was running with GuC enabled at the time so that might be the differenct.
>
> gem_close_race in general worked for me just yesterday.
It crashes in __lll_unlock_elision called from pthread_mutex_unlock, but I don't see anything weird there that could cause it.
Broadwell with the exact same config works.

It's tempting to blame libc, it looks like a bug in its tsx stuff.

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

[PATCH v4 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.

v2:	transform an extra instance of ring->default_context introduced by
    42f1cae8c drm/i915: Restore inhibiting the load of the default context
    That patch's commentary includes:
	v2: Mark the global default context as uninitialized on GPU reset so
	    that the context-local workarounds are reloaded upon re-enabling
    The code implementing that now also benefits from the replacement of
    the multiple (per-ring) pointers to the default context with a single
    pointer to the unique kernel context.

v4:	Rebased, remove underused local (Nick Hoath)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 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    | 29 +++++++++++------------------
 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           | 17 +++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
 8 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0b3550f..37c2c50 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1961,7 +1961,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);
 		}
@@ -2058,7 +2058,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, ctx, ring);
 		}
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dfebf9f..59dc701 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1959,6 +1959,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 8e716b6..06abe1b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2680,7 +2680,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);
 		}
 
@@ -2776,7 +2776,7 @@ struct drm_i915_gem_request *
 	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;
 }
@@ -4864,7 +4864,7 @@ int i915_gem_init_rings(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 c25083c..6a4f64b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -347,22 +347,20 @@ void i915_gem_context_reset(struct drm_device *dev)
 			i915_gem_context_unreference(lctx);
 			ring->last_context = NULL;
 		}
-
-		/* Force the GPU state to be reinitialised on enabling */
-		if (ring->default_context)
-			ring->default_context->legacy_hw_ctx.initialized = false;
 	}
+
+	/* Force the GPU state to be reinitialised on enabling */
+	dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
 }
 
 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)) {
@@ -392,12 +390,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" :
@@ -408,7 +401,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) {
@@ -435,17 +428,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 ec2482da..2c6da40 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -598,7 +598,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);
@@ -690,7 +690,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;
@@ -1006,7 +1006,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);
@@ -1529,7 +1529,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);
@@ -2005,6 +2005,7 @@ 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 = to_i915(dev)->kernel_context;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -2027,12 +2028,12 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	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);
+	ret = intel_lr_context_do_pin(ring, dctx);
 	if (ret) {
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
@@ -2398,7 +2399,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);
 			}
@@ -2517,7 +2518,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 ede5795..ce10228 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -309,7 +309,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 v4 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.

v4:	Rebased

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 37c2c50..c5db235 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1960,11 +1960,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');
@@ -2056,12 +2053,10 @@ 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, ctx, ring);
-		}
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 06abe1b..6a3e4ee 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2679,10 +2679,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 2c6da40..134379d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -686,16 +686,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
@@ -709,7 +703,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,
@@ -2391,22 +2388,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);
 	}
 }
 
@@ -2481,7 +2477,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

✗ Fi.CI.BAT: warning for improve handling of the driver's default (kernel) context

[Patchwork]

== Summary ==

Built on 7d26528d30b0d8119c858115b6e5e22deb74ec71 drm-intel-nightly: 2016y-01m-19d-19h-38m-52s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (bdw-nuci7) UNSTABLE
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-warn -> PASS       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (ilk-hp8440p)

bdw-nuci7        total:140  pass:130  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:140  pass:132  dwarn:1   dfail:1   fail:0   skip:6  
byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38 
skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1225/

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

Re: ✗ Fi.CI.BAT: warning for improve handling of the driver's default (kernel) context

[Dave Gordon]

On 20/01/16 07:49, Patchwork wrote:
> == Summary ==
>
> Built on 7d26528d30b0d8119c858115b6e5e22deb74ec71 drm-intel-nightly: 2016y-01m-19d-19h-38m-52s UTC integration manifest
>
> Test gem_storedw_loop:
>          Subgroup basic-render:
>                  pass       -> DMESG-WARN (bdw-nuci7) UNSTABLE

https://bugs.freedesktop.org/show_bug.cgi?id=93693
[BAT SKL BDW] missed interrupt in gem_storedw_loop/basic-render with 
*ERROR* Hangcheck timer elapsed...

> Test kms_flip:
>          Subgroup basic-flip-vs-wf_vblank:
>                  dmesg-warn -> PASS       (ilk-hp8440p)
> Test kms_pipe_crc_basic:
>          Subgroup hang-read-crc-pipe-a:
>                  pass       -> DMESG-WARN (ilk-hp8440p)

https://bugs.freedesktop.org/show_bug.cgi?id=93787
"[BAT ILK] sporadic fifo underruns"

The two 'dmesg-warn' -> PASS changes are similar cases where the 
underrun just happened to occur in the reference run and not in this 
patch test.

Also compare Bug 93640 - [BAT ILK SNB IVB bisected] Fifo underruns since 
CI_DRM_952

I'd consider marking these tests as UNSTABLE on ILK for now.

(Although, this was against CI_DRM_987, and it looks like maybe it's 
fixed after CI_DRM_989, at least on ILK).

.Dave.

>          Subgroup read-crc-pipe-b:
>                  dmesg-warn -> PASS       (ilk-hp8440p)
>
> bdw-nuci7        total:140  pass:130  dwarn:1   dfail:0   fail:0   skip:9
> bdw-ultra        total:140  pass:132  dwarn:1   dfail:1   fail:0   skip:6
> byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15
> hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7
> hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4
> ilk-hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38
> skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8
> skl-i7k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8
> snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14
> snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13
>
> Results at /archive/results/CI_IGT_test/Patchwork_1225/


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

Re: ✗ Fi.CI.BAT: warning for improve handling of the driver's default (kernel) context

[Daniel Vetter]

On Wed, Jan 20, 2016 at 05:31:00PM +0000, Dave Gordon wrote:
> On 20/01/16 07:49, Patchwork wrote:
> >== Summary ==
> >
> >Built on 7d26528d30b0d8119c858115b6e5e22deb74ec71 drm-intel-nightly: 2016y-01m-19d-19h-38m-52s UTC integration manifest
> >
> >Test gem_storedw_loop:
> >         Subgroup basic-render:
> >                 pass       -> DMESG-WARN (bdw-nuci7) UNSTABLE
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=93693
> [BAT SKL BDW] missed interrupt in gem_storedw_loop/basic-render with *ERROR*
> Hangcheck timer elapsed...
> 
> >Test kms_flip:
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 dmesg-warn -> PASS       (ilk-hp8440p)
> >Test kms_pipe_crc_basic:
> >         Subgroup hang-read-crc-pipe-a:
> >                 pass       -> DMESG-WARN (ilk-hp8440p)
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=93787
> "[BAT ILK] sporadic fifo underruns"
> 
> The two 'dmesg-warn' -> PASS changes are similar cases where the underrun
> just happened to occur in the reference run and not in this patch test.
> 
> Also compare Bug 93640 - [BAT ILK SNB IVB bisected] Fifo underruns since
> CI_DRM_952
> 
> I'd consider marking these tests as UNSTABLE on ILK for now.
> 
> (Although, this was against CI_DRM_987, and it looks like maybe it's fixed
> after CI_DRM_989, at least on ILK).

It's jumping around all over right now, which is why we can't really mark
it as unstable. I thought this was just fallout from Matt breaking
watermarks on ilk-ivb, but reverting that patch uncovered that we have
some other fifo underruns still (or again, was 2 weeks without coverage
due to Matt's regression) happening. I'm looking into this, if I can coax
my ilk into coperation.
-Daniel

> 
> .Dave.
> 
> >         Subgroup read-crc-pipe-b:
> >                 dmesg-warn -> PASS       (ilk-hp8440p)
> >
> >bdw-nuci7        total:140  pass:130  dwarn:1   dfail:0   fail:0   skip:9
> >bdw-ultra        total:140  pass:132  dwarn:1   dfail:1   fail:0   skip:6
> >byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15
> >hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7
> >hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4
> >ilk-hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38
> >skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8
> >skl-i7k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8
> >snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14
> >snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13
> >
> >Results at /archive/results/CI_IGT_test/Patchwork_1225/
> 
> 
> _______________________________________________
> 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 v4 0/3] improve handling of the driver's default (kernel) context

[Daniel Vetter]

On Tue, Jan 19, 2016 at 07:02:52PM +0000, Dave Gordon wrote:
> 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.
> (We now call it "the kernel context", and compare against the device-wide
> pointer).  [Chris Wilson] 
> 
> v4: Rebased

entire series vacuumed up, thanks.
-Daniel
-- 
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

✓ Fi.CI.BAT: success for improve handling of the driver's default (kernel) context (rev3)

[Patchwork]

== Summary ==

Built on 5ae916607e3e12ba18c848dff42baaad5b118c4b drm-intel-nightly: 2016y-01m-27d-12h-48m-36s UTC integration manifest


bdw-nuci7        total:141  pass:132  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:144  pass:138  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:144  pass:120  dwarn:0   dfail:0   fail:0   skip:24 
hsw-brixbox      total:144  pass:137  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:144  pass:140  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:144  pass:105  dwarn:0   dfail:0   fail:1   skip:38 
ivb-t430s        total:144  pass:138  dwarn:0   dfail:0   fail:0   skip:6  
skl-i5k-2        total:144  pass:135  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:144  pass:130  dwarn:0   dfail:0   fail:0   skip:14 
snb-x220t        total:144  pass:130  dwarn:0   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1262/

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

[PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

From: Dave Gordon <david.s.gordon@intel.com>

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

  2682708 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5cb57f642ac1..8fd00d279447 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1655,7 +1655,7 @@ 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 && req)
+	if (ret && !IS_ERR_OR_NULL(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 8104511ad302..b88cdac747eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 cleanup_unpin:
 	intel_unpin_fb_obj(fb, crtc->primary->state);
 cleanup_pending:
-	if (request)
+	if (!IS_ERR_OR_NULL(request))
 		i915_gem_request_cancel(request);
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.9.1

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 28/01/16 10:48, Tvrtko Ursulin wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
>
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
>
>    2682708 drm/i915: simplify allocation of driver-internal requests
>
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Re-sent for CI since we need this fix in ASAP.

Regards,

Tvrtko

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