All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel
@ 2016-04-14 17:19 Dave Gordon
  2016-04-14 17:19 ` [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-14 17:19 UTC (permalink / raw)
  To: intel-gfx

With the new i915_gem_obj_pin_map() interface, it makes sense to keep
GuC objects (which are always pinned in memory and in the GGTT anyway)
mapped into kernel address space, rather than mapping and unmapping them
on each access.

This preliminary patch sets up the pin-and-map for all GuC-specific
objects, and updates the various setup/shutdown functions to use these
long-term mappings rather than doing their own kmap_atomic() calls.

Cc: <tvrtko.ursulin@intel.com>
Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 37 +++++++++++-------------------
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da86bdb..f80f577 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
 	struct guc_doorbell_info *doorbell;
-	void *base;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	doorbell = base + client->doorbell_offset;
+	doorbell = client->client_base + client->doorbell_offset;
 
-	doorbell->db_status = 1;
+	doorbell->db_status = GUC_DOORBELL_ENABLED;
 	doorbell->cookie = 0;
-
-	kunmap_atomic(base);
 }
 
 static int guc_ring_doorbell(struct i915_guc_client *gc)
@@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct intel_guc *guc,
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct guc_doorbell_info *doorbell;
-	void *base;
 	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
 	int value;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	doorbell = base + client->doorbell_offset;
-
-	doorbell->db_status = 0;
+	doorbell = client->client_base + client->doorbell_offset;
 
-	kunmap_atomic(base);
+	doorbell->db_status = GUC_DOORBELL_DISABLED;
 
 	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
 
@@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc *guc,
 			       struct i915_guc_client *client)
 {
 	struct guc_process_desc *desc;
-	void *base;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	desc = base + client->proc_desc_offset;
+	desc = client->client_base + client->proc_desc_offset;
 
 	memset(desc, 0, sizeof(*desc));
 
@@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
 	desc->wq_size_bytes = client->wq_size;
 	desc->wq_status = WQ_STATUS_ACTIVE;
 	desc->priority = client->priority;
-
-	kunmap_atomic(base);
 }
 
 /*
@@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
  * This is a wrapper to create a gem obj. In order to use it inside GuC, the
  * object needs to be pinned lifetime. Also we must pin it to gtt space other
  * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
+ * The object is also pinned & mapped into kernel address space.
  *
  * Return:	A drm_i915_gem_object if successful, otherwise NULL.
  */
@@ -620,13 +609,14 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
 	if (!obj)
 		return NULL;
 
-	if (i915_gem_object_get_pages(obj)) {
+	if (i915_gem_object_pin_map(obj) == NULL) {
 		drm_gem_object_unreference(&obj->base);
 		return NULL;
 	}
 
 	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
 			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
+		i915_gem_object_unpin_map(obj);
 		drm_gem_object_unreference(&obj->base);
 		return NULL;
 	}
@@ -649,6 +639,8 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
 	if (i915_gem_obj_is_pinned(obj))
 		i915_gem_object_ggtt_unpin(obj);
 
+	i915_gem_object_unpin_map(obj);
+
 	drm_gem_object_unreference(&obj->base);
 }
 
@@ -729,6 +721,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 		goto err;
 
 	client->client_obj = obj;
+	client->client_base = obj->mapping;
+	WARN_ON(!client->client_base);
 	client->wq_offset = GUC_DB_SIZE;
 	client->wq_size = GUC_WQ_SIZE;
 
@@ -841,7 +835,6 @@ static void guc_create_ads(struct intel_guc *guc)
 	struct guc_policies *policies;
 	struct guc_mmio_reg_state *reg_state;
 	struct intel_engine_cs *engine;
-	struct page *page;
 	u32 size;
 
 	/* The ads obj includes the struct itself and buffers passed to GuC */
@@ -857,9 +850,7 @@ static void guc_create_ads(struct intel_guc *guc)
 
 		guc->ads_obj = obj;
 	}
-
-	page = i915_gem_object_get_page(obj, 0);
-	ads = kmap(page);
+	ads = obj->mapping;
 
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
@@ -897,8 +888,6 @@ static void guc_create_ads(struct intel_guc *guc)
 
 	ads->reg_state_buffer = ads->reg_state_addr +
 			sizeof(struct guc_mmio_reg_state);
-
-	kunmap(page);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3bb85b1..9ab3564 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -31,6 +31,7 @@ struct drm_i915_gem_request;
 
 struct i915_guc_client {
 	struct drm_i915_gem_object *client_obj;
+	void *client_base;		/* Mapped address of above	*/
 	struct intel_context *owner;
 	struct intel_guc *guc;
 	uint32_t priority;
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission
  2016-04-14 17:19 [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel Dave Gordon
@ 2016-04-14 17:19 ` Dave Gordon
  2016-04-15 10:08   ` Tvrtko Ursulin
  2016-04-14 17:19 ` [PATCH 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Dave Gordon @ 2016-04-14 17:19 UTC (permalink / raw)
  To: intel-gfx; +Cc: daniel.vetter

From: Alex Dai <yu.dai@intel.com>

Now that we keep GuC shared objects mapped into kernel space for their
entire lifetime, we can simplify the code for accessing the doorbells
and work queue, which were previously calling kmap_atomic() on *each*
request submission.

This patch fixes the BUG shown below, where the thread could sleep
while holding the kmap_atomic mapping.

[   34.098798] BUG: scheduling while atomic:
gem_close_race/1941/0x00000002
[   34.098822] Modules linked in: hid_generic usbhid i915 asix usbnet
libphy mii i2c_algo_bit drm_kms_helper cfbfillrect syscopyarea cfbimgblt
sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm coretemp i2c_hid hid
video pinctrl_sunrisepoint pinctrl_intel acpi_pad nls_iso8859_1 e1000e
ptp psmouse pps_core ahci libahci
[   34.098824] CPU: 0 PID: 1941 Comm: gem_close_race Tainted: G     U
          4.4.0-160121+ #123
[   34.098824] Hardware name: Intel Corporation Skylake Client
platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551
09/22/2015
[   34.098825]  0000000000013e40 ffff880166c27a78 ffffffff81280d02
ffff880172c13e40
[   34.098826]  ffff880166c27a88 ffffffff810c203a ffff880166c27ac8
ffffffff814ec808
[   34.098827]  ffff88016b7c6000 ffff880166c28000 00000000000f4240
0000000000000001
[   34.098827] Call Trace:
[   34.098831]  [<ffffffff81280d02>] dump_stack+0x4b/0x79
[   34.098833]  [<ffffffff810c203a>] __schedule_bug+0x41/0x4f
[   34.098834]  [<ffffffff814ec808>] __schedule+0x5a8/0x690
[   34.098835]  [<ffffffff814ec927>] schedule+0x37/0x80
[   34.098836]  [<ffffffff814ef3fd>]
schedule_hrtimeout_range_clock+0xad/0x130
[   34.098837]  [<ffffffff81090be0>] ? hrtimer_init+0x10/0x10
[   34.098838]  [<ffffffff814ef3f1>] ?
schedule_hrtimeout_range_clock+0xa1/0x130
[   34.098839]  [<ffffffff814ef48e>] schedule_hrtimeout_range+0xe/0x10
[   34.098840]  [<ffffffff814eef9b>] usleep_range+0x3b/0x40
[   34.098853]  [<ffffffffa01ec109>] i915_guc_wq_check_space+0x119/0x210
[i915]
[   34.098861]  [<ffffffffa01da47c>]
intel_logical_ring_alloc_request_extras+0x5c/0x70 [i915]
[   34.098869]  [<ffffffffa01cdbf1>] i915_gem_request_alloc+0x91/0x170
[i915]
[   34.098875]  [<ffffffffa01c1c07>]
i915_gem_do_execbuffer.isra.25+0xbc7/0x12a0 [i915]
[   34.098882]  [<ffffffffa01cb785>] ?
i915_gem_object_get_pages_gtt+0x225/0x3c0 [i915]
[   34.098889]  [<ffffffffa01d1fb6>] ? i915_gem_pwrite_ioctl+0xd6/0x9f0
[i915]
[   34.098895]  [<ffffffffa01c2e68>] i915_gem_execbuffer2+0xa8/0x250
[i915]
[   34.098900]  [<ffffffffa00f65d8>] drm_ioctl+0x258/0x4f0 [drm]
[   34.098906]  [<ffffffffa01c2dc0>] ? i915_gem_execbuffer+0x340/0x340
[i915]
[   34.098908]  [<ffffffff8111590d>] do_vfs_ioctl+0x2cd/0x4a0
[   34.098909]  [<ffffffff8111eac2>] ? __fget+0x72/0xb0
[   34.098910]  [<ffffffff81115b1c>] SyS_ioctl+0x3c/0x70
[   34.098911]  [<ffffffff814effd7>] entry_SYSCALL_64_fastpath+0x12/0x6a
[   34.100208] ------------[ cut here ]------------

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93847
Cc: <daniel.vetter@ffwll.ch>
Cc: <tvrtko.ursulin@intel.com>
Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f80f577..fbd57ca 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -191,11 +191,9 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 	struct guc_process_desc *desc;
 	union guc_doorbell_qw db_cmp, db_exc, db_ret;
 	union guc_doorbell_qw *db;
-	void *base;
 	int attempt = 2, ret = -EAGAIN;
 
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
+	desc = gc->client_base + gc->proc_desc_offset;
 
 	/* Update the tail so it is visible to GuC */
 	desc->tail = gc->wq_tail;
@@ -211,7 +209,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 		db_exc.cookie = 1;
 
 	/* pointer of current doorbell cacheline */
-	db = base + gc->doorbell_offset;
+	db = gc->client_base + gc->doorbell_offset;
 
 	while (attempt--) {
 		/* lets ring the doorbell */
@@ -243,7 +241,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 	/* Finally, update the cached copy of the GuC's WQ head */
 	gc->wq_head = desc->head;
 
-	kunmap_atomic(base);
 	return ret;
 }
 
@@ -462,7 +459,6 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 int i915_guc_wq_check_space(struct i915_guc_client *gc)
 {
 	struct guc_process_desc *desc;
-	void *base;
 	u32 size = sizeof(struct guc_wq_item);
 	int ret = -ETIMEDOUT, timeout_counter = 200;
 
@@ -474,8 +470,7 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
 		return 0;
 
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
+	desc = gc->client_base + gc->proc_desc_offset;
 
 	while (timeout_counter-- > 0) {
 		gc->wq_head = desc->head;
@@ -489,8 +484,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 			usleep_range(1000, 2000);
 	};
 
-	kunmap_atomic(base);
-
 	return ret;
 }
 
@@ -498,7 +491,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 				  struct drm_i915_gem_request *rq)
 {
 	struct guc_wq_item *wqi;
-	void *base;
 	u32 tail, wq_len, wq_off, space;
 
 	space = CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size);
@@ -521,10 +513,7 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	WARN_ON(wq_off & 3);
 
 	/* wq starts from the page after doorbell / process_desc */
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
-	wq_off &= PAGE_SIZE - 1;
-	wqi = (struct guc_wq_item *)((char *)base + wq_off);
+	wqi = gc->client_base + GUC_DB_SIZE + wq_off;
 
 	/* len does not include the header */
 	wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
@@ -542,8 +531,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
 	wqi->fence_id = 0; /*XXX: what fence to be here */
 
-	kunmap_atomic(base);
-
 	return 0;
 }
 
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] drm/i915/guc: drop cached copy of 'wq_head'
  2016-04-14 17:19 [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel Dave Gordon
  2016-04-14 17:19 ` [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
@ 2016-04-14 17:19 ` Dave Gordon
  2016-04-15  6:55 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/guc: keep GuC objects mapped in kernel Patchwork
  2016-04-15 10:04 ` [PATCH 1/3] " Tvrtko Ursulin
  3 siblings, 0 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-14 17:19 UTC (permalink / raw)
  To: intel-gfx

From: Alex Dai <yu.dai@intel.com>

Now that we keep the GuC client structure (and therefore the process
descriptor embedded therein) permanently mapped, we don't really need to
keep a local copy of the GuC's work-queue-head. So we can simplify the
code a little by not doing this.

Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 16 ++++------------
 drivers/gpu/drm/i915/intel_guc.h           |  2 +-
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index fbd57ca..fc27599 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -238,9 +238,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 			db_exc.cookie = 1;
 	}
 
-	/* Finally, update the cached copy of the GuC's WQ head */
-	gc->wq_head = desc->head;
-
 	return ret;
 }
 
@@ -465,17 +462,10 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 	if (!gc)
 		return 0;
 
-	/* Quickly return if wq space is available since last time we cache the
-	 * head position. */
-	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
-		return 0;
-
 	desc = gc->client_base + gc->proc_desc_offset;
 
 	while (timeout_counter-- > 0) {
-		gc->wq_head = desc->head;
-
-		if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
+		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
 			ret = 0;
 			break;
 		}
@@ -490,10 +480,12 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 static int guc_add_workqueue_item(struct i915_guc_client *gc,
 				  struct drm_i915_gem_request *rq)
 {
+	struct guc_process_desc *desc;
 	struct guc_wq_item *wqi;
 	u32 tail, wq_len, wq_off, space;
 
-	space = CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size);
+	desc = gc->client_base + gc->proc_desc_offset;
+	space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
 	if (WARN_ON(space < sizeof(struct guc_wq_item)))
 		return -ENOSPC; /* shouldn't happen */
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9ab3564..558793b 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -46,7 +46,7 @@ struct i915_guc_client {
 	uint32_t wq_offset;
 	uint32_t wq_size;
 	uint32_t wq_tail;
-	uint32_t wq_head;
+	uint32_t unused;		/* Was 'wq_head'		*/
 
 	/* GuC submission statistics & status */
 	uint64_t submissions[GUC_MAX_ENGINES_NUM];
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-14 17:19 [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel Dave Gordon
  2016-04-14 17:19 ` [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
  2016-04-14 17:19 ` [PATCH 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
@ 2016-04-15  6:55 ` Patchwork
  2016-04-15 10:04 ` [PATCH 1/3] " Tvrtko Ursulin
  3 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2016-04-15  6:55 UTC (permalink / raw)
  To: Dave Gordon; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915/guc: keep GuC objects mapped in kernel
URL   : https://patchwork.freedesktop.org/series/5732/
State : failure

== Summary ==

Series 5732v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5732/revisions/1/mbox/

Test gem_busy:
        Subgroup basic-blt:
                pass       -> SKIP       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (bsw-nuc-2)

bdw-ultra        total:203  pass:179  dwarn:0   dfail:0   fail:1   skip:23 
bsw-nuc-2        total:202  pass:160  dwarn:0   dfail:0   fail:2   skip:40 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:178  dwarn:0   dfail:0   fail:1   skip:24 
ivb-t430s        total:203  pass:174  dwarn:0   dfail:0   fail:1   skip:28 
skl-i7k-2        total:203  pass:177  dwarn:0   dfail:0   fail:1   skip:25 
skl-nuci5        total:203  pass:191  dwarn:0   dfail:0   fail:1   skip:11 
snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:2   skip:37 
BOOT FAILED for snb-dellxps

Results at /archive/results/CI_IGT_test/Patchwork_1906/

c7583aec08ba04e2336bd9879a10f30d4e0cdc60 drm-intel-nightly: 2016y-04m-14d-14h-53m-34s UTC integration manifest
1315d32 drm/i915/guc: drop cached copy of 'wq_head'
ec99e9a drm/i915/guc: stop using kmap_atomic() during request submission
dcf504c drm/i915/guc: keep GuC objects mapped in kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-14 17:19 [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel Dave Gordon
                   ` (2 preceding siblings ...)
  2016-04-15  6:55 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/guc: keep GuC objects mapped in kernel Patchwork
@ 2016-04-15 10:04 ` Tvrtko Ursulin
  2016-04-15 11:12   ` Dave Gordon
  3 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-04-15 10:04 UTC (permalink / raw)
  To: Dave Gordon, intel-gfx


On 14/04/16 18:19, Dave Gordon wrote:
> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
> GuC objects (which are always pinned in memory and in the GGTT anyway)
> mapped into kernel address space, rather than mapping and unmapping them
> on each access.
>
> This preliminary patch sets up the pin-and-map for all GuC-specific
> objects, and updates the various setup/shutdown functions to use these
> long-term mappings rather than doing their own kmap_atomic() calls.
>
> Cc: <tvrtko.ursulin@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 37 +++++++++++-------------------
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   2 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index da86bdb..f80f577 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   			      struct i915_guc_client *client)
>   {
>   	struct guc_doorbell_info *doorbell;
> -	void *base;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	doorbell = base + client->doorbell_offset;
> +	doorbell = client->client_base + client->doorbell_offset;
>
> -	doorbell->db_status = 1;
> +	doorbell->db_status = GUC_DOORBELL_ENABLED;
>   	doorbell->cookie = 0;
> -
> -	kunmap_atomic(base);
>   }
>
>   static int guc_ring_doorbell(struct i915_guc_client *gc)
> @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct intel_guc *guc,
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct guc_doorbell_info *doorbell;
> -	void *base;
>   	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>   	int value;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	doorbell = base + client->doorbell_offset;
> -
> -	doorbell->db_status = 0;
> +	doorbell = client->client_base + client->doorbell_offset;

Not 100% sure of the object lifetimes in GuC, but would it be even 
simpler to store a pointer to struct struct guc_doorbell_info as 
guc->doorbell ? There aren't that many call sites true, but kind of 
looks logical at least from the outside.

>
> -	kunmap_atomic(base);
> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
>
>   	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>
> @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>   			       struct i915_guc_client *client)
>   {
>   	struct guc_process_desc *desc;
> -	void *base;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	desc = base + client->proc_desc_offset;
> +	desc = client->client_base + client->proc_desc_offset;

And the same maybe for this?

>   	memset(desc, 0, sizeof(*desc));
>
> @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>   	desc->wq_size_bytes = client->wq_size;
>   	desc->wq_status = WQ_STATUS_ACTIVE;
>   	desc->priority = client->priority;
> -
> -	kunmap_atomic(base);
>   }
>
>   /*
> @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
>    * This is a wrapper to create a gem obj. In order to use it inside GuC, the
>    * object needs to be pinned lifetime. Also we must pin it to gtt space other
>    * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
> + * The object is also pinned & mapped into kernel address space.
>    *
>    * Return:	A drm_i915_gem_object if successful, otherwise NULL.
>    */
> @@ -620,13 +609,14 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
>   	if (!obj)
>   		return NULL;
>
> -	if (i915_gem_object_get_pages(obj)) {
> +	if (i915_gem_object_pin_map(obj) == NULL) {

This should be IS_ERR check.

>   		drm_gem_object_unreference(&obj->base);
>   		return NULL;
>   	}
>
>   	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>   			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
> +		i915_gem_object_unpin_map(obj);
>   		drm_gem_object_unreference(&obj->base);
>   		return NULL;
>   	}
> @@ -649,6 +639,8 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>   	if (i915_gem_obj_is_pinned(obj))
>   		i915_gem_object_ggtt_unpin(obj);
>
> +	i915_gem_object_unpin_map(obj);
> +
>   	drm_gem_object_unreference(&obj->base);
>   }
>
> @@ -729,6 +721,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   		goto err;
>
>   	client->client_obj = obj;
> +	client->client_base = obj->mapping;

It think outside code should not access obj->mapping directly but use 
what i915_gem_object_pin_map has returned.

> +	WARN_ON(!client->client_base);

And this has already been handled at the i915_gem_object_pin_map call 
site so I don't think it serves any purpose.

>   	client->wq_offset = GUC_DB_SIZE;
>   	client->wq_size = GUC_WQ_SIZE;
>
> @@ -841,7 +835,6 @@ static void guc_create_ads(struct intel_guc *guc)
>   	struct guc_policies *policies;
>   	struct guc_mmio_reg_state *reg_state;
>   	struct intel_engine_cs *engine;
> -	struct page *page;
>   	u32 size;
>
>   	/* The ads obj includes the struct itself and buffers passed to GuC */
> @@ -857,9 +850,7 @@ static void guc_create_ads(struct intel_guc *guc)
>
>   		guc->ads_obj = obj;
>   	}
> -
> -	page = i915_gem_object_get_page(obj, 0);
> -	ads = kmap(page);
> +	ads = obj->mapping;

Same as above. I suggest storing the base address in the guc client or 
somewhere appropriate.

Or if objects have separate explicit lifetimes, even if they don't 
overlap, you could even nest i915_gem_object_pin_map and unpin. Depends 
what makes the code simpler.

>
>   	/*
>   	 * The GuC requires a "Golden Context" when it reinitialises
> @@ -897,8 +888,6 @@ static void guc_create_ads(struct intel_guc *guc)
>
>   	ads->reg_state_buffer = ads->reg_state_addr +
>   			sizeof(struct guc_mmio_reg_state);
> -
> -	kunmap(page);
>   }
>
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 3bb85b1..9ab3564 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -31,6 +31,7 @@ struct drm_i915_gem_request;
>
>   struct i915_guc_client {
>   	struct drm_i915_gem_object *client_obj;
> +	void *client_base;		/* Mapped address of above	*/
>   	struct intel_context *owner;
>   	struct intel_guc *guc;
>   	uint32_t priority;
>

Regards,

Tvrtko

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission
  2016-04-14 17:19 ` [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
@ 2016-04-15 10:08   ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-04-15 10:08 UTC (permalink / raw)
  To: Dave Gordon, intel-gfx; +Cc: daniel.vetter


On 14/04/16 18:19, Dave Gordon wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> Now that we keep GuC shared objects mapped into kernel space for their
> entire lifetime, we can simplify the code for accessing the doorbells
> and work queue, which were previously calling kmap_atomic() on *each*
> request submission.
>
> This patch fixes the BUG shown below, where the thread could sleep
> while holding the kmap_atomic mapping.
>
> [   34.098798] BUG: scheduling while atomic:
> gem_close_race/1941/0x00000002
> [   34.098822] Modules linked in: hid_generic usbhid i915 asix usbnet
> libphy mii i2c_algo_bit drm_kms_helper cfbfillrect syscopyarea cfbimgblt
> sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm coretemp i2c_hid hid
> video pinctrl_sunrisepoint pinctrl_intel acpi_pad nls_iso8859_1 e1000e
> ptp psmouse pps_core ahci libahci
> [   34.098824] CPU: 0 PID: 1941 Comm: gem_close_race Tainted: G     U
>            4.4.0-160121+ #123
> [   34.098824] Hardware name: Intel Corporation Skylake Client
> platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551
> 09/22/2015
> [   34.098825]  0000000000013e40 ffff880166c27a78 ffffffff81280d02
> ffff880172c13e40
> [   34.098826]  ffff880166c27a88 ffffffff810c203a ffff880166c27ac8
> ffffffff814ec808
> [   34.098827]  ffff88016b7c6000 ffff880166c28000 00000000000f4240
> 0000000000000001
> [   34.098827] Call Trace:
> [   34.098831]  [<ffffffff81280d02>] dump_stack+0x4b/0x79
> [   34.098833]  [<ffffffff810c203a>] __schedule_bug+0x41/0x4f
> [   34.098834]  [<ffffffff814ec808>] __schedule+0x5a8/0x690
> [   34.098835]  [<ffffffff814ec927>] schedule+0x37/0x80
> [   34.098836]  [<ffffffff814ef3fd>]
> schedule_hrtimeout_range_clock+0xad/0x130
> [   34.098837]  [<ffffffff81090be0>] ? hrtimer_init+0x10/0x10
> [   34.098838]  [<ffffffff814ef3f1>] ?
> schedule_hrtimeout_range_clock+0xa1/0x130
> [   34.098839]  [<ffffffff814ef48e>] schedule_hrtimeout_range+0xe/0x10
> [   34.098840]  [<ffffffff814eef9b>] usleep_range+0x3b/0x40
> [   34.098853]  [<ffffffffa01ec109>] i915_guc_wq_check_space+0x119/0x210
> [i915]
> [   34.098861]  [<ffffffffa01da47c>]
> intel_logical_ring_alloc_request_extras+0x5c/0x70 [i915]
> [   34.098869]  [<ffffffffa01cdbf1>] i915_gem_request_alloc+0x91/0x170
> [i915]
> [   34.098875]  [<ffffffffa01c1c07>]
> i915_gem_do_execbuffer.isra.25+0xbc7/0x12a0 [i915]
> [   34.098882]  [<ffffffffa01cb785>] ?
> i915_gem_object_get_pages_gtt+0x225/0x3c0 [i915]
> [   34.098889]  [<ffffffffa01d1fb6>] ? i915_gem_pwrite_ioctl+0xd6/0x9f0
> [i915]
> [   34.098895]  [<ffffffffa01c2e68>] i915_gem_execbuffer2+0xa8/0x250
> [i915]
> [   34.098900]  [<ffffffffa00f65d8>] drm_ioctl+0x258/0x4f0 [drm]
> [   34.098906]  [<ffffffffa01c2dc0>] ? i915_gem_execbuffer+0x340/0x340
> [i915]
> [   34.098908]  [<ffffffff8111590d>] do_vfs_ioctl+0x2cd/0x4a0
> [   34.098909]  [<ffffffff8111eac2>] ? __fget+0x72/0xb0
> [   34.098910]  [<ffffffff81115b1c>] SyS_ioctl+0x3c/0x70
> [   34.098911]  [<ffffffff814effd7>] entry_SYSCALL_64_fastpath+0x12/0x6a
> [   34.100208] ------------[ cut here ]------------
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93847
> Cc: <daniel.vetter@ffwll.ch>
> Cc: <tvrtko.ursulin@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++-----------------
>   1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index f80f577..fbd57ca 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -191,11 +191,9 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
>   	struct guc_process_desc *desc;
>   	union guc_doorbell_qw db_cmp, db_exc, db_ret;
>   	union guc_doorbell_qw *db;
> -	void *base;
>   	int attempt = 2, ret = -EAGAIN;
>
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> -	desc = base + gc->proc_desc_offset;
> +	desc = gc->client_base + gc->proc_desc_offset;
>
>   	/* Update the tail so it is visible to GuC */
>   	desc->tail = gc->wq_tail;
> @@ -211,7 +209,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
>   		db_exc.cookie = 1;
>
>   	/* pointer of current doorbell cacheline */
> -	db = base + gc->doorbell_offset;
> +	db = gc->client_base + gc->doorbell_offset;
>
>   	while (attempt--) {
>   		/* lets ring the doorbell */
> @@ -243,7 +241,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
>   	/* Finally, update the cached copy of the GuC's WQ head */
>   	gc->wq_head = desc->head;
>
> -	kunmap_atomic(base);
>   	return ret;
>   }
>
> @@ -462,7 +459,6 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>   int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   {
>   	struct guc_process_desc *desc;
> -	void *base;
>   	u32 size = sizeof(struct guc_wq_item);
>   	int ret = -ETIMEDOUT, timeout_counter = 200;
>
> @@ -474,8 +470,7 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
>   		return 0;
>
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> -	desc = base + gc->proc_desc_offset;
> +	desc = gc->client_base + gc->proc_desc_offset;
>
>   	while (timeout_counter-- > 0) {
>   		gc->wq_head = desc->head;
> @@ -489,8 +484,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   			usleep_range(1000, 2000);
>   	};
>
> -	kunmap_atomic(base);
> -
>   	return ret;
>   }

Yes, as I commented in the previous patch in the series, if the 
lifetimes allow it, it would be shorter to have gc->doorbell and 
gc->desc and then functions like these would not have to do anything but 
use them.

Regards,

Tvrtko


>
> @@ -498,7 +491,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>   				  struct drm_i915_gem_request *rq)
>   {
>   	struct guc_wq_item *wqi;
> -	void *base;
>   	u32 tail, wq_len, wq_off, space;
>
>   	space = CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size);
> @@ -521,10 +513,7 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>   	WARN_ON(wq_off & 3);
>
>   	/* wq starts from the page after doorbell / process_desc */
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
> -			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
> -	wq_off &= PAGE_SIZE - 1;
> -	wqi = (struct guc_wq_item *)((char *)base + wq_off);
> +	wqi = gc->client_base + GUC_DB_SIZE + wq_off;
>
>   	/* len does not include the header */
>   	wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
> @@ -542,8 +531,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>   	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
>   	wqi->fence_id = 0; /*XXX: what fence to be here */
>
> -	kunmap_atomic(base);
> -
>   	return 0;
>   }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-15 10:04 ` [PATCH 1/3] " Tvrtko Ursulin
@ 2016-04-15 11:12   ` Dave Gordon
  2016-04-15 11:42     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Dave Gordon @ 2016-04-15 11:12 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

On 15/04/2016 11:04, Tvrtko Ursulin wrote:
>
> On 14/04/16 18:19, Dave Gordon wrote:
>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
>> GuC objects (which are always pinned in memory and in the GGTT anyway)
>> mapped into kernel address space, rather than mapping and unmapping them
>> on each access.
>>
>> This preliminary patch sets up the pin-and-map for all GuC-specific
>> objects, and updates the various setup/shutdown functions to use these
>> long-term mappings rather than doing their own kmap_atomic() calls.
>>
>> Cc: <tvrtko.ursulin@intel.com>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 37 
>> +++++++++++-------------------
>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>   2 files changed, 14 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index da86bdb..f80f577 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc 
>> *guc,
>>                     struct i915_guc_client *client)
>>   {
>>       struct guc_doorbell_info *doorbell;
>> -    void *base;
>>
>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 
>> 0));
>> -    doorbell = base + client->doorbell_offset;
>> +    doorbell = client->client_base + client->doorbell_offset;
>>
>> -    doorbell->db_status = 1;
>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>       doorbell->cookie = 0;
>> -
>> -    kunmap_atomic(base);
>>   }
>>
>>   static int guc_ring_doorbell(struct i915_guc_client *gc)
>> @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct 
>> intel_guc *guc,
>>   {
>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>       struct guc_doorbell_info *doorbell;
>> -    void *base;
>>       i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>>       int value;
>>
>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 
>> 0));
>> -    doorbell = base + client->doorbell_offset;
>> -
>> -    doorbell->db_status = 0;
>> +    doorbell = client->client_base + client->doorbell_offset;
>
> Not 100% sure of the object lifetimes in GuC, but would it be even 
> simpler to store a pointer to struct struct guc_doorbell_info as 
> guc->doorbell ? There aren't that many call sites true, but kind of 
> looks logical at least from the outside.

Well probably, but that would be a separate patch. This is just dealing 
with eliminating the repeated kmap/unmap calls.

Also, maybe this helps remind people that these are actually parts of 
the same object. There's just one allocated, but it encompasses the 
process descriptor and the doorbell in the first page, and the workqueue 
in the second and third.
>> -    kunmap_atomic(base);
>> +    doorbell->db_status = GUC_DOORBELL_DISABLED;
>>
>>       I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>>
>> @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc 
>> *guc,
>>                      struct i915_guc_client *client)
>>   {
>>       struct guc_process_desc *desc;
>> -    void *base;
>>
>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 
>> 0));
>> -    desc = base + client->proc_desc_offset;
>> +    desc = client->client_base + client->proc_desc_offset;
>
> And the same maybe for this?
>
>>       memset(desc, 0, sizeof(*desc));
>>
>> @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc 
>> *guc,
>>       desc->wq_size_bytes = client->wq_size;
>>       desc->wq_status = WQ_STATUS_ACTIVE;
>>       desc->priority = client->priority;
>> -
>> -    kunmap_atomic(base);
>>   }
>>
>>   /*
>> @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
>>    * This is a wrapper to create a gem obj. In order to use it inside 
>> GuC, the
>>    * object needs to be pinned lifetime. Also we must pin it to gtt 
>> space other
>>    * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
>> + * The object is also pinned & mapped into kernel address space.
>>    *
>>    * Return:    A drm_i915_gem_object if successful, otherwise NULL.
>>    */
>> @@ -620,13 +609,14 @@ static struct drm_i915_gem_object 
>> *gem_allocate_guc_obj(struct drm_device *dev,
>>       if (!obj)
>>           return NULL;
>>
>> -    if (i915_gem_object_get_pages(obj)) {
>> +    if (i915_gem_object_pin_map(obj) == NULL) {
>
> This should be IS_ERR check.

OK, will update.

>> drm_gem_object_unreference(&obj->base);
>>           return NULL;
>>       }
>>
>>       if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>>               PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
>> +        i915_gem_object_unpin_map(obj);
>>           drm_gem_object_unreference(&obj->base);
>>           return NULL;
>>       }
>> @@ -649,6 +639,8 @@ static void gem_release_guc_obj(struct 
>> drm_i915_gem_object *obj)
>>       if (i915_gem_obj_is_pinned(obj))
>>           i915_gem_object_ggtt_unpin(obj);
>>
>> +    i915_gem_object_unpin_map(obj);
>> +
>>       drm_gem_object_unreference(&obj->base);
>>   }
>>
>> @@ -729,6 +721,8 @@ static struct i915_guc_client 
>> *guc_client_alloc(struct drm_device *dev,
>>           goto err;
>>
>>       client->client_obj = obj;
>> +    client->client_base = obj->mapping;
>
> It think outside code should not access obj->mapping directly but use 
> what i915_gem_object_pin_map has returned.

No, that would be quite inconvenient. You shouldn't need to hold 
auxiliary information about an allocated object when you can get that 
information directly from the object itself.

Also, the function that does the pin-and-map doesn't have access to the 
structure where the address is going to be cached, it just returns the 
allocated-pinned-and-mapped object.

OTOH I have no objection to wrapping it an accessor function/macro.

void *i914_gem_object_mapped_addr(object) ?

returning NULL if object is not mapped?

>> +    WARN_ON(!client->client_base);
>
> And this has already been handled at the i915_gem_object_pin_map call 
> site so I don't think it serves any purpose.

In case the obj->mapping *wasn't* the same value that was returned from 
pin-and-map and checked.

>>       client->wq_offset = GUC_DB_SIZE;
>>       client->wq_size = GUC_WQ_SIZE;
>>
>> @@ -841,7 +835,6 @@ static void guc_create_ads(struct intel_guc *guc)
>>       struct guc_policies *policies;
>>       struct guc_mmio_reg_state *reg_state;
>>       struct intel_engine_cs *engine;
>> -    struct page *page;
>>       u32 size;
>>
>>       /* The ads obj includes the struct itself and buffers passed to 
>> GuC */
>> @@ -857,9 +850,7 @@ static void guc_create_ads(struct intel_guc *guc)
>>
>>           guc->ads_obj = obj;
>>       }
>> -
>> -    page = i915_gem_object_get_page(obj, 0);
>> -    ads = kmap(page);
>> +    ads = obj->mapping;
>
> Same as above. I suggest storing the base address in the guc client or 
> somewhere appropriate.
>
> Or if objects have separate explicit lifetimes, even if they don't 
> overlap, you could even nest i915_gem_object_pin_map and unpin. 
> Depends what makes the code simpler.
All GuC objects have to stay memory-resident and pinned at permanent 
addresses in the GGTT.
For some of them we might not need the kernel mapping all the time, but 
it's probably simpler to keep it.
If we didn't, I would have to change the 'release' code to do the unmap 
iff it was still mapped at that point.

.Dave.
>>       /*
>>        * The GuC requires a "Golden Context" when it reinitialises
>> @@ -897,8 +888,6 @@ static void guc_create_ads(struct intel_guc *guc)
>>
>>       ads->reg_state_buffer = ads->reg_state_addr +
>>               sizeof(struct guc_mmio_reg_state);
>> -
>> -    kunmap(page);
>>   }
>>
>>   /*
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 3bb85b1..9ab3564 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -31,6 +31,7 @@ struct drm_i915_gem_request;
>>
>>   struct i915_guc_client {
>>       struct drm_i915_gem_object *client_obj;
>> +    void *client_base;        /* Mapped address of above    */
>>       struct intel_context *owner;
>>       struct intel_guc *guc;
>>       uint32_t priority;
>>
>
> Regards,
>
> Tvrtko

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-15 11:12   ` Dave Gordon
@ 2016-04-15 11:42     ` Tvrtko Ursulin
  2016-04-18 10:15       ` [PATCH v2 " Dave Gordon
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-04-15 11:42 UTC (permalink / raw)
  To: Dave Gordon, intel-gfx


On 15/04/16 12:12, Dave Gordon wrote:
> On 15/04/2016 11:04, Tvrtko Ursulin wrote:
>>
>> On 14/04/16 18:19, Dave Gordon wrote:
>>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
>>> GuC objects (which are always pinned in memory and in the GGTT anyway)
>>> mapped into kernel address space, rather than mapping and unmapping them
>>> on each access.
>>>
>>> This preliminary patch sets up the pin-and-map for all GuC-specific
>>> objects, and updates the various setup/shutdown functions to use these
>>> long-term mappings rather than doing their own kmap_atomic() calls.
>>>
>>> Cc: <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 37
>>> +++++++++++-------------------
>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>   2 files changed, 14 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index da86bdb..f80f577 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc
>>> *guc,
>>>                     struct i915_guc_client *client)
>>>   {
>>>       struct guc_doorbell_info *doorbell;
>>> -    void *base;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    doorbell = base + client->doorbell_offset;
>>> +    doorbell = client->client_base + client->doorbell_offset;
>>>
>>> -    doorbell->db_status = 1;
>>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>>       doorbell->cookie = 0;
>>> -
>>> -    kunmap_atomic(base);
>>>   }
>>>
>>>   static int guc_ring_doorbell(struct i915_guc_client *gc)
>>> @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct
>>> intel_guc *guc,
>>>   {
>>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>>       struct guc_doorbell_info *doorbell;
>>> -    void *base;
>>>       i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>>>       int value;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    doorbell = base + client->doorbell_offset;
>>> -
>>> -    doorbell->db_status = 0;
>>> +    doorbell = client->client_base + client->doorbell_offset;
>>
>> Not 100% sure of the object lifetimes in GuC, but would it be even
>> simpler to store a pointer to struct struct guc_doorbell_info as
>> guc->doorbell ? There aren't that many call sites true, but kind of
>> looks logical at least from the outside.
>
> Well probably, but that would be a separate patch. This is just dealing
> with eliminating the repeated kmap/unmap calls.

Okay, just thought it may be easier to do it at once since it looked 
really straightforward, like same amount of work and cleaner end result.

> Also, maybe this helps remind people that these are actually parts of
> the same object. There's just one allocated, but it encompasses the
> process descriptor and the doorbell in the first page, and the workqueue
> in the second and third.

I don't think anyone from the outside would care since it is all 
internal guc code so it is free to define its rules with a nice comment 
in the relevant structure definition.

>>> -    kunmap_atomic(base);
>>> +    doorbell->db_status = GUC_DOORBELL_DISABLED;
>>>
>>>       I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>>>
>>> @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc
>>> *guc,
>>>                      struct i915_guc_client *client)
>>>   {
>>>       struct guc_process_desc *desc;
>>> -    void *base;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    desc = base + client->proc_desc_offset;
>>> +    desc = client->client_base + client->proc_desc_offset;
>>
>> And the same maybe for this?
>>
>>>       memset(desc, 0, sizeof(*desc));
>>>
>>> @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc
>>> *guc,
>>>       desc->wq_size_bytes = client->wq_size;
>>>       desc->wq_status = WQ_STATUS_ACTIVE;
>>>       desc->priority = client->priority;
>>> -
>>> -    kunmap_atomic(base);
>>>   }
>>>
>>>   /*
>>> @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
>>>    * This is a wrapper to create a gem obj. In order to use it inside
>>> GuC, the
>>>    * object needs to be pinned lifetime. Also we must pin it to gtt
>>> space other
>>>    * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
>>> + * The object is also pinned & mapped into kernel address space.
>>>    *
>>>    * Return:    A drm_i915_gem_object if successful, otherwise NULL.
>>>    */
>>> @@ -620,13 +609,14 @@ static struct drm_i915_gem_object
>>> *gem_allocate_guc_obj(struct drm_device *dev,
>>>       if (!obj)
>>>           return NULL;
>>>
>>> -    if (i915_gem_object_get_pages(obj)) {
>>> +    if (i915_gem_object_pin_map(obj) == NULL) {
>>
>> This should be IS_ERR check.
>
> OK, will update.
>
>>> drm_gem_object_unreference(&obj->base);
>>>           return NULL;
>>>       }
>>>
>>>       if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>>>               PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
>>> +        i915_gem_object_unpin_map(obj);
>>>           drm_gem_object_unreference(&obj->base);
>>>           return NULL;
>>>       }
>>> @@ -649,6 +639,8 @@ static void gem_release_guc_obj(struct
>>> drm_i915_gem_object *obj)
>>>       if (i915_gem_obj_is_pinned(obj))
>>>           i915_gem_object_ggtt_unpin(obj);
>>>
>>> +    i915_gem_object_unpin_map(obj);
>>> +
>>>       drm_gem_object_unreference(&obj->base);
>>>   }
>>>
>>> @@ -729,6 +721,8 @@ static struct i915_guc_client
>>> *guc_client_alloc(struct drm_device *dev,
>>>           goto err;
>>>
>>>       client->client_obj = obj;
>>> +    client->client_base = obj->mapping;
>>
>> It think outside code should not access obj->mapping directly but use
>> what i915_gem_object_pin_map has returned.
>
> No, that would be quite inconvenient. You shouldn't need to hold
> auxiliary information about an allocated object when you can get that
> information directly from the object itself.

You can not unless you break the API layer. obj->mapping is IMHO private 
to GEM and GuC should not touch it. Even must not IMHO.

> Also, the function that does the pin-and-map doesn't have access to the
> structure where the address is going to be cached, it just returns the
> allocated-pinned-and-mapped object.

That sounds like a local issue which can be worked around by storing it 
in the appropriate GuC data structure, no?

> OTOH I have no objection to wrapping it an accessor function/macro.
>
> void *i914_gem_object_mapped_addr(object) ?
>
> returning NULL if object is not mapped?

I don't feel strongly either way. Question for Chris I suppose.

In this particular case I would look to avoid the need for it by storing 
the address in my own data structure(s), if they have constructors and 
destructors which start and end with i915_gem_object_pin_map/unmap 
respectively.

>>> +    WARN_ON(!client->client_base);
>>
>> And this has already been handled at the i915_gem_object_pin_map call
>> site so I don't think it serves any purpose.
>
> In case the obj->mapping *wasn't* the same value that was returned from
> pin-and-map and checked.

Ah guard against touching forbidden parts. :)

Regards,

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-15 11:42     ` Tvrtko Ursulin
@ 2016-04-18 10:15       ` Dave Gordon
  2016-04-18 10:15         ` [PATCH v2 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
                           ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-18 10:15 UTC (permalink / raw)
  To: intel-gfx

With the new i915_gem_obj_pin_map() interface, it makes sense to keep
GuC objects (which are always pinned in memory and in the GGTT anyway)
mapped into kernel address space, rather than mapping and unmapping them
on each access.

This preliminary patch sets up the pin-and-map for all GuC-specific
objects, and updates the various setup/shutdown functions to use these
long-term mappings rather than doing their own kmap_atomic() calls.

v2:
    Invent & use accessor function i915_object_mapped_address() rather
    than accessing obj->mapping directly (Tvrtko Ursulin)
    Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
    Added big comment about struct i915_guc_client & usage of the GEM
    object that it describes 

Cc: <tvrtko.ursulin@intel.com>
Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
 drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
 drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
 3 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 85102ad..439f149 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
 
 /**
+ * i915_object_mapped_address - address at which a GEM object is mapped
+ * @obj - the object (presumably already mapped into kernel address space)
+ *
+ * Returns the address at which an object has been mapped by a call to
+ * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
+ */
+static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
+{
+       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
+}
+
+/**
  * i915_gem_object_unpin_map - releases an earlier mapping
  * @obj - the object to unmap
  *
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da86bdb..de96306 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
 	struct guc_doorbell_info *doorbell;
-	void *base;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	doorbell = base + client->doorbell_offset;
+	doorbell = client->client_base + client->doorbell_offset;
 
-	doorbell->db_status = 1;
+	doorbell->db_status = GUC_DOORBELL_ENABLED;
 	doorbell->cookie = 0;
-
-	kunmap_atomic(base);
 }
 
 static int guc_ring_doorbell(struct i915_guc_client *gc)
@@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct intel_guc *guc,
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct guc_doorbell_info *doorbell;
-	void *base;
 	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
 	int value;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	doorbell = base + client->doorbell_offset;
-
-	doorbell->db_status = 0;
+	doorbell = client->client_base + client->doorbell_offset;
 
-	kunmap_atomic(base);
+	doorbell->db_status = GUC_DOORBELL_DISABLED;
 
 	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
 
@@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc *guc,
 			       struct i915_guc_client *client)
 {
 	struct guc_process_desc *desc;
-	void *base;
 
-	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
-	desc = base + client->proc_desc_offset;
+	desc = client->client_base + client->proc_desc_offset;
 
 	memset(desc, 0, sizeof(*desc));
 
@@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
 	desc->wq_size_bytes = client->wq_size;
 	desc->wq_status = WQ_STATUS_ACTIVE;
 	desc->priority = client->priority;
-
-	kunmap_atomic(base);
 }
 
 /*
@@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
  * This is a wrapper to create a gem obj. In order to use it inside GuC, the
  * object needs to be pinned lifetime. Also we must pin it to gtt space other
  * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
+ * The object is also pinned & mapped into kernel address space.
  *
  * Return:	A drm_i915_gem_object if successful, otherwise NULL.
  */
@@ -620,13 +609,14 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
 	if (!obj)
 		return NULL;
 
-	if (i915_gem_object_get_pages(obj)) {
+	if (IS_ERR(i915_gem_object_pin_map(obj))) {
 		drm_gem_object_unreference(&obj->base);
 		return NULL;
 	}
 
 	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
 			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
+		i915_gem_object_unpin_map(obj);
 		drm_gem_object_unreference(&obj->base);
 		return NULL;
 	}
@@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
 	if (i915_gem_obj_is_pinned(obj))
 		i915_gem_object_ggtt_unpin(obj);
 
+	if (i915_object_mapped_address(obj))
+		i915_gem_object_unpin_map(obj);
+
 	drm_gem_object_unreference(&obj->base);
 }
 
@@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 		goto err;
 
 	client->client_obj = obj;
+	client->client_base = i915_object_mapped_address(obj);
+	WARN_ON(!client->client_base);
 	client->wq_offset = GUC_DB_SIZE;
 	client->wq_size = GUC_WQ_SIZE;
 
@@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
 	struct guc_policies *policies;
 	struct guc_mmio_reg_state *reg_state;
 	struct intel_engine_cs *engine;
-	struct page *page;
 	u32 size;
 
 	/* The ads obj includes the struct itself and buffers passed to GuC */
@@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
 
 		guc->ads_obj = obj;
 	}
-
-	page = i915_gem_object_get_page(obj, 0);
-	ads = kmap(page);
+	ads = i915_object_mapped_address(obj);
 
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
@@ -897,8 +889,6 @@ static void guc_create_ads(struct intel_guc *guc)
 
 	ads->reg_state_buffer = ads->reg_state_addr +
 			sizeof(struct guc_mmio_reg_state);
-
-	kunmap(page);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3bb85b1..f2c051e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -29,8 +29,31 @@
 
 struct drm_i915_gem_request;
 
+/*
+ * This structure primarily describes the GEM object shared with the GuC.
+ * The GEM object is held for the entire lifetime of our interaction with
+ * the GuC, being allocated before the GuC is loaded with its firmware. 
+ * Because there's no way to update the address used by the GuC after
+ * initialisation, the shared object must stay pinned into the GGTT as
+ * long as the GuC is in use. We also keep it mapped into kernel address
+ * space, as it includes shared data that must be updated on every request
+ * submission.
+ *
+ * The single GEM object described here is actually made up of several
+ * separate areas, as far as the GuC is concerned. The first page includes
+ * the "process decriptor" which holds sequence data for the doorbell, and
+ * one cacheline which actually *is* the doorbell; a write to this will
+ * "ring" the doorbell (i.e. send an interrupt to the GuC). The subsequent
+ * pages of the client object constitute the work queue (a circular array
+ * of work items), again described in the process descriptor.
+ *
+ * Finally, we also keep a few statistics here, including the number of
+ * submissions to each engine, and a record of the last submission failure
+ * (if any).
+ */
 struct i915_guc_client {
 	struct drm_i915_gem_object *client_obj;
+	void *client_base;		/* Mapped address of above	*/
 	struct intel_context *owner;
 	struct intel_guc *guc;
 	uint32_t priority;
@@ -52,6 +75,7 @@ struct i915_guc_client {
 	uint32_t q_fail;
 	uint32_t b_fail;
 	int retcode;
+	int spare;			/* pad to 32 DWords		*/
 };
 
 enum intel_guc_fw_status {
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] drm/i915/guc: stop using kmap_atomic() during request submission
  2016-04-18 10:15       ` [PATCH v2 " Dave Gordon
@ 2016-04-18 10:15         ` Dave Gordon
  2016-04-18 10:15         ` [PATCH v2 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
  2016-04-18 10:25         ` [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel Chris Wilson
  2 siblings, 0 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-18 10:15 UTC (permalink / raw)
  To: intel-gfx; +Cc: daniel.vetter

From: Alex Dai <yu.dai@intel.com>

Now that we keep GuC shared objects mapped into kernel space for their
entire lifetime, we can simplify the code for accessing the doorbells
and work queue, which were previously calling kmap_atomic() on *each*
request submission.

This patch fixes the BUG shown below, where the thread could sleep
while holding the kmap_atomic mapping.

[   34.098798] BUG: scheduling while atomic:
gem_close_race/1941/0x00000002
[   34.098822] Modules linked in: hid_generic usbhid i915 asix usbnet
libphy mii i2c_algo_bit drm_kms_helper cfbfillrect syscopyarea cfbimgblt
sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm coretemp i2c_hid hid
video pinctrl_sunrisepoint pinctrl_intel acpi_pad nls_iso8859_1 e1000e
ptp psmouse pps_core ahci libahci
[   34.098824] CPU: 0 PID: 1941 Comm: gem_close_race Tainted: G     U
          4.4.0-160121+ #123
[   34.098824] Hardware name: Intel Corporation Skylake Client
platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551
09/22/2015
[   34.098825]  0000000000013e40 ffff880166c27a78 ffffffff81280d02
ffff880172c13e40
[   34.098826]  ffff880166c27a88 ffffffff810c203a ffff880166c27ac8
ffffffff814ec808
[   34.098827]  ffff88016b7c6000 ffff880166c28000 00000000000f4240
0000000000000001
[   34.098827] Call Trace:
[   34.098831]  [<ffffffff81280d02>] dump_stack+0x4b/0x79
[   34.098833]  [<ffffffff810c203a>] __schedule_bug+0x41/0x4f
[   34.098834]  [<ffffffff814ec808>] __schedule+0x5a8/0x690
[   34.098835]  [<ffffffff814ec927>] schedule+0x37/0x80
[   34.098836]  [<ffffffff814ef3fd>]
schedule_hrtimeout_range_clock+0xad/0x130
[   34.098837]  [<ffffffff81090be0>] ? hrtimer_init+0x10/0x10
[   34.098838]  [<ffffffff814ef3f1>] ?
schedule_hrtimeout_range_clock+0xa1/0x130
[   34.098839]  [<ffffffff814ef48e>] schedule_hrtimeout_range+0xe/0x10
[   34.098840]  [<ffffffff814eef9b>] usleep_range+0x3b/0x40
[   34.098853]  [<ffffffffa01ec109>] i915_guc_wq_check_space+0x119/0x210
[i915]
[   34.098861]  [<ffffffffa01da47c>]
intel_logical_ring_alloc_request_extras+0x5c/0x70 [i915]
[   34.098869]  [<ffffffffa01cdbf1>] i915_gem_request_alloc+0x91/0x170
[i915]
[   34.098875]  [<ffffffffa01c1c07>]
i915_gem_do_execbuffer.isra.25+0xbc7/0x12a0 [i915]
[   34.098882]  [<ffffffffa01cb785>] ?
i915_gem_object_get_pages_gtt+0x225/0x3c0 [i915]
[   34.098889]  [<ffffffffa01d1fb6>] ? i915_gem_pwrite_ioctl+0xd6/0x9f0
[i915]
[   34.098895]  [<ffffffffa01c2e68>] i915_gem_execbuffer2+0xa8/0x250
[i915]
[   34.098900]  [<ffffffffa00f65d8>] drm_ioctl+0x258/0x4f0 [drm]
[   34.098906]  [<ffffffffa01c2dc0>] ? i915_gem_execbuffer+0x340/0x340
[i915]
[   34.098908]  [<ffffffff8111590d>] do_vfs_ioctl+0x2cd/0x4a0
[   34.098909]  [<ffffffff8111eac2>] ? __fget+0x72/0xb0
[   34.098910]  [<ffffffff81115b1c>] SyS_ioctl+0x3c/0x70
[   34.098911]  [<ffffffff814effd7>] entry_SYSCALL_64_fastpath+0x12/0x6a
[   34.100208] ------------[ cut here ]------------

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93847
Cc: <daniel.vetter@ffwll.ch>
Cc: <tvrtko.ursulin@intel.com>
Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index de96306..35231fd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -191,11 +191,9 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 	struct guc_process_desc *desc;
 	union guc_doorbell_qw db_cmp, db_exc, db_ret;
 	union guc_doorbell_qw *db;
-	void *base;
 	int attempt = 2, ret = -EAGAIN;
 
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
+	desc = gc->client_base + gc->proc_desc_offset;
 
 	/* Update the tail so it is visible to GuC */
 	desc->tail = gc->wq_tail;
@@ -211,7 +209,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 		db_exc.cookie = 1;
 
 	/* pointer of current doorbell cacheline */
-	db = base + gc->doorbell_offset;
+	db = gc->client_base + gc->doorbell_offset;
 
 	while (attempt--) {
 		/* lets ring the doorbell */
@@ -243,7 +241,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 	/* Finally, update the cached copy of the GuC's WQ head */
 	gc->wq_head = desc->head;
 
-	kunmap_atomic(base);
 	return ret;
 }
 
@@ -462,7 +459,6 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 int i915_guc_wq_check_space(struct i915_guc_client *gc)
 {
 	struct guc_process_desc *desc;
-	void *base;
 	u32 size = sizeof(struct guc_wq_item);
 	int ret = -ETIMEDOUT, timeout_counter = 200;
 
@@ -474,8 +470,7 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
 		return 0;
 
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
+	desc = gc->client_base + gc->proc_desc_offset;
 
 	while (timeout_counter-- > 0) {
 		gc->wq_head = desc->head;
@@ -489,8 +484,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 			usleep_range(1000, 2000);
 	};
 
-	kunmap_atomic(base);
-
 	return ret;
 }
 
@@ -498,7 +491,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 				  struct drm_i915_gem_request *rq)
 {
 	struct guc_wq_item *wqi;
-	void *base;
 	u32 tail, wq_len, wq_off, space;
 
 	space = CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size);
@@ -521,10 +513,7 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	WARN_ON(wq_off & 3);
 
 	/* wq starts from the page after doorbell / process_desc */
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
-	wq_off &= PAGE_SIZE - 1;
-	wqi = (struct guc_wq_item *)((char *)base + wq_off);
+	wqi = gc->client_base + GUC_DB_SIZE + wq_off;
 
 	/* len does not include the header */
 	wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
@@ -542,8 +531,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
 	wqi->fence_id = 0; /*XXX: what fence to be here */
 
-	kunmap_atomic(base);
-
 	return 0;
 }
 
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] drm/i915/guc: drop cached copy of 'wq_head'
  2016-04-18 10:15       ` [PATCH v2 " Dave Gordon
  2016-04-18 10:15         ` [PATCH v2 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
@ 2016-04-18 10:15         ` Dave Gordon
  2016-04-18 10:25         ` [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel Chris Wilson
  2 siblings, 0 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-18 10:15 UTC (permalink / raw)
  To: intel-gfx

From: Alex Dai <yu.dai@intel.com>

Now that we keep the GuC client structure (and therefore the process
descriptor embedded therein) permanently mapped, we don't really need
to keep a local copy of the GuC's work-queue-head. So we can simplify
the code a little by not doing this. Also, optimise away a few calls
by caching results in local variables.

v2:
    Added local optimisations (Dave Gordon)

Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 54 ++++++++++++------------------
 drivers/gpu/drm/i915/intel_guc.h           |  2 +-
 2 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 35231fd..5d35570 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -238,9 +238,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 			db_exc.cookie = 1;
 	}
 
-	/* Finally, update the cached copy of the GuC's WQ head */
-	gc->wq_head = desc->head;
-
 	return ret;
 }
 
@@ -361,12 +358,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
 static void guc_init_ctx_desc(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
+	struct drm_i915_gem_object *client_obj = client->client_obj;
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx = client->owner;
 	struct guc_context_desc desc;
 	struct sg_table *sg;
 	enum intel_engine_id id;
+	u32 gfx_addr;
 
 	memset(&desc, 0, sizeof(desc));
 
@@ -395,16 +394,17 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 		lrc->context_desc = (u32)ctx_desc;
 
 		/* The state page is after PPHWSP */
-		lrc->ring_lcra = i915_gem_obj_ggtt_offset(obj) +
-				LRC_STATE_PN * PAGE_SIZE;
+		gfx_addr = i915_gem_obj_ggtt_offset(obj);
+		lrc->ring_lcra = gfx_addr + LRC_STATE_PN * PAGE_SIZE;
 		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
 				(engine->guc_id << GUC_ELC_ENGINE_OFFSET);
 
 		obj = ctx->engine[id].ringbuf->obj;
+		gfx_addr = i915_gem_obj_ggtt_offset(obj);
 
-		lrc->ring_begin = i915_gem_obj_ggtt_offset(obj);
-		lrc->ring_end = lrc->ring_begin + obj->base.size - 1;
-		lrc->ring_next_free_location = lrc->ring_begin;
+		lrc->ring_begin = gfx_addr;
+		lrc->ring_end = gfx_addr + obj->base.size - 1;
+		lrc->ring_next_free_location = gfx_addr;
 		lrc->ring_current_tail_pointer_value = 0;
 
 		desc.engines_used |= (1 << engine->guc_id);
@@ -413,22 +413,17 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	WARN_ON(desc.engines_used == 0);
 
 	/*
-	 * The CPU address is only needed at certain points, so kmap_atomic on
-	 * demand instead of storing it in the ctx descriptor.
-	 * XXX: May make debug easier to have it mapped
+	 * The doorbell, process descriptor, and workqueue are all parts
+	 * of the client object, which the GuC will reference via the GGTT
 	 */
-	desc.db_trigger_cpu = 0;
-	desc.db_trigger_uk = client->doorbell_offset +
-		i915_gem_obj_ggtt_offset(client->client_obj);
-	desc.db_trigger_phy = client->doorbell_offset +
-		sg_dma_address(client->client_obj->pages->sgl);
-
-	desc.process_desc = client->proc_desc_offset +
-		i915_gem_obj_ggtt_offset(client->client_obj);
-
-	desc.wq_addr = client->wq_offset +
-		i915_gem_obj_ggtt_offset(client->client_obj);
-
+	gfx_addr = i915_gem_obj_ggtt_offset(client_obj);
+	desc.db_trigger_phy = sg_dma_address(client_obj->pages->sgl) +
+				client->doorbell_offset;
+	desc.db_trigger_cpu = (uintptr_t)client->client_base +
+				client->doorbell_offset;
+	desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
+	desc.process_desc = gfx_addr + client->proc_desc_offset;
+	desc.wq_addr = gfx_addr + client->wq_offset;
 	desc.wq_size = client->wq_size;
 
 	/*
@@ -465,17 +460,10 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 	if (!gc)
 		return 0;
 
-	/* Quickly return if wq space is available since last time we cache the
-	 * head position. */
-	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
-		return 0;
-
 	desc = gc->client_base + gc->proc_desc_offset;
 
 	while (timeout_counter-- > 0) {
-		gc->wq_head = desc->head;
-
-		if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
+		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
 			ret = 0;
 			break;
 		}
@@ -490,10 +478,12 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
 static int guc_add_workqueue_item(struct i915_guc_client *gc,
 				  struct drm_i915_gem_request *rq)
 {
+	struct guc_process_desc *desc;
 	struct guc_wq_item *wqi;
 	u32 tail, wq_len, wq_off, space;
 
-	space = CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size);
+	desc = gc->client_base + gc->proc_desc_offset;
+	space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
 	if (WARN_ON(space < sizeof(struct guc_wq_item)))
 		return -ENOSPC; /* shouldn't happen */
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index f2c051e..cc32a18 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -68,7 +68,7 @@ struct i915_guc_client {
 	uint32_t wq_offset;
 	uint32_t wq_size;
 	uint32_t wq_tail;
-	uint32_t wq_head;
+	uint32_t unused;		/* Was 'wq_head'		*/
 
 	/* GuC submission statistics & status */
 	uint64_t submissions[GUC_MAX_ENGINES_NUM];
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-18 10:15       ` [PATCH v2 " Dave Gordon
  2016-04-18 10:15         ` [PATCH v2 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
  2016-04-18 10:15         ` [PATCH v2 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
@ 2016-04-18 10:25         ` Chris Wilson
  2016-04-18 11:28           ` Dave Gordon
  2 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2016-04-18 10:25 UTC (permalink / raw)
  To: Dave Gordon; +Cc: intel-gfx

On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote:
> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
> GuC objects (which are always pinned in memory and in the GGTT anyway)
> mapped into kernel address space, rather than mapping and unmapping them
> on each access.
> 
> This preliminary patch sets up the pin-and-map for all GuC-specific
> objects, and updates the various setup/shutdown functions to use these
> long-term mappings rather than doing their own kmap_atomic() calls.
> 
> v2:
>     Invent & use accessor function i915_object_mapped_address() rather
>     than accessing obj->mapping directly (Tvrtko Ursulin)
>     Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
>     Added big comment about struct i915_guc_client & usage of the GEM
>     object that it describes 
> 
> Cc: <tvrtko.ursulin@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
>  drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
>  drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
>  3 files changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 85102ad..439f149 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>  void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
>  
>  /**
> + * i915_object_mapped_address - address at which a GEM object is mapped
> + * @obj - the object (presumably already mapped into kernel address space)
> + *
> + * Returns the address at which an object has been mapped by a call to
> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
> + */
> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
> +{
> +       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
> +}

No.

> @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>  	if (i915_gem_obj_is_pinned(obj))
>  		i915_gem_object_ggtt_unpin(obj);
>  
> +	if (i915_object_mapped_address(obj))
> +		i915_gem_object_unpin_map(obj);

Eh? If you aren't tracking your pin, you can't just randomly free
someone elses.

>  	drm_gem_object_unreference(&obj->base);
>  }
>  
> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>  		goto err;
>  
>  	client->client_obj = obj;
> +	client->client_base = i915_object_mapped_address(obj);
> +	WARN_ON(!client->client_base);
>  	client->wq_offset = GUC_DB_SIZE;
>  	client->wq_size = GUC_WQ_SIZE;
>  
> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
>  	struct guc_policies *policies;
>  	struct guc_mmio_reg_state *reg_state;
>  	struct intel_engine_cs *engine;
> -	struct page *page;
>  	u32 size;
>  
>  	/* The ads obj includes the struct itself and buffers passed to GuC */
> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
>  
>  		guc->ads_obj = obj;
>  	}
> -
> -	page = i915_gem_object_get_page(obj, 0);
> -	ads = kmap(page);
> +	ads = i915_object_mapped_address(obj);

You need to explicitly aknowlege that you are using the memory tracked
by the object otherwise it *will* be removed by the shrinker.
So what is wrong with using i915_gem_object_pin_map()?
-Chris

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-18 10:25         ` [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel Chris Wilson
@ 2016-04-18 11:28           ` Dave Gordon
  2016-04-18 11:37             ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Dave Gordon @ 2016-04-18 11:28 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

On 18/04/16 11:25, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote:
>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
>> GuC objects (which are always pinned in memory and in the GGTT anyway)
>> mapped into kernel address space, rather than mapping and unmapping them
>> on each access.
>>
>> This preliminary patch sets up the pin-and-map for all GuC-specific
>> objects, and updates the various setup/shutdown functions to use these
>> long-term mappings rather than doing their own kmap_atomic() calls.
>>
>> v2:
>>      Invent & use accessor function i915_object_mapped_address() rather
>>      than accessing obj->mapping directly (Tvrtko Ursulin)
>>      Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
>>      Added big comment about struct i915_guc_client & usage of the GEM
>>      object that it describes
>>
>> Cc: <tvrtko.ursulin@intel.com>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
>>   drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
>>   3 files changed, 50 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 85102ad..439f149 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>   void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
>>
>>   /**
>> + * i915_object_mapped_address - address at which a GEM object is mapped
>> + * @obj - the object (presumably already mapped into kernel address space)
>> + *
>> + * Returns the address at which an object has been mapped by a call to
>> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
>> + */
>> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
>> +{
>> +       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
>> +}
>
> No.

The kernel address at which an object is pinned is just as much a 
property of the object as its GGTT address, so should have a similar 
function for retrieving it.

The pages_pin_count check is there because once that goes to zero, the 
object doesn't logically /have/ a memory address, even if 'mapping' is 
still set.

Or was it just the name you objected to?

>> @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>>   	if (i915_gem_obj_is_pinned(obj))
>>   		i915_gem_object_ggtt_unpin(obj);
>>
>> +	if (i915_object_mapped_address(obj))
>> +		i915_gem_object_unpin_map(obj);
>
> Eh? If you aren't tracking your pin, you can't just randomly free
> someone elses.

The GuC code exclusively owns this object, so it can't be "someone 
else's". The pages_pin count here will always be 1 (at present). This 
was to allow for the possibility that we already chose to release the 
pin-and-map while leaving the object in the GGTT, in which case the 
pages_pin_count will be 0. (We don't do this, at present, but see below).

>>   	drm_gem_object_unreference(&obj->base);
>>   }
>>
>> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>>   		goto err;
>>
>>   	client->client_obj = obj;
>> +	client->client_base = i915_object_mapped_address(obj);
>> +	WARN_ON(!client->client_base);
>>   	client->wq_offset = GUC_DB_SIZE;
>>   	client->wq_size = GUC_WQ_SIZE;
>>
>> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
>>   	struct guc_policies *policies;
>>   	struct guc_mmio_reg_state *reg_state;
>>   	struct intel_engine_cs *engine;
>> -	struct page *page;
>>   	u32 size;
>>
>>   	/* The ads obj includes the struct itself and buffers passed to GuC */
>> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
>>
>>   		guc->ads_obj = obj;
>>   	}
>> -
>> -	page = i915_gem_object_get_page(obj, 0);
>> -	ads = kmap(page);
>> +	ads = i915_object_mapped_address(obj);
>
> You need to explicitly aknowlege that you are using the memory tracked
> by the object otherwise it *will* be removed by the shrinker.
> So what is wrong with using i915_gem_object_pin_map()?
> -Chris

The object is pinned-and-mapped just once, when it's created, and kept 
that way until it's destroyed. Calling pin-and-map here would give it a 
pin count of two, which would be redundant, and require us to 
unpin-and-map it again at the end of the function - which just seems a 
waste of time.

I also considered dropping the original pin-and-map on the ADS object 
once this function has finished with it, but decided it wasn't worth the 
extra work. It has to stay in memory (and in the GGTT) anyway, so it 
would only save a few pages of vmap space. If we did that, the check on 
whether the object was still mapped (in gem_release_guc_obj() above) 
would absolutely be necessary.

.Dave.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-18 11:28           ` Dave Gordon
@ 2016-04-18 11:37             ` Chris Wilson
  2016-04-19 12:23               ` Dave Gordon
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2016-04-18 11:37 UTC (permalink / raw)
  To: Dave Gordon; +Cc: intel-gfx

On Mon, Apr 18, 2016 at 12:28:43PM +0100, Dave Gordon wrote:
> On 18/04/16 11:25, Chris Wilson wrote:
> >On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote:
> >>With the new i915_gem_obj_pin_map() interface, it makes sense to keep
> >>GuC objects (which are always pinned in memory and in the GGTT anyway)
> >>mapped into kernel address space, rather than mapping and unmapping them
> >>on each access.
> >>
> >>This preliminary patch sets up the pin-and-map for all GuC-specific
> >>objects, and updates the various setup/shutdown functions to use these
> >>long-term mappings rather than doing their own kmap_atomic() calls.
> >>
> >>v2:
> >>     Invent & use accessor function i915_object_mapped_address() rather
> >>     than accessing obj->mapping directly (Tvrtko Ursulin)
> >>     Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
> >>     Added big comment about struct i915_guc_client & usage of the GEM
> >>     object that it describes
> >>
> >>Cc: <tvrtko.ursulin@intel.com>
> >>Signed-off-by: Alex Dai <yu.dai@intel.com>
> >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
> >>  drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
> >>  drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
> >>  3 files changed, 50 insertions(+), 24 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 85102ad..439f149 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
> >>  void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
> >>
> >>  /**
> >>+ * i915_object_mapped_address - address at which a GEM object is mapped
> >>+ * @obj - the object (presumably already mapped into kernel address space)
> >>+ *
> >>+ * Returns the address at which an object has been mapped by a call to
> >>+ * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
> >>+ */
> >>+static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
> >>+{
> >>+       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
> >>+}
> >
> >No.
> 
> The kernel address at which an object is pinned is just as much a
> property of the object as its GGTT address, so should have a similar
> function for retrieving it.

An object doesn't have a GGTT, a vma does. The object has vma. To use
the vma, you must pin it.
 
> The pages_pin_count check is there because once that goes to zero,
> the object doesn't logically /have/ a memory address, even if
> 'mapping' is still set.
> 
> Or was it just the name you objected to?

Name and function. obj->mapping is tied into the shinker which can and
will run at any time, stealing all locks. Using obj->mapping which
holding an actual page reference for yourself is broken.
> 

> >>@@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
> >>  	if (i915_gem_obj_is_pinned(obj))
> >>  		i915_gem_object_ggtt_unpin(obj);
> >>
> >>+	if (i915_object_mapped_address(obj))
> >>+		i915_gem_object_unpin_map(obj);
> >
> >Eh? If you aren't tracking your pin, you can't just randomly free
> >someone elses.
> 
> The GuC code exclusively owns this object, so it can't be "someone
> else's". The pages_pin count here will always be 1 (at present).
> This was to allow for the possibility that we already chose to
> release the pin-and-map while leaving the object in the GGTT, in
> which case the pages_pin_count will be 0. (We don't do this, at
> present, but see below).

That is bad justification for adding unsafe code.

> >>  	drm_gem_object_unreference(&obj->base);
> >>  }
> >>
> >>@@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
> >>  		goto err;
> >>
> >>  	client->client_obj = obj;
> >>+	client->client_base = i915_object_mapped_address(obj);
> >>+	WARN_ON(!client->client_base);
> >>  	client->wq_offset = GUC_DB_SIZE;
> >>  	client->wq_size = GUC_WQ_SIZE;
> >>
> >>@@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
> >>  	struct guc_policies *policies;
> >>  	struct guc_mmio_reg_state *reg_state;
> >>  	struct intel_engine_cs *engine;
> >>-	struct page *page;
> >>  	u32 size;
> >>
> >>  	/* The ads obj includes the struct itself and buffers passed to GuC */
> >>@@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
> >>
> >>  		guc->ads_obj = obj;
> >>  	}
> >>-
> >>-	page = i915_gem_object_get_page(obj, 0);
> >>-	ads = kmap(page);
> >>+	ads = i915_object_mapped_address(obj);
> >
> >You need to explicitly aknowlege that you are using the memory tracked
> >by the object otherwise it *will* be removed by the shrinker.
> >So what is wrong with using i915_gem_object_pin_map()?
> >-Chris
> 
> The object is pinned-and-mapped just once, when it's created, and
> kept that way until it's destroyed. Calling pin-and-map here would
> give it a pin count of two, which would be redundant, and require us
> to unpin-and-map it again at the end of the function - which just
> seems a waste of time.

Considering the alternative is an unsound API. Just do it, or pass
around the mapping in a local you have already pinned.
-Chris

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel
  2016-04-18 11:37             ` Chris Wilson
@ 2016-04-19 12:23               ` Dave Gordon
  0 siblings, 0 replies; 15+ messages in thread
From: Dave Gordon @ 2016-04-19 12:23 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

On 18/04/16 12:37, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:28:43PM +0100, Dave Gordon wrote:
>> On 18/04/16 11:25, Chris Wilson wrote:
>>> On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote:
>>>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
>>>> GuC objects (which are always pinned in memory and in the GGTT anyway)
>>>> mapped into kernel address space, rather than mapping and unmapping them
>>>> on each access.
>>>>
>>>> This preliminary patch sets up the pin-and-map for all GuC-specific
>>>> objects, and updates the various setup/shutdown functions to use these
>>>> long-term mappings rather than doing their own kmap_atomic() calls.
>>>>
>>>> v2:
>>>>      Invent & use accessor function i915_object_mapped_address() rather
>>>>      than accessing obj->mapping directly (Tvrtko Ursulin)
>>>>      Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
>>>>      Added big comment about struct i915_guc_client & usage of the GEM
>>>>      object that it describes
>>>>
>>>> Cc: <tvrtko.ursulin@intel.com>
>>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
>>>>   drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
>>>>   3 files changed, 50 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 85102ad..439f149 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>>>   void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
>>>>
>>>>   /**
>>>> + * i915_object_mapped_address - address at which a GEM object is mapped
>>>> + * @obj - the object (presumably already mapped into kernel address space)
>>>> + *
>>>> + * Returns the address at which an object has been mapped by a call to
>>>> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
>>>> + */
>>>> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
>>>> +{
>>>> +       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
>>>> +}
>>>
>>> No.
>>
>> The kernel address at which an object is pinned is just as much a
>> property of the object as its GGTT address, so should have a similar
>> function for retrieving it.
>
> An object doesn't have a GGTT, a vma does. The object has vma. To use
> the vma, you must pin it.

Well you obviously have a very different usage model in mind. The GuC 
object lifecycle is:

    guc_obj::create()
    {
       allocate
       get-pages-into-RAM
       pin-pages-in-RAM
       pin-to-gtt
       kmap
    }

    ~guc_obj()
    {
       undo all the above
    }

    guc_obj::do_stuff()
    {
       addr = self.mapped_address()
       ... // use *addr
    }

where all the code relies on the constructor having set up all mappings 
once and for all; whereas it looks like you're thinking:

    do_stuff(gem_obj)
    {
       addr = pin-and-map(gem_obj);
       ... // use *addr
       unpin-and-map(gem_obj)
    }

where each user of a mapping doesn't know about any others that might 
exist, and therefore has to manage pincounting.

This local-knowledge-only model might be more natural if the function 
were called something like i915_gem_object_mapping_create() and returned 
an blob representing a binding between a GEM object and an address space 
-- in other words, a VMA where the "address space" would be "kernel" 
rather than GGTT or PPGTT (and the unmap function would take the 
returned blob, not the GEM object, as the thing-to-be-unmade). Then we 
could have partial mappings, and mappings with different properties 
(e.g. caching) coexisting.

But the current pin-and-map implements *the* mapping as a singleton 
property of a GEM object, so it looks far more natural to set it up once 
and then expect to read its unchanged value at any time thereafter, 
until the mapping is finally destroyed along with the object.

>> The pages_pin_count check is there because once that goes to zero,
>> the object doesn't logically /have/ a memory address, even if
>> 'mapping' is still set.
>>
>> Or was it just the name you objected to?
>
> Name and function. obj->mapping is tied into the shinker which can and
> will run at any time, stealing all locks. Using obj->mapping which
> holding an actual page reference for yourself is broken.

All GuC objects are permanently pinned in RAM and GGTT, and hence immune 
to the shrinker. If the shrinker does touch them, the GuC will break.

>>>> @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>>>>   	if (i915_gem_obj_is_pinned(obj))
>>>>   		i915_gem_object_ggtt_unpin(obj);
>>>>
>>>> +	if (i915_object_mapped_address(obj))
>>>> +		i915_gem_object_unpin_map(obj);
>>>
>>> Eh? If you aren't tracking your pin, you can't just randomly free
>>> someone elses.
>>
>> The GuC code exclusively owns this object, so it can't be "someone
>> else's". The pages_pin count here will always be 1 (at present).
>> This was to allow for the possibility that we already chose to
>> release the pin-and-map while leaving the object in the GGTT, in
>> which case the pages_pin_count will be 0. (We don't do this, at
>> present, but see below).
>
> That is bad justification for adding unsafe code.

NOT unsafe, because the GuC code holds the only reference to these 
*private* objects. As I said, no code outside this file will ever access 
these objects. And the code above is the destructor, so it would be a 
really bad idea for anyone else to have a pin-and-mapping of the object 
at this point.

>>>>   	drm_gem_object_unreference(&obj->base);
>>>>   }
>>>>
>>>> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>>>>   		goto err;
>>>>
>>>>   	client->client_obj = obj;
>>>> +	client->client_base = i915_object_mapped_address(obj);
>>>> +	WARN_ON(!client->client_base);
>>>>   	client->wq_offset = GUC_DB_SIZE;
>>>>   	client->wq_size = GUC_WQ_SIZE;
>>>>
>>>> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
>>>>   	struct guc_policies *policies;
>>>>   	struct guc_mmio_reg_state *reg_state;
>>>>   	struct intel_engine_cs *engine;
>>>> -	struct page *page;
>>>>   	u32 size;
>>>>
>>>>   	/* The ads obj includes the struct itself and buffers passed to GuC */
>>>> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
>>>>
>>>>   		guc->ads_obj = obj;
>>>>   	}
>>>> -
>>>> -	page = i915_gem_object_get_page(obj, 0);
>>>> -	ads = kmap(page);
>>>> +	ads = i915_object_mapped_address(obj);
>>>
>>> You need to explicitly aknowlege that you are using the memory tracked
>>> by the object otherwise it *will* be removed by the shrinker.
>>> So what is wrong with using i915_gem_object_pin_map()?
>>> -Chris
>>
>> The object is pinned-and-mapped just once, when it's created, and
>> kept that way until it's destroyed. Calling pin-and-map here would
>> give it a pin count of two, which would be redundant, and require us
>> to unpin-and-map it again at the end of the function - which just
>> seems a waste of time.
>
> Considering the alternative is an unsound API. Just do it, or pass
> around the mapping in a local you have already pinned.
> -Chris

No, I've decided just not to use the pin-and-map API at all. We really 
only need direct access to the shared doorbell/process descriptor page, 
so we can just kmap the single page instead. This is more economical in 
use of address space, and simpler to keep away from the shrinker.

.Dave.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-04-19 12:24 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-14 17:19 [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel Dave Gordon
2016-04-14 17:19 ` [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
2016-04-15 10:08   ` Tvrtko Ursulin
2016-04-14 17:19 ` [PATCH 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
2016-04-15  6:55 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/guc: keep GuC objects mapped in kernel Patchwork
2016-04-15 10:04 ` [PATCH 1/3] " Tvrtko Ursulin
2016-04-15 11:12   ` Dave Gordon
2016-04-15 11:42     ` Tvrtko Ursulin
2016-04-18 10:15       ` [PATCH v2 " Dave Gordon
2016-04-18 10:15         ` [PATCH v2 2/3] drm/i915/guc: stop using kmap_atomic() during request submission Dave Gordon
2016-04-18 10:15         ` [PATCH v2 3/3] drm/i915/guc: drop cached copy of 'wq_head' Dave Gordon
2016-04-18 10:25         ` [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel Chris Wilson
2016-04-18 11:28           ` Dave Gordon
2016-04-18 11:37             ` Chris Wilson
2016-04-19 12:23               ` Dave Gordon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.