[PATCH] drm/i915: Add null state batch to active list

[PATCH] drm/i915: Add null state batch to active list

[Mika Kuoppala]

for proper refcounting to take place as we use
i915_add_request() for it.

i915_add_request() also takes the context for the request
from ring->last_context so move the null state batch
submission after the ring context has been set.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |    3 +++
 drivers/gpu/drm/i915/i915_gem.c              |    4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_render_state.c |    7 +++++--
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b90ec69..6881f70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2204,6 +2204,9 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
 int i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			 struct intel_ring_buffer *to);
+void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
+				    struct intel_ring_buffer *ring);
+void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj);
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct intel_ring_buffer *ring);
 int i915_gem_dumb_create(struct drm_file *file_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 440979f..d795800 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2038,7 +2038,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static void
+void
 i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 			       struct intel_ring_buffer *ring)
 {
@@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 	return i915_gem_object_move_to_active(vma->obj, ring);
 }
 
-static void
+void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f220c94..d3aad5b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->obj);
 		i915_gem_context_unreference(from);
-	} else {
-		if (to->is_initialized == false) {
-			ret = i915_gem_render_state_init(ring);
-			if (ret)
-				DRM_ERROR("init render state: %d\n", ret);
-		}
 	}
 
-	to->is_initialized = true;
-
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 	to->last_ring = ring;
 
+	if (to->is_initialized == false && from == NULL) {
+		ret = i915_gem_render_state_init(ring);
+		if (ret)
+			DRM_ERROR("init render state: %d\n", ret);
+	}
+
+	to->is_initialized = true;
+
 	return 0;
 
 unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 392aa7b..d29e6b2 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -164,7 +164,6 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	const int gen = INTEL_INFO(ring->dev)->gen;
 	struct i915_render_state *so;
 	const struct intel_renderstate_rodata *rodata;
-	u32 seqno;
 	int ret;
 
 	rodata = render_state_get_rodata(ring->dev, gen);
@@ -186,7 +185,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	if (ret)
 		goto out;
 
-	ret = i915_add_request(ring, &seqno);
+	i915_gem_object_move_to_active(so->obj, ring);
+
+	ret = __i915_add_request(ring, NULL, so->obj, NULL);
+	if (ret)
+		i915_gem_object_move_to_inactive(so->obj);
 
 out:
 	render_state_free(so);
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Add null state batch to active list

[Ville Syrjälä]

On Wed, May 21, 2014 at 03:08:51PM +0300, Mika Kuoppala wrote:
> for proper refcounting to take place as we use
> i915_add_request() for it.
> 
> i915_add_request() also takes the context for the request
> from ring->last_context so move the null state batch
> submission after the ring context has been set.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h              |    3 +++
>  drivers/gpu/drm/i915/i915_gem.c              |    4 ++--
>  drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
>  drivers/gpu/drm/i915/i915_gem_render_state.c |    7 +++++--
>  4 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b90ec69..6881f70 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2204,6 +2204,9 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>  int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
>  int i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  			 struct intel_ring_buffer *to);
> +void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> +				    struct intel_ring_buffer *ring);
> +void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj);
>  void i915_vma_move_to_active(struct i915_vma *vma,
>  			     struct intel_ring_buffer *ring);
>  int i915_gem_dumb_create(struct drm_file *file_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 440979f..d795800 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2038,7 +2038,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
>  	return 0;
>  }
>  
> -static void
> +void
>  i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
>  			       struct intel_ring_buffer *ring)
>  {
> @@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>  	return i915_gem_object_move_to_active(vma->obj, ring);
>  }
>  
> -static void
> +void
>  i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f220c94..d3aad5b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
>  		/* obj is kept alive until the next request by its active ref */
>  		i915_gem_object_ggtt_unpin(from->obj);
>  		i915_gem_context_unreference(from);
> -	} else {
> -		if (to->is_initialized == false) {
> -			ret = i915_gem_render_state_init(ring);
> -			if (ret)
> -				DRM_ERROR("init render state: %d\n", ret);
> -		}
>  	}
>  
> -	to->is_initialized = true;
> -
>  done:
>  	i915_gem_context_reference(to);
>  	ring->last_context = to;
>  	to->last_ring = ring;
>  
> +	if (to->is_initialized == false && from == NULL) {
> +		ret = i915_gem_render_state_init(ring);
> +		if (ret)
> +			DRM_ERROR("init render state: %d\n", ret);
> +	}

I think this will end badly on !RCS. So needs a ring check, and maybe
throw a WARN into i915_gem_render_state_init() if it gets called with
the wrong ring.

> +
> +	to->is_initialized = true;
> +
>  	return 0;
>  
>  unpin_out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 392aa7b..d29e6b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -164,7 +164,6 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	const int gen = INTEL_INFO(ring->dev)->gen;
>  	struct i915_render_state *so;
>  	const struct intel_renderstate_rodata *rodata;
> -	u32 seqno;
>  	int ret;
>  
>  	rodata = render_state_get_rodata(ring->dev, gen);
> @@ -186,7 +185,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	if (ret)
>  		goto out;
>  
> -	ret = i915_add_request(ring, &seqno);
> +	i915_gem_object_move_to_active(so->obj, ring);
> +
> +	ret = __i915_add_request(ring, NULL, so->obj, NULL);
> +	if (ret)
> +		i915_gem_object_move_to_inactive(so->obj);
>  
>  out:
>  	render_state_free(so);
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC

[PATCH v2] drm/i915: Add null state batch to active list

[Mika Kuoppala]

for proper refcounting to take place as we use
i915_add_request() for it.

i915_add_request() also takes the context for the request
from ring->last_context so move the null state batch
submission after the ring context has been set.

v2: we need to check for correct ring now (Ville Syrjälä)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |    3 +++
 drivers/gpu/drm/i915/i915_gem.c              |    4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_render_state.c |   10 ++++++++--
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b90ec69..6881f70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2204,6 +2204,9 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
 int i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			 struct intel_ring_buffer *to);
+void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
+				    struct intel_ring_buffer *ring);
+void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj);
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct intel_ring_buffer *ring);
 int i915_gem_dumb_create(struct drm_file *file_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 440979f..d795800 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2038,7 +2038,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static void
+void
 i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 			       struct intel_ring_buffer *ring)
 {
@@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 	return i915_gem_object_move_to_active(vma->obj, ring);
 }
 
-static void
+void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f220c94..6a2d847a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->obj);
 		i915_gem_context_unreference(from);
-	} else {
-		if (to->is_initialized == false) {
-			ret = i915_gem_render_state_init(ring);
-			if (ret)
-				DRM_ERROR("init render state: %d\n", ret);
-		}
 	}
 
-	to->is_initialized = true;
-
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 	to->last_ring = ring;
 
+	if (ring->id == RCS && !to->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;
+
 	return 0;
 
 unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 392aa7b..463fff1 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -164,9 +164,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	const int gen = INTEL_INFO(ring->dev)->gen;
 	struct i915_render_state *so;
 	const struct intel_renderstate_rodata *rodata;
-	u32 seqno;
 	int ret;
 
+	if (WARN_ON(ring->id != RCS))
+		return -ENOENT;
+
 	rodata = render_state_get_rodata(ring->dev, gen);
 	if (rodata == NULL)
 		return 0;
@@ -186,7 +188,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	if (ret)
 		goto out;
 
-	ret = i915_add_request(ring, &seqno);
+	i915_gem_object_move_to_active(so->obj, ring);
+
+	ret = __i915_add_request(ring, NULL, so->obj, NULL);
+	if (ret)
+		i915_gem_object_move_to_inactive(so->obj);
 
 out:
 	render_state_free(so);
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Add null state batch to active list

[Chris Wilson]

On Wed, May 21, 2014 at 03:08:51PM +0300, Mika Kuoppala wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 392aa7b..d29e6b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -164,7 +164,6 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	const int gen = INTEL_INFO(ring->dev)->gen;
>  	struct i915_render_state *so;
>  	const struct intel_renderstate_rodata *rodata;
> -	u32 seqno;
>  	int ret;
>  
>  	rodata = render_state_get_rodata(ring->dev, gen);
> @@ -186,7 +185,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	if (ret)
>  		goto out;
>  
> -	ret = i915_add_request(ring, &seqno);
> +	i915_gem_object_move_to_active(so->obj, ring);

You have the vma, use it rather than exposing this function.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3] drm/i915: Add null state batch to active list

[Mika Kuoppala]

for proper refcounting to take place as we use
i915_add_request() for it.

i915_add_request() also takes the context for the request
from ring->last_context so move the null state batch
submission after the ring context has been set.

v2: we need to check for correct ring now (Ville Syrjälä)
v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |    1 +
 drivers/gpu/drm/i915/i915_gem.c              |    2 +-
 drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_render_state.c |   17 +++++++++++++++--
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b90ec69..0bf7bfb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2206,6 +2206,7 @@ int i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			 struct intel_ring_buffer *to);
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct intel_ring_buffer *ring);
+void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj);
 int i915_gem_dumb_create(struct drm_file *file_priv,
 			 struct drm_device *dev,
 			 struct drm_mode_create_dumb *args);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 440979f..d9bf694 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 	return i915_gem_object_move_to_active(vma->obj, ring);
 }
 
-static void
+void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f220c94..6a2d847a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->obj);
 		i915_gem_context_unreference(from);
-	} else {
-		if (to->is_initialized == false) {
-			ret = i915_gem_render_state_init(ring);
-			if (ret)
-				DRM_ERROR("init render state: %d\n", ret);
-		}
 	}
 
-	to->is_initialized = true;
-
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 	to->last_ring = ring;
 
+	if (ring->id == RCS && !to->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;
+
 	return 0;
 
 unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 392aa7b..82abe1e 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -164,9 +164,12 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	const int gen = INTEL_INFO(ring->dev)->gen;
 	struct i915_render_state *so;
 	const struct intel_renderstate_rodata *rodata;
-	u32 seqno;
+	struct i915_vma *vma;
 	int ret;
 
+	if (WARN_ON(ring->id != RCS))
+		return -ENOENT;
+
 	rodata = render_state_get_rodata(ring->dev, gen);
 	if (rodata == NULL)
 		return 0;
@@ -186,7 +189,17 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	if (ret)
 		goto out;
 
-	ret = i915_add_request(ring, &seqno);
+	vma = i915_gem_obj_to_ggtt(so->obj);
+	if (vma == NULL) {
+		ret = -ENOSPC;
+		goto out;
+	}
+
+	i915_vma_move_to_active(vma, ring);
+
+	ret = __i915_add_request(ring, NULL, so->obj, NULL);
+	if (ret)
+		i915_gem_object_move_to_inactive(so->obj);
 
 out:
 	render_state_free(so);
-- 
1.7.9.5

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

Re: [PATCH v3] drm/i915: Add null state batch to active list

[Daniel Vetter]

On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote:
> for proper refcounting to take place as we use
> i915_add_request() for it.
> 
> i915_add_request() also takes the context for the request
> from ring->last_context so move the null state batch
> submission after the ring context has been set.
> 
> v2: we need to check for correct ring now (Ville Syrjälä)
> v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Merged with Ville's irc r-b, thanks for the quick fix.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h              |    1 +
>  drivers/gpu/drm/i915/i915_gem.c              |    2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
>  drivers/gpu/drm/i915/i915_gem_render_state.c |   17 +++++++++++++++--
>  4 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b90ec69..0bf7bfb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2206,6 +2206,7 @@ int i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  			 struct intel_ring_buffer *to);
>  void i915_vma_move_to_active(struct i915_vma *vma,
>  			     struct intel_ring_buffer *ring);
> +void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj);
>  int i915_gem_dumb_create(struct drm_file *file_priv,
>  			 struct drm_device *dev,
>  			 struct drm_mode_create_dumb *args);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 440979f..d9bf694 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>  	return i915_gem_object_move_to_active(vma->obj, ring);
>  }
>  
> -static void
> +void
>  i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f220c94..6a2d847a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
>  		/* obj is kept alive until the next request by its active ref */
>  		i915_gem_object_ggtt_unpin(from->obj);
>  		i915_gem_context_unreference(from);
> -	} else {
> -		if (to->is_initialized == false) {
> -			ret = i915_gem_render_state_init(ring);
> -			if (ret)
> -				DRM_ERROR("init render state: %d\n", ret);
> -		}
>  	}
>  
> -	to->is_initialized = true;
> -
>  done:
>  	i915_gem_context_reference(to);
>  	ring->last_context = to;
>  	to->last_ring = ring;
>  
> +	if (ring->id == RCS && !to->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;
> +
>  	return 0;
>  
>  unpin_out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 392aa7b..82abe1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -164,9 +164,12 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	const int gen = INTEL_INFO(ring->dev)->gen;
>  	struct i915_render_state *so;
>  	const struct intel_renderstate_rodata *rodata;
> -	u32 seqno;
> +	struct i915_vma *vma;
>  	int ret;
>  
> +	if (WARN_ON(ring->id != RCS))
> +		return -ENOENT;
> +
>  	rodata = render_state_get_rodata(ring->dev, gen);
>  	if (rodata == NULL)
>  		return 0;
> @@ -186,7 +189,17 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	if (ret)
>  		goto out;
>  
> -	ret = i915_add_request(ring, &seqno);
> +	vma = i915_gem_obj_to_ggtt(so->obj);
> +	if (vma == NULL) {
> +		ret = -ENOSPC;
> +		goto out;
> +	}
> +
> +	i915_vma_move_to_active(vma, ring);
> +
> +	ret = __i915_add_request(ring, NULL, so->obj, NULL);
> +	if (ret)
> +		i915_gem_object_move_to_inactive(so->obj);
>  
>  out:
>  	render_state_free(so);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3] drm/i915: Add null state batch to active list

[Chris Wilson]

On Wed, May 21, 2014 at 04:54:55PM +0200, Daniel Vetter wrote:
> On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote:
> > for proper refcounting to take place as we use
> > i915_add_request() for it.
> > 
> > i915_add_request() also takes the context for the request
> > from ring->last_context so move the null state batch
> > submission after the ring context has been set.
> > 
> > v2: we need to check for correct ring now (Ville Syrjälä)
> > v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Merged with Ville's irc r-b, thanks for the quick fix.

Pity, it still contains some code that I'd rather not start
cargo-culting.
> > -	ret = i915_add_request(ring, &seqno);
> > +	vma = i915_gem_obj_to_ggtt(so->obj);
> > +	if (vma == NULL) {
> > +		ret = -ENOSPC;
> > +		goto out;
> > +	}

We use the GGTT vma much earlier, so this is suspect.

> > +
> > +	i915_vma_move_to_active(vma, ring);
> > +
> > +	ret = __i915_add_request(ring, NULL, so->obj, NULL);
> > +	if (ret)
> > +		i915_gem_object_move_to_inactive(so->obj);

As is move-to-inactive here. The only way add-request can fail is via an
EIO, and that will have triggered the move-to-inactive already - which
should nicely catch a BUG or two.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3] drm/i915: Add null state batch to active list

[Daniel Vetter]

On Wed, May 21, 2014 at 04:00:40PM +0100, Chris Wilson wrote:
> On Wed, May 21, 2014 at 04:54:55PM +0200, Daniel Vetter wrote:
> > On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote:
> > > for proper refcounting to take place as we use
> > > i915_add_request() for it.
> > > 
> > > i915_add_request() also takes the context for the request
> > > from ring->last_context so move the null state batch
> > > submission after the ring context has been set.
> > > 
> > > v2: we need to check for correct ring now (Ville Syrjälä)
> > > v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Merged with Ville's irc r-b, thanks for the quick fix.
> 
> Pity, it still contains some code that I'd rather not start
> cargo-culting.

Using vmas more as first-class objects I support. Mika can you please
resend? I'll keep v3 for now to avoid blocking people.
-Daniel

> > > -	ret = i915_add_request(ring, &seqno);
> > > +	vma = i915_gem_obj_to_ggtt(so->obj);
> > > +	if (vma == NULL) {
> > > +		ret = -ENOSPC;
> > > +		goto out;
> > > +	}
> 
> We use the GGTT vma much earlier, so this is suspect.
> 
> > > +
> > > +	i915_vma_move_to_active(vma, ring);
> > > +
> > > +	ret = __i915_add_request(ring, NULL, so->obj, NULL);
> > > +	if (ret)
> > > +		i915_gem_object_move_to_inactive(so->obj);
> 
> As is move-to-inactive here. The only way add-request can fail is via an
> EIO, and that will have triggered the move-to-inactive already - which
> should nicely catch a BUG or two.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

[PATCH v4] drm/i915: Add null state batch to active list

[Mika Kuoppala]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

for proper refcounting to take place as we use
i915_add_request() for it.

i915_add_request() also takes the context for the request
from ring->last_context so move the null state batch
submission after the ring context has been set.

v2: we need to check for correct ring now (Ville Syrjälä)
v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)
v4: cargoculted vma/active/inactive error handling removed (Chris Wilson)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_render_state.c |    8 ++++++--
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f220c94..6a2d847a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->obj);
 		i915_gem_context_unreference(from);
-	} else {
-		if (to->is_initialized == false) {
-			ret = i915_gem_render_state_init(ring);
-			if (ret)
-				DRM_ERROR("init render state: %d\n", ret);
-		}
 	}
 
-	to->is_initialized = true;
-
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 	to->last_ring = ring;
 
+	if (ring->id == RCS && !to->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;
+
 	return 0;
 
 unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 392aa7b..cfbf6fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -164,9 +164,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	const int gen = INTEL_INFO(ring->dev)->gen;
 	struct i915_render_state *so;
 	const struct intel_renderstate_rodata *rodata;
-	u32 seqno;
 	int ret;
 
+	if (WARN_ON(ring->id != RCS))
+		return -ENOENT;
+
 	rodata = render_state_get_rodata(ring->dev, gen);
 	if (rodata == NULL)
 		return 0;
@@ -186,8 +188,10 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
 	if (ret)
 		goto out;
 
-	ret = i915_add_request(ring, &seqno);
+	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so->obj), ring);
 
+	ret = __i915_add_request(ring, NULL, so->obj, NULL);
+	/* __i915_add_request moves object to inactive if it fails */
 out:
 	render_state_free(so);
 	return ret;
-- 
1.7.9.5

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

Re: [PATCH v4] drm/i915: Add null state batch to active list

[Daniel Vetter]

On Wed, May 21, 2014 at 07:01:06PM +0300, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> for proper refcounting to take place as we use
> i915_add_request() for it.
> 
> i915_add_request() also takes the context for the request
> from ring->last_context so move the null state batch
> submission after the ring context has been set.
> 
> v2: we need to check for correct ring now (Ville Syrjälä)
> v3: no need to expose i915_gem_move_object_to_active (Chris Wilson)
> v4: cargoculted vma/active/inactive error handling removed (Chris Wilson)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, merged instead of v3 with Chris' irc r-b.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c      |   16 ++++++++--------
>  drivers/gpu/drm/i915/i915_gem_render_state.c |    8 ++++++--
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f220c94..6a2d847a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring,
>  		/* obj is kept alive until the next request by its active ref */
>  		i915_gem_object_ggtt_unpin(from->obj);
>  		i915_gem_context_unreference(from);
> -	} else {
> -		if (to->is_initialized == false) {
> -			ret = i915_gem_render_state_init(ring);
> -			if (ret)
> -				DRM_ERROR("init render state: %d\n", ret);
> -		}
>  	}
>  
> -	to->is_initialized = true;
> -
>  done:
>  	i915_gem_context_reference(to);
>  	ring->last_context = to;
>  	to->last_ring = ring;
>  
> +	if (ring->id == RCS && !to->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;
> +
>  	return 0;
>  
>  unpin_out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 392aa7b..cfbf6fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -164,9 +164,11 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	const int gen = INTEL_INFO(ring->dev)->gen;
>  	struct i915_render_state *so;
>  	const struct intel_renderstate_rodata *rodata;
> -	u32 seqno;
>  	int ret;
>  
> +	if (WARN_ON(ring->id != RCS))
> +		return -ENOENT;
> +
>  	rodata = render_state_get_rodata(ring->dev, gen);
>  	if (rodata == NULL)
>  		return 0;
> @@ -186,8 +188,10 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring)
>  	if (ret)
>  		goto out;
>  
> -	ret = i915_add_request(ring, &seqno);
> +	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so->obj), ring);
>  
> +	ret = __i915_add_request(ring, NULL, so->obj, NULL);
> +	/* __i915_add_request moves object to inactive if it fails */
>  out:
>  	render_state_free(so);
>  	return ret;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3] drm/i915: Add null state batch to active list

[Volkin, Bradley D]

On Wed, May 21, 2014 at 07:02:56AM -0700, Mika Kuoppala wrote:
> +	if (ring->id == RCS && !to->is_initialized && from == NULL) {
> +		ret = i915_gem_render_state_init(ring);
> +		if (ret)
> +			DRM_ERROR("init render state: %d\n", ret);
> +	}

Apologies if this has already been discussed, but why do we have the
'from == NULL' check? Shouldn't we initialize all uninitialized RCS
contexts? Otherwise I thought we'll inherit whatever state 'from' left
behind.

The hw state should be valid in either case (and so I expect would fix
the rc6 issue either way), it's just the difference between initializing
every context to a specific valid state or initializing every context to
_some_ valid state. The commit message on the first render state patch
seemed to indicate the former while the implementation looks like the
latter. Just want to understand which we intended.

Thanks,
Brad

Re: [PATCH v3] drm/i915: Add null state batch to active list

[Mika Kuoppala]

"Volkin, Bradley D" <bradley.d.volkin@intel.com> writes:

> On Wed, May 21, 2014 at 07:02:56AM -0700, Mika Kuoppala wrote:
>> +	if (ring->id == RCS && !to->is_initialized && from == NULL) {
>> +		ret = i915_gem_render_state_init(ring);
>> +		if (ret)
>> +			DRM_ERROR("init render state: %d\n", ret);
>> +	}
>
> Apologies if this has already been discussed, but why do we have the
> 'from == NULL' check? Shouldn't we initialize all uninitialized RCS
> contexts? Otherwise I thought we'll inherit whatever state 'from' left
> behind.
>
> The hw state should be valid in either case (and so I expect would fix
> the rc6 issue either way), it's just the difference between initializing
> every context to a specific valid state or initializing every context to
> _some_ valid state. The commit message on the first render state patch
> seemed to indicate the former while the implementation looks like the
> latter. Just want to understand which we intended.

It seems that the intentions changed and I forgot to update the
commit message. For now this is just to push some state and hoping
that we can get in/out from rc6 without symptomps. 

The idea that we would restore/initialize to a specific (golden) state for
each new context makes me think that we would get rid of some transient 
bugs we are seeing. As how I understand things are now, app might
inherit some parts from previous ctx and then have lacking
initialization by itself and then see a hang..sometimes.

I guess the main opponent here is the performance implications.
And I lack the experience from user/application side to estimate the impact.

I hope that other devs with more experience on this topic will join
the discussion.

Thanks,
-Mika