[3/3] drm/i915: Cache LRC state page in the context

Commit Message

Tvrtko Ursulin Jan. 15, 2016, 3:10 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

LRC lifetime is well defined so we can cache the page pointing
to the object backing store in the context in order to avoid
walking over the object SG page list from the interrupt context
without the big lock held.

v2: Also cache the mapping. (Chris Wilson)
v3: Unmap on the error path.
v4: No need to cache the page. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Dave Gordon <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_lrc.c | 53 ++++++++++++++++++++++++++--------------
 2 files changed, 35 insertions(+), 19 deletions(-)

Comments

Chris Wilson Jan. 15, 2016, 4:01 p.m. UTC | #1

On Fri, Jan 15, 2016 at 03:10:29PM +0000, Tvrtko Ursulin wrote:
> @@ -1118,15 +1126,22 @@ void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
> +	if (--rq->ctx->engine[ring->id].pin_count == 0) {
> +		lrc_state_page = i915_gem_object_get_dirty_page(ctx_obj,
> +								LRC_STATE_PN);

Interesting choice. We called set_page_dirty() when we took the mapping.
Should that page flag be preserved whilst we hold the kmap - I think so,
i.e. the mm cannot flush the page whilst it has an elevated mapcount. So
calling set_page_dirty() again is redundant, right?
-Chris

Tvrtko Ursulin Jan. 15, 2016, 4:05 p.m. UTC | #2

On 15/01/16 16:01, Chris Wilson wrote:
> On Fri, Jan 15, 2016 at 03:10:29PM +0000, Tvrtko Ursulin wrote:
>> @@ -1118,15 +1126,22 @@ void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
>> +	if (--rq->ctx->engine[ring->id].pin_count == 0) {
>> +		lrc_state_page = i915_gem_object_get_dirty_page(ctx_obj,
>> +								LRC_STATE_PN);
>
> Interesting choice. We called set_page_dirty() when we took the mapping.
> Should that page flag be preserved whilst we hold the kmap - I think so,
> i.e. the mm cannot flush the page whilst it has an elevated mapcount. So
> calling set_page_dirty() again is redundant, right?

If you call mindless copy & paste interesting. :D

Any other concerns or I can respin with that only?

Regards,

Tvrtko

Chris Wilson Jan. 15, 2016, 4:17 p.m. UTC | #3

On Fri, Jan 15, 2016 at 04:05:24PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/01/16 16:01, Chris Wilson wrote:
> >On Fri, Jan 15, 2016 at 03:10:29PM +0000, Tvrtko Ursulin wrote:
> >>@@ -1118,15 +1126,22 @@ void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
> >>+	if (--rq->ctx->engine[ring->id].pin_count == 0) {
> >>+		lrc_state_page = i915_gem_object_get_dirty_page(ctx_obj,
> >>+								LRC_STATE_PN);
> >
> >Interesting choice. We called set_page_dirty() when we took the mapping.
> >Should that page flag be preserved whilst we hold the kmap - I think so,
> >i.e. the mm cannot flush the page whilst it has an elevated mapcount. So
> >calling set_page_dirty() again is redundant, right?
> 
> If you call mindless copy & paste interesting. :D
> 
> Any other concerns or I can respin with that only?

No. I was quibbling over the excess clearing of state on unpinning :)

Pity we have to respin even for innoculous changes just to get a CI
tick.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index acff98b9c148..af301482e6f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -890,6 +890,7 @@  struct intel_context {
 		int pin_count;
 		struct i915_vma *lrc_vma;
 		u64 lrc_desc;
+		uint32_t *lrc_reg_state;
 	} engine[I915_NUM_RINGS];
 
 	struct list_head link;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 545173c1b5c5..70bd28cc8887 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -390,14 +390,7 @@  static int execlists_update_context(struct drm_i915_gem_request *rq)
 {
 	struct intel_engine_cs *ring = rq->ring;
 	struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt;
-	struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
-	struct page *page;
-	uint32_t *reg_state;
-
-	BUG_ON(!ctx_obj);
-
-	page = i915_gem_object_get_dirty_page(ctx_obj, LRC_STATE_PN);
-	reg_state = kmap_atomic(page);
+	uint32_t *reg_state = rq->ctx->engine[ring->id].lrc_reg_state;
 
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
 	reg_state[CTX_RING_BUFFER_START+1] = rq->ringbuf->vma->node.start;
@@ -414,8 +407,6 @@  static int execlists_update_context(struct drm_i915_gem_request *rq)
 		ASSIGN_CTX_PDP(ppgtt, reg_state, 0);
 	}
 
-	kunmap_atomic(reg_state);
-
 	return 0;
 }
 
@@ -1067,6 +1058,8 @@  static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
 	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
+	struct page *lrc_state_page;
+	uint32_t *lrc_reg_state;
 	int ret;
 
 	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
@@ -1076,12 +1069,25 @@  static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
 	if (ret)
 		return ret;
 
+	lrc_state_page = i915_gem_object_get_dirty_page(ctx_obj, LRC_STATE_PN);
+	if (WARN_ON(!lrc_state_page)) {
+		ret = -ENODEV;
+		goto unpin_ctx_obj;
+	}
+
+	lrc_reg_state = kmap(lrc_state_page);
+	if (!lrc_reg_state) {
+		ret = -ENOMEM;
+		goto unpin_ctx_obj;
+	}
+
 	ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
 	if (ret)
-		goto unpin_ctx_obj;
+		goto unmap_state_page;
 
 	ctx->engine[ring->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
 	intel_lr_context_descriptor_update(ctx, ring);
+	ctx->engine[ring->id].lrc_reg_state = lrc_reg_state;
 	ctx_obj->dirty = true;
 
 	/* Invalidate GuC TLB. */
@@ -1090,6 +1096,8 @@  static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
 
 	return ret;
 
+unmap_state_page:
+	kunmap(lrc_state_page);
 unpin_ctx_obj:
 	i915_gem_object_ggtt_unpin(ctx_obj);
 
@@ -1118,15 +1126,22 @@  void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
 	struct intel_engine_cs *ring = rq->ring;
 	struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
 	struct intel_ringbuffer *ringbuf = rq->ringbuf;
+	struct page *lrc_state_page;
 
-	if (ctx_obj) {
-		WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
-		if (--rq->ctx->engine[ring->id].pin_count == 0) {
-			intel_unpin_ringbuffer_obj(ringbuf);
-			i915_gem_object_ggtt_unpin(ctx_obj);
-			rq->ctx->engine[ring->id].lrc_vma = NULL;
-			rq->ctx->engine[ring->id].lrc_desc = 0;
-		}
+	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+
+	if (!ctx_obj)
+		return;
+
+	if (--rq->ctx->engine[ring->id].pin_count == 0) {
+		lrc_state_page = i915_gem_object_get_dirty_page(ctx_obj,
+								LRC_STATE_PN);
+		kunmap(lrc_state_page);
+		intel_unpin_ringbuffer_obj(ringbuf);
+		i915_gem_object_ggtt_unpin(ctx_obj);
+		rq->ctx->engine[ring->id].lrc_vma = NULL;
+		rq->ctx->engine[ring->id].lrc_desc = 0;
+		rq->ctx->engine[ring->id].lrc_reg_state = NULL;
 	}
 }