Re: [PATCH] drm/i915: Split alloc from init for lrc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: John Harrison <john.c.harrison@xxxxxxxxx>
> Cc: David Gordon <david.s.gordon@xxxxxxxxx>
> Cc: Thomas Daniel <thomas.daniel@xxxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux