[Intel-gfx] [PATCH] drm/i915: Split alloc from init for lrc
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Sep 21 03:25:51 PDT 2015
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 at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: John Harrison <john.c.harrison at intel.com>
> Cc: David Gordon <david.s.gordon at intel.com>
> Cc: Thomas Daniel <thomas.daniel at 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
More information about the Intel-gfx
mailing list