[PATCH v4 1/3] drm/i915: Avoid early GPU idling due to race with new request

[PATCH v4 1/3] 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 v4 2/3] 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 v4 3/3] 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

✗ Fi.CI.BAT: failure for series starting with [v4,1/3] drm/i915: Avoid early GPU idling due to race with new request

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/3] drm/i915: Avoid early GPU idling due to race with new request
URL   : https://patchwork.freedesktop.org/series/14877/
State : failure

== Summary ==

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

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-skl-6770hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-6770hq)
Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-hsw-4770r)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (fi-skl-6770hq)

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-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
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:224  dwarn:2   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 

49a651a2e66ef603995f88a470d0986c2ef8b5b8 drm-intel-nightly: 2016y-11m-04d-18h-04m-36s UTC integration manifest
84f0faf drm/i915: Add assert for no pending GPU requests during suspend/resume in LR mode
5b4c71f drm/i915: Make sure engines are idle during GPU idling in LR mode
0cb2a32 drm/i915: Avoid early GPU idling due to race with new request

== Logs ==

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

Re: [PATCH v4 1/3] drm/i915: Avoid early GPU idling due to race with new request

[Chris Wilson]

On Sat, Nov 05, 2016 at 10:35:59AM +0200, Imre Deak wrote:
> 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;

Ok, it took some digging around inside kernel/workqueue.c to come to
agreement that this works.

WORK_STRUCT_PENDING_BIT is set when we queue the work and released just
prior to execution of the work->func. A race on active_requests before
we take the struct_mutex will leave this set.

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

For bonus points

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index cfda095..5d349e0 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -1146,7 +1146,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));
 }

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

Re: [PATCH v4 2/3] drm/i915: Make sure engines are idle during GPU idling in LR mode

[Chris Wilson]

On Sat, Nov 05, 2016 at 10:36:00AM +0200, Imre Deak wrote:
> 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>
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

Re: [PATCH v4 1/3] drm/i915: Avoid early GPU idling due to race with new request

[Imre Deak]

On la, 2016-11-05 at 18:35 +0000, Chris Wilson wrote:
> On Sat, Nov 05, 2016 at 10:35:59AM +0200, Imre Deak wrote:
> > 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;
> 
> Ok, it took some digging around inside kernel/workqueue.c to come to
> agreement that this works.
> 
> WORK_STRUCT_PENDING_BIT is set when we queue the work and released just
> prior to execution of the work->func. A race on active_requests before
> we take the struct_mutex will leave this set.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> For bonus points
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index cfda095..5d349e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -1146,7 +1146,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));
>  }

Yep, without this we could still do early idling and that scenario has
a bigger likelihood:/ But I think it's still a separate patch, I can
follow up with it. Good to know the difference between the above two
helpers!

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