A collection of cleanups

A collection of cleanups

[Dave Gordon]

Also includes Nick Hoath's patch to swap context/engine teardown.

In-Reply-To: 

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

[PATCH 1/4] drm/i915: handle teardown of HWSP when context is freed

[Dave Gordon]

Existing code did a kunmap() on the wrong page, and didn't account for the
fact that the HWSP and the default context are different offsets within the
same GEM object.

This patch improves symmetry by creating lrc_teardown_hardware_status_page()
to complement lrc_setup_hardware_status_page(), making sure that they both
take the same argument (pointer to engine). They encapsulate all the details
of the relationship between the default context and the status page, so the
rest of the LRC code doesn't have to know about it.

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

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 73d4347..3914120 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);
@@ -1992,10 +1990,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) {
-		kunmap(sg_page(ring->status_page.obj->pages->sgl));
-		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;
@@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
 			continue;
 
 		if (ctx == ctx->i915->kernel_context) {
+			/*
+			 * The HWSP is part of the default context
+			 * object in LRC mode.
+			 */
+			lrc_teardown_hardware_status_page(ringbuf->ring);
 			intel_unpin_ringbuffer_obj(ringbuf);
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
@@ -2482,24 +2484,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

[PATCH 2/4] 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: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

---
 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

[PATCH 3/4] 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

[PATCH 4/4] 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 3914120..c9b0917 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1972,17 +1972,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

Re: A collection of cleanups

[Dave Gordon]

On 21/01/16 19:37, Dave Gordon wrote:
> Also includes Nick Hoath's patch to swap context/engine teardown.
>
> In-Reply-To:
>

Hmm, editor/mailer seems to have dropped most of this summary :(
Probably something to do with that IRT-line above ... what it was 
supposed to say was approximately this:

Subject: [PATCH 0/4] A collection of cleanups

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.

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

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

✗ Fi.CI.BAT: warning for series starting with [1/4] drm/i915: handle teardown of HWSP when context is freed

[Patchwork]

== Summary ==

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

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (ilk-hp8440p)

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:103  dwarn:2   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  
skl-i7k-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_1241/

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

Re: [PATCH 1/4] drm/i915: handle teardown of HWSP when context is freed

[Mika Kuoppala]

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

> Existing code did a kunmap() on the wrong page, and didn't account for the
> fact that the HWSP and the default context are different offsets within the
> same GEM object.
>

Just to note here that usually we try to split bug fixes early in the
series with minimal code changes. This helps cherrypickups
to fixes/stable. And also lessens the probability that we accidentally
revert bugfix when some other problem occurs with the patch.

> This patch improves symmetry by creating lrc_teardown_hardware_status_page()
> to complement lrc_setup_hardware_status_page(), making sure that they both
> take the same argument (pointer to engine). They encapsulate all the details
> of the relationship between the default context and the status page, so the
> rest of the LRC code doesn't have to know about it.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 57 ++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 73d4347..3914120 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);
> @@ -1992,10 +1990,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) {
> -		kunmap(sg_page(ring->status_page.obj->pages->sgl));
> -		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;
> @@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
>  			continue;
>  
>  		if (ctx == ctx->i915->kernel_context) {
> +			/*
> +			 * The HWSP is part of the default context
> +			 * object in LRC mode.
> +			 */
> +			lrc_teardown_hardware_status_page(ringbuf->ring);
>  			intel_unpin_ringbuffer_obj(ringbuf);
>  			i915_gem_object_ggtt_unpin(ctx_obj);
>  		}
> @@ -2482,24 +2484,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;
> +	}
> +

As the page is always kmapped along with setting of status_page.obj,
I fail to see why we need the check here for the page_addr.

How about just:

if (WARN_ON(!dctx_obj))
   return;

-Mika

> +	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: handle teardown of HWSP when context is freed

[Chris Wilson]

On Fri, Jan 22, 2016 at 12:28:22PM +0200, Mika Kuoppala wrote:
> As the page is always kmapped along with setting of status_page.obj,
> I fail to see why we need the check here for the page_addr.
> 
> How about just:
> 
> if (WARN_ON(!dctx_obj))
>    return;

Just let it die in a GPF. We already have code to detect impossible
situations.
-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 1/4] drm/i915: handle teardown of HWSP when context is freed

[Dave Gordon]

On 22/01/16 10:28, Mika Kuoppala wrote:
> Dave Gordon <david.s.gordon@intel.com> writes:
>
>> Existing code did a kunmap() on the wrong page, and didn't account for the
>> fact that the HWSP and the default context are different offsets within the
>> same GEM object.
>>
>
> Just to note here that usually we try to split bug fixes early in the
> series with minimal code changes. This helps cherrypickups
> to fixes/stable. And also lessens the probability that we accidentally
> revert bugfix when some other problem occurs with the patch.

OK, let's drop this one from the series (they're orthogonal code 
changes, only related by the fact they're all to do with init/fini), and 
I'll resubmit it separately.

Please continue this series from 2/4 ...

>> This patch improves symmetry by creating lrc_teardown_hardware_status_page()
>> to complement lrc_setup_hardware_status_page(), making sure that they both
>> take the same argument (pointer to engine). They encapsulate all the details
>> of the relationship between the default context and the status page, so the
>> rest of the LRC code doesn't have to know about it.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 57 ++++++++++++++++++++++++++++------------
>>   1 file changed, 40 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 73d4347..3914120 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);
>> @@ -1992,10 +1990,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) {
>> -		kunmap(sg_page(ring->status_page.obj->pages->sgl));
>> -		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;
>> @@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
>>   			continue;
>>
>>   		if (ctx == ctx->i915->kernel_context) {
>> +			/*
>> +			 * The HWSP is part of the default context
>> +			 * object in LRC mode.
>> +			 */
>> +			lrc_teardown_hardware_status_page(ringbuf->ring);
>>   			intel_unpin_ringbuffer_obj(ringbuf);
>>   			i915_gem_object_ggtt_unpin(ctx_obj);
>>   		}
>> @@ -2482,24 +2484,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;
>> +	}
>> +
>
> As the page is always kmapped along with setting of status_page.obj,
> I fail to see why we need the check here for the page_addr.

The WARN_ON() tells the developer something unexpected happened.

The page_addr test avoids doing something that may cause an OOPS so that 
the driver can continue (to unload!).

IMHO, teardown code *in particular* should be written to anticipate 
being called in all sorts of inconsistent states, because a common error 
handling strategy is just to say,

     oops, something bad happened at some stage of setup!
     let's tear down everything and pass the error back

which is certainly easier than tracking exactly where the error occurred 
and undoing only the steps known to have succeeded -- that leads to lots 
of "goto err_1", "goto err_2", etc :(

This code therefore copes with the situation where the object exists, 
but the mapping hasn't been set up. With the structure of the code in 
lrc_setup_hardware_status_page() as implemented above, this won't 
happen. But someone might someday add a new early error exit that left 
these variables in that state, and the failure in setup would then 
result in the driver load failing, at which point teardown() would be 
called ...

So I might as well guard the kunmap() with a test that the page has 
actually been kmap()ed, to save some other developer from frustration in 
the future.

.Dave.

> How about just:
>
> if (WARN_ON(!dctx_obj))
>     return;
>
> -Mika
>
>> +	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

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

Re: ✗ Fi.CI.BAT: warning for series starting with [1/4] drm/i915: handle teardown of HWSP when context is freed

[Dave Gordon]

On 22/01/16 07:42, Patchwork wrote:
> == Summary ==
>
> Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest
>
> Test kms_pipe_crc_basic:
>          Subgroup read-crc-pipe-a-frame-sequence:
>                  pass       -> DMESG-WARN (ilk-hp8440p)

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

.Dave.

> 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:103  dwarn:2   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
> skl-i7k-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_1241/


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

Re: [PATCH 1/4] drm/i915: handle teardown of HWSP when context is freed

[Daniel Vetter]

On Fri, Jan 22, 2016 at 02:51:50PM +0000, Dave Gordon wrote:
> On 22/01/16 10:28, Mika Kuoppala wrote:
> >Dave Gordon <david.s.gordon@intel.com> writes:
> >
> >>Existing code did a kunmap() on the wrong page, and didn't account for the
> >>fact that the HWSP and the default context are different offsets within the
> >>same GEM object.
> >>
> >
> >Just to note here that usually we try to split bug fixes early in the
> >series with minimal code changes. This helps cherrypickups
> >to fixes/stable. And also lessens the probability that we accidentally
> >revert bugfix when some other problem occurs with the patch.
> 
> OK, let's drop this one from the series (they're orthogonal code changes,
> only related by the fact they're all to do with init/fini), and I'll
> resubmit it separately.
> 
> Please continue this series from 2/4 ...
> 
> >>This patch improves symmetry by creating lrc_teardown_hardware_status_page()
> >>to complement lrc_setup_hardware_status_page(), making sure that they both
> >>take the same argument (pointer to engine). They encapsulate all the details
> >>of the relationship between the default context and the status page, so the
> >>rest of the LRC code doesn't have to know about it.
> >>
> >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_lrc.c | 57 ++++++++++++++++++++++++++++------------
> >>  1 file changed, 40 insertions(+), 17 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>index 73d4347..3914120 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);
> >>@@ -1992,10 +1990,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) {
> >>-		kunmap(sg_page(ring->status_page.obj->pages->sgl));
> >>-		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;
> >>@@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
> >>  			continue;
> >>
> >>  		if (ctx == ctx->i915->kernel_context) {
> >>+			/*
> >>+			 * The HWSP is part of the default context
> >>+			 * object in LRC mode.
> >>+			 */
> >>+			lrc_teardown_hardware_status_page(ringbuf->ring);
> >>  			intel_unpin_ringbuffer_obj(ringbuf);
> >>  			i915_gem_object_ggtt_unpin(ctx_obj);
> >>  		}
> >>@@ -2482,24 +2484,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;
> >>+	}
> >>+
> >
> >As the page is always kmapped along with setting of status_page.obj,
> >I fail to see why we need the check here for the page_addr.
> 
> The WARN_ON() tells the developer something unexpected happened.
> 
> The page_addr test avoids doing something that may cause an OOPS so that the
> driver can continue (to unload!).
> 
> IMHO, teardown code *in particular* should be written to anticipate being
> called in all sorts of inconsistent states, because a common error handling
> strategy is just to say,
> 
>     oops, something bad happened at some stage of setup!
>     let's tear down everything and pass the error back
> 
> which is certainly easier than tracking exactly where the error occurred and
> undoing only the steps known to have succeeded -- that leads to lots of
> "goto err_1", "goto err_2", etc :(

Russian dolls error code unwinding is pretty much standard kernel
practice. And since it's also mostly dead code (well in practice) it's not
too much of a validation horror show. This will likely change a lot if we
start using EPROBE_DEFER in earnest.

This comment is just for context, i.e. I think this is totally fine. It's
a bit fragile, but you'll be fighting against a _lot_ of inertia with this
;-)

Cheers, 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

Re: [PATCH 2/4] drm/i915: Fix context/engine cleanup order

[Daniel Vetter]

On Thu, Jan 21, 2016 at 07:37:45PM +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: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)

Trusting that v2==v4 and applied it.
-Daniel

> 
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> ---
>  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
> 

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

Re: [PATCH 2/4] drm/i915: Fix context/engine cleanup order

[Tvrtko Ursulin]

On 21/01/16 19:37, 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: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>
> ---
>   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);

context_fini assumes it is the one who is dropping the last reference to 
the default/kernel context and NULLs the pointer.

Going forward this means there is this hidden dependency that engines 
must not hold the reference to the kernel context, at least not rely on 
dev_priv->kernel_context, or we would either leak the reference or null 
ptr deref.

I don't know, somehow feels a bit fragile. But a lot of things seem 
fragile around here.

If the main motivation is to ensure access to ring->dev, then ctx->i915 
could be used from the context cleanup code. For the ordering itself I 
am not sure what is more logical. Idle GPU -> shutdown engines -> 
cleanup contexts sounds OK to me since the engines are the ones which 
can hold references to contexts. So logically cleaning up context before 
engines feels strange. But I am sure an opposite argument could also be 
made?

Regards,

Tvrtko

>   	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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx