From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Nick Hoath <nicholas.hoath@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH] drm/i915: Split alloc from init for lrc
Date: Mon, 21 Sep 2015 11:25:51 +0100 [thread overview]
Message-ID: <55FFDB2F.7020504@linux.intel.com> (raw)
In-Reply-To: <1441972426-6204-1-git-send-email-nicholas.hoath@intel.com>
Hi,
On 09/11/2015 12:53 PM, Nick Hoath wrote:
> Extend init/init_hw split to context init.
> - Move context initialisation in to i915_gem_init_hw
> - Move one off initialisation for render ring to
> i915_gem_validate_context
> - Move default context initialisation to logical_ring_init
>
> Rename intel_lr_context_deferred_create to
> intel_lr_context_deferred_alloc, to reflect reduced functionality &
> alloc/init split.
>
> This patch is intended to split out the allocation of resources &
> initialisation to allow easier reuse of code for resume/gpu reset.
>
> v2: Removed function ptr wrapping of do_switch_context (Daniel Vetter)
> Left ->init_context int intel_lr_context_deferred_alloc
> (Daniel Vetter)
> Remove unnecessary init flag & ring type test. (Daniel Vetter)
> Improve commit message (Daniel Vetter)
> v3: On init/reinit, set the hw next sequence number to the sw next
> sequence number. This is set to 1 at driver load time. This prevents
> the seqno being reset on reinit (Chris Wilson)
> v4: Set seqno back to ~0 - 0x1000 at start-of-day, and increment by 0x100
> on reset.
> This makes it obvious which bbs are which after a reset. (David Gordon
> & John Harrison)
> Rebase.
> v5: Rebase. Fixed rebase breakage. Put context pinning in separate
> function. Removed code churn. (Thomas Daniel)
> v6: Cleanup up issues introduced in v2 & v5 (Thomas Daniel)
>
> Issue: VIZ-4798
> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: John Harrison <john.c.harrison@intel.com>
> Cc: David Gordon <david.s.gordon@intel.com>
> Cc: Thomas Daniel <thomas.daniel@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/i915_gem.c | 22 ++--
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 164 ++++++++++++++---------------
> drivers/gpu/drm/i915/intel_lrc.h | 4 +-
> 5 files changed, 99 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 18859cd..23dd57d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -890,7 +890,6 @@ struct intel_context {
> } legacy_hw_ctx;
>
> /* Execlists */
> - bool rcs_initialized;
> struct {
> struct drm_i915_gem_object *state;
> struct intel_ringbuffer *ringbuf;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5859fc4..e7eb325 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4650,14 +4650,8 @@ int i915_gem_init_rings(struct drm_device *dev)
> goto cleanup_vebox_ring;
> }
>
> - ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
> - if (ret)
> - goto cleanup_bsd2_ring;
> -
> return 0;
>
> -cleanup_bsd2_ring:
> - intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
> cleanup_vebox_ring:
> intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> cleanup_blt_ring:
> @@ -4743,6 +4737,14 @@ i915_gem_init_hw(struct drm_device *dev)
> goto out;
> }
>
> + /*
> + * Increment the next seqno by 0x100 so we have a visible break
> + * on re-initialisation
> + */
> + ret = i915_gem_set_seqno(dev, dev_priv->next_seqno+0x100);
> + if (ret)
> + goto out;
> +
> /* Now it is safe to go back round and do everything else: */
> for_each_ring(ring, dev_priv, i) {
> struct drm_i915_gem_request *req;
> @@ -4944,6 +4946,14 @@ i915_gem_load(struct drm_device *dev)
> dev_priv->num_fence_regs =
> I915_READ(vgtif_reg(avail_rs.fence_num));
>
> + /*
> + * Set initial sequence number for requests.
> + * Using this number allows the wraparound to happen early,
> + * catching any obvious problems.
> + */
> + dev_priv->next_seqno = ((u32)~0 - 0x1100);
> + dev_priv->last_seqno = ((u32)~0 - 0x1101);
> +
> /* Initialize fence registers to zero */
> INIT_LIST_HEAD(&dev_priv->mm.fence_list);
> i915_gem_restore_fences(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a953d49..67ef118 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1009,7 +1009,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
> }
>
> if (i915.enable_execlists && !ctx->engine[ring->id].state) {
> - int ret = intel_lr_context_deferred_create(ctx, ring);
> + int ret = intel_lr_context_deferred_alloc(ctx, ring);
> if (ret) {
> DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
> return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 639da4d..71892dd 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -221,6 +221,9 @@ enum {
> #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
>
> static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
> +static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> + struct drm_i915_gem_object *default_ctx_obj);
> +
>
> /**
> * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
> @@ -978,39 +981,54 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
> return 0;
> }
>
> -static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
> +static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
> + struct drm_i915_gem_object *ctx_obj,
> + struct intel_ringbuffer *ringbuf)
> {
> - struct drm_i915_private *dev_priv = rq->i915;
> - struct intel_engine_cs *ring = rq->ring;
> - struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
> - struct intel_ringbuffer *ringbuf = rq->ringbuf;
> + struct drm_device *dev = ring->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> int ret = 0;
>
> WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> - if (rq->ctx->engine[ring->id].pin_count++ == 0) {
> - ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> - if (ret)
> - goto reset_pin_count;
> + ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> + PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + if (ret)
> + return ret;
>
> - ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
> - if (ret)
> - goto unpin_ctx_obj;
> + ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
> + if (ret)
> + goto unpin_ctx_obj;
>
> - ctx_obj->dirty = true;
> + ctx_obj->dirty = true;
>
> - /* Invalidate GuC TLB. */
> - if (i915.enable_guc_submission)
> - I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> - }
> + /* Invalidate GuC TLB. */
> + if (i915.enable_guc_submission)
> + I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>
> return ret;
>
> unpin_ctx_obj:
> i915_gem_object_ggtt_unpin(ctx_obj);
> +
> + return ret;
> +}
> +
> +static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
> +{
> + int ret = 0;
> + struct intel_engine_cs *ring = rq->ring;
> + struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
> + struct intel_ringbuffer *ringbuf = rq->ringbuf;
> +
> + if (rq->ctx->engine[ring->id].pin_count++ == 0) {
> + ret = intel_lr_context_do_pin(ring, ctx_obj, ringbuf);
> + if (ret)
> + goto reset_pin_count;
> + }
> + return ret;
> +
> reset_pin_count:
> rq->ctx->engine[ring->id].pin_count = 0;
> -
> return ret;
> }
>
> @@ -1420,6 +1438,9 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
> struct drm_device *dev = ring->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + lrc_setup_hardware_status_page(ring,
> + ring->default_context->engine[ring->id].state);
> +
> I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
> I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
>
> @@ -1858,7 +1879,21 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
> if (ret)
> return ret;
>
> - ret = intel_lr_context_deferred_create(ring->default_context, ring);
> + ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
> + if (ret)
> + return ret;
> +
> + /* As this is the default context, always pin it */
> + ret = intel_lr_context_do_pin(
> + ring,
> + ring->default_context->engine[ring->id].state,
> + ring->default_context->engine[ring->id].ringbuf);
> + if (ret) {
> + DRM_ERROR(
> + "Failed to pin and map ringbuffer %s: %d\n",
> + ring->name, ret);
> + return ret;
> + }
>
> return ret;
> }
> @@ -2081,14 +2116,8 @@ int intel_logical_rings_init(struct drm_device *dev)
> goto cleanup_vebox_ring;
> }
>
> - ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
> - if (ret)
> - goto cleanup_bsd2_ring;
> -
> return 0;
>
> -cleanup_bsd2_ring:
> - intel_logical_ring_cleanup(&dev_priv->ring[VCS2]);
> cleanup_vebox_ring:
> intel_logical_ring_cleanup(&dev_priv->ring[VECS]);
> cleanup_blt_ring:
> @@ -2358,7 +2387,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> }
>
> /**
> - * intel_lr_context_deferred_create() - create the LRC specific bits of a context
> + * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
> * @ctx: LR context to create.
> * @ring: engine to be used with the context.
> *
> @@ -2370,12 +2399,11 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> *
> * Return: non-zero on error.
> */
> -int intel_lr_context_deferred_create(struct intel_context *ctx,
> +
> +int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> struct intel_engine_cs *ring)
> {
> - const bool is_global_default_ctx = (ctx == ring->default_context);
> struct drm_device *dev = ring->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *ctx_obj;
> uint32_t context_size;
> struct intel_ringbuffer *ringbuf;
> @@ -2395,82 +2423,50 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
> return -ENOMEM;
> }
>
> - if (is_global_default_ctx) {
> - ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> - if (ret) {
> - DRM_DEBUG_DRIVER("Pin LRC backing obj failed: %d\n",
> - ret);
> - drm_gem_object_unreference(&ctx_obj->base);
> - return ret;
> - }
> -
> - /* Invalidate GuC TLB. */
> - if (i915.enable_guc_submission)
> - I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> - }
> -
> ringbuf = intel_engine_create_ringbuffer(ring, 4 * PAGE_SIZE);
> if (IS_ERR(ringbuf)) {
> ret = PTR_ERR(ringbuf);
> - goto error_unpin_ctx;
> - }
> -
> - if (is_global_default_ctx) {
> - ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
> - if (ret) {
> - DRM_ERROR(
> - "Failed to pin and map ringbuffer %s: %d\n",
> - ring->name, ret);
> - goto error_ringbuf;
> - }
> + goto error_deref_obj;
> }
>
> ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
> if (ret) {
> DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
> - goto error;
> + goto error_ringbuf;
> }
>
> ctx->engine[ring->id].ringbuf = ringbuf;
> ctx->engine[ring->id].state = ctx_obj;
>
> - if (ctx == ring->default_context)
> - lrc_setup_hardware_status_page(ring, ctx_obj);
> - else if (ring->id == RCS && !ctx->rcs_initialized) {
> - if (ring->init_context) {
> - struct drm_i915_gem_request *req;
> -
> - ret = i915_gem_request_alloc(ring, ctx, &req);
> - if (ret)
> - return ret;
> + if (ctx != ring->default_context && ring->init_context) {
> + struct drm_i915_gem_request *req;
>
> - ret = ring->init_context(req);
> - if (ret) {
> - DRM_ERROR("ring init context: %d\n", ret);
> - i915_gem_request_cancel(req);
> - ctx->engine[ring->id].ringbuf = NULL;
> - ctx->engine[ring->id].state = NULL;
> - goto error;
> - }
> -
> - i915_add_request_no_flush(req);
> + ret = i915_gem_request_alloc(ring,
> + ctx, &req);
> + if (ret) {
> + DRM_ERROR("ring create req: %d\n",
> + ret);
> + i915_gem_request_cancel(req);
> + goto error_ringbuf;
It looks like I've hit a bug here, when i915_gem_request_alloc
fails with ENOSPC it goes on to call i915_gem_request_cancel
with req == NULL.
Provoked by flink-and-exit-vma-leak IGT, which
is not in master yet, but I posted it recently as
"[PATCH i-g-t] gem_ppgtt: Test VMA leak on context destruction".
[ 243.925442] [drm:intel_lr_context_deferred_alloc [i915]] *ERROR* ring create req: -28
[ 243.934659] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
[ 243.943792] IP: [<ffffffffa01ae96c>] i915_gem_request_cancel+0xc/0x70 [i915]
[ 243.952065] PGD ba6e5067 PUD ba1db067 PMD 0
[ 243.957139] Oops: 0000 [#1] PREEMPT SMP
[ 243.961861] Modules linked in: coretemp i915 asix libphy drm_kms_helper usbnet mii drm fb_sys_fops sysimgblt sysfillrect lpc_ich syscopyarea mfd_core i2c_algo_bit video i2c_hid gpio_lynxpoint hid_generic nls_iso8859_1 acpi_pad usbhid hid e1000e ptp ahci libahci pps_core
[ 243.989963] CPU: 1 PID: 1246 Comm: gem_ppgtt Tainted: G U W 4.3.0-rc2-150910+ #6
[ 243.999777] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
[ 244.014446] task: ffff8800ba935000 ti: ffff8800b8510000 task.ti: ffff8800b8510000
[ 244.023350] RIP: 0010:[<ffffffffa01ae96c>] [<ffffffffa01ae96c>] i915_gem_request_cancel+0xc/0x70 [i915]
[ 244.034556] RSP: 0018:ffff8800b8513af8 EFLAGS: 00010292
[ 244.040993] RAX: 0000000000000049 RBX: 0000000000000000 RCX: 0000000000000000
[ 244.049526] RDX: ffff88013f48e4c0 RSI: ffff88013f48cb78 RDI: 0000000000000000
[ 244.058069] RBP: ffff8800b8513b08 R08: 0000000000000001 R09: 0000000000000000
[ 244.066660] R10: 0000000000000001 R11: 0000000000000001 R12: ffff880010335f80
[ 244.075208] R13: ffff880010317800 R14: 00000000ffffffe4 R15: ffff88001038a000
[ 244.083791] FS: 00007f2c6de3a740(0000) GS:ffff88013f480000(0000) knlGS:0000000000000000
[ 244.093448] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 244.100496] CR2: 0000000000000030 CR3: 00000000ba341000 CR4: 00000000003406e0
[ 244.109128] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 244.117759] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 244.126363] Stack:
[ 244.129182] ffff880010322900 ffff8800b8a42f28 ffff8800b8513b78 ffffffffa01bd0ee
[ 244.138184] ffff8800ad715000 ffffea0002b5c540 ffff8800b9761000 ffff8800101b9e80
[ 244.147191] 00000000fffffffa 0000000000000000 ffff8800b8513b78 ffff8800b8513db0
[ 244.156220] Call Trace:
[ 244.159592] [<ffffffffa01bd0ee>] intel_lr_context_deferred_alloc+0x67e/0x8e0 [i915]
[ 244.169002] [<ffffffffa01a2ff8>] i915_gem_do_execbuffer.isra.30+0x1128/0x11a0 [i915]
[ 244.178488] [<ffffffff8108f45d>] ? __lock_acquire+0xabd/0x1e70
[ 244.185845] [<ffffffffa01a423c>] i915_gem_execbuffer2+0xdc/0x290 [i915]
[ 244.194064] [<ffffffffa00e5d69>] drm_ioctl+0x2f9/0x540 [drm]
[ 244.201239] [<ffffffff810a51ed>] ? __call_rcu.constprop.66+0x8d/0x2a0
[ 244.209318] [<ffffffffa01a4160>] ? i915_gem_execbuffer+0x390/0x390 [i915]
[ 244.217795] [<ffffffff810a5472>] ? call_rcu+0x12/0x20
[ 244.224280] [<ffffffff81127692>] ? put_object+0x32/0x50
[ 244.230992] [<ffffffff811254c9>] ? kmem_cache_free+0xa9/0x190
[ 244.238276] [<ffffffff8113dea7>] do_vfs_ioctl+0x87/0x5b0
[ 244.245102] [<ffffffff8113aa0a>] ? putname+0x5a/0x60
[ 244.251538] [<ffffffff81149a14>] ? __fget_light+0x74/0xa0
[ 244.258483] [<ffffffff8113e417>] SyS_ioctl+0x47/0x80
[ 244.264933] [<ffffffff8156c317>] entry_SYSCALL_64_fastpath+0x12/0x6f
[ 244.272947] Code: 48 c7 c2 28 42 24 a0 be d7 09 00 00 48 c7 c7 10 40 24 a0 31 c0 e8 d5 01 ea e0 e9 b5 fe ff ff 55 48 89 e5 53 48 89 fb 48 83 ec 08 <48> 8b 7f 30 e8 2b 2d 01 00 48 8b 43 10 48 8b 40 10 8b 40 60 83
[ 244.295873] RIP [<ffffffffa01ae96c>] i915_gem_request_cancel+0xc/0x70 [i915]
[ 244.304789] RSP <ffff8800b8513af8>
[ 244.309535] CR2: 0000000000000030
[ 244.314119] ---[ end trace 37329e4c817ee12a ]---
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-09-21 10:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-11 11:53 [PATCH] drm/i915: Split alloc from init for lrc Nick Hoath
2015-09-11 13:09 ` Daniel, Thomas
2015-09-21 10:25 ` Tvrtko Ursulin [this message]
-- strict thread matches above, loose matches on Subject: below --
2015-09-10 13:32 Nick Hoath
2015-09-04 13:49 Nick Hoath
2015-09-09 13:54 ` Daniel, Thomas
2015-08-19 12:24 Nick Hoath
2015-08-19 12:37 ` Chris Wilson
2015-08-19 14:40 ` Nick Hoath
2015-08-28 13:25 ` shuang.he
2015-08-18 14:23 Nick Hoath
2015-08-18 14:31 ` Chris Wilson
2015-08-18 14:55 ` Nick Hoath
2015-08-18 15:06 ` Chris Wilson
2015-08-26 8:32 ` Daniel Vetter
2015-08-26 8:49 ` Chris Wilson
2015-08-28 8:12 ` shuang.he
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55FFDB2F.7020504@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=daniel.vetter@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=nicholas.hoath@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.