[PATCH v5 00/11] A collection of cleanups, revisited

[PATCH v5 00/11] A collection of cleanups, revisited

[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; but more importantly, that
patch [10/11] is dependent on patch [9/11] of this set to work correctly.
Applying [10/11] without at least [9/11] as well may cause a fault on
driver unload!

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*).

v3:
    Patches reordered again; the bug fixes are now in patches 3 and 5.
    The former could stand alone; the latter depends on patch 4 and is
    a prerequisite for Nick's patch [6/6] to function.

v4:
    Rebased

v5:
    Patches repartitioned, reordered, and rebased again.
    The first two are now the ones that have already been reviewed
    (R-b: Chris Wilson), the remaining ones have been broken up
    into smaller more digestible chunks.

Dave Gordon (9):
  drm/i915: unmap the correct page in intel_logical_ring_cleanup()
  drm/i915: consolidate LRC mode HWSP setup & teardown
  drm/i915: remove useless copypasted code
  drm/i915: tidy up initialisation failure paths (GEM)
  drm/i915: tidy initialisation failure paths (legacy, part 1)
  drm/i915: tidy initialisation failure paths (legacy, part 2)
  drm/i915: tidy initialisation failure paths (legacy, part 3)
  drm/i915: tear down hardware status page mappings earlier
  drm/i915: make LRC status page teardown code a bit more robust

Nick Hoath (2):
  drm/i915: two small moves towards more consistency
  drm/i915: interchange context/engine cleanup order

 drivers/gpu/drm/i915/i915_dma.c         |  4 +-
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/i915_gem.c         | 28 ++++++++------
 drivers/gpu/drm/i915/intel_lrc.c        | 65 ++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 40 ++++++++------------
 5 files changed, 75 insertions(+), 64 deletions(-)

-- 
1.9.1

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

[PATCH v5 01/11] drm/i915: unmap the correct page in intel_logical_ring_cleanup()

[Dave Gordon]

The kunmap() call here didn't match the corresponding kmap().
The kmap()ing 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.

v3:
    Use kmap_to_page() rather than repeat the mapping calculation.
    [Chris Wilson]

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3a03646..c551c97 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2012,7 +2012,7 @@ 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));
+		kunmap(kmap_to_page(ring->status_page.page_addr));
 		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

[PATCH v5 02/11] 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 fixed in this patchset 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
        (trivial) 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.

v3: Rebased

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 41 +++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c551c97..148d291 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -227,9 +227,8 @@ enum {
 
 static int intel_lr_context_pin(struct intel_context *ctx,
 				struct intel_engine_cs *engine);
-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
@@ -1555,8 +1554,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);
@@ -2011,10 +2009,7 @@ 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) {
-		kunmap(kmap_to_page(ring->status_page.page_addr));
-		ring->status_page.obj = NULL;
-	}
+	lrc_teardown_hardware_status_page(ring);
 
 	ring->disable_lite_restore_wa = false;
 	ring->ctx_desc_template = 0;
@@ -2506,24 +2501,36 @@ 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));
 }
 
+static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring)
+{
+	if (ring->status_page.obj) {
+		kunmap(kmap_to_page(ring->status_page.page_addr));
+		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

[PATCH v5 03/11] drm/i915: remove useless copypasted code

[Dave Gordon]

There's some unreachable code in intel_logical_ring_cleanup(),
presumably copypasted from the legacy ringbuffer version at
creation. Let's delete it :)

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

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 148d291..ff38e57 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1991,17 +1991,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 v5 04/11] drm/i915: tidy up initialisation failure paths (GEM)

[Dave Gordon]

Add call to i915_gem_context_fini() to deallocate the default context
if the call to init_rings() fails, so that we don't leak the allocated
memory in that situation.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e9b19bc..2bd5b5f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4987,8 +4987,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) {
-- 
1.9.1

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

[PATCH v5 05/11] drm/i915: tidy initialisation failure paths (legacy, part 1)

[Dave Gordon]

Make sure intel_unpin_ringbuffer_obj() can handle the case where the
ringbuffer has been allocated but map-and-pin failed. Return early if
the ringbuffer isn't mapped [Chris Wilson].

That allows us to simplify the cleanup path in intel_init_ring_buffer(),
as it can just take the error exit and let intel_cleanup_ring_buffer()
deal with any partially-set-up state.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6f5b511..3e1aec8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2055,6 +2055,9 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
 
 void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 {
+	if (!ringbuf->virtual_start)
+		return;
+
 	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
 		vunmap(ringbuf->virtual_start);
 	else
@@ -2232,6 +2235,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)
@@ -2243,14 +2253,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;
-- 
1.9.1

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

[PATCH v5 06/11] drm/i915: tidy initialisation failure paths (legacy, part 2)

[Dave Gordon]

After the last patch, there is only one caller of the trivial function
intel_destroy_ringbuffer_obj(), so we might as well fold it into the
caller.

v3:
   Don't bother to clear a pointer in an object about to be freed.
   [Chris Wilson]

v4:
   Don't bother to check for NULL pointer, as drm_gem_object_unreference
   can handle NULL [Chris Wilson]. This relies on 'base' being the very
   first member of 'struct intel_ringbuffer'!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3e1aec8..284da10 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2135,12 +2135,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)
 {
@@ -2203,11 +2197,11 @@ 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);
+	drm_gem_object_unreference(&ringbuf->obj->base);
+	list_del(&ringbuf->link);
+	kfree(ringbuf);
 }
 
 static int intel_init_ring_buffer(struct drm_device *dev,
-- 
1.9.1

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

[PATCH v5 07/11] drm/i915: tidy initialisation failure paths (legacy, part 3)

[Dave Gordon]

intel_cleanup_ring_buffer() contains one low-level register access,
which is not really appropriate for its level of abstraction.

It calls intel_stop_ring_buffer() which then calls stop_ring() -- which
is the level that deals with h/w registers -- then reads a GEN-specific
register to see whether the ring is actually now idle. It only prints a
WARNING, though, and doesn't actually refrain from continuing even if
the test fails!

So, let's move the register-level check and WARNING down into the
low-level function that's already doing register access. If we wanted
to, we could pass a pass/fail status back, but since the high-level
code continues anyway, there's no reason to at present.

As a bonus, apart from fixing the lavering violation, moving the code
lets us eliminate the implicitly-used local 'dev_priv' from the caller.

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 284da10..b47d140 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;
 }
 
@@ -2260,17 +2262,11 @@ 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;
-
 	if (!intel_ring_initialized(ring))
 		return;
 
-	dev_priv = to_i915(ring->dev);
-
 	if (ring->buffer) {
 		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);
 		ring->buffer = NULL;
-- 
1.9.1

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

[PATCH v5 08/11] drm/i915: two small moves towards more consistency

[Dave Gordon]

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

Rename i915_gem_cleanup_ringbuffer() to i915_gem_cleanup_engines(),
to relect what it actually does (clean up all the engines, not a
single ringbuffer). The name is probably a legacy from the days
when there was only one "ring" (i.e. engine) and one ringbuffer!

While we're looking at this function, there's a strangely-formatted
comment block that looks ugly, so let's prettify it.

Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
Signed-off-by: David Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
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 a42eb58..5d95c80 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -444,7 +444,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_gem:
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_ringbuffer(dev);
+	i915_gem_cleanup_engines(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 cleanup_irq:
@@ -1253,7 +1253,7 @@ 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_cleanup_engines(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 	intel_fbc_cleanup_cfb(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8216665..e11eef1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3058,7 +3058,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 2bd5b5f..c85029b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4913,7 +4913,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;
 		}
 
@@ -4926,7 +4926,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;
 		}
 
@@ -4934,7 +4934,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;
 		}
 
@@ -5012,7 +5012,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;
@@ -5021,13 +5021,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

[PATCH v5 09/11] drm/i915: tear down hardware status page mappings earlier

[Dave Gordon]

This has to be done before the containing object is freed, otherwise
the mapping ends up pointing to no-longer-allocated memory :(

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff38e57..95ba8ec 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2003,7 +2003,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	i915_cmd_parser_fini_ring(ring);
 	i915_gem_batch_pool_fini(&ring->batch_pool);
 
-	lrc_teardown_hardware_status_page(ring);
 
 	ring->disable_lite_restore_wa = false;
 	ring->ctx_desc_template = 0;
@@ -2447,6 +2446,7 @@ void intel_lr_context_free(struct intel_context *ctx)
 			continue;
 
 		if (ctx == ctx->i915->kernel_context) {
+			lrc_teardown_hardware_status_page(ringbuf->ring);
 			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

[PATCH v5 10/11] drm/i915: interchange 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.

Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
Signed-off-by: David Gordon <david.s.gordon@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5d95c80..57afec8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -444,8 +444,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_gem:
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_engines(dev);
 	i915_gem_context_fini(dev);
+	i915_gem_cleanup_engines(dev);
 	mutex_unlock(&dev->struct_mutex);
 cleanup_irq:
 	intel_guc_ucode_fini(dev);
@@ -1253,8 +1253,8 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_guc_ucode_fini(dev);
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_engines(dev);
 	i915_gem_context_fini(dev);
+	i915_gem_cleanup_engines(dev);
 	mutex_unlock(&dev->struct_mutex);
 	intel_fbc_cleanup_cfb(dev_priv);
 
-- 
1.9.1

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

[PATCH v5 11/11] drm/i915: make LRC status page teardown code a bit more robust

[Dave Gordon]

Having recently fixed a few ordering/lifetime bugs in this area, it
seems worthwhile putting a few more checks and warnings here, to provide
early detection of any future code changes that cause problems;
especially as we hope to rework the pinning/refcounting of the kernel
context soon.

Specifically, we want to check that teardown_status_page is called
*before* the underlying storage is freed, and that by the time
intel_logical_ring_cleanup() is called, the status page mapping have
already been released. We'll also take care to undo the status page
kmapping only if it has been set up.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 95ba8ec..b4deaca 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2003,6 +2003,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);
 
+	/* 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;
@@ -2446,6 +2449,10 @@ void intel_lr_context_free(struct intel_context *ctx)
 			continue;
 
 		if (ctx == ctx->i915->kernel_context) {
+			/*
+			 * The HWSP is part of the kernel context
+			 * object in LRC mode, so tear it down *now*
+			 */
 			lrc_teardown_hardware_status_page(ringbuf->ring);
 			intel_unpin_ringbuffer_obj(ringbuf);
 			i915_gem_object_ggtt_unpin(ctx_obj);
@@ -2517,12 +2524,19 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring)
 	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)
 {
-	if (ring->status_page.obj) {
+	struct drm_i915_gem_object *dctx_obj = ring->status_page.obj;
+
+	WARN_ON(!dctx_obj);
+
+	if (ring->status_page.page_addr) {
 		kunmap(kmap_to_page(ring->status_page.page_addr));
-		ring->status_page.obj = NULL;
+		ring->status_page.page_addr = NULL;
 	}
+
+	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

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

[Patchwork]

== Summary ==

Series 3129v1 A collection of cleanups, revisited
http://patchwork.freedesktop.org/api/1.0/series/3129/revisions/1/mbox/


bdw-nuci7        total:161  pass:152  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:164  pass:152  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:164  pass:136  dwarn:0   dfail:0   fail:0   skip:28 
byt-nuc          total:164  pass:141  dwarn:0   dfail:0   fail:0   skip:23 
hsw-brixbox      total:164  pass:151  dwarn:0   dfail:0   fail:0   skip:13 
hsw-gt2          total:164  pass:154  dwarn:0   dfail:0   fail:0   skip:10 
hsw-xps12        total:161  pass:151  dwarn:0   dfail:0   fail:0   skip:10 
ilk-hp8440p      total:164  pass:116  dwarn:0   dfail:0   fail:0   skip:48 
ivb-t430s        total:164  pass:150  dwarn:0   dfail:0   fail:0   skip:14 
snb-dellxps      total:164  pass:142  dwarn:0   dfail:0   fail:0   skip:22 
snb-x220t        total:164  pass:142  dwarn:0   dfail:0   fail:1   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1373/

9ae71c139227311e70dcc91d16d5acba34ce9a71 drm-intel-nightly: 2016y-02m-08d-09h-39m-29s UTC integration manifest
c6e46d42282341054b3183736b63c9dc4f96c431 drm/i915: make LRC status page teardown code a bit more robust
80b1f268aab94610ff05e3ac3765f7fba4404a5f drm/i915: interchange context/engine cleanup order
17415233d212f9220d65d946eacbacc3d365c927 drm/i915: tear down hardware status page mappings earlier
22a820bb751fbec8fdf30cd6adb06c70ca7033b7 drm/i915: two small moves towards more consistency
79462059fa840146f741ac3802670597127249a5 drm/i915: tidy initialisation failure paths (legacy, part 3)
86b8561e262d4fa88c32047e7274d324d7e841c6 drm/i915: tidy initialisation failure paths (legacy, part 2)
afee40fb6f764b0d4e68652711586d94000076de drm/i915: tidy initialisation failure paths (legacy, part 1)
a9a4abe918dc8772ecf29dac0a28ea538fa64e02 drm/i915: tidy up initialisation failure paths (GEM)
82cbb0a772f12b08d55371da2b380e313aa102f4 drm/i915: remove useless copypasted code
2c935ea952dafcb9374782354c778eee264eac7e drm/i915: consolidate LRC mode HWSP setup & teardown
2be18abe529bc5d6a5611ae27cedd3bcd1a3e6d7 drm/i915: unmap the correct page in intel_logical_ring_cleanup()

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