[PATCH v2 0/6] A collection of cleanups

[PATCH v2 0/6] A collection of cleanups

[Dave Gordon]

Various things can go wrong during initialisation and teardown, but they
usually don't, so the error-handling paths go largely untested.  This
collection of patches fixes some things I recently noticed. Some might
lead to a kernel OOPS, but mostly they're leaks and other inconsistencies.

Includes Nick Hoath's patch to swap context/engine teardown, because
that helps make setup & teardown more consistent too.

v2:
    Patches reordered and the previous [1/4] split into [4..6/6],
    so that the bugs that it's fixing are more clearly visible
    (Mika Kuoppala, although he said it wasn't actually *necessary*).

GIT: [PATCH 1/6] drm/i915: fix context/engine cleanup order
GIT: [PATCH 2/6] drm/i915: tidy up initialisation failure paths (GEM &
GIT: [PATCH 3/6] drm/i915: tidy up initialisation failure paths (legacy)
GIT: [PATCH 4/6] drm/i915: unmap the correct page in
GIT: [PATCH 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown
GIT: [PATCH 6/6] drm/i915: consolidate LRC mode HWSP setup & teardown
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/6] drm/i915: fix context/engine cleanup order

[Dave Gordon]

From: Nick Hoath <nicholas.hoath@intel.com>

Swap the order of context & engine cleanup, so that contexts are cleaned
up first, and *then* engines. This is a more sensible order anyway, but
in particular has become necessary since the 'intel_ring_initialized()
must be simple and inline' patch, which now uses ring->dev as an
'initialised' flag, so it can now be NULL after engine teardown. This
in turn can cause a problem in the context code, which (used to) check
the ring->dev->struct_mutex -- causing a fault if ring->dev was NULL.

Also rename the cleanup function to reflect what it actually does
(cleanup engines, not a ringbuffer), and fix an annoying whitespace issue.

v2: Also make the fix in i915_load_modeset_init, not just in
    i915_driver_unload (Chris Wilson)
v3: Had extra stuff in it.
v4: Reverted extra stuff (so we're back to v2).
    Rebased and updated commentary above (Dave Gordon).

Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
Signed-off-by: David Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++-----------
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d70d96f..4725e8d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -451,8 +451,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_gem:
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
+	i915_gem_cleanup_engines(dev);
 	mutex_unlock(&dev->struct_mutex);
 cleanup_irq:
 	intel_guc_ucode_fini(dev);
@@ -1196,8 +1196,8 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_guc_ucode_fini(dev);
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
+	i915_gem_cleanup_engines(dev);
 	mutex_unlock(&dev->struct_mutex);
 	intel_fbc_cleanup_cfb(dev_priv);
 	i915_gem_cleanup_stolen(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 204661f..021e88a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3012,7 +3012,7 @@ int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
+void i915_gem_cleanup_engines(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 371bbb2..799a53a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4912,7 +4912,7 @@ int i915_gem_init_rings(struct drm_device *dev)
 		req = i915_gem_request_alloc(ring, NULL);
 		if (IS_ERR(req)) {
 			ret = PTR_ERR(req);
-			i915_gem_cleanup_ringbuffer(dev);
+			i915_gem_cleanup_engines(dev);
 			goto out;
 		}
 
@@ -4925,7 +4925,7 @@ int i915_gem_init_rings(struct drm_device *dev)
 		if (ret && ret != -EIO) {
 			DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
 			i915_gem_request_cancel(req);
-			i915_gem_cleanup_ringbuffer(dev);
+			i915_gem_cleanup_engines(dev);
 			goto out;
 		}
 
@@ -4933,7 +4933,7 @@ int i915_gem_init_rings(struct drm_device *dev)
 		if (ret && ret != -EIO) {
 			DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
 			i915_gem_request_cancel(req);
-			i915_gem_cleanup_ringbuffer(dev);
+			i915_gem_cleanup_engines(dev);
 			goto out;
 		}
 
@@ -5008,7 +5008,7 @@ int i915_gem_init(struct drm_device *dev)
 }
 
 void
-i915_gem_cleanup_ringbuffer(struct drm_device *dev)
+i915_gem_cleanup_engines(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring;
@@ -5017,13 +5017,14 @@ int i915_gem_init(struct drm_device *dev)
 	for_each_ring(ring, dev_priv, i)
 		dev_priv->gt.cleanup_ring(ring);
 
-    if (i915.enable_execlists)
-            /*
-             * Neither the BIOS, ourselves or any other kernel
-             * expects the system to be in execlists mode on startup,
-             * so we need to reset the GPU back to legacy mode.
-             */
-            intel_gpu_reset(dev);
+	if (i915.enable_execlists) {
+		/*
+		 * Neither the BIOS, ourselves or any other kernel
+		 * expects the system to be in execlists mode on startup,
+		 * so we need to reset the GPU back to legacy mode.
+		 */
+		intel_gpu_reset(dev);
+	}
 }
 
 static void
-- 
1.9.1

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

Re: [PATCH v2 1/6] drm/i915: fix context/engine cleanup order

[Daniel Vetter]

On Fri, Jan 22, 2016 at 11:10:06PM +0000, Dave Gordon wrote:
> From: Nick Hoath <nicholas.hoath@intel.com>
> 
> Swap the order of context & engine cleanup, so that contexts are cleaned
> up first, and *then* engines. This is a more sensible order anyway, but
> in particular has become necessary since the 'intel_ring_initialized()
> must be simple and inline' patch, which now uses ring->dev as an
> 'initialised' flag, so it can now be NULL after engine teardown. This
> in turn can cause a problem in the context code, which (used to) check
> the ring->dev->struct_mutex -- causing a fault if ring->dev was NULL.
> 
> Also rename the cleanup function to reflect what it actually does
> (cleanup engines, not a ringbuffer), and fix an annoying whitespace issue.
> 
> v2: Also make the fix in i915_load_modeset_init, not just in
>     i915_driver_unload (Chris Wilson)
> v3: Had extra stuff in it.
> v4: Reverted extra stuff (so we're back to v2).
>     Rebased and updated commentary above (Dave Gordon).
> 
> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> Signed-off-by: David Gordon <david.s.gordon@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++-----------
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index d70d96f..4725e8d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -451,8 +451,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_gem:
>  	mutex_lock(&dev->struct_mutex);
> -	i915_gem_cleanup_ringbuffer(dev);
>  	i915_gem_context_fini(dev);
> +	i915_gem_cleanup_engines(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  cleanup_irq:
>  	intel_guc_ucode_fini(dev);
> @@ -1196,8 +1196,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	intel_guc_ucode_fini(dev);
>  	mutex_lock(&dev->struct_mutex);
> -	i915_gem_cleanup_ringbuffer(dev);
>  	i915_gem_context_fini(dev);
> +	i915_gem_cleanup_engines(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  	intel_fbc_cleanup_cfb(dev_priv);
>  	i915_gem_cleanup_stolen(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 204661f..021e88a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3012,7 +3012,7 @@ int i915_gem_init_rings(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
>  void i915_gem_init_swizzling(struct drm_device *dev);
> -void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
> +void i915_gem_cleanup_engines(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
>  int __must_check i915_gem_suspend(struct drm_device *dev);
>  void __i915_add_request(struct drm_i915_gem_request *req,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 371bbb2..799a53a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4912,7 +4912,7 @@ int i915_gem_init_rings(struct drm_device *dev)
>  		req = i915_gem_request_alloc(ring, NULL);
>  		if (IS_ERR(req)) {
>  			ret = PTR_ERR(req);
> -			i915_gem_cleanup_ringbuffer(dev);
> +			i915_gem_cleanup_engines(dev);
>  			goto out;
>  		}
>  
> @@ -4925,7 +4925,7 @@ int i915_gem_init_rings(struct drm_device *dev)
>  		if (ret && ret != -EIO) {
>  			DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
>  			i915_gem_request_cancel(req);
> -			i915_gem_cleanup_ringbuffer(dev);
> +			i915_gem_cleanup_engines(dev);
>  			goto out;
>  		}
>  
> @@ -4933,7 +4933,7 @@ int i915_gem_init_rings(struct drm_device *dev)
>  		if (ret && ret != -EIO) {
>  			DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
>  			i915_gem_request_cancel(req);
> -			i915_gem_cleanup_ringbuffer(dev);
> +			i915_gem_cleanup_engines(dev);
>  			goto out;
>  		}
>  
> @@ -5008,7 +5008,7 @@ int i915_gem_init(struct drm_device *dev)
>  }
>  
>  void
> -i915_gem_cleanup_ringbuffer(struct drm_device *dev)
> +i915_gem_cleanup_engines(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *ring;
> @@ -5017,13 +5017,14 @@ int i915_gem_init(struct drm_device *dev)
>  	for_each_ring(ring, dev_priv, i)
>  		dev_priv->gt.cleanup_ring(ring);
>  
> -    if (i915.enable_execlists)
> -            /*
> -             * Neither the BIOS, ourselves or any other kernel
> -             * expects the system to be in execlists mode on startup,
> -             * so we need to reset the GPU back to legacy mode.
> -             */
> -            intel_gpu_reset(dev);
> +	if (i915.enable_execlists) {
> +		/*
> +		 * Neither the BIOS, ourselves or any other kernel
> +		 * expects the system to be in execlists mode on startup,
> +		 * so we need to reset the GPU back to legacy mode.
> +		 */
> +		intel_gpu_reset(dev);
> +	}
>  }
>  
>  static void
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Dave Gordon]

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

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

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

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

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

[PATCH v2 3/6] drm/i915: tidy up initialisation failure paths (legacy)

[Dave Gordon]

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 9030e2b..29de64e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -551,6 +551,8 @@ static bool stop_ring(struct intel_engine_cs *ring)
 		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
 	}
 
+	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
+
 	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
 }
 
@@ -2071,12 +2073,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 	return 0;
 }
 
-static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
-{
-	drm_gem_object_unreference(&ringbuf->obj->base);
-	ringbuf->obj = NULL;
-}
-
 static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 				      struct intel_ringbuffer *ringbuf)
 {
@@ -2139,11 +2135,14 @@ struct intel_ringbuffer *
 }
 
 void
-intel_ringbuffer_free(struct intel_ringbuffer *ring)
+intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)
 {
-	intel_destroy_ringbuffer_obj(ring);
-	list_del(&ring->link);
-	kfree(ring);
+	if (ringbuf->obj) {
+		drm_gem_object_unreference(&ringbuf->obj->base);
+		ringbuf->obj = NULL;
+	}
+	list_del(&ringbuf->link);
+	kfree(ringbuf);
 }
 
 static int intel_init_ring_buffer(struct drm_device *dev,
@@ -2171,6 +2170,13 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	}
 	ring->buffer = ringbuf;
 
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
+	if (ret) {
+		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
+				ring->name, ret);
+		goto error;
+	}
+
 	if (I915_NEED_GFX_HWS(dev)) {
 		ret = init_status_page(ring);
 		if (ret)
@@ -2182,14 +2188,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 			goto error;
 	}
 
-	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
-	if (ret) {
-		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
-				ring->name, ret);
-		intel_destroy_ringbuffer_obj(ringbuf);
-		goto error;
-	}
-
 	ret = i915_cmd_parser_init_ring(ring);
 	if (ret)
 		goto error;
@@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 
 void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv;
+	struct intel_ringbuffer *ringbuf;
 
 	if (!intel_ring_initialized(ring))
 		return;
 
-	dev_priv = to_i915(ring->dev);
-
-	if (ring->buffer) {
+	ringbuf = ring->buffer;
+	if (ringbuf) {
 		intel_stop_ring_buffer(ring);
-		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-		intel_unpin_ringbuffer_obj(ring->buffer);
-		intel_ringbuffer_free(ring->buffer);
+		if (ringbuf->virtual_start)
+			intel_unpin_ringbuffer_obj(ringbuf);
+		intel_ringbuffer_free(ringbuf);
 		ring->buffer = NULL;
 	}
 
-- 
1.9.1

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

Re: [PATCH v2 3/6] drm/i915: tidy up initialisation failure paths (legacy)

[Chris Wilson]

On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon wrote:
> 1. Fix intel_cleanup_ring_buffer() to handle the error cleanup case where
>    the ringbuffer has been allocated but map-and-pin failed.  Unpin it iff
>    it's previously been mapped-and-pinned.
> 
> 2. Fix the error path in intel_init_ring_buffer(), which already called
>    intel_destroy_ringbuffer_obj(), but failed to free the actual ringbuffer
>    structure. Calling intel_ringbuffer_free() instead does both in one go.
> 
> 3. With the above change, intel_destroy_ringbuffer_obj() is only called in
>    one place (intel_ringbuffer_free()), so flatten it into that function.
> 
> 4. move low-level register accesses from intel_cleanup_ring_buffer()
>    (which calls intel_stop_ring_buffer(ring) which calls stop_ring())
>    down into stop_ring() itself), which is already doing low-level
>    register accesses. Then, intel_cleanup_ring_buffer() no longer
>    needs 'dev_priv'.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 9030e2b..29de64e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -551,6 +551,8 @@ static bool stop_ring(struct intel_engine_cs *ring)
>  		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
>  	}
>  
> +	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
> +
>  	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
>  }
>  
> @@ -2071,12 +2073,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
> -{
> -	drm_gem_object_unreference(&ringbuf->obj->base);
> -	ringbuf->obj = NULL;
> -}
> -
>  static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>  				      struct intel_ringbuffer *ringbuf)
>  {
> @@ -2139,11 +2135,14 @@ struct intel_ringbuffer *
>  }
>  
>  void
> -intel_ringbuffer_free(struct intel_ringbuffer *ring)
> +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)

ringbuf?

> -	intel_destroy_ringbuffer_obj(ring);
> -	list_del(&ring->link);
> -	kfree(ring);
> +	if (ringbuf->obj) {
> +		drm_gem_object_unreference(&ringbuf->obj->base);

Already tests for NULL

> +		ringbuf->obj = NULL;

Redundant as we are aobut to free it.

> +	}
> +	list_del(&ringbuf->link);
> +	kfree(ringbuf);
>  }
>  
>  static int intel_init_ring_buffer(struct drm_device *dev,
> @@ -2171,6 +2170,13 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  	}
>  	ring->buffer = ringbuf;
>  
> +	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
> +	if (ret) {
> +		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
> +				ring->name, ret);
> +		goto error;
> +	}
> +
>  	if (I915_NEED_GFX_HWS(dev)) {
>  		ret = init_status_page(ring);
>  		if (ret)
> @@ -2182,14 +2188,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  			goto error;
>  	}
>  
> -	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
> -	if (ret) {
> -		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
> -				ring->name, ret);
> -		intel_destroy_ringbuffer_obj(ringbuf);
> -		goto error;
> -	}
> -
>  	ret = i915_cmd_parser_init_ring(ring);
>  	if (ret)
>  		goto error;
> @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>  {
> -	struct drm_i915_private *dev_priv;
> +	struct intel_ringbuffer *ringbuf;

ringbuf?

>  
>  	if (!intel_ring_initialized(ring))
>  		return;
>  
> -	dev_priv = to_i915(ring->dev);
> -
> -	if (ring->buffer) {
> +	ringbuf = ring->buffer;
> +	if (ringbuf) {
>  		intel_stop_ring_buffer(ring);
> -		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>  
> -		intel_unpin_ringbuffer_obj(ring->buffer);
> -		intel_ringbuffer_free(ring->buffer);
> +		if (ringbuf->virtual_start)

Cleaner code, and more idiomatic, if we let unpin early return.

> +			intel_unpin_ringbuffer_obj(ringbuf);
> +		intel_ringbuffer_free(ringbuf);
>  		ring->buffer = NULL;
>  	}
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 3/6] drm/i915: tidy up initialisation failure paths (legacy)

[Dave Gordon]

On 25/01/16 10:52, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon wrote:
>> 1. Fix intel_cleanup_ring_buffer() to handle the error cleanup case where
>>     the ringbuffer has been allocated but map-and-pin failed.  Unpin it iff
>>     it's previously been mapped-and-pinned.
>>
>> 2. Fix the error path in intel_init_ring_buffer(), which already called
>>     intel_destroy_ringbuffer_obj(), but failed to free the actual ringbuffer
>>     structure. Calling intel_ringbuffer_free() instead does both in one go.
>>
>> 3. With the above change, intel_destroy_ringbuffer_obj() is only called in
>>     one place (intel_ringbuffer_free()), so flatten it into that function.
>>
>> 4. move low-level register accesses from intel_cleanup_ring_buffer()
>>     (which calls intel_stop_ring_buffer(ring) which calls stop_ring())
>>     down into stop_ring() itself), which is already doing low-level
>>     register accesses. Then, intel_cleanup_ring_buffer() no longer
>>     needs 'dev_priv'.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------
>>   1 file changed, 22 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 9030e2b..29de64e 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -551,6 +551,8 @@ static bool stop_ring(struct intel_engine_cs *ring)
>>   		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
>>   	}
>>
>> +	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>> +
>>   	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
>>   }
>>
>> @@ -2071,12 +2073,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>>   	return 0;
>>   }
>>
>> -static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>> -{
>> -	drm_gem_object_unreference(&ringbuf->obj->base);
>> -	ringbuf->obj = NULL;
>> -}
>> -
>>   static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>>   				      struct intel_ringbuffer *ringbuf)
>>   {
>> @@ -2139,11 +2135,14 @@ struct intel_ringbuffer *
>>   }
>>
>>   void
>> -intel_ringbuffer_free(struct intel_ringbuffer *ring)
>> +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)
>
> ringbuf?

Yes, it's a ring*buffer*, so 'ringbuf' is a good name for it. It's NOT 
an *engine*, which is what the name 'ring' has historically been used 
for. I count 276 instances of 'ringbuf' in the driver; AFAICT, they all 
refer to a 'struct intel_ringbuffer'. Of the ~2000 instances of 'ring', 
they nearly (but not quite) all refer to a 'struct intel_engine_cs' :(

Someday we may want to call ringbuffers 'ring', but not until the last 
vestige of the earlier usage has been purged from the codebase and from 
people's heads. For now, the name 'ringbuf' will reduce confusion.

>> -	intel_destroy_ringbuffer_obj(ring);
>> -	list_del(&ring->link);
>> -	kfree(ring);
>> +	if (ringbuf->obj) {
>> +		drm_gem_object_unreference(&ringbuf->obj->base);
>
> Already tests for NULL

But there's no guarantee that ringbuf->obj == NULL implies that 
&ringbuf->obj->base == NULL. It may happen to be so at present, but 
that's an implementation detail, so I'm not assuming that it will always 
be so.

I suspect also that some static analysis tools would complain about this 
construct if it didn't have the test-before-use.

>> +		ringbuf->obj = NULL;
>
> Redundant as we are aobut to free it.

True, but it's harmless.

>> +	}
>> +	list_del(&ringbuf->link);
>> +	kfree(ringbuf);
>>   }
>>
>>   static int intel_init_ring_buffer(struct drm_device *dev,
>> @@ -2171,6 +2170,13 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>>   	}
>>   	ring->buffer = ringbuf;
>>
>> +	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
>> +				ring->name, ret);
>> +		goto error;
>> +	}
>> +
>>   	if (I915_NEED_GFX_HWS(dev)) {
>>   		ret = init_status_page(ring);
>>   		if (ret)
>> @@ -2182,14 +2188,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>>   			goto error;
>>   	}
>>
>> -	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
>> -	if (ret) {
>> -		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
>> -				ring->name, ret);
>> -		intel_destroy_ringbuffer_obj(ringbuf);
>> -		goto error;
>> -	}
>> -
>>   	ret = i915_cmd_parser_init_ring(ring);
>>   	if (ret)
>>   		goto error;
>> @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>>
>>   void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>>   {
>> -	struct drm_i915_private *dev_priv;
>> +	struct intel_ringbuffer *ringbuf;
>
> ringbuf?

See above. Also the name 'ring' here already refers to an engine.

>>   	if (!intel_ring_initialized(ring))
>>   		return;
>>
>> -	dev_priv = to_i915(ring->dev);
>> -
>> -	if (ring->buffer) {
>> +	ringbuf = ring->buffer;
>> +	if (ringbuf) {
>>   		intel_stop_ring_buffer(ring);
>> -		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>>
>> -		intel_unpin_ringbuffer_obj(ring->buffer);
>> -		intel_ringbuffer_free(ring->buffer);
>> +		if (ringbuf->virtual_start)
>
> Cleaner code, and more idiomatic, if we let unpin early return.

Maybe, but that's not the way it was previously written, so I didn't 
change it around.

>> +			intel_unpin_ringbuffer_obj(ringbuf);
>> +		intel_ringbuffer_free(ringbuf);
>>   		ring->buffer = NULL;
>>   	}
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH v2 3/6] drm/i915: tidy up initialisation failure paths (legacy)

[Daniel Vetter]

On Mon, Jan 25, 2016 at 12:08:11PM +0000, Dave Gordon wrote:
> On 25/01/16 10:52, Chris Wilson wrote:
> >On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon wrote:
> >>+	if (ringbuf) {
> >>  		intel_stop_ring_buffer(ring);
> >>-		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
> >>
> >>-		intel_unpin_ringbuffer_obj(ring->buffer);
> >>-		intel_ringbuffer_free(ring->buffer);
> >>+		if (ringbuf->virtual_start)
> >
> >Cleaner code, and more idiomatic, if we let unpin early return.
> 
> Maybe, but that's not the way it was previously written, so I didn't change
> it around.

We unfortunately let a lot of these through ... Early returns are
preferred, if it's possible. Same for skips in loops using if (!cond)
continue;, it makes for much less right-leaning code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/6] drm/i915: unmap the correct page in intel_logical_ring_cleanup()

[Dave Gordon]

The kunmap() call here didn't match the corresponding kmap().
The kmapping was changed with the introduction of the GuC-compatible
layout of context objects and the introduction of "LRC_PPHWSP_PN", in

    d167519 drm/i915: Integrate GuC-based command submission

but the corresponding kunmap() wasn't, probably because the old code
dug into the underlying sgl implementation instead of than calling
"i915_gem_object_get_page(ring->status_page.obj, 0)", which might more
easily have been noticed as containing an assumption about page 0.

v2:
    Split from "handle teardown of HWSP when context is freed".

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 282d66a..a3ab2b4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1987,7 +1987,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	i915_gem_batch_pool_fini(&ring->batch_pool);
 
 	if (ring->status_page.obj) {
-		kunmap(sg_page(ring->status_page.obj->pages->sgl));
+		struct page *page;
+
+		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
+		kunmap(page);
 		ring->status_page.obj = NULL;
 	}
 
-- 
1.9.1

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

Re: [PATCH v2 4/6] drm/i915: unmap the correct page in intel_logical_ring_cleanup()

[Chris Wilson]

On Fri, Jan 22, 2016 at 11:10:09PM +0000, Dave Gordon wrote:
> The kunmap() call here didn't match the corresponding kmap().
> The kmapping was changed with the introduction of the GuC-compatible
> layout of context objects and the introduction of "LRC_PPHWSP_PN", in
> 
>     d167519 drm/i915: Integrate GuC-based command submission
> 
> but the corresponding kunmap() wasn't, probably because the old code
> dug into the underlying sgl implementation instead of than calling
> "i915_gem_object_get_page(ring->status_page.obj, 0)", which might more
> easily have been noticed as containing an assumption about page 0.
> 
> v2:
>     Split from "handle teardown of HWSP when context is freed".
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 282d66a..a3ab2b4 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1987,7 +1987,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>  	i915_gem_batch_pool_fini(&ring->batch_pool);
>  
>  	if (ring->status_page.obj) {
> -		kunmap(sg_page(ring->status_page.obj->pages->sgl));
> +		struct page *page;
> +
> +		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);

kunmap(kmap_to_page(ring->status_page.page_addr));
-Chris

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

Re: [PATCH v2 4/6] drm/i915: unmap the correct page in intel_logical_ring_cleanup()

[Dave Gordon]

On 25/01/16 10:54, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 11:10:09PM +0000, Dave Gordon wrote:
>> The kunmap() call here didn't match the corresponding kmap().
>> The kmapping was changed with the introduction of the GuC-compatible
>> layout of context objects and the introduction of "LRC_PPHWSP_PN", in
>>
>>      d167519 drm/i915: Integrate GuC-based command submission
>>
>> but the corresponding kunmap() wasn't, probably because the old code
>> dug into the underlying sgl implementation instead of than calling
>> "i915_gem_object_get_page(ring->status_page.obj, 0)", which might more
>> easily have been noticed as containing an assumption about page 0.
>>
>> v2:
>>      Split from "handle teardown of HWSP when context is freed".
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 282d66a..a3ab2b4 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1987,7 +1987,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>>   	i915_gem_batch_pool_fini(&ring->batch_pool);
>>
>>   	if (ring->status_page.obj) {
>> -		kunmap(sg_page(ring->status_page.obj->pages->sgl));
>> +		struct page *page;
>> +
>> +		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
>
> kunmap(kmap_to_page(ring->status_page.page_addr));
> -Chris

That's handy, that way it doesn't need to know *what* the offset within 
the GEM object is.

I'll fix that one up in the next cycle :)

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

[PATCH v2 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

[Dave Gordon]

In LRC mode, the HWSP is part of the default context object, and
therefore does not exist independently. Worse, it doesn't contribute
to the refcount on the default context object either.

Currently, the default context is deallocated in intel_lr_context_free(),
but the HWSP kmapping is not torn down until the subsequent call to
intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj
continues to point to the (now non-existent) default context object,
and the kernel mapping likewise points to a page which is now free.

The solution is to dispose of the HWSP kmapping and pointer before the
object itself is freed, so this patch moves the kmap teardown code from
intel_lr_context_free() to intel_logical_ring_cleanup().

This code was introduced in

    48d8238 drm/i915/bdw: Generic logical ring init and cleanup

i.e. it has been there ever since LRC mode was first implemented.

v2:
    Split from "handle teardown of HWSP when context is freed".

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a3ab2b4..c702fc2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1986,13 +1986,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	i915_cmd_parser_fini_ring(ring);
 	i915_gem_batch_pool_fini(&ring->batch_pool);
 
-	if (ring->status_page.obj) {
-		struct page *page;
-
-		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
-		kunmap(page);
-		ring->status_page.obj = NULL;
-	}
+	/* Status page should have gone already */
+	WARN_ON(ring->status_page.page_addr);
+	WARN_ON(ring->status_page.obj);
 
 	ring->disable_lite_restore_wa = false;
 	ring->ctx_desc_template = 0;
@@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx)
 			continue;
 
 		if (ctx == ctx->i915->kernel_context) {
+			struct intel_engine_cs *ring = ringbuf->ring;
+
+			/*
+			 * The HWSP is part of the kernel context
+			 * object in LRC mode, so tear it down now.
+			 */
+			if (ring->status_page.obj) {
+				struct page *page;
+
+				page = i915_gem_object_get_page(
+						ring->status_page.obj,
+						LRC_PPHWSP_PN);
+				kunmap(page);
+				ring->status_page.obj = NULL;
+			}
+
 			intel_unpin_ringbuffer_obj(ringbuf);
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
-- 
1.9.1

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

Re: [PATCH v2 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

[Chris Wilson]

On Fri, Jan 22, 2016 at 11:10:10PM +0000, Dave Gordon wrote:
> In LRC mode, the HWSP is part of the default context object, and
> therefore does not exist independently. Worse, it doesn't contribute
> to the refcount on the default context object either.
> 
> Currently, the default context is deallocated in intel_lr_context_free(),
> but the HWSP kmapping is not torn down until the subsequent call to
> intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj
> continues to point to the (now non-existent) default context object,
> and the kernel mapping likewise points to a page which is now free.
> 
> The solution is to dispose of the HWSP kmapping and pointer before the
> object itself is freed, so this patch moves the kmap teardown code from
> intel_lr_context_free() to intel_logical_ring_cleanup().
> 
> This code was introduced in
> 
>     48d8238 drm/i915/bdw: Generic logical ring init and cleanup
> 
> i.e. it has been there ever since LRC mode was first implemented.
> 
> v2:
>     Split from "handle teardown of HWSP when context is freed".
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index a3ab2b4..c702fc2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1986,13 +1986,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>  	i915_cmd_parser_fini_ring(ring);
>  	i915_gem_batch_pool_fini(&ring->batch_pool);
>  
> -	if (ring->status_page.obj) {
> -		struct page *page;
> -
> -		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
> -		kunmap(page);
> -		ring->status_page.obj = NULL;
> -	}
> +	/* Status page should have gone already */
> +	WARN_ON(ring->status_page.page_addr);
> +	WARN_ON(ring->status_page.obj);
>  
>  	ring->disable_lite_restore_wa = false;
>  	ring->ctx_desc_template = 0;
> @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx)
>  			continue;
>  
>  		if (ctx == ctx->i915->kernel_context) {
> +			struct intel_engine_cs *ring = ringbuf->ring;
> +
> +			/*
> +			 * The HWSP is part of the kernel context
> +			 * object in LRC mode, so tear it down now.
> +			 */
> +			if (ring->status_page.obj) {
> +				struct page *page;
> +
> +				page = i915_gem_object_get_page(
> +						ring->status_page.obj,
> +						LRC_PPHWSP_PN);
> +				kunmap(page);
> +				ring->status_page.obj = NULL;
> +			}

No. I see no reason why we need to add special code to the context-free.
-Chris

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

Re: [PATCH v2 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

[Dave Gordon]

On 25/01/16 10:56, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 11:10:10PM +0000, Dave Gordon wrote:
>> In LRC mode, the HWSP is part of the default context object, and
>> therefore does not exist independently. Worse, it doesn't contribute
>> to the refcount on the default context object either.
>>
>> Currently, the default context is deallocated in intel_lr_context_free(),
>> but the HWSP kmapping is not torn down until the subsequent call to
>> intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj
>> continues to point to the (now non-existent) default context object,
>> and the kernel mapping likewise points to a page which is now free.
>>
>> The solution is to dispose of the HWSP kmapping and pointer before the
>> object itself is freed, so this patch moves the kmap teardown code from
>> intel_lr_context_free() to intel_logical_ring_cleanup().
>>
>> This code was introduced in
>>
>>      48d8238 drm/i915/bdw: Generic logical ring init and cleanup
>>
>> i.e. it has been there ever since LRC mode was first implemented.
>>
>> v2:
>>      Split from "handle teardown of HWSP when context is freed".
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 26 +++++++++++++++++++-------
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index a3ab2b4..c702fc2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1986,13 +1986,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>>   	i915_cmd_parser_fini_ring(ring);
>>   	i915_gem_batch_pool_fini(&ring->batch_pool);
>>
>> -	if (ring->status_page.obj) {
>> -		struct page *page;
>> -
>> -		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
>> -		kunmap(page);
>> -		ring->status_page.obj = NULL;
>> -	}
>> +	/* Status page should have gone already */
>> +	WARN_ON(ring->status_page.page_addr);
>> +	WARN_ON(ring->status_page.obj);
>>
>>   	ring->disable_lite_restore_wa = false;
>>   	ring->ctx_desc_template = 0;
>> @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx)
>>   			continue;
>>
>>   		if (ctx == ctx->i915->kernel_context) {
>> +			struct intel_engine_cs *ring = ringbuf->ring;
>> +
>> +			/*
>> +			 * The HWSP is part of the kernel context
>> +			 * object in LRC mode, so tear it down now.
>> +			 */
>> +			if (ring->status_page.obj) {
>> +				struct page *page;
>> +
>> +				page = i915_gem_object_get_page(
>> +						ring->status_page.obj,
>> +						LRC_PPHWSP_PN);
>> +				kunmap(page);
>> +				ring->status_page.obj = NULL;
>> +			}
>
> No. I see no reason why we need to add special code to the context-free.
> -Chris

The special-case-kernel-context test /is already there/.

I'm just making it do the right thing by moving this block out of 
intel_logical_ring_cleanup() and into the /existing/ special case.

Getting rid of the special case entirely is a project for another day, 
whereas this is a bug fix (NOT unmapping it here leaves a dangling 
reference to an object that has been freed).

OTOH I can change this to use:
	kunmap(kmap_to_page(ring->status_page.page_addr));
per the previous reply, which will make it a bit neater.

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

[PATCH v2 6/6] drm/i915: consolidate LRC mode HWSP setup & teardown

[Dave Gordon]

In legacy ringbuffer mode, the HWSP is a separate GEM object with
its own pinning and reference counts. In LRC mode, however, it's not;
instead its part of the default context object. The LRC-mode setup &
teardown code therefore needs to handle this specially; the presence
of the two bugs just fixed in this area suggests that this code is not
well-understood or maintained at present.

So, this patch:
    moves the (newly-fixed) LRC-mode HWSP teardown code to its own
        function lrc_teardown_hardware_status_page(), and
    changes the call signature of lrc_setup_hardware_status_page()
        to match
so that all knowledge of this special arrangement is local to these
two functions.

It will also help with efforts in progress to eliminate special handling
of the default (kernel) context elsewhere in LRC code :)

v2:
    Split from "handle teardown of HWSP when context is freed".

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 58 +++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c702fc2..83a841e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -226,9 +226,8 @@ enum {
 #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT  0x17
 
 static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
-static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
-		struct drm_i915_gem_object *default_ctx_obj);
-
+static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring);
+static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring);
 
 /**
  * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
@@ -1536,8 +1535,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u8 next_context_status_buffer_hw;
 
-	lrc_setup_hardware_status_page(ring,
-				dev_priv->kernel_context->engine[ring->id].state);
+	lrc_setup_hardware_status_page(ring);
 
 	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
 	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
@@ -2427,22 +2425,11 @@ void intel_lr_context_free(struct intel_context *ctx)
 			continue;
 
 		if (ctx == ctx->i915->kernel_context) {
-			struct intel_engine_cs *ring = ringbuf->ring;
-
 			/*
 			 * The HWSP is part of the kernel context
 			 * object in LRC mode, so tear it down now.
 			 */
-			if (ring->status_page.obj) {
-				struct page *page;
-
-				page = i915_gem_object_get_page(
-						ring->status_page.obj,
-						LRC_PPHWSP_PN);
-				kunmap(page);
-				ring->status_page.obj = NULL;
-			}
-
+			lrc_teardown_hardware_status_page(ringbuf->ring);
 			intel_unpin_ringbuffer_obj(ringbuf);
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
@@ -2491,24 +2478,45 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *ring)
 	return ret;
 }
 
-static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
-		struct drm_i915_gem_object *default_ctx_obj)
+static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(ring->dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
+	struct drm_i915_gem_object *dctx_obj = dctx->engine[ring->id].state;
+	u64 dctx_addr = i915_gem_obj_ggtt_offset(dctx_obj);
 	struct page *page;
 
-	/* The HWSP is part of the default context object in LRC mode. */
-	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(default_ctx_obj)
-			+ LRC_PPHWSP_PN * PAGE_SIZE;
-	page = i915_gem_object_get_page(default_ctx_obj, LRC_PPHWSP_PN);
+	/*
+	 * The HWSP is part of the default context object in LRC mode.
+	 * Note that it doesn't contribute to the refcount!
+	 */
+	page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
 	ring->status_page.page_addr = kmap(page);
-	ring->status_page.obj = default_ctx_obj;
+	ring->status_page.gfx_addr = dctx_addr + LRC_PPHWSP_PN * PAGE_SIZE;
+	ring->status_page.obj = dctx_obj;
 
 	I915_WRITE(RING_HWS_PGA(ring->mmio_base),
 			(u32)ring->status_page.gfx_addr);
 	POSTING_READ(RING_HWS_PGA(ring->mmio_base));
 }
 
+/* This should be called before the default context is destroyed */
+static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring)
+{
+	struct drm_i915_gem_object *dctx_obj = ring->status_page.obj;
+	struct page *page;
+
+	WARN_ON(!dctx_obj);
+
+	if (ring->status_page.page_addr) {
+		page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
+		kunmap(page);
+		ring->status_page.page_addr = NULL;
+	}
+
+	ring->status_page.obj = NULL;
+}
+
 /**
  * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
  * @ctx: LR context to create.
-- 
1.9.1

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

✓ Fi.CI.BAT: success for A collection of cleanups

[Patchwork]

== Summary ==

Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest


bdw-nuci7        total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:143  pass:119  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:143  pass:128  dwarn:0   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:104  dwarn:1   dfail:0   fail:0   skip:38 
ivb-t430s        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:129  dwarn:0   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:129  dwarn:0   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1254/

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

✓ Fi.CI.BAT: success for A collection of cleanups

[Patchwork]

== Summary ==

Built on b3f8ad64bc71f6236f05c2e9f4ad49a61745869a drm-intel-nightly: 2016y-01m-28d-10h-26m-23s UTC integration manifest


bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:153  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:159  pass:135  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:159  pass:142  dwarn:0   dfail:0   fail:0   skip:17 
hsw-brixbox      total:159  pass:152  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:159  pass:155  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:159  pass:114  dwarn:0   dfail:0   fail:1   skip:44 
ivb-t430s        total:159  pass:151  dwarn:0   dfail:0   fail:0   skip:8  
skl-i5k-2        total:159  pass:150  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:159  pass:141  dwarn:0   dfail:0   fail:0   skip:18 

Results at /archive/results/CI_IGT_test/Patchwork_1286/

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