[PATCH 0/8] Execlists prep-work (II)

[PATCH 0/8] Execlists prep-work (II)

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

These patches contain more refactoring and preparatory work for Execlists [1].

[1] http://lists.freedesktop.org/archives/intel-gfx/2014-June/047138.html

Oscar Mateo (8):
  drm/i915: Extract context backing object allocation
  drm/i915: Rename ctx->obj to ctx->rcs_state
  drm/i915: Rename ctx->is_initialized to ctx->rcs_is_initialized
  drm/i915: Rename ctx->id to ctx->handle
  drm/i915: Extract ringbuffer destroy & generalize alloc to take a
    ringbuf
  drm/i915: Generalize ring_space to take a ringbuf
  drm/i915: Generalize intel_ring_get_tail to take a ringbuf
  drm/i915: Extract the actual workload submission mechanism from
    execbuffer

 drivers/gpu/drm/i915/i915_debugfs.c        |   8 +-
 drivers/gpu/drm/i915/i915_drv.h            |  10 +-
 drivers/gpu/drm/i915/i915_gem.c            |   4 +-
 drivers/gpu/drm/i915/i915_gem_context.c    | 132 +++++++------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 300 ++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  39 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   4 +-
 drivers/gpu/drm/i915/intel_uncore.c        |   2 +-
 8 files changed, 273 insertions(+), 226 deletions(-)

-- 
1.9.0

[PATCH 1/8] drm/i915: Extract context backing object allocation

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

This is preparatory work for Execlists: we plan to use it later to
allocate our own context objects (since Logical Ring Contexts do
not have the same kind of backing objects).

No functional changes.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 54 +++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 21eda88..ab25368 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -198,6 +198,36 @@ void i915_gem_context_free(struct kref *ctx_ref)
 	kfree(ctx);
 }
 
+static struct drm_i915_gem_object *
+i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
+{
+	struct drm_i915_gem_object *obj;
+	int ret;
+
+	obj = i915_gem_alloc_object(dev, size);
+	if (obj == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * Try to make the context utilize L3 as well as LLC.
+	 *
+	 * On VLV we don't have L3 controls in the PTEs so we
+	 * shouldn't touch the cache level, especially as that
+	 * would make the object snooped which might have a
+	 * negative performance impact.
+	 */
+	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
+		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_L3_LLC);
+		/* Failure shouldn't ever happen this early */
+		if (WARN_ON(ret)) {
+			drm_gem_object_unreference(&obj->base);
+			return ERR_PTR(ret);
+		}
+	}
+
+	return obj;
+}
+
 static struct i915_hw_ppgtt *
 create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx)
 {
@@ -234,27 +264,13 @@ __create_hw_context(struct drm_device *dev,
 	list_add_tail(&ctx->link, &dev_priv->context_list);
 
 	if (dev_priv->hw_context_size) {
-		ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
-		if (ctx->obj == NULL) {
-			ret = -ENOMEM;
+		struct drm_i915_gem_object *obj =
+				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
+		if (IS_ERR(obj)) {
+			ret = PTR_ERR(obj);
 			goto err_out;
 		}
-
-		/*
-		 * Try to make the context utilize L3 as well as LLC.
-		 *
-		 * On VLV we don't have L3 controls in the PTEs so we
-		 * shouldn't touch the cache level, especially as that
-		 * would make the object snooped which might have a
-		 * negative performance impact.
-		 */
-		if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
-			ret = i915_gem_object_set_cache_level(ctx->obj,
-							      I915_CACHE_L3_LLC);
-			/* Failure shouldn't ever happen this early */
-			if (WARN_ON(ret))
-				goto err_out;
-		}
+		ctx->obj = obj;
 	}
 
 	/* Default context will never have a file_priv */
-- 
1.9.0

Re: [PATCH 1/8] drm/i915: Extract context backing object allocation

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:12 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is preparatory work for Execlists: we plan to use it later to
> allocate our own context objects (since Logical Ring Contexts do
> not have the same kind of backing objects).
> 
> No functional changes.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 54 +++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 21eda88..ab25368 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -198,6 +198,36 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  	kfree(ctx);
>  }
>  
> +static struct drm_i915_gem_object *
> +i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
> +{
> +	struct drm_i915_gem_object *obj;
> +	int ret;
> +
> +	obj = i915_gem_alloc_object(dev, size);
> +	if (obj == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * Try to make the context utilize L3 as well as LLC.
> +	 *
> +	 * On VLV we don't have L3 controls in the PTEs so we
> +	 * shouldn't touch the cache level, especially as that
> +	 * would make the object snooped which might have a
> +	 * negative performance impact.
> +	 */
> +	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
> +		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_L3_LLC);
> +		/* Failure shouldn't ever happen this early */
> +		if (WARN_ON(ret)) {
> +			drm_gem_object_unreference(&obj->base);
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	return obj;
> +}
> +
>  static struct i915_hw_ppgtt *
>  create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx)
>  {
> @@ -234,27 +264,13 @@ __create_hw_context(struct drm_device *dev,
>  	list_add_tail(&ctx->link, &dev_priv->context_list);
>  
>  	if (dev_priv->hw_context_size) {
> -		ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> -		if (ctx->obj == NULL) {
> -			ret = -ENOMEM;
> +		struct drm_i915_gem_object *obj =
> +				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
> +		if (IS_ERR(obj)) {
> +			ret = PTR_ERR(obj);
>  			goto err_out;
>  		}
> -
> -		/*
> -		 * Try to make the context utilize L3 as well as LLC.
> -		 *
> -		 * On VLV we don't have L3 controls in the PTEs so we
> -		 * shouldn't touch the cache level, especially as that
> -		 * would make the object snooped which might have a
> -		 * negative performance impact.
> -		 */
> -		if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
> -			ret = i915_gem_object_set_cache_level(ctx->obj,
> -							      I915_CACHE_L3_LLC);
> -			/* Failure shouldn't ever happen this early */
> -			if (WARN_ON(ret))
> -				goto err_out;
> -		}
> +		ctx->obj = obj;
>  	}
>  
>  	/* Default context will never have a file_priv */

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

This is Execlists preparatory work.

We have already advanced that Logical Ring Contexts have their own kind
ob backing objects, but everything will be better explained in the Execlists
series. For now, suffice it to say that this backing object is only
ever used with the render ring, so we're making this fact more explicit
(which is a good reason on its own).

Done with the following Coccinelle patch (plus manual renaming of the
struct field):

	@@
	struct intel_context c;
	@@
	- (c).obj
	+ c.rcs_state

	@@
	 *c;
	@@
	- (c)->obj
	+ c->rcs_state

No functional changes.

v2: Go with rcs_state instead of render_obj, as suggested by Chris Wilson.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  4 +--
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 62 ++++++++++++++++-----------------
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6b7b32b..b7bcfd5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1744,7 +1744,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
 	}
 
 	list_for_each_entry(ctx, &dev_priv->context_list, link) {
-		if (ctx->obj == NULL)
+		if (ctx->rcs_state == NULL)
 			continue;
 
 		seq_puts(m, "HW context ");
@@ -1753,7 +1753,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
 			if (ring->default_context == ctx)
 				seq_printf(m, "(default context %s) ", ring->name);
 
-		describe_obj(m, ctx->obj);
+		describe_obj(m, ctx->rcs_state);
 		seq_putc(m, '\n');
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..b7c6388 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -591,7 +591,7 @@ struct intel_context {
 	bool is_initialized;
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *rcs_state;
 	struct i915_ctx_hang_stats hang_stats;
 	struct i915_address_space *vm;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ab25368..9cc31c6 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -182,14 +182,14 @@ void i915_gem_context_free(struct kref *ctx_ref)
 						   typeof(*ctx), ref);
 	struct i915_hw_ppgtt *ppgtt = NULL;
 
-	if (ctx->obj) {
+	if (ctx->rcs_state) {
 		/* We refcount even the aliasing PPGTT to keep the code symmetric */
-		if (USES_PPGTT(ctx->obj->base.dev))
+		if (USES_PPGTT(ctx->rcs_state->base.dev))
 			ppgtt = ctx_to_ppgtt(ctx);
 
 		/* XXX: Free up the object before tearing down the address space, in
 		 * case we're bound in the PPGTT */
-		drm_gem_object_unreference(&ctx->obj->base);
+		drm_gem_object_unreference(&ctx->rcs_state->base);
 	}
 
 	if (ppgtt)
@@ -270,7 +270,7 @@ __create_hw_context(struct drm_device *dev,
 			ret = PTR_ERR(obj);
 			goto err_out;
 		}
-		ctx->obj = obj;
+		ctx->rcs_state = obj;
 	}
 
 	/* Default context will never have a file_priv */
@@ -317,7 +317,7 @@ i915_gem_create_context(struct drm_device *dev,
 	if (IS_ERR(ctx))
 		return ctx;
 
-	if (is_global_default_ctx && ctx->obj) {
+	if (is_global_default_ctx && 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
@@ -325,7 +325,7 @@ i915_gem_create_context(struct drm_device *dev,
 		 * be available. To avoid this we always pin the default
 		 * context.
 		 */
-		ret = i915_gem_obj_ggtt_pin(ctx->obj,
+		ret = i915_gem_obj_ggtt_pin(ctx->rcs_state,
 					    get_context_alignment(dev), 0);
 		if (ret) {
 			DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
@@ -365,8 +365,8 @@ i915_gem_create_context(struct drm_device *dev,
 	return ctx;
 
 err_unpin:
-	if (is_global_default_ctx && ctx->obj)
-		i915_gem_object_ggtt_unpin(ctx->obj);
+	if (is_global_default_ctx && ctx->rcs_state)
+		i915_gem_object_ggtt_unpin(ctx->rcs_state);
 err_destroy:
 	i915_gem_context_unreference(ctx);
 	return ERR_PTR(ret);
@@ -390,12 +390,12 @@ void i915_gem_context_reset(struct drm_device *dev)
 		if (!ring->last_context)
 			continue;
 
-		if (dctx->obj && i == RCS) {
-			WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
+		if (dctx->rcs_state && i == RCS) {
+			WARN_ON(i915_gem_obj_ggtt_pin(dctx->rcs_state,
 						      get_context_alignment(dev), 0));
 			/* Fake a finish/inactive */
-			dctx->obj->base.write_domain = 0;
-			dctx->obj->active = 0;
+			dctx->rcs_state->base.write_domain = 0;
+			dctx->rcs_state->active = 0;
 		}
 
 		i915_gem_context_unreference(ring->last_context);
@@ -445,7 +445,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
 	int i;
 
-	if (dctx->obj) {
+	if (dctx->rcs_state) {
 		/* The only known way to stop the gpu from accessing the hw context is
 		 * to reset it. Do this as the very last operation to avoid confusing
 		 * other code, leading to spurious errors. */
@@ -460,13 +460,13 @@ void i915_gem_context_fini(struct drm_device *dev)
 		WARN_ON(!dev_priv->ring[RCS].last_context);
 		if (dev_priv->ring[RCS].last_context == dctx) {
 			/* Fake switch to NULL context */
-			WARN_ON(dctx->obj->active);
-			i915_gem_object_ggtt_unpin(dctx->obj);
+			WARN_ON(dctx->rcs_state->active);
+			i915_gem_object_ggtt_unpin(dctx->rcs_state);
 			i915_gem_context_unreference(dctx);
 			dev_priv->ring[RCS].last_context = NULL;
 		}
 
-		i915_gem_object_ggtt_unpin(dctx->obj);
+		i915_gem_object_ggtt_unpin(dctx->rcs_state);
 	}
 
 	for (i = 0; i < I915_NUM_RINGS; i++) {
@@ -586,7 +586,7 @@ mi_set_context(struct intel_engine_cs *ring,
 
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
+	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->rcs_state) |
 			MI_MM_SPACE_GTT |
 			MI_SAVE_EXT_STATE_EN |
 			MI_RESTORE_EXT_STATE_EN |
@@ -617,8 +617,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	int ret, i;
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
-		BUG_ON(from->obj == NULL);
-		BUG_ON(!i915_gem_obj_is_pinned(from->obj));
+		BUG_ON(from->rcs_state == NULL);
+		BUG_ON(!i915_gem_obj_is_pinned(from->rcs_state));
 	}
 
 	if (from == to && !to->remap_slice)
@@ -626,7 +626,7 @@ static int do_switch(struct intel_engine_cs *ring,
 
 	/* Trying to pin first makes error handling easier. */
 	if (ring == &dev_priv->ring[RCS]) {
-		ret = i915_gem_obj_ggtt_pin(to->obj,
+		ret = i915_gem_obj_ggtt_pin(to->rcs_state,
 					    get_context_alignment(ring->dev), 0);
 		if (ret)
 			return ret;
@@ -659,14 +659,14 @@ static int do_switch(struct intel_engine_cs *ring,
 	 *
 	 * XXX: We need a real interface to do this instead of trickery.
 	 */
-	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
+	ret = i915_gem_object_set_to_gtt_domain(to->rcs_state, false);
 	if (ret)
 		goto unpin_out;
 
-	if (!to->obj->has_global_gtt_mapping) {
-		struct i915_vma *vma = i915_gem_obj_to_vma(to->obj,
+	if (!to->rcs_state->has_global_gtt_mapping) {
+		struct i915_vma *vma = i915_gem_obj_to_vma(to->rcs_state,
 							   &dev_priv->gtt.base);
-		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
+		vma->bind_vma(vma, to->rcs_state->cache_level, GLOBAL_BIND);
 	}
 
 	if (!to->is_initialized || i915_gem_context_is_default(to))
@@ -695,8 +695,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	 * MI_SET_CONTEXT instead of when the next seqno has completed.
 	 */
 	if (from != NULL) {
-		from->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->obj), ring);
+		from->rcs_state->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->rcs_state), ring);
 		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
 		 * whole damn pipeline, we don't need to explicitly mark the
 		 * object dirty. The only exception is that the context must be
@@ -704,11 +704,11 @@ static int do_switch(struct intel_engine_cs *ring,
 		 * able to defer doing this until we know the object would be
 		 * swapped, but there is no way to do that yet.
 		 */
-		from->obj->dirty = 1;
-		BUG_ON(from->obj->ring != ring);
+		from->rcs_state->dirty = 1;
+		BUG_ON(from->rcs_state->ring != ring);
 
 		/* obj is kept alive until the next request by its active ref */
-		i915_gem_object_ggtt_unpin(from->obj);
+		i915_gem_object_ggtt_unpin(from->rcs_state);
 		i915_gem_context_unreference(from);
 	}
 
@@ -728,7 +728,7 @@ done:
 
 unpin_out:
 	if (ring->id == RCS)
-		i915_gem_object_ggtt_unpin(to->obj);
+		i915_gem_object_ggtt_unpin(to->rcs_state);
 	return ret;
 }
 
@@ -749,7 +749,7 @@ int i915_switch_context(struct intel_engine_cs *ring,
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
-	if (to->obj == NULL) { /* We have the fake context */
+	if (to->rcs_state == NULL) { /* We have the fake context */
 		if (to != ring->last_context) {
 			i915_gem_context_reference(to);
 			if (ring->last_context)
-- 
1.9.0

Re: [PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:13 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is Execlists preparatory work.
> 
> We have already advanced that Logical Ring Contexts have their own kind
> ob backing objects, but everything will be better explained in the Execlists
> series. For now, suffice it to say that this backing object is only
> ever used with the render ring, so we're making this fact more explicit
> (which is a good reason on its own).
> 
> Done with the following Coccinelle patch (plus manual renaming of the
> struct field):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).obj
> 	+ c.rcs_state
> 
> 	@@
> 	 *c;
> 	@@
> 	- (c)->obj
> 	+ c->rcs_state
> 
> No functional changes.
> 
> v2: Go with rcs_state instead of render_obj, as suggested by Chris Wilson.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  4 +--
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 62 ++++++++++++++++-----------------
>  3 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6b7b32b..b7bcfd5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1744,7 +1744,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  	}
>  
>  	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> -		if (ctx->obj == NULL)
> +		if (ctx->rcs_state == NULL)
>  			continue;
>  
>  		seq_puts(m, "HW context ");
> @@ -1753,7 +1753,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  			if (ring->default_context == ctx)
>  				seq_printf(m, "(default context %s) ", ring->name);
>  
> -		describe_obj(m, ctx->obj);
> +		describe_obj(m, ctx->rcs_state);
>  		seq_putc(m, '\n');
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8cea596..b7c6388 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -591,7 +591,7 @@ struct intel_context {
>  	bool is_initialized;
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_gem_object *rcs_state;
>  	struct i915_ctx_hang_stats hang_stats;
>  	struct i915_address_space *vm;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ab25368..9cc31c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -182,14 +182,14 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  						   typeof(*ctx), ref);
>  	struct i915_hw_ppgtt *ppgtt = NULL;
>  
> -	if (ctx->obj) {
> +	if (ctx->rcs_state) {
>  		/* We refcount even the aliasing PPGTT to keep the code symmetric */
> -		if (USES_PPGTT(ctx->obj->base.dev))
> +		if (USES_PPGTT(ctx->rcs_state->base.dev))
>  			ppgtt = ctx_to_ppgtt(ctx);
>  
>  		/* XXX: Free up the object before tearing down the address space, in
>  		 * case we're bound in the PPGTT */
> -		drm_gem_object_unreference(&ctx->obj->base);
> +		drm_gem_object_unreference(&ctx->rcs_state->base);
>  	}
>  
>  	if (ppgtt)
> @@ -270,7 +270,7 @@ __create_hw_context(struct drm_device *dev,
>  			ret = PTR_ERR(obj);
>  			goto err_out;
>  		}
> -		ctx->obj = obj;
> +		ctx->rcs_state = obj;
>  	}
>  
>  	/* Default context will never have a file_priv */
> @@ -317,7 +317,7 @@ i915_gem_create_context(struct drm_device *dev,
>  	if (IS_ERR(ctx))
>  		return ctx;
>  
> -	if (is_global_default_ctx && ctx->obj) {
> +	if (is_global_default_ctx && 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
> @@ -325,7 +325,7 @@ i915_gem_create_context(struct drm_device *dev,
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->obj,
> +		ret = i915_gem_obj_ggtt_pin(ctx->rcs_state,
>  					    get_context_alignment(dev), 0);
>  		if (ret) {
>  			DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
> @@ -365,8 +365,8 @@ i915_gem_create_context(struct drm_device *dev,
>  	return ctx;
>  
>  err_unpin:
> -	if (is_global_default_ctx && ctx->obj)
> -		i915_gem_object_ggtt_unpin(ctx->obj);
> +	if (is_global_default_ctx && ctx->rcs_state)
> +		i915_gem_object_ggtt_unpin(ctx->rcs_state);
>  err_destroy:
>  	i915_gem_context_unreference(ctx);
>  	return ERR_PTR(ret);
> @@ -390,12 +390,12 @@ void i915_gem_context_reset(struct drm_device *dev)
>  		if (!ring->last_context)
>  			continue;
>  
> -		if (dctx->obj && i == RCS) {
> -			WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> +		if (dctx->rcs_state && i == RCS) {
> +			WARN_ON(i915_gem_obj_ggtt_pin(dctx->rcs_state,
>  						      get_context_alignment(dev), 0));
>  			/* Fake a finish/inactive */
> -			dctx->obj->base.write_domain = 0;
> -			dctx->obj->active = 0;
> +			dctx->rcs_state->base.write_domain = 0;
> +			dctx->rcs_state->active = 0;
>  		}
>  
>  		i915_gem_context_unreference(ring->last_context);
> @@ -445,7 +445,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
>  	int i;
>  
> -	if (dctx->obj) {
> +	if (dctx->rcs_state) {
>  		/* The only known way to stop the gpu from accessing the hw context is
>  		 * to reset it. Do this as the very last operation to avoid confusing
>  		 * other code, leading to spurious errors. */
> @@ -460,13 +460,13 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		WARN_ON(!dev_priv->ring[RCS].last_context);
>  		if (dev_priv->ring[RCS].last_context == dctx) {
>  			/* Fake switch to NULL context */
> -			WARN_ON(dctx->obj->active);
> -			i915_gem_object_ggtt_unpin(dctx->obj);
> +			WARN_ON(dctx->rcs_state->active);
> +			i915_gem_object_ggtt_unpin(dctx->rcs_state);
>  			i915_gem_context_unreference(dctx);
>  			dev_priv->ring[RCS].last_context = NULL;
>  		}
>  
> -		i915_gem_object_ggtt_unpin(dctx->obj);
> +		i915_gem_object_ggtt_unpin(dctx->rcs_state);
>  	}
>  
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
> @@ -586,7 +586,7 @@ mi_set_context(struct intel_engine_cs *ring,
>  
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
> +	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->rcs_state) |
>  			MI_MM_SPACE_GTT |
>  			MI_SAVE_EXT_STATE_EN |
>  			MI_RESTORE_EXT_STATE_EN |
> @@ -617,8 +617,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> -		BUG_ON(from->obj == NULL);
> -		BUG_ON(!i915_gem_obj_is_pinned(from->obj));
> +		BUG_ON(from->rcs_state == NULL);
> +		BUG_ON(!i915_gem_obj_is_pinned(from->rcs_state));
>  	}
>  
>  	if (from == to && !to->remap_slice)
> @@ -626,7 +626,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  
>  	/* Trying to pin first makes error handling easier. */
>  	if (ring == &dev_priv->ring[RCS]) {
> -		ret = i915_gem_obj_ggtt_pin(to->obj,
> +		ret = i915_gem_obj_ggtt_pin(to->rcs_state,
>  					    get_context_alignment(ring->dev), 0);
>  		if (ret)
>  			return ret;
> @@ -659,14 +659,14 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 *
>  	 * XXX: We need a real interface to do this instead of trickery.
>  	 */
> -	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
> +	ret = i915_gem_object_set_to_gtt_domain(to->rcs_state, false);
>  	if (ret)
>  		goto unpin_out;
>  
> -	if (!to->obj->has_global_gtt_mapping) {
> -		struct i915_vma *vma = i915_gem_obj_to_vma(to->obj,
> +	if (!to->rcs_state->has_global_gtt_mapping) {
> +		struct i915_vma *vma = i915_gem_obj_to_vma(to->rcs_state,
>  							   &dev_priv->gtt.base);
> -		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
> +		vma->bind_vma(vma, to->rcs_state->cache_level, GLOBAL_BIND);
>  	}
>  
>  	if (!to->is_initialized || i915_gem_context_is_default(to))
> @@ -695,8 +695,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 * MI_SET_CONTEXT instead of when the next seqno has completed.
>  	 */
>  	if (from != NULL) {
> -		from->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> -		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->obj), ring);
> +		from->rcs_state->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->rcs_state), ring);
>  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> @@ -704,11 +704,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  		 * able to defer doing this until we know the object would be
>  		 * swapped, but there is no way to do that yet.
>  		 */
> -		from->obj->dirty = 1;
> -		BUG_ON(from->obj->ring != ring);
> +		from->rcs_state->dirty = 1;
> +		BUG_ON(from->rcs_state->ring != ring);
>  
>  		/* obj is kept alive until the next request by its active ref */
> -		i915_gem_object_ggtt_unpin(from->obj);
> +		i915_gem_object_ggtt_unpin(from->rcs_state);
>  		i915_gem_context_unreference(from);
>  	}
>  
> @@ -728,7 +728,7 @@ done:
>  
>  unpin_out:
>  	if (ring->id == RCS)
> -		i915_gem_object_ggtt_unpin(to->obj);
> +		i915_gem_object_ggtt_unpin(to->rcs_state);
>  	return ret;
>  }
>  
> @@ -749,7 +749,7 @@ int i915_switch_context(struct intel_engine_cs *ring,
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>  
> -	if (to->obj == NULL) { /* We have the fake context */
> +	if (to->rcs_state == NULL) { /* We have the fake context */
>  		if (to != ring->last_context) {
>  			i915_gem_context_reference(to);
>  			if (ring->last_context)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[Chris Wilson]

On Thu, Jun 26, 2014 at 02:24:13PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is Execlists preparatory work.
> 
> We have already advanced that Logical Ring Contexts have their own kind
> ob backing objects, but everything will be better explained in the Execlists
> series. For now, suffice it to say that this backing object is only
> ever used with the render ring, so we're making this fact more explicit
> (which is a good reason on its own).
> 
> Done with the following Coccinelle patch (plus manual renaming of the
> struct field):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).obj
> 	+ c.rcs_state
> 
> 	@@
> 	 *c;
> 	@@
> 	- (c)->obj
> 	+ c->rcs_state
> 
> No functional changes.
> 
> v2: Go with rcs_state instead of render_obj, as suggested by Chris Wilson.

Another little change here is ctx->is_initialised if you create
  struct {
	  struct drm_i915_gem_object *rcs_state;
	  bool initialised;
  } legacy_hw_ctx;
that should also address Daniel's confusion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 03, 2014 10:47 AM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/8] drm/i915: Rename ctx->obj to ctx-
> >rcs_state
> 
> On Thu, Jun 26, 2014 at 02:24:13PM +0100, oscar.mateo@intel.com wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> >
> > This is Execlists preparatory work.
> >
> > We have already advanced that Logical Ring Contexts have their own
> > kind ob backing objects, but everything will be better explained in
> > the Execlists series. For now, suffice it to say that this backing
> > object is only ever used with the render ring, so we're making this
> > fact more explicit (which is a good reason on its own).
> >
> > Done with the following Coccinelle patch (plus manual renaming of the
> > struct field):
> >
> > 	@@
> > 	struct intel_context c;
> > 	@@
> > 	- (c).obj
> > 	+ c.rcs_state
> >
> > 	@@
> > 	 *c;
> > 	@@
> > 	- (c)->obj
> > 	+ c->rcs_state
> >
> > No functional changes.
> >
> > v2: Go with rcs_state instead of render_obj, as suggested by Chris Wilson.
> 
> Another little change here is ctx->is_initialised if you create
>   struct {
> 	  struct drm_i915_gem_object *rcs_state;
> 	  bool initialised;
>   } legacy_hw_ctx;
> that should also address Daniel's confusion.
> -Chris

Daniel said exactly the same thing, but I´m reusing the rcs_initialized field in Execlists to mark the default render context as ready after setting the golden/null state (so that I only do it after module load, and not after reset/thaw). I can add a new field for this, but IMHO this one makes sense.

Re: [PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[Chris Wilson]

On Thu, Jul 03, 2014 at 12:08:42PM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, July 03, 2014 10:47 AM
> > To: Mateo Lozano, Oscar
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 2/8] drm/i915: Rename ctx->obj to ctx-
> > >rcs_state
> > 
> > On Thu, Jun 26, 2014 at 02:24:13PM +0100, oscar.mateo@intel.com wrote:
> > > From: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > This is Execlists preparatory work.
> > >
> > > We have already advanced that Logical Ring Contexts have their own
> > > kind ob backing objects, but everything will be better explained in
> > > the Execlists series. For now, suffice it to say that this backing
> > > object is only ever used with the render ring, so we're making this
> > > fact more explicit (which is a good reason on its own).
> > >
> > > Done with the following Coccinelle patch (plus manual renaming of the
> > > struct field):
> > >
> > > 	@@
> > > 	struct intel_context c;
> > > 	@@
> > > 	- (c).obj
> > > 	+ c.rcs_state
> > >
> > > 	@@
> > > 	 *c;
> > > 	@@
> > > 	- (c)->obj
> > > 	+ c->rcs_state
> > >
> > > No functional changes.
> > >
> > > v2: Go with rcs_state instead of render_obj, as suggested by Chris Wilson.
> > 
> > Another little change here is ctx->is_initialised if you create
> >   struct {
> > 	  struct drm_i915_gem_object *rcs_state;
> > 	  bool initialised;
> >   } legacy_hw_ctx;
> > that should also address Daniel's confusion.
> > -Chris
> 
> Daniel said exactly the same thing, but I´m reusing the rcs_initialized field in Execlists to mark the default render context as ready after setting the golden/null state (so that I only do it after module load, and not after reset/thaw). I can add a new field for this, but IMHO this one makes sense.

I would isolate the two. The use may be mutually exclusive, but the
semantics are not...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: Rename ctx->obj to ctx->rcs_state

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 03, 2014 1:28 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/8] drm/i915: Rename ctx->obj to ctx-
> >rcs_state
> 
> On Thu, Jul 03, 2014 at 12:08:42PM +0000, Mateo Lozano, Oscar wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Thursday, July 03, 2014 10:47 AM
> > > To: Mateo Lozano, Oscar
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 2/8] drm/i915: Rename ctx->obj to
> > > ctx-
> > > >rcs_state
> > >
> > > On Thu, Jun 26, 2014 at 02:24:13PM +0100, oscar.mateo@intel.com
> wrote:
> > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > >
> > > > This is Execlists preparatory work.
> > > >
> > > > We have already advanced that Logical Ring Contexts have their own
> > > > kind ob backing objects, but everything will be better explained
> > > > in the Execlists series. For now, suffice it to say that this
> > > > backing object is only ever used with the render ring, so we're
> > > > making this fact more explicit (which is a good reason on its own).
> > > >
> > > > Done with the following Coccinelle patch (plus manual renaming of
> > > > the struct field):
> > > >
> > > > 	@@
> > > > 	struct intel_context c;
> > > > 	@@
> > > > 	- (c).obj
> > > > 	+ c.rcs_state
> > > >
> > > > 	@@
> > > > 	 *c;
> > > > 	@@
> > > > 	- (c)->obj
> > > > 	+ c->rcs_state
> > > >
> > > > No functional changes.
> > > >
> > > > v2: Go with rcs_state instead of render_obj, as suggested by Chris
> Wilson.
> > >
> > > Another little change here is ctx->is_initialised if you create
> > >   struct {
> > > 	  struct drm_i915_gem_object *rcs_state;
> > > 	  bool initialised;
> > >   } legacy_hw_ctx;
> > > that should also address Daniel's confusion.
> > > -Chris
> >
> > Daniel said exactly the same thing, but I´m reusing the rcs_initialized field in
> Execlists to mark the default render context as ready after setting the
> golden/null state (so that I only do it after module load, and not after
> reset/thaw). I can add a new field for this, but IMHO this one makes sense.
> 
> I would isolate the two. The use may be mutually exclusive, but the
> semantics are not...

But... are we arguing or *debating* semantics? :)
Ok, I´ll rearrange the HW context stuff and create a separate initialized flag for the Execlists case.

[PATCH 3/8] drm/i915: Rename ctx->is_initialized to ctx->rcs_is_initialized

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

We only use this flag to signify that the render state (a.k.a. golden
context, a.k.a. null context) has been initialized. It doesn't mean
anything for the other engines, so make that distinction obvious.
This renaming was suggested by Daniel Vetter.

Implemented with this cocci script (plus manual changes to the struct
declaration):

	@
	struct intel_context c;
	@@
	- (c).is_initialized
	+ c.rcs_is_initialized

	@@
	struct intel_context *c;
	@@
	- (c)->is_initialized
	+ c->rcs_is_initialized

No functional changes.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 2 +-
 drivers/gpu/drm/i915/i915_drv.h         | 2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b7bcfd5..d4b8391 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -176,7 +176,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 
 static void describe_ctx(struct seq_file *m, struct intel_context *ctx)
 {
-	seq_putc(m, ctx->is_initialized ? 'I' : 'i');
+	seq_putc(m, ctx->rcs_is_initialized ? 'I' : 'i');
 	seq_putc(m, ctx->remap_slice ? 'R' : 'r');
 	seq_putc(m, ' ');
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b7c6388..122e942 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -588,7 +588,7 @@ struct i915_ctx_hang_stats {
 struct intel_context {
 	struct kref ref;
 	int id;
-	bool is_initialized;
+	bool rcs_is_initialized;
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
 	struct drm_i915_gem_object *rcs_state;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9cc31c6..b8b9859 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -669,7 +669,7 @@ static int do_switch(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->rcs_state->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->is_initialized || i915_gem_context_is_default(to))
+	if (!to->rcs_is_initialized || i915_gem_context_is_default(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
 
 	ret = mi_set_context(ring, to, hw_flags);
@@ -716,13 +716,13 @@ done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 
-	if (ring->id == RCS && !to->is_initialized && from == NULL) {
+	if (ring->id == RCS && !to->rcs_is_initialized && from == NULL) {
 		ret = i915_gem_render_state_init(ring);
 		if (ret)
 			DRM_ERROR("init render state: %d\n", ret);
 	}
 
-	to->is_initialized = true;
+	to->rcs_is_initialized = true;
 
 	return 0;
 
-- 
1.9.0

Re: [PATCH 3/8] drm/i915: Rename ctx->is_initialized to ctx->rcs_is_initialized

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:14 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> We only use this flag to signify that the render state (a.k.a. golden
> context, a.k.a. null context) has been initialized. It doesn't mean
> anything for the other engines, so make that distinction obvious.
> This renaming was suggested by Daniel Vetter.
> 
> Implemented with this cocci script (plus manual changes to the struct
> declaration):
> 
> 	@
> 	struct intel_context c;
> 	@@
> 	- (c).is_initialized
> 	+ c.rcs_is_initialized
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->is_initialized
> 	+ c->rcs_is_initialized
> 
> No functional changes.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 2 +-
>  drivers/gpu/drm/i915/i915_drv.h         | 2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b7bcfd5..d4b8391 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -176,7 +176,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  
>  static void describe_ctx(struct seq_file *m, struct intel_context *ctx)
>  {
> -	seq_putc(m, ctx->is_initialized ? 'I' : 'i');
> +	seq_putc(m, ctx->rcs_is_initialized ? 'I' : 'i');
>  	seq_putc(m, ctx->remap_slice ? 'R' : 'r');
>  	seq_putc(m, ' ');
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b7c6388..122e942 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -588,7 +588,7 @@ struct i915_ctx_hang_stats {
>  struct intel_context {
>  	struct kref ref;
>  	int id;
> -	bool is_initialized;
> +	bool rcs_is_initialized;
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
>  	struct drm_i915_gem_object *rcs_state;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9cc31c6..b8b9859 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -669,7 +669,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  		vma->bind_vma(vma, to->rcs_state->cache_level, GLOBAL_BIND);
>  	}
>  
> -	if (!to->is_initialized || i915_gem_context_is_default(to))
> +	if (!to->rcs_is_initialized || i915_gem_context_is_default(to))
>  		hw_flags |= MI_RESTORE_INHIBIT;
>  
>  	ret = mi_set_context(ring, to, hw_flags);
> @@ -716,13 +716,13 @@ done:
>  	i915_gem_context_reference(to);
>  	ring->last_context = to;
>  
> -	if (ring->id == RCS && !to->is_initialized && from == NULL) {
> +	if (ring->id == RCS && !to->rcs_is_initialized && from == NULL) {
>  		ret = i915_gem_render_state_init(ring);
>  		if (ret)
>  			DRM_ERROR("init render state: %d\n", ret);
>  	}
>  
> -	to->is_initialized = true;
> +	to->rcs_is_initialized = true;
>  
>  	return 0;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/8] drm/i915: Rename ctx->is_initialized to ctx->rcs_is_initialized

[Chris Wilson]

On Thu, Jun 26, 2014 at 02:24:14PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> We only use this flag to signify that the render state (a.k.a. golden
> context, a.k.a. null context) has been initialized. It doesn't mean
> anything for the other engines, so make that distinction obvious.
> This renaming was suggested by Daniel Vetter.
> 
> Implemented with this cocci script (plus manual changes to the struct
> declaration):
> 
> 	@
> 	struct intel_context c;
> 	@@
> 	- (c).is_initialized
> 	+ c.rcs_is_initialized
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->is_initialized
> 	+ c->rcs_is_initialized
> 
> No functional changes.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Ugh. This works better with the rearrangement, and
s/rcs_is_initialized/rcs_initialised/
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/8] drm/i915: Rename ctx->is_initialized to ctx->rcs_is_initialized

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 03, 2014 10:49 AM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915: Rename ctx->is_initialized to
> ctx->rcs_is_initialized
> 
> On Thu, Jun 26, 2014 at 02:24:14PM +0100, oscar.mateo@intel.com wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> >
> > We only use this flag to signify that the render state (a.k.a. golden
> > context, a.k.a. null context) has been initialized. It doesn't mean
> > anything for the other engines, so make that distinction obvious.
> > This renaming was suggested by Daniel Vetter.
> >
> > Implemented with this cocci script (plus manual changes to the struct
> > declaration):
> >
> > 	@
> > 	struct intel_context c;
> > 	@@
> > 	- (c).is_initialized
> > 	+ c.rcs_is_initialized
> >
> > 	@@
> > 	struct intel_context *c;
> > 	@@
> > 	- (c)->is_initialized
> > 	+ c->rcs_is_initialized
> >
> > No functional changes.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> Ugh. This works better with the rearrangement, and
> s/rcs_is_initialized/rcs_initialised/

ACK to the renaming. Re the rearrangement, I replied in the previous conservation.

[PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

This is an Execlists preparatory patch, since they make context ID become an
overloaded term:

- In the software, it was used to distinguish which context userspace was
  trying to use.
- In the BSpec, the term is used to describe the 20-bits long field the
  hardware uses to it to discriminate the contexts that are submitted to
  the ELSP and inform the driver about their current status (via Context
  Switch Interrupts and Context Status Buffers).

Initially, I tried to make the different meanings converge, but it proved
impossible:

- The software ctx->id is per-filp, while the hardware one needs to be
  globally unique.
- Also, we multiplex several backing states objects per intel_context,
  and all of them need unique HW IDs.
- I tried adding a per-filp ID and then composing the HW context ID as:
  ctx->id + file_priv->id + ring->id, but the fact that the hardware only
  uses 20-bits means we have to artificially limit the number of filps or
  contexts the userspace can create.

The ctx->handle bits are done with this Cocci patch (plus manual frobbing
of the struct declaration):

	@@
	struct intel_context c;
	@@
	- (c).id
	+ c.handle

	@@
	struct intel_context *c;
	@@
	- (c)->id
	+ c->handle

Also, while we are at it, s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
 drivers/gpu/drm/i915/i915_drv.h            |  6 +++---
 drivers/gpu/drm/i915/i915_gem_context.c    | 12 ++++++------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d4b8391..7484e22 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
 	if (i915_gem_context_is_default(ctx))
 		seq_puts(m, "  default context:\n");
 	else
-		seq_printf(m, "  context %d:\n", ctx->id);
+		seq_printf(m, "  context %d:\n", ctx->handle);
 	ppgtt->debug_dump(ppgtt, m);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 122e942..5d2b6ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -584,10 +584,10 @@ struct i915_ctx_hang_stats {
 };
 
 /* This must match up with the value previously used for execbuf2.rsvd1. */
-#define DEFAULT_CONTEXT_ID 0
+#define DEFAULT_CONTEXT_HANDLE 0
 struct intel_context {
 	struct kref ref;
-	int id;
+	int handle;
 	bool rcs_is_initialized;
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
@@ -2458,7 +2458,7 @@ static inline void i915_gem_context_unreference(struct intel_context *ctx)
 
 static inline bool i915_gem_context_is_default(const struct intel_context *c)
 {
-	return c->id == DEFAULT_CONTEXT_ID;
+	return c->handle == DEFAULT_CONTEXT_HANDLE;
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index b8b9859..75e903f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
 	/* Default context will never have a file_priv */
 	if (file_priv != NULL) {
 		ret = idr_alloc(&file_priv->context_idr, ctx,
-				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
+				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
 		if (ret < 0)
 			goto err_out;
 	} else
-		ret = DEFAULT_CONTEXT_ID;
+		ret = DEFAULT_CONTEXT_HANDLE;
 
 	ctx->file_priv = file_priv;
-	ctx->id = ret;
+	ctx->handle = ret;
 	/* NB: Mark all slices as needing a remap so that when the context first
 	 * loads it will restore whatever remap state already exists. If there
 	 * is no remap info, it will be a NOP. */
@@ -787,7 +787,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
-	args->ctx_id = ctx->id;
+	args->ctx_id = ctx->handle;
 	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
 
 	return 0;
@@ -801,7 +801,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 	struct intel_context *ctx;
 	int ret;
 
-	if (args->ctx_id == DEFAULT_CONTEXT_ID)
+	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
 		return -ENOENT;
 
 	ret = i915_mutex_lock_interruptible(dev);
@@ -814,7 +814,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return PTR_ERR(ctx);
 	}
 
-	idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	idr_remove(&ctx->file_priv->context_idr, ctx->handle);
 	i915_gem_context_unreference(ctx);
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d815ef5..c97178e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -938,7 +938,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
 	struct intel_context *ctx = NULL;
 	struct i915_ctx_hang_stats *hs;
 
-	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_ID)
+	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
 		return ERR_PTR(-EINVAL);
 
 	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 29145df..e0f0843 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1010,7 +1010,7 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
 	if (args->flags || args->pad)
 		return -EINVAL;
 
-	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
+	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
-- 
1.9.0

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:15 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is an Execlists preparatory patch, since they make context ID become an
> overloaded term:
> 
> - In the software, it was used to distinguish which context userspace was
>   trying to use.
> - In the BSpec, the term is used to describe the 20-bits long field the
>   hardware uses to it to discriminate the contexts that are submitted to
>   the ELSP and inform the driver about their current status (via Context
>   Switch Interrupts and Context Status Buffers).
> 
> Initially, I tried to make the different meanings converge, but it proved
> impossible:
> 
> - The software ctx->id is per-filp, while the hardware one needs to be
>   globally unique.
> - Also, we multiplex several backing states objects per intel_context,
>   and all of them need unique HW IDs.
> - I tried adding a per-filp ID and then composing the HW context ID as:
>   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
>   uses 20-bits means we have to artificially limit the number of filps or
>   contexts the userspace can create.
> 
> The ctx->handle bits are done with this Cocci patch (plus manual frobbing
> of the struct declaration):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).id
> 	+ c.handle
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->id
> 	+ c->handle
> 
> Also, while we are at it, s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  6 +++---
>  drivers/gpu/drm/i915/i915_gem_context.c    | 12 ++++++------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d4b8391..7484e22 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
>  	if (i915_gem_context_is_default(ctx))
>  		seq_puts(m, "  default context:\n");
>  	else
> -		seq_printf(m, "  context %d:\n", ctx->id);
> +		seq_printf(m, "  context %d:\n", ctx->handle);
>  	ppgtt->debug_dump(ppgtt, m);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 122e942..5d2b6ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -584,10 +584,10 @@ struct i915_ctx_hang_stats {
>  };
>  
>  /* This must match up with the value previously used for execbuf2.rsvd1. */
> -#define DEFAULT_CONTEXT_ID 0
> +#define DEFAULT_CONTEXT_HANDLE 0
>  struct intel_context {
>  	struct kref ref;
> -	int id;
> +	int handle;
>  	bool rcs_is_initialized;
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
> @@ -2458,7 +2458,7 @@ static inline void i915_gem_context_unreference(struct intel_context *ctx)
>  
>  static inline bool i915_gem_context_is_default(const struct intel_context *c)
>  {
> -	return c->id == DEFAULT_CONTEXT_ID;
> +	return c->handle == DEFAULT_CONTEXT_HANDLE;
>  }
>  
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b8b9859..75e903f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
>  	/* Default context will never have a file_priv */
>  	if (file_priv != NULL) {
>  		ret = idr_alloc(&file_priv->context_idr, ctx,
> -				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> +				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
>  		if (ret < 0)
>  			goto err_out;
>  	} else
> -		ret = DEFAULT_CONTEXT_ID;
> +		ret = DEFAULT_CONTEXT_HANDLE;
>  
>  	ctx->file_priv = file_priv;
> -	ctx->id = ret;
> +	ctx->handle = ret;
>  	/* NB: Mark all slices as needing a remap so that when the context first
>  	 * loads it will restore whatever remap state already exists. If there
>  	 * is no remap info, it will be a NOP. */
> @@ -787,7 +787,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> -	args->ctx_id = ctx->id;
> +	args->ctx_id = ctx->handle;
>  	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
>  
>  	return 0;
> @@ -801,7 +801,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  	struct intel_context *ctx;
>  	int ret;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
>  		return -ENOENT;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> @@ -814,7 +814,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  		return PTR_ERR(ctx);
>  	}
>  
> -	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> +	idr_remove(&ctx->file_priv->context_idr, ctx->handle);
>  	i915_gem_context_unreference(ctx);
>  	mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..c97178e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -938,7 +938,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>  	struct intel_context *ctx = NULL;
>  	struct i915_ctx_hang_stats *hs;
>  
> -	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_ID)
> +	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
>  		return ERR_PTR(-EINVAL);
>  
>  	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 29145df..e0f0843 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1010,7 +1010,7 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
>  	if (args->flags || args->pad)
>  		return -EINVAL;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);

So this handle is just the sw tracking ID right?  Would be nice to have
a patch on top commenting that in the context struct (well commenting
the whole struct really).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> Sent: Monday, June 30, 2014 9:54 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
> 
> On Thu, 26 Jun 2014 14:24:15 +0100
> oscar.mateo@intel.com wrote:
> 
> > From: Oscar Mateo <oscar.mateo@intel.com>
> >
> > This is an Execlists preparatory patch, since they make context ID
> > become an overloaded term:
> >
> > - In the software, it was used to distinguish which context userspace was
> >   trying to use.
> > - In the BSpec, the term is used to describe the 20-bits long field the
> >   hardware uses to it to discriminate the contexts that are submitted to
> >   the ELSP and inform the driver about their current status (via Context
> >   Switch Interrupts and Context Status Buffers).
> >
> > Initially, I tried to make the different meanings converge, but it
> > proved
> > impossible:
> >
> > - The software ctx->id is per-filp, while the hardware one needs to be
> >   globally unique.
> > - Also, we multiplex several backing states objects per intel_context,
> >   and all of them need unique HW IDs.
> > - I tried adding a per-filp ID and then composing the HW context ID as:
> >   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
> >   uses 20-bits means we have to artificially limit the number of filps or
> >   contexts the userspace can create.
> >
> > The ctx->handle bits are done with this Cocci patch (plus manual
> > frobbing of the struct declaration):
> >
> > 	@@
> > 	struct intel_context c;
> > 	@@
> > 	- (c).id
> > 	+ c.handle
> >
> > 	@@
> > 	struct intel_context *c;
> > 	@@
> > 	- (c)->id
> > 	+ c->handle
> >
> > Also, while we are at it,
> s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h            |  6 +++---
> >  drivers/gpu/drm/i915/i915_gem_context.c    | 12 ++++++------
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
> >  drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
> >  5 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index d4b8391..7484e22 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void
> *data)
> >  	if (i915_gem_context_is_default(ctx))
> >  		seq_puts(m, "  default context:\n");
> >  	else
> > -		seq_printf(m, "  context %d:\n", ctx->id);
> > +		seq_printf(m, "  context %d:\n", ctx->handle);
> >  	ppgtt->debug_dump(ppgtt, m);
> >
> >  	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 122e942..5d2b6ab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -584,10 +584,10 @@ struct i915_ctx_hang_stats {  };
> >
> >  /* This must match up with the value previously used for
> > execbuf2.rsvd1. */ -#define DEFAULT_CONTEXT_ID 0
> > +#define DEFAULT_CONTEXT_HANDLE 0
> >  struct intel_context {
> >  	struct kref ref;
> > -	int id;
> > +	int handle;
> >  	bool rcs_is_initialized;
> >  	uint8_t remap_slice;
> >  	struct drm_i915_file_private *file_priv; @@ -2458,7 +2458,7 @@
> > static inline void i915_gem_context_unreference(struct intel_context
> > *ctx)
> >
> >  static inline bool i915_gem_context_is_default(const struct
> > intel_context *c)  {
> > -	return c->id == DEFAULT_CONTEXT_ID;
> > +	return c->handle == DEFAULT_CONTEXT_HANDLE;
> >  }
> >
> >  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index b8b9859..75e903f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
> >  	/* Default context will never have a file_priv */
> >  	if (file_priv != NULL) {
> >  		ret = idr_alloc(&file_priv->context_idr, ctx,
> > -				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> > +				DEFAULT_CONTEXT_HANDLE, 0,
> GFP_KERNEL);
> >  		if (ret < 0)
> >  			goto err_out;
> >  	} else
> > -		ret = DEFAULT_CONTEXT_ID;
> > +		ret = DEFAULT_CONTEXT_HANDLE;
> >
> >  	ctx->file_priv = file_priv;
> > -	ctx->id = ret;
> > +	ctx->handle = ret;
> >  	/* NB: Mark all slices as needing a remap so that when the context
> first
> >  	 * loads it will restore whatever remap state already exists. If there
> >  	 * is no remap info, it will be a NOP. */ @@ -787,7 +787,7 @@ int
> > i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >  	if (IS_ERR(ctx))
> >  		return PTR_ERR(ctx);
> >
> > -	args->ctx_id = ctx->id;
> > +	args->ctx_id = ctx->handle;
> >  	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
> >
> >  	return 0;
> > @@ -801,7 +801,7 @@ int i915_gem_context_destroy_ioctl(struct
> drm_device *dev, void *data,
> >  	struct intel_context *ctx;
> >  	int ret;
> >
> > -	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> > +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
> >  		return -ENOENT;
> >
> >  	ret = i915_mutex_lock_interruptible(dev);
> > @@ -814,7 +814,7 @@ int i915_gem_context_destroy_ioctl(struct
> drm_device *dev, void *data,
> >  		return PTR_ERR(ctx);
> >  	}
> >
> > -	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> > +	idr_remove(&ctx->file_priv->context_idr, ctx->handle);
> >  	i915_gem_context_unreference(ctx);
> >  	mutex_unlock(&dev->struct_mutex);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index d815ef5..c97178e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -938,7 +938,7 @@ i915_gem_validate_context(struct drm_device
> *dev, struct drm_file *file,
> >  	struct intel_context *ctx = NULL;
> >  	struct i915_ctx_hang_stats *hs;
> >
> > -	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_ID)
> > +	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
> >  		return ERR_PTR(-EINVAL);
> >
> >  	ctx = i915_gem_context_get(file->driver_priv, ctx_id); diff --git
> > a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 29145df..e0f0843 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1010,7 +1010,7 @@ int i915_get_reset_stats_ioctl(struct drm_device
> *dev,
> >  	if (args->flags || args->pad)
> >  		return -EINVAL;
> >
> > -	if (args->ctx_id == DEFAULT_CONTEXT_ID &&
> !capable(CAP_SYS_ADMIN))
> > +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE &&
> > +!capable(CAP_SYS_ADMIN))
> >  		return -EPERM;
> >
> >  	ret = mutex_lock_interruptible(&dev->struct_mutex);
> 
> So this handle is just the sw tracking ID right?  Would be nice to have a patch
> on top commenting that in the context struct (well commenting the whole
> struct really).

No problem, I can do that as a follow up. How does this work? should these be kerneldoc or good ol´ code comments?

BTW: if this patch is still contended, its merging can be postponed with the rest of Execlists (the other 7 are not dependant on this one).

-- Oscar

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Jesse Barnes]

On Tue, 1 Jul 2014 15:46:51 +0000
"Mateo Lozano, Oscar" <oscar.mateo@intel.com> wrote:

> > -----Original Message-----
> > From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> > Sent: Monday, June 30, 2014 9:54 PM
> > To: Mateo Lozano, Oscar
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
> > 
> > On Thu, 26 Jun 2014 14:24:15 +0100
> > oscar.mateo@intel.com wrote:
> > 
> > > From: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > This is an Execlists preparatory patch, since they make context ID
> > > become an overloaded term:
> > >
> > > - In the software, it was used to distinguish which context userspace was
> > >   trying to use.
> > > - In the BSpec, the term is used to describe the 20-bits long field the
> > >   hardware uses to it to discriminate the contexts that are submitted to
> > >   the ELSP and inform the driver about their current status (via Context
> > >   Switch Interrupts and Context Status Buffers).
> > >
> > > Initially, I tried to make the different meanings converge, but it
> > > proved
> > > impossible:
> > >
> > > - The software ctx->id is per-filp, while the hardware one needs to be
> > >   globally unique.
> > > - Also, we multiplex several backing states objects per intel_context,
> > >   and all of them need unique HW IDs.
> > > - I tried adding a per-filp ID and then composing the HW context ID as:
> > >   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
> > >   uses 20-bits means we have to artificially limit the number of filps or
> > >   contexts the userspace can create.
> > >
> > > The ctx->handle bits are done with this Cocci patch (plus manual
> > > frobbing of the struct declaration):
> > >
> > > 	@@
> > > 	struct intel_context c;
> > > 	@@
> > > 	- (c).id
> > > 	+ c.handle
> > >
> > > 	@@
> > > 	struct intel_context *c;
> > > 	@@
> > > 	- (c)->id
> > > 	+ c->handle
> > >
> > > Also, while we are at it,
> > s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
> > >  drivers/gpu/drm/i915/i915_drv.h            |  6 +++---
> > >  drivers/gpu/drm/i915/i915_gem_context.c    | 12 ++++++------
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
> > >  5 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index d4b8391..7484e22 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void
> > *data)
> > >  	if (i915_gem_context_is_default(ctx))
> > >  		seq_puts(m, "  default context:\n");
> > >  	else
> > > -		seq_printf(m, "  context %d:\n", ctx->id);
> > > +		seq_printf(m, "  context %d:\n", ctx->handle);
> > >  	ppgtt->debug_dump(ppgtt, m);
> > >
> > >  	return 0;
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h index 122e942..5d2b6ab 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -584,10 +584,10 @@ struct i915_ctx_hang_stats {  };
> > >
> > >  /* This must match up with the value previously used for
> > > execbuf2.rsvd1. */ -#define DEFAULT_CONTEXT_ID 0
> > > +#define DEFAULT_CONTEXT_HANDLE 0
> > >  struct intel_context {
> > >  	struct kref ref;
> > > -	int id;
> > > +	int handle;
> > >  	bool rcs_is_initialized;
> > >  	uint8_t remap_slice;
> > >  	struct drm_i915_file_private *file_priv; @@ -2458,7 +2458,7 @@
> > > static inline void i915_gem_context_unreference(struct intel_context
> > > *ctx)
> > >
> > >  static inline bool i915_gem_context_is_default(const struct
> > > intel_context *c)  {
> > > -	return c->id == DEFAULT_CONTEXT_ID;
> > > +	return c->handle == DEFAULT_CONTEXT_HANDLE;
> > >  }
> > >
> > >  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index b8b9859..75e903f 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
> > >  	/* Default context will never have a file_priv */
> > >  	if (file_priv != NULL) {
> > >  		ret = idr_alloc(&file_priv->context_idr, ctx,
> > > -				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> > > +				DEFAULT_CONTEXT_HANDLE, 0,
> > GFP_KERNEL);
> > >  		if (ret < 0)
> > >  			goto err_out;
> > >  	} else
> > > -		ret = DEFAULT_CONTEXT_ID;
> > > +		ret = DEFAULT_CONTEXT_HANDLE;
> > >
> > >  	ctx->file_priv = file_priv;
> > > -	ctx->id = ret;
> > > +	ctx->handle = ret;
> > >  	/* NB: Mark all slices as needing a remap so that when the context
> > first
> > >  	 * loads it will restore whatever remap state already exists. If there
> > >  	 * is no remap info, it will be a NOP. */ @@ -787,7 +787,7 @@ int
> > > i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > >  	if (IS_ERR(ctx))
> > >  		return PTR_ERR(ctx);
> > >
> > > -	args->ctx_id = ctx->id;
> > > +	args->ctx_id = ctx->handle;
> > >  	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
> > >
> > >  	return 0;
> > > @@ -801,7 +801,7 @@ int i915_gem_context_destroy_ioctl(struct
> > drm_device *dev, void *data,
> > >  	struct intel_context *ctx;
> > >  	int ret;
> > >
> > > -	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> > > +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
> > >  		return -ENOENT;
> > >
> > >  	ret = i915_mutex_lock_interruptible(dev);
> > > @@ -814,7 +814,7 @@ int i915_gem_context_destroy_ioctl(struct
> > drm_device *dev, void *data,
> > >  		return PTR_ERR(ctx);
> > >  	}
> > >
> > > -	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> > > +	idr_remove(&ctx->file_priv->context_idr, ctx->handle);
> > >  	i915_gem_context_unreference(ctx);
> > >  	mutex_unlock(&dev->struct_mutex);
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index d815ef5..c97178e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -938,7 +938,7 @@ i915_gem_validate_context(struct drm_device
> > *dev, struct drm_file *file,
> > >  	struct intel_context *ctx = NULL;
> > >  	struct i915_ctx_hang_stats *hs;
> > >
> > > -	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_ID)
> > > +	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
> > >  		return ERR_PTR(-EINVAL);
> > >
> > >  	ctx = i915_gem_context_get(file->driver_priv, ctx_id); diff --git
> > > a/drivers/gpu/drm/i915/intel_uncore.c
> > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 29145df..e0f0843 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1010,7 +1010,7 @@ int i915_get_reset_stats_ioctl(struct drm_device
> > *dev,
> > >  	if (args->flags || args->pad)
> > >  		return -EINVAL;
> > >
> > > -	if (args->ctx_id == DEFAULT_CONTEXT_ID &&
> > !capable(CAP_SYS_ADMIN))
> > > +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE &&
> > > +!capable(CAP_SYS_ADMIN))
> > >  		return -EPERM;
> > >
> > >  	ret = mutex_lock_interruptible(&dev->struct_mutex);
> > 
> > So this handle is just the sw tracking ID right?  Would be nice to have a patch
> > on top commenting that in the context struct (well commenting the whole
> > struct really).
> 
> No problem, I can do that as a follow up. How does this work? should these be kerneldoc or good ol´ code comments?
> 
> BTW: if this patch is still contended, its merging can be postponed with the rest of Execlists (the other 7 are not dependant on this one).

Yeah using kdoc is preferred, for structs i think that just means a /**
at the top with each field documented with @field: summary.  There are
lots of examples to steal.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Chris Wilson]

On Thu, Jun 26, 2014 at 02:24:15PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is an Execlists preparatory patch, since they make context ID become an
> overloaded term:
> 
> - In the software, it was used to distinguish which context userspace was
>   trying to use.
> - In the BSpec, the term is used to describe the 20-bits long field the
>   hardware uses to it to discriminate the contexts that are submitted to
>   the ELSP and inform the driver about their current status (via Context
>   Switch Interrupts and Context Status Buffers).
> 
> Initially, I tried to make the different meanings converge, but it proved
> impossible:
> 
> - The software ctx->id is per-filp, while the hardware one needs to be
>   globally unique.
> - Also, we multiplex several backing states objects per intel_context,
>   and all of them need unique HW IDs.
> - I tried adding a per-filp ID and then composing the HW context ID as:
>   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
>   uses 20-bits means we have to artificially limit the number of filps or
>   contexts the userspace can create.
> 
> The ctx->handle bits are done with this Cocci patch (plus manual frobbing
> of the struct declaration):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).id
> 	+ c.handle
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->id
> 	+ c->handle
> 
> Also, while we are at it, s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Let's go whole hog here and call it ctx->user_handle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Chris Wilson]

On Thu, Jul 03, 2014 at 10:30:52AM +0100, Chris Wilson wrote:
> On Thu, Jun 26, 2014 at 02:24:15PM +0100, oscar.mateo@intel.com wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> > 
> > This is an Execlists preparatory patch, since they make context ID become an
> > overloaded term:
> > 
> > - In the software, it was used to distinguish which context userspace was
> >   trying to use.
> > - In the BSpec, the term is used to describe the 20-bits long field the
> >   hardware uses to it to discriminate the contexts that are submitted to
> >   the ELSP and inform the driver about their current status (via Context
> >   Switch Interrupts and Context Status Buffers).
> > 
> > Initially, I tried to make the different meanings converge, but it proved
> > impossible:
> > 
> > - The software ctx->id is per-filp, while the hardware one needs to be
> >   globally unique.
> > - Also, we multiplex several backing states objects per intel_context,
> >   and all of them need unique HW IDs.
> > - I tried adding a per-filp ID and then composing the HW context ID as:
> >   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
> >   uses 20-bits means we have to artificially limit the number of filps or
> >   contexts the userspace can create.
> > 
> > The ctx->handle bits are done with this Cocci patch (plus manual frobbing
> > of the struct declaration):
> > 
> > 	@@
> > 	struct intel_context c;
> > 	@@
> > 	- (c).id
> > 	+ c.handle
> > 
> > 	@@
> > 	struct intel_context *c;
> > 	@@
> > 	- (c)->id
> > 	+ c->handle
> > 
> > Also, while we are at it, s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> Let's go whole hog here and call it ctx->user_handle.

And it's unsigned and only 32bits...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 03, 2014 10:53 AM
> To: Mateo Lozano, Oscar; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
> 
> On Thu, Jul 03, 2014 at 10:30:52AM +0100, Chris Wilson wrote:
> > On Thu, Jun 26, 2014 at 02:24:15PM +0100, oscar.mateo@intel.com
> wrote:
> > > From: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > This is an Execlists preparatory patch, since they make context ID
> > > become an overloaded term:
> > >
> > > - In the software, it was used to distinguish which context userspace was
> > >   trying to use.
> > > - In the BSpec, the term is used to describe the 20-bits long field the
> > >   hardware uses to it to discriminate the contexts that are submitted to
> > >   the ELSP and inform the driver about their current status (via Context
> > >   Switch Interrupts and Context Status Buffers).
> > >
> > > Initially, I tried to make the different meanings converge, but it
> > > proved
> > > impossible:
> > >
> > > - The software ctx->id is per-filp, while the hardware one needs to be
> > >   globally unique.
> > > - Also, we multiplex several backing states objects per intel_context,
> > >   and all of them need unique HW IDs.
> > > - I tried adding a per-filp ID and then composing the HW context ID as:
> > >   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
> > >   uses 20-bits means we have to artificially limit the number of filps or
> > >   contexts the userspace can create.
> > >
> > > The ctx->handle bits are done with this Cocci patch (plus manual
> > > frobbing of the struct declaration):
> > >
> > > 	@@
> > > 	struct intel_context c;
> > > 	@@
> > > 	- (c).id
> > > 	+ c.handle
> > >
> > > 	@@
> > > 	struct intel_context *c;
> > > 	@@
> > > 	- (c)->id
> > > 	+ c->handle
> > >
> > > Also, while we are at it,
> s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> >
> > Let's go whole hog here and call it ctx->user_handle.

ACK

> And it's unsigned and only 32bits...

ACK, I´ll change the type to unit32_t

Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Daniel Vetter]

On Thu, Jul 03, 2014 at 12:01:35PM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, July 03, 2014 10:53 AM
> > To: Mateo Lozano, Oscar; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
> > 
> > On Thu, Jul 03, 2014 at 10:30:52AM +0100, Chris Wilson wrote:
> > > On Thu, Jun 26, 2014 at 02:24:15PM +0100, oscar.mateo@intel.com
> > wrote:
> > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > >
> > > > This is an Execlists preparatory patch, since they make context ID
> > > > become an overloaded term:
> > > >
> > > > - In the software, it was used to distinguish which context userspace was
> > > >   trying to use.
> > > > - In the BSpec, the term is used to describe the 20-bits long field the
> > > >   hardware uses to it to discriminate the contexts that are submitted to
> > > >   the ELSP and inform the driver about their current status (via Context
> > > >   Switch Interrupts and Context Status Buffers).
> > > >
> > > > Initially, I tried to make the different meanings converge, but it
> > > > proved
> > > > impossible:
> > > >
> > > > - The software ctx->id is per-filp, while the hardware one needs to be
> > > >   globally unique.
> > > > - Also, we multiplex several backing states objects per intel_context,
> > > >   and all of them need unique HW IDs.
> > > > - I tried adding a per-filp ID and then composing the HW context ID as:
> > > >   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
> > > >   uses 20-bits means we have to artificially limit the number of filps or
> > > >   contexts the userspace can create.
> > > >
> > > > The ctx->handle bits are done with this Cocci patch (plus manual
> > > > frobbing of the struct declaration):
> > > >
> > > > 	@@
> > > > 	struct intel_context c;
> > > > 	@@
> > > > 	- (c).id
> > > > 	+ c.handle
> > > >
> > > > 	@@
> > > > 	struct intel_context *c;
> > > > 	@@
> > > > 	- (c)->id
> > > > 	+ c->handle
> > > >
> > > > Also, while we are at it,
> > s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> > > >
> > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > Let's go whole hog here and call it ctx->user_handle.
> 
> ACK
> 
> > And it's unsigned and only 32bits...
> 
> ACK, I´ll change the type to unit32_t

Aside when resending please pull in all the r-b tags from Jesse so that
lazy me has less hassle when merging this ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 5/8] drm/i915: Extract ringbuffer destroy & generalize alloc to take a ringbuf

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

More prep work: with Execlists, we are going to start creating a lot
of extra ringbuffers soon, so these functions are handy.

No functional changes.

v2: rename allocate/destroy_ring_buffer to alloc/destroy_ringbuffer_obj
because the name is more meaningful and to mirror a similar function in
the context world: i915_gem_alloc_context_obj(). Change suggested by Brad
Volkin.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2faef26..ffdb366 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1380,15 +1380,25 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
 	return 0;
 }
 
-static int allocate_ring_buffer(struct intel_engine_cs *ring)
+static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+{
+	if (!ringbuf->obj)
+		return;
+
+	iounmap(ringbuf->virtual_start);
+	i915_gem_object_ggtt_unpin(ringbuf->obj);
+	drm_gem_object_unreference(&ringbuf->obj->base);
+	ringbuf->obj = NULL;
+}
+
+static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
+				      struct intel_ringbuffer *ringbuf)
 {
-	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_ringbuffer *ringbuf = ring->buffer;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	if (intel_ring_initialized(ring))
+	if (ringbuf->obj)
 		return 0;
 
 	obj = NULL;
@@ -1460,7 +1470,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 			goto error;
 	}
 
-	ret = allocate_ring_buffer(ring);
+	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
 	if (ret) {
 		DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret);
 		goto error;
@@ -1501,11 +1511,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 	intel_stop_ring_buffer(ring);
 	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-	iounmap(ringbuf->virtual_start);
-
-	i915_gem_object_ggtt_unpin(ringbuf->obj);
-	drm_gem_object_unreference(&ringbuf->obj->base);
-	ringbuf->obj = NULL;
+	intel_destroy_ringbuffer_obj(ringbuf);
 	ring->preallocated_lazy_request = NULL;
 	ring->outstanding_lazy_seqno = 0;
 
-- 
1.9.0

Re: [PATCH 5/8] drm/i915: Extract ringbuffer destroy & generalize alloc to take a ringbuf

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:16 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> More prep work: with Execlists, we are going to start creating a lot
> of extra ringbuffers soon, so these functions are handy.
> 
> No functional changes.
> 
> v2: rename allocate/destroy_ring_buffer to alloc/destroy_ringbuffer_obj
> because the name is more meaningful and to mirror a similar function in
> the context world: i915_gem_alloc_context_obj(). Change suggested by Brad
> Volkin.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2faef26..ffdb366 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1380,15 +1380,25 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
>  	return 0;
>  }
>  
> -static int allocate_ring_buffer(struct intel_engine_cs *ring)
> +static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
> +{
> +	if (!ringbuf->obj)
> +		return;
> +
> +	iounmap(ringbuf->virtual_start);
> +	i915_gem_object_ggtt_unpin(ringbuf->obj);
> +	drm_gem_object_unreference(&ringbuf->obj->base);
> +	ringbuf->obj = NULL;
> +}
> +
> +static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
> +				      struct intel_ringbuffer *ringbuf)
>  {
> -	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> -	if (intel_ring_initialized(ring))
> +	if (ringbuf->obj)
>  		return 0;
>  
>  	obj = NULL;
> @@ -1460,7 +1470,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  			goto error;
>  	}
>  
> -	ret = allocate_ring_buffer(ring);
> +	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
>  	if (ret) {
>  		DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret);
>  		goto error;
> @@ -1501,11 +1511,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>  	intel_stop_ring_buffer(ring);
>  	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>  
> -	iounmap(ringbuf->virtual_start);
> -
> -	i915_gem_object_ggtt_unpin(ringbuf->obj);
> -	drm_gem_object_unreference(&ringbuf->obj->base);
> -	ringbuf->obj = NULL;
> +	intel_destroy_ringbuffer_obj(ringbuf);
>  	ring->preallocated_lazy_request = NULL;
>  	ring->outstanding_lazy_seqno = 0;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 6/8] drm/i915: Generalize ring_space to take a ringbuf

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

It's simple enough that it doesn't need to know anything about the
engine.

Trivial change.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ffdb366..405edec 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -48,9 +48,8 @@ static inline int __ring_space(int head, int tail, int size)
 	return space;
 }
 
-static inline int ring_space(struct intel_engine_cs *ring)
+static inline int ring_space(struct intel_ringbuffer *ringbuf)
 {
-	struct intel_ringbuffer *ringbuf = ring->buffer;
 	return __ring_space(ringbuf->head & HEAD_ADDR, ringbuf->tail, ringbuf->size);
 }
 
@@ -545,7 +544,7 @@ static int init_ring_common(struct intel_engine_cs *ring)
 	else {
 		ringbuf->head = I915_READ_HEAD(ring);
 		ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
-		ringbuf->space = ring_space(ring);
+		ringbuf->space = ring_space(ringbuf);
 		ringbuf->last_retired_head = -1;
 	}
 
@@ -1537,7 +1536,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 		ringbuf->head = ringbuf->last_retired_head;
 		ringbuf->last_retired_head = -1;
 
-		ringbuf->space = ring_space(ring);
+		ringbuf->space = ring_space(ringbuf);
 		if (ringbuf->space >= n)
 			return 0;
 	}
@@ -1560,7 +1559,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 	ringbuf->head = ringbuf->last_retired_head;
 	ringbuf->last_retired_head = -1;
 
-	ringbuf->space = ring_space(ring);
+	ringbuf->space = ring_space(ringbuf);
 	return 0;
 }
 
@@ -1589,7 +1588,7 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
 	trace_i915_ring_wait_begin(ring);
 	do {
 		ringbuf->head = I915_READ_HEAD(ring);
-		ringbuf->space = ring_space(ring);
+		ringbuf->space = ring_space(ringbuf);
 		if (ringbuf->space >= n) {
 			ret = 0;
 			break;
@@ -1641,7 +1640,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
 		iowrite32(MI_NOOP, virt++);
 
 	ringbuf->tail = 0;
-	ringbuf->space = ring_space(ring);
+	ringbuf->space = ring_space(ringbuf);
 
 	return 0;
 }
-- 
1.9.0

Re: [PATCH 6/8] drm/i915: Generalize ring_space to take a ringbuf

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:17 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> It's simple enough that it doesn't need to know anything about the
> engine.
> 
> Trivial change.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ffdb366..405edec 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -48,9 +48,8 @@ static inline int __ring_space(int head, int tail, int size)
>  	return space;
>  }
>  
> -static inline int ring_space(struct intel_engine_cs *ring)
> +static inline int ring_space(struct intel_ringbuffer *ringbuf)
>  {
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
>  	return __ring_space(ringbuf->head & HEAD_ADDR, ringbuf->tail, ringbuf->size);
>  }
>  
> @@ -545,7 +544,7 @@ static int init_ring_common(struct intel_engine_cs *ring)
>  	else {
>  		ringbuf->head = I915_READ_HEAD(ring);
>  		ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
> -		ringbuf->space = ring_space(ring);
> +		ringbuf->space = ring_space(ringbuf);
>  		ringbuf->last_retired_head = -1;
>  	}
>  
> @@ -1537,7 +1536,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>  		ringbuf->head = ringbuf->last_retired_head;
>  		ringbuf->last_retired_head = -1;
>  
> -		ringbuf->space = ring_space(ring);
> +		ringbuf->space = ring_space(ringbuf);
>  		if (ringbuf->space >= n)
>  			return 0;
>  	}
> @@ -1560,7 +1559,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>  	ringbuf->head = ringbuf->last_retired_head;
>  	ringbuf->last_retired_head = -1;
>  
> -	ringbuf->space = ring_space(ring);
> +	ringbuf->space = ring_space(ringbuf);
>  	return 0;
>  }
>  
> @@ -1589,7 +1588,7 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>  	trace_i915_ring_wait_begin(ring);
>  	do {
>  		ringbuf->head = I915_READ_HEAD(ring);
> -		ringbuf->space = ring_space(ring);
> +		ringbuf->space = ring_space(ringbuf);
>  		if (ringbuf->space >= n) {
>  			ret = 0;
>  			break;
> @@ -1641,7 +1640,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>  		iowrite32(MI_NOOP, virt++);
>  
>  	ringbuf->tail = 0;
> -	ringbuf->space = ring_space(ring);
> +	ringbuf->space = ring_space(ringbuf);
>  
>  	return 0;
>  }

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 7/8] drm/i915: Generalize intel_ring_get_tail to take a ringbuf

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Again, it's low-level enough to simply take a ringbuf and nothing
else.

Trivial change.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         | 4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f6d1238..ac7d50a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2330,7 +2330,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 	u32 request_ring_position, request_start;
 	int ret;
 
-	request_start = intel_ring_get_tail(ring);
+	request_start = intel_ring_get_tail(ring->buffer);
 	/*
 	 * Emit any outstanding flushes - execbuf can fail to emit the flush
 	 * after having emitted the batchbuffer command. Hence we need to fix
@@ -2351,7 +2351,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 	 * GPU processing the request, we never over-estimate the
 	 * position of the head.
 	 */
-	request_ring_position = intel_ring_get_tail(ring);
+	request_ring_position = intel_ring_get_tail(ring->buffer);
 
 	ret = ring->add_request(ring);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index e72017b..070568b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -318,9 +318,9 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev);
 u64 intel_ring_get_active_head(struct intel_engine_cs *ring);
 void intel_ring_setup_status_page(struct intel_engine_cs *ring);
 
-static inline u32 intel_ring_get_tail(struct intel_engine_cs *ring)
+static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
 {
-	return ring->buffer->tail;
+	return ringbuf->tail;
 }
 
 static inline u32 intel_ring_get_seqno(struct intel_engine_cs *ring)
-- 
1.9.0

Re: [PATCH 7/8] drm/i915: Generalize intel_ring_get_tail to take a ringbuf

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:18 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> Again, it's low-level enough to simply take a ringbuf and nothing
> else.
> 
> Trivial change.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 4 ++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f6d1238..ac7d50a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2330,7 +2330,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	u32 request_ring_position, request_start;
>  	int ret;
>  
> -	request_start = intel_ring_get_tail(ring);
> +	request_start = intel_ring_get_tail(ring->buffer);
>  	/*
>  	 * Emit any outstanding flushes - execbuf can fail to emit the flush
>  	 * after having emitted the batchbuffer command. Hence we need to fix
> @@ -2351,7 +2351,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	 * GPU processing the request, we never over-estimate the
>  	 * position of the head.
>  	 */
> -	request_ring_position = intel_ring_get_tail(ring);
> +	request_ring_position = intel_ring_get_tail(ring->buffer);
>  
>  	ret = ring->add_request(ring);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index e72017b..070568b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -318,9 +318,9 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev);
>  u64 intel_ring_get_active_head(struct intel_engine_cs *ring);
>  void intel_ring_setup_status_page(struct intel_engine_cs *ring);
>  
> -static inline u32 intel_ring_get_tail(struct intel_engine_cs *ring)
> +static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
>  {
> -	return ring->buffer->tail;
> +	return ringbuf->tail;
>  }
>  
>  static inline u32 intel_ring_get_seqno(struct intel_engine_cs *ring)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 8/8] drm/i915: Extract the actual workload submission mechanism from execbuffer

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

So that we isolate the legacy ringbuffer submission mechanism, which becomes
a good candidate to be abstracted away. This is prep-work for Execlists (which
will its own workload submission mechanism).

No functional changes.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 298 ++++++++++++++++-------------
 1 file changed, 162 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c97178e..60998fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1026,6 +1026,163 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
+static int
+legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
+			     struct intel_engine_cs *ring,
+			     struct intel_context *ctx,
+			     struct drm_i915_gem_execbuffer2 *args,
+			     struct list_head *vmas,
+			     struct drm_i915_gem_object *batch_obj,
+			     u64 exec_start, u32 flags)
+{
+	struct drm_clip_rect *cliprects = NULL;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 exec_len;
+	int instp_mode;
+	u32 instp_mask;
+	int i, ret = 0;
+
+	if (args->num_cliprects != 0) {
+		if (ring != &dev_priv->ring[RCS]) {
+			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
+			return -EINVAL;
+		}
+
+		if (INTEL_INFO(dev)->gen >= 5) {
+			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
+			return -EINVAL;
+		}
+
+		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
+			DRM_DEBUG("execbuf with %u cliprects\n",
+				  args->num_cliprects);
+			return -EINVAL;
+		}
+
+		cliprects = kcalloc(args->num_cliprects,
+				    sizeof(*cliprects),
+				    GFP_KERNEL);
+		if (cliprects == NULL) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		if (copy_from_user(cliprects,
+				   to_user_ptr(args->cliprects_ptr),
+				   sizeof(*cliprects)*args->num_cliprects)) {
+			ret = -EFAULT;
+			goto error;
+		}
+	} else {
+		if (args->DR4 == 0xffffffff) {
+			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
+			args->DR4 = 0;
+		}
+
+		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
+			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
+			return -EINVAL;
+		}
+	}
+
+	ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
+	if (ret)
+		goto error;
+
+	ret = i915_switch_context(ring, ctx);
+	if (ret)
+		goto error;
+
+	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	instp_mask = I915_EXEC_CONSTANTS_MASK;
+	switch (instp_mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
+			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		if (instp_mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4) {
+				DRM_DEBUG("no rel constants on pre-gen4\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		}
+		break;
+	default:
+		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
+		ret = -EINVAL;
+		goto error;
+	}
+
+	if (ring == &dev_priv->ring[RCS] &&
+			instp_mode != dev_priv->relative_constants_mode) {
+		ret = intel_ring_begin(ring, 4);
+		if (ret)
+			goto error;
+
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+		intel_ring_emit(ring, INSTPM);
+		intel_ring_emit(ring, instp_mask << 16 | instp_mode);
+		intel_ring_advance(ring);
+
+		dev_priv->relative_constants_mode = instp_mode;
+	}
+
+	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
+		ret = i915_reset_gen7_sol_offsets(dev, ring);
+		if (ret)
+			goto error;
+	}
+
+	exec_len = args->batch_len;
+	if (cliprects) {
+		for (i = 0; i < args->num_cliprects; i++) {
+			ret = i915_emit_box(dev, &cliprects[i],
+					    args->DR1, args->DR4);
+			if (ret)
+				goto error;
+
+			ret = ring->dispatch_execbuffer(ring,
+							exec_start, exec_len,
+							flags);
+			if (ret)
+				goto error;
+		}
+	} else {
+		ret = ring->dispatch_execbuffer(ring,
+						exec_start, exec_len,
+						flags);
+		if (ret)
+			return ret;
+	}
+
+	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
+
+	i915_gem_execbuffer_move_to_active(vmas, ring);
+	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
+
+error:
+	kfree(cliprects);
+	return ret;
+}
+
 /**
  * Find one BSD ring to dispatch the corresponding BSD command.
  * The Ring ID is returned.
@@ -1085,14 +1242,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct eb_vmas *eb;
 	struct drm_i915_gem_object *batch_obj;
-	struct drm_clip_rect *cliprects = NULL;
 	struct intel_engine_cs *ring;
 	struct intel_context *ctx;
 	struct i915_address_space *vm;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
-	u64 exec_start = args->batch_start_offset, exec_len;
-	u32 mask, flags;
-	int ret, mode, i;
+	u64 exec_start = args->batch_start_offset;
+	u32 flags;
+	int ret;
 	bool need_relocs;
 
 	if (!i915_gem_check_execbuffer(args))
@@ -1136,87 +1292,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	mask = I915_EXEC_CONSTANTS_MASK;
-	switch (mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (mode != 0 && ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
-			return -EINVAL;
-		}
-
-		if (mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				return -EINVAL;
-			}
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				return -EINVAL;
-			}
-
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev)->gen >= 6)
-				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
-		}
-		break;
-	default:
-		DRM_DEBUG("execbuf with unknown constants: %d\n", mode);
-		return -EINVAL;
-	}
-
 	if (args->buffer_count < 1) {
 		DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
 		return -EINVAL;
 	}
 
-	if (args->num_cliprects != 0) {
-		if (ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
-			return -EINVAL;
-		}
-
-		if (INTEL_INFO(dev)->gen >= 5) {
-			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
-			return -EINVAL;
-		}
-
-		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
-			DRM_DEBUG("execbuf with %u cliprects\n",
-				  args->num_cliprects);
-			return -EINVAL;
-		}
-
-		cliprects = kcalloc(args->num_cliprects,
-				    sizeof(*cliprects),
-				    GFP_KERNEL);
-		if (cliprects == NULL) {
-			ret = -ENOMEM;
-			goto pre_mutex_err;
-		}
-
-		if (copy_from_user(cliprects,
-				   to_user_ptr(args->cliprects_ptr),
-				   sizeof(*cliprects)*args->num_cliprects)) {
-			ret = -EFAULT;
-			goto pre_mutex_err;
-		}
-	} else {
-		if (args->DR4 == 0xffffffff) {
-			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
-			args->DR4 = 0;
-		}
-
-		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
-			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
-			return -EINVAL;
-		}
-	}
-
 	intel_runtime_pm_get(dev_priv);
 
 	ret = i915_mutex_lock_interruptible(dev);
@@ -1320,63 +1400,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
-	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
+	ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
+			args, &eb->vmas, batch_obj, exec_start, flags);
 	if (ret)
 		goto err;
 
-	ret = i915_switch_context(ring, ctx);
-	if (ret)
-		goto err;
-
-	if (ring == &dev_priv->ring[RCS] &&
-	    mode != dev_priv->relative_constants_mode) {
-		ret = intel_ring_begin(ring, 4);
-		if (ret)
-				goto err;
-
-		intel_ring_emit(ring, MI_NOOP);
-		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-		intel_ring_emit(ring, INSTPM);
-		intel_ring_emit(ring, mask << 16 | mode);
-		intel_ring_advance(ring);
-
-		dev_priv->relative_constants_mode = mode;
-	}
-
-	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
-		ret = i915_reset_gen7_sol_offsets(dev, ring);
-		if (ret)
-			goto err;
-	}
-
-
-	exec_len = args->batch_len;
-	if (cliprects) {
-		for (i = 0; i < args->num_cliprects; i++) {
-			ret = i915_emit_box(dev, &cliprects[i],
-					    args->DR1, args->DR4);
-			if (ret)
-				goto err;
-
-			ret = ring->dispatch_execbuffer(ring,
-							exec_start, exec_len,
-							flags);
-			if (ret)
-				goto err;
-		}
-	} else {
-		ret = ring->dispatch_execbuffer(ring,
-						exec_start, exec_len,
-						flags);
-		if (ret)
-			goto err;
-	}
-
-	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
-
-	i915_gem_execbuffer_move_to_active(&eb->vmas, ring);
-	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
-
 err:
 	/* the request owns the ref now */
 	i915_gem_context_unreference(ctx);
@@ -1385,8 +1413,6 @@ err:
 	mutex_unlock(&dev->struct_mutex);
 
 pre_mutex_err:
-	kfree(cliprects);
-
 	/* intel_gpu_busy should also get a ref, so it will free when the device
 	 * is really idle. */
 	intel_runtime_pm_put(dev_priv);
-- 
1.9.0

Re: [PATCH 8/8] drm/i915: Extract the actual workload submission mechanism from execbuffer

[Jesse Barnes]

On Thu, 26 Jun 2014 14:24:19 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> So that we isolate the legacy ringbuffer submission mechanism, which becomes
> a good candidate to be abstracted away. This is prep-work for Execlists (which
> will its own workload submission mechanism).

"will have" ?

> 
> No functional changes.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 8/8] drm/i915: Extract the actual workload submission mechanism from execbuffer

[Chris Wilson]

On Thu, Jun 26, 2014 at 02:24:19PM +0100, oscar.mateo@intel.com wrote:
> +	i915_gem_execbuffer_move_to_active(vmas, ring);
> +	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);

This is where I start freaking out over the mix of global vs logical
state and the implications of reordering.

The key question for me is how clean busy-ioctl is when it has to pick
up the pieces from a partial failure to submit.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/8] drm/i915: Extract the actual workload submission mechanism from execbuffer

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 03, 2014 8:32 AM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Extract the actual workload
> submission mechanism from execbuffer
> 
> On Thu, Jun 26, 2014 at 02:24:19PM +0100, oscar.mateo@intel.com wrote:
> > +	i915_gem_execbuffer_move_to_active(vmas, ring);
> > +	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> 
> This is where I start freaking out over the mix of global vs logical state and
> the implications of reordering.

I hear you, Chris. This is scary stuff because it has a lot of implications, but it has been in review for a long time and we seem to be stalled.

> The key question for me is how clean busy-ioctl is when it has to pick up the
> pieces from a partial failure to submit.

AFAICT, __i915_add_request() needs some changes if we don´t want to reuse the intel_ringbuffer.c functionality (Daniel was wondering why I needed to abstract __i915_add_request away with another vfunc: this is the reason) and i915_gem_retire_requests_ring() needs to update last_retired_head in the correct ringbuf (global or logical). Other than that, everything seems healthy, as things like the list of active objects and the list of requests reside on intel_engine_cs, so they are common for both paths.

How could I put your mind at ease? maybe by creating a topic branch and going through QA for a while? or merging it as disabled by default? I don´t feel rewriting it forever is going to make it any better :(

Re: [PATCH 8/8] drm/i915: Extract the actual workload submission mechanism from execbuffer

[Chris Wilson]

On Thu, Jul 03, 2014 at 09:07:41AM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, July 03, 2014 8:32 AM
> > To: Mateo Lozano, Oscar
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Extract the actual workload
> > submission mechanism from execbuffer
> > 
> > On Thu, Jun 26, 2014 at 02:24:19PM +0100, oscar.mateo@intel.com wrote:
> > > +	i915_gem_execbuffer_move_to_active(vmas, ring);
> > > +	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> > 
> > This is where I start freaking out over the mix of global vs logical state and
> > the implications of reordering.
> 
> I hear you, Chris. This is scary stuff because it has a lot of implications, but it has been in review for a long time and we seem to be stalled.
> 
> > The key question for me is how clean busy-ioctl is when it has to pick up the
> > pieces from a partial failure to submit.
> 
> AFAICT, __i915_add_request() needs some changes if we don´t want to reuse the intel_ringbuffer.c functionality (Daniel was wondering why I needed to abstract __i915_add_request away with another vfunc: this is the reason) and i915_gem_retire_requests_ring() needs to update last_retired_head in the correct ringbuf (global or logical). Other than that, everything seems healthy, as things like the list of active objects and the list of requests reside on intel_engine_cs, so they are common for both paths.
> 
> How could I put your mind at ease? maybe by creating a topic branch and going through QA for a while? or merging it as disabled by default? I don´t feel rewriting it forever is going to make it any better :(

At the moment, I feel happy to merge as-is and then folding back in the new
functionality. The problem with relying solely on i-g-t here is that I
am concerned about the nasty sort of races that only appear on Linus's
machine. My feeling is that some of the more tricksy and problematic global
ring state is actually part of the request state.

I am feeling more confident that we can make intel_context our view of
the hardware state and hang the legacy plumbing off it, and that can be
done afterwards. Though it looks more and more like we need to make the
request as the central object through which we program the hardware.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre