[PATCH v5 1/4] drm/i915: Avoid early GPU idling due to already pending idle work

[PATCH v5 1/4] drm/i915: Avoid early GPU idling due to already pending idle work

[Imre Deak]

Atm, in case an idle work handler is already pending but haven't yet
started to run, retiring a new request will not extend the idle period
as required, rather simply leaves the pending work to be scheduled at
the original expiration time. This may lead to idling the GPU too early.
Fix this by using the delayed-work scheduler alternative which makes
sure the handler's expiration time is extended in this case.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 79b0046..0b3b051 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -1019,7 +1019,7 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 		engine_retire_requests(engine);
 
 	if (!dev_priv->gt.active_requests)
-		queue_delayed_work(dev_priv->wq,
-				   &dev_priv->gt.idle_work,
-				   msecs_to_jiffies(100));
+		mod_delayed_work(dev_priv->wq,
+				 &dev_priv->gt.idle_work,
+				 msecs_to_jiffies(100));
 }
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 2/4] drm/i915: Avoid early GPU idling due to race with new request

[Imre Deak]

There is a small race where a new request can be submitted and retired
after the idle worker started to run which leads to idling the GPU too
early. Fix this by deferring the idling to the pending instance of the
worker.

This scenario was pointed out by Chris.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0dbf38c..36a16df 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2766,6 +2766,13 @@ i915_gem_idle_work_handler(struct work_struct *work)
 		goto out_rearm;
 	}
 
+	/*
+	 * New request retired after this work handler started, extend active
+	 * period until next instance of the work.
+	 */
+	if (work_pending(work))
+		goto out_unlock;
+
 	if (dev_priv->gt.active_requests)
 		goto out_unlock;
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 3/4] drm/i915: Make sure engines are idle during GPU idling in LR mode

[Imre Deak]

We assume that the GPU is idle once receiving the seqno via the last
request's user interrupt. In execlist mode the corresponding context
completed interrupt can be delayed though and until this latter
interrupt arrives we consider the request to be pending on the ELSP
submit port. This can cause a problem during system suspend where this
last request will be seen by the resume code as still pending. Such
pending requests are normally replayed after a GPU reset, but during
resume we reset both SW and HW tracking of the ring head/tail pointers,
so replaying the pending request with its stale tail pointer will leave
the ring in an inconsistent state. A subsequent request submission can
lead then to the GPU executing from uninitialized area in the ring
behind the above stale tail pointer.

Fix this by making sure any pending request on the ELSP port is
completed before suspending. I used a polling wait since the completion
time I measured was <1ms and since normally we only need to wait during
system suspend. GPU idling during runtime suspend is scheduled with a
delay (currently 50-100ms) after the retirement of the last request at
which point the context completed interrupt must have arrived already.

The chance of this bug was increased by

commit 1c777c5d1dcdf8fa0223fcff35fb387b5bb9517a
Author: Imre Deak <imre.deak@intel.com>
Date:   Wed Oct 12 17:46:37 2016 +0300

    drm/i915/hsw: Fix GPU hang during resume from S3-devices state

but it could happen even without the explicit GPU reset, since we
disable interrupts afterwards during the suspend sequence.

v2:
- Do an unlocked poll-wait first. (Chris)
v3-4:
- s/intel_lr_engines_idle/intel_execlists_idle/ and move
  i915.enable_execlists check to the new helper. (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98470
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  | 10 ++++++++++
 drivers/gpu/drm/i915/intel_lrc.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.h |  1 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 36a16df..44ecab0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2752,6 +2752,13 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	if (!READ_ONCE(dev_priv->gt.awake))
 		return;
 
+	/*
+	 * Wait for last execlists context complete, but bail out in case a
+	 * new request is submitted.
+	 */
+	wait_for(READ_ONCE(dev_priv->gt.active_requests) ||
+		 intel_execlists_idle(dev_priv), 10);
+
 	if (READ_ONCE(dev_priv->gt.active_requests))
 		return;
 
@@ -2776,6 +2783,9 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	if (dev_priv->gt.active_requests)
 		goto out_unlock;
 
+	if (wait_for(intel_execlists_idle(dev_priv), 10))
+		DRM_ERROR("Timeout waiting for engines to idle\n");
+
 	for_each_engine(engine, dev_priv, id)
 		i915_gem_batch_pool_fini(&engine->batch_pool);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fa3012c..dde04b764 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -522,6 +522,28 @@ static bool execlists_elsp_idle(struct intel_engine_cs *engine)
 	return !engine->execlist_port[0].request;
 }
 
+/**
+ * intel_execlists_idle() - Determine if all engine submission ports are idle
+ * @dev_priv: i915 device private
+ *
+ * Return true if there are no requests pending on any of the submission ports
+ * of any engines.
+ */
+bool intel_execlists_idle(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	if (!i915.enable_execlists)
+		return true;
+
+	for_each_engine(engine, dev_priv, id)
+		if (!execlists_elsp_idle(engine))
+			return false;
+
+	return true;
+}
+
 static bool execlists_elsp_ready(struct intel_engine_cs *engine)
 {
 	int port;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 4fed816..c1f5461 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -95,5 +95,6 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
 int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
 				    int enable_execlists);
 void intel_execlists_enable_submission(struct drm_i915_private *dev_priv);
+bool intel_execlists_idle(struct drm_i915_private *dev_priv);
 
 #endif /* _INTEL_LRC_H_ */
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 4/4] drm/i915: Add assert for no pending GPU requests during suspend/resume in LR mode

[Imre Deak]

During resume we will reset the SW/HW tracking for each ring head/tail
pointers and so are not prepared to replay any pending requests (as
opposed to GPU reset time). Add an assert for this both to the suspend
and the resume code.

v2:
- Check for ELSP port idle already during suspend and check !gt.awake
  during resume. (Chris)
v3:
- Move the !gt.awake check to i915_gem_resume().
v4:
- s/intel_lr_engines_idle/intel_execlists_idle/ (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 44ecab0..b6578d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4446,6 +4446,7 @@ int i915_gem_suspend(struct drm_device *dev)
 	 * reset the GPU back to its idle, low power state.
 	 */
 	WARN_ON(dev_priv->gt.awake);
+	WARN_ON(!intel_execlists_idle(dev_priv));
 
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
@@ -4482,6 +4483,8 @@ void i915_gem_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
+	WARN_ON(dev_priv->gt.awake);
+
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev);
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/4] drm/i915: Avoid early GPU idling due to already pending idle work

[Chris Wilson]

On Mon, Nov 07, 2016 at 11:20:02AM +0200, Imre Deak wrote:
> Atm, in case an idle work handler is already pending but haven't yet
> started to run, retiring a new request will not extend the idle period
> as required, rather simply leaves the pending work to be scheduled at
> the original expiration time. This may lead to idling the GPU too early.
> Fix this by using the delayed-work scheduler alternative which makes
> sure the handler's expiration time is extended in this case.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Series is
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [v5,1/4] drm/i915: Avoid early GPU idling due to already pending idle work

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/4] drm/i915: Avoid early GPU idling due to already pending idle work
URL   : https://patchwork.freedesktop.org/series/14917/
State : warning

== Summary ==

Series 14917v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14917/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:208  dwarn:1   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 

5385d13fdacbf8fbb345ecdc533ee407fd86e2f2 drm-intel-nightly: 2016y-11m-07d-09h-18m-42s UTC integration manifest
57c4449 drm/i915: Add assert for no pending GPU requests during suspend/resume in LR mode
bf04cab drm/i915: Make sure engines are idle during GPU idling in LR mode
de31de0 drm/i915: Avoid early GPU idling due to race with new request
f7fce15 drm/i915: Avoid early GPU idling due to already pending idle work

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2916/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for series starting with [v5,1/4] drm/i915: Avoid early GPU idling due to already pending idle work

[Imre Deak]

On ma, 2016-11-07 at 10:15 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v5,1/4] drm/i915: Avoid early GPU idling due to already pending idle work
> URL   : https://patchwork.freedesktop.org/series/14917/
> State : warning
> 
> == Summary ==
> 
> Series 14917v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/14917/revisions/1/mbox/
> 
> Test kms_flip:
>         Subgroup basic-plain-flip:
>                 pass       -> DMESG-WARN (fi-snb-2520m)

[  384.622585] [drm:intel_hdmi_detect [i915]] [CONNECTOR:46:HDMI-A-1
[  384.650610] [drm:gmbus_xfer [i915]] GMBUS [i915 gmbus dpb] NAK for addr: 0040 w(1)
[  384.650659] [drm:gmbus_xfer [i915]] GMBUS [i915 gmbus dpb] NAK on first message, retry
[  384.650956] [drm:gmbus_xfer [i915]] GMBUS [i915 gmbus dpb] NAK for addr: 0040 w(1)
[  384.650968] [drm:drm_dp_dual_mode_detect] DP dual mode HDMI ID:  (err -6)
[  384.650979] [drm:drm_detect_monitor_audio] Monitor has basic audio support
[  384.651016] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 162

Looks like a recent issue, happened on the same machine also in
https://intel-gfx-ci.01.org/CI/Patchwork_2915/
kms_force_connector_basic@prune-stale-modes.

The monitor has been the same for quite some time without this error. I
opened a new bug:
https://bugs.freedesktop.org/show_bug.cgi?id=98625

Thanks for the review pushed the patchset to -dinq.

--Imre

> fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
> fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
> fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
> fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
> fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
> fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
> fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
> fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
> fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
> fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-2520m     total:241  pass:208  dwarn:1   dfail:0   fail:0   skip:32 
> fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
> 
> 5385d13fdacbf8fbb345ecdc533ee407fd86e2f2 drm-intel-nightly: 2016y-11m-07d-09h-18m-42s UTC integration manifest
> 57c4449 drm/i915: Add assert for no pending GPU requests during suspend/resume in LR mode
> bf04cab drm/i915: Make sure engines are idle during GPU idling in LR mode
> de31de0 drm/i915: Avoid early GPU idling due to race with new request
> f7fce15 drm/i915: Avoid early GPU idling due to already pending idle work
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2916/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx