| Message ID | 1460654342-6071-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > >
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; }