Re: [PATCH 1/2] drm/i915: Push the wakeref->count deferral to the backend

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

 



Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:

> If the backend wishes to defer the wakeref parking, make it responsible
> for unlocking the wakeref (i.e. bumping the counter). This allows it to
> time the unlock much more carefully in case it happens to needs the
> wakeref to be active during its deferral.
>
> For instance, during engine parking we may choose to emit an idle
> barrier (a request). To do so, we borrow the engine->kernel_context
> timeline and to ensure exclusive access we keep the
> engine->wakeref.count as 0. However, to submit that request to HW may
> require a intel_engine_pm_get() (e.g. to keep the submission tasklet
> alive) and before we allow that we have to rewake our wakeref to avoid a
> recursive deadlock.
>
> <4> [257.742916] IRQs not enabled as expected
> <4> [257.742930] WARNING: CPU: 0 PID: 0 at kernel/softirq.c:169 __local_bh_enable_ip+0xa9/0x100
> <4> [257.742936] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 btusb btrtl btbcm btintel snd_hda_intel snd_intel_nhlt bluetooth snd_hda_codec coretemp snd_hwdep crct10dif_pclmul snd_hda_core crc32_pclmul ecdh_generic ecc ghash_clmulni_intel snd_pcm r8169 realtek lpc_ich prime_numbers i2c_hid
> <4> [257.742991] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G     U  W         5.3.0-rc3-g5d0a06cd532c-drmtip_340+ #1
> <4> [257.742998] Hardware name: GIGABYTE GB-BXBT-1900/MZBAYAB-00, BIOS F6 02/17/2015
> <4> [257.743008] RIP: 0010:__local_bh_enable_ip+0xa9/0x100
> <4> [257.743017] Code: 37 5b 5d c3 8b 80 50 08 00 00 85 c0 75 a9 80 3d 0b be 25 01 00 75 a0 48 c7 c7 f3 0c 06 ac c6 05 fb bd 25 01 01 e8 77 84 ff ff <0f> 0b eb 89 48 89 ef e8 3b 41 06 00 eb 98 e8 e4 5c f4 ff 5b 5d c3
> <4> [257.743025] RSP: 0018:ffffa78600003cb8 EFLAGS: 00010086
> <4> [257.743035] RAX: 0000000000000000 RBX: 0000000000000200 RCX: 0000000000010302
> <4> [257.743042] RDX: 0000000080010302 RSI: 0000000000000000 RDI: 00000000ffffffff
> <4> [257.743050] RBP: ffffffffc0494bb3 R08: 0000000000000000 R09: 0000000000000001
> <4> [257.743058] R10: 0000000014c8f0e9 R11: 00000000fee2ff8e R12: ffffa23ba8c38008
> <4> [257.743065] R13: ffffa23bacc579c0 R14: ffffa23bb7db0f60 R15: ffffa23b9cc8c430
> <4> [257.743074] FS:  0000000000000000(0000) GS:ffffa23bbba00000(0000) knlGS:0000000000000000
> <4> [257.743082] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [257.743089] CR2: 00007fe477b20778 CR3: 000000011f72a000 CR4: 00000000001006f0
> <4> [257.743096] Call Trace:
> <4> [257.743104]  <IRQ>
> <4> [257.743265]  __i915_request_commit+0x240/0x5d0 [i915]
> <4> [257.743427]  ? __i915_request_create+0x228/0x4c0 [i915]
> <4> [257.743584]  __engine_park+0x64/0x250 [i915]
> <4> [257.743730]  ____intel_wakeref_put_last+0x1c/0x70 [i915]
> <4> [257.743878]  i915_sample+0x2ee/0x310 [i915]
> <4> [257.744030]  ? i915_pmu_cpu_offline+0xb0/0xb0 [i915]
> <4> [257.744040]  __hrtimer_run_queues+0x11e/0x4b0
> <4> [257.744068]  hrtimer_interrupt+0xea/0x250
> <4> [257.744079]  ? lockdep_hardirqs_off+0x79/0xd0
> <4> [257.744101]  smp_apic_timer_interrupt+0x96/0x280
> <4> [257.744114]  apic_timer_interrupt+0xf/0x20
> <4> [257.744125] RIP: 0010:__do_softirq+0xb3/0x4ae
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111378
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_pm.c |  8 ++-
>  drivers/gpu/drm/i915/i915_request.c       | 66 ++++++++++++-----------
>  drivers/gpu/drm/i915/i915_request.h       |  2 +
>  drivers/gpu/drm/i915/i915_scheduler.c     |  3 +-
>  drivers/gpu/drm/i915/intel_wakeref.c      |  4 +-
>  drivers/gpu/drm/i915/intel_wakeref.h      | 11 ++++
>  6 files changed, 56 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 6b15e3335dd6..ad37c9808c1f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -68,9 +68,13 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
>  
>  	/* Check again on the next retirement. */
>  	engine->wakeref_serial = engine->serial + 1;
> -
>  	i915_request_add_active_barriers(rq);
> +
> +	rq->sched.attr.priority = INT_MAX; /* Preemption barrier */
> +
>  	__i915_request_commit(rq);
> +	__intel_wakeref_defer_park(&engine->wakeref);
> +	__i915_request_queue(rq, NULL);
>  
>  	return false;
>  }
> @@ -98,7 +102,7 @@ static int __engine_park(struct intel_wakeref *wf)
>  	intel_engine_pool_park(&engine->pool);
>  
>  	/* Must be reset upon idling, or we may miss the busy wakeup. */
> -	GEM_BUG_ON(engine->execlists.queue_priority_hint != INT_MIN);
> +	engine->execlists.queue_priority_hint = INT_MIN;

To speed up the turnaround, we went through this on irc and this
was the only thing that was not agreed upon so with it removed,

Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>

>  
>  	if (engine->park)
>  		engine->park(engine);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 0ea1136d453e..4021334dd2c5 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1186,6 +1186,12 @@ struct i915_request *__i915_request_commit(struct i915_request *rq)
>  		list_add(&ring->active_link, &rq->i915->gt.active_rings);
>  	rq->emitted_jiffies = jiffies;
>  
> +	return prev;
> +}
> +
> +void __i915_request_queue(struct i915_request *rq,
> +			  const struct i915_sched_attr *attr)
> +{
>  	/*
>  	 * Let the backend know a new request has arrived that may need
>  	 * to adjust the existing execution schedule due to a high priority
> @@ -1199,43 +1205,15 @@ struct i915_request *__i915_request_commit(struct i915_request *rq)
>  	 */
>  	local_bh_disable();
>  	i915_sw_fence_commit(&rq->semaphore);
> -	if (engine->schedule) {
> -		struct i915_sched_attr attr = rq->gem_context->sched;
> -
> -		/*
> -		 * Boost actual workloads past semaphores!
> -		 *
> -		 * With semaphores we spin on one engine waiting for another,
> -		 * simply to reduce the latency of starting our work when
> -		 * the signaler completes. However, if there is any other
> -		 * work that we could be doing on this engine instead, that
> -		 * is better utilisation and will reduce the overall duration
> -		 * of the current work. To avoid PI boosting a semaphore
> -		 * far in the distance past over useful work, we keep a history
> -		 * of any semaphore use along our dependency chain.
> -		 */
> -		if (!(rq->sched.flags & I915_SCHED_HAS_SEMAPHORE_CHAIN))
> -			attr.priority |= I915_PRIORITY_NOSEMAPHORE;
> -
> -		/*
> -		 * Boost priorities to new clients (new request flows).
> -		 *
> -		 * Allow interactive/synchronous clients to jump ahead of
> -		 * the bulk clients. (FQ_CODEL)
> -		 */
> -		if (list_empty(&rq->sched.signalers_list))
> -			attr.priority |= I915_PRIORITY_WAIT;
> -
> -		engine->schedule(rq, &attr);
> -	}
> +	if (attr && rq->engine->schedule)
> +		rq->engine->schedule(rq, attr);
>  	i915_sw_fence_commit(&rq->submit);
>  	local_bh_enable(); /* Kick the execlists tasklet if just scheduled */
> -
> -	return prev;
>  }
>  
>  void i915_request_add(struct i915_request *rq)
>  {
> +	struct i915_sched_attr attr = rq->gem_context->sched;
>  	struct i915_request *prev;
>  
>  	lockdep_assert_held(&rq->timeline->mutex);
> @@ -1245,6 +1223,32 @@ void i915_request_add(struct i915_request *rq)
>  
>  	prev = __i915_request_commit(rq);
>  
> +	/*
> +	 * Boost actual workloads past semaphores!
> +	 *
> +	 * With semaphores we spin on one engine waiting for another,
> +	 * simply to reduce the latency of starting our work when
> +	 * the signaler completes. However, if there is any other
> +	 * work that we could be doing on this engine instead, that
> +	 * is better utilisation and will reduce the overall duration
> +	 * of the current work. To avoid PI boosting a semaphore
> +	 * far in the distance past over useful work, we keep a history
> +	 * of any semaphore use along our dependency chain.
> +	 */
> +	if (!(rq->sched.flags & I915_SCHED_HAS_SEMAPHORE_CHAIN))
> +		attr.priority |= I915_PRIORITY_NOSEMAPHORE;
> +
> +	/*
> +	 * Boost priorities to new clients (new request flows).
> +	 *
> +	 * Allow interactive/synchronous clients to jump ahead of
> +	 * the bulk clients. (FQ_CODEL)
> +	 */
> +	if (list_empty(&rq->sched.signalers_list))
> +		attr.priority |= I915_PRIORITY_WAIT;
> +
> +	__i915_request_queue(rq, &attr);
> +
>  	/*
>  	 * In typical scenarios, we do not expect the previous request on
>  	 * the timeline to be still tracked by timeline->last_request if it
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 313df3c37158..fec1d5f17c94 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -251,6 +251,8 @@ struct i915_request * __must_check
>  i915_request_create(struct intel_context *ce);
>  
>  struct i915_request *__i915_request_commit(struct i915_request *request);
> +void __i915_request_queue(struct i915_request *rq,
> +			  const struct i915_sched_attr *attr);
>  
>  void i915_request_retire_upto(struct i915_request *rq);
>  
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 0bd452e851d8..7b84ebca2901 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -349,8 +349,7 @@ void i915_schedule_bump_priority(struct i915_request *rq, unsigned int bump)
>  	unsigned long flags;
>  
>  	GEM_BUG_ON(bump & ~I915_PRIORITY_MASK);
> -
> -	if (READ_ONCE(rq->sched.attr.priority) == I915_PRIORITY_INVALID)
> +	if (READ_ONCE(rq->sched.attr.priority) & bump)
>  		return;
>  
>  	spin_lock_irqsave(&schedule_lock, flags);
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
> index d4443e81c1c8..868cc78048d0 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -57,12 +57,10 @@ static void ____intel_wakeref_put_last(struct intel_wakeref *wf)
>  	if (!atomic_dec_and_test(&wf->count))
>  		goto unlock;
>  
> +	/* ops->put() must reschedule its own release on error/deferral */
>  	if (likely(!wf->ops->put(wf))) {
>  		rpm_put(wf);
>  		wake_up_var(&wf->wakeref);
> -	} else {
> -		/* ops->put() must schedule its own release on deferral */
> -		atomic_set_release(&wf->count, 1);
>  	}
>  
>  unlock:
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h
> index 535a3a12864b..5f0c972a80fb 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.h
> +++ b/drivers/gpu/drm/i915/intel_wakeref.h
> @@ -163,6 +163,17 @@ intel_wakeref_is_active(const struct intel_wakeref *wf)
>  	return READ_ONCE(wf->wakeref);
>  }
>  
> +/**
> + * __intel_wakeref_defer_park: Defer the current park callback
> + * @wf: the wakeref
> + */
> +static inline void
> +__intel_wakeref_defer_park(struct intel_wakeref *wf)
> +{
> +	INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count));
> +	atomic_set_release(&wf->count, 1);
> +}
> +
>  /**
>   * intel_wakeref_wait_for_idle: Wait until the wakeref is idle
>   * @wf: the wakeref
> -- 
> 2.23.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux