drm/i915/bdw: Clean up execlist queue items in retire_work

Write HWS_PGA address even in execlists mode as the global hardware status page is still required. This address was previously uninitialized and HWSP writes would clobber whatever buffer happened to reside at GGTT address 0.

v2: Break out hardware status page setup into a separate function.

Issue: VIZ-2020

Signed-off-by Thomas Daniel <[email protected]>

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

Issue: VIZ-4277

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 03, 2014 4:54 PM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 21 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

Quoted 4 lines

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

Issue: VIZ-4277

This doesn't really do the full task since the integration with the shrinker and related igt testcases are missing. What's your plane here?

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

Quoted 34 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 03, 2014 4:54 PM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 24 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

Quoted 4 lines

Quoted 1 line

Issue: VIZ-4277

This doesn't really do the full task since the integration with the shrinker and related igt testcases are missing. What's your plane here?

This is a rebase and bug fix of the original patch to unblock execlists enabling. Plan is to address the rest of the issues after the big seqno->request rearchitecting change goes in.

Quoted 21 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

Quoted 4 lines

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

Issue: VIZ-4277

This doesn't really do the full task since the integration with the shrinker and related igt testcases are missing. What's your plane here?

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

v5: Remove unpin_lock as all pinning and unpinning is done with the struct mutex already locked. Add WARN_ONs to make sure this is the case in future.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
  drivers/gpu/drm/i915/i915_debugfs.c |   12 +++++-
  drivers/gpu/drm/i915/i915_drv.h     |    1 +
  drivers/gpu/drm/i915/i915_gem.c     |   39 +++++++++++++-------
  drivers/gpu/drm/i915/intel_lrc.c    |   69 +++++++++++++++++++++++++++++------
  drivers/gpu/drm/i915/intel_lrc.h    |    4 ++
  5 files changed, 98 insertions(+), 27 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +10 -2

index e60d5c2..6eaf813 100644
@@ -1799,10 +1799,16 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
  				continue;
 
  			if (ctx_obj) {
-				struct page *page = i915_gem_object_get_page(ctx_obj, 1);
-				uint32_t *reg_state = kmap_atomic(page);
+				struct page *page;
+				uint32_t *reg_state;
  				int j;
 
+				i915_gem_obj_ggtt_pin(ctx_obj,
+						GEN8_LR_CONTEXT_ALIGN, 0);
+
+				page = i915_gem_object_get_page(ctx_obj, 1);
+				reg_state = kmap_atomic(page);
+
  				seq_printf(m, "CONTEXT: %s %u\n", ring->name,
  						intel_execlists_ctx_id(ctx_obj));
 

@@ -1814,6 +1820,8 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
  				}
  				kunmap_atomic(reg_state);
 
+				i915_gem_object_ggtt_unpin(ctx_obj);
+
  				seq_putc(m, '\n');
  			}
  		}

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 059330c..3c7299d 100644
@@ -655,6 +655,7 @@ struct intel_context {
  	struct {
  		struct drm_i915_gem_object *state;
  		struct intel_ringbuffer *ringbuf;
+		int unpin_count;
  	} engine[I915_NUM_RINGS];
 
  	struct list_head link;

drivers/gpu/drm/i915/i915_gem.c +25 -14

index 408afe7..2ee6996 100644
@@ -2494,12 +2494,18 @@ static void i915_set_reset_status(struct drm_i915_private *dev_priv,
 
  static void i915_gem_free_request(struct drm_i915_gem_request *request)
  {
+	struct intel_context *ctx = request->ctx;
+
  	list_del(&request->list);
  	i915_gem_request_remove_from_client(request);
 
-	if (request->ctx)
-		i915_gem_context_unreference(request->ctx);
+	if (i915.enable_execlists && ctx) {
+		struct intel_engine_cs *ring = request->ring;
 
+		if (ctx != ring->default_context)
+			intel_lr_context_unpin(ring, ctx);
+		i915_gem_context_unreference(ctx);
+	}
  	kfree(request);
  }
 

@@ -2554,6 +2560,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
  	}
 
  	/*
+	 * Clear the execlists queue up before freeing the requests, as those
+	 * are the ones that keep the context and ringbuffer backing objects
+	 * pinned in place.
+	 */
+	while (!list_empty(&ring->execlist_queue)) {
+		struct intel_ctx_submit_request *submit_req;
+
+		submit_req = list_first_entry(&ring->execlist_queue,
+				struct intel_ctx_submit_request,
+				execlist_link);
+		list_del(&submit_req->execlist_link);
+		intel_runtime_pm_put(dev_priv);
+		i915_gem_context_unreference(submit_req->ctx);
+		kfree(submit_req);
+	}
+
+	/*
  	 * We must free the requests after all the corresponding objects have
  	 * been moved off active lists. Which is the same order as the normal
  	 * retire_requests function does. This is important if object hold

@@ -2570,18 +2593,6 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
  		i915_gem_free_request(request);
  	}
 
-	while (!list_empty(&ring->execlist_queue)) {
-		struct intel_ctx_submit_request *submit_req;
-
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct intel_ctx_submit_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
-		intel_runtime_pm_put(dev_priv);
-		i915_gem_context_unreference(submit_req->ctx);
-		kfree(submit_req);
-	}
-
  	/* These may not have been flush before the reset, do so now */
  	kfree(ring->preallocated_lazy_request);
  	ring->preallocated_lazy_request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -2

index 906b985..f7fa0f7 100644
@@ -139,8 +139,6 @@
  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
  #define GEN8_LR_CONTEXT_OTHER_SIZE (2 * PAGE_SIZE)
 
-#define GEN8_LR_CONTEXT_ALIGN 4096
-
  #define RING_EXECLIST_QFULL		(1 << 0x2)
  #define RING_EXECLIST1_VALID		(1 << 0x3)
  #define RING_EXECLIST0_VALID		(1 << 0x4)

drivers/gpu/drm/i915/intel_lrc.c +51 -9

+	if (ctx->engine[ring->id].unpin_count++ == 0) {
+		ret = i915_gem_obj_ggtt_pin(ctx_obj,
+				GEN8_LR_CONTEXT_ALIGN, 0);
+		if (ret)
+			ctx->engine[ring->id].unpin_count = 0;
+	}
+
+	return ret;
+}
+
+void intel_lr_context_unpin(struct intel_engine_cs *ring,
+		struct intel_context *ctx)
+{
+	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+
+	if (ctx_obj) {
+		WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+		if (--ctx->engine[ring->id].unpin_count == 0)
+			i915_gem_object_ggtt_unpin(ctx_obj);
+	}
+}
+
  static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
  				    struct intel_context *ctx)
  {
+	int ret;
+
  	if (ring->outstanding_lazy_seqno)
  		return 0;
 

@@ -814,6 +843,14 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
  		if (request == NULL)
  			return -ENOMEM;
 
+		if (ctx != ring->default_context) {
+			ret = intel_lr_context_pin(ring, ctx);
+			if (ret) {
+				kfree(request);
+				return ret;
+			}
+		}
+
  		/* Hold a reference to the context this request belongs to
  		 * (we will need it when the time comes to emit/retire the
  		 * request).

@@ -1626,12 +1663,16 @@ void intel_lr_context_free(struct intel_context *ctx)
 
  	for (i = 0; i < I915_NUM_RINGS; i++) {
  		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
-		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
 
  		if (ctx_obj) {
+			struct intel_ringbuffer *ringbuf =
+					ctx->engine[i].ringbuf;
+			struct intel_engine_cs *ring = ringbuf->ring;
+
  			intel_destroy_ringbuffer_obj(ringbuf);
  			kfree(ringbuf);
-			i915_gem_object_ggtt_unpin(ctx_obj);
+			if (ctx == ring->default_context)
+				i915_gem_object_ggtt_unpin(ctx_obj);
  			drm_gem_object_unreference(&ctx_obj->base);
  		}
  	}

@@ -1695,6 +1736,7 @@ static int lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
  int intel_lr_context_deferred_create(struct intel_context *ctx,
  				     struct intel_engine_cs *ring)
  {
+	const bool is_global_default_ctx = (ctx == ring->default_context);
  	struct drm_device *dev = ring->dev;
  	struct drm_i915_gem_object *ctx_obj;
  	uint32_t context_size;

@@ -1714,18 +1756,22 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
  		return ret;
  	}
 
-	ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN, 0);
-	if (ret) {
-		DRM_DEBUG_DRIVER("Pin LRC backing obj failed: %d\n", ret);
-		drm_gem_object_unreference(&ctx_obj->base);
-		return ret;
+	if (is_global_default_ctx) {
+		ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN, 0);
+		if (ret) {
+			DRM_DEBUG_DRIVER("Pin LRC backing obj failed: %d\n",
+					ret);
+			drm_gem_object_unreference(&ctx_obj->base);
+			return ret;
+		}
  	}
 
  	ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
  	if (!ringbuf) {
  		DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
  				ring->name);
-		i915_gem_object_ggtt_unpin(ctx_obj);
+		if (is_global_default_ctx)
+			i915_gem_object_ggtt_unpin(ctx_obj);
  		drm_gem_object_unreference(&ctx_obj->base);
  		ret = -ENOMEM;
  		return ret;

@@ -1787,7 +1833,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 
  error:
  	kfree(ringbuf);
-	i915_gem_object_ggtt_unpin(ctx_obj);
+	if (is_global_default_ctx)
+		i915_gem_object_ggtt_unpin(ctx_obj);
  	drm_gem_object_unreference(&ctx_obj->base);
  	return ret;
  }

drivers/gpu/drm/i915/intel_lrc.h +2 -0

index 84bbf19..14b216b 100644
@@ -24,6 +24,8 @@
  #ifndef _INTEL_LRC_H_
  #define _INTEL_LRC_H_
 
+#define GEN8_LR_CONTEXT_ALIGN 4096
+
  /* Execlists regs */
  #define RING_ELSP(ring)			((ring)->mmio_base+0x230)
  #define RING_EXECLIST_STATUS(ring)	((ring)->mmio_base+0x234)

Quoted 16 lines · Thomas Daniel

drivers/gpu/drm/i915/intel_lrc.c +1 -2

index 906b985..f7fa0f7 100644
@@ -139,8 +139,6 @@
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_OTHER_SIZE (2 * PAGE_SIZE)
-#define GEN8_LR_CONTEXT_ALIGN 4096
-
 #define RING_EXECLIST_QFULL		(1 << 0x2)
 #define RING_EXECLIST1_VALID		(1 << 0x3)
 #define RING_EXECLIST0_VALID		(1 << 0x4)

@@ -801,9 +799,40 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
 	execlists_context_queue(ring, ctx, ringbuf->tail);
 }
+static int intel_lr_context_pin(struct intel_engine_cs *ring,

drivers/gpu/drm/i915/intel_lrc.c void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf) · line 808

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +6 -0

+		struct intel_context *ctx)
+{
+	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+	int ret = 0;
+
+	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

With pin specific mutex from previous patch set removed.

Quoted 16 lines · Deepak S

Quoted 15 lines · Thomas Daniel

drivers/gpu/drm/i915/intel_lrc.c +0 -2

index 906b985..f7fa0f7 100644
@@ -139,8 +139,6 @@
  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
  #define GEN8_LR_CONTEXT_OTHER_SIZE (2 * PAGE_SIZE)
-#define GEN8_LR_CONTEXT_ALIGN 4096
-
  #define RING_EXECLIST_QFULL		(1 << 0x2)
  #define RING_EXECLIST1_VALID		(1 << 0x3)
  #define RING_EXECLIST0_VALID		(1 << 0x4)

@@ -801,9 +799,40 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
  	execlists_context_queue(ring, ctx, ringbuf->tail);
  }

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf) · line 808

Quoted 7 lines

drivers/gpu/drm/i915/intel_lrc.c +7 -0

+static int intel_lr_context_pin(struct intel_engine_cs *ring,
+		struct intel_context *ctx)
+{
+	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+	int ret = 0;
+
+	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

With pin specific mutex from previous patch set removed.

Pardon my ignorance but I'm completely lost on this review comment here. Deepak, can you please elaborate what kind of lock on which exact version of the previous patch you mean? I didn't find any locking at all in the preceeding patch here ...

Thanks, Daniel

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

v5: Remove unpin_lock as all pinning and unpinning is done with the struct mutex already locked. Add WARN_ONs to make sure this is the case in future.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c |   12 +++++-
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_gem.c     |   39 +++++++++++++-------
 drivers/gpu/drm/i915/intel_lrc.c    |   69
+++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_lrc.h    |    4 ++
 5 files changed, 98 insertions(+), 27 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +8 -2

index e60d5c2..6eaf813 100644
@@ -1799,10 +1799,16 @@ static int i915_dump_lrc(struct seq_file *m, void
 *unused)
                                continue;
 
                        if (ctx_obj) {
-                               struct page *page =
 i915_gem_object_get_page(ctx_obj, 1);
-                               uint32_t *reg_state = kmap_atomic(page);
+                               struct page *page;
+                               uint32_t *reg_state;
                                int j;
 
+                               i915_gem_obj_ggtt_pin(ctx_obj,
+                                               GEN8_LR_CONTEXT_ALIGN, 0);
+
+                               page = i915_gem_object_get_page(ctx_obj,
 1);
+                               reg_state = kmap_atomic(page);
+

                                seq_printf(m, "CONTEXT: %s %u\n",

ring->name,

intel_execlists_ctx_id(ctx_obj));

drivers/gpu/drm/i915/i915_debugfs.c +2 -0

@@ -1814,6 +1820,8 @@ static int i915_dump_lrc(struct seq_file *m, void
 *unused)
                                }
                                kunmap_atomic(reg_state);
 
+                               i915_gem_object_ggtt_unpin(ctx_obj);
+
                                seq_putc(m, '\n');
                        }

                }

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 059330c..3c7299d 100644
@@ -655,6 +655,7 @@ struct intel_context {
        struct {
                struct drm_i915_gem_object *state;
                struct intel_ringbuffer *ringbuf;
+               int unpin_count;
        } engine[I915_NUM_RINGS];
 
        struct list_head link;

drivers/gpu/drm/i915/i915_gem.c +9 -2

index 408afe7..2ee6996 100644
@@ -2494,12 +2494,18 @@ static void i915_set_reset_status(struct
 drm_i915_private *dev_priv,
 
 static void i915_gem_free_request(struct drm_i915_gem_request *request)
 {
+       struct intel_context *ctx = request->ctx;
+
        list_del(&request->list);
        i915_gem_request_remove_from_client(request);
 
-       if (request->ctx)
-               i915_gem_context_unreference(request->ctx);
+       if (i915.enable_execlists && ctx) {
+               struct intel_engine_cs *ring = request->ring;
 
+               if (ctx != ring->default_context)
+                       intel_lr_context_unpin(ring, ctx);
+               i915_gem_context_unreference(ctx);
+       }
        kfree(request);
 }

@@ -2554,6 +2560,23 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
        }
 
        /*
+        * Clear the execlists queue up before freeing the requests, as

those
+ * are the ones that keep the context and ringbuffer backing
objects

drivers/gpu/drm/i915/i915_gem.c +15 -0

+        * pinned in place.
+        */
+       while (!list_empty(&ring->execlist_queue)) {
+               struct intel_ctx_submit_request *submit_req;
+
+               submit_req = list_first_entry(&ring->execlist_queue,
+                               struct intel_ctx_submit_request,
+                               execlist_link);
+               list_del(&submit_req->execlist_link);
+               intel_runtime_pm_put(dev_priv);
+               i915_gem_context_unreference(submit_req->ctx);
+               kfree(submit_req);
+       }
+
+       /*
         * We must free the requests after all the corresponding objects

have
* been moved off active lists. Which is the same order as the
normal
* retire_requests function does. This is important if object hold

drivers/gpu/drm/i915/i915_gem.c +0 -12

@@ -2570,18 +2593,6 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
                i915_gem_free_request(request);
        }
 
-       while (!list_empty(&ring->execlist_queue)) {
-               struct intel_ctx_submit_request *submit_req;
-
-               submit_req = list_first_entry(&ring->execlist_queue,
-                               struct intel_ctx_submit_request,
-                               execlist_link);
-               list_del(&submit_req->execlist_link);
-               intel_runtime_pm_put(dev_priv);
-               i915_gem_context_unreference(submit_req->ctx);
-               kfree(submit_req);
-       }
-
        /* These may not have been flush before the reset, do so now */
        kfree(ring->preallocated_lazy_request);

        ring->preallocated_lazy_request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +39 -2

index 906b985..f7fa0f7 100644
@@ -139,8 +139,6 @@
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_OTHER_SIZE (2 * PAGE_SIZE)
 
-#define GEN8_LR_CONTEXT_ALIGN 4096
-
 #define RING_EXECLIST_QFULL            (1 << 0x2)
 #define RING_EXECLIST1_VALID           (1 << 0x3)
 #define RING_EXECLIST0_VALID           (1 << 0x4)

@@ -801,9 +799,40 @@ void intel_logical_ring_advance_and_submit(struct
 intel_ringbuffer *ringbuf)
        execlists_context_queue(ring, ctx, ringbuf->tail);
 }
 
+static int intel_lr_context_pin(struct intel_engine_cs *ring,
+               struct intel_context *ctx)
+{
+       struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+       int ret = 0;
+
+       WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+       if (ctx->engine[ring->id].unpin_count++ == 0) {
+               ret = i915_gem_obj_ggtt_pin(ctx_obj,
+                               GEN8_LR_CONTEXT_ALIGN, 0);
+               if (ret)
+                       ctx->engine[ring->id].unpin_count = 0;
+       }
+
+       return ret;
+}
+
+void intel_lr_context_unpin(struct intel_engine_cs *ring,
+               struct intel_context *ctx)
+{
+       struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+
+       if (ctx_obj) {
+               WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+               if (--ctx->engine[ring->id].unpin_count == 0)
+                       i915_gem_object_ggtt_unpin(ctx_obj);
+       }
+}
+
 static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
                                    struct intel_context *ctx)
 {
+       int ret;
+
        if (ring->outstanding_lazy_seqno)
                return 0;

@@ -814,6 +843,14 @@ static int logical_ring_alloc_seqno(struct
 intel_engine_cs *ring,
                if (request == NULL)
                        return -ENOMEM;
 
+               if (ctx != ring->default_context) {
+                       ret = intel_lr_context_pin(ring, ctx);
+                       if (ret) {
+                               kfree(request);
+                               return ret;
+                       }
+               }
+
                /* Hold a reference to the context this request belongs to
                 * (we will need it when the time comes to emit/retire the

* request).

drivers/gpu/drm/i915/intel_lrc.c +6 -2

@@ -1626,12 +1663,16 @@ void intel_lr_context_free(struct intel_context
 *ctx)
 
        for (i = 0; i < I915_NUM_RINGS; i++) {
                struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
-               struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
 
                if (ctx_obj) {
+                       struct intel_ringbuffer *ringbuf =
+                                       ctx->engine[i].ringbuf;
+                       struct intel_engine_cs *ring = ringbuf->ring;
+
                        intel_destroy_ringbuffer_obj(ringbuf);
                        kfree(ringbuf);
-                       i915_gem_object_ggtt_unpin(ctx_obj);
+                       if (ctx == ring->default_context)
+                               i915_gem_object_ggtt_unpin(ctx_obj);
                        drm_gem_object_unreference(&ctx_obj->base);
                }

        }

drivers/gpu/drm/i915/intel_lrc.c +1 -0

@@ -1695,6 +1736,7 @@ static int lrc_setup_hardware_status_page(struct
 intel_engine_cs *ring,
 int intel_lr_context_deferred_create(struct intel_context *ctx,
                                     struct intel_engine_cs *ring)
 {
+       const bool is_global_default_ctx = (ctx == ring->default_context);
        struct drm_device *dev = ring->dev;
        struct drm_i915_gem_object *ctx_obj;

        uint32_t context_size;

drivers/gpu/drm/i915/intel_lrc.c +10 -6

@@ -1714,18 +1756,22 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
                return ret;
        }
 
-       ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN, 0);
-       if (ret) {
-               DRM_DEBUG_DRIVER("Pin LRC backing obj failed: %d\n", ret);
-               drm_gem_object_unreference(&ctx_obj->base);
-               return ret;
+       if (is_global_default_ctx) {
+               ret = i915_gem_obj_ggtt_pin(ctx_obj,
 GEN8_LR_CONTEXT_ALIGN, 0);
+               if (ret) {
+                       DRM_DEBUG_DRIVER("Pin LRC backing obj failed:
 %d\n",
+                                       ret);
+                       drm_gem_object_unreference(&ctx_obj->base);
+                       return ret;
+               }
        }
 
        ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
        if (!ringbuf) {
                DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
                                ring->name);
-               i915_gem_object_ggtt_unpin(ctx_obj);
+               if (is_global_default_ctx)
+                       i915_gem_object_ggtt_unpin(ctx_obj);

                drm_gem_object_unreference(&ctx_obj->base);
                ret = -ENOMEM;
                return ret;

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -1787,7 +1833,8 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
 
 error:
        kfree(ringbuf);
-       i915_gem_object_ggtt_unpin(ctx_obj);
+       if (is_global_default_ctx)
+               i915_gem_object_ggtt_unpin(ctx_obj);
        drm_gem_object_unreference(&ctx_obj->base);
        return ret;

}

drivers/gpu/drm/i915/intel_lrc.h +4 -0

index 84bbf19..14b216b 100644
@@ -24,6 +24,8 @@
 #ifndef _INTEL_LRC_H_
 #define _INTEL_LRC_H_
 
+#define GEN8_LR_CONTEXT_ALIGN 4096
+
 /* Execlists regs */
 #define RING_ELSP(ring)                        ((ring)->mmio_base+0x230)
 #define RING_EXECLIST_STATUS(ring)     ((ring)->mmio_base+0x234)

@@ -67,6 +69,8 @@ int intel_lr_context_render_state_init(struct
 intel_engine_cs *ring,
 void intel_lr_context_free(struct intel_context *ctx);
 int intel_lr_context_deferred_create(struct intel_context *ctx,
                                     struct intel_engine_cs *ring);
+void intel_lr_context_unpin(struct intel_engine_cs *ring,
+               struct intel_context *ctx);
 
 /* Execlists */

int intel_sanitize_enable_execlists(struct drm_device *dev, int
enable_execlists);
--
1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Monday, November 17, 2014 2:56 PM
To: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Here is the actual review...

_____________________________________________
From: Daniel, Thomas
Sent: Wednesday, November 12, 2014 8:52 PM
To: Goel, Akash
Subject: RE: Execlists patches code review

Hi Akash,

I will put the WARN messages back in and remove the need_unpin. The elsp_submitted count does not behave exactly as you would expect because of some race condition.
Have a look at the patch “Avoid non-lite-restore preemptions” by Oscar Mateo for a description.

Thanks,
Thomas.
_____________________________________________
From: Goel, Akash
Sent: Tuesday, November 11, 2014 4:37 PM
To: Daniel, Thomas
Subject: RE: Execlists patches code review

Hi Thomas,

Few comments on http://patchwork.freedesktop.org/patch/35830/

	int elsp_submitted;

(quoted diff) +1 -0

+	bool need_unpin;

This new field has not been used anywhere.

		if (intel_execlists_ctx_id(ctx_obj) == request_id) {

(quoted diff) +0 -2

-			WARN(head_req->elsp_submitted == 0,
-			     "Never submitted head request\n");

Sorry couldn’t get this change. Even if a request has been merged, still the elsp_submitted count should not be 0 here, when this function is executed on arrival of Context switch interrupt. When a new request is merged with a previously submitted request, the original value of elsp_submitted is still retained.

(quoted diff) +1 -0

+			/* If the request has been merged, it is possible to

get

(quoted diff) +1 -0

+			 * here with an unsubmitted request. */
 			if (--head_req->elsp_submitted <= 0) {

		if (status & GEN8_CTX_STATUS_PREEMPTED) {

(quoted diff) +0 -1

 			if (status & GEN8_CTX_STATUS_LITE_RESTORE) {
-				if (execlists_check_remove_request(ring,

status_id))

(quoted diff) +0 -1

-					WARN(1, "Lite Restored request

removed from queue\n");

(quoted diff) +1 -0

+				execlists_check_remove_request(ring,

status_id);

Same doubt here, thought that in this case of interrupt due to Preemption (Lite restore), which will occur when the same Context is submitted as the one already being executed by the Hw, the count will not drop to 0. Count will drop to 0 when the context switch interrupt will be generated subsequently.

Best regards
Akash
_____________________________________________
From: Goel, Akash
Sent: Tuesday, November 11, 2014 8:58 PM
To: Daniel, Thomas
Subject: RE: Execlists patches code review

Hi Thomas,

I was OOP today, I will provide this review comment tomorrow on the GFX mailing list.

Best regards
Akash
_____________________________________________
From: Daniel, Thomas
Sent: Monday, November 10, 2014 10:41 PM
To: Goel, Akash
Subject: RE: Execlists patches code review

Hi Akash,

Please post this comment to the mailing list.
Assuming nobody else comments I will remove the unpin_lock and replace the mutex_lock(&unpin_lock) with WARN_ON(!mutex_is_locked(&dev-

Quoted 1 line

struct_mutex)).

Cheers,
Thomas.

_____________________________________________
From: Goel, Akash
Sent: Monday, November 10, 2014 11:19 AM
To: Daniel, Thomas
Subject: RE: Execlists patches code review

In context of the 3rd patch http://patchwork.freedesktop.org/patch/35829/
intel_lr_context_pin is being called from logical_ring_alloc_seqno function and intel_lr_context_unpin gets called from i915_gem_free_request & i915_gem_reset_ring_cleanup functions

All these 3 paths are already protected by dev->struct_mutex (Global lock), so they will always execute sequentially with respect to each other.

Do we need to have a new lock ?

(quoted diff) +1 -0

+		struct mutex unpin_lock;

Best regards
Akash
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

On Thu, Nov 13, 2014 at 10:28:10AM +0000, Thomas Daniel wrote:

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

And yes I've only realized this now that you've supplied the review comments from Akash. I really rely upon the review discussions to spot such low-level implementation details.

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

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 10 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

On Thu, Nov 13, 2014 at 10:28:10AM +0000, Thomas Daniel wrote:

Quoted 11 lines

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Quoted 3 lines

And yes I've only realized this now that you've supplied the review comments from Akash. I really rely upon the review discussions to spot such low-level implementation details.

I know, and I explicitly asked the guys to post comments to the mailing list.

Cheers,
Thomas.

Quoted 5 lines

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

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 31 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 21 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 9 lines · Thomas Daniel

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

Quoted 2 lines

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Actually I just tried to implement this, it causes a problem for patch 4 of this set as the unpin_count is also used for the ringbuffer object which has an ioremap as well as a ggtt pin.

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 2:33 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 31 lines · Thomas

Quoted 31 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 21 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 9 lines · Thomas Daniel

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

Quoted 2 lines

Quoted 2 lines

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Those 4 bits are good enough for legacy contexts, so I wonder a bit what's so massively different for execlist contexts.

Quoted 3 lines

Actually I just tried to implement this, it causes a problem for patch 4 of this set as the unpin_count is also used for the ringbuffer object which has an ioremap as well as a ggtt pin.

Yeah, ioremap needs to be redone every time we pin/unpin. But on sane archs it's almost no overhead really. And if this does start to matter (shudder for 32bit kernels on gen8) then we can fix it ...

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

Quoted 44 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 2:33 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 33 lines · Thomas

Quoted 33 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 10 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

On Thu, Nov 13, 2014 at 10:28:10AM +0000, Thomas Daniel wrote:

Quoted 11 lines

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Quoted 2 lines

Those 4 bits are good enough for legacy contexts, so I wonder a bit what's so massively different for execlist contexts.

With execlists, in order to dynamically unpin the LRC backing object and ring buffer object when not required we take a reference for each execlist request that uses them (remember that the execlist request lifecycle is currently different from the execbuffer request). This can be a lot, especially in some of the less sane i-g-t tests.

Quoted 7 lines

Quoted 3 lines

Actually I just tried to implement this, it causes a problem for patch 4 of this set as the unpin_count is also used for the ringbuffer object which has an ioremap as well as a ggtt pin.

Yeah, ioremap needs to be redone every time we pin/unpin. But on sane archs it's almost no overhead really. And if this does start to matter (shudder for 32bit kernels on gen8) then we can fix it ...

Hm, so the CPU vaddr of the ring buffer will move around as more requests reference it which I suppose is not a problem. We will use a lot of address space (again, especially with the i-g-t stress tests which can submit tens of thousands of requests in a very short space of time). What would the fix be? An extra reference count for the ioremap? Looks familiar :)

I still think it's best to keep the context unpin_count for execlists mode.

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 3:11 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 45 lines · Thomas

Quoted 45 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 2:33 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 34 lines · Thomas

Quoted 34 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 10 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

On Thu, Nov 13, 2014 at 10:28:10AM +0000, Thomas Daniel wrote:

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 10 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Quoted 7 lines

Quoted 2 lines

Those 4 bits are good enough for legacy contexts, so I wonder a bit what's so massively different for execlist contexts.

With execlists, in order to dynamically unpin the LRC backing object and ring buffer object when not required we take a reference for each execlist request that uses them (remember that the execlist request lifecycle is currently different from the execbuffer request). This can be a lot, especially in some of the less sane i-g-t tests.

Why?

Presuming the buffer objects is properly pushed onto the active list you only need to pin while doing the command submission up to the point where you've committed the buffer object to the active list.

I know documentation sucks for this stuff since I have this discussion with roughly everyone ever touching anything related to active buffers :( If you want some recent examples the cmd parser's shadow batch should serve well (including the entire evolution from reinvented wheel to just using the active list, although the latest patches are only 90% there and still have 1-2 misplaced pieces).

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Actually I just tried to implement this, it causes a problem for patch
4 of this set as the unpin_count is also used for the ringbuffer object which has an ioremap as well as a ggtt pin.

Yeah, ioremap needs to be redone every time we pin/unpin. But on sane archs it's almost no overhead really. And if this does start to matter (shudder for 32bit kernels on gen8) then we can fix it ...

Quoted 6 lines

Hm, so the CPU vaddr of the ring buffer will move around as more requests reference it which I suppose is not a problem. We will use a lot of address space (again, especially with the i-g-t stress tests which can submit tens of thousands of requests in a very short space of time). What would the fix be? An extra reference count for the ioremap? Looks familiar :)

ioremap always gives you the same linear address on 64bit kernels. On 32bit it makes a new one, but if you ioremap for each request it'll fall over anyway.

The solution would be to ioremap just the required pages using the atomic kmap stuff wrapped up into the io_mapping stuff.

Quoted 1 line

I still think it's best to keep the context unpin_count for execlists mode.

Well just means the todo-list to fix up execlist grows longer.

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

Quoted 63 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 3:11 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 52 lines · Thomas

Quoted 42 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 18, 2014 2:33 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 32 lines · Thomas

Quoted 32 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel, Thomas
Sent: Tuesday, November 18, 2014 9:28 AM
To: Daniel Vetter
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 22 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 17, 2014 6:09 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 658

Quoted 10 lines · Thomas Daniel

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.h index 059330c..3c7299d 100644

drivers/gpu/drm/i915/i915_drv.h +1 -0

@@ -655,6 +655,7 @@ struct intel_context {
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
+		int unpin_count;

Pinning is already refcounted. Why this additional refcount?

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The vma.pin_count is only allocated 4 bits of storage. If this restriction can be lifted then I can use that.

Those 4 bits are good enough for legacy contexts, so I wonder a bit what's so massively different for execlist contexts.

With execlists, in order to dynamically unpin the LRC backing object and ring buffer object when not required we take a reference for each execlist request that uses them (remember that the execlist request lifecycle is currently different from the execbuffer request). This can be a lot, especially in some of the less sane i-g-t tests.

Quoted 5 lines

Why?

Presuming the buffer objects is properly pushed onto the active list you only need to pin while doing the command submission up to the point where you've committed the buffer object to the active list.

This is not currently the case. Using the active list for context object management is one of the refactoring tasks, as we agreed.

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

v5: Remove unpin_lock as all pinning and unpinning is done with the struct mutex already locked. Add WARN_ONs to make sure this is the case in future.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, November 24, 2014 2:25 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

Quoted 18 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Up until now, we have pinned every logical ring context backing object during creation, and left it pinned until destruction. This made my life easier, but it's a harmful thing to do, because we cause fragmentation of the GGTT (and, eventually, we would run out of space).

This patch makes the pinning on-demand: the backing objects of the two contexts that are written to the ELSP are pinned right before submission and unpinned once the hardware is done with them. The only context that is still pinned regardless is the global default one, so that the HWS can still be accessed in the same way (ring->status_page).

v2: In the early version of this patch, we were pinning the context as we put it into the ELSP: on the one hand, this is very efficient because only a maximum two contexts are pinned at any given time, but on the other hand, we cannot really pin in interrupt time :(

Quoted 8 lines

v3: Use a mutex rather than atomic_t to protect pin count to avoid races. Do not unpin default context in free_request.

v4: Break out pin and unpin into functions. Fix style problems reported by checkpatch

v5: Remove unpin_lock as all pinning and unpinning is done with the struct mutex already locked. Add WARN_ONs to make sure this is the case

in future.

Quoted 4 lines

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

This patch here scored a regression (leak in the module unload path), please address it asap. Deadline for regressions should be 1 week, then I'll just drop the patch or apply the revert. That includes review and everything.

https://bugs.freedesktop.org/show_bug.cgi?id=86507

Quoted 5 lines

This patch here scored a regression (leak in the module unload path), please address it asap. Deadline for regressions should be 1 week, then I'll just drop the patch or apply the revert. That includes review and everything.

https://bugs.freedesktop.org/show_bug.cgi?id=86507

Leak identified. The fix is simple.
Do you want a v6 or a follow-up patch?

Same as with the context, pinning to GGTT regardless is harmful (it badly fragments the GGTT and can even exhaust it).

Unfortunately, this case is also more complex than the previous one because we need to map and access the ringbuffer in several places along the execbuffer path (and we cannot make do by leaving the default ringbuffer pinned, as before). Also, the context object itself contains a pointer to the ringbuffer address that we have to keep updated if we are going to allow the ringbuffer to move around.

v2: Same as with the context pinning, we cannot really do it during an interrupt. Also, pin the default ringbuffers objects regardless (makes error capture a lot easier).

v3: Rebased. Take a pin reference of the ringbuffer for each item in the execlist request queue because the hardware may still be using the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update is executed. The ringbuffer must remain pinned until the context save is complete. No longer pin and unpin ringbuffer in populate_lr_context() - this transient address is meaningless and the pinning can cause a sleep while atomic.

v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. Downgraded pinning check BUG_ONs to WARN_ONs.

v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused variable.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
  drivers/gpu/drm/i915/intel_lrc.c        |  102 +++++++++++++++++++++++--------
  drivers/gpu/drm/i915/intel_ringbuffer.c |   85 +++++++++++++++-----------
  drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +
  3 files changed, 128 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_lrc.c +39 -7

index f7fa0f7..ca20f91 100644
@@ -202,6 +202,9 @@ enum {
  };
  #define GEN8_CTX_ID_SHIFT 32
 
+static int intel_lr_context_pin(struct intel_engine_cs *ring,
+		struct intel_context *ctx);
+
  /**
   * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
   * @dev: DRM device.

@@ -339,7 +342,9 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
  }
 
-static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tail)
+static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
+				    struct drm_i915_gem_object *ring_obj,
+				    u32 tail)
  {
  	struct page *page;
  	uint32_t *reg_state;

@@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tai
  	reg_state = kmap_atomic(page);
 
  	reg_state[CTX_RING_TAIL+1] = tail;
+	reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj);
 
  	kunmap_atomic(reg_state);
 

@@ -358,21 +364,25 @@ static int execlists_submit_context(struct intel_engine_cs *ring,
  				    struct intel_context *to0, u32 tail0,
  				    struct intel_context *to1, u32 tail1)
  {
-	struct drm_i915_gem_object *ctx_obj0;
+	struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state;
+	struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf;
  	struct drm_i915_gem_object *ctx_obj1 = NULL;
+	struct intel_ringbuffer *ringbuf1 = NULL;
 
-	ctx_obj0 = to0->engine[ring->id].state;
  	BUG_ON(!ctx_obj0);
  	WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
+	WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
 
-	execlists_ctx_write_tail(ctx_obj0, tail0);
+	execlists_update_context(ctx_obj0, ringbuf0->obj, tail0);
 
  	if (to1) {
+		ringbuf1 = to1->engine[ring->id].ringbuf;
  		ctx_obj1 = to1->engine[ring->id].state;
  		BUG_ON(!ctx_obj1);
  		WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1));
+		WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj));
 
-		execlists_ctx_write_tail(ctx_obj1, tail1);
+		execlists_update_context(ctx_obj1, ringbuf1->obj, tail1);
  	}
 
  	execlists_elsp_write(ring, ctx_obj0, ctx_obj1);

@@ -524,6 +534,10 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
  		return -ENOMEM;
  	req->ctx = to;
  	i915_gem_context_reference(req->ctx);
+
+	if (to != ring->default_context)
+		intel_lr_context_pin(ring, to);
+
  	req->ring = ring;
  	req->tail = tail;
 

@@ -544,7 +558,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
 
  		if (to == tail_req->ctx) {
  			WARN(tail_req->elsp_submitted != 0,
-			     "More than 2 already-submitted reqs queued\n");
+				"More than 2 already-submitted reqs queued\n");
  			list_del(&tail_req->execlist_link);
  			list_add_tail(&tail_req->execlist_link,
  				&ring->execlist_retired_req_list);

@@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
  	spin_unlock_irqrestore(&ring->execlist_lock, flags);
 
  	list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
+		struct intel_context *ctx = req->ctx;
+		struct drm_i915_gem_object *ctx_obj =
+				ctx->engine[ring->id].state;
+
+		if (ctx_obj && (ctx != ring->default_context))
+			intel_lr_context_unpin(ring, ctx);
  		intel_runtime_pm_put(dev_priv);
  		i915_gem_context_unreference(req->ctx);
  		list_del(&req->execlist_link);

@@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring,
  		struct intel_context *ctx)
  {
  	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
  	int ret = 0;
 
  	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

@@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring,
  		ret = i915_gem_obj_ggtt_pin(ctx_obj,
  				GEN8_LR_CONTEXT_ALIGN, 0);
  		if (ret)
-			ctx->engine[ring->id].unpin_count = 0;
+			goto reset_unpin_count;
+
+		ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
+		if (ret)
+			goto unpin_ctx_obj;
  	}
 
  	return ret;
+
+unpin_ctx_obj:
+	i915_gem_object_ggtt_unpin(ctx_obj);
+reset_unpin_count:
+	ctx->engine[ring->id].unpin_count = 0;
+
+	return ret;

drivers/gpu/drm/i915/intel_lrc.c +35 -20

-		if (--ctx->engine[ring->id].unpin_count == 0)
+		if (--ctx->engine[ring->id].unpin_count == 0) {
+			intel_unpin_ringbuffer_obj(ringbuf);
  			i915_gem_object_ggtt_unpin(ctx_obj);
+		}
  	}
  }
 

@@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o
  {
  	struct drm_device *dev = ring->dev;
  	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *ring_obj = ringbuf->obj;
  	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
  	struct page *page;
  	uint32_t *reg_state;

@@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o
  	reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base);
  	reg_state[CTX_RING_TAIL+1] = 0;
  	reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base);
-	reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj);
+	/* Ring buffer start address is not known until the buffer is pinned.
+	 * It is written to the context image in execlists_update_context()
+	 */
  	reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base);
  	reg_state[CTX_RING_BUFFER_CONTROL+1] =
  			((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID;

@@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct intel_context *ctx)
  					ctx->engine[i].ringbuf;
  			struct intel_engine_cs *ring = ringbuf->ring;
 
+			if (ctx == ring->default_context) {
+				intel_unpin_ringbuffer_obj(ringbuf);
+				i915_gem_object_ggtt_unpin(ctx_obj);
+			}
  			intel_destroy_ringbuffer_obj(ringbuf);
  			kfree(ringbuf);
-			if (ctx == ring->default_context)
-				i915_gem_object_ggtt_unpin(ctx_obj);
  			drm_gem_object_unreference(&ctx_obj->base);
  		}
  	}

@@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
  	if (!ringbuf) {
  		DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
  				ring->name);
-		if (is_global_default_ctx)
-			i915_gem_object_ggtt_unpin(ctx_obj);
-		drm_gem_object_unreference(&ctx_obj->base);
  		ret = -ENOMEM;
-		return ret;
+		goto error_unpin_ctx;
  	}
 
  	ringbuf->ring = ring;

@@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
  	ringbuf->space = ringbuf->size;
  	ringbuf->last_retired_head = -1;
 
-	/* TODO: For now we put this in the mappable region so that we can reuse
-	 * the existing ringbuffer code which ioremaps it. When we start
-	 * creating many contexts, this will no longer work and we must switch
-	 * to a kmapish interface.
-	 */
-	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-	if (ret) {
-		DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n",
+	if (ringbuf->obj == NULL) {
+		ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+		if (ret) {
+			DRM_DEBUG_DRIVER(
+				"Failed to allocate ringbuffer obj %s: %d\n",
  				ring->name, ret);
-		goto error;
+			goto error_free_rbuf;
+		}
+
+		if (is_global_default_ctx) {
+			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_destroy_rbuf;
+			}
+		}
+
  	}
 
  	ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
  	if (ret) {
  		DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
-		intel_destroy_ringbuffer_obj(ringbuf);
  		goto error;
  	}
 

@@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
  			DRM_ERROR("Init render state failed: %d\n", ret);
  			ctx->engine[ring->id].ringbuf = NULL;
  			ctx->engine[ring->id].state = NULL;
-			intel_destroy_ringbuffer_obj(ringbuf);
  			goto error;
  		}
  		ctx->rcs_initialized = true;

@@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
  	return 0;
 
  error:
+	if (is_global_default_ctx)
+		intel_unpin_ringbuffer_obj(ringbuf);
+error_destroy_rbuf:
+	intel_destroy_ringbuffer_obj(ringbuf);
+error_free_rbuf:
  	kfree(ringbuf);
+error_unpin_ctx:
  	if (is_global_default_ctx)
  		i915_gem_object_ggtt_unpin(ctx_obj);
  	drm_gem_object_unreference(&ctx_obj->base);

drivers/gpu/drm/i915/intel_ringbuffer.c +50 -35

index a8f72e8..0c4aab1 100644
@@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
  	return 0;
  }
 
-void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
  {
-	if (!ringbuf->obj)
-		return;
-
  	iounmap(ringbuf->virtual_start);
+	ringbuf->virtual_start = NULL;
  	i915_gem_object_ggtt_unpin(ringbuf->obj);
+}
+
+int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
+				     struct intel_ringbuffer *ringbuf)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_gem_object *obj = ringbuf->obj;
+	int ret;
+
+	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, true);
+	if (ret) {
+		i915_gem_object_ggtt_unpin(obj);
+		return ret;
+	}
+
+	ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
+			i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+	if (ringbuf->virtual_start == NULL) {
+		i915_gem_object_ggtt_unpin(obj);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+{
  	drm_gem_object_unreference(&ringbuf->obj->base);
  	ringbuf->obj = NULL;
  }

@@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
  int intel_alloc_ringbuffer_obj(struct drm_device *dev,
  			       struct intel_ringbuffer *ringbuf)
  {
-	struct drm_i915_private *dev_priv = to_i915(dev);
  	struct drm_i915_gem_object *obj;
-	int ret;
-
-	if (ringbuf->obj)
-		return 0;
 
  	obj = NULL;
  	if (!HAS_LLC(dev))

@@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct drm_device *dev,
  	/* mark ring buffers as read-only from GPU side by default */
  	obj->gt_ro = 1;
 
-	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
-	if (ret)
-		goto err_unref;
-
-	ret = i915_gem_object_set_to_gtt_domain(obj, true);
-	if (ret)
-		goto err_unpin;
-
-	ringbuf->virtual_start =
-		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
-				ringbuf->size);
-	if (ringbuf->virtual_start == NULL) {
-		ret = -EINVAL;
-		goto err_unpin;
-	}
-
  	ringbuf->obj = obj;
-	return 0;
 
-err_unpin:
-	i915_gem_object_ggtt_unpin(obj);
-err_unref:
-	drm_gem_object_unreference(&obj->base);
-	return ret;
+	return 0;
  }
 
  static int intel_init_ring_buffer(struct drm_device *dev,

@@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct drm_device *dev,
  			goto error;
  	}
 
-	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-	if (ret) {
-		DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret);
-		goto error;
+	if (ringbuf->obj == NULL) {
+		ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+		if (ret) {
+			DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
+					ring->name, ret);
+			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;
+		}
  	}
 
  	/* Workaround an erratum on the i830 which causes a hang if

@@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
  	intel_stop_ring_buffer(ring);
  	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
+	intel_unpin_ringbuffer_obj(ringbuf);
  	intel_destroy_ringbuffer_obj(ringbuf);
  	ring->preallocated_lazy_request = NULL;
  	ring->outstanding_lazy_seqno = 0;

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

index 8c002d2..365854ad 100644

Quoted 162 lines · Thomas Daniel

Same as with the context, pinning to GGTT regardless is harmful (it badly fragments the GGTT and can even exhaust it).

Unfortunately, this case is also more complex than the previous one because we need to map and access the ringbuffer in several places along the execbuffer path (and we cannot make do by leaving the default ringbuffer pinned, as before). Also, the context object itself contains a pointer to the ringbuffer address that we have to keep updated if we are going to allow the ringbuffer to move around.

v2: Same as with the context pinning, we cannot really do it during an interrupt. Also, pin the default ringbuffers objects regardless (makes error capture a lot easier).

v3: Rebased. Take a pin reference of the ringbuffer for each item in the execlist request queue because the hardware may still be using the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update is executed. The ringbuffer must remain pinned until the context save is complete. No longer pin and unpin ringbuffer in populate_lr_context() - this transient address is meaningless and the pinning can cause a sleep while atomic.

v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. Downgraded pinning check BUG_ONs to WARN_ONs.

v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused variable.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
  drivers/gpu/drm/i915/intel_lrc.c        |  102 
+++++++++++++++++++++++--------
  drivers/gpu/drm/i915/intel_ringbuffer.c |   85 
+++++++++++++++-----------
  drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +
  3 files changed, 128 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_lrc.c +5 -0

index f7fa0f7..ca20f91 100644
@@ -202,6 +202,9 @@ enum {
  };
  #define GEN8_CTX_ID_SHIFT 32
  +static int intel_lr_context_pin(struct intel_engine_cs *ring,
+        struct intel_context *ctx);
+
  /**
   * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
   * @dev: DRM device.

@@ -339,7 +342,9 @@ static void execlists_elsp_write(struct 
 intel_engine_cs *ring,
      spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
  }
  -static int execlists_ctx_write_tail(struct drm_i915_gem_object 
 *ctx_obj, u32 tail)
+static int execlists_update_context(struct drm_i915_gem_object 
 *ctx_obj,
+                    struct drm_i915_gem_object *ring_obj,
+                    u32 tail)
  {
      struct page *page;
      uint32_t *reg_state;

@@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct 

drm_i915_gem_object *ctx_obj, u32 tai

      reg_state = kmap_atomic(page);
        reg_state[CTX_RING_TAIL+1] = tail;
+    reg_state[CTX_RING_BUFFER_START+1] = 
i915_gem_obj_ggtt_offset(ring_obj);
        kunmap_atomic(reg_state);

@@ -358,21 +364,25 @@ static int execlists_submit_context(struct
intel_engine_cs *ring,
struct intel_context *to0, u32 tail0, struct intel_context *to1, u32 tail1)
{

drivers/gpu/drm/i915/intel_lrc.c +12 -1

-    struct drm_i915_gem_object *ctx_obj0;
+    struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state;
+    struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf;
      struct drm_i915_gem_object *ctx_obj1 = NULL;
+    struct intel_ringbuffer *ringbuf1 = NULL;
  -    ctx_obj0 = to0->engine[ring->id].state;
      BUG_ON(!ctx_obj0);
      WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
+    WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
  -    execlists_ctx_write_tail(ctx_obj0, tail0);
+    execlists_update_context(ctx_obj0, ringbuf0->obj, tail0);
        if (to1) {
+        ringbuf1 = to1->engine[ring->id].ringbuf;
          ctx_obj1 = to1->engine[ring->id].state;
          BUG_ON(!ctx_obj1);
          WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1));
+        WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj));
  -        execlists_ctx_write_tail(ctx_obj1, tail1);
+        execlists_update_context(ctx_obj1, ringbuf1->obj, tail1);
      }
        execlists_elsp_write(ring, ctx_obj0, ctx_obj1);

@@ -524,6 +534,10 @@ static int execlists_context_queue(struct 
 intel_engine_cs *ring,
          return -ENOMEM;
      req->ctx = to;
      i915_gem_context_reference(req->ctx);
+
+    if (to != ring->default_context)
+        intel_lr_context_pin(ring, to);
+
      req->ring = ring;
      req->tail = tail;

@@ -544,7 +558,7 @@ static int execlists_context_queue(struct
intel_engine_cs *ring,

            if (to == tail_req->ctx) {
              WARN(tail_req->elsp_submitted != 0,

drivers/gpu/drm/i915/intel_lrc.c +7 -1

-                 "More than 2 already-submitted reqs queued\n");
+                "More than 2 already-submitted reqs queued\n");
              list_del(&tail_req->execlist_link);
              list_add_tail(&tail_req->execlist_link,
                  &ring->execlist_retired_req_list);

@@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct 
 intel_engine_cs *ring)
      spin_unlock_irqrestore(&ring->execlist_lock, flags);
        list_for_each_entry_safe(req, tmp, &retired_list, 
 execlist_link) {
+        struct intel_context *ctx = req->ctx;
+        struct drm_i915_gem_object *ctx_obj =
+                ctx->engine[ring->id].state;
+
+        if (ctx_obj && (ctx != ring->default_context))
+            intel_lr_context_unpin(ring, ctx);
          intel_runtime_pm_put(dev_priv);
          i915_gem_context_unreference(req->ctx);

          list_del(&req->execlist_link);

drivers/gpu/drm/i915/intel_lrc.c +13 -1

@@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct 
 intel_engine_cs *ring,
          struct intel_context *ctx)
  {
      struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+    struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
      int ret = 0;
 WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

@@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct 
 intel_engine_cs *ring,
          ret = i915_gem_obj_ggtt_pin(ctx_obj,
                  GEN8_LR_CONTEXT_ALIGN, 0);
          if (ret)
-            ctx->engine[ring->id].unpin_count = 0;
+            goto reset_unpin_count;
+
+        ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
+        if (ret)
+            goto unpin_ctx_obj;
      }
        return ret;
+
+unpin_ctx_obj:
+    i915_gem_object_ggtt_unpin(ctx_obj);
+reset_unpin_count:
+    ctx->engine[ring->id].unpin_count = 0;
+
+    return ret;
  }
    void intel_lr_context_unpin(struct intel_engine_cs *ring,

drivers/gpu/drm/i915/intel_lrc.c static int intel_lr_context_pin(struct intel_engine_cs *ring, · line 858

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

          struct intel_context *ctx)
  {
      struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+    struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
        if (ctx_obj) {
 WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

With pin specific mutex from previous patch set removed

Reviewed-by Deepak S<[email protected]>

Quoted 265 lines

drivers/gpu/drm/i915/intel_lrc.c +3 -1

-        if (--ctx->engine[ring->id].unpin_count == 0)
+        if (--ctx->engine[ring->id].unpin_count == 0) {
+            intel_unpin_ringbuffer_obj(ringbuf);
              i915_gem_object_ggtt_unpin(ctx_obj);
+        }
      }
  }
  @@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context 
 *ctx, struct drm_i915_gem_object *ctx_o
  {

      struct drm_device *dev = ring->dev;
      struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_lrc.c +0 -1

-    struct drm_i915_gem_object *ring_obj = ringbuf->obj;
      struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
      struct page *page;
      uint32_t *reg_state;

@@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, 

struct drm_i915_gem_object *ctx_o

      reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base);
      reg_state[CTX_RING_TAIL+1] = 0;
      reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base);

- reg_state[CTX_RING_BUFFER_START+1] =
i915_gem_obj_ggtt_offset(ring_obj);
+ /* Ring buffer start address is not known until the buffer is
pinned.

drivers/gpu/drm/i915/intel_lrc.c +2 -0

+     * It is written to the context image in execlists_update_context()
+     */
      reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base);
      reg_state[CTX_RING_BUFFER_CONTROL+1] =
              ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | 

RING_VALID;

drivers/gpu/drm/i915/intel_lrc.c +4 -6

@@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct 
 intel_context *ctx)
                      ctx->engine[i].ringbuf;
              struct intel_engine_cs *ring = ringbuf->ring;
  +            if (ctx == ring->default_context) {
+                intel_unpin_ringbuffer_obj(ringbuf);
+                i915_gem_object_ggtt_unpin(ctx_obj);
+            }
              intel_destroy_ringbuffer_obj(ringbuf);
              kfree(ringbuf);
-            if (ctx == ring->default_context)
-                i915_gem_object_ggtt_unpin(ctx_obj);
              drm_gem_object_unreference(&ctx_obj->base);
          }
      }

@@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct 
 intel_context *ctx,
      if (!ringbuf) {
          DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
                  ring->name);
-        if (is_global_default_ctx)
-            i915_gem_object_ggtt_unpin(ctx_obj);
-        drm_gem_object_unreference(&ctx_obj->base);
          ret = -ENOMEM;
-        return ret;
+        goto error_unpin_ctx;
      }
        ringbuf->ring = ring;

@@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct 
 intel_context *ctx,
      ringbuf->space = ringbuf->size;
      ringbuf->last_retired_head = -1;
  -    /* TODO: For now we put this in the mappable region so that we 

can reuse

drivers/gpu/drm/i915/intel_lrc.c +0 -2

-     * the existing ringbuffer code which ioremaps it. When we start
-     * creating many contexts, this will no longer work and we must 

switch

drivers/gpu/drm/i915/intel_lrc.c +18 -7

-     * to a kmapish interface.
-     */
-    ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-    if (ret) {
-        DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n",
+    if (ringbuf->obj == NULL) {
+        ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+        if (ret) {
+            DRM_DEBUG_DRIVER(
+                "Failed to allocate ringbuffer obj %s: %d\n",
                  ring->name, ret);
-        goto error;
+            goto error_free_rbuf;
+        }
+
+        if (is_global_default_ctx) {
+            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_destroy_rbuf;
+            }
+        }
+
      }
        ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
      if (ret) {
          DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
-        intel_destroy_ringbuffer_obj(ringbuf);
          goto error;
      }
  @@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct 

intel_context *ctx,

              DRM_ERROR("Init render state failed: %d\n", ret);
              ctx->engine[ring->id].ringbuf = NULL;
              ctx->engine[ring->id].state = NULL;

drivers/gpu/drm/i915/intel_lrc.c +6 -1

-            intel_destroy_ringbuffer_obj(ringbuf);
              goto error;
          }
          ctx->rcs_initialized = true;

@@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct 
 intel_context *ctx,
      return 0;
    error:
+    if (is_global_default_ctx)
+        intel_unpin_ringbuffer_obj(ringbuf);
+error_destroy_rbuf:
+    intel_destroy_ringbuffer_obj(ringbuf);
+error_free_rbuf:
      kfree(ringbuf);
+error_unpin_ctx:
      if (is_global_default_ctx)
          i915_gem_object_ggtt_unpin(ctx_obj);
      drm_gem_object_unreference(&ctx_obj->base);

drivers/gpu/drm/i915/intel_ringbuffer.c +49 -31

index a8f72e8..0c4aab1 100644
@@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct 
 intel_engine_cs *ring)
      return 0;
  }
  -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
  {
-    if (!ringbuf->obj)
-        return;
-
      iounmap(ringbuf->virtual_start);
+    ringbuf->virtual_start = NULL;
      i915_gem_object_ggtt_unpin(ringbuf->obj);
+}
+
+int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
+                     struct intel_ringbuffer *ringbuf)
+{
+    struct drm_i915_private *dev_priv = to_i915(dev);
+    struct drm_i915_gem_object *obj = ringbuf->obj;
+    int ret;
+
+    ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
+    if (ret)
+        return ret;
+
+    ret = i915_gem_object_set_to_gtt_domain(obj, true);
+    if (ret) {
+        i915_gem_object_ggtt_unpin(obj);
+        return ret;
+    }
+
+    ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
+            i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+    if (ringbuf->virtual_start == NULL) {
+        i915_gem_object_ggtt_unpin(obj);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+{
      drm_gem_object_unreference(&ringbuf->obj->base);
      ringbuf->obj = NULL;
  }

@@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct 
 intel_ringbuffer *ringbuf)
  int intel_alloc_ringbuffer_obj(struct drm_device *dev,
                     struct intel_ringbuffer *ringbuf)
  {
-    struct drm_i915_private *dev_priv = to_i915(dev);
      struct drm_i915_gem_object *obj;
-    int ret;
-
-    if (ringbuf->obj)
-        return 0;
        obj = NULL;
      if (!HAS_LLC(dev))

@@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct 
 drm_device *dev,
      /* mark ring buffers as read-only from GPU side by default */
      obj->gt_ro = 1;
  -    ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
-    if (ret)
-        goto err_unref;
-
-    ret = i915_gem_object_set_to_gtt_domain(obj, true);
-    if (ret)
-        goto err_unpin;
-
-    ringbuf->virtual_start =
-        ioremap_wc(dev_priv->gtt.mappable_base + 
 i915_gem_obj_ggtt_offset(obj),
-                ringbuf->size);
-    if (ringbuf->virtual_start == NULL) {
-        ret = -EINVAL;
-        goto err_unpin;
-    }
-
      ringbuf->obj = obj;
-    return 0;
  -err_unpin:
-    i915_gem_object_ggtt_unpin(obj);
-err_unref:
-    drm_gem_object_unreference(&obj->base);
-    return ret;
+    return 0;
  }
    static int intel_init_ring_buffer(struct drm_device *dev,

@@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct 
 drm_device *dev,
              goto error;
      }
  -    ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-    if (ret) {
-        DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", 
 ring->name, ret);
-        goto error;
+    if (ringbuf->obj == NULL) {
+        ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+        if (ret) {
+            DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
+                    ring->name, ret);
+            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;
+        }
      }
        /* Workaround an erratum on the i830 which causes a hang if

@@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct 
 intel_engine_cs *ring)
      intel_stop_ring_buffer(ring);
      WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & 
 MODE_IDLE) == 0);
  +    intel_unpin_ringbuffer_obj(ringbuf);
      intel_destroy_ringbuffer_obj(ringbuf);
      ring->preallocated_lazy_request = NULL;
      ring->outstanding_lazy_seqno = 0;

drivers/gpu/drm/i915/intel_ringbuffer.h +2 -0

index 8c002d2..365854ad 100644
@@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs 
 *ring,
  #define I915_GEM_HWS_SCRATCH_INDEX    0x30
  #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX << 
 MI_STORE_DWORD_INDEX_SHIFT)
  +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
+int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
+                     struct intel_ringbuffer *ringbuf);

drivers/gpu/drm/i915/intel_ringbuffer.h intel_write_status_page(struct intel_engine_cs *ring, · line 390

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

  void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
  int intel_alloc_ringbuffer_obj(struct drm_device *dev,
                     struct intel_ringbuffer *ringbuf);

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 1 line · Deepak S

With pin specific mutex from previous patch set removed

Oops This comment was for previous patch in the series :( Since i reviewed the patch offline, comments got mixed :)

Quoted 4 lines · Deepak S

Quoted 1 line · Deepak S

With pin specific mutex from previous patch set removed

Oops This comment was for previous patch in the series :( Since i reviewed the patch offline, comments got mixed :)

Please forward these comments from the private discussion to the mailing list. Review isn't just about code correctness, but about communication - yes, I (and domain experts) actually read all this stuff that floats around and will jump into the discussion if there's something important or tricky being discussed.

Second reason for public review is that the important part about the r-b tag isn't that review happened, but by whom. So this is all about reputation building and playing to people's various strenght. And if you do review in private nothing of that can happen, which makes the review a lot less useful. So let's extract the most value from all that engineering time we invest into reviewing and _always_ do the review in public.

Thanks, Daniel

Same as with the context, pinning to GGTT regardless is harmful (it badly fragments the GGTT and can even exhaust it).

Unfortunately, this case is also more complex than the previous one because we need to map and access the ringbuffer in several places along the execbuffer path (and we cannot make do by leaving the default ringbuffer pinned, as before). Also, the context object itself contains a pointer to the ringbuffer address that we have to keep updated if we are going to allow the ringbuffer to move around.

v2: Same as with the context pinning, we cannot really do it during an interrupt. Also, pin the default ringbuffers objects regardless (makes error capture a lot easier).

v3: Rebased. Take a pin reference of the ringbuffer for each item in the execlist request queue because the hardware may still be using the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update is executed. The ringbuffer must remain pinned until the context save is complete. No longer pin and unpin ringbuffer in populate_lr_context() - this transient address is meaningless and the pinning can cause a sleep while atomic.

v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. Downgraded pinning check BUG_ONs to WARN_ONs.

v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused variable.

Issue: VIZ-4277

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |  102
+++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   85 +++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +
 3 files changed, 128 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_lrc.c +6 -1

index f7fa0f7..ca20f91 100644
@@ -202,6 +202,9 @@ enum {
 };
 #define GEN8_CTX_ID_SHIFT 32
 
+static int intel_lr_context_pin(struct intel_engine_cs *ring,
+               struct intel_context *ctx);
+
 /**
  * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
  * @dev: DRM device.

@@ -339,7 +342,9 @@ static void execlists_elsp_write(struct
 intel_engine_cs *ring,
        spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
 }
 
-static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj,
 u32 tail)
+static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
+                                   struct drm_i915_gem_object *ring_obj,
+                                   u32 tail)
 {

        struct page *page;
        uint32_t *reg_state;

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct

drm_i915_gem_object *ctx_obj, u32 tai

        reg_state = kmap_atomic(page);

        reg_state[CTX_RING_TAIL+1] = tail;

+ reg_state[CTX_RING_BUFFER_START+1] =
i915_gem_obj_ggtt_offset(ring_obj);

        kunmap_atomic(reg_state);

drivers/gpu/drm/i915/intel_lrc.c +8 -4

@@ -358,21 +364,25 @@ static int execlists_submit_context(struct
 intel_engine_cs *ring,
                                    struct intel_context *to0, u32 tail0,
                                    struct intel_context *to1, u32 tail1)
 {
-       struct drm_i915_gem_object *ctx_obj0;
+       struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state;
+       struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf;
        struct drm_i915_gem_object *ctx_obj1 = NULL;
+       struct intel_ringbuffer *ringbuf1 = NULL;
 
-       ctx_obj0 = to0->engine[ring->id].state;
        BUG_ON(!ctx_obj0);
        WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
+       WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
 
-       execlists_ctx_write_tail(ctx_obj0, tail0);
+       execlists_update_context(ctx_obj0, ringbuf0->obj, tail0);
 
        if (to1) {
+               ringbuf1 = to1->engine[ring->id].ringbuf;
                ctx_obj1 = to1->engine[ring->id].state;
                BUG_ON(!ctx_obj1);
                WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1));
+               WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj));
 
-               execlists_ctx_write_tail(ctx_obj1, tail1);
+               execlists_update_context(ctx_obj1, ringbuf1->obj, tail1);
        }
 

        execlists_elsp_write(ring, ctx_obj0, ctx_obj1);

drivers/gpu/drm/i915/intel_lrc.c +5 -1

@@ -524,6 +534,10 @@ static int execlists_context_queue(struct
 intel_engine_cs *ring,
                return -ENOMEM;
        req->ctx = to;
        i915_gem_context_reference(req->ctx);
+
+       if (to != ring->default_context)
+               intel_lr_context_pin(ring, to);
+
        req->ring = ring;
        req->tail = tail;

@@ -544,7 +558,7 @@ static int execlists_context_queue(struct
 intel_engine_cs *ring,
 
                if (to == tail_req->ctx) {
                        WARN(tail_req->elsp_submitted != 0,
-                            "More than 2 already-submitted reqs
 queued\n");
+                               "More than 2 already-submitted reqs
 queued\n");

                        list_del(&tail_req->execlist_link);
                        list_add_tail(&tail_req->execlist_link,
                                &ring->execlist_retired_req_list);

drivers/gpu/drm/i915/intel_lrc.c +6 -0

@@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct
 intel_engine_cs *ring)
        spin_unlock_irqrestore(&ring->execlist_lock, flags);
 
        list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
+               struct intel_context *ctx = req->ctx;
+               struct drm_i915_gem_object *ctx_obj =
+                               ctx->engine[ring->id].state;
+
+               if (ctx_obj && (ctx != ring->default_context))
+                       intel_lr_context_unpin(ring, ctx);
                intel_runtime_pm_put(dev_priv);
                i915_gem_context_unreference(req->ctx);

                list_del(&req->execlist_link);

drivers/gpu/drm/i915/intel_lrc.c +1 -0

@@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct intel_engine_cs
 *ring,
                struct intel_context *ctx)
 {
        struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+       struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
        int ret = 0;
 

        WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));

drivers/gpu/drm/i915/intel_lrc.c +16 -2

@@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct
 intel_engine_cs *ring,
                ret = i915_gem_obj_ggtt_pin(ctx_obj,
                                GEN8_LR_CONTEXT_ALIGN, 0);
                if (ret)
-                       ctx->engine[ring->id].unpin_count = 0;
+                       goto reset_unpin_count;
+
+               ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
+               if (ret)
+                       goto unpin_ctx_obj;
        }
 
        return ret;
+
+unpin_ctx_obj:
+       i915_gem_object_ggtt_unpin(ctx_obj);
+reset_unpin_count:
+       ctx->engine[ring->id].unpin_count = 0;
+
+       return ret;
 }
 
 void intel_lr_context_unpin(struct intel_engine_cs *ring,
                struct intel_context *ctx)
 {
        struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
+       struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
 
        if (ctx_obj) {
                WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
-               if (--ctx->engine[ring->id].unpin_count == 0)
+               if (--ctx->engine[ring->id].unpin_count == 0) {
+                       intel_unpin_ringbuffer_obj(ringbuf);
                        i915_gem_object_ggtt_unpin(ctx_obj);
+               }
        }
 }

@@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context *ctx,

struct drm_i915_gem_object *ctx_o

 {
        struct drm_device *dev = ring->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_lrc.c +0 -1

-       struct drm_i915_gem_object *ring_obj = ringbuf->obj;
        struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
        struct page *page;
        uint32_t *reg_state;

@@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx,

struct drm_i915_gem_object *ctx_o

        reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base);
        reg_state[CTX_RING_TAIL+1] = 0;
        reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base);

- reg_state[CTX_RING_BUFFER_START+1] =
i915_gem_obj_ggtt_offset(ring_obj);
+ /* Ring buffer start address is not known until the buffer is
pinned.

drivers/gpu/drm/i915/intel_lrc.c +2 -0

+        * It is written to the context image in execlists_update_context()
+        */
        reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base);
        reg_state[CTX_RING_BUFFER_CONTROL+1] =
                        ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) |

RING_VALID;

drivers/gpu/drm/i915/intel_lrc.c +4 -2

@@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct intel_context
 *ctx)
                                        ctx->engine[i].ringbuf;
                        struct intel_engine_cs *ring = ringbuf->ring;
 
+                       if (ctx == ring->default_context) {
+                               intel_unpin_ringbuffer_obj(ringbuf);
+                               i915_gem_object_ggtt_unpin(ctx_obj);
+                       }
                        intel_destroy_ringbuffer_obj(ringbuf);
                        kfree(ringbuf);
-                       if (ctx == ring->default_context)
-                               i915_gem_object_ggtt_unpin(ctx_obj);
                        drm_gem_object_unreference(&ctx_obj->base);
                }

        }

drivers/gpu/drm/i915/intel_lrc.c +1 -4

@@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
        if (!ringbuf) {
                DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
                                ring->name);
-               if (is_global_default_ctx)
-                       i915_gem_object_ggtt_unpin(ctx_obj);
-               drm_gem_object_unreference(&ctx_obj->base);
                ret = -ENOMEM;
-               return ret;
+               goto error_unpin_ctx;
        }
 

        ringbuf->ring = ring;

drivers/gpu/drm/i915/intel_lrc.c +0 -1

@@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
        ringbuf->space = ringbuf->size;
        ringbuf->last_retired_head = -1;
 
-       /* TODO: For now we put this in the mappable region so that we can

reuse

drivers/gpu/drm/i915/intel_lrc.c +0 -2

-        * the existing ringbuffer code which ioremaps it. When we start
-        * creating many contexts, this will no longer work and we must

switch

drivers/gpu/drm/i915/intel_lrc.c +0 -5

-        * to a kmapish interface.
-        */
-       ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-       if (ret) {
-               DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s:

%d\n",

drivers/gpu/drm/i915/intel_lrc.c +5 -0

+       if (ringbuf->obj == NULL) {
+               ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+               if (ret) {
+                       DRM_DEBUG_DRIVER(
+                               "Failed to allocate ringbuffer obj %s:

%d\n",

                                ring->name, ret);

drivers/gpu/drm/i915/intel_lrc.c +5 -1

-               goto error;
+                       goto error_free_rbuf;
+               }
+
+               if (is_global_default_ctx) {
+                       ret = intel_pin_and_map_ringbuffer_obj(dev,

ringbuf);

drivers/gpu/drm/i915/intel_lrc.c +3 -0

+                       if (ret) {
+                               DRM_ERROR(
+                                       "Failed to pin and map ringbuffer

%s: %d\n",

drivers/gpu/drm/i915/intel_lrc.c +5 -2

+                                       ring->name, ret);
+                               goto error_destroy_rbuf;
+                       }
+               }
+
        }
 
        ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
        if (ret) {
                DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
-               intel_destroy_ringbuffer_obj(ringbuf);
                goto error;
        }

@@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
                        DRM_ERROR("Init render state failed: %d\n", ret);
                        ctx->engine[ring->id].ringbuf = NULL;
                        ctx->engine[ring->id].state = NULL;
-                       intel_destroy_ringbuffer_obj(ringbuf);
                        goto error;
                }

                ctx->rcs_initialized = true;

drivers/gpu/drm/i915/intel_lrc.c +6 -0

@@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
        return 0;
 
 error:
+       if (is_global_default_ctx)
+               intel_unpin_ringbuffer_obj(ringbuf);
+error_destroy_rbuf:
+       intel_destroy_ringbuffer_obj(ringbuf);
+error_free_rbuf:
        kfree(ringbuf);
+error_unpin_ctx:
        if (is_global_default_ctx)
                i915_gem_object_ggtt_unpin(ctx_obj);

        drm_gem_object_unreference(&ctx_obj->base);

drivers/gpu/drm/i915/intel_ringbuffer.c +33 -4

index a8f72e8..0c4aab1 100644
@@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct
 intel_engine_cs *ring)
        return 0;
 }
 
-void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 {
-       if (!ringbuf->obj)
-               return;
-
        iounmap(ringbuf->virtual_start);
+       ringbuf->virtual_start = NULL;
        i915_gem_object_ggtt_unpin(ringbuf->obj);
+}
+
+int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
+                                    struct intel_ringbuffer *ringbuf)
+{
+       struct drm_i915_private *dev_priv = to_i915(dev);
+       struct drm_i915_gem_object *obj = ringbuf->obj;
+       int ret;
+
+       ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
+       if (ret)
+               return ret;
+
+       ret = i915_gem_object_set_to_gtt_domain(obj, true);
+       if (ret) {
+               i915_gem_object_ggtt_unpin(obj);
+               return ret;
+       }
+
+       ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
+                       i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+       if (ringbuf->virtual_start == NULL) {
+               i915_gem_object_ggtt_unpin(obj);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
+{
        drm_gem_object_unreference(&ringbuf->obj->base);
        ringbuf->obj = NULL;

}

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -5

@@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct
 intel_ringbuffer *ringbuf)
 int intel_alloc_ringbuffer_obj(struct drm_device *dev,
                               struct intel_ringbuffer *ringbuf)
 {
-       struct drm_i915_private *dev_priv = to_i915(dev);
        struct drm_i915_gem_object *obj;
-       int ret;
-
-       if (ringbuf->obj)
-               return 0;
 
        obj = NULL;

        if (!HAS_LLC(dev))

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -22

@@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct drm_device
 *dev,
        /* mark ring buffers as read-only from GPU side by default */
        obj->gt_ro = 1;
 
-       ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
-       if (ret)
-               goto err_unref;
-
-       ret = i915_gem_object_set_to_gtt_domain(obj, true);
-       if (ret)
-               goto err_unpin;
-
-       ringbuf->virtual_start =
-               ioremap_wc(dev_priv->gtt.mappable_base +
 i915_gem_obj_ggtt_offset(obj),
-                               ringbuf->size);
-       if (ringbuf->virtual_start == NULL) {
-               ret = -EINVAL;
-               goto err_unpin;
-       }
-
        ringbuf->obj = obj;
-       return 0;
 
-err_unpin:
-       i915_gem_object_ggtt_unpin(obj);
-err_unref:
-       drm_gem_object_unreference(&obj->base);
-       return ret;
+       return 0;
 }

static int intel_init_ring_buffer(struct drm_device *dev,

drivers/gpu/drm/i915/intel_ringbuffer.c +15 -4

@@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct
 drm_device *dev,
                        goto error;
        }
 
-       ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-       if (ret) {
-               DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
 ring->name, ret);
-               goto error;
+       if (ringbuf->obj == NULL) {
+               ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+               if (ret) {
+                       DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
+                                       ring->name, ret);
+                       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;
+               }

        }

        /* Workaround an erratum on the i830 which causes a hang if

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct
 intel_engine_cs *ring)
        intel_stop_ring_buffer(ring);
        WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE)
 == 0);
 
+       intel_unpin_ringbuffer_obj(ringbuf);
        intel_destroy_ringbuffer_obj(ringbuf);

        ring->preallocated_lazy_request = NULL;
        ring->outstanding_lazy_seqno = 0;

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index 8c002d2..365854ad 100644
@@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs *ring,
 #define I915_GEM_HWS_SCRATCH_INDEX     0x30
 #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX <<
 MI_STORE_DWORD_INDEX_SHIFT)
 
+void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
+int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
+                                    struct intel_ringbuffer *ringbuf);
 void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
 int intel_alloc_ringbuffer_obj(struct drm_device *dev,

                               struct intel_ringbuffer *ringbuf);