Re: [PATCH 2/3] drm/i915/guc: stop using kmap_atomic() during request submission

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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